Detect in-place changes on mutable AR attributes

We have several mutable types on Active Record now. (Serialized, JSON,
HStore). We need to be able to detect if these have been modified in
place.

12 files changed, 149 insertions, 36 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index efa4abfa4d..2915e9b42d 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   `ActiveRecord::Dirty` now detects in-place changes to mutable values.
+    Serialized attributes on ActiveRecord models will no longer save when
+    unchanged. Fixes #8328.
+
+    Sean Griffin
+
 *   Fixed automatic maintaining test schema to properly handle sql structure
     schema format.
 
diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb
index be438aba95..6a5c057384 100644
--- a/activerecord/lib/active_record/attribute_methods/dirty.rb
+++ b/activerecord/lib/active_record/attribute_methods/dirty.rb
@@ -38,12 +38,39 @@ module ActiveRecord
         end
       end
 
-    def initialize_dup(other) # :nodoc:
-      super
-      init_changed_attributes
-    end
+      def initialize_dup(other) # :nodoc:
+        super
+        init_changed_attributes
+      end
+
+      def changed?
+        super || changed_in_place.any?
+      end
+
+      def changed
+        super | changed_in_place
+      end
+
+      def attribute_changed?(attr_name, options = {})
+        result = super
+        # We can't change "from" something in place. Only setters can define
+        # "from" and "to"
+        result ||= changed_in_place?(attr_name) unless options.key?(:from)
+        result
+      end
+
+      def changes_applied
+        super
+        store_original_raw_attributes
+      end
+
+      def reset_changes
+        super
+        original_raw_attributes.clear
+      end
+
+      private
 
-    private
       def initialize_internals_callback
         super
         init_changed_attributes
@@ -65,11 +92,20 @@ module ActiveRecord
 
         old_value = old_attribute_value(attr)
 
-        result = super(attr, value)
+        result = super
+        store_original_raw_attribute(attr)
         save_changed_attribute(attr, old_value)
         result
       end
 
+      def raw_write_attribute(attr, value)
+        attr = attr.to_s
+
+        result = super
+        original_raw_attributes[attr] = value
+        result
+      end
+
       def save_changed_attribute(attr, old_value)
         if attribute_changed?(attr)
           changed_attributes.delete(attr) unless _field_changed?(attr, old_value)
@@ -105,6 +141,41 @@ module ActiveRecord
         raw_value = read_attribute_before_type_cast(attr)
         column_for_attribute(attr).changed?(old_value, new_value, raw_value)
       end
+
+      def changed_in_place
+        self.class.attribute_names.select do |attr_name|
+          changed_in_place?(attr_name)
+        end
+      end
+
+      def changed_in_place?(attr_name)
+        type = type_for_attribute(attr_name)
+        old_value = original_raw_attribute(attr_name)
+        value = read_attribute(attr_name)
+        type.changed_in_place?(old_value, value)
+      end
+
+      def original_raw_attribute(attr_name)
+        original_raw_attributes.fetch(attr_name) do
+          read_attribute_before_type_cast(attr_name)
+        end
+      end
+
+      def original_raw_attributes
+        @original_raw_attributes ||= {}
+      end
+
+      def store_original_raw_attribute(attr_name)
+        type = type_for_attribute(attr_name)
+        value = type.type_cast_for_database(read_attribute(attr_name))
+        original_raw_attributes[attr_name] = value
+      end
+
+      def store_original_raw_attributes
+        attribute_names.each do |attr|
+          store_original_raw_attribute(attr)
+        end
+      end
     end
   end
 end
diff --git a/activerecord/lib/active_record/attribute_methods/serialization.rb b/activerecord/lib/active_record/attribute_methods/serialization.rb
index 60debb7d18..cec50f62a3 100644
--- a/activerecord/lib/active_record/attribute_methods/serialization.rb
+++ b/activerecord/lib/active_record/attribute_methods/serialization.rb
@@ -50,8 +50,6 @@ module ActiveRecord
         #     serialize :preferences, Hash
         #   end
         def serialize(attr_name, class_name_or_coder = Object)
