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..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 @@ -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.class, hash) serialize_associations(object, projection, hash, path) if projection @@ -65,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 @@ -72,8 +74,43 @@ def hash_joined_relation(projection, relation_path) hash end + def normalize_polymorphic_types(model_class, hash) + return hash if model_class.nil? + + 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 NameError => e + warn_unable(association.name, model_class, e) + 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 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 e39f96783..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 @@ -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 joins.nil? || 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,29 @@ 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[] + nil + end + + def signature(reflection) + "#{reflection.active_record.table_name}.#{Array(reflection.foreign_key).join(",")}" \ + "->#{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 7bd88ac6a..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 @@ -2,4 +2,6 @@ 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 + 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/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/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 new file mode 100644 index 000000000..f3f771f33 --- /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,59 @@ +require 'spec_helper' + +module ForestAdminDatasourceActiveRecord + 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 +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..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 @@ -172,6 +172,32 @@ 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, 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 describe 'a has_one :through with a has_one hop (supplier -> account_history) stays on preload' do @@ -239,22 +265,35 @@ 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' => 'accounts.supplier_id->suppliers.id') + + reused = query.send(:joinable_joins, collection, 'supplier', Projection.new(['name']), + { '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.id' }) + expect(conflicting).to be_nil # same target/FK from a different parent -> would alias + end - already_used = query.send(:joinable_tables, collection, 'supplier', Projection.new(['name']), Set['accounts', 'suppliers']) - expect(already_used).to be_nil + 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.id') end end @@ -317,10 +356,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