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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Apr 12, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions activerecord/lib/active_record/autosave_association.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,12 +505,12 @@ def save_belongs_to_association(reflection)
saved = record.save(validate: !autosave) if record.new_record? || (autosave && record.changed_for_autosave?)

if association.updated?
primary_key = Array(compute_primary_key(reflection, record))
primary_key = Array(compute_primary_key(reflection, record)).map(&:to_s)
foreign_key = Array(reflection.foreign_key)

primary_key_foreign_key_pairs = primary_key.zip(foreign_key)
primary_key_foreign_key_pairs.each do |primary_key, foreign_key|
association_id = record.public_send(primary_key)
association_id = record._read_attribute(primary_key)
self[foreign_key] = association_id unless self[foreign_key] == association_id
end
association.loaded!
Expand All @@ -527,7 +527,7 @@ def compute_primary_key(reflection, record)
elsif reflection.options[:query_constraints] && (query_constraints = record.class.query_constraints_list)
query_constraints
else
:id
record.class.primary_key
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just wanted to add a bit of context on this change. This is something we have already discussed in
https://github.com/rails/rails/pull/47378/files#r1104639481
and
#47230 (comment)

Hardcoded :id value was incorrect but was working because :id through object.public_send(:id) translates to whichever column being used as a primary key. So if the primary key is title column - object.public_send(:id) returns the title value. But it wasn't working well with a composite primary key where id column is a part of the key so in order for it to work properly we changed it to be dynamically derived so if the record's primary key is title the compute_primary_key method returns "title"

end
end

Expand Down
16 changes: 16 additions & 0 deletions activerecord/test/cases/associations_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,22 @@ def test_assign_composite_foreign_key_belongs_to_association
assert_equal(another_blog.id, comment.blog_id)
end


def test_assign_belongs_to_cpk_model_by_id_attribute
order = cpk_orders(:cpk_groceries_order_1)
agreement = Cpk::OrderAgreement.new(signature: "signed")

agreement.order = order
agreement.save

assert_not_nil(agreement.reload.order)
assert_not_nil(agreement.order_id)

assert_equal(order, agreement.order)
_shop_id, order_id = order.id
assert_equal(order_id, agreement.order_id)
end

def test_append_composite_foreign_key_has_many_association_with_autosave
blog_post = sharded_blog_posts(:great_post_blog_one)
comment = Sharded::Comment.new(body: "Great post! :clap:")
Expand Down