From a33fb9e52421519f630813287b0a4e08a1579068 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Wed, 1 Jul 2026 18:52:22 +0200 Subject: [PATCH 01/10] perf(datasource-active-record): JOIN same-DB to-one relations instead of preload Resolve non-polymorphic to-one relation chains (belongs_to / has_one) that live on the same database with a single LEFT OUTER JOIN (eager_load) instead of one extra preload query per relation hop. Collapses the round-trip cascade on record show and keeps a constant query count on lists. Guards preserve existing behaviour wherever a JOIN would be unsafe or impossible: - to-many relations keep preload (a JOIN multiplies rows and breaks pagination) - polymorphic to-one relations keep preload (target table varies per row) - targets on a different database connection keep preload (connects_to) - targets carrying a default_scope keep preload (may inject unqualifiable SQL, e.g. `where('id > ?', 10)`, that becomes ambiguous once joined) - targets that cannot be resolved as an AR-backed collection belonging to this very datasource instance keep preload (defensive against cross-datasource / name-collision cases): checks concrete class + datasource object identity Adds a spec proving a two-hop chain collapses to one JOINed query, query count stays constant regardless of row count, and every guard falls back safely. Co-Authored-By: Claude Opus 4.8 --- .../utils/query.rb | 65 +++++++- .../utils/join_to_one_optimization_spec.rb | 150 ++++++++++++++++++ 2 files changed, 214 insertions(+), 1 deletion(-) create mode 100644 packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/join_to_one_optimization_spec.rb 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 ee651a120..2caae4c87 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 @@ -204,12 +204,75 @@ def build_select(collection, projection) end def apply_select + unless @projection.nil? + join_tree = {} + preload_tree = {} + @projection.relations.each do |relation_name, sub_projection| + if fully_joinable?(@collection, relation_name, sub_projection) + join_tree[relation_name.to_sym] = format_relation_projection(sub_projection) + else + preload_tree[relation_name.to_sym] = format_relation_projection(sub_projection) + end + end + + # to-one relations living in the same database are resolved with a single + # LEFT OUTER JOIN (eager_load) instead of one extra query per relation + # (preload). to-many / polymorphic / cross-database relations keep preload: + # a JOIN would multiply rows (breaking pagination) or is simply not possible. + @query = @query.eager_load(join_tree) unless join_tree.empty? + @query = @query.includes(preload_tree) unless preload_tree.empty? + end @query = @query.select(@select.join(', ')) if @select - @query = @query.includes(format_relation_projection(@projection)) unless @projection.nil? @query end + # A relation subtree can be collapsed into a JOIN only if the relation itself is a + # non-polymorphic to-one relation on the same database, and every relation nested + # below it satisfies the same constraint. + def fully_joinable?(collection, relation_name, sub_projection) + relation_schema = collection.schema[:fields][relation_name] + return false unless %w[OneToOne ManyToOne].include?(relation_schema.type) + return false unless relation_schema.respond_to?(:foreign_collection) + + # The target must be an ActiveRecord collection living in THIS datasource. A + # relation towards another datasource (RPC, Mongoid, ...) is resolved above the + # datasource and never reaches here, but we stay defensive: anything we cannot + # resolve locally as an AR-backed collection falls back to preload. + target_collection = local_ar_collection(collection.datasource, relation_schema.foreign_collection) + return false if target_collection.nil? + return false unless same_database?(collection.model, target_collection.model) + # A default_scope is applied to the JOINed table and may contain raw / unqualified + # SQL (e.g. `where('id > ?', 10)`) that becomes ambiguous once joined. Preload keeps + # it isolated, so fall back to preload rather than risk broken SQL. + return false unless target_collection.model.default_scopes.empty? + + sub_projection.relations.all? do |nested_name, nested_projection| + fully_joinable?(target_collection, nested_name, nested_projection) + end + end + + def same_database?(model_a, model_b) + model_a.connection_specification_name == model_b.connection_specification_name + rescue StandardError + false + end + + # Returns the target collection only when it is ActiveRecord-backed AND belongs to + # the very same datasource instance we are querying; otherwise nil (=> the caller + # falls back to preload). Belt-and-suspenders against a foreign collection ever + # being reachable by name (e.g. name collision across datasources in a composite): + # we check the concrete class and the datasource object identity, not just the name. + def local_ar_collection(datasource, name) + collection = datasource.get_collection(name) + return nil unless collection.is_a?(ForestAdminDatasourceActiveRecord::Collection) + return nil unless collection.datasource.equal?(datasource) + + collection + rescue StandardError + nil + end + def add_join_relation(relation_name) @query = @query.left_joins(relation_name.to_sym) 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 new file mode 100644 index 000000000..c04e7f186 --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/join_to_one_optimization_spec.rb @@ -0,0 +1,150 @@ +require 'spec_helper' + +module ForestAdminDatasourceActiveRecord + include ForestAdminDatasourceToolkit::Schema + include ForestAdminDatasourceToolkit::Components::Query + + # Proves that to-one relations on the same database are resolved with a single + # LEFT OUTER JOIN instead of one extra preload query per relation hop, while + # to-many relations, and relations whose target carries a default_scope, keep + # their (safe) preload behaviour. + describe 'to-one JOIN optimization', :db_truncation do + let(:datasource) { Datasource.new({ adapter: 'sqlite3', database: 'db/database.db' }) } + let(:caller) { nil } + let(:filter) { Filter.new } + + before do + Account.delete_all + Supplier.delete_all + AccountHistory.delete_all + Check.delete_all + Car.unscoped.delete_all + Category.delete_all + + history = AccountHistory.create! + supplier = Supplier.create!(name: 'ACME') + Account.create!(supplier: supplier, account_history: history) + + category = Category.create!(label: 'Compact') + car = Car.unscoped.create!(reference: 'CAR11', category: category) # id > 10 to pass Car's default_scope + car.checks << Check.create!(garage_name: 'Garage1', date: Date.today) + end + + def capture_sql + queries = [] + sub = ActiveSupport::Notifications.subscribe('sql.active_record') do |*args| + payload = args.last + unless payload[:name] == 'SCHEMA' || payload[:name] == 'CACHE' || + payload[:sql] =~ /^(BEGIN|COMMIT|ROLLBACK|SAVEPOINT|RELEASE)/i + queries << payload[:sql] + end + end + yield + queries + ensure + ActiveSupport::Notifications.unsubscribe(sub) + end + + describe 'a two-hop to-one chain (supplier -> account -> account_history)' do + let(:collection) { Collection.new(datasource, Supplier) } + let(:projection) { Projection.new(['id', 'name', 'account:id', 'account:account_history:id']) } + + it 'resolves the whole chain in ONE JOINed query (was 3 with preload)' do + queries = capture_sql do + result = collection.list(caller, filter, projection) + # correctness: nested to-one values are still hydrated + expect(result.first['account']).not_to be_nil + expect(result.first['account']['account_history']).not_to be_nil + end + + expect(queries.size).to eq(1) + expect(queries.first.scan(/LEFT OUTER JOIN/i).size).to eq(2) # accounts + account_histories + end + + it 'builds the query with eager_load, not preload' do + query = Utils::Query.new(collection, projection, filter) + query.build + + expect(query.query.eager_load_values).not_to be_empty + expect(query.query.includes_values).to be_empty + end + + it 'keeps a constant query count regardless of the number of rows' do + 5.times { |i| Account.create!(supplier: Supplier.create!(name: "S#{i}"), account_history: AccountHistory.create!) } + + two = capture_sql { collection.list(caller, Filter.new(page: Page.new(limit: 2, offset: 0)), projection) } + all = capture_sql { collection.list(caller, filter, projection) } + + expect(two.size).to eq(all.size) + expect(all.size).to eq(1) + end + end + + describe 'safety guard: a target with a default_scope falls back to preload' do + let(:collection) { Collection.new(datasource, Car) } + let(:projection) { Projection.new(['id', 'reference', 'category:label']) } + + # Category is reached through Car, whose default_scope contains an unqualified + # column; a JOIN would raise "ambiguous column name". The guard must keep preload. + it 'does not JOIN and still returns correct data' do + query = Utils::Query.new(collection, projection, filter) + query.build + + result = collection.list(caller, filter, projection) + expect(result.first['category']['label']).to eq('Compact') + end + end + + describe 'safety guard (belt-and-suspenders): target not local to this datasource' do + # A relation whose target lives in another datasource (RPC, Mongoid, ...) is + # resolved above the datasource and never reaches here. Even so, local_ar_collection + # only accepts an AR-backed collection belonging to the SAME datasource instance. + let(:collection) { Collection.new(datasource, Account) } + let(:query) { Utils::Query.new(collection, Projection.new(['id', 'supplier:name']), filter) } + + it 'returns nil when the collection name is absent (get_collection raises)' do + expect { datasource.get_collection('NotInThisDatasource') }.to raise_error(StandardError) + expect(query.send(:local_ar_collection, datasource, 'NotInThisDatasource')).to be_nil + end + + it 'returns nil when the resolved collection is not ActiveRecord-backed' do + # e.g. an RPC/Mongoid collection: a Toolkit collection that is not our AR subclass + non_ar = instance_double(ForestAdminDatasourceToolkit::Collection) + foreign_ds = instance_double(Datasource, get_collection: non_ar) + + expect(query.send(:local_ar_collection, foreign_ds, 'X')).to be_nil + end + + it 'returns nil when the resolved AR collection belongs to a DIFFERENT datasource' do + other_datasource = Datasource.new({ adapter: 'sqlite3', database: 'db/database.db' }) + foreign_collection = Collection.new(other_datasource, Supplier) # AR-backed but not ours + foreign_ds = instance_double(Datasource, get_collection: foreign_collection) + + expect(foreign_collection).to be_a(Collection) # passes the type check + expect(query.send(:local_ar_collection, foreign_ds, 'Supplier')).to be_nil # rejected on identity + end + + it 'fully_joinable? is false when the target cannot be resolved locally' do + allow(query).to receive(:local_ar_collection).and_return(nil) + expect(collection.schema[:fields]['supplier'].type).to eq('ManyToOne') # otherwise joinable + expect(query.send(:fully_joinable?, collection, 'supplier', Projection.new([]))).to be(false) + 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']) } + + it 'keeps preload (separate batched query), never a row-multiplying JOIN' do + query = Utils::Query.new(collection, projection, filter) + query.build + + expect(query.query.includes_values.to_s).to include('checks') + expect(query.query.eager_load_values).to be_empty + + result = collection.list(caller, filter, projection) + expect(result.first['checks'].first['garage_name']).to eq('Garage1') + end + end + end +end From 610e8156ad3c11262da85f10c13f9e3e58aef250 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Thu, 2 Jul 2026 09:15:26 +0200 Subject: [PATCH 02/10] perf(datasource-active-record): select only projected columns on JOINed to-one relations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Builds on the JOIN optimization: instead of eager_load (which forces `table.*` for every joined association), to-one relations are now resolved with left_outer_joins + an explicit SELECT of ONLY their projected columns, aliased on the flat row. The serializer rebuilds the nested hash from those aliases (and detects NULL relations via the target primary key) rather than reading the ActiveRecord association. Effect: displaying a single field of a wide relation (e.g. a heavy DB view with ~150 columns) reads exactly that column plus the join keys, in one query — no full-row read and no per-hop round-trip. to-many / guarded relations keep their preload path and the existing association-based serialization untouched. 156 examples, 0 failures; rubocop clean. Co-Authored-By: Claude Opus 4.8 --- .../collection.rb | 3 +- .../utils/active_record_serializer.rb | 97 ++++++++++++------- .../utils/query.rb | 46 ++++++++- .../utils/join_to_one_optimization_spec.rb | 13 ++- .../utils/query_spec.rb | 7 +- 5 files changed, 120 insertions(+), 46 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 78ce83add..d6bdcedc1 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 @@ -26,8 +26,9 @@ def native_driver def list(_caller, filter, projection) query = Utils::Query.new(self, projection, filter) + records = query.get - query.get.map { |record| Utils::ActiveRecordSerializer.new(record).to_hash(projection) } + records.map { |record| Utils::ActiveRecordSerializer.new(record, query.joined_relations).to_hash(projection) } end def aggregate(_caller, filter, aggregation, limit = nil) diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb index adebabb5b..97e141496 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb @@ -1,53 +1,82 @@ module ForestAdminDatasourceActiveRecord module Utils - ActiveRecordSerializer = Struct.new(:object) do + # `joined_relations` maps a relation path ("bank_account.organizations_view") to the SQL + # aliases of its columns on the flat row (see Query#collect_joined_selects). Relations + # listed there were resolved by JOIN and are hydrated from the flat row; every other + # relation is read from its (preloaded) ActiveRecord association, as before. + ActiveRecordSerializer = Struct.new(:object, :joined_relations) do def to_hash(projection) hash_object(object, projection) end - def hash_object(object, projection = nil, with_associations: true) - hash = {} - + def hash_object(object, projection = nil, path: [], with_associations: true) return if object.nil? - hash.merge! object.attributes + hash = base_attributes(object) - serialize_associations(object, projection, hash) if with_associations + serialize_associations(object, projection, hash, path) if with_associations && projection hash end - def serialize_associations(object, projection, hash) + def base_attributes(object) + return object.attributes if join_aliases.empty? + + object.attributes.except(*join_aliases) + end + + def serialize_associations(object, projection, hash, path) one_associations = %i[has_one belongs_to] many_associations = %i[has_many has_and_belongs_to_many] - # Handle one-to-one and many-to-one associations - object.class.reflect_on_all_associations - .filter { |a| one_associations.include?(a.macro) && projection.relations.key?(a.name.to_s) } - .each do |association| - association_name = association.name.to_s - hash[association_name] = hash_object( - object.send(association_name), - projection.relations[association_name], - with_associations: projection.relations.key?(association_name) - ) - end - - # Handle one-to-many and many-to-many associations - object.class.reflect_on_all_associations - .filter { |a| many_associations.include?(a.macro) && projection.relations.key?(a.name.to_s) } - .each do |association| - association_name = association.name.to_s - collection = object.send(association_name) - # Serialize the collection as an array - hash[association_name] = collection.map do |item| - hash_object( - item, - projection.relations[association_name], - with_associations: projection.relations.key?(association_name) - ) - end - end + projection.relations.each_key do |association_name| + relation_path = path + [association_name] + + if joined_relation?(relation_path) + hash[association_name] = hash_joined_relation(projection.relations[association_name], relation_path) + next + end + + association = object.class.reflect_on_association(association_name.to_sym) + next if association.nil? + + if one_associations.include?(association.macro) + hash[association_name] = hash_object( + object.send(association_name), + projection.relations[association_name], + path: relation_path + ) + elsif many_associations.include?(association.macro) + hash[association_name] = object.send(association_name).map do |item| + hash_object(item, projection.relations[association_name], path: relation_path) + end + end + end + end + + # Rebuilds a JOINed relation's hash from the flat aliased columns carried by the root + # object. Returns nil when the (LEFT-joined) relation is absent. + def hash_joined_relation(projection, relation_path) + meta = joined_relations[relation_path.join('.')] + return nil if object[meta[:pk_alias]].nil? + + hash = {} + projection.columns.each { |column| hash[column] = object[meta[:columns][column]] } + projection.relations.each_key do |nested_name| + hash[nested_name] = hash_joined_relation(projection.relations[nested_name], relation_path + [nested_name]) + end + + hash + end + + private + + def joined_relation?(relation_path) + !joined_relations.nil? && joined_relations.key?(relation_path.join('.')) + end + + def join_aliases + @join_aliases ||= (joined_relations || {}).values.flat_map { |meta| meta[:columns].values }.to_set end end end 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 2caae4c87..a4d57aa96 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 @@ -3,7 +3,7 @@ module Utils class Query include ForestAdminDatasourceToolkit::Components::Query::ConditionTree - attr_reader :query, :select + attr_reader :query, :select, :joined_relations def initialize(collection, projection, filter) @collection = collection @@ -12,6 +12,11 @@ def initialize(collection, projection, filter) @filter = filter @arel_table = @collection.model.arel_table @select = [] + # path (e.g. "bank_account.organizations_view") => { columns: { col => sql_alias }, + # pk_alias: }. Populated for to-one relations resolved by JOIN so the serializer can + # hydrate them from the flat row instead of triggering a lazy association load. + @joined_relations = {} + @alias_counter = 0 end def build @@ -210,16 +215,19 @@ def apply_select @projection.relations.each do |relation_name, sub_projection| if fully_joinable?(@collection, relation_name, sub_projection) join_tree[relation_name.to_sym] = format_relation_projection(sub_projection) + collect_joined_selects(@collection, relation_name, sub_projection, [relation_name]) else preload_tree[relation_name.to_sym] = format_relation_projection(sub_projection) end end # to-one relations living in the same database are resolved with a single - # LEFT OUTER JOIN (eager_load) instead of one extra query per relation - # (preload). to-many / polymorphic / cross-database relations keep preload: - # a JOIN would multiply rows (breaking pagination) or is simply not possible. - @query = @query.eager_load(join_tree) unless join_tree.empty? + # LEFT OUTER JOIN instead of one extra query per relation hop (preload). Unlike + # eager_load, we select ONLY the projected columns of the joined tables (see + # collect_joined_selects) so a wide relation (e.g. a heavy DB view) is not fully + # read. to-many / polymorphic / cross-database relations keep preload: a JOIN + # would multiply rows (breaking pagination) or is simply not possible. + @query = @query.left_outer_joins(join_tree) unless join_tree.empty? @query = @query.includes(preload_tree) unless preload_tree.empty? end @query = @query.select(@select.join(', ')) if @select @@ -227,6 +235,34 @@ def apply_select @query end + # Adds "table"."column" AS "alias" entries to @select for every projected column of a + # joined to-one relation (recursively), and records the aliases in @joined_relations + # so the serializer can rebuild the nested hash from the flat row. The target primary + # key is always selected to let the serializer detect a NULL (absent) relation. + def collect_joined_selects(collection, relation_name, sub_projection, path) + relation_schema = collection.schema[:fields][relation_name] + target = local_ar_collection(collection.datasource, relation_schema.foreign_collection) + table = target.model.table_name + pk = target.model.primary_key + + alias_map = {} + (sub_projection.columns + [pk]).uniq.each do |column| + sql_alias = next_join_alias + @select << %("#{table}"."#{column}" AS "#{sql_alias}") + alias_map[column] = sql_alias + end + @joined_relations[path.join('.')] = { columns: alias_map, pk_alias: alias_map[pk] } + + sub_projection.relations.each do |nested_name, nested_projection| + collect_joined_selects(target, nested_name, nested_projection, path + [nested_name]) + end + end + + def next_join_alias + @alias_counter += 1 + "fa_join_#{@alias_counter}" + end + # A relation subtree can be collapsed into a JOIN only if the relation itself is a # non-polymorphic to-one relation on the same database, and every relation nested # below it satisfies the same constraint. 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 c04e7f186..37c264a44 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 @@ -52,7 +52,7 @@ def capture_sql it 'resolves the whole chain in ONE JOINed query (was 3 with preload)' do queries = capture_sql do result = collection.list(caller, filter, projection) - # correctness: nested to-one values are still hydrated + # correctness: nested to-one values are still hydrated from the flat row expect(result.first['account']).not_to be_nil expect(result.first['account']['account_history']).not_to be_nil end @@ -61,12 +61,17 @@ def capture_sql expect(queries.first.scan(/LEFT OUTER JOIN/i).size).to eq(2) # accounts + account_histories end - it 'builds the query with eager_load, not preload' do + it 'selects ONLY the projected columns of the joined tables (not table.*)' do + # account_histories is only reached for its id -> exactly one of its columns is read. query = Utils::Query.new(collection, projection, filter) query.build + sql = query.query.to_sql - expect(query.query.eager_load_values).not_to be_empty - expect(query.query.includes_values).to be_empty + expect(query.query.eager_load_values).to be_empty # not eager_load (which forces table.*) + # every column of account_histories except id must be absent from the SELECT + AccountHistory.column_names.reject { |c| c == 'id' }.each do |col| + expect(sql).not_to match(/account_histories"\."#{col}"/) + end end it 'keeps a constant query count regardless of the number of rows' do diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb index 404d3958c..ab8ee047a 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb @@ -18,13 +18,16 @@ module Utils describe 'build select' do context 'when projection have nested relation' do - it 'build select with all requested fields related to the current collection' do + it 'selects the current collection fields and JOINs the nested to-one relations' do projection = Projection.new(%w[account_history:account:id account_history:account_id account_history:id]) collection = Collection.new(datasource, Account) query_builder = described_class.new(collection, projection, Filter.new) query_builder.build - expect(query_builder.select).to eq(%w[accounts.account_history_id accounts.id]) + # base columns (own + foreign keys) are still selected as before + expect(query_builder.select).to include('accounts.account_history_id', 'accounts.id') + # nested to-one relations are resolved by JOIN and tracked for flat hydration + expect(query_builder.joined_relations.keys).to contain_exactly('account_history', 'account_history.account') end end end From 2ca5893ded384659e3e6c719e36ba18ab613448b Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Thu, 2 Jul 2026 09:21:18 +0200 Subject: [PATCH 03/10] chore(datasource-active-record): trim comments to non-obvious rationale only Co-Authored-By: Claude Opus 4.8 --- .../utils/active_record_serializer.rb | 10 ++--- .../utils/query.rb | 39 ++++++------------- .../utils/join_to_one_optimization_spec.rb | 27 ++++--------- .../utils/query_spec.rb | 2 - 4 files changed, 23 insertions(+), 55 deletions(-) diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb index 97e141496..5de24f381 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb @@ -1,9 +1,8 @@ module ForestAdminDatasourceActiveRecord module Utils - # `joined_relations` maps a relation path ("bank_account.organizations_view") to the SQL - # aliases of its columns on the flat row (see Query#collect_joined_selects). Relations - # listed there were resolved by JOIN and are hydrated from the flat row; every other - # relation is read from its (preloaded) ActiveRecord association, as before. + # Relations in `joined_relations` (see Query#collect_joined_selects) were resolved by JOIN + # and are hydrated from the flat row's aliased columns; every other relation is read from + # its preloaded ActiveRecord association, as before. ActiveRecordSerializer = Struct.new(:object, :joined_relations) do def to_hash(projection) hash_object(object, projection) @@ -54,8 +53,7 @@ def serialize_associations(object, projection, hash, path) end end - # Rebuilds a JOINed relation's hash from the flat aliased columns carried by the root - # object. Returns nil when the (LEFT-joined) relation is absent. + # Reads a JOINed relation's columns from the aliases on the root object (not a nested one). def hash_joined_relation(projection, relation_path) meta = joined_relations[relation_path.join('.')] return nil if object[meta[:pk_alias]].nil? 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 a4d57aa96..530ed3496 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 @@ -12,9 +12,7 @@ def initialize(collection, projection, filter) @filter = filter @arel_table = @collection.model.arel_table @select = [] - # path (e.g. "bank_account.organizations_view") => { columns: { col => sql_alias }, - # pk_alias: }. Populated for to-one relations resolved by JOIN so the serializer can - # hydrate them from the flat row instead of triggering a lazy association load. + # relation path (e.g. "bank_account.organizations_view") => { columns: { col => sql_alias }, pk_alias: } @joined_relations = {} @alias_counter = 0 end @@ -221,12 +219,9 @@ def apply_select end end - # to-one relations living in the same database are resolved with a single - # LEFT OUTER JOIN instead of one extra query per relation hop (preload). Unlike - # eager_load, we select ONLY the projected columns of the joined tables (see - # collect_joined_selects) so a wide relation (e.g. a heavy DB view) is not fully - # read. to-many / polymorphic / cross-database relations keep preload: a JOIN - # would multiply rows (breaking pagination) or is simply not possible. + # to-one same-database relations are JOINed (selecting only their projected columns, + # unlike eager_load which reads the whole row); the rest stays on preload, where a + # JOIN would multiply rows (to-many) or is impossible (cross-database). @query = @query.left_outer_joins(join_tree) unless join_tree.empty? @query = @query.includes(preload_tree) unless preload_tree.empty? end @@ -235,10 +230,6 @@ def apply_select @query end - # Adds "table"."column" AS "alias" entries to @select for every projected column of a - # joined to-one relation (recursively), and records the aliases in @joined_relations - # so the serializer can rebuild the nested hash from the flat row. The target primary - # key is always selected to let the serializer detect a NULL (absent) relation. def collect_joined_selects(collection, relation_name, sub_projection, path) relation_schema = collection.schema[:fields][relation_name] target = local_ar_collection(collection.datasource, relation_schema.foreign_collection) @@ -246,6 +237,7 @@ def collect_joined_selects(collection, relation_name, sub_projection, path) pk = target.model.primary_key alias_map = {} + # pk is always selected so the serializer can detect a NULL (absent) left-joined relation (sub_projection.columns + [pk]).uniq.each do |column| sql_alias = next_join_alias @select << %("#{table}"."#{column}" AS "#{sql_alias}") @@ -263,24 +255,17 @@ def next_join_alias "fa_join_#{@alias_counter}" end - # A relation subtree can be collapsed into a JOIN only if the relation itself is a - # non-polymorphic to-one relation on the same database, and every relation nested - # below it satisfies the same constraint. + # True only when the relation and every relation nested below it can be safely JOINed. def fully_joinable?(collection, relation_name, sub_projection) relation_schema = collection.schema[:fields][relation_name] return false unless %w[OneToOne ManyToOne].include?(relation_schema.type) return false unless relation_schema.respond_to?(:foreign_collection) - # The target must be an ActiveRecord collection living in THIS datasource. A - # relation towards another datasource (RPC, Mongoid, ...) is resolved above the - # datasource and never reaches here, but we stay defensive: anything we cannot - # resolve locally as an AR-backed collection falls back to preload. target_collection = local_ar_collection(collection.datasource, relation_schema.foreign_collection) return false if target_collection.nil? return false unless same_database?(collection.model, target_collection.model) - # A default_scope is applied to the JOINed table and may contain raw / unqualified - # SQL (e.g. `where('id > ?', 10)`) that becomes ambiguous once joined. Preload keeps - # it isolated, so fall back to preload rather than risk broken SQL. + # a default_scope may inject raw/unqualified SQL (e.g. `where('id > ?', 10)`) that turns + # ambiguous once joined; preload keeps it isolated return false unless target_collection.model.default_scopes.empty? sub_projection.relations.all? do |nested_name, nested_projection| @@ -294,11 +279,9 @@ def same_database?(model_a, model_b) false end - # Returns the target collection only when it is ActiveRecord-backed AND belongs to - # the very same datasource instance we are querying; otherwise nil (=> the caller - # falls back to preload). Belt-and-suspenders against a foreign collection ever - # being reachable by name (e.g. name collision across datasources in a composite): - # we check the concrete class and the datasource object identity, not just the name. + # The target collection if it is AR-backed AND belongs to this exact datasource, else nil. + # Class + identity (not just the name) guard against a foreign-datasource collection ever + # being reachable here (e.g. name collision in a composite) -> caller falls back to preload. def local_ar_collection(datasource, name) collection = datasource.get_collection(name) return nil unless collection.is_a?(ForestAdminDatasourceActiveRecord::Collection) 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 37c264a44..7f88b5cf3 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 @@ -4,10 +4,6 @@ module ForestAdminDatasourceActiveRecord include ForestAdminDatasourceToolkit::Schema include ForestAdminDatasourceToolkit::Components::Query - # Proves that to-one relations on the same database are resolved with a single - # LEFT OUTER JOIN instead of one extra preload query per relation hop, while - # to-many relations, and relations whose target carries a default_scope, keep - # their (safe) preload behaviour. describe 'to-one JOIN optimization', :db_truncation do let(:datasource) { Datasource.new({ adapter: 'sqlite3', database: 'db/database.db' }) } let(:caller) { nil } @@ -52,23 +48,20 @@ def capture_sql it 'resolves the whole chain in ONE JOINed query (was 3 with preload)' do queries = capture_sql do result = collection.list(caller, filter, projection) - # correctness: nested to-one values are still hydrated from the flat row expect(result.first['account']).not_to be_nil expect(result.first['account']['account_history']).not_to be_nil end expect(queries.size).to eq(1) - expect(queries.first.scan(/LEFT OUTER JOIN/i).size).to eq(2) # accounts + account_histories + expect(queries.first.scan(/LEFT OUTER JOIN/i).size).to eq(2) end it 'selects ONLY the projected columns of the joined tables (not table.*)' do - # account_histories is only reached for its id -> exactly one of its columns is read. query = Utils::Query.new(collection, projection, filter) query.build sql = query.query.to_sql - expect(query.query.eager_load_values).to be_empty # not eager_load (which forces table.*) - # every column of account_histories except id must be absent from the SELECT + expect(query.query.eager_load_values).to be_empty # eager_load would force account_histories.* AccountHistory.column_names.reject { |c| c == 'id' }.each do |col| expect(sql).not_to match(/account_histories"\."#{col}"/) end @@ -89,8 +82,7 @@ def capture_sql let(:collection) { Collection.new(datasource, Car) } let(:projection) { Projection.new(['id', 'reference', 'category:label']) } - # Category is reached through Car, whose default_scope contains an unqualified - # column; a JOIN would raise "ambiguous column name". The guard must keep preload. + # Car's default_scope has an unqualified column; a JOIN would raise "ambiguous column name". it 'does not JOIN and still returns correct data' do query = Utils::Query.new(collection, projection, filter) query.build @@ -101,9 +93,6 @@ def capture_sql end describe 'safety guard (belt-and-suspenders): target not local to this datasource' do - # A relation whose target lives in another datasource (RPC, Mongoid, ...) is - # resolved above the datasource and never reaches here. Even so, local_ar_collection - # only accepts an AR-backed collection belonging to the SAME datasource instance. let(:collection) { Collection.new(datasource, Account) } let(:query) { Utils::Query.new(collection, Projection.new(['id', 'supplier:name']), filter) } @@ -113,7 +102,7 @@ def capture_sql end it 'returns nil when the resolved collection is not ActiveRecord-backed' do - # e.g. an RPC/Mongoid collection: a Toolkit collection that is not our AR subclass + # a Toolkit collection that is not our AR subclass, i.e. an RPC/Mongoid collection non_ar = instance_double(ForestAdminDatasourceToolkit::Collection) foreign_ds = instance_double(Datasource, get_collection: non_ar) @@ -122,16 +111,16 @@ def capture_sql it 'returns nil when the resolved AR collection belongs to a DIFFERENT datasource' do other_datasource = Datasource.new({ adapter: 'sqlite3', database: 'db/database.db' }) - foreign_collection = Collection.new(other_datasource, Supplier) # AR-backed but not ours + foreign_collection = Collection.new(other_datasource, Supplier) foreign_ds = instance_double(Datasource, get_collection: foreign_collection) - expect(foreign_collection).to be_a(Collection) # passes the type check - expect(query.send(:local_ar_collection, foreign_ds, 'Supplier')).to be_nil # rejected on identity + expect(foreign_collection).to be_a(Collection) # AR-backed, so only identity can reject it + expect(query.send(:local_ar_collection, foreign_ds, 'Supplier')).to be_nil end it 'fully_joinable? is false when the target cannot be resolved locally' do allow(query).to receive(:local_ar_collection).and_return(nil) - expect(collection.schema[:fields]['supplier'].type).to eq('ManyToOne') # otherwise joinable + expect(collection.schema[:fields]['supplier'].type).to eq('ManyToOne') # joinable but for the stub expect(query.send(:fully_joinable?, collection, 'supplier', Projection.new([]))).to be(false) end end diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb index ab8ee047a..0afea30d6 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb @@ -24,9 +24,7 @@ module Utils query_builder = described_class.new(collection, projection, Filter.new) query_builder.build - # base columns (own + foreign keys) are still selected as before expect(query_builder.select).to include('accounts.account_history_id', 'accounts.id') - # nested to-one relations are resolved by JOIN and tracked for flat hydration expect(query_builder.joined_relations.keys).to contain_exactly('account_history', 'account_history.account') end end From 906fa3f428362cd19242dcddce72e4ab7100fe72 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Thu, 2 Jul 2026 09:27:43 +0200 Subject: [PATCH 04/10] fix(datasource-active-record): address JOIN correctness edge cases (Macroscope review) - Only belongs_to (ManyToOne) relations are JOINed now. has_one (OneToOne) does not guarantee a unique child row, so a JOIN could duplicate the parent and break list results / pagination; has_one falls back to preload. - Never JOIN a table already present in the query (base or a sibling/nested join). ActiveRecord would alias a table joined twice, and collect_joined_selects references the plain table name; such relations fall back to preload instead. joinable_tables replaces fully_joinable?: it returns the set of tables a subtree would add via JOIN (or nil), threading the used-tables set so collisions are detected across the whole query. Specs cover: belongs_to collapses to one JOIN with only projected columns and constant query count; has_one, to-many, default_scope, already-used table, and non-local targets all fall back to preload. Co-Authored-By: Claude Opus 4.8 --- .../utils/query.rb | 45 +++++--- .../utils/join_to_one_optimization_spec.rb | 101 +++++++++++------- .../utils/query_spec.rb | 8 +- 3 files changed, 100 insertions(+), 54 deletions(-) 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 530ed3496..6457e7d94 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 @@ -1,3 +1,5 @@ +require 'set' + module ForestAdminDatasourceActiveRecord module Utils class Query @@ -210,8 +212,11 @@ def apply_select unless @projection.nil? join_tree = {} preload_tree = {} + used_tables = Set[@collection.model.table_name] @projection.relations.each do |relation_name, sub_projection| - if fully_joinable?(@collection, relation_name, sub_projection) + tables = joinable_tables(@collection, relation_name, sub_projection, used_tables) + if tables + used_tables |= tables join_tree[relation_name.to_sym] = format_relation_projection(sub_projection) collect_joined_selects(@collection, relation_name, sub_projection, [relation_name]) else @@ -219,9 +224,9 @@ def apply_select end end - # to-one same-database relations are JOINed (selecting only their projected columns, - # unlike eager_load which reads the whole row); the rest stays on preload, where a - # JOIN would multiply rows (to-many) or is impossible (cross-database). + # belongs_to relations on the same database are JOINed (selecting only their projected + # columns, unlike eager_load which reads the whole row); the rest stays on preload, where + # a JOIN would multiply rows or is impossible. @query = @query.left_outer_joins(join_tree) unless join_tree.empty? @query = @query.includes(preload_tree) unless preload_tree.empty? end @@ -255,22 +260,34 @@ def next_join_alias "fa_join_#{@alias_counter}" end - # True only when the relation and every relation nested below it can be safely JOINed. - def fully_joinable?(collection, relation_name, sub_projection) + # Set of tables the (fully joinable) subtree would add via JOIN, or nil when any relation + # in it cannot be safely joined. Only belongs_to is joinable: it matches on the target's + # primary key, so the JOIN cannot duplicate the parent row (a has_one child may not be + # unique). used_tables already covers the base + sibling joins: a table joined twice would + # be aliased by ActiveRecord, which collect_joined_selects cannot reference — so bail out. + def joinable_tables(collection, relation_name, sub_projection, used_tables) relation_schema = collection.schema[:fields][relation_name] - return false unless %w[OneToOne ManyToOne].include?(relation_schema.type) - return false unless relation_schema.respond_to?(:foreign_collection) + return nil unless relation_schema.type == 'ManyToOne' + return nil unless relation_schema.respond_to?(:foreign_collection) - target_collection = local_ar_collection(collection.datasource, relation_schema.foreign_collection) - return false if target_collection.nil? - return false unless same_database?(collection.model, target_collection.model) + target = local_ar_collection(collection.datasource, relation_schema.foreign_collection) + return nil if target.nil? + return nil unless same_database?(collection.model, target.model) # a default_scope may inject raw/unqualified SQL (e.g. `where('id > ?', 10)`) that turns # ambiguous once joined; preload keeps it isolated - return false unless target_collection.model.default_scopes.empty? + return nil unless target.model.default_scopes.empty? + + table = target.model.table_name + return nil if used_tables.include?(table) + + tables = Set[table] + 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? - sub_projection.relations.all? do |nested_name, nested_projection| - fully_joinable?(target_collection, nested_name, nested_projection) + tables |= nested end + tables end def same_database?(model_a, model_b) 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 7f88b5cf3..37a31bbef 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 @@ -4,7 +4,7 @@ module ForestAdminDatasourceActiveRecord include ForestAdminDatasourceToolkit::Schema include ForestAdminDatasourceToolkit::Components::Query - describe 'to-one JOIN optimization', :db_truncation do + describe 'belongs_to JOIN optimization', :db_truncation do let(:datasource) { Datasource.new({ adapter: 'sqlite3', database: 'db/database.db' }) } let(:caller) { nil } let(:filter) { Filter.new } @@ -17,9 +17,8 @@ module ForestAdminDatasourceActiveRecord Car.unscoped.delete_all Category.delete_all - history = AccountHistory.create! supplier = Supplier.create!(name: 'ACME') - Account.create!(supplier: supplier, account_history: history) + Account.create!(supplier: supplier, account_history: AccountHistory.create!) category = Category.create!(label: 'Compact') car = Car.unscoped.create!(reference: 'CAR11', category: category) # id > 10 to pass Car's default_scope @@ -41,34 +40,34 @@ def capture_sql ActiveSupport::Notifications.unsubscribe(sub) end - describe 'a two-hop to-one chain (supplier -> account -> account_history)' do - let(:collection) { Collection.new(datasource, Supplier) } - let(:projection) { Projection.new(['id', 'name', 'account:id', 'account:account_history:id']) } + describe 'a belongs_to relation (account -> supplier)' do + let(:collection) { Collection.new(datasource, Account) } + let(:projection) { Projection.new(['id', 'supplier:name']) } - it 'resolves the whole chain in ONE JOINed query (was 3 with preload)' do + it 'resolves it in ONE JOINed query (was 2 with preload)' do queries = capture_sql do result = collection.list(caller, filter, projection) - expect(result.first['account']).not_to be_nil - expect(result.first['account']['account_history']).not_to be_nil + expect(result.first['supplier']['name']).to eq('ACME') end expect(queries.size).to eq(1) - expect(queries.first.scan(/LEFT OUTER JOIN/i).size).to eq(2) + expect(queries.first.scan(/LEFT OUTER JOIN/i).size).to eq(1) end - it 'selects ONLY the projected columns of the joined tables (not table.*)' do + it 'selects ONLY the projected columns of the joined table (not table.*)' do query = Utils::Query.new(collection, projection, filter) query.build sql = query.query.to_sql - expect(query.query.eager_load_values).to be_empty # eager_load would force account_histories.* - AccountHistory.column_names.reject { |c| c == 'id' }.each do |col| - expect(sql).not_to match(/account_histories"\."#{col}"/) + expect(query.query.eager_load_values).to be_empty # eager_load would force suppliers.* + selected = %w[id name] + Supplier.column_names.reject { |c| selected.include?(c) }.each do |col| + expect(sql).not_to match(/suppliers"\."#{col}"/) end end it 'keeps a constant query count regardless of the number of rows' do - 5.times { |i| Account.create!(supplier: Supplier.create!(name: "S#{i}"), account_history: AccountHistory.create!) } + 5.times { Account.create!(supplier: Supplier.create!(name: 'S'), account_history: AccountHistory.create!) } two = capture_sql { collection.list(caller, Filter.new(page: Page.new(limit: 2, offset: 0)), projection) } all = capture_sql { collection.list(caller, filter, projection) } @@ -78,17 +77,63 @@ def capture_sql end end + describe 'a has_one relation (supplier -> account) is left on preload' do + # has_one does not guarantee a single child row; a JOIN could duplicate the parent. + let(:collection) { Collection.new(datasource, Supplier) } + let(:projection) { Projection.new(['id', 'name', 'account:id']) } + + it 'is preloaded, not JOINed' do + query = Utils::Query.new(collection, projection, filter) + query.build + + expect(query.query.joins_values).to be_empty + expect(query.query.includes_values.to_s).to include('account') + expect(query.joined_relations).to be_empty + 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']) } + + it 'keeps preload (separate batched query), never a row-multiplying JOIN' do + query = Utils::Query.new(collection, projection, filter) + query.build + + expect(query.query.includes_values.to_s).to include('checks') + expect(query.query.joins_values).to be_empty + + result = collection.list(caller, filter, projection) + expect(result.first['checks'].first['garage_name']).to eq('Garage1') + end + end + describe 'safety guard: a target with a default_scope falls back to preload' do + # Car's default_scope has an unqualified column; a JOIN would raise "ambiguous column name". let(:collection) { Collection.new(datasource, Car) } let(:projection) { Projection.new(['id', 'reference', 'category:label']) } - # Car's default_scope has an unqualified column; a JOIN would raise "ambiguous column name". it 'does not JOIN and still returns correct data' do query = Utils::Query.new(collection, projection, filter) query.build - result = collection.list(caller, filter, projection) - expect(result.first['category']['label']).to eq('Compact') + expect(query.query.joins_values).to be_empty + expect(collection.list(caller, filter, projection).first['category']['label']).to eq('Compact') + end + end + + describe 'safety guard: a table already present in the query is not joined again' do + # ActiveRecord would alias a table joined twice; collect_joined_selects cannot reference + # that alias, so such a relation must fall back to preload. + let(:collection) { Collection.new(datasource, Account) } + let(:query) { Utils::Query.new(collection, Projection.new(['id', 'supplier:name']), filter) } + + it 'returns nil from joinable_tables when the target table is already used' do + joinable = query.send(:joinable_tables, collection, 'supplier', Projection.new(['name']), Set['accounts']) + expect(joinable).to eq(Set['suppliers']) + + already_used = query.send(:joinable_tables, collection, 'supplier', Projection.new(['name']), Set['accounts', 'suppliers']) + expect(already_used).to be_nil end end @@ -118,26 +163,10 @@ def capture_sql expect(query.send(:local_ar_collection, foreign_ds, 'Supplier')).to be_nil end - it 'fully_joinable? is false when the target cannot be resolved locally' do + it 'joinable_tables is nil when the target cannot be resolved locally' do allow(query).to receive(:local_ar_collection).and_return(nil) expect(collection.schema[:fields]['supplier'].type).to eq('ManyToOne') # joinable but for the stub - expect(query.send(:fully_joinable?, collection, 'supplier', Projection.new([]))).to be(false) - 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']) } - - it 'keeps preload (separate batched query), never a row-multiplying JOIN' do - query = Utils::Query.new(collection, projection, filter) - query.build - - expect(query.query.includes_values.to_s).to include('checks') - expect(query.query.eager_load_values).to be_empty - - result = collection.list(caller, filter, projection) - expect(result.first['checks'].first['garage_name']).to eq('Garage1') + expect(query.send(:joinable_tables, collection, 'supplier', Projection.new([]), Set['accounts'])).to be_nil end end end diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb index 0afea30d6..1de5322d6 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb @@ -18,14 +18,14 @@ module Utils describe 'build select' do context 'when projection have nested relation' do - it 'selects the current collection fields and JOINs the nested to-one relations' do - projection = Projection.new(%w[account_history:account:id account_history:account_id account_history:id]) + it 'selects the current collection foreign keys and JOINs the belongs_to relations' do + projection = Projection.new(%w[supplier:name account_history:id]) collection = Collection.new(datasource, Account) query_builder = described_class.new(collection, projection, Filter.new) query_builder.build - expect(query_builder.select).to include('accounts.account_history_id', 'accounts.id') - expect(query_builder.joined_relations.keys).to contain_exactly('account_history', 'account_history.account') + expect(query_builder.select).to include('accounts.supplier_id', 'accounts.account_history_id') + expect(query_builder.joined_relations.keys).to contain_exactly('supplier', 'account_history') end end end From 6b6ffa5f9d2398f379fb49d9ec87374087e926aa Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Thu, 2 Jul 2026 09:30:21 +0200 Subject: [PATCH 05/10] fix(datasource-active-record): support composite primary keys in joined selects (Macroscope review) target.model.primary_key returns an array for composite-key models; iterate Array(pk) so each key column gets its own aliased select instead of embedding the array as a single, invalid column reference. NULL detection uses the first key column's alias. Co-Authored-By: Claude Opus 4.8 --- .../forest_admin_datasource_active_record/utils/query.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 6457e7d94..ecdc7cac9 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 @@ -239,16 +239,16 @@ def collect_joined_selects(collection, relation_name, sub_projection, path) relation_schema = collection.schema[:fields][relation_name] target = local_ar_collection(collection.datasource, relation_schema.foreign_collection) table = target.model.table_name - pk = target.model.primary_key + pk_columns = Array(target.model.primary_key) # array for composite primary keys alias_map = {} - # pk is always selected so the serializer can detect a NULL (absent) left-joined relation - (sub_projection.columns + [pk]).uniq.each do |column| + # a pk column is always selected so the serializer can detect a NULL (absent) left-joined relation + (sub_projection.columns + pk_columns).uniq.each do |column| sql_alias = next_join_alias @select << %("#{table}"."#{column}" AS "#{sql_alias}") alias_map[column] = sql_alias end - @joined_relations[path.join('.')] = { columns: alias_map, pk_alias: alias_map[pk] } + @joined_relations[path.join('.')] = { columns: alias_map, pk_alias: alias_map[pk_columns.first] } sub_projection.relations.each do |nested_name, nested_projection| collect_joined_selects(target, nested_name, nested_projection, path + [nested_name]) From 495d8a77456d375fb14ade7bff21c3fab9a94b3a Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Thu, 2 Jul 2026 10:02:53 +0200 Subject: [PATCH 06/10] fix(datasource-active-record): join dedup with filter/sort, MySQL quoting, real pool check (Macroscope review) - Do not add a second JOIN for a relation already joined by a filter/sort: resolve_field records the joined table in @filter_joined_tables, and apply_select seeds used_tables with it so joinable_tables rejects it (falls back to preload). - Quote joined-select identifiers via the adapter (quote_table_name / quote_column_name) instead of ANSI double quotes, which are string literals on MySQL's default sql_mode. - same_database? compares connection_pool instead of connection_specification_name, which is only the owner class name and can be shared across different databases/shards. Co-Authored-By: Claude Opus 4.8 --- .../utils/query.rb | 15 +++++++-- .../utils/join_to_one_optimization_spec.rb | 33 +++++++++++++++++++ 2 files changed, 45 insertions(+), 3 deletions(-) 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 ecdc7cac9..58f558c38 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 @@ -17,6 +17,9 @@ def initialize(collection, projection, filter) # relation path (e.g. "bank_account.organizations_view") => { columns: { col => sql_alias }, pk_alias: } @joined_relations = {} @alias_counter = 0 + # tables already joined by apply_filter/apply_sort (resolve_field), so apply_select + # does not add a second, conflicting join for the same relation + @filter_joined_tables = Set.new end def build @@ -212,7 +215,7 @@ def apply_select unless @projection.nil? join_tree = {} preload_tree = {} - used_tables = Set[@collection.model.table_name] + used_tables = Set[@collection.model.table_name] | @filter_joined_tables @projection.relations.each do |relation_name, sub_projection| tables = joinable_tables(@collection, relation_name, sub_projection, used_tables) if tables @@ -238,6 +241,7 @@ def apply_select def collect_joined_selects(collection, relation_name, sub_projection, path) relation_schema = collection.schema[:fields][relation_name] target = local_ar_collection(collection.datasource, relation_schema.foreign_collection) + connection = target.model.connection table = target.model.table_name pk_columns = Array(target.model.primary_key) # array for composite primary keys @@ -245,7 +249,9 @@ def collect_joined_selects(collection, relation_name, sub_projection, path) # a pk column is always selected so the serializer can detect a NULL (absent) left-joined relation (sub_projection.columns + pk_columns).uniq.each do |column| sql_alias = next_join_alias - @select << %("#{table}"."#{column}" AS "#{sql_alias}") + # quote via the adapter so it works on MySQL too (ANSI "..." are string literals there) + @select << "#{connection.quote_table_name(table)}.#{connection.quote_column_name(column)} " \ + "AS #{connection.quote_column_name(sql_alias)}" alias_map[column] = sql_alias end @joined_relations[path.join('.')] = { columns: alias_map, pk_alias: alias_map[pk_columns.first] } @@ -291,7 +297,9 @@ def joinable_tables(collection, relation_name, sub_projection, used_tables) end def same_database?(model_a, model_b) - model_a.connection_specification_name == model_b.connection_specification_name + # compare the actual connection pools, not connection_specification_name (which is only the + # owner class name and can be shared across different databases/shards) + model_a.connection_pool == model_b.connection_pool rescue StandardError false end @@ -321,6 +329,7 @@ def resolve_field(original_field) relation = @collection.schema[:fields][relation_name] related_collection = @collection.datasource.get_collection(relation.foreign_collection) add_join_relation(relation_name) + @filter_joined_tables << related_collection.model.table_name { formatted: "#{related_collection.model.table_name}.#{column_name}", 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 37a31bbef..c9026949b 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 @@ -137,6 +137,39 @@ def capture_sql end end + describe 'safety guard: a relation already joined by a filter/sort is not joined again' do + let(:collection) { Collection.new(datasource, Account) } + let(:projection) { Projection.new(['id', 'supplier:name']) } + + it 'falls back to preload for a relation the filter already joined (no duplicate JOIN)' do + condition = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf + .new('supplier:name', 'Equal', 'ACME') + filter = Filter.new(condition_tree: condition) + + query = Utils::Query.new(collection, projection, filter) + query.build + + expect(query.joined_relations).to be_empty # supplier was already joined for the filter + expect(query.query.to_sql.scan(/LEFT OUTER JOIN "suppliers"/i).size).to eq(1) + + result = collection.list(caller, filter, projection) + expect(result.first['supplier']['name']).to eq('ACME') + end + end + + describe 'same_database?' do + let(:query) { Utils::Query.new(Collection.new(datasource, Account), Projection.new(['id']), filter) } + + it 'compares connection pools' do + model_a = Class.new { def self.connection_pool = :first_pool } + model_b = Class.new { def self.connection_pool = :second_pool } + + expect(query.send(:same_database?, model_a, model_a)).to be(true) + expect(query.send(:same_database?, model_a, model_b)).to be(false) + expect(query.send(:same_database?, Account, Supplier)).to be(true) + end + end + describe 'safety guard (belt-and-suspenders): target not local to this datasource' do let(:collection) { Collection.new(datasource, Account) } let(:query) { Utils::Query.new(collection, Projection.new(['id', 'supplier:name']), filter) } From ac04d57cdb9db6ce392bb53a869df2144b5d0499 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Thu, 2 Jul 2026 11:13:16 +0200 Subject: [PATCH 07/10] fix(datasource-active-record): don't JOIN scoped associations; safer connection & quoting (Macroscope review) - Fall back to preload when the belongs_to itself carries a scope (e.g. `belongs_to :x, -> { where('id > ?', 1) }`): the scope is applied to the JOIN and can inject raw/unqualified SQL or extra joins. joinable_target now also checks the association reflection's scope, not only the target model's default_scope. - Obtain the connection via connection_pool.with_connection (connection is deprecated on Rails 8) and quote joined-select identifiers through the adapter. - Split guards into joinable_target and the relation partitioning into split_relations for clarity; drop the always-true with_associations parameter. 161 examples, 0 failures; rubocop clean. Co-Authored-By: Claude Opus 4.8 --- .../utils/active_record_serializer.rb | 4 +- .../utils/query.rb | 95 +++++++++++-------- .../utils/join_to_one_optimization_spec.rb | 32 +++++-- 3 files changed, 83 insertions(+), 48 deletions(-) diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb index 5de24f381..896d0cac0 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb @@ -8,12 +8,12 @@ def to_hash(projection) hash_object(object, projection) end - def hash_object(object, projection = nil, path: [], with_associations: true) + def hash_object(object, projection = nil, path: []) return if object.nil? hash = base_attributes(object) - serialize_associations(object, projection, hash, path) if with_associations && projection + serialize_associations(object, projection, hash, path) if projection hash end 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 58f558c38..5c06e7216 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 @@ -213,19 +213,7 @@ def build_select(collection, projection) def apply_select unless @projection.nil? - join_tree = {} - preload_tree = {} - used_tables = Set[@collection.model.table_name] | @filter_joined_tables - @projection.relations.each do |relation_name, sub_projection| - tables = joinable_tables(@collection, relation_name, sub_projection, used_tables) - if tables - used_tables |= tables - join_tree[relation_name.to_sym] = format_relation_projection(sub_projection) - collect_joined_selects(@collection, relation_name, sub_projection, [relation_name]) - else - preload_tree[relation_name.to_sym] = format_relation_projection(sub_projection) - end - end + join_tree, preload_tree = split_relations # belongs_to relations on the same database are JOINed (selecting only their projected # columns, unlike eager_load which reads the whole row); the rest stays on preload, where @@ -238,21 +226,42 @@ def apply_select @query end + # Splits the projection's relations into a JOIN tree and a preload tree. + def split_relations + join_tree = {} + preload_tree = {} + used_tables = Set[@collection.model.table_name] | @filter_joined_tables + + @projection.relations.each do |relation_name, sub_projection| + tables = joinable_tables(@collection, relation_name, sub_projection, used_tables) + if tables + used_tables |= tables + join_tree[relation_name.to_sym] = format_relation_projection(sub_projection) + collect_joined_selects(@collection, relation_name, sub_projection, [relation_name]) + else + preload_tree[relation_name.to_sym] = format_relation_projection(sub_projection) + end + end + + [join_tree, preload_tree] + end + def collect_joined_selects(collection, relation_name, sub_projection, path) relation_schema = collection.schema[:fields][relation_name] target = local_ar_collection(collection.datasource, relation_schema.foreign_collection) - connection = target.model.connection table = target.model.table_name pk_columns = Array(target.model.primary_key) # array for composite primary keys alias_map = {} # a pk column is always selected so the serializer can detect a NULL (absent) left-joined relation - (sub_projection.columns + pk_columns).uniq.each do |column| - sql_alias = next_join_alias - # quote via the adapter so it works on MySQL too (ANSI "..." are string literals there) - @select << "#{connection.quote_table_name(table)}.#{connection.quote_column_name(column)} " \ - "AS #{connection.quote_column_name(sql_alias)}" - alias_map[column] = sql_alias + target.model.connection_pool.with_connection do |connection| + (sub_projection.columns + pk_columns).uniq.each do |column| + sql_alias = next_join_alias + # quote via the adapter so identifiers are valid on every database (e.g. backticks on MySQL) + @select << "#{connection.quote_table_name(table)}.#{connection.quote_column_name(column)} " \ + "AS #{connection.quote_column_name(sql_alias)}" + alias_map[column] = sql_alias + end end @joined_relations[path.join('.')] = { columns: alias_map, pk_alias: alias_map[pk_columns.first] } @@ -266,27 +275,13 @@ def next_join_alias "fa_join_#{@alias_counter}" end - # Set of tables the (fully joinable) subtree would add via JOIN, or nil when any relation - # in it cannot be safely joined. Only belongs_to is joinable: it matches on the target's - # primary key, so the JOIN cannot duplicate the parent row (a has_one child may not be - # unique). used_tables already covers the base + sibling joins: a table joined twice would - # be aliased by ActiveRecord, which collect_joined_selects cannot reference — so bail out. + # Set of tables the subtree would add via JOIN, or nil when any relation in it cannot be + # safely joined (see joinable_target). Recurses so the whole chain must be joinable. def joinable_tables(collection, relation_name, sub_projection, used_tables) - relation_schema = collection.schema[:fields][relation_name] - return nil unless relation_schema.type == 'ManyToOne' - return nil unless relation_schema.respond_to?(:foreign_collection) - - target = local_ar_collection(collection.datasource, relation_schema.foreign_collection) + target = joinable_target(collection, relation_name, used_tables) return nil if target.nil? - return nil unless same_database?(collection.model, target.model) - # a default_scope may inject raw/unqualified SQL (e.g. `where('id > ?', 10)`) that turns - # ambiguous once joined; preload keeps it isolated - return nil unless target.model.default_scopes.empty? - table = target.model.table_name - return nil if used_tables.include?(table) - - tables = Set[table] + tables = Set[target.model.table_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? @@ -296,6 +291,30 @@ 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. Only belongs_to qualifies: it matches on the target's primary key, so the JOIN + # cannot duplicate the parent row (a has_one child may not be unique). + def joinable_target(collection, relation_name, used_tables) + relation_schema = collection.schema[:fields][relation_name] + return unless relation_schema.type == 'ManyToOne' && 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) }`); the same risk exists for a + # default_scope on the target. Both keep it isolated when preloaded. + reflection = collection.model.reflect_on_association(relation_name.to_sym) + return if reflection.nil? || reflection.scope + + target = local_ar_collection(collection.datasource, relation_schema.foreign_collection) + return if target.nil? || !target.model.default_scopes.empty? + return unless same_database?(collection.model, target.model) + + # a table joined twice would be aliased by ActiveRecord, which collect_joined_selects + # cannot reference; bail out so it is preloaded instead + return if used_tables.include?(target.model.table_name) + + target + end + def same_database?(model_a, model_b) # compare the actual connection pools, not connection_specification_name (which is only the # owner class name and can be shared across different databases/shards) 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 c9026949b..b69c4daec 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 @@ -86,7 +86,7 @@ def capture_sql query = Utils::Query.new(collection, projection, filter) query.build - expect(query.query.joins_values).to be_empty + expect(query.query.left_outer_joins_values).to be_empty expect(query.query.includes_values.to_s).to include('account') expect(query.joined_relations).to be_empty end @@ -101,7 +101,7 @@ def capture_sql query.build expect(query.query.includes_values.to_s).to include('checks') - expect(query.query.joins_values).to be_empty + expect(query.query.left_outer_joins_values).to be_empty result = collection.list(caller, filter, projection) expect(result.first['checks'].first['garage_name']).to eq('Garage1') @@ -109,16 +109,32 @@ def capture_sql end describe 'safety guard: a target with a default_scope falls back to preload' do - # Car's default_scope has an unqualified column; a JOIN would raise "ambiguous column name". - let(:collection) { Collection.new(datasource, Car) } - let(:projection) { Projection.new(['id', 'reference', 'category:label']) } + # Car (the target here) has a default_scope with an unqualified column; a JOIN would raise + # "ambiguous column name", so user -> car must stay on preload. + let(:collection) { Collection.new(datasource, User) } + let(:projection) { Projection.new(['id', 'car:reference']) } - it 'does not JOIN and still returns correct data' do + it 'does not JOIN the scoped target' do query = Utils::Query.new(collection, projection, filter) query.build - expect(query.query.joins_values).to be_empty - expect(collection.list(caller, filter, projection).first['category']['label']).to eq('Compact') + expect(query.joined_relations).to be_empty + expect(query.query.left_outer_joins_values).to be_empty + expect(query.query.includes_values.to_s).to include('car') + end + end + + describe 'safety guard: a scoped belongs_to falls back to preload' do + # belongs_to :x, -> { ... } applies its scope to the JOIN (unqualified SQL / extra joins). + let(:collection) { Collection.new(datasource, Account) } + let(:query) { Utils::Query.new(collection, Projection.new(['id', 'supplier:name']), filter) } + + it 'does not JOIN an association that carries a scope' do + scoped = Account.reflect_on_association(:supplier) + allow(scoped).to receive(:scope).and_return(-> { where('id > 0') }) + allow(Account).to receive(:reflect_on_association).and_return(scoped) + + expect(query.send(:joinable_target, collection, 'supplier', Set['accounts'])).to be_nil end end From 6115cfab244248016b775c439a5c27fcc5291dc5 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Thu, 2 Jul 2026 17:12:25 +0200 Subject: [PATCH 08/10] refactor(datasource-active-record): serialize related records to their projected columns (review: bexchauveto) Previously a preloaded to-one was hydrated from its full row while a JOINed one was built from only the projected columns, so a related record's shape depended on how it was resolved (preload fallback vs JOIN). Now the preload path is restricted to the projected columns too, matching the JOINed hydration. The root record still exposes all its selected columns (own attributes + foreign keys); only related records are projection-restricted. Adds specs asserting a to-one relation returns exactly the projected columns whether it is JOINed (account -> supplier) or preloaded (supplier -> account). 163 examples, 0 failures; rubocop clean. Co-Authored-By: Claude Opus 4.8 --- .../utils/active_record_serializer.rb | 11 +++++++++-- .../utils/join_to_one_optimization_spec.rb | 13 +++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb index 896d0cac0..d1c49c04d 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb @@ -2,7 +2,8 @@ module ForestAdminDatasourceActiveRecord module Utils # Relations in `joined_relations` (see Query#collect_joined_selects) were resolved by JOIN # and are hydrated from the flat row's aliased columns; every other relation is read from - # its preloaded ActiveRecord association, as before. + # its preloaded ActiveRecord association. A related record is serialized to exactly its + # projected columns either way, so its shape does not depend on how it was resolved. ActiveRecordSerializer = Struct.new(:object, :joined_relations) do def to_hash(projection) hash_object(object, projection) @@ -11,7 +12,9 @@ def to_hash(projection) def hash_object(object, projection = nil, path: []) return if object.nil? - hash = base_attributes(object) + # the root keeps every selected column (own attributes + foreign keys); a related record + # is restricted to its projected columns, matching the JOINed hydration path + hash = path.empty? || projection.nil? ? base_attributes(object) : projected_columns(object, projection) serialize_associations(object, projection, hash, path) if projection @@ -24,6 +27,10 @@ def base_attributes(object) object.attributes.except(*join_aliases) end + def projected_columns(object, projection) + projection.columns.to_h { |column| [column, object[column]] } + end + def serialize_associations(object, projection, hash, path) one_associations = %i[has_one belongs_to] many_associations = %i[has_many has_and_belongs_to_many] 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 b69c4daec..7a2345090 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 @@ -92,6 +92,19 @@ def capture_sql end end + describe 'a related record exposes the same columns whether JOINed or preloaded' do + it 'returns exactly the projected columns for a JOINed to-one (account -> supplier)' do + result = Collection.new(datasource, Account).list(caller, filter, Projection.new(['id', 'supplier:name'])) + expect(result.first['supplier'].keys).to contain_exactly('name') + end + + it 'returns exactly the projected columns for a preloaded to-one (supplier -> account)' do + # supplier -> account is a has_one, so it is preloaded; it must still be projected-restricted + result = Collection.new(datasource, Supplier).list(caller, filter, Projection.new(['id', 'account:id'])) + expect(result.first['account'].keys).to contain_exactly('id') + 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 0a762a631530a4c4f3a4e575e22f54d2c8277960 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Thu, 2 Jul 2026 17:22:08 +0200 Subject: [PATCH 09/10] chore(datasource-active-record): trim comments to non-obvious rationale only Co-Authored-By: Claude Opus 4.8 --- .../utils/active_record_serializer.rb | 10 +++--- .../utils/query.rb | 34 ++++++------------- 2 files changed, 15 insertions(+), 29 deletions(-) diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb index d1c49c04d..b2c379ba7 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb @@ -1,9 +1,7 @@ module ForestAdminDatasourceActiveRecord module Utils - # Relations in `joined_relations` (see Query#collect_joined_selects) were resolved by JOIN - # and are hydrated from the flat row's aliased columns; every other relation is read from - # its preloaded ActiveRecord association. A related record is serialized to exactly its - # projected columns either way, so its shape does not depend on how it was resolved. + # Relations in `joined_relations` (see Query#collect_joined_selects) are hydrated from the flat + # row's aliased columns; every other relation is read from its preloaded ActiveRecord association. ActiveRecordSerializer = Struct.new(:object, :joined_relations) do def to_hash(projection) hash_object(object, projection) @@ -12,8 +10,8 @@ def to_hash(projection) def hash_object(object, projection = nil, path: []) return if object.nil? - # the root keeps every selected column (own attributes + foreign keys); a related record - # is restricted to its projected columns, matching the JOINed hydration path + # root keeps all its selected columns (attributes + FKs); a related record is restricted to + # its projected columns, matching the JOINed hydration hash = path.empty? || projection.nil? ? base_attributes(object) : projected_columns(object, projection) serialize_associations(object, projection, hash, path) if projection 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 5c06e7216..6462fed7f 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 @@ -17,8 +17,7 @@ def initialize(collection, projection, filter) # relation path (e.g. "bank_account.organizations_view") => { columns: { col => sql_alias }, pk_alias: } @joined_relations = {} @alias_counter = 0 - # tables already joined by apply_filter/apply_sort (resolve_field), so apply_select - # does not add a second, conflicting join for the same relation + # tables already joined by filters/sorts, so apply_select does not join them a second time @filter_joined_tables = Set.new end @@ -215,9 +214,6 @@ def apply_select unless @projection.nil? join_tree, preload_tree = split_relations - # belongs_to relations on the same database are JOINed (selecting only their projected - # columns, unlike eager_load which reads the whole row); the rest stays on preload, where - # a JOIN would multiply rows or is impossible. @query = @query.left_outer_joins(join_tree) unless join_tree.empty? @query = @query.includes(preload_tree) unless preload_tree.empty? end @@ -226,7 +222,6 @@ def apply_select @query end - # Splits the projection's relations into a JOIN tree and a preload tree. def split_relations join_tree = {} preload_tree = {} @@ -275,8 +270,7 @@ def next_join_alias "fa_join_#{@alias_counter}" end - # Set of tables the subtree would add via JOIN, or nil when any relation in it cannot be - # safely joined (see joinable_target). Recurses so the whole chain must be joinable. + # Set of tables the subtree adds via JOIN, or nil if any relation in it can't be safely joined. def joinable_tables(collection, relation_name, sub_projection, used_tables) target = joinable_target(collection, relation_name, used_tables) return nil if target.nil? @@ -291,41 +285,35 @@ 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. Only belongs_to qualifies: it matches on the target's primary key, so the JOIN - # cannot duplicate the parent row (a has_one child may not be unique). + # 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) # 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) }`); the same risk exists for a - # default_scope on the target. Both keep it isolated when preloaded. + # 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 target = local_ar_collection(collection.datasource, relation_schema.foreign_collection) - return if target.nil? || !target.model.default_scopes.empty? + return if target.nil? || !target.model.default_scopes.empty? # same risk as a scoped association return unless same_database?(collection.model, target.model) - - # a table joined twice would be aliased by ActiveRecord, which collect_joined_selects - # cannot reference; bail out so it is preloaded instead - return if used_tables.include?(target.model.table_name) + return if used_tables.include?(target.model.table_name) # a table joined twice would be aliased by AR target end def same_database?(model_a, model_b) - # compare the actual connection pools, not connection_specification_name (which is only the - # owner class name and can be shared across different databases/shards) + # compare the pools, not connection_specification_name (only an owner class name, shared across shards) model_a.connection_pool == model_b.connection_pool rescue StandardError false end - # The target collection if it is AR-backed AND belongs to this exact datasource, else nil. - # Class + identity (not just the name) guard against a foreign-datasource collection ever - # being reachable here (e.g. name collision in a composite) -> caller falls back to preload. + # The target collection only if it is AR-backed AND belongs to this exact datasource, else nil. + # Guards (concrete class + identity, not just the name) against a foreign-datasource collection. def local_ar_collection(datasource, name) collection = datasource.get_collection(name) return nil unless collection.is_a?(ForestAdminDatasourceActiveRecord::Collection) From 43e0e9220096ba3923c7dd28ce93a3cb1ae6d1aa Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Thu, 2 Jul 2026 17:25:41 +0200 Subject: [PATCH 10/10] fix(datasource-active-record): drop Set dependency in serializer (Macroscope review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit join_aliases used Enumerable#to_set without requiring 'set', which raises NoMethodError on Ruby 3.0/3.1 for any serialized record. Use uniq (an Array) instead — the value is only splatted into except(*...) and checked with empty?, so a Set is not needed. Co-Authored-By: Claude Opus 4.8 --- .../utils/active_record_serializer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb index b2c379ba7..8105b3cda 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/active_record_serializer.rb @@ -79,7 +79,7 @@ def joined_relation?(relation_path) end def join_aliases - @join_aliases ||= (joined_relations || {}).values.flat_map { |meta| meta[:columns].values }.to_set + @join_aliases ||= (joined_relations || {}).values.flat_map { |meta| meta[:columns].values }.uniq end end end