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 replacing foreign key for a CPK association by id attribute #47924

Merged

Conversation

nvasilevski
Copy link
Contributor

Given a model associated with a composite primary key by id attribute, for example:

Order.primary_key = [:shop_id, :id]
OrderAgreement.primary_key = :id

OrderAgreement.belongs_to :order, primary_key: :id

Assigning order to an OrderAgreement object will replace the order_id foreign key:

order = Order.last # => #<Order id: 1, shop_id: 2>

order_agreement = OrderAgreement.new(order: order).save
order_agreement.order_id # => 1

Implementation details

Same as in #47923 we can't use public_send which results in the id method being called which for a composite primary key model returns a composite primary key values instead of the id column value. We are using _read_attribute instead

@nvasilevski
Copy link
Contributor Author

I'm going to have a look on these failures

Given a model associated with a composite primary key by `id` attribute,
for example:
```ruby
Order.primary_key = [:shop_id, :id]
OrderAgreement.primary_key = :id

OrderAgreement.belongs_to :order, primary_key: :id
```

Assigning `order` to an `OrderAgreement` object will replace the
`order_id` foreign key:

```ruby
order = Order.last # => #<Order id: 1, shop_id: 2>

order_agreement = OrderAgreement.new(order: order).save
order_agreement.order_id # => 1
```
@nvasilevski nvasilevski force-pushed the fix-autosave-for-models-assoc-with-a-cpk-model branch from 6a55048 to c0da60f Compare April 12, 2023 18:54
@eileencodes eileencodes merged commit 12fc98f into rails:main Apr 12, 2023
8 checks passed
@eileencodes eileencodes deleted the fix-autosave-for-models-assoc-with-a-cpk-model branch April 12, 2023 19:15
@@ -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"

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