Merge pull request #15674 from sgrif/sg-mutable-attributes

Detect in-place changes on mutable AR attributes

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