Skip to content
Open
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 High forest_admin_datasource_active_record/collection.rb:119

When a non-polymorphic has_one :through association is processed, the OneToOneSchema is created with origin_key: association.klass.primary_key and origin_key_target: @model.primary_key, which describes a direct primary-key-to-primary-key link. Downstream write paths (update_related, associate_related, store) use those fields to update the child collection, so updating a has_one :through relation writes the target model's primary key (e.g. AccountHistory.id) instead of the intermediate join table's foreign key, corrupting identifiers or breaking the relation update. Consider using through_reflection.foreign_key / through_reflection.join_foreign_key (or the intermediate collection's keys) as the origin_key / origin_key_target, consistent with the ManyToManySchema branch above.

Also found in 1 other location(s)

packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb:308

Enabling OneToOne joins at joinable_target without also tracking the intermediate through table breaks the duplicate-join safeguard. For a projection like ['order:reference', 'account_history:id'] on Account, the new has_one :through branch treats order as joinable but joinable_tables only records the final orders table, even though ActiveRecord must also join account_histories for order. The later account_history relation is therefore considered joinable too, so the same table is joined twice and ActiveRecord aliases one occurrence. collect_joined_selects and resolve_field still refer to the plain account_histories table name, which can generate incorrect SQL/alias references or serialize data from the wrong join.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb around line 119:

When a non-polymorphic `has_one :through` association is processed, the `OneToOneSchema` is created with `origin_key: association.klass.primary_key` and `origin_key_target: @model.primary_key`, which describes a direct primary-key-to-primary-key link. Downstream write paths (`update_related`, `associate_related`, `store`) use those fields to update the child collection, so updating a `has_one :through` relation writes the target model's primary key (e.g. `AccountHistory.id`) instead of the intermediate join table's foreign key, corrupting identifiers or breaking the relation update. Consider using `through_reflection.foreign_key` / `through_reflection.join_foreign_key` (or the intermediate collection's keys) as the `origin_key` / `origin_key_target`, consistent with the `ManyToManySchema` branch above.

Also found in 1 other location(s):
- packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb:308 -- Enabling `OneToOne` joins at `joinable_target` without also tracking the intermediate `through` table breaks the duplicate-join safeguard. For a projection like `['order:reference', 'account_history:id']` on `Account`, the new `has_one :through` branch treats `order` as joinable but `joinable_tables` only records the final `orders` table, even though ActiveRecord must also join `account_histories` for `order`. The later `account_history` relation is therefore considered joinable too, so the same table is joined twice and ActiveRecord aliases one occurrence. `collect_joined_selects` and `resolve_field` still refer to the plain `account_histories` table name, which can generate incorrect SQL/alias references or serialize data from the wrong join.

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}"
# a has_one :through preloads via the intermediate FK on this table; select it so a
# relation that falls back to preload (e.g. it is also used by a filter) still loads.
through_fk = through_foreign_key(collection, relation_name)
@select << "#{collection.model.table_name}.#{through_fk}" if through_fk
Comment on lines +201 to +202

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 High utils/query.rb:201

build_select calls through_foreign_key for every OneToOne relation and appends the result to @select prefixed with the root table name. For a has_one :through where the intermediate hop is itself a has_one (e.g. Supplier.has_one :account_history, through: :account), through_foreign_key returns the child table's foreign key (accounts.supplier_id), not a column on the root table. This emits SQL like SELECT suppliers.supplier_id, and the database raises an unknown-column error whenever such a relation is projected. The fix should only select the intermediate FK when it actually lives on the root table — i.e. when the through reflection is a belongs_to.

-              through_fk = through_foreign_key(collection, relation_name)
-              @select << "#{collection.model.table_name}.#{through_fk}" if through_fk
+              through_fk = root_through_foreign_key(collection, relation_name)
+              @select << "#{collection.model.table_name}.#{through_fk}" if through_fk
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb around lines 201-202:

`build_select` calls `through_foreign_key` for every `OneToOne` relation and appends the result to `@select` prefixed with the root table name. For a `has_one :through` where the intermediate hop is itself a `has_one` (e.g. `Supplier.has_one :account_history, through: :account`), `through_foreign_key` returns the child table's foreign key (`accounts.supplier_id`), not a column on the root table. This emits SQL like `SELECT suppliers.supplier_id`, and the database raises an unknown-column error whenever such a relation is projected. The fix should only select the intermediate FK when it actually lives on the root table — i.e. when the `through` reflection is a `belongs_to`.

elsif many_to_one_relations.include?(relation_schema.type)
@select << "#{collection.model.table_name}.#{relation_schema.foreign_key}"
end
Expand Down Expand Up @@ -285,18 +289,26 @@ def joinable_tables(collection, relation_name, sub_projection, used_tables)
tables
end

# The target collection when this hop is safe to collapse into a JOIN, else nil (-> preload).
# The target collection when this single hop is safe to collapse into a LEFT OUTER JOIN,
# else nil. A belongs_to qualifies: it matches on the target's primary key, so the JOIN
# cannot duplicate the parent row (a plain has_one child may not be unique). A has_one :through
# also qualifies when every hop is a belongs_to (see belongs_to_chain_through?): AR resolves the
# intermediate join, saving the extra per-hop preload queries.
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)
Expand All @@ -305,6 +317,21 @@ def joinable_target(collection, relation_name, 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 = 7): joinable_target [qlty:return-statements]

end

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

through = reflection.through_reflection
source = reflection.source_reflection
return false unless through && source
return false unless through.belongs_to? && source.belongs_to?
return false if through.scope || source.scope
return false unless through.klass.default_scopes.empty?

same_database?(reflection.active_record, through.klass)
rescue StandardError
false

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 2 issues:

1. Function with many returns (count = 5): belongs_to_chain_through? [qlty:return-statements]


2. Function with high complexity (count = 8): belongs_to_chain_through? [qlty:function-complexity]

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 +357,16 @@ def add_join_relation(relation_name)
@query
end

# The intermediate foreign key a has_one :through uses on this table, or nil for a plain to-one.
def through_foreign_key(collection, relation_name)
reflection = collection.model.reflect_on_association(relation_name.to_sym)
return unless reflection&.through_reflection?

reflection.through_reflection.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,65 @@ def capture_sql
end
end

describe 'a has_one :through of belongs_to hops (account -> account_history -> order)' do
# every hop is a belongs_to, so the chained JOIN stays 1:1 and cannot multiply the parent.
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) # account_histories + orders
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
end

describe 'a has_one :through with a has_one hop (supplier -> account_history) stays on preload' do
# the intermediate `account` hop is a has_one and can multiply rows, so it must not be JOINed.
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
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