Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -100,21 +100,32 @@ def fetch_associations
source_polymorphic = association.source_reflection&.polymorphic? &&
association.options[:source_type].present?

add_field(
association.name.to_s,
ForestAdminDatasourceToolkit::Schema::Relations::ManyToManySchema.new(
foreign_collection: format_model_name(association.klass.name),
origin_key: through_reflection.foreign_key,
origin_key_target: through_reflection.join_foreign_key,
foreign_key: association.join_foreign_key,
foreign_key_target: association.association_primary_key,
through_collection: format_model_name(through_reflection.klass.name),
origin_type_field: is_polymorphic ? through_reflection.type : nil,
origin_type_value: is_polymorphic ? @model.name : nil,
foreign_type_field: source_polymorphic ? association.source_reflection.foreign_type : nil,
foreign_type_value: source_polymorphic ? association.options[:source_type] : nil
if is_polymorphic || source_polymorphic
add_field(
association.name.to_s,
ForestAdminDatasourceToolkit::Schema::Relations::ManyToManySchema.new(
foreign_collection: format_model_name(association.klass.name),
origin_key: through_reflection.foreign_key,
origin_key_target: through_reflection.join_foreign_key,
foreign_key: association.join_foreign_key,
foreign_key_target: association.association_primary_key,
through_collection: format_model_name(through_reflection.klass.name),
origin_type_field: is_polymorphic ? through_reflection.type : nil,
origin_type_value: is_polymorphic ? @model.name : nil,
foreign_type_field: source_polymorphic ? association.source_reflection.foreign_type : nil,
foreign_type_value: source_polymorphic ? association.options[:source_type] : nil
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 15 lines of similar code in 2 locations (mass = 94) [qlty:similar-code]

)
)
else
add_field(
association.name.to_s,
ForestAdminDatasourceToolkit::Schema::Relations::OneToOneSchema.new(
foreign_collection: format_model_name(association.klass.name),
origin_key: association.klass.primary_key,
origin_key_target: @model.primary_key
)
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deeply nested control flow (level = 4) [qlty:nested-control-flow]

end
elsif association.inverse_of&.polymorphic?
add_field(
association.name.to_s,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,10 @@ def build_select(collection, projection)
if collection.model.table_name == @collection.model.table_name
if one_to_one_relations.include?(relation_schema.type)
@select << "#{collection.model.table_name}.#{relation_schema.origin_key_target}"
# foreign_key is an array for a composite-key belongs_to through hop
Array(root_through_foreign_key(collection, relation_name)).each do |through_fk|
@select << "#{collection.model.table_name}.#{through_fk}"
end
elsif many_to_one_relations.include?(relation_schema.type)
@select << "#{collection.model.table_name}.#{relation_schema.foreign_key}"
end
Expand Down Expand Up @@ -275,7 +279,7 @@ 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]
tables = Set[target.model.table_name] | through_tables(collection, relation_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?
Expand All @@ -288,23 +292,50 @@ def joinable_tables(collection, relation_name, sub_projection, used_tables)
# 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)
return unless 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

case relation_schema.type
when 'ManyToOne' then nil
when 'OneToOne' then return unless belongs_to_chain_through?(reflection)
else return
end

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function with many returns (count = 8): joinable_target [qlty:return-statements]

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]
rescue StandardError
Set[]
end

def belongs_to_chain_through?(reflection)
return false unless reflection.through_reflection?

through = reflection.through_reflection
source = reflection.source_reflection

through && source && through.belongs_to? && source.belongs_to? &&

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Complex binary expression [qlty:boolean-logic]

through.scope.nil? && source.scope.nil? && through.klass.default_scopes.empty? &&
same_database?(reflection.active_record, through.klass)
rescue StandardError
false
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
Expand All @@ -330,6 +361,15 @@ def add_join_relation(relation_name)
@query
end

def root_through_foreign_key(collection, relation_name)
through = collection.model.reflect_on_association(relation_name.to_sym)&.through_reflection
return unless through&.belongs_to?

through.foreign_key
rescue StandardError
nil
end

def resolve_field(original_field)
if original_field.include?(':')
relation_name, column_name = original_field.split(':')
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
class Account < ApplicationRecord
belongs_to :supplier
belongs_to :account_history
has_one :order, through: :account_history
end
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
class AccountHistory < ApplicationRecord
has_one :account
belongs_to :order, optional: true
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddOrderToAccountHistories < ActiveRecord::Migration[7.1]
def change
add_reference :account_histories, :order, null: true, foreign_key: true
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,16 @@ module ForestAdminDatasourceActiveRecord
expect(collection.schema[:fields].keys).to include('users')
end

it 'add has_one_through relation' do
it 'add has_one_through relation as a to-one (OneToOne)' do
collection = described_class.new(datasource, Supplier)

expect(collection.schema[:fields].keys).to include('account_history')

expect(collection.schema[:fields]['account_history'].class).to eq(Relations::ManyToManySchema)
field = collection.schema[:fields]['account_history']
expect(field.class).to eq(Relations::OneToOneSchema)
expect(field.foreign_collection).to eq('AccountHistory')
expect(field.origin_key).to eq(AccountHistory.primary_key)
expect(field.origin_key_target).to eq(Supplier.primary_key)
end

it 'skips association when foreign_key raises an error' do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,98 @@ def capture_sql
end
end

describe 'a has_one :through of belongs_to hops (account -> account_history -> order)' do
let(:collection) { Collection.new(datasource, Account) }
let(:projection) { Projection.new(['id', 'order:reference']) }

before do
order = Order.create!(reference: 'ORD-1')
Account.first.account_history.update!(order: order)
end

it 'folds the whole chain into ONE JOINed query (was 3 with per-hop preload)' do
queries = capture_sql do
result = collection.list(caller, filter, projection)
expect(result.first['order']['reference']).to eq('ORD-1')
end

expect(queries.size).to eq(1)
expect(queries.first.scan(/LEFT OUTER JOIN/i).size).to eq(2)
end

it 'filters on a field of the through relation' do
condition = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf
.new('order:reference', 'Equal', 'ORD-1')
result = collection.list(caller, Filter.new(condition_tree: condition), projection)

expect(result.size).to eq(1)
expect(result.first['order']['reference']).to eq('ORD-1')
end

it 'returns nothing when the through relation value does not match' do
condition = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf
.new('order:reference', 'Equal', 'NOPE')
result = collection.list(caller, Filter.new(condition_tree: condition), Projection.new(['id']))

expect(result).to be_empty
end

it 'aggregates grouped by a field of the through relation' do
aggregation = Aggregation.new(operation: 'Count', field: nil, groups: [{ field: 'order:reference' }])
result = Utils::QueryAggregate.new(collection, aggregation).get

expect(result).to contain_exactly('value' => 1, 'group' => { 'order:reference' => 'ORD-1' })
end

it 'does not JOIN the intermediate table twice when it is also projected on its own' 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)

result = collection.list(caller, filter, projection)
expect(result.first['order']['reference']).to eq('ORD-1')
expect(result.first['account_history']['id']).to eq(Account.first.account_history_id)
end

it 'does not JOIN the intermediate table twice when a filter already joined the through' do
projection = Projection.new(['id', 'account_history:id'])
condition = ForestAdminDatasourceToolkit::Components::Query::ConditionTree::Nodes::ConditionTreeLeaf
.new('order:reference', 'Equal', 'ORD-1')
query = Utils::Query.new(collection, projection, Filter.new(condition_tree: condition))
query.build

expect(query.query.to_sql.scan(/JOIN "account_histories"/i).size).to eq(1)

result = collection.list(caller, Filter.new(condition_tree: condition), projection)
expect(result.first['account_history']['id']).to eq(Account.first.account_history_id)
end
end

describe 'a has_one :through with a has_one hop (supplier -> account_history) stays on preload' do
let(:collection) { Collection.new(datasource, Supplier) }
let(:projection) { Projection.new(['id', 'account_history: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_history')
end

it 'does not select the intermediate FK against the root table (it lives on the child)' do
query = Utils::Query.new(collection, projection, filter)
query.build
sql = query.query.to_sql

expect(sql).not_to match(/suppliers"\."supplier_id"/)
expect(sql).not_to match(/suppliers"\."account_id"/)
expect { collection.list(caller, filter, projection) }.not_to raise_error
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']) }
Expand Down
Loading