Merge pull request #48274 from Shopify/fix-serialized-blob-column-changed-in-place

Fix change_in_place? for binary serialized columns

Author: byroot

activemodel/lib/active_model/type/binary.rb

@@ -31,7 +31,9 @@ def changed_in_place?(raw_old_value, value)
 
       class Data # :nodoc:
         def initialize(value)
-          @value = value.to_s
+          value = value.to_s
+          value = value.b unless value.encoding == Encoding::BINARY
+          @value = value
         end
 
         def to_s

activemodel/test/cases/type/binary_test.rb

@@ -11,6 +11,12 @@ def test_type_cast_binary
         assert_equal "1", type.cast("1")
         assert_equal 1, type.cast(1)
       end
+
+      def test_serialize_binary_strings
+        type = Type::Binary.new
+        assert_equal "Ďe".b, type.serialize("Ďe")
+        assert_not_equal "Ďe", type.serialize("Ďe")
+      end
     end
   end
 end

activerecord/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Fix mutation detection for serialized attributes backed by binary columns.
+
+    *Jean Boussier*
+
 *   Fix a bug where using groups and counts with long table names would return incorrect results.
 
     *Shota Toguchi*, *Yusaku Ono*

activerecord/lib/active_record/type/serialized.rb

@@ -63,11 +63,11 @@ def default_value?(value)
         def encoded(value)
           return if default_value?(value)
           payload = coder.dump(value)
-          if payload && binary? && payload.encoding != Encoding::BINARY
-            payload = payload.dup if payload.frozen?
-            payload.force_encoding(Encoding::BINARY)
+          if payload && @subtype.binary?
+            ActiveModel::Type::Binary::Data.new(payload)
+          else
+            payload
           end
-          payload
         end
     end
   end

activerecord/test/cases/core_test.rb

@@ -18,15 +18,15 @@ def test_inspect_class
 
   def test_inspect_instance
     topic = topics(:first)
