fix(datasource-active-record): skip unrepresentable has_many/has_one :through relations and deprecate silent identity joins (#370)

[matthv]

What

ThroughReflection#join_foreign_key delegates to the source reflection. Only belongs_to overrides it to return a real FK column; any other macro (has_one, has_many, including a nested :through) falls back to the base implementation, AssociationReflection#active_record_primary_key — the through model's own primary key column (honoring a custom primary_key:, and possibly composite), not the custom foreign_key: option on the association.

This produces two distinct outcomes depending on the shape:

• Single-hop non-belongs_to source (the common case, e.g. has_many :things, through: :joins, source: :thing where Join#thing is a has_one/has_many): the fallback resolves to a column that trivially exists on the through table (usually id), so nothing looks broken. The relation gets published with a ManyToManySchema silently encoding a through.id = foreign.id join — wrong data shown in the UI, not a missing-field log line.
• Nested :through (a :through sourced from another :through, i.e. Spurious "Field '<col>' not found in schema of collection '<Model>'" WARN during schema generation for has_many :through walking a has_one with custom foreign_key #370's actual shape): the fallback resolves all the way down to the innermost real FK name (e.g. custom_child_id), which genuinely doesn't exist as a column on the immediate through collection. This is the shape that produces Spurious "Field '<col>' not found in schema of collection '<Model>'" WARN during schema generation for has_many :through walking a has_one with custom foreign_key #370's reported symptom.

Fix

Two new guard methods in Collection:

• foreign_key_missing_from_through? — true only when the resolved join_foreign_key (which can itself be an Array on a composite key) names a column genuinely absent from the through collection. Only the nested-:through case above hits this.
• valid_many_to_many_source? — true only for a belongs_to source.

Applied in add_many_to_many_field (shared by both the has_one and has_many :through branches):

• Genuinely missing column → skip the association, log a warning. This is the only case dropped from the schema, and it's narrowly scoped to nested :through chains.
• Column exists but source isn't belongs_to (the common single-hop case) → publish the exact same ManyToManySchema as before — verified field-for-field identical to main's output for this shape — plus a deprecation warning that this identity join will be removed in a future major version.

Not a breaking change: the published .forestadmin-schema.json is unchanged from main for every relation that was previously published, including composite-key belongs_to-sourced ones (a regression introduced in an earlier revision of this branch, since fixed — see history below). The only behavioral delta is that a small, genuinely-broken subset of nested :through relations that were silently corrupting data now get dropped with a warning instead.

Revision history on this PR (context for reviewers)

This PR changed shape several times across review rounds:

1. Original fix patched PublicationCollectionDecorator#published? to resolve foreign_key/origin_key against either candidate collection. Macroscope flagged two real risks in that approach (first-match resolution across collections on a name collision, and a SystemStackError on cyclic relations). Investigating those findings turned up the actual mechanism above, so the fix moved upstream to the AR datasource; the customizer change was fully reverted (zero diff vs main).
2. The first AR-side fix unconditionally dropped every non-belongs_to-sourced :through relation. A reviewer found this changed the published schema far more widely than Spurious "Field '<col>' not found in schema of collection '<Model>'" WARN during schema generation for has_many :through walking a has_one with custom foreign_key #370 describes — plain has_many :through sourced from a has_one/has_many is a common Rails idiom, silently dropped with no test coverage for that shape.
3. Redesigned into the bucket A/B split described above, so the published schema stays byte-identical to main in the common case.
4. Our own follow-up review pass found foreign_key_missing_from_through? didn't handle composite/array foreign keys, which reintroduced the same "drops a valid relation" regression from (2) through a different door (a belongs_to-sourced relation with a composite foreign_key: was misclassified as missing). Fixed, along with two comments and a warning message ("...by their own primary keys...") that were factually inaccurate for custom-primary_key: and composite-key shapes.

Also: fixtures were rebuilt once after a broken Rails-inflection fixture (Parent/Child/Leaf — "leaf".pluralize is "leafs", not "leaves") made an earlier regression test vacuous.

Relationship to #370

The nested-:through case above reproduces #370's root cause (a join_foreign_key resolving to a column absent from the immediate through collection) and the same failure mode, though not the exact reported warning text. Left as "Related to #370" rather than "Fixes #370" since the reporter's exact minimal repro wasn't independently confirmed.

Out of scope

Non-polymorphic has_one :through (the OneToOneSchema path, e.g. Supplier#account_history) has the same class of bug but isn't guarded here — it never reaches this code at all. Tracked as #379 / PRD-1068.

agent-nodejs was checked and is not affected: Sequelize's belongsToMany always owns both join columns directly on the through model, with no delegation to a third association and no primary-key fallback.

How tested

Fixtures, each verified to fail against unfixed collection.rb (checked out from origin/main) and pass with the fix restored:

• Parent/Kid/Detail — has_many :through, polymorphic, bucket B (published + deprecated)
• Solo/Kid/Detail — has_one :through, polymorphic, bucket B
• Box/Slot/Tag — has_many :through, non-polymorphic, bucket B
• NestedThroughProbe (Category → Car → Check, nested :through) — bucket A (skipped + warned)
• A composite-foreign-key belongs_to-sourced fixture — confirms the fix doesn't regress the valid path

Bucket-B tests assert full field equality (have_attributes on all 10 ManyToManySchema attributes), not just presence/class, to lock in the "schema unchanged" claim. A negative test confirms a normal belongs_to-sourced relation (User#projects) emits no deprecation warning.

forest_admin_datasource_active_record: 197 examples, 0 failures. forest_admin_datasource_customizer: unchanged from main. Rubocop clean on all changed/new files.

🤖 Generated with Claude Code

Note

Changes since #378 opened

• Modified ForestAdminDatasourceActiveRecord.Collection.warn_deprecated_identity_join to format composite foreign keys as comma-separated strings in deprecation warning messages [16eaf2e]
• Expanded test coverage in collection_spec for composite-keys many-to-many schema validation and deprecation warning formatting [16eaf2e]
• Expanded validation in ForestAdminDatasourceActiveRecord::Collection.unrepresentable_many_to_many? to check three key/collection pairs and detect composite keys [76384ef]
• Changed warning behavior for many-to-many associations from deprecation warnings to diagnostic warnings [76384ef]
• Updated test expectations in collection_spec.rb to verify skipping of unrepresentable associations and new warning messages [76384ef]

[qltysh]

3 new issues

Tool	Category	Rule	Count
qlty	Structure	Function with many parameters (count = 4): build_many_to_many_field	2
qlty	Structure	Deeply nested control flow (level = 4)	1

[qltysh]

Coverage Impact

⬆️ Merging this pull request will increase total coverage on main by 0.1%.

Modified Files with Diff Coverage (1)

Rating	File	% Diff	Uncovered Line #s
	...record/lib/forest_admin_datasource_active_record/collection.rb	100.0%
	Total	100.0%

🚦 See full report on Qlty Cloud »

🛟 Help

• Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

• Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

• File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

  • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

[christophebrun-forest]

Reviewed and reproduced on 849e64e. The two issues Macroscope already flagged (schema re-entrancy / SystemStackError, and first-match key resolution) are real — I confirmed both empirically. Two further points on top of those, one blocking.

---

1. 🔴 Blocking: this makes schema:generate crash, where today it only logs

Issue #370 is explicit that the outcome is cosmetic:

The schema is still generated correctly; only the log is wrong.

Publishing the relation changes that. GeneratorCollection.build_fields iterates the published schema, so the relation now reaches GeneratorField.build_many_to_many_schema:

# forest_admin_agent/utils/schema/generator_field.rb:80-91
through_schema = collection.datasource.get_collection(relation.through_collection)
foreign_schema = through_schema.schema[:fields][relation.foreign_key]   # nil in this exact shape
origin_key     = through_schema.schema[:fields][relation.origin_key]
...
isReadOnly: origin_key.is_read_only || foreign_schema.is_read_only      # NoMethodError

Reproduced on this branch using the very fixture added in this PR's spec (parent -> child -> leaf, custom_child_id on leaf):

published? => true
NoMethodError: undefined method 'is_read_only' for nil
  .../forest_admin_agent/lib/forest_admin_agent/utils/schema/generator_field.rb:91:in 'build_many_to_many_schema'
  .../forest_admin_agent/lib/forest_admin_agent/utils/schema/generator_field.rb:196:in 'build_relation_schema'

On main the same fixture yields published? => false, the field is filtered out of the refined schema, and generation completes — matching what the issue reports.

So rails forest_admin:schema:generate — the exact command in #370 — goes from "spurious WARN, schema generated successfully" to a hard failure. That is a strictly worse outcome for the reporter.

The new spec only asserts published? == true; it never asserts the published relation is usable. An assertion through GeneratorField.build_schema on the same fixture would have caught this straight away.

2. 🟠 The root cause is in the Active Record datasource, not the publication decorator

ManyToManySchema's contract across the whole toolkit is that foreign_key is a column on the through collection. Four independent call sites depend on it:

Site	What it does with foreign_key
toolkit/utils/collection.rb:103 get_through_target	scans through's fields for a ManyToOne with that fk -> returns nil here
toolkit/components/query/filter_factory.rb:92-105	through_collection.list(..., Projection.new([relation.foreign_key])) -> projects a column the through table does not have
agent/routes/resources/related/associate_related.rb:74-79	through_collection.create({relation.foreign_key => ...}) -> writes to a non-existent column
agent/utils/schema/generator_field.rb:80-91	the crash in point 1

Meanwhile forest_admin_datasource_active_record/collection.rb:190-199 emits, for a has_many :through whose source reflection is a has_one:

foreign_key: association.join_foreign_key,   # => 'custom_child_id', a column on `leaves`
through_collection: format_model_name(through_reflection.klass.name),  # => 'Child'

That ManyToManySchema is simply invalid — the join direction is inverted, and the toolkit's ManyToMany model cannot express a 1:N -> 1:1 chain. Publication was accidentally the thing keeping it out of the UI; relaxing publication just lets a broken relation through to the front-end and to the four call sites above.

Suggested direction: guard at the emission site instead. Only emit a ManyToManySchema when the source reflection is a belongs_to (i.e. the FK genuinely lives on the through table), and skip the association otherwise. That removes the WARN at its source, keeps nothing broken published, and needs no change in the publication decorator. The same applies to the has_one :through branch at collection.rb:105-115.

---

Smaller notes

• origin_key gets the same candidate list as foreign_key. No AR shape I can find puts origin_key on the foreign collection, and the new spec does not exercise it (owner_id is on child, the through collection). Widening it doubles the blast radius of the first-match issue Macroscope flagged, for no benefit — worth passing candidates per key.
• The fallback diagnostic still names the wrong collection. The second new spec pins "Field 'custom_child_id' not found in schema of collection 'child'" — which is the misleading message Spurious "Field '<col>' not found in schema of collection '<Model>'" WARN during schema generation for has_many :through walking a has_one with custom foreign_key #370 is about. If the key is missing from both candidates, the message should say so: "... not found in schemas of collections 'child', 'leaf'".
• relation_key_published? is public (the class has no private section). It is an internal helper; worth marking private.
• The two qlty comments (return-statements on published?, boolean-logic on L93) are pre-existing and only surfaced because the lines moved. Extracting the ManyToMany branch into a many_to_many_published?(field) helper would silence both and read better. Optional.

---

If the AR-side guard is too large for this PR, the minimal mitigation for the two Macroscope findings is to test declaration against the undecorated schema — which is exactly what published? itself does on line 64, and for the same reason:

return collection.published?(key) if collection.child_collection.schema[:fields].key?(key)

I verified that this resolves both the SystemStackError and the hidden-key-shadowing case, keeps both new specs green, leaves the customizer suite at 701/701, and is rubocop-clean. It does not address point 1 — that one needs the relation to stop being published, or build_many_to_many_schema to stop assuming the key is on the through collection.

[christophebrun-forest]

Need fixes , agree with macroscopp and 2 claude's comments

[matthv]

Pivoted based on the feedback above — thanks for the thorough repro on both.

Both of Macroscope's findings were real (confirmed independently): relation_key_published? could resolve the wrong collection on a name collision, and it risked a SystemStackError on cyclic relations by calling the decorated .schema instead of the undecorated one. And the blocking point stands — publishing this shape reaches GeneratorField.build_many_to_many_schema, which makes the same on-through-collection assumption and crashes.

Went with the direction you suggested: guard at the emission site instead of guessing in the publication decorator.

• Reverted publication_collection_decorator.rb and its spec entirely — back to exactly what's on main. This removes both Macroscope findings (the code they apply to no longer exists) and the two qlty findings on the same lines.
• New guard in forest_admin_datasource_active_record/collection.rb: foreign_key_on_through_collection? checks association.source_reflection.belongs_to? before emitting a ManyToManySchema, in both the has_one and has_many :through branches. When it's not a belongs_to (this shape), the association is skipped with a warning instead of published — so it never reaches build_many_to_many_schema at all.
• New Parent/Child/Leaf fixture in the dummy app, mirroring Spurious "Field '<col>' not found in schema of collection '<Model>'" WARN during schema generation for has_many :through walking a has_one with custom foreign_key #370 exactly, confirms the field is simply absent from the schema and nothing raises.

forest_admin_datasource_active_record 188/188, forest_admin_datasource_customizer 700/700 (unchanged from main, confirming the revert is clean), rubocop clean.

[christophebrun-forest]

Reviewed by running the branch locally (193 examples / 0 failures, rubocop clean) and probing reflection behaviour against main.

The diagnosis is right and I confirmed the mechanism. ThroughReflection#join_foreign_key is a plain delegate ... to: :source_reflection (activerecord reflection.rb:986) and only BelongsToReflection overrides it; everything else inherits active_record_primary_key. The toolkit genuinely cannot consume that: Utils::Collection.get_through_target looks for a ManyToOne on the through collection whose foreign_key matches, finds none, and FilterFactory.make_foreign_filter then falls back to foreign_key_target IN (through.id values) — returning rows whose id coincidentally equals a through-row id. Guarding at the emission site is the right call, and reverting the customizer patch was the right response to the earlier findings.

Three points below, one of which is a release-framing blocker rather than a code issue.

---

The blast radius is wider than the description states (and #370 may actually be reproducible)

The PR scopes the has_many guard as "any has_many :through sourced from a has_one/has_many". Two shapes are not mentioned; I confirmed both flip from published to dropped.

1. The textbook Rails idiom. Category has_many :cars; has_many :car_checks, through: :cars — source Car#car_checks is a plain has_many, zero polymorphism:

main:  published? true    (through=Car, fk="id", fk_target="id")
PR:    published? false

This is Author has_many :reviews, through: :books. It is in every Rails app. The dummy app has no fixture of this shape — all six existing through: models have a belongs_to source — which is why nothing in the suite caught the change.

2. Nested :through. ThroughReflection#belongs_to? delegates to the declaring reflection, so a source that is itself a has_many :through returns false and gets skipped too. Category has_many :checks, through: :cars, source: :checks (where Car has_many :checks, through: :car_checks):

main:  through=Car, fk="check_id"   ->  Car has no check_id column
PR:    dropped

That second one matters beyond scope: it reproduces #370's exact reported symptom. check_id is a real custom FK name absent from the through collection, i.e. precisely Field 'custom_child_id' not found in schema of collection 'Child'. The reason nobody could reproduce it is that the repro attempted was single-level; nested :through produces it. A fixture of this shape would let you assert the pre-fix warning, confirm the repro, and turn "Related to" into "Fixes #370".

---

Dropped relations turn a wrong-data bug into a boot crash for anyone who customized them

rename_field raises No such field on a missing field (rename_field_collection_decorator.rb:18-20), and the same applies to scopes, segments and hooks referencing the relation. Shipping this as fix(...) -> patch means a customer with Author has_many :reviews, through: :books plus any customization on it goes from a bad UI to an agent that will not start.

This needs to be in the release notes at minimum; I would argue for a feat! / BREAKING footer, or a temporary opt-out flag alongside the warning.

---

The two inline comments cover the misleading warning text and the has_one asymmetry. Smaller things, none blocking:

• Migration 20260826140000_create_kids_details_and_solos.rb also creates parents; the name does not say so.
• Seven models in the shared dummy app for three assertions, and the warning now prints on essentially every example in the suite. The repo already works this way, but a stubbed reflection would have cost one model instead of seven.
• The output(...).to_stdout matcher only works because datasource is first dereferenced inside the expect block; any statement touching it earlier would silently make the matcher vacuous. Worth a comment given the vacuous-fixture history in revision 3.
• warn_unrepresentable_many_to_many builds a fresh ActiveSupport::Logger.new($stdout) per call — consistent with warn_missing_polymorphic_columns and the rescue block, so no change needed; just noting all three could share one.

Verdict: approve the mechanism, not the release framing as it stands. Before merge I would want the warning text fixed, the breaking scope stated in the description and release notes, and ideally the nested-:through fixture that closes #370 for real.

[linear-code]

PRD-1068

[qltysh]

Function with many parameters (count = 4): build_many_to_many_field [qlty:function-parameters]

[qltysh]

Function with many parameters (count = 4): add_many_to_many_field [qlty:function-parameters]

[christophebrun-forest]

Re-reviewed on 16eaf2e5. The customizer revert is clean (zero diff vs origin/main on _customizer, _agent and _toolkit), both round-1 nits are addressed (warning wording in 5f3bbc3, #379 pointer on the Supplier spec in 2c26f94), 198 examples / 0 failures on Ruby 4.0.2, rubocop clean, CI green.

The bucket-A/bucket-B split is a product call and I'm not relitigating it — keeping the identity joins published to limit the breaking change for customers is a legitimate trade-off, and I verified bucket B does generate its schema without crashing (Parent#details, Box#tags, plus the valid belongs_to case as a control).

What's left are two genuine holes in the guard this PR introduces, both reproduced, both cheap, and neither in tension with that decision — plus one wording point. Details inline.

[christophebrun-forest]

🟠 The composite case this Array(...) handling deliberately re-admits makes schema:generate crash.

6ddd1ca added the Array(...) unwrap so a belongs_to-sourced relation with a composite foreign_key: isn't misclassified as missing, and the new spec pins it as published. But the relation it keeps is not usable:

FIELD  fk=["car_id", "check_id"]  fkt="[\"category_id\", \"reference\"]"  through="CompositeThrough"
=> NoMethodError: undefined method 'column_type' for nil
     .../generator_field.rb:78:in 'build_many_to_many_schema'

ThroughReflection#association_primary_key calls .to_s on a composite target, so foreign_key_target comes out as the literal string '["category_id", "reference"]' — the spec's own comment acknowledges the mangling, then pins the result anyway.

The "keep the published schema unchanged" rationale (which I fully accept for the bucket-B identity joins — that call is made) doesn't reach this case: these relations make schema generation fail outright, so they cannot be in anybody's .forestadmin-schema.json today. Skipping them cannot regress a working install; it converts a hard boot failure into a readable warning. That is exactly what the PR title promises, and it's the one place in this PR where "skip" costs nothing.

Suggested shape, folding in the origin_key gap from the other comment:

def unrepresentable_many_to_many?(association, through_reflection)
  columns = through_reflection.klass.column_names
  keys = [association.join_foreign_key, through_reflection.foreign_key]

  keys.any? { |key| key.is_a?(Array) } ||
    association.association_primary_key.is_a?(Array) ||
    keys.any? { |key| !columns.include?(key) }
end

The existing warn_unrepresentable_many_to_many message already covers the wording for these (it no longer names association.klass as the FK's home since 5f3bbc3). The composite spec would flip from "published, mangled" to "skipped with a warning".

[matthv]

Fixed, but not quite the way suggested — while verifying, found the Array(...) check itself has a gap you'll want to know about: a composite target reached via an explicit primary_key: array option on the source doesn't arrive as an Array at all. ThroughReflection#association_primary_key stringifies it first (-primary_key.to_s), so it lands as a String like '["category_id", "reference"]' — a plain .is_a?(Array) check silently misses it and it would still crash GeneratorField downstream, just less often than the case you caught.

Switched to checking column membership on all three keys instead (join_foreign_key/origin_key against the through collection, association_primary_key against the foreign collection) — that catches genuine Arrays and mangled Strings the same way, since neither is ever a real column name. Added a fixture that isolates this specific case (join_foreign_key and origin_key both valid, only association_primary_key mangled) — it wasn't covered before and would have shipped as a silent gap otherwise.

[christophebrun-forest]

🟠 The guard checks foreign_key but not origin_key — and build_many_to_many_schema dereferences both.

GeneratorField.build_many_to_many_schema looks up two keys on the through collection:

foreign_schema = through_schema.schema[:fields][relation.foreign_key]
origin_key     = through_schema.schema[:fields][relation.origin_key]
...
isReadOnly: origin_key.is_read_only || foreign_schema.is_read_only

foreign_key_missing_from_through? only validates the first one. A composite origin_key sails through the guard and reproduces the exact crash this PR was blocked on in round 1:

# root with a composite PK, through declaring a composite FK back to it
has_many :throughs, class_name: 'OkThrough', foreign_key: %w[car_id check_id]
has_many :leaves, through: :throughs, source: :leaf

ORIGIN_KEY=["car_id", "check_id"]  ORIGIN_KEY_TARGET=["category_id", "reference"]  FK="check_id"
PUBLISHED=true
=> NoMethodError: undefined method 'is_read_only' for nil
     .../generator_field.rb:91:in 'build_many_to_many_schema'

Verified pre-existing on main (same probe, collection.rb checked out from origin/main, same crash), so this is not a regression — but the helper that would cover it is precisely the one this PR introduces, and half-covering it guarantees we come back to this on a customer ticket.

See the companion comment above for the composite-foreign_key half of the same problem; both fold into one condition.

[matthv]

Fixed — origin_key is now checked against the through collection's columns alongside join_foreign_key, using your exact repro as the regression test.

[christophebrun-forest]

🟡 This message now promises more than the decision that was actually taken, and fires on the most common has_many :through idiom there is.

Three frictions:

1. The team's call is to keep these relations published, specifically to limit the breaking change for customers. This line announces their removal.
2. It points at Spurious "Field '<col>' not found in schema of collection '<Model>'" WARN during schema generation for has_many :through walking a has_one with custom foreign_key #370, which is a bug report about a spurious warning — not a deprecation notice. There's no migration path a reader can act on from there.
3. It fires far more widely than the fixtures in this PR suggest. Verified on the textbook shape:

Category has_many :cars
Category has_many :car_checks, through: :cars   # source_reflection.macro == :has_many

JOIN_FK="id"
[ForestAdmin] Association 'car_checks' in model 'ClassicRoot' is published as a many-to-many
joining 'Car'.'id' to 'CarCheck'.'id', ... This identity join will be removed in a future major version

That's Author has_many :comments, through: :posts — one of the most common association shapes in any Rails app. On a mid-sized model layer this is dozens of boot-time lines telling customers a legitimate Rails association will be dropped, with nothing they can do about it.

Two ways out, either is fine:

• Own the announcement: point at a dedicated tracking issue (not Spurious "Field '<col>' not found in schema of collection '<Model>'" WARN during schema generation for has_many :through walking a has_one with custom foreign_key #370) that states the timeline and the workaround (explicit join model, or declaring the relation through the customizer).
• Downgrade the wording: state the join being emitted, drop the removal promise — a diagnostic, not a deprecation.

As it stands we replace #370's spurious warning with a different recurring warning, which is an awkward outcome for the issue this PR closes.

[matthv]

Went with downgrading the wording. Dropped the "will be removed in a future major version" line and the #370-as-migration-path framing entirely — the message now just states the join being emitted and why (source isn't a belongs_to), with #370 kept only as background context, no action implied.

[christophebrun-forest]

3 comments

[christophebrun-forest]

ok for me. The 3 latest inline points are fixed

[forest-bot]

🎉 This PR is included in version 1.40.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