fix(datasource-active-record): normalize demodulized polymorphic types & reuse shared joins (#328)

Author: bexchauveto

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,15 +66,51 @@ 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
 
         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

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)

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

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

packages/forest_admin_datasource_active_record/spec/dummy/app/models/api/topic.rb

@@ -0,0 +1,4 @@
+module Api
+  class Topic < ApplicationRecord
+  end
+end

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

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

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

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