-          include Behavior
-
           coder = if [:load, :dump].all? { |x| class_name_or_coder.respond_to?(x) }
                     class_name_or_coder
                   else
@@ -67,21 +65,6 @@ module ActiveRecord
           self.serialized_attributes = serialized_attributes.merge(attr_name.to_s => coder)
         end
       end
-
-
-      # This is only added to the model when serialize is called, which
-      # ensures we do not make things slower when serialization is not used.
-      module Behavior # :nodoc:
-        extend ActiveSupport::Concern
-
-        def should_record_timestamps?
-          super || (self.record_timestamps && (attributes.keys & self.class.serialized_attributes.keys).present?)
-        end
-
-        def keys_for_partial_write
-          super | (attributes.keys & self.class.serialized_attributes.keys)
-        end
-      end
     end
   end
 end
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/oid/hstore.rb b/activerecord/lib/active_record/connection_adapters/postgresql/oid/hstore.rb
index 88de816d4f..0dd4b65333 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/oid/hstore.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/oid/hstore.rb
@@ -3,14 +3,12 @@ module ActiveRecord
     module PostgreSQL
       module OID # :nodoc:
         class Hstore < Type::Value
+          include Type::Mutable
+
           def type
             :hstore
           end
 
-          def type_cast_from_user(value)
-            type_cast_from_database(type_cast_for_database(value))
-          end
-
           def type_cast_from_database(value)
             ConnectionAdapters::PostgreSQLColumn.string_to_hstore(value)
           end
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/oid/json.rb b/activerecord/lib/active_record/connection_adapters/postgresql/oid/json.rb
index b4fed1bcab..d1347c7bb5 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/oid/json.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/oid/json.rb
@@ -3,14 +3,12 @@ module ActiveRecord
     module PostgreSQL
       module OID # :nodoc:
         class Json < Type::Value
+          include Type::Mutable
+
           def type
             :json
           end
 
-          def type_cast_from_user(value)
-            type_cast_from_database(type_cast_for_database(value))
-          end
-
           def type_cast_from_database(value)
             ConnectionAdapters::PostgreSQLColumn.string_to_json(value)
           end
diff --git a/activerecord/lib/active_record/type.rb b/activerecord/lib/active_record/type.rb
index e9b827886a..f1384e0bb2 100644
--- a/activerecord/lib/active_record/type.rb
+++ b/activerecord/lib/active_record/type.rb
@@ -1,3 +1,4 @@
+require 'active_record/type/mutable'
 require 'active_record/type/numeric'
 require 'active_record/type/time_value'
 require 'active_record/type/value'
diff --git a/activerecord/lib/active_record/type/mutable.rb b/activerecord/lib/active_record/type/mutable.rb
new file mode 100644
index 0000000000..64cf4b9b93
--- /dev/null
+++ b/activerecord/lib/active_record/type/mutable.rb
@@ -0,0 +1,16 @@
+module ActiveRecord
+  module Type
+    module Mutable
+      def type_cast_from_user(value)
+        type_cast_from_database(type_cast_for_database(value))
+      end
+
+      # +raw_old_value+ will be the `_before_type_cast` version of the
+      # value (likely a string). +new_value+ will be the current, type
+      # cast value.
+      def changed_in_place?(raw_old_value, new_value) # :nodoc:
+        raw_old_value != type_cast_for_database(new_value)
+      end
+    end
+  end
+end
diff --git a/activerecord/lib/active_record/type/serialized.rb b/activerecord/lib/active_record/type/serialized.rb
index e6d84eadc0..ebde14634c 100644
--- a/activerecord/lib/active_record/type/serialized.rb
+++ b/activerecord/lib/active_record/type/serialized.rb
@@ -1,6 +1,8 @@
 module ActiveRecord
   module Type
     class Serialized < SimpleDelegator # :nodoc:
