-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Should not change serializable value for the enum attribute #41486
Should not change serializable value for the enum attribute #41486
Conversation
`EnumType#serializable?` which allows values only in the enum mapping was added in 72fd0ba, and it is called only in `HomogeneousIn`, it caused the serialization inconsistency whether the values are in `IN` clause or not. ```ruby # SELECT "users".* FROM "users" WHERE "users"."role" = '0' User.where(role: "0") # SELECT "users".* FROM "users" WHERE "users"."role" = '1' User.where(role: "1") # SELECT "users".* FROM "users" WHERE "users"."role" IN (NULL) User.where(role: ["0", "1"]) ``` Even if we introduce validation for the enum serialization in the future, we should at least not cause the inconsistency for now. Fixes rails#41474.
def serialize(value) | ||
mapping.fetch(value, value) | ||
end | ||
|
||
def assert_valid_value(value) | ||
unless serializable?(value) | ||
unless value.blank? || mapping.has_key?(value) || mapping.has_value?(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you instead remove the super
from serializable?
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eileencodes I've tried to restore serializable?, but test has failed.
Looks like removing super
from serializable?
alone can't fix the problem, but removing serializable?
can.
test failed
Using sqlite3
Run options: --seed 20368
# Running:
.........F
Failure:
EnumTest#test_find_via_where_with_values.to_s [/vagrant/rails/activerecord/test/cases/enum_test.rb:69]:
--- expected
+++ actual
@@ -1 +1 @@
-#<Book id: 1, author_id: 1, format: "paperback", name: "Agile Web Development with Rails", status: "published", last_read: "read", nullable_status: nil, language: "english", author_visibility: "visible", illustrator_visibility: "visible", font_size: "medium", difficulty: "medium", cover: "hard", isbn: nil, external_id: nil, published_on: nil, boolean_status: "enabled", tags_count: 0, created_at: "2021-02-19 16:30:24.429872000 +0000", updated_at: "2021-02-19 16:30:24.429872000 +0000", updated_on: "2021-02-19">
+nil
bin/test test/cases/enum_test.rb:64
...................................................................
Finished in 2.017204s, 38.1716 runs/s, 126.9083 assertions/s.
77 runs, 256 assertions, 1 failures, 0 errors, 0 skips
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively we could cast stringified digits into numeric type, for example:
What do you think? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yevgenko It will break the existing behavior effed76.
Can you instead remove the
super
fromserializable?
?
I'm not sure I get your point, so let me explain why the value.blank? || mapping.has_key?(value) || mapping.has_value?(value)
part should not be left in the serializable?
.
EnumType#serialize
is mapping.fetch(value, value)
, it means that all values were serializable before 72fd0ba.
rails/activerecord/lib/active_record/enum.rb
Lines 146 to 148 in effed76
def serialize(value) | |
mapping.fetch(value, value) | |
end |
After 72fd0ba, serializable?
is introduced which checks a value is whether cast
-able or not, it is used only in HomogeneousIn
.
rails/activerecord/lib/arel/nodes/homogeneous_in.rb
Lines 50 to 52 in effed76
casted_values = values.map do |raw_value| | |
type.serialize(raw_value) if type.serializable?(raw_value) | |
end |
So now, all values are no longer always serializable in HomogeneousIn
, it is the cause for the issue #41474.
I'm not sure about we'd like the enum serialization more stricter in the future, at least for now the serialization inconsistency whether in HomogeneousIn
or others should be fixed.
…value Should not change serializable value for the enum attribute
EnumType#serializable?
which allows values only in the enum mappingwas added in 72fd0ba, and it is called only in
HomogeneousIn
, itcaused the serialization inconsistency whether the values are in
IN
clause or not.
Even if we introduce validation for the enum serialization in the future,
we should at least not cause the inconsistency for now.
Fixes #41474.
cc @eileencodes