-    assert_equal %(#<Topic id: 1, title: "The First Topic", author_name: "David", author_email_address: "[email protected]", written_on: "#{topic.written_on.to_fs(:inspect)}", bonus_time: "#{topic.bonus_time.to_fs(:inspect)}", last_read: "#{topic.last_read.to_fs(:inspect)}", content: "Have a nice day", important: nil, approved: false, replies_count: 1, unique_replies_count: 0, parent_id: nil, parent_title: nil, type: nil, group: nil, created_at: "#{topic.created_at.to_fs(:inspect)}", updated_at: "#{topic.updated_at.to_fs(:inspect)}">), topic.inspect
+    assert_equal %(#<Topic id: 1, title: "The First Topic", author_name: "David", author_email_address: "[email protected]", written_on: "#{topic.written_on.to_fs(:inspect)}", bonus_time: "#{topic.bonus_time.to_fs(:inspect)}", last_read: "#{topic.last_read.to_fs(:inspect)}", content: "Have a nice day", important: nil, binary_content: nil, approved: false, replies_count: 1, unique_replies_count: 0, parent_id: nil, parent_title: nil, type: nil, group: nil, created_at: "#{topic.created_at.to_fs(:inspect)}", updated_at: "#{topic.updated_at.to_fs(:inspect)}">), topic.inspect
   end
 
   def test_inspect_instance_with_lambda_date_formatter
     before = Time::DATE_FORMATS[:inspect]
     Time::DATE_FORMATS[:inspect] = ->(date) { "my_format" }
     topic = topics(:first)
 
-    assert_equal %(#<Topic id: 1, title: "The First Topic", author_name: "David", author_email_address: "[email protected]", written_on: "my_format", bonus_time: "my_format", last_read: "2004-04-15", content: "Have a nice day", important: nil, approved: false, replies_count: 1, unique_replies_count: 0, parent_id: nil, parent_title: nil, type: nil, group: nil, created_at: "my_format", updated_at: "my_format">), topic.inspect
+    assert_equal %(#<Topic id: 1, title: "The First Topic", author_name: "David", author_email_address: "[email protected]", written_on: "my_format", bonus_time: "my_format", last_read: "2004-04-15", content: "Have a nice day", important: nil, binary_content: nil, approved: false, replies_count: 1, unique_replies_count: 0, parent_id: nil, parent_title: nil, type: nil, group: nil, created_at: "my_format", updated_at: "my_format">), topic.inspect
 
   ensure
     Time::DATE_FORMATS[:inspect] = before
@@ -65,6 +65,7 @@ def test_pretty_print_new
        last_read: nil,
        content: nil,
        important: nil,
+       binary_content: nil,
        approved: true,
        replies_count: 0,
        unique_replies_count: 0,
@@ -94,6 +95,7 @@ def test_pretty_print_persisted
        last_read: Thu, 15 Apr 2004,
        content: "Have a nice day",
        important: nil,
+       binary_content: nil,
        approved: false,
        replies_count: 1,
        unique_replies_count: 0,

activerecord/test/cases/reflection_test.rb

@@ -44,25 +44,25 @@ def test_human_name
 
   def test_read_attribute_names
     assert_equal(
-      %w( id title author_name author_email_address bonus_time written_on last_read content important group approved replies_count unique_replies_count parent_id parent_title type created_at updated_at ).sort,
+      %w( id title author_name author_email_address bonus_time written_on last_read content important binary_content group approved replies_count unique_replies_count parent_id parent_title type created_at updated_at ).sort,
       @first.attribute_names.sort
     )
   end
 
   def test_columns
-    assert_equal 18, Topic.columns.length
+    assert_equal 19, Topic.columns.length
   end
 
   def test_columns_are_returned_in_the_order_they_were_declared
     column_names = Topic.columns.map(&:name)
-    assert_equal %w(id title author_name author_email_address written_on bonus_time last_read content important approved replies_count unique_replies_count parent_id parent_title type group created_at updated_at), column_names
+    assert_equal %w(id title author_name author_email_address written_on bonus_time last_read content important binary_content approved replies_count unique_replies_count parent_id parent_title type group created_at updated_at), column_names
   end
 
   def test_content_columns
     content_columns        = Topic.content_columns
     content_column_names   = content_columns.map(&:name)
-    assert_equal 13, content_columns.length
-    assert_equal %w(title author_name author_email_address written_on bonus_time last_read content important group approved parent_title created_at updated_at).sort, content_column_names.sort
+    assert_equal 14, content_columns.length
+    assert_equal %w(title author_name author_email_address written_on bonus_time last_read content important binary_content group approved parent_title created_at updated_at).sort, content_column_names.sort
   end
 
   def test_column_string_type_and_limit

activerecord/test/cases/serialized_attribute_test.rb

@@ -4,7 +4,6 @@
 require "models/person"
 require "models/traffic_light"
 require "models/post"
-require "models/binary_field"
 
 class SerializedAttributeTest < ActiveRecord::TestCase
   def setup
@@ -358,36 +357,37 @@ def test_newly_emptied_serialized_hash_is_changed
     assert_equal({}, topic.content)
   end
 
-  if current_adapter?(:Mysql2Adapter)
-    def test_is_not_changed_when_stored_in_mysql_blob
-      value = %w(Fée)
-      model = BinaryField.create!(normal_blob: value, normal_text: value)
-      model.reload
+  def test_is_not_changed_when_stored_blob
+    Topic.serialize(:binary_content, type: Array)
+    Topic.serialize(:content, type: Array)
 
-      model.normal_text = value
-      assert_not_predicate model, :normal_text_changed?
+    value = %w(Fée)
+    model = Topic.create!(binary_content: value, content: value)
+    model.reload
 
-      model.normal_blob = value
-      assert_not_predicate model, :normal_blob_changed?
-    end
+    model.binary_content = value
+    assert_not_predicate model, :binary_content_changed?
 
-    class FrozenBinaryField < BinaryField
-      class FrozenCoder < ActiveRecord::Coders::YAMLColumn
-        def dump(obj)
-          super&.freeze
-        end
-      end
-      serialize :normal_blob, FrozenCoder.new(:normal_blob, Array)
+    model.content = value
+    assert_not_predicate model, :content_changed?
+  end
+
+  class FrozenCoder < ActiveRecord::Coders::YAMLColumn
+    def dump(obj)
+      super&.freeze
     end
+  end
 
-    def test_is_not_changed_when_stored_in_mysql_blob_frozen_payload
-      value = %w(Fée)
-      model = FrozenBinaryField.create!(normal_blob: value, normal_text: value)
-      model.reload
+  def test_is_not_changed_when_stored_in_blob_frozen_payload
+    Topic.serialize(:binary_content, coder: FrozenCoder.new(:binary_content, Array))
+    Topic.serialize(:content, coder: FrozenCoder.new(:content, Array))
 
-      model.normal_blob = value
-      assert_not_predicate model, :normal_blob_changed?
-    end
+    value = %w(Fée)
+    model = Topic.create!(binary_content: value, content: value)
+    model.reload
+
+    model.content = value
+    assert_not_predicate model, :content_changed?
   end
 
   def test_values_cast_from_nil_are_persisted_as_nil

activerecord/test/models/binary_field.rb

@@ -1103,6 +1103,7 @@
       t.text     :content
       t.text     :important
     end
+    t.blob     :binary_content
     t.boolean  :approved, default: true
     t.integer  :replies_count, default: 0
     t.integer  :unique_replies_count, default: 0