+      include Mutable
+
       attr_reader :subtype, :coder
 
       def initialize(subtype, coder)
@@ -17,10 +19,6 @@ module ActiveRecord
         end
       end
 
-      def type_cast_from_user(value)
-        type_cast_from_database(type_cast_for_database(value))
-      end
-
       def type_cast_for_database(value)
         return if value.nil?
         unless is_default_value?(value)
diff --git a/activerecord/lib/active_record/type/value.rb b/activerecord/lib/active_record/type/value.rb
index efcdab1c0e..b34d1697cd 100644
--- a/activerecord/lib/active_record/type/value.rb
+++ b/activerecord/lib/active_record/type/value.rb
@@ -60,6 +60,10 @@ module ActiveRecord
         old_value != new_value
       end
 
+      def changed_in_place?(*) # :nodoc:
+        false
+      end
+
       private
       # Takes an input from the database, or from attribute setters,
       # and casts it to a type appropriate for this object. This method
diff --git a/activerecord/test/cases/adapters/postgresql/hstore_test.rb b/activerecord/test/cases/adapters/postgresql/hstore_test.rb
index a25c9cb5e4..83b495d600 100644
--- a/activerecord/test/cases/adapters/postgresql/hstore_test.rb
+++ b/activerecord/test/cases/adapters/postgresql/hstore_test.rb
@@ -163,6 +163,15 @@ class PostgresqlHstoreTest < ActiveRecord::TestCase
       assert_equal "GMT", y.timezone
     end
 
+    def test_changes_in_place
+      hstore = Hstore.create!(settings: { 'one' => 'two' })
+      hstore.settings['three'] = 'four'
+      hstore.save!
+      hstore.reload
+
+      assert_equal 'four', hstore.settings['three']
+    end
+
     def test_gen1
       assert_equal(%q(" "=>""), @column.class.hstore_to_string({' '=>''}))
     end
diff --git a/activerecord/test/cases/adapters/postgresql/json_test.rb b/activerecord/test/cases/adapters/postgresql/json_test.rb
index 3ee8839823..a3400a5a19 100644
--- a/activerecord/test/cases/adapters/postgresql/json_test.rb
+++ b/activerecord/test/cases/adapters/postgresql/json_test.rb
@@ -165,4 +165,24 @@ class PostgresqlJSONTest < ActiveRecord::TestCase
     JsonDataType.update_all payload: { }
     assert_equal({ }, json.reload.payload)
   end
+
+  def test_changes_in_place
+    json = JsonDataType.new
+    assert_not json.changed?
+
+    json.payload = { 'one' => 'two' }
+    assert json.changed?
+    assert json.payload_changed?
+
+    json.save!
+    assert_not json.changed?
+
+    json.payload['three'] = 'four'
+    assert json.payload_changed?
+
+    json.save!
+    json.reload
+
+    assert json.payload['three'] = 'four'
+  end
 end
diff --git a/activerecord/test/cases/dirty_test.rb b/activerecord/test/cases/dirty_test.rb
index 87f24e32b2..5d6601a881 100644
--- a/activerecord/test/cases/dirty_test.rb
+++ b/activerecord/test/cases/dirty_test.rb
@@ -445,11 +445,20 @@ class DirtyTest < ActiveRecord::TestCase
   def test_save_should_store_serialized_attributes_even_with_partial_writes
     with_partial_writes(Topic) do
       topic = Topic.create!(:content => {:a => "a"})
+
+      assert_not topic.changed?
+
       topic.content[:b] = "b"
-      #assert topic.changed? # Known bug, will fail
+
+      assert topic.changed?
+
       topic.save!
+
+      assert_not topic.changed?
       assert_equal "b", topic.content[:b]
+
       topic.reload
+
       assert_equal "b", topic.content[:b]
     end
   end