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

Use foreign_key to reference parent association #548

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

slucaskim
Copy link
Contributor

@slucaskim slucaskim commented Nov 29, 2023

After a db update happens on a child record, identity_cache has mechanisms to invalidate the cache on associated parent records that have the child record as an embedded cache association (ex. cache_has_many :children, embed: true). This works even for "old" parent records where the child incurs an update where it no longer belongs to the old parent - identity cache is designed to invalidate the cache on the old parent record as well.

To know which parent records to invalidate the cache for, identity_cache first understands which attributes on the child records changed, then detects which of those changes are for parent associations. The second step uses the Rails association_foreign_key attribute. This works in most cases, but there is a bug when it comes to associations defined with custom foreign keys. Take the following example:

class Parent < ActiveRecord::Base
  has_many :children, foreign_key: :person_id
end

class Child < ActiveRecord::Base
  belongs_to :parent, foreign_key: :person_id
end

Child.reflect_on_association(:parent).association_foreign_key
=> "parent_id"
Child.reflect_on_association(:parent).foreign_key
=> "person_id"

When calling association_foreign_key on the association, Rails gives back parent_id, which is inappropriate for us to use in the context of detecting attribute changes on the child record. The correct key to use is person_id, ie the actual db column that ties the two models together, which is what gets passed back with foreign_key.

From the Rails docs, it seems that association_foreign_key is intended to be useful for has_and_belongs_to_many type associations (option is only defined under that section). While from the docs, it's clear foreign_key is a well-supported option across all the association types.

Looking at the history of the repo, association_foreign_key was used since the very early days of the gem. I believe it was introduced in error, and the bug hasn't been prominent enough to warrant a revisit.

✅ Ran CI on shopify core to ensure no regression

@slucaskim slucaskim force-pushed the fix-association-foreign-key-reference branch from ecbeadf to a30abc9 Compare November 29, 2023 20:53
@slucaskim slucaskim force-pushed the fix-association-foreign-key-reference branch from a30abc9 to 26a4bcf Compare November 29, 2023 21:15
@slucaskim slucaskim requested a review from cbothner November 29, 2023 21:15
@slucaskim slucaskim marked this pull request as ready for review November 29, 2023 21:15
@slucaskim slucaskim requested a review from mitchvoll November 29, 2023 21:15
Copy link
Member

@cbothner cbothner left a comment

Choose a reason for hiding this comment

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

I was interested in how specifically has_and_belongs_to_many associations made use of these two fields. Of course, this is irrelevant because this gem doesn't support has_and_belongs_to_many associations. Just interesting

class Store < ActiveRecord::Base
  has_and_belongs_to_many :customers
end

class Customer < ActiveRecord::Base
  has_and_belongs_to_many :stores
end

Store.reflect_on_association(:customers).foreign_key # => "store_id"
Store.reflect_on_association(:customers).association_foreign_key # => "customer_id"

@slucaskim slucaskim merged commit b391146 into main Dec 4, 2023
5 checks passed
@slucaskim slucaskim deleted the fix-association-foreign-key-reference branch December 4, 2023 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants