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..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 @@ -1,53 +1,85 @@ module ForestAdminDatasourceActiveRecord module Utils - ActiveRecordSerializer = Struct.new(:object) do + # 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) end - def hash_object(object, projection = nil, with_associations: true) - hash = {} - + def hash_object(object, projection = nil, path: []) return if object.nil? - hash.merge! object.attributes + # 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) if with_associations + serialize_associations(object, projection, hash, path) if 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 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] - # 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 + + # 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? + + 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 }.uniq 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 ee651a120..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 @@ -1,9 +1,11 @@ +require 'set' + module ForestAdminDatasourceActiveRecord 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 +14,11 @@ def initialize(collection, projection, filter) @filter = filter @arel_table = @collection.model.arel_table @select = [] + # relation path (e.g. "bank_account.organizations_view") => { columns: { col => sql_alias }, pk_alias: } + @joined_relations = {} + @alias_counter = 0 + # tables already joined by filters/sorts, so apply_select does not join them a second time + @filter_joined_tables = Set.new end def build @@ -204,12 +211,119 @@ def build_select(collection, projection) end def apply_select + unless @projection.nil? + join_tree, preload_tree = split_relations + + @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 - @query = @query.includes(format_relation_projection(@projection)) unless @projection.nil? @query end + 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) + 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 + 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] } + + 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 + + # 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? + + 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? + + tables |= nested + end + tables + 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) + 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) }`) + 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? # 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 + + target + end + + def same_database?(model_a, model_b) + # compare the pools, not connection_specification_name (only an owner class name, shared across shards) + model_a.connection_pool == model_b.connection_pool + rescue StandardError + false + end + + # 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) + 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) @@ -222,6 +336,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 new file mode 100644 index 000000000..7a2345090 --- /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,235 @@ +require 'spec_helper' + +module ForestAdminDatasourceActiveRecord + include ForestAdminDatasourceToolkit::Schema + include ForestAdminDatasourceToolkit::Components::Query + + 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 } + + before do + Account.delete_all + Supplier.delete_all + AccountHistory.delete_all + Check.delete_all + Car.unscoped.delete_all + Category.delete_all + + supplier = Supplier.create!(name: 'ACME') + 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 + 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 belongs_to relation (account -> supplier)' do + let(:collection) { Collection.new(datasource, Account) } + let(:projection) { Projection.new(['id', 'supplier:name']) } + + 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['supplier']['name']).to eq('ACME') + end + + expect(queries.size).to eq(1) + expect(queries.first.scan(/LEFT OUTER JOIN/i).size).to eq(1) + end + + 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 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 { 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) } + + expect(two.size).to eq(all.size) + expect(all.size).to eq(1) + 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.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 + 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']) } + + 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.left_outer_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 (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 the scoped target' do + query = Utils::Query.new(collection, projection, filter) + query.build + + 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 + + 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 + + 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) } + + 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 + # 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) + + 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) + foreign_ds = instance_double(Datasource, get_collection: foreign_collection) + + 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 '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(:joinable_tables, collection, 'supplier', Projection.new([]), Set['accounts'])).to be_nil + end + 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 404d3958c..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,13 +18,14 @@ 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 - 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 eq(%w[accounts.account_history_id accounts.id]) + 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