Fix to_param id generation for partial composite keys #49080
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation / Background
This Pull Request has been created because parameter generation for CPK models is subtly broken.
Detail
This Pull Request changes Action View and Active Model by adding tests for url_for when using composite primary key models and fixes to_param output for CPK models with partially complete IDs (eg. one column has a value but the other doesn't).
Additional information
Somewhat similar to #49069, which made me think that
to_key
might actually be the source of both bugs. However, after reviewing the documentation, it seems like the current behaviour is correct:rails/activemodel/lib/active_model/conversion.rb
Lines 53 to 54 in 9d8ac62
Note any attribute being set, so partial CPK IDs are expected. I assume we don't want parameters with incomplete CPK IDs, so I fixed it in
to_param
instead.Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]