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..e39f96783 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}" + # foreign_key is an array for a composite-key belongs_to through hop + Array(root_through_foreign_key(collection, relation_name)).each do |through_fk| + @select << "#{collection.model.table_name}.#{through_fk}" + end elsif many_to_one_relations.include?(relation_schema.type) @select << "#{collection.model.table_name}.#{relation_schema.foreign_key}" end @@ -275,7 +279,7 @@ 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] + 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? @@ -288,23 +292,50 @@ def joinable_tables(collection, relation_name, sub_projection, used_tables) # 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] - # 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) return if used_tables.include?(target.model.table_name) # a table joined twice would be aliased by AR + return if through_tables(collection, relation_name).intersect?(used_tables) target end + 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 + + 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 + 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 +361,15 @@ def add_join_relation(relation_name) @query end + 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 + 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..021eb2fa6 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,98 @@ def capture_sql end end + describe 'a has_one :through of belongs_to hops (account -> account_history -> order)' do + 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) + 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 + + 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 + + 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 + + it 'does not JOIN the intermediate table twice when a filter already joined the through' do + projection = Projection.new(['id', 'account_history:id']) + condition = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + .new('order:reference', 'Equal', 'ORD-1') + query = Utils::Query.new(collection, projection, Filter.new(condition_tree: condition)) + query.build + + expect(query.query.to_sql.scan(/JOIN "account_histories"/i).size).to eq(1) + + result = collection.list(caller, Filter.new(condition_tree: condition), projection) + 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 + 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 + + 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 let(:collection) { Collection.new(datasource, Car) } let(:projection) { Projection.new(['id', 'reference', 'checks:garage_name']) }