Support composite foreign key association assignments #47230

nvasilevski · 2023-02-02T15:02:21Z

This PR is built on top of #46962 and adds support for association assignments for Active Record models that use composite foreign and primary keys.

The high-level approach is pretty simple: Prepare code for the primary_key and the foreign_key to always be an array by temporarily wrapping it using Array() and assign all foreign key columns accordingly

What reviewers should focus on

You may have noticed that implementation has some duplication that can possibly be extracted into it's own concept and reused to avoid repetition. The logic goes the following way: "Given an array of foreign_key column names and an array of primary_key column names create a mapping of foreign key column names to the primary key column names, or vice-versa. For example:

primary_key = [:blog_id, :id]
foreign_key = [:blog_id, :blog_post_id]
# create a mapping like 
# { blog_id: :blog_id, id: :blog_post_id }
# which is currently achieved through "zipping" the values
primary_key.zip(foreign_key) # => [[:blog_id, :blog_id], [:id, :blog_post_id]]
# [[:blog_id, :blog_id], [:id, :blog_post_id]] is basically a mapping, it's just unnecessary to turn it into an actual Hash for our use-cases

However I'm afraid of DRYing it up prematurely and either coming up with a wrongly named concept or placing the abstraction in a wrong place. Though most likely it will end up being a Reflection concept. Let me know if you think it is a good time to avoid duplication but I would prefer to wait for more usage examples to have a bigger picture

nvasilevski · 2023-02-02T15:14:51Z

activerecord/lib/active_record/autosave_association.rb

@@ -505,8 +505,14 @@ def save_belongs_to_association(reflection)
            saved = record.save(validate: !autosave) if record.new_record? || (autosave && record.changed_for_autosave?)

            if association.updated?
-              association_id = record.public_send(reflection.options[:primary_key] || :id)
-              self[reflection.foreign_key] = association_id unless self[reflection.foreign_key] == association_id
+              primary_key = Array(reflection.options[:primary_key] || reflection.active_record_primary_key)


I want to specifically highlight the :id => reflection.active_record_primary_key change. I believe usage of :id here was a little confusing as it may appear like we are hardcoding the "id column value" in this context while actually :id here is being used as a method call which by implication returns the @primary_key value. So similar change could have happened even out of the scope of this PR.

In scope of this PR the change has to happen as we transition to a higher-level query_constraints_list abstraction which in this case may be treated as a "virtual primary key" and for all existing models it equals to [:id] . And we have following options for making the change:

Ask the record for columns names like record.class.query_constraints_list. I don't like this option as it couples AutosaveAssociation module with lower-level query_constraints model concept.

Ask reflection for the primary key value. This is my preferable choice as reflection is what actually defines how associations should behave in a given context.

I end up using record.class.query_constraints_list as asking reflection for the value breaks a few corner-cases which I missed. I still feel like that the value should be derived through reflection and not directly from the record though I'm not familiar with the associations internals enough to be confident. Using record.class.query_constraints_list narrows down the scope of the change and avoid breaking these corner-cases

eileencodes

Requested a minor change, otherwise looks good.

eileencodes · 2023-02-02T20:45:20Z

activerecord/lib/active_record/associations/foreign_association.rb

@@ -22,8 +22,15 @@ def nullified_owner_attributes
      def set_owner_attributes(record)
        return if options[:through]

-        key = owner._read_attribute(reflection.join_foreign_key)
-        record._write_attribute(reflection.join_primary_key, key)
+        primary_key = Array.wrap(reflection.join_primary_key)


Suggested change

primary_key = Array.wrap(reflection.join_primary_key)

primary_key = Array(reflection.join_primary_key)

Fixed, thanks!
CI is red but it looks like it's just unstable as rubies 2.7 and 3.1 are green

eileencodes · 2023-02-02T20:45:28Z

activerecord/lib/active_record/associations/foreign_association.rb

-        key = owner._read_attribute(reflection.join_foreign_key)
-        record._write_attribute(reflection.join_primary_key, key)
+        primary_key = Array.wrap(reflection.join_primary_key)
+        foreign_key = Array.wrap(reflection.join_foreign_key)


Suggested change

foreign_key = Array.wrap(reflection.join_foreign_key)

foreign_key = Array(reflection.join_foreign_key)

Given a model with a composite primary key, it is possible to assign the association associated with the model by the composite foreign key. For example, given two `Sharded::BlogPost` and `Sharded::Comment` models that are associated using a composite foreign key so that: `Sharded::BlogPost.has_many :comments, foreign_key: [:blog_id, :blog_post_id]`, it is possible to assign an instance of `Comment` to an instance of `BlogPost` like `blog_post.comments << comment`. The same applies to the opposing `:belongs_to` association.

