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

Add better TypeError when assigning CPK #48436

Merged
merged 1 commit into from
Jun 12, 2023
+4 −1
Merged

Add better TypeError when assigning CPK #48436

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
1 change: 1 addition & 0 deletions activerecord/lib/active_record/attribute_methods/primary_key.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ def primary_key_values_present? # :nodoc:
# Sets the primary key column's value.
def id=(value)
if self.class.composite_primary_key?
raise TypeError, "Expected value matching #{self.class.primary_key.inspect}, got #{value.inspect}." unless value.is_a?(Enumerable)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if we should be even more restrictive and check for the enumerable's size so we also raise if the value is smaller or larger than the primary key.

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered this as well, but decided not to because I'm not sure of the performance implications. I'll take a look at benchmarking it to see if checking the length of the PK impacts much.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm totally ok if we ship it as is since it's very valuable in this form and it's less likely that someone will try to assign an array of different size as a CPK model id
I expect main scenarios to be about passing a single value when we expected to pass an array which is covered by this validation

@primary_key.zip(value) { |attr, value| _write_attribute(attr, value) }
else
_write_attribute(@primary_key, value)
Expand Down
4 changes: 3 additions & 1 deletion activerecord/test/cases/primary_keys_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,9 +426,11 @@ def test_assigning_a_composite_primary_key
def test_assigning_a_non_array_value_to_model_with_composite_primary_key_raises
book = Cpk::Book.new

assert_raises(TypeError) do
error = assert_raises(TypeError) do
book.id = 1
end

assert_equal("Expected value matching [\"author_id\", \"number\"], got 1.", error.message)
end

def test_id_was_composite
Expand Down