Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix #51129] Fix issue with IDs reader on preloaded associations for composite primary keys #51167

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Jay0921
Copy link

@Jay0921 Jay0921 commented Feb 22, 2024

Motivation / Background

Fixes #51129.

Detail

When using composite primary keys in a model, the primary_key will be an array. This raises an issue when calling the <association>_ids method on a preloaded association. Internally, Rails uses the pluck method from Enumerable to retrieve the preloaded results. However, the pluck method does not accept an array as its argument. To accommodate that, we need to use the splat operator to split the array into multiple arguments dynamically.

Additional information

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

Copy link

@Slotos Slotos left a comment

Choose a reason for hiding this comment

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

Thank you so much for tackling this.

elsif !target.empty?
load_target.pluck(reflection.association_primary_key)
load_target.pluck(*reflection.association_primary_key)
else
@association_ids ||= scope.pluck(reflection.association_primary_key)
Copy link

@Slotos Slotos Feb 23, 2024

Choose a reason for hiding this comment

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

I believe this should also use splat operator. Not doing so result in primary keys not getting wrapped into an arel attribute node, which could lead to differences in behaviour in complex scenarios.

Specifically, in ActiveRecord::Calculations#pluck, the argument gets passed to ActiveRecord::QueryMethods#arel_columns, which doesn't have a case for processing array argument, and simply returns it as is. Meaning that scope.pluck(:id, :tenant_id) and scope.pluck([:id, :tenant_id]) are subtly different.

Low level difference:

> User.all.__send__(:arel_columns, [:id, :tenant_id])
=> 
[#<struct Arel::Attributes::Attribute
  relation=
   #<Arel::Table:0x0000000122b14f18
    @klass=User(id: uuid, email: string, password_digest: string, client: boolean, active: boolean, created_at: datetime, updated_at: datetime, practise_id: uuid, full_name: string),
    @name="users",
    @table_alias=nil,
    @type_caster=
     #<ActiveRecord::TypeCaster::Map:0x0000000122ef9a48
      @klass=User(id: uuid, email: string, password_digest: string, client: boolean, active: boolean, created_at: datetime, updated_at: datetime, practise_id: uuid, full_name: string)>>,
  name="id">,
 "\"tenant_id\""]


> User.all.__send__(:arel_columns, [[:id, :tenant_id]])
=> [:id, :tenant_id]

I couln't devise a scenario where this actually breaks things, but by adding a splat operator here, array and singular primary keys get handled the same way, lowering the likelihood of bugs down the line.

Copy link
Author

Choose a reason for hiding this comment

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

@Slotos After diving into the code, I found that using pluck(:id, :post_id) and pluck([:id, :post_id]) will produce the same results. The arguments that passed to pluck method will go through the arel_columns method twice.

The first call occurs when we try to extract the value and assign it to selected_values.

columns = arel_columns(column_names)
relation = spawn
relation.select_values = columns

The second call occurs during the construction of the select query, where we call the arel_columns method again on selected_values, even though we had already done so once and assigned the value to selected_values.

def build_select(arel)
if select_values.any?
arel.project(*arel_columns(select_values))
elsif klass.ignored_columns.any? || klass.enumerate_columns_in_select_statements
arel.project(*klass.column_names.map { |field| table[field] })
else
arel.project(table[Arel.star])
end
end


When using pluck(:id, :post_id):
The first call goes to this block and generates some Arel objects and assign to selected_values

when Symbol
arel_column(field.to_s) do |attr_name|
connection.quote_table_name(attr_name)
end

selected_values is an array of Arel objects. When we call arel_columns for the second time, we hit the else block and return the values as they are

However, the process is entirely different when using pluck([:id, :post_id]). In this case, the first call passes through the else block because the argument is an array. The second call, however, matches the Symbol case, thus generating the Arel objects.

Copy link
Author

Choose a reason for hiding this comment

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

The second call on arel_columns seems unnecessary. The issue is related to how flat_map is used inside the arel_columns method.

At first glance, it might seem that [[:name, :age]].flat_map { |field| field } and [:name, :age].flat_map { |field| field } behave the same since they produce the same output. However,the underlying process reveals a critical difference, particularly in this case. The main difference is the value of class in both cases

[:id, :post_id].flat_map { |field| field.class }
# Outputs: [Symbol, Symbol]
[[:id, :post_id]].flat_map { |field| field.class }
# Outputs: [Array]

Changing the code as shown below will eliminate the need for the second call. However, I'm not sure about the rationale behind using flat_map. Nonetheless, I think this issue deserves a PR on its own. I will investigate further and open one if necessary.

[:id, :post_id].flatten.map { |field| field.class }
# Outputs: [Symbol, Symbol]
[[:id, :post_id]].flatten.map { |field| field.class }
# Outputs: [Symbol, Symbol]

@Jay0921 Jay0921 requested a review from Slotos February 25, 2024 04:30
else
@association_ids ||= scope.pluck(reflection.association_primary_key)
@association_ids ||= scope.pluck(*reflection.association_primary_key)
Copy link
Author

@Jay0921 Jay0921 Feb 25, 2024

Choose a reason for hiding this comment

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

The doc show pluck with multiple arguments like (:id, :name), but ([:id, :name]) also works. To align with docs, we should use the splat operator here.

Copy link
Contributor

@nvasilevski nvasilevski left a comment

Choose a reason for hiding this comment

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

Let's squash commits so when someone from the core team gives a review they can merge immediately if all looks good

@Jay0921 Jay0921 force-pushed the fix-preload-ids-reader-composite-pk branch from 92daaa2 to dcf7984 Compare April 5, 2024 15:39
@Jay0921 Jay0921 force-pushed the fix-preload-ids-reader-composite-pk branch from dcf7984 to 9db868f Compare April 6, 2024 05:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array primary keys (e.g. composite) break collection_singular_ids method when using preload
3 participants