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']) }