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 nullification of has_many :through association with `query_constraints #47721

Merged

Conversation

nvasilevski
Copy link
Contributor

@nvasilevski nvasilevski commented Mar 20, 2023

Given a has_many :through association with query_constraints:

BlogPost.has_many :posts_tags
BlogPost.has_many :tags,
  through: :posts_tags,
  query_constraints: [:blog_id, :post_id]

It is possible to nullify the association like blog_post.tags = []
which results in deletion of records from the posts_tags joining table.

Implementation details

The test we added fails on main with NoMethodError: undefined method to_sym for ["blog_id", "id"]:Array on the line

source_reflection.foreign_key => records.map(&association_primary_key.to_sym)

However this failure is misleading as we shouldn't be reaching this line at all, at least for the test we built.
In our case the issue lies a few lines above, at:

if association_primary_key == reflection.klass.primary_key && !options[:source_type]

Since the introduction of query_constraints in associations association_primary_key can be an array while primary_key may not necessarily be an array in case if the model uses virtual query_constraints definition. So essentially we need to compare association_primary_key with query_constraints_list if it is not empty and only then compare it with a primary key. To avoid branching I've introduced a new internal concept - composite_query_constraints_list this is an always non-empty array of query constraints that either equals to manually configured query_constraints or to the primary_key wrapped in an array. This concept will be useful in the future in internal codeplaces that are ready to work with an array of columns to avoid unnecessary branching.

After comparing primary keys as Arrays the logic follows into the branch and queries the records by the association name which has already been fixed in #47692

We still need to address the else branch but for this we will need to extend our Sharded fixtures to have an association with a custom source_type or query_constraints: configuration different from the parent's query_constraints

@eileencodes
Copy link
Member

@nvasilevski can you take a look at the test failures?

# Supposed to be used internally in Rails in places where the primary key value can always be treated as an array.
# This simpifies the code as it avoids branches `on query_constraints_list.nil?`
# Long-term this method can be removed and behavior added to the query_constraints_list itself.
# However until then we need to keep `query_constraints_list` nillable for backwards compatibility.
Copy link
Member

Choose a reason for hiding this comment

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

This documentation needs improvement, it's a bit more wordy than we need here. Can you replace it with something like

Returns an array of column names to be used in queries. The source of column 
names is derived from +query_constraints_list+ or +primary_key+. This method 
is for internal use when the primary key is to be treated as an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@eileencodes eileencodes self-assigned this Mar 21, 2023
…aints`

Given a `has_many :through` association with `query_constraints`:

```ruby
BlogPost.has_many :posts_tags
BlogPost.has_many :tags,
  through: :posts_tags,
  query_constraints: [:blog_id, :post_id]
``

It is possible to nullify the association like `blog_post.tags = []`
which results in deletion of records from the `posts_tags` joining table.
@nvasilevski nvasilevski force-pushed the fix-nullifying-has-many-through-association branch from cf41341 to 300c853 Compare March 21, 2023 13:51
@nvasilevski
Copy link
Contributor Author

can you take a look at the test failures?

Should be fixed now, I shouldn't have wrapped association_primary_key into an array for the whole flow. At this moment we should only wrap for the comparision

@eileencodes eileencodes merged commit ba19dbc into rails:main Mar 21, 2023
@eileencodes eileencodes deleted the fix-nullifying-has-many-through-association branch March 21, 2023 15:57
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.

None yet

2 participants