From ebc2dfec678da6091d74bf6c7cbab12a917f8f4b Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Thu, 2 Jul 2026 15:49:42 +0200 Subject: [PATCH 1/3] feat(datasource-active-record): expose has_one :through as a to-one relation Map a `has_one :through` association to a OneToOne (HasOne) schema instead of ManyToMany, so it renders as a to-one relation (matching forest_liana v1) rather than a to-many list. ActiveRecord resolves the join natively via the association name, so read, filter and aggregate all work through the relation. Resolve it efficiently too: when every hop of the through chain is a belongs_to (no scope/default_scope, same database), fold it into the existing to-one LEFT JOIN instead of issuing per-hop preload queries. When it does fall back to preload (e.g. the same relation is also used by a filter), the intermediate foreign key is now selected so ActiveRecord can still load it. BREAKING CHANGE: a `has_one :through` relation is now exposed as a to-one (HasOne) relation instead of a to-many (BelongsToMany). Agents that reference such a relation as a collection (segments, smart views, permissions) must be updated to treat it as a to-one. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../collection.rb | 39 +++++++----- .../utils/query.rb | 45 ++++++++++++-- .../spec/dummy/app/models/account.rb | 1 + .../spec/dummy/app/models/account_history.rb | 1 + ...26100004_add_order_to_account_histories.rb | 5 ++ .../collection_spec.rb | 8 ++- .../utils/join_to_one_optimization_spec.rb | 59 +++++++++++++++++++ 7 files changed, 138 insertions(+), 20 deletions(-) create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20251126100004_add_order_to_account_histories.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 d6bdcedc1..f5f9b77c2 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 @@ -100,21 +100,32 @@ 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 is_polymorphic || source_polymorphic + 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 + add_field( + association.name.to_s, + ForestAdminDatasourceToolkit::Schema::Relations::OneToOneSchema.new( + foreign_collection: format_model_name(association.klass.name), + origin_key: association.klass.primary_key, + origin_key_target: @model.primary_key + ) + ) + end elsif association.inverse_of&.polymorphic? add_field( association.name.to_s, diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb index 6462fed7f..9a15cf19e 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb @@ -196,6 +196,10 @@ def build_select(collection, projection) if collection.model.table_name == @collection.model.table_name if one_to_one_relations.include?(relation_schema.type) @select << "#{collection.model.table_name}.#{relation_schema.origin_key_target}" + # a has_one :through preloads via the intermediate FK on this table; select it so a + # relation that falls back to preload (e.g. it is also used by a filter) still loads. + through_fk = through_foreign_key(collection, relation_name) + @select << "#{collection.model.table_name}.#{through_fk}" if through_fk elsif many_to_one_relations.include?(relation_schema.type) @select << "#{collection.model.table_name}.#{relation_schema.foreign_key}" end @@ -285,18 +289,26 @@ def joinable_tables(collection, relation_name, sub_projection, used_tables) tables end - # The target collection when this hop is safe to collapse into a JOIN, else nil (-> preload). + # The target collection when this single hop is safe to collapse into a LEFT OUTER JOIN, + # else nil. A belongs_to qualifies: it matches on the target's primary key, so the JOIN + # cannot duplicate the parent row (a plain has_one child may not be unique). A has_one :through + # also qualifies when every hop is a belongs_to (see belongs_to_chain_through?): AR resolves the + # intermediate join, saving the extra per-hop preload queries. def joinable_target(collection, relation_name, used_tables) relation_schema = collection.schema[:fields][relation_name] - # belongs_to only: it joins on the target's primary key, so it can't duplicate the parent - # (a has_one child may not be unique) - return unless relation_schema.type == 'ManyToOne' && relation_schema.respond_to?(:foreign_collection) + return unless relation_schema.respond_to?(:foreign_collection) # a scoped association applies its scope to the JOIN and may inject raw/unqualified SQL or # extra joins (e.g. `belongs_to :x, -> { where('id > ?', 1) }`) reflection = collection.model.reflect_on_association(relation_name.to_sym) return if reflection.nil? || reflection.scope + case relation_schema.type + when 'ManyToOne' then nil + when 'OneToOne' then return unless belongs_to_chain_through?(reflection) + else return + end + target = local_ar_collection(collection.datasource, relation_schema.foreign_collection) return if target.nil? || !target.model.default_scopes.empty? # same risk as a scoped association return unless same_database?(collection.model, target.model) @@ -305,6 +317,21 @@ def joinable_target(collection, relation_name, used_tables) target end + def belongs_to_chain_through?(reflection) + return false unless reflection.through_reflection? + + through = reflection.through_reflection + source = reflection.source_reflection + return false unless through && source + return false unless through.belongs_to? && source.belongs_to? + return false if through.scope || source.scope + return false unless through.klass.default_scopes.empty? + + same_database?(reflection.active_record, through.klass) + rescue StandardError + false + end + def same_database?(model_a, model_b) # compare the pools, not connection_specification_name (only an owner class name, shared across shards) model_a.connection_pool == model_b.connection_pool @@ -330,6 +357,16 @@ def add_join_relation(relation_name) @query end + # The intermediate foreign key a has_one :through uses on this table, or nil for a plain to-one. + def through_foreign_key(collection, relation_name) + reflection = collection.model.reflect_on_association(relation_name.to_sym) + return unless reflection&.through_reflection? + + reflection.through_reflection.foreign_key + rescue StandardError + nil + end + def resolve_field(original_field) if original_field.include?(':') relation_name, column_name = original_field.split(':') diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/account.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/account.rb index 5315e32e6..7bd88ac6a 100644 --- a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/account.rb +++ b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/account.rb @@ -1,4 +1,5 @@ class Account < ApplicationRecord belongs_to :supplier belongs_to :account_history + has_one :order, through: :account_history end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/account_history.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/account_history.rb index 337842259..04a237d0b 100644 --- a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/account_history.rb +++ b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/account_history.rb @@ -1,3 +1,4 @@ class AccountHistory < ApplicationRecord has_one :account + belongs_to :order, optional: true end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20251126100004_add_order_to_account_histories.rb b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20251126100004_add_order_to_account_histories.rb new file mode 100644 index 000000000..aed5a966d --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20251126100004_add_order_to_account_histories.rb @@ -0,0 +1,5 @@ +class AddOrderToAccountHistories < ActiveRecord::Migration[7.1] + def change + add_reference :account_histories, :order, null: true, foreign_key: true + 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 ddcb15376..259c48b11 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,12 +44,16 @@ module ForestAdminDatasourceActiveRecord expect(collection.schema[:fields].keys).to include('users') end - it 'add has_one_through relation' do + it 'add has_one_through relation as a to-one (OneToOne)' do collection = described_class.new(datasource, Supplier) expect(collection.schema[:fields].keys).to include('account_history') - expect(collection.schema[:fields]['account_history'].class).to eq(Relations::ManyToManySchema) + field = collection.schema[:fields]['account_history'] + expect(field.class).to eq(Relations::OneToOneSchema) + expect(field.foreign_collection).to eq('AccountHistory') + expect(field.origin_key).to eq(AccountHistory.primary_key) + expect(field.origin_key_target).to eq(Supplier.primary_key) end it 'skips association when foreign_key raises an error' do diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/join_to_one_optimization_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/join_to_one_optimization_spec.rb index 7a2345090..a9bc292dd 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/join_to_one_optimization_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/join_to_one_optimization_spec.rb @@ -105,6 +105,65 @@ def capture_sql end end + describe 'a has_one :through of belongs_to hops (account -> account_history -> order)' do + # every hop is a belongs_to, so the chained JOIN stays 1:1 and cannot multiply the parent. + let(:collection) { Collection.new(datasource, Account) } + let(:projection) { Projection.new(['id', 'order:reference']) } + + before do + order = Order.create!(reference: 'ORD-1') + Account.first.account_history.update!(order: order) + end + + it 'folds the whole chain into ONE JOINed query (was 3 with per-hop preload)' do + queries = capture_sql do + result = collection.list(caller, filter, projection) + expect(result.first['order']['reference']).to eq('ORD-1') + end + + expect(queries.size).to eq(1) + expect(queries.first.scan(/LEFT OUTER JOIN/i).size).to eq(2) # account_histories + orders + end + + it 'filters on a field of the through relation' do + condition = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + .new('order:reference', 'Equal', 'ORD-1') + result = collection.list(caller, Filter.new(condition_tree: condition), projection) + + expect(result.size).to eq(1) + expect(result.first['order']['reference']).to eq('ORD-1') + end + + it 'returns nothing when the through relation value does not match' do + condition = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + .new('order:reference', 'Equal', 'NOPE') + result = collection.list(caller, Filter.new(condition_tree: condition), Projection.new(['id'])) + + expect(result).to be_empty + end + + it 'aggregates grouped by a field of the through relation' do + aggregation = Aggregation.new(operation: 'Count', field: nil, groups: [{ field: 'order:reference' }]) + result = Utils::QueryAggregate.new(collection, aggregation).get + + expect(result).to contain_exactly('value' => 1, 'group' => { 'order:reference' => 'ORD-1' }) + end + end + + describe 'a has_one :through with a has_one hop (supplier -> account_history) stays on preload' do + # the intermediate `account` hop is a has_one and can multiply rows, so it must not be JOINed. + let(:collection) { Collection.new(datasource, Supplier) } + let(:projection) { Projection.new(['id', 'account_history:id']) } + + it 'is preloaded, not JOINed' do + query = Utils::Query.new(collection, projection, filter) + query.build + + expect(query.query.left_outer_joins_values).to be_empty + expect(query.query.includes_values.to_s).to include('account_history') + end + end + describe 'a to-many relation (car -> checks) is left on preload' do let(:collection) { Collection.new(datasource, Car) } let(:projection) { Projection.new(['id', 'reference', 'checks:garage_name']) } From a32e27db991cf68745c6e70acd6455553db52df6 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Fri, 3 Jul 2026 09:13:47 +0200 Subject: [PATCH 2/3] fix(datasource-active-record): address review feedback on has_one :through to-one - select the intermediate through FK only when it lives on the root table (the through hop is a belongs_to); a has_one through hop keeps the FK on the child, so selecting it against the root table emitted an invalid column. - count the intermediate through table in the JOIN safeguard so a separately projected relation on that same table falls back to preload instead of being JOINed twice (which ActiveRecord would alias, breaking column references). - document that a has_one :through to-one is display-only: it has no direct foreign key, so updating/associating it via the OneToOne write path is unsupported (the link lives on the intermediate table). - simplify belongs_to_chain_through?. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../collection.rb | 5 +++ .../utils/query.rb | 41 +++++++++++++------ .../utils/join_to_one_optimization_spec.rb | 24 +++++++++++ 3 files changed, 58 insertions(+), 12 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 f5f9b77c2..693ebad26 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 @@ -117,6 +117,11 @@ def fetch_associations ) ) else + # A has_one :through is single-valued, so expose it as a to-one (HasOne). ActiveRecord + # resolves the join by association name, so the primary keys below only need to be real + # columns for reads/schema generation. This relation is display-only: it has no direct + # foreign key to set, so updating/associating it through the OneToOne write path is not + # supported (the link lives on the intermediate table). add_field( association.name.to_s, ForestAdminDatasourceToolkit::Schema::Relations::OneToOneSchema.new( diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb index 9a15cf19e..161312edc 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb @@ -198,7 +198,7 @@ def build_select(collection, projection) @select << "#{collection.model.table_name}.#{relation_schema.origin_key_target}" # a has_one :through preloads via the intermediate FK on this table; select it so a # relation that falls back to preload (e.g. it is also used by a filter) still loads. - through_fk = through_foreign_key(collection, relation_name) + through_fk = root_through_foreign_key(collection, relation_name) @select << "#{collection.model.table_name}.#{through_fk}" if through_fk elsif many_to_one_relations.include?(relation_schema.type) @select << "#{collection.model.table_name}.#{relation_schema.foreign_key}" @@ -279,7 +279,9 @@ def joinable_tables(collection, relation_name, sub_projection, used_tables) target = joinable_target(collection, relation_name, used_tables) return nil if target.nil? - tables = Set[target.model.table_name] + # a has_one :through also joins its intermediate table(s); count them so a separately + # projected relation on the same table is not joined a second time (AR would alias it) + tables = Set[target.model.table_name] | through_tables(collection, relation_name) sub_projection.relations.each do |nested_name, nested_projection| nested = joinable_tables(target, nested_name, nested_projection, used_tables | tables) return nil if nested.nil? @@ -313,21 +315,33 @@ def joinable_target(collection, relation_name, used_tables) return if target.nil? || !target.model.default_scopes.empty? # same risk as a scoped association return unless same_database?(collection.model, target.model) return if used_tables.include?(target.model.table_name) # a table joined twice would be aliased by AR + # same for the intermediate table(s) a has_one :through pulls in + return if through_tables(collection, relation_name).intersect?(used_tables) target end + # Intermediate table(s) ActiveRecord joins for a has_one :through (empty for a direct to-one). + def through_tables(collection, relation_name) + through = collection.model.reflect_on_association(relation_name.to_sym)&.through_reflection + return Set[] unless through + + Set[through.table_name] + rescue StandardError + Set[] + end + def belongs_to_chain_through?(reflection) return false unless reflection.through_reflection? through = reflection.through_reflection source = reflection.source_reflection - return false unless through && source - return false unless through.belongs_to? && source.belongs_to? - return false if through.scope || source.scope - return false unless through.klass.default_scopes.empty? - same_database?(reflection.active_record, through.klass) + # every hop matches on a primary key (no row multiplication), carries no scope, and stays in + # this database, so the chained LEFT JOIN is safe + through && source && through.belongs_to? && source.belongs_to? && + through.scope.nil? && source.scope.nil? && through.klass.default_scopes.empty? && + same_database?(reflection.active_record, through.klass) rescue StandardError false end @@ -358,11 +372,14 @@ def add_join_relation(relation_name) end # The intermediate foreign key a has_one :through uses on this table, or nil for a plain to-one. - def through_foreign_key(collection, relation_name) - reflection = collection.model.reflect_on_association(relation_name.to_sym) - return unless reflection&.through_reflection? - - reflection.through_reflection.foreign_key + # The intermediate FK a has_one :through preloads through, but only when it lives on THIS (root) + # table — i.e. the through hop is a belongs_to. For a has_one through hop the FK is on the child, + # so selecting it against the root table would emit an invalid column. + def root_through_foreign_key(collection, relation_name) + through = collection.model.reflect_on_association(relation_name.to_sym)&.through_reflection + return unless through&.belongs_to? + + through.foreign_key rescue StandardError nil end diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/join_to_one_optimization_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/join_to_one_optimization_spec.rb index a9bc292dd..9cc656395 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/join_to_one_optimization_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/join_to_one_optimization_spec.rb @@ -148,6 +148,20 @@ def capture_sql expect(result).to contain_exactly('value' => 1, 'group' => { 'order:reference' => 'ORD-1' }) end + + it 'does not JOIN the intermediate table twice when it is also projected on its own' do + projection = Projection.new(['id', 'order:reference', 'account_history:id']) + query = Utils::Query.new(collection, projection, filter) + query.build + + # the through folds `account_histories` in for `order`; the standalone `account_history` + # must fall back to preload rather than JOIN the same table again + expect(query.query.to_sql.scan(/JOIN "account_histories"/i).size).to eq(1) + + result = collection.list(caller, filter, projection) + expect(result.first['order']['reference']).to eq('ORD-1') + expect(result.first['account_history']['id']).to eq(Account.first.account_history_id) + end end describe 'a has_one :through with a has_one hop (supplier -> account_history) stays on preload' do @@ -162,6 +176,16 @@ def capture_sql expect(query.query.left_outer_joins_values).to be_empty expect(query.query.includes_values.to_s).to include('account_history') end + + it 'does not select the intermediate FK against the root table (it lives on the child)' do + query = Utils::Query.new(collection, projection, filter) + query.build + sql = query.query.to_sql + + expect(sql).not_to match(/suppliers"\."supplier_id"/) + expect(sql).not_to match(/suppliers"\."account_id"/) + expect { collection.list(caller, filter, projection) }.not_to raise_error + end end describe 'a to-many relation (car -> checks) is left on preload' do From c2a90de8ecd3284fb200882add9512581661430e Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Fri, 3 Jul 2026 10:11:55 +0200 Subject: [PATCH 3/3] docs(datasource-active-record): drop comments added for has_one :through work Co-Authored-By: Claude Opus 4.8 (1M context) --- .../collection.rb | 5 ----- .../utils/query.rb | 18 +----------------- .../utils/join_to_one_optimization_spec.rb | 6 +----- 3 files changed, 2 insertions(+), 27 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 693ebad26..f5f9b77c2 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 @@ -117,11 +117,6 @@ def fetch_associations ) ) else - # A has_one :through is single-valued, so expose it as a to-one (HasOne). ActiveRecord - # resolves the join by association name, so the primary keys below only need to be real - # columns for reads/schema generation. This relation is display-only: it has no direct - # foreign key to set, so updating/associating it through the OneToOne write path is not - # supported (the link lives on the intermediate table). add_field( association.name.to_s, ForestAdminDatasourceToolkit::Schema::Relations::OneToOneSchema.new( diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb index 161312edc..74d9c0e0e 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb @@ -196,8 +196,6 @@ def build_select(collection, projection) if collection.model.table_name == @collection.model.table_name if one_to_one_relations.include?(relation_schema.type) @select << "#{collection.model.table_name}.#{relation_schema.origin_key_target}" - # a has_one :through preloads via the intermediate FK on this table; select it so a - # relation that falls back to preload (e.g. it is also used by a filter) still loads. through_fk = root_through_foreign_key(collection, relation_name) @select << "#{collection.model.table_name}.#{through_fk}" if through_fk elsif many_to_one_relations.include?(relation_schema.type) @@ -279,8 +277,6 @@ def joinable_tables(collection, relation_name, sub_projection, used_tables) target = joinable_target(collection, relation_name, used_tables) return nil if target.nil? - # a has_one :through also joins its intermediate table(s); count them so a separately - # projected relation on the same table is not joined a second time (AR would alias it) tables = Set[target.model.table_name] | through_tables(collection, relation_name) sub_projection.relations.each do |nested_name, nested_projection| nested = joinable_tables(target, nested_name, nested_projection, used_tables | tables) @@ -291,11 +287,7 @@ def joinable_tables(collection, relation_name, sub_projection, used_tables) tables end - # The target collection when this single hop is safe to collapse into a LEFT OUTER JOIN, - # else nil. A belongs_to qualifies: it matches on the target's primary key, so the JOIN - # cannot duplicate the parent row (a plain has_one child may not be unique). A has_one :through - # also qualifies when every hop is a belongs_to (see belongs_to_chain_through?): AR resolves the - # intermediate join, saving the extra per-hop preload queries. + # The target collection when this hop is safe to collapse into a JOIN, else nil (-> preload). def joinable_target(collection, relation_name, used_tables) relation_schema = collection.schema[:fields][relation_name] return unless relation_schema.respond_to?(:foreign_collection) @@ -315,13 +307,11 @@ def joinable_target(collection, relation_name, used_tables) return if target.nil? || !target.model.default_scopes.empty? # same risk as a scoped association return unless same_database?(collection.model, target.model) return if used_tables.include?(target.model.table_name) # a table joined twice would be aliased by AR - # same for the intermediate table(s) a has_one :through pulls in return if through_tables(collection, relation_name).intersect?(used_tables) target end - # Intermediate table(s) ActiveRecord joins for a has_one :through (empty for a direct to-one). def through_tables(collection, relation_name) through = collection.model.reflect_on_association(relation_name.to_sym)&.through_reflection return Set[] unless through @@ -337,8 +327,6 @@ def belongs_to_chain_through?(reflection) through = reflection.through_reflection source = reflection.source_reflection - # every hop matches on a primary key (no row multiplication), carries no scope, and stays in - # this database, so the chained LEFT JOIN is safe through && source && through.belongs_to? && source.belongs_to? && through.scope.nil? && source.scope.nil? && through.klass.default_scopes.empty? && same_database?(reflection.active_record, through.klass) @@ -371,10 +359,6 @@ def add_join_relation(relation_name) @query end - # The intermediate foreign key a has_one :through uses on this table, or nil for a plain to-one. - # The intermediate FK a has_one :through preloads through, but only when it lives on THIS (root) - # table — i.e. the through hop is a belongs_to. For a has_one through hop the FK is on the child, - # so selecting it against the root table would emit an invalid column. def root_through_foreign_key(collection, relation_name) through = collection.model.reflect_on_association(relation_name.to_sym)&.through_reflection return unless through&.belongs_to? diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/join_to_one_optimization_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/join_to_one_optimization_spec.rb index 9cc656395..edd3a8213 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/join_to_one_optimization_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/join_to_one_optimization_spec.rb @@ -106,7 +106,6 @@ def capture_sql end describe 'a has_one :through of belongs_to hops (account -> account_history -> order)' do - # every hop is a belongs_to, so the chained JOIN stays 1:1 and cannot multiply the parent. let(:collection) { Collection.new(datasource, Account) } let(:projection) { Projection.new(['id', 'order:reference']) } @@ -122,7 +121,7 @@ def capture_sql end expect(queries.size).to eq(1) - expect(queries.first.scan(/LEFT OUTER JOIN/i).size).to eq(2) # account_histories + orders + expect(queries.first.scan(/LEFT OUTER JOIN/i).size).to eq(2) end it 'filters on a field of the through relation' do @@ -154,8 +153,6 @@ def capture_sql query = Utils::Query.new(collection, projection, filter) query.build - # the through folds `account_histories` in for `order`; the standalone `account_history` - # must fall back to preload rather than JOIN the same table again expect(query.query.to_sql.scan(/JOIN "account_histories"/i).size).to eq(1) result = collection.list(caller, filter, projection) @@ -165,7 +162,6 @@ def capture_sql end describe 'a has_one :through with a has_one hop (supplier -> account_history) stays on preload' do - # the intermediate `account` hop is a has_one and can multiply rows, so it must not be JOINed. let(:collection) { Collection.new(datasource, Supplier) } let(:projection) { Projection.new(['id', 'account_history:id']) }