From dbf61fe1f55fb3ca09c8fb16ddc3f7ee371a5f86 Mon Sep 17 00:00:00 2001 From: Matt Date: Wed, 26 Aug 2026 11:08:23 +0200 Subject: [PATCH 01/10] fix(datasource-customizer): resolve ManyToMany keys on either side published? assumed a ManyToMany's foreign_key/origin_key always live on the through collection. A has_many :through chaining into a polymorphic has_one/belongs_to with a custom foreign_key can put the column on the foreign collection instead, which made published? log a false 'field not found' warning and, more importantly, incorrectly unpublish (hide) an otherwise valid relation. relation_key_published? now checks whichever candidate collection actually declares the field, falling back to the through collection so a genuinely missing field still logs and stays hidden. Fixes #370 --- .../publication_collection_decorator.rb | 20 ++++- .../publication_collection_decorator_spec.rb | 77 +++++++++++++++++++ 2 files changed, 95 insertions(+), 2 deletions(-) diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/publication/publication_collection_decorator.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/publication/publication_collection_decorator.rb index ed1aca45a..68cf00492 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/publication/publication_collection_decorator.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/publication/publication_collection_decorator.rb @@ -86,11 +86,13 @@ def published?(name) end if field.type == 'ManyToMany' + candidates = [field.through_collection, field.foreign_collection] + return ( datasource.published?(field.through_collection) && datasource.published?(field.foreign_collection) && - datasource.get_collection(field.through_collection).published?(field.foreign_key) && - datasource.get_collection(field.through_collection).published?(field.origin_key) && + relation_key_published?(candidates, field.foreign_key) && + relation_key_published?(candidates, field.origin_key) && published?(field.origin_key_target) && datasource.get_collection(field.foreign_collection).published?(field.foreign_key_target) ) @@ -99,6 +101,20 @@ def published?(name) true end + # foreign_key/origin_key are normally columns on the through collection, but a + # has_many :through chaining into a has_one/belongs_to with a custom foreign_key + # can produce a schema where the column actually lives on the foreign collection + # instead (see #370). Check whichever candidate collection actually declares the + # field, falling back to the first one so a genuinely missing field still logs. + def relation_key_published?(candidate_collections, key) + candidate_collections.each do |collection_name| + collection = datasource.get_collection(collection_name) + return collection.published?(key) if collection.schema[:fields].key?(key) + end + + datasource.get_collection(candidate_collections.first).published?(key) + end + # rubocop:disable Lint/UselessMethodDefinition def mark_schema_as_dirty super diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/publication/publication_collection_decorator_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/publication/publication_collection_decorator_spec.rb index d9436d1ed..ad1e68168 100644 --- a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/publication/publication_collection_decorator_spec.rb +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/publication/publication_collection_decorator_spec.rb @@ -203,6 +203,83 @@ module Publication expect(result).to be(false) end end + + context 'when a ManyToMany foreign_key lives on the foreign collection instead of the through collection' do + # Reproduces #370: a `has_many :through` chaining into a polymorphic + # `has_one`/`belongs_to` with a custom foreign_key makes the AR datasource emit + # a ManyToManySchema whose foreign_key column actually lives on the foreign + # collection ('leaf'), not on the through collection ('child'). + before do + @collection_parent = instance_double( + ForestAdminDatasourceToolkit::Collection, + name: 'parent', + schema: { + fields: { + 'id' => ColumnSchema.new(column_type: 'Number', is_primary_key: true), + 'leaves' => Relations::ManyToManySchema.new( + origin_key: 'owner_id', + origin_key_target: 'id', + foreign_key: 'custom_child_id', + foreign_key_target: 'id', + foreign_collection: 'leaf', + through_collection: 'child', + origin_type_field: 'owner_type', + origin_type_value: 'Parent' + ) + } + } + ) + + @collection_child = instance_double( + ForestAdminDatasourceToolkit::Collection, + name: 'child', + schema: { + fields: { + 'id' => ColumnSchema.new(column_type: 'Number', is_primary_key: true), + 'owner_id' => ColumnSchema.new(column_type: 'Number'), + 'owner_type' => ColumnSchema.new(column_type: 'String') + } + } + ) + + @collection_leaf = instance_double( + ForestAdminDatasourceToolkit::Collection, + name: 'leaf', + schema: { + fields: { + 'id' => ColumnSchema.new(column_type: 'Number', is_primary_key: true), + 'custom_child_id' => ColumnSchema.new(column_type: 'Number') + } + } + ) + + through_datasource = ForestAdminDatasourceToolkit::Datasource.new + through_datasource.add_collection(@collection_parent) + through_datasource.add_collection(@collection_child) + through_datasource.add_collection(@collection_leaf) + + @decorated_parent = PublicationDatasourceDecorator.new(through_datasource).get_collection('parent') + end + + it 'is published without logging a false "field not found" warning' do + logger = instance_spy(Logger) + allow(ForestAdminAgent::Facades::Container).to receive(:logger).and_return(logger) + + expect(@decorated_parent.published?('leaves')).to be(true) + expect(logger).not_to have_received(:log) + end + + it 'still logs and hides the relation when the foreign key is truly missing everywhere' do + @collection_leaf.schema[:fields].delete('custom_child_id') + logger = instance_spy(Logger) + allow(ForestAdminAgent::Facades::Container).to receive(:logger).and_return(logger) + + result = @decorated_parent.published?('leaves') + + expect(logger).to have_received(:log).with('Warn', "Field 'custom_child_id' not found in schema of collection 'child'") + expect(result).to be(false) + end + end end end end From 66a2ceeaf95790dacca0d1799e76d99793554383 Mon Sep 17 00:00:00 2001 From: Matt Date: Wed, 26 Aug 2026 15:15:49 +0200 Subject: [PATCH 02/10] fix(datasource-active-record): don't emit ManyToMany for a non-belongs_to through source Revert the publication_collection_decorator.rb guessing game entirely and fix #370 at the source instead, per @christophebrun-forest's review: ManyToManySchema's foreign_key must live on the through collection, which only holds when the through model reaches the target via belongs_to. A has_many/has_one :through sourced from a has_one/has_many (as in #370's Parent -> Child -> Leaf chain) puts the fk on the target collection instead, producing a schema no consumer can safely use. Publishing that broken schema (this branch's previous commit) made things worse, not better: GeneratorField.build_many_to_many_schema assumes the same contract and crashes with a NoMethodError once the relation is no longer filtered out -- confirmed against this exact fixture. The old customizer-level fix also carried the two risks Macroscope flagged (first-match key resolution across collections, and a SystemStackError on cyclic relations via decorated .schema calls) that are moot now that the decorator change is gone. foreign_key_on_through_collection? now gates ManyToManySchema construction in both the has_one and has_many :through branches; an invalid shape is skipped with a warning instead of published. New Parent/Child/Leaf fixture (mirroring #370 exactly) confirms the field is absent from the schema and nothing raises. Fixes #370 --- .../collection.rb | 53 +++++++++---- .../spec/dummy/app/models/child.rb | 4 + .../spec/dummy/app/models/leaf.rb | 3 + .../spec/dummy/app/models/parent.rb | 4 + ...0000_create_parents_children_and_leaves.rb | 13 ++++ .../collection_spec.rb | 12 +++ .../publication_collection_decorator.rb | 20 +---- .../publication_collection_decorator_spec.rb | 77 ------------------- 8 files changed, 76 insertions(+), 110 deletions(-) create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/app/models/child.rb create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/app/models/leaf.rb create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/app/models/parent.rb create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826130000_create_parents_children_and_leaves.rb diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb index f5f9b77c2..9cd9dcf84 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb @@ -88,6 +88,12 @@ def cascade_on_delete?(association) %i[destroy destroy_async delete_all].include?(association.options[:dependent]) end + # True only when the source reflection is belongs_to; a has_one/has_many source + # means the column actually lives on the target collection instead (see #370). + def foreign_key_on_through_collection?(association) + association.source_reflection&.belongs_to? + end + # rubocop:disable Metrics/BlockNesting def fetch_associations associations(@model, support_polymorphic_relations: @support_polymorphic_relations).each do |association| @@ -99,8 +105,9 @@ def fetch_associations is_polymorphic = through_reflection.options[:as].present? source_polymorphic = association.source_reflection&.polymorphic? && association.options[:source_type].present? + many_to_many_shape = is_polymorphic || source_polymorphic - if is_polymorphic || source_polymorphic + if many_to_many_shape && foreign_key_on_through_collection?(association) add_field( association.name.to_s, ForestAdminDatasourceToolkit::Schema::Relations::ManyToManySchema.new( @@ -116,6 +123,8 @@ def fetch_associations foreign_type_value: source_polymorphic ? association.options[:source_type] : nil ) ) + elsif many_to_many_shape + warn_unrepresentable_many_to_many(association, through_reflection) else add_field( association.name.to_s, @@ -186,21 +195,25 @@ def fetch_associations source_polymorphic = association.source_reflection&.polymorphic? && association.options[:source_type].present? - add_field( - association.name.to_s, - ForestAdminDatasourceToolkit::Schema::Relations::ManyToManySchema.new( - foreign_collection: format_model_name(association.klass.name), - origin_key: through_reflection.foreign_key, - origin_key_target: through_reflection.join_foreign_key, - foreign_key: association.join_foreign_key, - foreign_key_target: association.association_primary_key, - through_collection: format_model_name(through_reflection.klass.name), - origin_type_field: is_polymorphic ? through_reflection.type : nil, - origin_type_value: is_polymorphic ? @model.name : nil, - foreign_type_field: source_polymorphic ? association.source_reflection.foreign_type : nil, - foreign_type_value: source_polymorphic ? association.options[:source_type] : nil + if foreign_key_on_through_collection?(association) + add_field( + association.name.to_s, + ForestAdminDatasourceToolkit::Schema::Relations::ManyToManySchema.new( + foreign_collection: format_model_name(association.klass.name), + origin_key: through_reflection.foreign_key, + origin_key_target: through_reflection.join_foreign_key, + foreign_key: association.join_foreign_key, + foreign_key_target: association.association_primary_key, + through_collection: format_model_name(through_reflection.klass.name), + origin_type_field: is_polymorphic ? through_reflection.type : nil, + origin_type_value: is_polymorphic ? @model.name : nil, + foreign_type_field: source_polymorphic ? association.source_reflection.foreign_type : nil, + foreign_type_value: source_polymorphic ? association.options[:source_type] : nil + ) ) - ) + else + warn_unrepresentable_many_to_many(association, through_reflection) + end elsif association.inverse_of&.polymorphic? add_field( association.name.to_s, @@ -303,6 +316,16 @@ def create_virtual_habtm_model(association, model_name) klass end + def warn_unrepresentable_many_to_many(association, through_reflection) + logger = ActiveSupport::Logger.new($stdout) + logger.warn( + "[ForestAdmin] ⚠️ Skipping association '#{association.name}' in model '#{@model.name}': " \ + "its foreign key lives on '#{format_model_name(association.klass.name)}', not on the through " \ + "collection '#{format_model_name(through_reflection.klass.name)}', so it cannot be represented " \ + 'as a Forest Admin many-to-many relation.' + ) + end + def warn_missing_polymorphic_columns(association) missing_columns = [] missing_columns << association.foreign_key unless schema[:fields][association.foreign_key] diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/child.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/child.rb new file mode 100644 index 000000000..e29d80b20 --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/child.rb @@ -0,0 +1,4 @@ +class Child < ApplicationRecord + belongs_to :owner, polymorphic: true + has_one :leaf, foreign_key: :custom_child_id, inverse_of: :child +end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/leaf.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/leaf.rb new file mode 100644 index 000000000..2ebc287a0 --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/leaf.rb @@ -0,0 +1,3 @@ +class Leaf < ApplicationRecord + belongs_to :child, foreign_key: :custom_child_id, inverse_of: :leaf +end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/parent.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/parent.rb new file mode 100644 index 000000000..8487a2c2a --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/parent.rb @@ -0,0 +1,4 @@ +class Parent < ApplicationRecord + has_many :children, as: :owner + has_many :leaves, through: :children +end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826130000_create_parents_children_and_leaves.rb b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826130000_create_parents_children_and_leaves.rb new file mode 100644 index 000000000..e842bffa8 --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826130000_create_parents_children_and_leaves.rb @@ -0,0 +1,13 @@ +class CreateParentsChildrenAndLeaves < ActiveRecord::Migration[7.1] + def change + create_table :parents + + create_table :children do |t| + t.references :owner, polymorphic: true + end + + create_table :leaves do |t| + t.integer :custom_child_id + end + end +end diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb index 259c48b11..22408dfc0 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb @@ -192,6 +192,18 @@ module ForestAdminDatasourceActiveRecord expect(projects_relation.origin_type_value).to be_nil end + it 'skips a has_many :through whose source reflection is not a belongs_to (#370)' do + # Parent -> (polymorphic has_many) -> Child -> (has_one, custom foreign_key) -> Leaf. + # The fk ends up on Leaf, not on Child (the through collection), which + # ManyToManySchema cannot express -- so the field must not be published at all. + expect do + described_class.new(datasource, Parent) + end.not_to raise_error + + collection = described_class.new(datasource, Parent) + expect(collection.schema[:fields].keys).not_to include('leaves') + end + # rubocop:disable RSpec/ExampleLength it 'handles polymorphic associations with missing foreign key columns' do # This test reproduces issue #202: Server crashing on startup when missing columns for foreign keys diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/publication/publication_collection_decorator.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/publication/publication_collection_decorator.rb index 68cf00492..ed1aca45a 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/publication/publication_collection_decorator.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/publication/publication_collection_decorator.rb @@ -86,13 +86,11 @@ def published?(name) end if field.type == 'ManyToMany' - candidates = [field.through_collection, field.foreign_collection] - return ( datasource.published?(field.through_collection) && datasource.published?(field.foreign_collection) && - relation_key_published?(candidates, field.foreign_key) && - relation_key_published?(candidates, field.origin_key) && + datasource.get_collection(field.through_collection).published?(field.foreign_key) && + datasource.get_collection(field.through_collection).published?(field.origin_key) && published?(field.origin_key_target) && datasource.get_collection(field.foreign_collection).published?(field.foreign_key_target) ) @@ -101,20 +99,6 @@ def published?(name) true end - # foreign_key/origin_key are normally columns on the through collection, but a - # has_many :through chaining into a has_one/belongs_to with a custom foreign_key - # can produce a schema where the column actually lives on the foreign collection - # instead (see #370). Check whichever candidate collection actually declares the - # field, falling back to the first one so a genuinely missing field still logs. - def relation_key_published?(candidate_collections, key) - candidate_collections.each do |collection_name| - collection = datasource.get_collection(collection_name) - return collection.published?(key) if collection.schema[:fields].key?(key) - end - - datasource.get_collection(candidate_collections.first).published?(key) - end - # rubocop:disable Lint/UselessMethodDefinition def mark_schema_as_dirty super diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/publication/publication_collection_decorator_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/publication/publication_collection_decorator_spec.rb index ad1e68168..d9436d1ed 100644 --- a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/publication/publication_collection_decorator_spec.rb +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/publication/publication_collection_decorator_spec.rb @@ -203,83 +203,6 @@ module Publication expect(result).to be(false) end end - - context 'when a ManyToMany foreign_key lives on the foreign collection instead of the through collection' do - # Reproduces #370: a `has_many :through` chaining into a polymorphic - # `has_one`/`belongs_to` with a custom foreign_key makes the AR datasource emit - # a ManyToManySchema whose foreign_key column actually lives on the foreign - # collection ('leaf'), not on the through collection ('child'). - before do - @collection_parent = instance_double( - ForestAdminDatasourceToolkit::Collection, - name: 'parent', - schema: { - fields: { - 'id' => ColumnSchema.new(column_type: 'Number', is_primary_key: true), - 'leaves' => Relations::ManyToManySchema.new( - origin_key: 'owner_id', - origin_key_target: 'id', - foreign_key: 'custom_child_id', - foreign_key_target: 'id', - foreign_collection: 'leaf', - through_collection: 'child', - origin_type_field: 'owner_type', - origin_type_value: 'Parent' - ) - } - } - ) - - @collection_child = instance_double( - ForestAdminDatasourceToolkit::Collection, - name: 'child', - schema: { - fields: { - 'id' => ColumnSchema.new(column_type: 'Number', is_primary_key: true), - 'owner_id' => ColumnSchema.new(column_type: 'Number'), - 'owner_type' => ColumnSchema.new(column_type: 'String') - } - } - ) - - @collection_leaf = instance_double( - ForestAdminDatasourceToolkit::Collection, - name: 'leaf', - schema: { - fields: { - 'id' => ColumnSchema.new(column_type: 'Number', is_primary_key: true), - 'custom_child_id' => ColumnSchema.new(column_type: 'Number') - } - } - ) - - through_datasource = ForestAdminDatasourceToolkit::Datasource.new - through_datasource.add_collection(@collection_parent) - through_datasource.add_collection(@collection_child) - through_datasource.add_collection(@collection_leaf) - - @decorated_parent = PublicationDatasourceDecorator.new(through_datasource).get_collection('parent') - end - - it 'is published without logging a false "field not found" warning' do - logger = instance_spy(Logger) - allow(ForestAdminAgent::Facades::Container).to receive(:logger).and_return(logger) - - expect(@decorated_parent.published?('leaves')).to be(true) - expect(logger).not_to have_received(:log) - end - - it 'still logs and hides the relation when the foreign key is truly missing everywhere' do - @collection_leaf.schema[:fields].delete('custom_child_id') - logger = instance_spy(Logger) - allow(ForestAdminAgent::Facades::Container).to receive(:logger).and_return(logger) - - result = @decorated_parent.published?('leaves') - - expect(logger).to have_received(:log).with('Warn', "Field 'custom_child_id' not found in schema of collection 'child'") - expect(result).to be(false) - end - end end end end From 5ae20e02f9015a04cc6bc5d8ce532225a7ef3984 Mon Sep 17 00:00:00 2001 From: Matt Date: Wed, 26 Aug 2026 17:03:16 +0200 Subject: [PATCH 03/10] fix(datasource-active-record): don't emit ManyToMany for a non-belongs_to through source ThroughReflection#join_foreign_key delegates to the source reflection; only belongs_to overrides it to return a real FK column. A has_one or has_many source (as in #370's Parent -> Child -> Leaf chain) falls back to the base implementation, which returns the source model's own primary key -- not the custom foreign_key option on the association. The previous fix on this branch (relation_key_published? in the publication decorator) treated this as a missing-column problem and tried to resolve the key against either candidate collection. That carried the two risks Macroscope flagged (first-match resolution across collections on a name collision, and a SystemStackError on cyclic relations via decorated .schema calls) and didn't address the real defect: with a has_one/has_many source, ManyToManySchema ends up with foreign_key resolving to the through collection's own primary key, silently publishing a `through.id = foreign.id` join -- wrong data with no warning, not a missing-field log message. foreign_key_on_through_collection? now gates ManyToManySchema construction on source_reflection.belongs_to?, in both the has_one and has_many :through branches; an invalid shape is skipped with a warning instead of published. The has_many guard applies unconditionally (any has_many :through sourced from a has_one/has_many, polymorphic or not); the has_one guard only fires when the relation is polymorphic -- a non-polymorphic has_one :through falls into the pre-existing OneToOneSchema path, which has the same class of bug and is out of scope here (see Supplier#account_history). Reverts the forest_admin_datasource_customizer changes back to main. Three fixtures cover this: Parent/Kid/Detail (has_many, polymorphic), Solo/Kid/Detail (has_one, polymorphic), Box/Slot/Tag (has_many, non-polymorphic) -- each verified to fail against unfixed collection.rb and pass with the fix. Neither we nor a second reviewer could reproduce #370's exact reported message (custom_child_id rather than the id this shape actually produces), so this fixes the same class of defect rather than a confirmed repro of the reporter's exact case. Related to #370 --- .../collection.rb | 5 +-- .../spec/dummy/app/models/box.rb | 4 +++ .../spec/dummy/app/models/child.rb | 4 --- .../spec/dummy/app/models/detail.rb | 3 ++ .../spec/dummy/app/models/kid.rb | 4 +++ .../spec/dummy/app/models/leaf.rb | 3 -- .../spec/dummy/app/models/parent.rb | 4 +-- .../spec/dummy/app/models/slot.rb | 4 +++ .../spec/dummy/app/models/solo.rb | 4 +++ .../spec/dummy/app/models/tag.rb | 3 ++ ...0000_create_parents_children_and_leaves.rb | 13 -------- ...826140000_create_kids_details_and_solos.rb | 14 ++++++++ ...60826150000_create_boxes_slots_and_tags.rb | 13 ++++++++ .../collection_spec.rb | 33 ++++++++++++++----- 14 files changed, 79 insertions(+), 32 deletions(-) create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/app/models/box.rb delete mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/app/models/child.rb create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/app/models/detail.rb create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/app/models/kid.rb delete mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/app/models/leaf.rb create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/app/models/slot.rb create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/app/models/solo.rb create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/app/models/tag.rb delete mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826130000_create_parents_children_and_leaves.rb create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826140000_create_kids_details_and_solos.rb create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826150000_create_boxes_slots_and_tags.rb diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb index 9cd9dcf84..37130239f 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb @@ -88,8 +88,9 @@ def cascade_on_delete?(association) %i[destroy destroy_async delete_all].include?(association.options[:dependent]) end - # True only when the source reflection is belongs_to; a has_one/has_many source - # means the column actually lives on the target collection instead (see #370). + # ThroughReflection#join_foreign_key delegates to the source reflection; only + # belongs_to overrides it to return a real FK column. Any other macro falls back + # to the source model's own primary key, producing a bogus identity join (#370). def foreign_key_on_through_collection?(association) association.source_reflection&.belongs_to? end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/box.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/box.rb new file mode 100644 index 000000000..90b579a22 --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/box.rb @@ -0,0 +1,4 @@ +class Box < ApplicationRecord + has_many :slots + has_many :tags, through: :slots +end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/child.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/child.rb deleted file mode 100644 index e29d80b20..000000000 --- a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/child.rb +++ /dev/null @@ -1,4 +0,0 @@ -class Child < ApplicationRecord - belongs_to :owner, polymorphic: true - has_one :leaf, foreign_key: :custom_child_id, inverse_of: :child -end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/detail.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/detail.rb new file mode 100644 index 000000000..038355b7e --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/detail.rb @@ -0,0 +1,3 @@ +class Detail < ApplicationRecord + belongs_to :kid, foreign_key: :custom_kid_id, inverse_of: :detail +end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/kid.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/kid.rb new file mode 100644 index 000000000..5887cb98a --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/kid.rb @@ -0,0 +1,4 @@ +class Kid < ApplicationRecord + belongs_to :owner, polymorphic: true + has_one :detail, foreign_key: :custom_kid_id, inverse_of: :kid +end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/leaf.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/leaf.rb deleted file mode 100644 index 2ebc287a0..000000000 --- a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/leaf.rb +++ /dev/null @@ -1,3 +0,0 @@ -class Leaf < ApplicationRecord - belongs_to :child, foreign_key: :custom_child_id, inverse_of: :leaf -end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/parent.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/parent.rb index 8487a2c2a..4d50e20f5 100644 --- a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/parent.rb +++ b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/parent.rb @@ -1,4 +1,4 @@ class Parent < ApplicationRecord - has_many :children, as: :owner - has_many :leaves, through: :children + has_many :kids, as: :owner + has_many :details, through: :kids, source: :detail end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/slot.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/slot.rb new file mode 100644 index 000000000..8fbd33bba --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/slot.rb @@ -0,0 +1,4 @@ +class Slot < ApplicationRecord + belongs_to :box + has_one :tag, foreign_key: :custom_slot_id, inverse_of: :slot +end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/solo.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/solo.rb new file mode 100644 index 000000000..ca1670e27 --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/solo.rb @@ -0,0 +1,4 @@ +class Solo < ApplicationRecord + has_one :kid, as: :owner + has_one :detail, through: :kid +end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/tag.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/tag.rb new file mode 100644 index 000000000..c456f93fc --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/tag.rb @@ -0,0 +1,3 @@ +class Tag < ApplicationRecord + belongs_to :slot, foreign_key: :custom_slot_id, inverse_of: :tag +end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826130000_create_parents_children_and_leaves.rb b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826130000_create_parents_children_and_leaves.rb deleted file mode 100644 index e842bffa8..000000000 --- a/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826130000_create_parents_children_and_leaves.rb +++ /dev/null @@ -1,13 +0,0 @@ -class CreateParentsChildrenAndLeaves < ActiveRecord::Migration[7.1] - def change - create_table :parents - - create_table :children do |t| - t.references :owner, polymorphic: true - end - - create_table :leaves do |t| - t.integer :custom_child_id - end - end -end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826140000_create_kids_details_and_solos.rb b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826140000_create_kids_details_and_solos.rb new file mode 100644 index 000000000..22508daa7 --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826140000_create_kids_details_and_solos.rb @@ -0,0 +1,14 @@ +class CreateKidsDetailsAndSolos < ActiveRecord::Migration[7.1] + def change + create_table :parents + create_table :solos + + create_table :kids do |t| + t.references :owner, polymorphic: true + end + + create_table :details do |t| + t.integer :custom_kid_id + end + end +end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826150000_create_boxes_slots_and_tags.rb b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826150000_create_boxes_slots_and_tags.rb new file mode 100644 index 000000000..1eb5ce6f5 --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826150000_create_boxes_slots_and_tags.rb @@ -0,0 +1,13 @@ +class CreateBoxesSlotsAndTags < ActiveRecord::Migration[7.1] + def change + create_table :boxes + + create_table :slots do |t| + t.references :box + end + + create_table :tags do |t| + t.integer :custom_slot_id + end + end +end diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb index 22408dfc0..339f4a67d 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb @@ -193,15 +193,32 @@ module ForestAdminDatasourceActiveRecord end it 'skips a has_many :through whose source reflection is not a belongs_to (#370)' do - # Parent -> (polymorphic has_many) -> Child -> (has_one, custom foreign_key) -> Leaf. - # The fk ends up on Leaf, not on Child (the through collection), which - # ManyToManySchema cannot express -- so the field must not be published at all. - expect do - described_class.new(datasource, Parent) - end.not_to raise_error + # Parent -> Kid (polymorphic has_many) -> Detail (has_one, custom foreign_key). + # Kid#detail is a has_one, so join_foreign_key resolves to Kid's own primary key + # instead of a real join column -- the relation can't be expressed as ManyToMany. + expect { datasource.get_collection('Parent') }.to output(/Skipping association 'details'/).to_stdout + + expect(datasource.get_collection('Parent').schema[:fields].keys).not_to include('details') + end + + it 'skips a polymorphic has_one :through whose source reflection is not a belongs_to' do + # Solo -> Kid (polymorphic has_one) -> Detail (has_one, custom foreign_key): same + # invalid shape as above, through the has_one branch instead of has_many. The + # has_one branch only reaches this guard when the relation is polymorphic -- + # a non-polymorphic has_one :through still falls into the pre-existing + # OneToOneSchema path below, unguarded (same class of bug, out of scope here). + expect { datasource.get_collection('Solo') }.to output(/Skipping association 'detail'/).to_stdout + + expect(datasource.get_collection('Solo').schema[:fields].keys).not_to include('detail') + end + + it 'skips a non-polymorphic has_many :through whose source reflection is not a belongs_to' do + # Box -> Slot (plain has_many, no polymorphism at all) -> Tag (has_one, custom + # foreign_key). Unlike the has_one branch above, the has_many branch has no + # polymorphic gate: this guard applies to ordinary has_many :through chains too. + expect { datasource.get_collection('Box') }.to output(/Skipping association 'tags'/).to_stdout - collection = described_class.new(datasource, Parent) - expect(collection.schema[:fields].keys).not_to include('leaves') + expect(datasource.get_collection('Box').schema[:fields].keys).not_to include('tags') end # rubocop:disable RSpec/ExampleLength From 5f3bbc3ab18a5ee8347722e6c2dd4763f0f40870 Mon Sep 17 00:00:00 2001 From: Matt Date: Thu, 27 Aug 2026 09:34:36 +0200 Subject: [PATCH 04/10] fix(datasource-active-record): fix warning wording and add the nested :through repro Christophe's review flagged two issues: - The warning message hardcoded association.klass as the FK's home, which is wrong once the source reflection is itself a ThroughReflection (nested :through): the real FK lives further down the chain, not on either of the two collections named. Reworded to state the general fact (no real join column between the through and foreign collections) without claiming a specific wrong location -- true regardless of nesting depth. Verified against the nested case below. - Neither of us could reproduce #370's exact reported message with a single-hop source. A nested has_many :through (Car#checks, itself through: :car_checks) does: ThroughReflection#belongs_to? delegates to the declaring reflection, so a ThroughReflection source is never belongs_to?, and join_foreign_key resolves all the way down to the innermost belongs_to's real FK column ("check_id") -- a genuine column name missing from the immediate through collection, not the generic "id" primary-key fallback the single-hop fixtures produce. New spec reproduces this via a stub_const on the existing categories table (no new model/migration needed), asserts join_foreign_key resolves to 'check_id', and confirms the guard catches it. Dropped the now-inaccurate '(#370)' tag from the original has_many test and moved it to this one, which actually proves the connection. --- .../collection.rb | 8 ++++-- .../collection_spec.rb | 28 ++++++++++++++++++- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb index 37130239f..f2c7d5b5c 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb @@ -318,12 +318,14 @@ def create_virtual_habtm_model(association, model_name) end def warn_unrepresentable_many_to_many(association, through_reflection) + source = association.source_reflection logger = ActiveSupport::Logger.new($stdout) logger.warn( "[ForestAdmin] ⚠️ Skipping association '#{association.name}' in model '#{@model.name}': " \ - "its foreign key lives on '#{format_model_name(association.klass.name)}', not on the through " \ - "collection '#{format_model_name(through_reflection.klass.name)}', so it cannot be represented " \ - 'as a Forest Admin many-to-many relation.' + "its source association '#{source.name}' on '#{format_model_name(through_reflection.klass.name)}' " \ + "is a #{source.macro}, not a belongs_to, so there is no real join column between " \ + "'#{format_model_name(through_reflection.klass.name)}' and '#{format_model_name(association.klass.name)}' " \ + '-- this relation cannot be represented as a Forest Admin many-to-many.' ) end diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb index 339f4a67d..067ec835b 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb @@ -192,7 +192,7 @@ module ForestAdminDatasourceActiveRecord expect(projects_relation.origin_type_value).to be_nil end - it 'skips a has_many :through whose source reflection is not a belongs_to (#370)' do + it 'skips a has_many :through whose source reflection is not a belongs_to' do # Parent -> Kid (polymorphic has_many) -> Detail (has_one, custom foreign_key). # Kid#detail is a has_one, so join_foreign_key resolves to Kid's own primary key # instead of a real join column -- the relation can't be expressed as ManyToMany. @@ -201,6 +201,32 @@ module ForestAdminDatasourceActiveRecord expect(datasource.get_collection('Parent').schema[:fields].keys).not_to include('details') end + it "skips a nested has_many :through, reproducing #370's exact symptom" do + # Category -> Car (plain has_many) -> Check, through Car's OWN has_many :through + # (Car#checks, through: :car_checks). Car.checks's source_reflection is itself a + # ThroughReflection, so #belongs_to? is false and join_foreign_key resolves all + # the way down to CarCheck#check's real FK ("check_id") -- a genuine column name, + # missing from Car specifically (not the generic "id" the single-hop cases above + # fall back to). This is the shape that actually produces #370's reported message. + stub_const('NestedThroughProbe', Class.new(ApplicationRecord) do + self.table_name = 'categories' + self.abstract_class = true + + has_many :cars, foreign_key: :category_id + has_many :checks, through: :cars, source: :checks + end) + + association = NestedThroughProbe.reflect_on_association(:checks) + expect(association.join_foreign_key).to eq('check_id') + + collection = nil + expect do + collection = described_class.new(datasource, NestedThroughProbe, support_polymorphic_relations: true) + end.to output(/Skipping association 'checks'/).to_stdout + + expect(collection.schema[:fields].keys).not_to include('checks') + end + it 'skips a polymorphic has_one :through whose source reflection is not a belongs_to' do # Solo -> Kid (polymorphic has_one) -> Detail (has_one, custom foreign_key): same # invalid shape as above, through the has_one branch instead of has_many. The From 795d6921e6a2ce01c9c98852f39245412f40304a Mon Sep 17 00:00:00 2001 From: Matt Date: Thu, 27 Aug 2026 09:36:23 +0200 Subject: [PATCH 05/10] test(datasource-active-record): address the remaining review nits - Rename the migration to mention it also creates parents (same timestamp, no re-run needed -- Rails tracks migrations by version number, not filename). - Point the Supplier#account_history spec at the fact that it pins the same class of bug as #370 (has_one :through with a non-belongs_to source), just via the pre-existing OneToOneSchema path, out of scope for this fix. - Document why every output(...).to_stdout guard test dereferences datasource for the first time inside the expect block -- the exact mistake that made an earlier revision of this fix vacuous. --- ...26140000_create_parents_kids_details_and_solos.rb} | 2 +- .../collection_spec.rb | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) rename packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/{20260826140000_create_kids_details_and_solos.rb => 20260826140000_create_parents_kids_details_and_solos.rb} (76%) diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826140000_create_kids_details_and_solos.rb b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826140000_create_parents_kids_details_and_solos.rb similarity index 76% rename from packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826140000_create_kids_details_and_solos.rb rename to packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826140000_create_parents_kids_details_and_solos.rb index 22508daa7..c9bbe5712 100644 --- a/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826140000_create_kids_details_and_solos.rb +++ b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260826140000_create_parents_kids_details_and_solos.rb @@ -1,4 +1,4 @@ -class CreateKidsDetailsAndSolos < ActiveRecord::Migration[7.1] +class CreateParentsKidsDetailsAndSolos < ActiveRecord::Migration[7.1] def change create_table :parents create_table :solos diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb index 067ec835b..ebb84845e 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb @@ -44,6 +44,11 @@ module ForestAdminDatasourceActiveRecord expect(collection.schema[:fields].keys).to include('users') end + # Supplier -> Account -> AccountHistory: same bug class as #370 (a :through relation + # sourced from a non-belongs_to), just via this pre-existing OneToOneSchema path + # instead of ManyToManySchema -- origin_key/origin_key_target below resolve to both + # models' own primary keys, not a real join column. Out of scope for #370's fix; + # this test pins the current (buggy) behavior, not correct behavior. it 'add has_one_through relation as a to-one (OneToOne)' do collection = described_class.new(datasource, Supplier) @@ -192,6 +197,12 @@ module ForestAdminDatasourceActiveRecord expect(projects_relation.origin_type_value).to be_nil end + # In every `output(...).to_stdout` guard test below, `datasource` must be + # dereferenced for the first time *inside* the `expect` block: Datasource#generate + # builds every collection (and so triggers the warning) once, at construction. If + # something earlier in the example touched `datasource` first, the matcher would + # pass vacuously with no warning to catch -- the exact failure mode a previous + # revision of this fix shipped with. it 'skips a has_many :through whose source reflection is not a belongs_to' do # Parent -> Kid (polymorphic has_many) -> Detail (has_one, custom foreign_key). # Kid#detail is a has_one, so join_foreign_key resolves to Kid's own primary key From 2c26f94f1a4401e2d7a10aa70157c7767a4b1e66 Mon Sep 17 00:00:00 2001 From: Matt Date: Thu, 27 Aug 2026 10:00:26 +0200 Subject: [PATCH 06/10] test(datasource-active-record): reference the tracking issue (#379) --- .../forest_admin_datasource_active_record/collection_spec.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb index ebb84845e..6e8e6086d 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb @@ -47,8 +47,9 @@ module ForestAdminDatasourceActiveRecord # Supplier -> Account -> AccountHistory: same bug class as #370 (a :through relation # sourced from a non-belongs_to), just via this pre-existing OneToOneSchema path # instead of ManyToManySchema -- origin_key/origin_key_target below resolve to both - # models' own primary keys, not a real join column. Out of scope for #370's fix; - # this test pins the current (buggy) behavior, not correct behavior. + # models' own primary keys, not a real join column. Out of scope for #370's fix, + # tracked separately as #379; this test pins the current (buggy) behavior, not + # correct behavior. it 'add has_one_through relation as a to-one (OneToOne)' do collection = described_class.new(datasource, Supplier) From 997d1a7823a0267defc59d2fe470f358b7fcdd27 Mon Sep 17 00:00:00 2001 From: Matt Date: Thu, 27 Aug 2026 10:34:45 +0200 Subject: [PATCH 07/10] fix(datasource-active-record): narrow the guard to genuinely missing FKs, deprecate identity joins instead of dropping them Per Christophe's review: the previous guard (source_reflection.belongs_to?) dropped every non-belongs_to-sourced :through relation, including the common case where join_foreign_key falls back to a primary key that happens to exist on the through collection (an identity join -- wrong data, but not the missing-column problem #370 actually reports). That silently changed the published schema for a much wider set of relations than the issue describes. New criterion: foreign_key_missing_from_through? checks whether join_foreign_key actually names a column on the through collection. - Missing entirely (only possible for nested :through, where the real FK lives further down the chain) -> skip + warn, same as before. This is the only case now dropped from the schema, and it's the one that reproduces #370's exact reported message. - Exists but the source isn't belongs_to (single-hop has_one/has_many sources always land here, since join_foreign_key falls back to a PK that trivially exists) -> publish the exact same ManyToManySchema as today, plus a deprecation warning. Zero change to any published schema; the identity-join cleanup becomes an announced future removal instead of a silent behavior change in a patch release. Consolidated the has_one/has_many branches' near-identical ManyToMany construction into add_many_to_many_field/build_many_to_many_field to avoid duplicating this logic a third time. Reclassifies the existing Kid/Detail, Solo, and Box/Slot/Tag fixtures as bucket-B (still published + deprecated) rather than dropped; only the nested Category/Car/Check fixture is genuinely bucket-A (dropped). Verified all 4 against origin/main's unfixed collection.rb before restoring the fix. --- .../collection.rb | 103 ++++++++++-------- .../collection_spec.rb | 64 ++++++----- 2 files changed, 93 insertions(+), 74 deletions(-) diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb index f2c7d5b5c..8992a3d60 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb @@ -88,13 +88,49 @@ def cascade_on_delete?(association) %i[destroy destroy_async delete_all].include?(association.options[:dependent]) end - # ThroughReflection#join_foreign_key delegates to the source reflection; only - # belongs_to overrides it to return a real FK column. Any other macro falls back - # to the source model's own primary key, producing a bogus identity join (#370). - def foreign_key_on_through_collection?(association) + # join_foreign_key can fall back to the source model's own PK (see + # valid_many_to_many_source? below); that PK usually still exists as a column, so + # "column missing" alone only catches a genuinely broken relation, not every + # identity-join case. + def foreign_key_missing_from_through?(association, through_reflection) + !through_reflection.klass.column_names.include?(association.join_foreign_key) + end + + # True only for a belongs_to source: ThroughReflection#join_foreign_key delegates to + # the source reflection, and only belongs_to overrides it to return a real FK column + # instead of falling back to the source model's own primary key (#370). + def valid_many_to_many_source?(association) association.source_reflection&.belongs_to? end + def build_many_to_many_field(association, through_reflection, is_polymorphic, source_polymorphic) + ForestAdminDatasourceToolkit::Schema::Relations::ManyToManySchema.new( + foreign_collection: format_model_name(association.klass.name), + origin_key: through_reflection.foreign_key, + origin_key_target: through_reflection.join_foreign_key, + foreign_key: association.join_foreign_key, + foreign_key_target: association.association_primary_key, + through_collection: format_model_name(through_reflection.klass.name), + origin_type_field: is_polymorphic ? through_reflection.type : nil, + origin_type_value: is_polymorphic ? @model.name : nil, + foreign_type_field: source_polymorphic ? association.source_reflection.foreign_type : nil, + foreign_type_value: source_polymorphic ? association.options[:source_type] : nil + ) + end + + def add_many_to_many_field(association, through_reflection, is_polymorphic, source_polymorphic) + if foreign_key_missing_from_through?(association, through_reflection) + warn_unrepresentable_many_to_many(association, through_reflection) + return + end + + add_field( + association.name.to_s, + build_many_to_many_field(association, through_reflection, is_polymorphic, source_polymorphic) + ) + warn_deprecated_identity_join(association, through_reflection) unless valid_many_to_many_source?(association) + end + # rubocop:disable Metrics/BlockNesting def fetch_associations associations(@model, support_polymorphic_relations: @support_polymorphic_relations).each do |association| @@ -108,24 +144,8 @@ def fetch_associations association.options[:source_type].present? many_to_many_shape = is_polymorphic || source_polymorphic - if many_to_many_shape && foreign_key_on_through_collection?(association) - add_field( - association.name.to_s, - ForestAdminDatasourceToolkit::Schema::Relations::ManyToManySchema.new( - foreign_collection: format_model_name(association.klass.name), - origin_key: through_reflection.foreign_key, - origin_key_target: through_reflection.join_foreign_key, - foreign_key: association.join_foreign_key, - foreign_key_target: association.association_primary_key, - through_collection: format_model_name(through_reflection.klass.name), - origin_type_field: is_polymorphic ? through_reflection.type : nil, - origin_type_value: is_polymorphic ? @model.name : nil, - foreign_type_field: source_polymorphic ? association.source_reflection.foreign_type : nil, - foreign_type_value: source_polymorphic ? association.options[:source_type] : nil - ) - ) - elsif many_to_many_shape - warn_unrepresentable_many_to_many(association, through_reflection) + if many_to_many_shape + add_many_to_many_field(association, through_reflection, is_polymorphic, source_polymorphic) else add_field( association.name.to_s, @@ -196,25 +216,7 @@ def fetch_associations source_polymorphic = association.source_reflection&.polymorphic? && association.options[:source_type].present? - if foreign_key_on_through_collection?(association) - add_field( - association.name.to_s, - ForestAdminDatasourceToolkit::Schema::Relations::ManyToManySchema.new( - foreign_collection: format_model_name(association.klass.name), - origin_key: through_reflection.foreign_key, - origin_key_target: through_reflection.join_foreign_key, - foreign_key: association.join_foreign_key, - foreign_key_target: association.association_primary_key, - through_collection: format_model_name(through_reflection.klass.name), - origin_type_field: is_polymorphic ? through_reflection.type : nil, - origin_type_value: is_polymorphic ? @model.name : nil, - foreign_type_field: source_polymorphic ? association.source_reflection.foreign_type : nil, - foreign_type_value: source_polymorphic ? association.options[:source_type] : nil - ) - ) - else - warn_unrepresentable_many_to_many(association, through_reflection) - end + add_many_to_many_field(association, through_reflection, is_polymorphic, source_polymorphic) elsif association.inverse_of&.polymorphic? add_field( association.name.to_s, @@ -318,14 +320,23 @@ def create_virtual_habtm_model(association, model_name) end def warn_unrepresentable_many_to_many(association, through_reflection) - source = association.source_reflection logger = ActiveSupport::Logger.new($stdout) logger.warn( "[ForestAdmin] ⚠️ Skipping association '#{association.name}' in model '#{@model.name}': " \ - "its source association '#{source.name}' on '#{format_model_name(through_reflection.klass.name)}' " \ - "is a #{source.macro}, not a belongs_to, so there is no real join column between " \ - "'#{format_model_name(through_reflection.klass.name)}' and '#{format_model_name(association.klass.name)}' " \ - '-- this relation cannot be represented as a Forest Admin many-to-many.' + "its foreign key ('#{association.join_foreign_key}') is not a column of the through " \ + "collection '#{format_model_name(through_reflection.klass.name)}' -- this relation cannot " \ + 'be represented as a Forest Admin many-to-many.' + ) + end + + def warn_deprecated_identity_join(association, through_reflection) + logger = ActiveSupport::Logger.new($stdout) + logger.warn( + "[ForestAdmin] ⚠️ Association '#{association.name}' in model '#{@model.name}' is published " \ + "as a many-to-many joining '#{format_model_name(through_reflection.klass.name)}' and " \ + "'#{format_model_name(association.klass.name)}' by their own primary keys, since its source " \ + 'association is not a belongs_to. This identity join will be removed in a future major ' \ + 'version -- see #370.' ) end diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb index 6e8e6086d..662837103 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb @@ -204,22 +204,13 @@ module ForestAdminDatasourceActiveRecord # something earlier in the example touched `datasource` first, the matcher would # pass vacuously with no warning to catch -- the exact failure mode a previous # revision of this fix shipped with. - it 'skips a has_many :through whose source reflection is not a belongs_to' do - # Parent -> Kid (polymorphic has_many) -> Detail (has_one, custom foreign_key). - # Kid#detail is a has_one, so join_foreign_key resolves to Kid's own primary key - # instead of a real join column -- the relation can't be expressed as ManyToMany. - expect { datasource.get_collection('Parent') }.to output(/Skipping association 'details'/).to_stdout - - expect(datasource.get_collection('Parent').schema[:fields].keys).not_to include('details') - end - - it "skips a nested has_many :through, reproducing #370's exact symptom" do + it "skips a has_many :through whose foreign key is missing from the through collection, reproducing #370's exact symptom" do # Category -> Car (plain has_many) -> Check, through Car's OWN has_many :through # (Car#checks, through: :car_checks). Car.checks's source_reflection is itself a - # ThroughReflection, so #belongs_to? is false and join_foreign_key resolves all - # the way down to CarCheck#check's real FK ("check_id") -- a genuine column name, - # missing from Car specifically (not the generic "id" the single-hop cases above - # fall back to). This is the shape that actually produces #370's reported message. + # ThroughReflection, so join_foreign_key resolves all the way down to CarCheck#check's + # real FK ("check_id") -- a genuine column name, missing from Car specifically (not + # the generic "id" the bucket-B cases below fall back to). This is the shape that + # actually produces #370's reported message, and the only one dropped from the schema. stub_const('NestedThroughProbe', Class.new(ApplicationRecord) do self.table_name = 'categories' self.abstract_class = true @@ -239,24 +230,41 @@ module ForestAdminDatasourceActiveRecord expect(collection.schema[:fields].keys).not_to include('checks') end - it 'skips a polymorphic has_one :through whose source reflection is not a belongs_to' do - # Solo -> Kid (polymorphic has_one) -> Detail (has_one, custom foreign_key): same - # invalid shape as above, through the has_one branch instead of has_many. The - # has_one branch only reaches this guard when the relation is polymorphic -- - # a non-polymorphic has_one :through still falls into the pre-existing - # OneToOneSchema path below, unguarded (same class of bug, out of scope here). - expect { datasource.get_collection('Solo') }.to output(/Skipping association 'detail'/).to_stdout + it 'still publishes, with a deprecation warning, a has_many :through whose foreign key ' \ + 'coincidentally exists on the through collection' do + # Parent -> Kid (polymorphic has_many) -> Detail (has_one, custom foreign_key). Kid#detail + # is a has_one, so join_foreign_key resolves to Kid's own primary key ("id") -- a column + # that trivially exists on every table, so it's not "missing" and the relation is still + # published exactly as before, as an (until now undetected) identity join. Dropping it + # outright would change the published schema for this shape, so it's deprecated instead. + expect { datasource.get_collection('Parent') }.to output(/is published as a many-to-many/).to_stdout + + field = datasource.get_collection('Parent').schema[:fields]['details'] + expect(field).to be_a(Relations::ManyToManySchema) + expect(field.through_collection).to eq('Kid') + expect(field.foreign_key).to eq('id') + end + + it 'still publishes, with a deprecation warning, a polymorphic has_one :through with the same shape' do + # Solo -> Kid (polymorphic has_one) -> Detail: same bucket-B shape as above, through the + # has_one branch. A non-polymorphic has_one :through isn't reached by this guard at all -- + # it falls into the pre-existing OneToOneSchema path, unguarded (tracked as #379). + expect { datasource.get_collection('Solo') }.to output(/is published as a many-to-many/).to_stdout - expect(datasource.get_collection('Solo').schema[:fields].keys).not_to include('detail') + field = datasource.get_collection('Solo').schema[:fields]['detail'] + expect(field).to be_a(Relations::ManyToManySchema) + expect(field.through_collection).to eq('Kid') end - it 'skips a non-polymorphic has_many :through whose source reflection is not a belongs_to' do - # Box -> Slot (plain has_many, no polymorphism at all) -> Tag (has_one, custom - # foreign_key). Unlike the has_one branch above, the has_many branch has no - # polymorphic gate: this guard applies to ordinary has_many :through chains too. - expect { datasource.get_collection('Box') }.to output(/Skipping association 'tags'/).to_stdout + it 'still publishes, with a deprecation warning, a non-polymorphic has_many :through with the same shape' do + # Box -> Slot (plain has_many, no polymorphism at all) -> Tag: same bucket-B shape, + # showing the has_many branch has no polymorphic gate -- it reaches this guard + # unconditionally, unlike the has_one branch above. + expect { datasource.get_collection('Box') }.to output(/is published as a many-to-many/).to_stdout - expect(datasource.get_collection('Box').schema[:fields].keys).not_to include('tags') + field = datasource.get_collection('Box').schema[:fields]['tags'] + expect(field).to be_a(Relations::ManyToManySchema) + expect(field.through_collection).to eq('Slot') end # rubocop:disable RSpec/ExampleLength From 6ddd1ca5c220a2fee41c7fb1180025236eebeb4d Mon Sep 17 00:00:00 2001 From: Matt Date: Thu, 27 Aug 2026 10:54:14 +0200 Subject: [PATCH 08/10] fix(datasource-active-record): handle composite foreign keys and inaccurate warning messages in the #370 many-to-many guard Two issues found in review after the previous push: - foreign_key_missing_from_through? compared join_foreign_key (which can be an Array on a composite/query_constraints key) against column_names via a plain #include?, always false for an Array -- silently dropping valid belongs_to-sourced relations with composite keys, exactly the kind of schema-narrowing beyond #370 a prior revision was rejected for. - warn_deprecated_identity_join claimed the relation is joined "by their own primary keys", which is false whenever the fallback FK honors a custom primary_key: option or is itself composite. Rewritten to name the actual columns instead. Also corrects two comments that misattributed the mechanism (collection.rb's valid_many_to_many_source?, and the pre-existing Supplier#account_history test, which is NOT sourced from a non-belongs_to despite the old comment's claim), strengthens the three bucket-B tests to assert full field equality instead of 2-3 attributes, and adds a composite-foreign-key regression test plus a negative (no-warning-on-belongs_to) test. 197 examples, 0 failures. Rubocop clean. Co-Authored-By: Claude Sonnet 5 --- .../collection.rb | 25 ++-- .../collection_spec.rb | 134 +++++++++++++++--- 2 files changed, 131 insertions(+), 28 deletions(-) diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb index 8992a3d60..972991db6 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb @@ -88,17 +88,20 @@ def cascade_on_delete?(association) %i[destroy destroy_async delete_all].include?(association.options[:dependent]) end - # join_foreign_key can fall back to the source model's own PK (see + # join_foreign_key can fall back to the through model's own PK (see # valid_many_to_many_source? below); that PK usually still exists as a column, so # "column missing" alone only catches a genuinely broken relation, not every - # identity-join case. + # identity-join case. join_foreign_key can also be an Array on a composite key. def foreign_key_missing_from_through?(association, through_reflection) - !through_reflection.klass.column_names.include?(association.join_foreign_key) + columns = through_reflection.klass.column_names + Array(association.join_foreign_key).any? { |key| !columns.include?(key) } end # True only for a belongs_to source: ThroughReflection#join_foreign_key delegates to - # the source reflection, and only belongs_to overrides it to return a real FK column - # instead of falling back to the source model's own primary key (#370). + # the source reflection, and only belongs_to overrides it to return a real FK column. + # Any other source macro falls back to AssociationReflection#active_record_primary_key + # -- the through model's own primary key column (honoring a custom primary_key:, and + # possibly composite), not a genuine join column (#370). def valid_many_to_many_source?(association) association.source_reflection&.belongs_to? end @@ -321,9 +324,10 @@ def create_virtual_habtm_model(association, model_name) def warn_unrepresentable_many_to_many(association, through_reflection) logger = ActiveSupport::Logger.new($stdout) + keys = Array(association.join_foreign_key).join(', ') logger.warn( "[ForestAdmin] ⚠️ Skipping association '#{association.name}' in model '#{@model.name}': " \ - "its foreign key ('#{association.join_foreign_key}') is not a column of the through " \ + "its foreign key ('#{keys}') is not a column of the through " \ "collection '#{format_model_name(through_reflection.klass.name)}' -- this relation cannot " \ 'be represented as a Forest Admin many-to-many.' ) @@ -333,10 +337,11 @@ def warn_deprecated_identity_join(association, through_reflection) logger = ActiveSupport::Logger.new($stdout) logger.warn( "[ForestAdmin] ⚠️ Association '#{association.name}' in model '#{@model.name}' is published " \ - "as a many-to-many joining '#{format_model_name(through_reflection.klass.name)}' and " \ - "'#{format_model_name(association.klass.name)}' by their own primary keys, since its source " \ - 'association is not a belongs_to. This identity join will be removed in a future major ' \ - 'version -- see #370.' + "as a many-to-many joining '#{format_model_name(through_reflection.klass.name)}'." \ + "'#{association.join_foreign_key}' to " \ + "'#{format_model_name(association.klass.name)}'.'#{association.association_primary_key}', " \ + 'since its source association is not a belongs_to. This identity join will be removed ' \ + 'in a future major version -- see #370.' ) end diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb index 662837103..a5440bc1a 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb @@ -33,6 +33,24 @@ module ForestAdminDatasourceActiveRecord expect(collection.schema[:fields].keys).to include('category', 'user', 'car_checks', 'checks') end + it 'builds a full ManyToManySchema for a has_many :through sourced from a belongs_to' do + field = collection.schema[:fields]['checks'] + + expect(field).to have_attributes( + class: Relations::ManyToManySchema, + foreign_collection: 'Check', + origin_key: 'car_id', + origin_key_target: 'id', + through_collection: 'CarCheck', + foreign_key: 'check_id', + foreign_key_target: 'id', + origin_type_field: nil, + origin_type_value: nil, + foreign_type_field: nil, + foreign_type_value: nil + ) + end + it 'do not add polymorphic relations' do expect(datasource.get_collection('User').schema[:fields].keys).not_to include('address') expect(datasource.get_collection('Address').schema[:fields].keys).not_to include('addressable') @@ -44,12 +62,14 @@ module ForestAdminDatasourceActiveRecord expect(collection.schema[:fields].keys).to include('users') end - # Supplier -> Account -> AccountHistory: same bug class as #370 (a :through relation - # sourced from a non-belongs_to), just via this pre-existing OneToOneSchema path - # instead of ManyToManySchema -- origin_key/origin_key_target below resolve to both - # models' own primary keys, not a real join column. Out of scope for #370's fix, - # tracked separately as #379; this test pins the current (buggy) behavior, not - # correct behavior. + # Supplier -> Account -> AccountHistory is a has_one :through that isn't polymorphic, + # so it never reaches the many_to_many_shape guard above -- it falls into this plain + # OneToOneSchema branch instead, which hardcodes {AccountHistory,Supplier}'s own + # primary keys unconditionally, regardless of the real FK or of whether the source is + # a belongs_to (here it actually is one; account_history_id is a real column on + # accounts, just never consulted by this branch). Unrelated to #370's guard, tracked + # separately as #379; this test pins current (arguably wrong) behavior, not correct + # behavior. it 'add has_one_through relation as a to-one (OneToOne)' do collection = described_class.new(datasource, Supplier) @@ -206,11 +226,11 @@ module ForestAdminDatasourceActiveRecord # revision of this fix shipped with. it "skips a has_many :through whose foreign key is missing from the through collection, reproducing #370's exact symptom" do # Category -> Car (plain has_many) -> Check, through Car's OWN has_many :through - # (Car#checks, through: :car_checks). Car.checks's source_reflection is itself a - # ThroughReflection, so join_foreign_key resolves all the way down to CarCheck#check's - # real FK ("check_id") -- a genuine column name, missing from Car specifically (not - # the generic "id" the bucket-B cases below fall back to). This is the shape that - # actually produces #370's reported message, and the only one dropped from the schema. + # (Car#checks, through: :car_checks). NestedThroughProbe#checks's source_reflection + # is Car#checks, which is itself a ThroughReflection, so join_foreign_key resolves + # all the way down to CarCheck#check's real FK ("check_id") -- a genuine column + # name, missing from Car specifically (not the generic "id" the bucket-B cases below + # fall back to). This is the shape that actually produces #370's reported message. stub_const('NestedThroughProbe', Class.new(ApplicationRecord) do self.table_name = 'categories' self.abstract_class = true @@ -240,9 +260,19 @@ module ForestAdminDatasourceActiveRecord expect { datasource.get_collection('Parent') }.to output(/is published as a many-to-many/).to_stdout field = datasource.get_collection('Parent').schema[:fields]['details'] - expect(field).to be_a(Relations::ManyToManySchema) - expect(field.through_collection).to eq('Kid') - expect(field.foreign_key).to eq('id') + expect(field).to have_attributes( + class: Relations::ManyToManySchema, + foreign_collection: 'Detail', + origin_key: 'owner_id', + origin_key_target: 'id', + through_collection: 'Kid', + foreign_key: 'id', + foreign_key_target: 'id', + origin_type_field: 'owner_type', + origin_type_value: 'Parent', + foreign_type_field: nil, + foreign_type_value: nil + ) end it 'still publishes, with a deprecation warning, a polymorphic has_one :through with the same shape' do @@ -252,8 +282,19 @@ module ForestAdminDatasourceActiveRecord expect { datasource.get_collection('Solo') }.to output(/is published as a many-to-many/).to_stdout field = datasource.get_collection('Solo').schema[:fields]['detail'] - expect(field).to be_a(Relations::ManyToManySchema) - expect(field.through_collection).to eq('Kid') + expect(field).to have_attributes( + class: Relations::ManyToManySchema, + foreign_collection: 'Detail', + origin_key: 'owner_id', + origin_key_target: 'id', + through_collection: 'Kid', + foreign_key: 'id', + foreign_key_target: 'id', + origin_type_field: 'owner_type', + origin_type_value: 'Solo', + foreign_type_field: nil, + foreign_type_value: nil + ) end it 'still publishes, with a deprecation warning, a non-polymorphic has_many :through with the same shape' do @@ -263,8 +304,65 @@ module ForestAdminDatasourceActiveRecord expect { datasource.get_collection('Box') }.to output(/is published as a many-to-many/).to_stdout field = datasource.get_collection('Box').schema[:fields]['tags'] - expect(field).to be_a(Relations::ManyToManySchema) - expect(field.through_collection).to eq('Slot') + expect(field).to have_attributes( + class: Relations::ManyToManySchema, + foreign_collection: 'Tag', + origin_key: 'box_id', + origin_key_target: 'id', + through_collection: 'Slot', + foreign_key: 'id', + foreign_key_target: 'id', + origin_type_field: nil, + origin_type_value: nil, + foreign_type_field: nil, + foreign_type_value: nil + ) + end + + it 'does not warn when a many-to-many :through source is a belongs_to' do + # datasource builds every collection eagerly on first dereference (see the guard + # comment above), so a bare `not_to output(/is published as a many-to-many/)` would + # also pick up Box/Parent/Solo's own deprecation warnings from that same eager build. + # Naming the exact association+model excludes those unrelated warnings. + expect { datasource.get_collection('User') } + .not_to output(/Association 'projects' in model 'User' is published as a many-to-many/).to_stdout + end + + it 'still publishes a has_many :through with a composite (array) foreign key on a belongs_to source' do + # join_foreign_key can be an Array, not just a String, whenever the belongs_to source + # declares a composite foreign_key (or query_constraints). column_names.include?(array) + # is always false, so a naive "missing" check misclassifies this as unrepresentable and + # drops a genuinely valid relation -- verified this fixture gets dropped without the + # Array(...) handling in foreign_key_missing_from_through?. + stub_const('CompositeLeaf', Class.new(ApplicationRecord) do + self.table_name = 'cars' + self.primary_key = %w[category_id reference] + self.abstract_class = true + end) + + stub_const('CompositeThrough', Class.new(ApplicationRecord) do + self.table_name = 'car_checks' + self.abstract_class = true + + belongs_to :leaf, class_name: 'CompositeLeaf', foreign_key: %w[car_id check_id], + primary_key: %w[category_id reference] + end) + + stub_const('CompositeRoot', Class.new(ApplicationRecord) do + self.table_name = 'categories' + self.abstract_class = true + + has_many :throughs, class_name: 'CompositeThrough', foreign_key: :car_id + has_many :leaves, through: :throughs, source: :leaf + end) + + association = CompositeRoot.reflect_on_association(:leaves) + expect(association.join_foreign_key).to eq(%w[car_id check_id]) + + collection = described_class.new(datasource, CompositeRoot, support_polymorphic_relations: true) + + field = collection.schema[:fields]['leaves'] + expect(field).to have_attributes(class: Relations::ManyToManySchema, foreign_key: %w[car_id check_id]) end # rubocop:disable RSpec/ExampleLength From 16eaf2e5a609df9d1b97fb9ac28715be8f9314ae Mon Sep 17 00:00:00 2001 From: Matt Date: Thu, 27 Aug 2026 11:31:21 +0200 Subject: [PATCH 09/10] fix(datasource-active-record): normalize composite keys in the deprecation warning message warn_deprecated_identity_join interpolated association.join_foreign_key / association.association_primary_key raw -- both can be Arrays on a composite key, rendering Ruby's inspect syntax (e.g. '["a", "b"]') in the message instead of a clean key list. Normalize the same way warn_unrepresentable_many_to_many already does. Strengthens the composite-foreign-key test to assert full field equality instead of just class + foreign_key, pinning (not endorsing) the mangled foreign_key_target inherited from Rails' ThroughReflection# association_primary_key (calls .to_s on a composite target, unlike its AssociationReflection sibling which maps to_s over each key). Adds a second test pinning the deprecation message's own rendering for an Array-valued key, verified to fail against the pre-fix raw interpolation. 198 examples, 0 failures. Rubocop clean. Co-Authored-By: Claude Sonnet 5 --- .../collection.rb | 6 ++- .../collection_spec.rb | 48 ++++++++++++++++++- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb index 972991db6..74eb62c9f 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb @@ -335,11 +335,13 @@ def warn_unrepresentable_many_to_many(association, through_reflection) def warn_deprecated_identity_join(association, through_reflection) logger = ActiveSupport::Logger.new($stdout) + foreign_key = Array(association.join_foreign_key).join(', ') + foreign_key_target = Array(association.association_primary_key).join(', ') logger.warn( "[ForestAdmin] ⚠️ Association '#{association.name}' in model '#{@model.name}' is published " \ "as a many-to-many joining '#{format_model_name(through_reflection.klass.name)}'." \ - "'#{association.join_foreign_key}' to " \ - "'#{format_model_name(association.klass.name)}'.'#{association.association_primary_key}', " \ + "'#{foreign_key}' to " \ + "'#{format_model_name(association.klass.name)}'.'#{foreign_key_target}', " \ 'since its source association is not a belongs_to. This identity join will be removed ' \ 'in a future major version -- see #370.' ) diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb index a5440bc1a..916beba34 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb @@ -361,8 +361,54 @@ module ForestAdminDatasourceActiveRecord collection = described_class.new(datasource, CompositeRoot, support_polymorphic_relations: true) + # This pins that foreign_key_target comes out mangled -- not that it's usable. + # association_primary_key stringifies a composite target (ThroughReflection# + # association_primary_key calls .to_s), so foreign_key_target is this mangled + # string rather than a real column reference; that's a pre-existing (main-inherited) + # limitation being pinned here, not endorsed. field = collection.schema[:fields]['leaves'] - expect(field).to have_attributes(class: Relations::ManyToManySchema, foreign_key: %w[car_id check_id]) + expect(field).to have_attributes( + class: Relations::ManyToManySchema, + origin_key: 'car_id', + origin_key_target: 'id', + through_collection: 'CompositeThrough', + foreign_key: %w[car_id check_id], + foreign_key_target: '["category_id", "reference"]' + ) + end + + it 'renders a composite (array) foreign key as a clean list in the deprecation warning' do + # Same Array-valued join_foreign_key case as the belongs_to fixture above, but + # sourced from a plain has_one (not belongs_to) so it reaches + # warn_deprecated_identity_join instead -- pins that this message normalizes the + # Array too, not just warn_unrepresentable_many_to_many. + stub_const('ArrayLeaf', Class.new(ApplicationRecord) do + self.table_name = 'users' + self.abstract_class = true + end) + + stub_const('ArrayThrough', Class.new(ApplicationRecord) do + self.table_name = 'cars' + self.primary_key = %w[category_id reference] + self.abstract_class = true + + has_one :leaf, class_name: 'ArrayLeaf', foreign_key: :id + end) + + stub_const('ArrayRoot', Class.new(ApplicationRecord) do + self.table_name = 'categories' + self.abstract_class = true + + has_many :throughs, class_name: 'ArrayThrough', foreign_key: :category_id + has_many :leaves, through: :throughs, source: :leaf + end) + + association = ArrayRoot.reflect_on_association(:leaves) + expect(association.join_foreign_key).to eq(%w[category_id reference]) + + expect do + described_class.new(datasource, ArrayRoot, support_polymorphic_relations: true) + end.to output(/joining 'ArrayThrough'\.'category_id, reference' to 'ArrayLeaf'\.'id'/).to_stdout end # rubocop:disable RSpec/ExampleLength From 76384ef78e012c03e1d3f87cbae583371121e0e4 Mon Sep 17 00:00:00 2001 From: Matt Date: Thu, 27 Aug 2026 14:58:42 +0200 Subject: [PATCH 10/10] fix(datasource-active-record): validate every many-to-many join key by column existence, not just Array-ness; downgrade the identity-join warning to a diagnostic Addresses christophebrun-forest's 3rd review round on PR #378: - unrepresentable_many_to_many? (renamed from foreign_key_missing_from_ through?) now checks all three keys GeneratorField#build_many_to_many_ schema actually dereferences -- join_foreign_key and origin_key against the through collection, association_primary_key (foreign_key_target) against the foreign collection -- by column existence rather than by Array-ness. A naive `.is_a?(Array)` check (added for the prior round's composite-key fix) misses the case where a composite target reaches us already mangled into a String by ThroughReflection#association_primary_ key (e.g. '["a", "b"]') whenever the source declares an explicit array primary_key: option -- never a real column, but not an Array either. Checking column membership catches both forms uniformly, and also fixes a second gap: origin_key (through_reflection.foreign_key) was never checked at all, so a composite root-to-through key -- independent of the source association's type -- sailed through unguarded. - warn_unrepresentable_many_to_many no longer blames "the through collection" unconditionally: association_primary_key actually resolves against the foreign collection, so the old wording was a false claim in that case (verified: it named an innocent through collection while the real offender was the foreign collection's composite PK). - warn_deprecated_identity_join (renamed warn_identity_join) drops the false "will be removed in a future major version" promise. The relations it covers are common, valid Rails associations (any has_many/ has_one :through sourced from a plain has_one/has_many) and are kept published on purpose to avoid narrowing the schema -- promising removal contradicted that decision and gave readers no actionable migration path. Reframed as a factual diagnostic; #370 kept only as background. - Removed PR-review-thread/reviewer-handle references from comments (process scaffolding that goes stale post-squash); kept the underlying "why" and the durable issue number. 199 examples, 0 failures. Rubocop clean. Co-Authored-By: Claude Sonnet 5 --- .../collection.rb | 51 +++++--- .../collection_spec.rb | 123 ++++++++++++------ 2 files changed, 116 insertions(+), 58 deletions(-) diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb index 74eb62c9f..674a9058d 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb @@ -90,11 +90,19 @@ def cascade_on_delete?(association) # join_foreign_key can fall back to the through model's own PK (see # valid_many_to_many_source? below); that PK usually still exists as a column, so - # "column missing" alone only catches a genuinely broken relation, not every - # identity-join case. join_foreign_key can also be an Array on a composite key. - def foreign_key_missing_from_through?(association, through_reflection) - columns = through_reflection.klass.column_names - Array(association.join_foreign_key).any? { |key| !columns.include?(key) } + # "column missing" alone doesn't catch every unrepresentable case -- a composite key is + # unrepresentable too, even when every individual column exists: GeneratorField looks + # fields up by the raw key, so a composite key never resolves. A composite key can reach + # us either as a real Array, or (via an explicit primary_key: option on the source) + # already flattened by Rails into a mangled String like '["a", "b"]' -- neither is ever a + # real column name, so checking column membership catches both forms uniformly, on + # whichever collection each key is actually resolved against downstream (#370). + def unrepresentable_many_to_many?(association, through_reflection) + [ + [association.join_foreign_key, through_reflection.klass], + [through_reflection.foreign_key, through_reflection.klass], + [association.association_primary_key, association.klass] + ].any? { |key, klass| !klass.column_names.include?(key) } end # True only for a belongs_to source: ThroughReflection#join_foreign_key delegates to @@ -122,7 +130,7 @@ def build_many_to_many_field(association, through_reflection, is_polymorphic, so end def add_many_to_many_field(association, through_reflection, is_polymorphic, source_polymorphic) - if foreign_key_missing_from_through?(association, through_reflection) + if unrepresentable_many_to_many?(association, through_reflection) warn_unrepresentable_many_to_many(association, through_reflection) return end @@ -131,7 +139,7 @@ def add_many_to_many_field(association, through_reflection, is_polymorphic, sour association.name.to_s, build_many_to_many_field(association, through_reflection, is_polymorphic, source_polymorphic) ) - warn_deprecated_identity_join(association, through_reflection) unless valid_many_to_many_source?(association) + warn_identity_join(association, through_reflection) unless valid_many_to_many_source?(association) end # rubocop:disable Metrics/BlockNesting @@ -324,26 +332,33 @@ def create_virtual_habtm_model(association, model_name) def warn_unrepresentable_many_to_many(association, through_reflection) logger = ActiveSupport::Logger.new($stdout) - keys = Array(association.join_foreign_key).join(', ') logger.warn( "[ForestAdmin] ⚠️ Skipping association '#{association.name}' in model '#{@model.name}': " \ - "its foreign key ('#{keys}') is not a column of the through " \ - "collection '#{format_model_name(through_reflection.klass.name)}' -- this relation cannot " \ - 'be represented as a Forest Admin many-to-many.' + 'this relation cannot be represented as a Forest Admin many-to-many -- its join keys must ' \ + 'each be a single existing column, on the through collection ' \ + "'#{format_model_name(through_reflection.klass.name)}' or the foreign collection " \ + "'#{format_model_name(association.klass.name)}' (composite/query_constraints keys are " \ + 'not supported).' ) end - def warn_deprecated_identity_join(association, through_reflection) + # join_foreign_key/association_primary_key are guaranteed to be single, real, existing + # columns here: unrepresentable_many_to_many? above already checks column membership for + # both (and for origin_key) before this is ever called. + # + # Deliberately a diagnostic, not a deprecation notice: this shape is common (any + # has_many/has_one :through sourced from a plain has_one/has_many, not just polymorphic + # ones) and the relation is kept published to avoid narrowing the schema -- see #370. + # No removal is promised or planned. + def warn_identity_join(association, through_reflection) logger = ActiveSupport::Logger.new($stdout) - foreign_key = Array(association.join_foreign_key).join(', ') - foreign_key_target = Array(association.association_primary_key).join(', ') logger.warn( "[ForestAdmin] ⚠️ Association '#{association.name}' in model '#{@model.name}' is published " \ "as a many-to-many joining '#{format_model_name(through_reflection.klass.name)}'." \ - "'#{foreign_key}' to " \ - "'#{format_model_name(association.klass.name)}'.'#{foreign_key_target}', " \ - 'since its source association is not a belongs_to. This identity join will be removed ' \ - 'in a future major version -- see #370.' + "'#{association.join_foreign_key}' to " \ + "'#{format_model_name(association.klass.name)}'.'#{association.association_primary_key}': " \ + "its source association is not a belongs_to, so this join uses each side's own identifier " \ + 'rather than a dedicated foreign key column -- see #370 for background.' ) end diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb index 916beba34..ce7b97c70 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb @@ -250,13 +250,13 @@ module ForestAdminDatasourceActiveRecord expect(collection.schema[:fields].keys).not_to include('checks') end - it 'still publishes, with a deprecation warning, a has_many :through whose foreign key ' \ + it 'still publishes, with a diagnostic warning, a has_many :through whose foreign key ' \ 'coincidentally exists on the through collection' do # Parent -> Kid (polymorphic has_many) -> Detail (has_one, custom foreign_key). Kid#detail # is a has_one, so join_foreign_key resolves to Kid's own primary key ("id") -- a column # that trivially exists on every table, so it's not "missing" and the relation is still # published exactly as before, as an (until now undetected) identity join. Dropping it - # outright would change the published schema for this shape, so it's deprecated instead. + # outright would change the published schema for this shape, so it's flagged instead. expect { datasource.get_collection('Parent') }.to output(/is published as a many-to-many/).to_stdout field = datasource.get_collection('Parent').schema[:fields]['details'] @@ -275,7 +275,7 @@ module ForestAdminDatasourceActiveRecord ) end - it 'still publishes, with a deprecation warning, a polymorphic has_one :through with the same shape' do + it 'still publishes, with a diagnostic warning, a polymorphic has_one :through with the same shape' do # Solo -> Kid (polymorphic has_one) -> Detail: same bucket-B shape as above, through the # has_one branch. A non-polymorphic has_one :through isn't reached by this guard at all -- # it falls into the pre-existing OneToOneSchema path, unguarded (tracked as #379). @@ -297,7 +297,7 @@ module ForestAdminDatasourceActiveRecord ) end - it 'still publishes, with a deprecation warning, a non-polymorphic has_many :through with the same shape' do + it 'still publishes, with a diagnostic warning, a non-polymorphic has_many :through with the same shape' do # Box -> Slot (plain has_many, no polymorphism at all) -> Tag: same bucket-B shape, # showing the has_many branch has no polymorphic gate -- it reaches this guard # unconditionally, unlike the has_one branch above. @@ -322,18 +322,20 @@ module ForestAdminDatasourceActiveRecord it 'does not warn when a many-to-many :through source is a belongs_to' do # datasource builds every collection eagerly on first dereference (see the guard # comment above), so a bare `not_to output(/is published as a many-to-many/)` would - # also pick up Box/Parent/Solo's own deprecation warnings from that same eager build. + # also pick up Box/Parent/Solo's own identity-join warnings from that same eager build. # Naming the exact association+model excludes those unrelated warnings. expect { datasource.get_collection('User') } .not_to output(/Association 'projects' in model 'User' is published as a many-to-many/).to_stdout end - it 'still publishes a has_many :through with a composite (array) foreign key on a belongs_to source' do + it 'skips a has_many :through whose foreign key is composite, since a composite key ' \ + 'crashes schema:generate downstream even when every column exists' do # join_foreign_key can be an Array, not just a String, whenever the belongs_to source - # declares a composite foreign_key (or query_constraints). column_names.include?(array) - # is always false, so a naive "missing" check misclassifies this as unrepresentable and - # drops a genuinely valid relation -- verified this fixture gets dropped without the - # Array(...) handling in foreign_key_missing_from_through?. + # declares a composite foreign_key (or query_constraints). GeneratorField looks fields + # up by the raw key (never resolves for an Array) and stringifies a composite + # foreign_key_target instead of naming real columns -- both crash schema:generate on + # this exact fixture (confirmed pre-existing on main too, so skipping it regresses + # nothing that ever worked, see #370). stub_const('CompositeLeaf', Class.new(ApplicationRecord) do self.table_name = 'cars' self.primary_key = %w[category_id reference] @@ -359,56 +361,97 @@ module ForestAdminDatasourceActiveRecord association = CompositeRoot.reflect_on_association(:leaves) expect(association.join_foreign_key).to eq(%w[car_id check_id]) - collection = described_class.new(datasource, CompositeRoot, support_polymorphic_relations: true) + collection = nil + expect do + collection = described_class.new(datasource, CompositeRoot, support_polymorphic_relations: true) + end.to output(/Skipping association 'leaves'/).to_stdout - # This pins that foreign_key_target comes out mangled -- not that it's usable. - # association_primary_key stringifies a composite target (ThroughReflection# - # association_primary_key calls .to_s), so foreign_key_target is this mangled - # string rather than a real column reference; that's a pre-existing (main-inherited) - # limitation being pinned here, not endorsed. - field = collection.schema[:fields]['leaves'] - expect(field).to have_attributes( - class: Relations::ManyToManySchema, - origin_key: 'car_id', - origin_key_target: 'id', - through_collection: 'CompositeThrough', - foreign_key: %w[car_id check_id], - foreign_key_target: '["category_id", "reference"]' - ) + expect(collection.schema[:fields].keys).not_to include('leaves') end - it 'renders a composite (array) foreign key as a clean list in the deprecation warning' do - # Same Array-valued join_foreign_key case as the belongs_to fixture above, but - # sourced from a plain has_one (not belongs_to) so it reaches - # warn_deprecated_identity_join instead -- pins that this message normalizes the - # Array too, not just warn_unrepresentable_many_to_many. - stub_const('ArrayLeaf', Class.new(ApplicationRecord) do - self.table_name = 'users' + it 'skips a has_many :through whose origin_key (the root-to-through key, not the ' \ + 'source association) is composite' do + # through_reflection.foreign_key (origin_key) can independently be an Array, driven by + # the ROOT's own has_many :through declaration rather than by the source association -- + # unrepresentable_many_to_many? must check it too, or a composite origin_key sails + # through with a perfectly normal (String) join_foreign_key and crashes + # GeneratorField#build_many_to_many_schema on origin_key instead (#370). + stub_const('OriginKeyLeaf', Class.new(ApplicationRecord) do + self.table_name = 'checks' self.abstract_class = true end) - stub_const('ArrayThrough', Class.new(ApplicationRecord) do + stub_const('OriginKeyThrough', Class.new(ApplicationRecord) do + self.table_name = 'car_checks' + self.abstract_class = true + + belongs_to :leaf, class_name: 'OriginKeyLeaf', foreign_key: :check_id + end) + + stub_const('OriginKeyRoot', Class.new(ApplicationRecord) do self.table_name = 'cars' self.primary_key = %w[category_id reference] self.abstract_class = true - has_one :leaf, class_name: 'ArrayLeaf', foreign_key: :id + has_many :throughs, class_name: 'OriginKeyThrough', foreign_key: %w[car_id check_id] + has_many :leaves, through: :throughs, source: :leaf end) - stub_const('ArrayRoot', Class.new(ApplicationRecord) do + association = OriginKeyRoot.reflect_on_association(:leaves) + expect(association.join_foreign_key).to eq('check_id') + expect(association.through_reflection.foreign_key).to eq(%w[car_id check_id]) + + collection = nil + expect do + collection = described_class.new(datasource, OriginKeyRoot, support_polymorphic_relations: true) + end.to output(/Skipping association 'leaves'/).to_stdout + + expect(collection.schema[:fields].keys).not_to include('leaves') + end + + it 'skips a has_many :through whose foreign_key_target is composite, even when it ' \ + 'reaches us as a mangled String rather than a real Array, and even when ' \ + 'join_foreign_key/origin_key are both fine' do + # association_primary_key can be composite two different ways: a genuine Array (the + # foreign class's own composite primary_key, no override), or -- as here -- a String + # that LOOKS scalar but is actually Array#to_s'd by ThroughReflection# + # association_primary_key whenever the source declares an explicit array primary_key: + # option. A naive `.is_a?(Array)` check only catches the first form; checking column + # membership catches both, since neither is ever a real column name. join_foreign_key + # and origin_key are both deliberately valid here, to isolate this one check. + stub_const('MangledLeaf', Class.new(ApplicationRecord) do + self.table_name = 'cars' + self.abstract_class = true + end) + + stub_const('MangledThrough', Class.new(ApplicationRecord) do + self.table_name = 'car_checks' + self.abstract_class = true + + belongs_to :leaf, class_name: 'MangledLeaf', foreign_key: :check_id, + primary_key: %w[category_id reference], optional: true + end) + + stub_const('MangledRoot', Class.new(ApplicationRecord) do self.table_name = 'categories' self.abstract_class = true - has_many :throughs, class_name: 'ArrayThrough', foreign_key: :category_id + has_many :throughs, class_name: 'MangledThrough', foreign_key: :car_id has_many :leaves, through: :throughs, source: :leaf end) - association = ArrayRoot.reflect_on_association(:leaves) - expect(association.join_foreign_key).to eq(%w[category_id reference]) + association = MangledRoot.reflect_on_association(:leaves) + expect(association.join_foreign_key).to eq('check_id') + expect(association.through_reflection.foreign_key).to eq('car_id') + expect(association.association_primary_key).to be_a(String) + expect(association.association_primary_key).to eq('["category_id", "reference"]') + collection = nil expect do - described_class.new(datasource, ArrayRoot, support_polymorphic_relations: true) - end.to output(/joining 'ArrayThrough'\.'category_id, reference' to 'ArrayLeaf'\.'id'/).to_stdout + collection = described_class.new(datasource, MangledRoot, support_polymorphic_relations: true) + end.to output(/Skipping association 'leaves'/).to_stdout + + expect(collection.schema[:fields].keys).not_to include('leaves') end # rubocop:disable RSpec/ExampleLength