Skip to content
  • eileencodes's avatar
    6833bf4d
    Remove implementation of unchecked_serialize · 6833bf4d
    eileencodes authored
    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.
    6833bf4d
    Remove implementation of unchecked_serialize
    eileencodes authored
    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.
Loading