From 14ef31b9ee2d23d53043f98d9c2b9476df915090 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Fri, 3 Jul 2026 10:29:06 +0200 Subject: [PATCH 1/5] fix(datasource-active-record): normalize demodulized polymorphic types on read [local] Local-only workaround so Forest can resolve the target collection when the app stores a demodulized polymorphic *_type (e.g. "Income" for Api::Income). Not for the has_one:through PR; the real fix lives in feat/support-polymorphic-qonto. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../utils/active_record_serializer.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) 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 8105b3cda..1b92d89fb 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 @@ -13,6 +13,7 @@ def hash_object(object, projection = nil, 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) + hash = normalize_polymorphic_types(object, hash) serialize_associations(object, projection, hash, path) if projection @@ -72,6 +73,23 @@ def hash_joined_relation(projection, relation_path) hash end + # local: the app stores demodulized polymorphic types (e.g. "Income" for Api::Income), but + # Forest identifies the target collection by the full class name (Api__Income). Translate the + # stored *_type back through the model's polymorphic_class_for. No-op for a default app. + def normalize_polymorphic_types(object, hash) + object.class.reflect_on_all_associations(:belongs_to).each do |association| + next unless association.polymorphic? + + stored = hash[association.foreign_type] + next if stored.nil? + + hash = hash.merge(association.foreign_type => object.class.polymorphic_class_for(stored).name) + rescue StandardError + next + end + hash + end + private def joined_relation?(relation_path) From bfcff9317e35273a8123d1fbc60c60fd3fb491a9 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Fri, 3 Jul 2026 11:42:21 +0200 Subject: [PATCH 2/5] perf(datasource-active-record): reuse a shared join instead of preloading a has_one :through Track joins by signature (their ON condition) rather than table name, so a has_one :through whose intermediate table is also joined by a belongs_to of the same association reuses that single join (ActiveRecord dedupes it) instead of falling back to per-hop preload queries. A belongs_to reaching the same table via a different foreign key still bails to preload, since ActiveRecord would alias it and collect_joined_selects cannot reference the alias. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../utils/active_record_serializer.rb | 3 -- .../utils/query.rb | 50 ++++++++++++------- .../spec/dummy/app/models/account.rb | 1 + ...00005_add_secondary_history_to_accounts.rb | 5 ++ .../utils/join_to_one_optimization_spec.rb | 46 +++++++++++++---- 5 files changed, 72 insertions(+), 33 deletions(-) create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20251126100005_add_secondary_history_to_accounts.rb 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 1b92d89fb..3d29e858f 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 @@ -73,9 +73,6 @@ def hash_joined_relation(projection, relation_path) hash end - # local: the app stores demodulized polymorphic types (e.g. "Income" for Api::Income), but - # Forest identifies the target collection by the full class name (Api__Income). Translate the - # stored *_type back through the model's polymorphic_class_for. No-op for a default app. def normalize_polymorphic_types(object, hash) object.class.reflect_on_all_associations(:belongs_to).each do |association| next unless association.polymorphic? 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 e39f96783..096bd899a 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 @@ -229,12 +229,13 @@ def apply_select def split_relations join_tree = {} preload_tree = {} - used_tables = Set[@collection.model.table_name] | @filter_joined_tables + used_joins = { @collection.model.table_name => :root } + @filter_joined_tables.each { |table| used_joins[table] ||= :filter } # never reuse a filter/sort join @projection.relations.each do |relation_name, sub_projection| - tables = joinable_tables(@collection, relation_name, sub_projection, used_tables) - if tables - used_tables |= tables + joins = joinable_joins(@collection, relation_name, sub_projection, used_joins) + if joins + used_joins.merge!(joins) join_tree[relation_name.to_sym] = format_relation_projection(sub_projection) collect_joined_selects(@collection, relation_name, sub_projection, [relation_name]) else @@ -275,22 +276,28 @@ def next_join_alias end # 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) + def joinable_joins(collection, relation_name, sub_projection, used_joins) + target = joinable_target(collection, relation_name) return nil if target.nil? - tables = Set[target.model.table_name] | through_tables(collection, relation_name) + joins = join_signatures(collection, relation_name) + return nil if conflicting?(joins, used_joins) + sub_projection.relations.each do |nested_name, nested_projection| - nested = joinable_tables(target, nested_name, nested_projection, used_tables | tables) + nested = joinable_joins(target, nested_name, nested_projection, used_joins.merge(joins)) return nil if nested.nil? - tables |= nested + joins = joins.merge(nested) end - tables + joins + end + + def conflicting?(new_joins, used_joins) + new_joins.any? { |table, signature| used_joins.key?(table) && used_joins[table] != signature } end # The target collection when this hop is safe to collapse into a JOIN, else nil (-> preload). - def joinable_target(collection, relation_name, used_tables) + def joinable_target(collection, relation_name) relation_schema = collection.schema[:fields][relation_name] return unless relation_schema.respond_to?(:foreign_collection) @@ -308,19 +315,24 @@ def joinable_target(collection, relation_name, used_tables) 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] + def join_signatures(collection, relation_name) + reflection = collection.model.reflect_on_association(relation_name.to_sym) + if reflection.through_reflection? + { reflection.through_reflection.klass.table_name => signature(reflection.through_reflection), + reflection.klass.table_name => signature(reflection.source_reflection) } + else + { reflection.klass.table_name => signature(reflection) } + end rescue StandardError - Set[] + {} + end + + def signature(reflection) + "#{Array(reflection.foreign_key).join(",")}->#{reflection.klass.table_name}" end def belongs_to_chain_through?(reflection) 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 7bd88ac6a..33aa5beae 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 @@ -2,4 +2,5 @@ class Account < ApplicationRecord belongs_to :supplier belongs_to :account_history has_one :order, through: :account_history + belongs_to :secondary_history, class_name: 'AccountHistory', optional: true end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20251126100005_add_secondary_history_to_accounts.rb b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20251126100005_add_secondary_history_to_accounts.rb new file mode 100644 index 000000000..b32e50733 --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20251126100005_add_secondary_history_to_accounts.rb @@ -0,0 +1,5 @@ +class AddSecondaryHistoryToAccounts < ActiveRecord::Migration[7.1] + def change + add_column :accounts, :secondary_history_id, :integer, null: true + end +end diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/join_to_one_optimization_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/join_to_one_optimization_spec.rb index 021eb2fa6..dbdd18c3c 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 @@ -172,6 +172,24 @@ def capture_sql result = collection.list(caller, Filter.new(condition_tree: condition), projection) expect(result.first['account_history']['id']).to eq(Account.first.account_history_id) end + + it 'reuses the intermediate join when a plain belongs_to shares the same signature (one query)' 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) + expect(query.query.includes_values).to be_empty + end + + it 'preloads a belongs_to that would alias the through intermediate (same table, different FK)' do + projection = Projection.new(['id', 'order:reference', 'secondary_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) + expect(query.query.includes_values.to_s).to include('secondary_history') + end end describe 'a has_one :through with a has_one hop (supplier -> account_history) stays on preload' do @@ -239,22 +257,28 @@ def capture_sql 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 + expect(query.send(:joinable_target, collection, 'supplier')).to be_nil 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. + describe 'safety guard: a table already present in the query is not joined with a different signature' do + # ActiveRecord reuses a join with the same ON condition, but aliases one with a different + # condition; collect_joined_selects cannot reference that alias, so a conflicting 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']) + it 'reuses the join for a matching signature and bails on a conflicting one' do + joinable = query.send(:joinable_joins, collection, 'supplier', Projection.new(['name']), { 'accounts' => :root }) + expect(joinable).to eq('suppliers' => 'supplier_id->suppliers') + + reused = query.send(:joinable_joins, collection, 'supplier', Projection.new(['name']), + { 'accounts' => :root, 'suppliers' => 'supplier_id->suppliers' }) + expect(reused).to eq('suppliers' => 'supplier_id->suppliers') # same signature -> reused, not aliased - already_used = query.send(:joinable_tables, collection, 'supplier', Projection.new(['name']), Set['accounts', 'suppliers']) - expect(already_used).to be_nil + conflicting = query.send(:joinable_joins, collection, 'supplier', Projection.new(['name']), + { 'accounts' => :root, 'suppliers' => 'other_id->suppliers' }) + expect(conflicting).to be_nil # different signature -> would alias end end @@ -317,10 +341,10 @@ def capture_sql expect(query.send(:local_ar_collection, foreign_ds, 'Supplier')).to be_nil end - it 'joinable_tables is nil when the target cannot be resolved locally' do + it 'joinable_joins 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(:joinable_tables, collection, 'supplier', Projection.new([]), Set['accounts'])).to be_nil + expect(query.send(:joinable_joins, collection, 'supplier', Projection.new([]), { 'accounts' => :root })).to be_nil end end end From e100c137d60c7466179f8c303caa8d5c62998ae4 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Fri, 3 Jul 2026 14:21:41 +0200 Subject: [PATCH 3/5] fix(datasource-active-record): scope join signature by source table Two associations joining the same target table with the same FK name from different parents produced identical signatures, so conflicting? treated them as one join and allowed reuse. AR aliases the second, but collect_joined_selects reads the unaliased table.column and got the wrong join's data. Include the source table in the signature. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../utils/query.rb | 3 ++- .../utils/join_to_one_optimization_spec.rb | 17 ++++++++++++----- 2 files changed, 14 insertions(+), 6 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 096bd899a..2bede15cd 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 @@ -332,7 +332,8 @@ def join_signatures(collection, relation_name) end def signature(reflection) - "#{Array(reflection.foreign_key).join(",")}->#{reflection.klass.table_name}" + "#{reflection.active_record.table_name}.#{Array(reflection.foreign_key).join(",")}" \ + "->#{reflection.klass.table_name}" end def belongs_to_chain_through?(reflection) 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 dbdd18c3c..8a2304083 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 @@ -270,15 +270,22 @@ def capture_sql it 'reuses the join for a matching signature and bails on a conflicting one' do joinable = query.send(:joinable_joins, collection, 'supplier', Projection.new(['name']), { 'accounts' => :root }) - expect(joinable).to eq('suppliers' => 'supplier_id->suppliers') + expect(joinable).to eq('suppliers' => 'accounts.supplier_id->suppliers') reused = query.send(:joinable_joins, collection, 'supplier', Projection.new(['name']), - { 'accounts' => :root, 'suppliers' => 'supplier_id->suppliers' }) - expect(reused).to eq('suppliers' => 'supplier_id->suppliers') # same signature -> reused, not aliased + { 'accounts' => :root, 'suppliers' => 'accounts.supplier_id->suppliers' }) + expect(reused).to eq('suppliers' => 'accounts.supplier_id->suppliers') # same signature -> reused, not aliased conflicting = query.send(:joinable_joins, collection, 'supplier', Projection.new(['name']), - { 'accounts' => :root, 'suppliers' => 'other_id->suppliers' }) - expect(conflicting).to be_nil # different signature -> would alias + { 'accounts' => :root, 'suppliers' => 'account_histories.order_id->suppliers' }) + expect(conflicting).to be_nil # same target/FK from a different parent -> would alias + end + + it 'scopes a signature by its source table so same target/FK from different parents differ' do + # the through order hop joins orders FROM account_histories; a direct belongs_to :order would + # join orders FROM accounts. Same FK name, same target -> must not share a signature. + sigs = query.send(:join_signatures, collection, 'order') + expect(sigs['orders']).to eq('account_histories.order_id->orders') end end From 4e5d8a0891a0df498f261d8add0b9964ddce8fef Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Fri, 3 Jul 2026 14:35:44 +0200 Subject: [PATCH 4/5] fix(datasource-active-record): normalize polymorphic types on JOINed relations too hash_joined_relation read projected *_type columns straight from the aliased root row, so a JOINed polymorphic belongs_to returned the raw stored value while the preloaded path returned the fully-qualified class name. Normalize the joined hash against the relation's target model (resolved by walking the reflection chain) so both paths agree. Also wrap the join signature string to satisfy Layout/LineLength. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../utils/active_record_serializer.rb | 20 ++++++++++++---- .../utils/active_record_serializer_spec.rb | 23 +++++++++++++++++++ 2 files changed, 39 insertions(+), 4 deletions(-) create mode 100644 packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/active_record_serializer_spec.rb 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 3d29e858f..8b66c9711 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 @@ -13,7 +13,7 @@ def hash_object(object, projection = nil, 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) - hash = normalize_polymorphic_types(object, hash) + hash = normalize_polymorphic_types(object.class, hash) serialize_associations(object, projection, hash, path) if projection @@ -66,6 +66,7 @@ def hash_joined_relation(projection, relation_path) hash = {} projection.columns.each { |column| hash[column] = object[meta[:columns][column]] } + hash = normalize_polymorphic_types(target_model(relation_path), hash) projection.relations.each_key do |nested_name| hash[nested_name] = hash_joined_relation(projection.relations[nested_name], relation_path + [nested_name]) end @@ -73,20 +74,31 @@ def hash_joined_relation(projection, relation_path) hash end - def normalize_polymorphic_types(object, hash) - object.class.reflect_on_all_associations(:belongs_to).each do |association| + def normalize_polymorphic_types(model_class, hash) + return hash if model_class.nil? + + model_class.reflect_on_all_associations(:belongs_to).each do |association| next unless association.polymorphic? stored = hash[association.foreign_type] next if stored.nil? - hash = hash.merge(association.foreign_type => object.class.polymorphic_class_for(stored).name) + hash = hash.merge(association.foreign_type => model_class.polymorphic_class_for(stored).name) rescue StandardError next end hash end + # Target model of a JOINed relation path (only belongs_to / has_one :through are ever JOINed). + def target_model(relation_path) + relation_path.reduce(object.class) do |model, name| + model&.reflect_on_association(name.to_sym)&.klass + end + rescue StandardError + nil + end + private def joined_relation?(relation_path) diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/active_record_serializer_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/active_record_serializer_spec.rb new file mode 100644 index 000000000..cbe73e09e --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/active_record_serializer_spec.rb @@ -0,0 +1,23 @@ +require 'spec_helper' + +module ForestAdminDatasourceActiveRecord + module Utils + describe ActiveRecordSerializer do + subject(:serializer) { described_class.new(Account.new, {}) } + + describe '#target_model' do + it 'resolves a belongs_to hop to its target model' do + expect(serializer.target_model(['supplier'])).to eq(Supplier) + end + + it 'resolves a has_one :through chain to the final target model' do + expect(serializer.target_model(['order'])).to eq(Order) + end + + it 'returns nil when a hop is not an association' do + expect(serializer.target_model(['not_a_relation'])).to be_nil + end + end + end + end +end From c9caa9818c25d88f5eb2f99213d6904483b3d1b0 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Fri, 3 Jul 2026 15:17:27 +0200 Subject: [PATCH 5/5] fix(datasource-active-record): fold target join key into join signature; test polymorphic normalization Address review (matthv): - signature: a belongs_to ON clause is target.join_key = source.fk; two associations with the same source/FK/target but a different :primary_key had identical signatures and were wrongly reused (AR aliases the 2nd join, collect_joined_selects reads the unaliased name -> wrong record served). Encode the target join key so a differing ON yields a differing signature (-> conflict -> safe preload). - join_signatures now returns nil on error -> preload, instead of {} which left the relation joinable but invisible to conflict detection. - normalize_polymorphic_types: narrow rescue to NameError + log, and memoize the polymorphic belongs_to lookup per model_class. - target_model: narrow rescue to NameError. - Add a namespaced polymorphic fixture (Api::Note/Api::Topic, stored demodulized) and assert normalization on both the preloaded and JOINed paths (previously untested / not reproducible with existing fixtures). - Preload-fallback spec now runs list() and asserts the secondary relation is served its own row, not the collapsed through row. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../utils/active_record_serializer.rb | 22 ++++-- .../utils/query.rb | 10 ++- .../spec/dummy/app/models/account.rb | 1 + .../spec/dummy/app/models/api/note.rb | 7 ++ .../spec/dummy/app/models/api/topic.rb | 4 ++ .../20260703120000_create_notes_and_topics.rb | 12 ++++ .../utils/active_record_serializer_spec.rb | 68 ++++++++++++++----- .../utils/join_to_one_optimization_spec.rb | 26 ++++--- 8 files changed, 116 insertions(+), 34 deletions(-) create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/app/models/api/note.rb create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/app/models/api/topic.rb create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260703120000_create_notes_and_topics.rb 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 8b66c9711..f49cf9297 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 @@ -77,15 +77,13 @@ def hash_joined_relation(projection, relation_path) def normalize_polymorphic_types(model_class, hash) return hash if model_class.nil? - model_class.reflect_on_all_associations(:belongs_to).each do |association| - next unless association.polymorphic? - + polymorphic_belongs_to(model_class).each do |association| stored = hash[association.foreign_type] next if stored.nil? hash = hash.merge(association.foreign_type => model_class.polymorphic_class_for(stored).name) - rescue StandardError - next + rescue NameError => e + warn_unable(association.name, model_class, e) end hash end @@ -95,12 +93,24 @@ def target_model(relation_path) relation_path.reduce(object.class) do |model, name| model&.reflect_on_association(name.to_sym)&.klass end - rescue StandardError + rescue NameError nil end private + def polymorphic_belongs_to(model_class) + (@polymorphic_belongs_to ||= {})[model_class] ||= + model_class.reflect_on_all_associations(:belongs_to).select(&:polymorphic?) + end + + def warn_unable(name, model_class, error) + ActiveSupport::Logger.new($stdout).warn( + "[ForestAdmin] Unable to normalize polymorphic type of '#{name}' " \ + "in model '#{model_class.name}': #{error.message}. Keeping the stored value." + ) + end + def joined_relation?(relation_path) !joined_relations.nil? && joined_relations.key?(relation_path.join('.')) 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 2bede15cd..12a800f48 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 @@ -281,7 +281,7 @@ def joinable_joins(collection, relation_name, sub_projection, used_joins) return nil if target.nil? joins = join_signatures(collection, relation_name) - return nil if conflicting?(joins, used_joins) + return nil if joins.nil? || conflicting?(joins, used_joins) sub_projection.relations.each do |nested_name, nested_projection| nested = joinable_joins(target, nested_name, nested_projection, used_joins.merge(joins)) @@ -328,12 +328,16 @@ def join_signatures(collection, relation_name) { reflection.klass.table_name => signature(reflection) } end rescue StandardError - {} + nil end def signature(reflection) "#{reflection.active_record.table_name}.#{Array(reflection.foreign_key).join(",")}" \ - "->#{reflection.klass.table_name}" + "->#{reflection.klass.table_name}.#{Array(join_key(reflection)).join(",")}" + end + + def join_key(reflection) + reflection.respond_to?(:join_primary_key) ? reflection.join_primary_key : reflection.association_primary_key end def belongs_to_chain_through?(reflection) 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 33aa5beae..88269b837 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 @@ -3,4 +3,5 @@ class Account < ApplicationRecord belongs_to :account_history has_one :order, through: :account_history belongs_to :secondary_history, class_name: 'AccountHistory', optional: true + belongs_to :note, class_name: 'Api::Note', optional: true end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/api/note.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/api/note.rb new file mode 100644 index 000000000..38fe92cb4 --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/api/note.rb @@ -0,0 +1,7 @@ +module Api + class Note < ApplicationRecord + # legacy/demodulized polymorphic types: the column stores "Topic", not "Api::Topic" + self.store_full_class_name = false + belongs_to :notable, polymorphic: true, optional: true + end +end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/api/topic.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/api/topic.rb new file mode 100644 index 000000000..84fab02e1 --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/api/topic.rb @@ -0,0 +1,4 @@ +module Api + class Topic < ApplicationRecord + end +end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260703120000_create_notes_and_topics.rb b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260703120000_create_notes_and_topics.rb new file mode 100644 index 000000000..2b03cbd4c --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20260703120000_create_notes_and_topics.rb @@ -0,0 +1,12 @@ +class CreateNotesAndTopics < ActiveRecord::Migration[7.1] + def change + create_table :topics + + create_table :notes do |t| + t.string :notable_type + t.integer :notable_id + end + + add_column :accounts, :note_id, :integer, null: true + end +end diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/active_record_serializer_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/active_record_serializer_spec.rb index cbe73e09e..f3f771f33 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/active_record_serializer_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/active_record_serializer_spec.rb @@ -1,22 +1,58 @@ require 'spec_helper' module ForestAdminDatasourceActiveRecord - module Utils - describe ActiveRecordSerializer do - subject(:serializer) { described_class.new(Account.new, {}) } - - describe '#target_model' do - it 'resolves a belongs_to hop to its target model' do - expect(serializer.target_model(['supplier'])).to eq(Supplier) - end - - it 'resolves a has_one :through chain to the final target model' do - expect(serializer.target_model(['order'])).to eq(Order) - end - - it 'returns nil when a hop is not an association' do - expect(serializer.target_model(['not_a_relation'])).to be_nil - end + include ForestAdminDatasourceToolkit::Components::Query + + describe Utils::ActiveRecordSerializer do + subject(:serializer) { described_class.new(Account.new, {}) } + + describe '#target_model' do + it 'resolves a belongs_to hop to its target model' do + expect(serializer.target_model(['supplier'])).to eq(Supplier) + end + + it 'resolves a has_one :through chain to the final target model' do + expect(serializer.target_model(['order'])).to eq(Order) + end + + it 'returns nil when a hop is not an association' do + expect(serializer.target_model(['not_a_relation'])).to be_nil + end + end + + # Api::Topic is namespaced but stored demodulized as "Topic" (store_full_class_name = false on + # Api::Note), so polymorphic_class_for("Topic").name == "Api::Topic" -> a real transform. + # Without normalization these assertions would read the raw "Topic". + describe 'polymorphic type normalization', :db_truncation do + let(:datasource) { Datasource.new({ adapter: 'sqlite3', database: 'db/database.db' }) } + let(:note) do + n = Api::Note.create! + n.update_columns(notable_type: 'Topic', notable_id: Api::Topic.create!.id) # legacy demodulized value + n.reload + end + + before do + Account.delete_all + Api::Note.delete_all + Api::Topic.delete_all + end + + it 'qualifies the stored type on the preloaded (hash_object) path' do + result = described_class.new(note, {}).to_hash(Projection.new(['id', 'notable_type'])) + expect(result['notable_type']).to eq('Api::Topic') + end + + it 'qualifies the stored type on the JOINed (hash_joined_relation) path' do + account = Account.create!(supplier: Supplier.create!(name: 'ACME'), + account_history: AccountHistory.create!, note: note) + + query = Utils::Query.new(Collection.new(datasource, Account), Projection.new(['id', 'note:notable_type']), + Filter.new) + query.build + expect(query.joined_relations).to have_key('note') # proves the JOINed path, not preload + + result = Collection.new(datasource, Account).list(nil, Filter.new, Projection.new(['id', 'note:notable_type'])) + expect(result.find { |r| r['id'] == account.id }['note']['notable_type']).to eq('Api::Topic') end end end diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/join_to_one_optimization_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/join_to_one_optimization_spec.rb index 8a2304083..1f85af423 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 @@ -182,13 +182,21 @@ def capture_sql expect(query.query.includes_values).to be_empty end - it 'preloads a belongs_to that would alias the through intermediate (same table, different FK)' do + it 'preloads a belongs_to that would alias the through intermediate, and serves the right rows' do + secondary = AccountHistory.create! # a row distinct from the account's own account_history + Account.first.update!(secondary_history: secondary) + projection = Projection.new(['id', 'order:reference', 'secondary_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) expect(query.query.includes_values.to_s).to include('secondary_history') + + # a wrong merge would collapse the two account_histories joins and serve secondary the through row + result = collection.list(caller, filter, projection) + expect(result.first['secondary_history']['id']).to eq(secondary.id) + expect(result.first['order']['reference']).to eq('ORD-1') end end @@ -270,22 +278,22 @@ def capture_sql it 'reuses the join for a matching signature and bails on a conflicting one' do joinable = query.send(:joinable_joins, collection, 'supplier', Projection.new(['name']), { 'accounts' => :root }) - expect(joinable).to eq('suppliers' => 'accounts.supplier_id->suppliers') + expect(joinable).to eq('suppliers' => 'accounts.supplier_id->suppliers.id') reused = query.send(:joinable_joins, collection, 'supplier', Projection.new(['name']), - { 'accounts' => :root, 'suppliers' => 'accounts.supplier_id->suppliers' }) - expect(reused).to eq('suppliers' => 'accounts.supplier_id->suppliers') # same signature -> reused, not aliased + { 'accounts' => :root, 'suppliers' => 'accounts.supplier_id->suppliers.id' }) + expect(reused).to eq('suppliers' => 'accounts.supplier_id->suppliers.id') # same signature -> reused, not aliased conflicting = query.send(:joinable_joins, collection, 'supplier', Projection.new(['name']), - { 'accounts' => :root, 'suppliers' => 'account_histories.order_id->suppliers' }) + { 'accounts' => :root, 'suppliers' => 'account_histories.order_id->suppliers.id' }) expect(conflicting).to be_nil # same target/FK from a different parent -> would alias end - it 'scopes a signature by its source table so same target/FK from different parents differ' do - # the through order hop joins orders FROM account_histories; a direct belongs_to :order would - # join orders FROM accounts. Same FK name, same target -> must not share a signature. + it 'scopes a signature by its source table and target join key' do + # the through order hop joins orders FROM account_histories ON orders.id = account_histories.order_id; + # a differing source table OR target :primary_key must yield a differing signature. sigs = query.send(:join_signatures, collection, 'order') - expect(sigs['orders']).to eq('account_histories.order_id->orders') + expect(sigs['orders']).to eq('account_histories.order_id->orders.id') end end