1
0
Fork 0
mirror of https://github.com/rails/rails.git synced 2022-11-09 12:12:34 -05:00

Remove implementation of unchecked_serialize

Since we're checking `serializable?` in the new `HomogeneousIn`
`serialize` will no longer raise an exception. We implemented
`unchecked_serialize` to avoid raising in these cases, but with some of
our refactoring we no longer need it.

I discovered this while trying to fix a query in our application that
was not properly serializing binary columns. I discovered that in at
least 2 of our active model types we were not calling the correct
serialization. Since `serialize` wasn't aliased to `unchecked_serialize`
in `ActiveModel::Type::Binary` and `ActiveModel::Type::Boolean` (I
didn't check others but pretty sure all the AM Types are broken) the SQL
was being treated as a `String` and not the correct type.

This caused Rails to incorrectly query by string values. This is
problematic for columns storing binary data like our emoji columns at
GitHub. The test added here is an example of how the Binary type was
broken previously. The SQL should be using the hex values, not the
string value of "🥦" or other emoji.

We still have the problem `unchecked_serialize` was supposed to fix -
that `serialize` shouldn't validate data, just convert it. We'll be
fixing that in a followup PR so for now we should use `serialize` so we
know all the values are going through the right serialization for their
SQL.
This commit is contained in:
eileencodes 2020-05-12 10:57:46 -04:00
parent af831a0963
commit 6833bf4d10
No known key found for this signature in database
GPG key ID: BA5C575120BBE8DF
6 changed files with 9 additions and 6 deletions

View file

@ -7,7 +7,6 @@ module ActiveModel
def serialize(value)
cast(value)
end
alias :unchecked_serialize :serialize
def cast(value)
# Checks whether the value is numeric. Spaceship operator

View file

@ -53,7 +53,6 @@ module ActiveModel
def serialize(value)
value
end
alias :unchecked_serialize :serialize
# Type casts a value for schema dumping. This method is private, as we are
# hoping to remove it entirely.

View file

@ -146,7 +146,6 @@ module ActiveRecord
def serialize(value)
mapping.fetch(value, value)
end
alias :unchecked_serialize :serialize
def assert_valid_value(value)
unless serializable?(value)

View file

@ -44,7 +44,7 @@ module Arel # :nodoc: all
type = attribute.type_caster
casted_values = values.map do |raw_value|
type.unchecked_serialize(raw_value) if type.serializable?(raw_value)
type.serialize(raw_value) if type.serializable?(raw_value)
end
casted_values.compact!

View file

@ -327,9 +327,9 @@ module Arel # :nodoc: all
collector << quote_table_name(o.table_name) << "." << quote_column_name(o.column_name)
if o.type == :in
collector << "IN ("
collector << " IN ("
else
collector << "NOT IN ("
collector << " NOT IN ("
end
values = o.casted_values.map { |v| @connection.quote(v) }

View file

@ -366,6 +366,12 @@ module ActiveRecord
assert_equal 0, count
end
def test_where_with_emoji_for_binary_column
Binary.create!(data: "🥦")
assert Binary.where(data: ["🥦", "🍦"]).to_sql.include?("f09fa5a6")
assert Binary.where(data: ["🥦", "🍦"]).to_sql.include?("f09f8da6")
end
def test_where_on_association_with_custom_primary_key
author = authors(:david)
essay = Essay.where(writer: author).first