etiennebarrie · 2023-02-06T17:01:39Z

activerecord/lib/active_record/autosave_association.rb

+              primary_key = Array(reflection.options[:primary_key] || record.class.query_constraints_list)
+              foreign_key = Array(reflection.foreign_key)
+
+              primary_key_foreign_key_pairs = primary_key.zip(foreign_key)


This breaks unless the belongs_to is properly configured. I can see that in the fixtures we have Sharded::Comment.belongs_to :blog_post, foreign_key: [:blog_id, :blog_post_id], but if you don't, you get an error down the stack.

rails/activerecord/test/models/sharded/comment.rb

Line 8 in 08f0b74

belongs_to :blog_post, foreign_key: [:blog_id, :blog_post_id]

Given the configuration of Sharded::Blog:

rails/activerecord/test/models/sharded/blog_post.rb

Line 6 in 08f0b74

query_constraints :blog_id, :id

If the foreign key on Comment is not set up properly: blog_post_id is mistakenly set to blog_id, and we try to set an attribute with an empty name to the id.

Since it fails anyway, should we raise if primary_key and foreign_keys don't have the same size?
We could also fallback to just :id as the primary_key and use the old behaviour, but I guess long term we want the query constraints to be used in autosave too…

Since it fails anyway, should we raise if primary_key and foreign_keys don't have the same size?

That's a fair option, though I still wanted to explore the possibility of deriving a foreign key like it's done here:
https://github.com/rails/rails/blob/0f5ac79e3e5e716f8699760239a34478b3a0b403/activerecord/lib/active_record/reflection.rb#L720-L727
We already know how to derive blog_post_id part of the key as this is the existing convention, so I was wondering if there is a way to tell that blog_id is the other part of the foreign key based on [:blog_id, :id] association primary key. But if there won't be a convenient way of doing that we should definitely raise if sizes don't match

rails-bot bot added the activerecord label Feb 2, 2023

nvasilevski commented Feb 2, 2023

View reviewed changes

nvasilevski force-pushed the composite-foreign-key-assoc-assignment branch from 8a49538 to ea347c9 Compare February 2, 2023 15:58

eileencodes requested changes Feb 2, 2023

View reviewed changes

nvasilevski force-pushed the composite-foreign-key-assoc-assignment branch from ea347c9 to fda7cf3 Compare February 2, 2023 21:03

nvasilevski requested a review from eileencodes February 2, 2023 21:24

eileencodes approved these changes Feb 2, 2023

View reviewed changes

eileencodes merged commit 5defeec into rails:main Feb 2, 2023

eileencodes deleted the composite-foreign-key-assoc-assignment branch February 2, 2023 21:34

etiennebarrie reviewed Feb 6, 2023

View reviewed changes

nvasilevski mentioned this pull request Feb 13, 2023

Refactor query constraints feature set #47378

Merged

nvasilevski mentioned this pull request Feb 28, 2023

Support assignments for composite key has_many through associations #47541

Merged

nvasilevski mentioned this pull request Apr 17, 2023

Fix replacing foreign key for a CPK association by id attribute #47924

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support composite foreign key association assignments #47230

Support composite foreign key association assignments #47230

nvasilevski commented Feb 2, 2023

nvasilevski Feb 2, 2023

nvasilevski Feb 2, 2023

eileencodes left a comment

eileencodes Feb 2, 2023

nvasilevski Feb 2, 2023

eileencodes Feb 2, 2023

etiennebarrie Feb 6, 2023

nvasilevski Feb 6, 2023

	primary_key = Array.wrap(reflection.join_primary_key)
	primary_key = Array(reflection.join_primary_key)

	foreign_key = Array.wrap(reflection.join_foreign_key)
	foreign_key = Array(reflection.join_foreign_key)

Support composite foreign key association assignments #47230

Support composite foreign key association assignments #47230

Conversation

nvasilevski commented Feb 2, 2023

What reviewers should focus on

nvasilevski Feb 2, 2023

Choose a reason for hiding this comment

nvasilevski Feb 2, 2023

Choose a reason for hiding this comment

eileencodes left a comment

Choose a reason for hiding this comment

eileencodes Feb 2, 2023

Choose a reason for hiding this comment

nvasilevski Feb 2, 2023

Choose a reason for hiding this comment

eileencodes Feb 2, 2023

Choose a reason for hiding this comment

etiennebarrie Feb 6, 2023

Choose a reason for hiding this comment

nvasilevski Feb 6, 2023

Choose a reason for hiding this comment