Merge pull request #667 from kaspth/revive-serialized-attributes

Use Active Record's type system from 4.2 onwards.

README.md

@@ -1181,22 +1181,6 @@ class ConvertVersionsObjectToJson < ActiveRecord::Migration
 end
 ```
 
-## SerializedAttributes support
-
-PaperTrail has a config option that can be used to enable/disable whether
-PaperTrail attempts to utilize `ActiveRecord`'s `serialized_attributes` feature.
-Note: This is enabled by default when PaperTrail is used with `ActiveRecord`
-version < `4.2`, and disabled by default when used with ActiveRecord `4.2.x`.
-Since `serialized_attributes` will be removed in `ActiveRecord` version `5.0`,
-this configuration value does nothing when PaperTrail is used with
-version `5.0` or greater.
-
-```ruby
-PaperTrail.config.serialized_attributes = true # enable
-PaperTrail.config.serialized_attributes = false # disable
-PaperTrail.serialized_attributes? # get current setting
-```
-
 ## Testing
 
 You may want to turn PaperTrail off to speed up your tests.  See the [Turning

lib/paper_trail/attributes_serialization.rb

@@ -0,0 +1,89 @@
+module PaperTrail
+  module AttributesSerialization
+    class NoOpAttribute
+      def type_cast_for_database(value)
+        value
+      end
+
+      def type_cast_from_database(data)
+        data
+      end
+    end
+    NO_OP_ATTRIBUTE = NoOpAttribute.new
+
+    class SerializedAttribute
+      def initialize(coder)
+        @coder = coder.respond_to?(:dump) ? coder : PaperTrail.serializer
+      end
+
+      def type_cast_for_database(value)
+        @coder.dump(value)
+      end
+
+      def type_cast_from_database(data)
+        @coder.load(data)
+      end
+    end
+
+    SERIALIZE, DESERIALIZE =
+      if ::ActiveRecord::VERSION::MAJOR >= 5
+        [:serialize, :deserialize]
+      else
+        [:type_cast_for_database, :type_cast_from_database]
+      end
+
+    if ::ActiveRecord::VERSION::STRING < '4.2'
+      # Backport Rails 4.2 and later's `type_for_attribute` to build
+      # on a common interface.
+      def type_for_attribute(attr_name)
+        serialized_attribute_types[attr_name.to_s] || NO_OP_ATTRIBUTE
+      end
+
+      def serialized_attribute_types
+        @attribute_types ||= Hash[serialized_attributes.map do |attr_name, coder|
+          [attr_name, SerializedAttribute.new(coder)]
+        end]
+      end
+      private :serialized_attribute_types
+    end
+
+    # Used for `Version#object` attribute.
+    def serialize_attributes_for_paper_trail!(attributes)
+      alter_attributes_for_paper_trail!(SERIALIZE, attributes)
+    end
+
+    def unserialize_attributes_for_paper_trail!(attributes)
+      alter_attributes_for_paper_trail!(DESERIALIZE, attributes)
+    end
+
+    def alter_attributes_for_paper_trail!(serializer, attributes)
+      # Don't serialize before values before inserting into columns of type
+      # `JSON` on `PostgreSQL` databases.
+      return attributes if paper_trail_version_class.object_col_is_json?
+
+      attributes.each do |key, value|
+        attributes[key] = type_for_attribute(key).send(serializer, value)
+      end
+    end
+
+    # Used for Version#object_changes attribute.
+    def serialize_attribute_changes_for_paper_trail!(changes)
+      alter_attribute_changes_for_paper_trail!(SERIALIZE, changes)
+    end
+
+    def unserialize_attribute_changes_for_paper_trail!(changes)
+      alter_attribute_changes_for_paper_trail!(DESERIALIZE, changes)
+    end
+
+    def alter_attribute_changes_for_paper_trail!(serializer, changes)
+      # Don't serialize before values before inserting into columns of type
+      # `JSON` on `PostgreSQL` databases.
+      return changes if paper_trail_version_class.object_changes_col_is_json?
+
+      changes.clone.each do |key, change|
+        type = type_for_attribute(key)
+        changes[key] = Array(change).map { |value| type.send(serializer, value) }
+      end
+    end
+  end
+end

lib/paper_trail/config.rb

@@ -5,30 +5,25 @@ module PaperTrail
   class Config
     include Singleton
     attr_accessor :timestamp_field, :serializer, :version_limit
-    attr_reader :serialized_attributes
     attr_writer :track_associations
 
     def initialize
       @timestamp_field = :created_at
       @serializer      = PaperTrail::Serializers::YAML
-
-      # This setting only defaults to false on AR 4.2+, because that's when
-      # it was deprecated. We want it to function with older versions of
-      # ActiveRecord by default.
-      if ::ActiveRecord::VERSION::STRING < '4.2'
-        @serialized_attributes = true
-      end
     end
 
-    def serialized_attributes=(value)
+    def serialized_attributes
-      if ::ActiveRecord::VERSION::MAJOR >= 5
+      ActiveSupport::Deprecation.warn(
-        ::ActiveSupport::Deprecation.warn(
+        "PaperTrail.config.serialized_attributes is deprecated without " \
-          "ActiveRecord 5.0 deprecated `serialized_attributes` "  +
+        "replacement and no longer has any effect."
-          "without replacement, so this PaperTrail config setting does " +
+      )
-          "nothing with this version, and is always turned off"
+    end
-        )
+
-      end
+    def serialized_attributes=(_)
-      @serialized_attributes = value
+      ActiveSupport::Deprecation.warn(
+        "PaperTrail.config.serialized_attributes= is deprecated without " \
+        "replacement and no longer has any effect."
+      )
     end
 
     def track_associations

lib/paper_trail/has_paper_trail.rb

@@ -1,4 +1,5 @@
 require 'active_support/core_ext/object' # provides the `try` method
+require 'paper_trail/attributes_serialization'
 
 module PaperTrail
   module Model
@@ -61,6 +62,7 @@ module PaperTrail
         # Lazily include the instance methods so we don't clutter up
         # any more ActiveRecord models than we have to.
         send :include, InstanceMethods
+        send :extend, AttributesSerialization
 
         class_attribute :version_association_name
         self.version_association_name = options[:version] || :version
@@ -161,74 +163,6 @@ module PaperTrail
       def paper_trail_version_class
         @paper_trail_version_class ||= version_class_name.constantize
       end
-
-      # Used for `Version#object` attribute.
-      def serialize_attributes_for_paper_trail!(attributes)
-        # Don't serialize before values before inserting into columns of type
-        # `JSON` on `PostgreSQL` databases.
-        return attributes if self.paper_trail_version_class.object_col_is_json?
-
-        serialized_attributes.each do |key, coder|
-          if attributes.key?(key)
-            # Fall back to current serializer if `coder` has no `dump` method.
-            coder = PaperTrail.serializer unless coder.respond_to?(:dump)
-            attributes[key] = coder.dump(attributes[key])
-          end
-        end
-      end
-
-      # TODO: There is a lot of duplication between this and
-      # `serialize_attributes_for_paper_trail!`.
-      def unserialize_attributes_for_paper_trail!(attributes)
-        # Don't serialize before values before inserting into columns of type
-        # `JSON` on `PostgreSQL` databases.
-        return attributes if self.paper_trail_version_class.object_col_is_json?
-
-        serialized_attributes.each do |key, coder|
-          if attributes.key?(key)
-            # Fall back to current serializer if `coder` has no `dump` method.
-            # TODO: Shouldn't this be `:load`?
-            coder = PaperTrail.serializer unless coder.respond_to?(:dump)
-            attributes[key] = coder.load(attributes[key])
-          end
-        end
-      end
-
-      # Used for Version#object_changes attribute.
-      def serialize_attribute_changes_for_paper_trail!(changes)
-        # Don't serialize before values before inserting into columns of type `JSON`
-# on `PostgreSQL` databases.
-        return changes if self.paper_trail_version_class.object_changes_col_is_json?
-
-        serialized_attributes.each do |key, coder|
-          if changes.key?(key)
-            # Fall back to current serializer if `coder` has no `dump` method.
-            coder = PaperTrail.serializer unless coder.respond_to?(:dump)
-            old_value, new_value = changes[key]
-            changes[key] = [coder.dump(old_value),
-                            coder.dump(new_value)]
-          end
-        end
-      end
-
-      # TODO: There is a lot of duplication between this and
-      # `serialize_attribute_changes_for_paper_trail!`.
-      def unserialize_attribute_changes_for_paper_trail!(changes)
-        # Don't serialize before values before inserting into columns of type
-        # `JSON` on `PostgreSQL` databases.
-        return changes if self.paper_trail_version_class.object_changes_col_is_json?
-
-        serialized_attributes.each do |key, coder|
-          if changes.key?(key)
-            # Fall back to current serializer if `coder` has no `dump` method.
-            # TODO: Shouldn't this be `:load`?
-            coder = PaperTrail.serializer unless coder.respond_to?(:dump)
-            old_value, new_value = changes[key]
-            changes[key] = [coder.load(old_value),
-                            coder.load(new_value)]
-          end
-        end
-      end
     end
 
     # Wrap the following methods in a module so we can include them only in the
@@ -440,9 +374,7 @@ module PaperTrail
 
       def changes_for_paper_trail
         _changes = changes.delete_if { |k,v| !notably_changed.include?(k) }
-        if PaperTrail.serialized_attributes?
+        self.class.serialize_attribute_changes_for_paper_trail!(_changes)
-          self.class.serialize_attribute_changes_for_paper_trail!(_changes)
-        end
         _changes.to_hash
       end
 
@@ -562,9 +494,7 @@ module PaperTrail
       # ommitting attributes to be skipped.
       def object_attrs_for_paper_trail(attributes_hash)
         attrs = attributes_hash.except(*self.paper_trail_options[:skip])
-        if PaperTrail.serialized_attributes?
+        self.class.serialize_attributes_for_paper_trail!(attrs)
-          self.class.serialize_attributes_for_paper_trail!(attrs)
-        end
         attrs
       end
 

lib/paper_trail/reifier.rb

@@ -59,9 +59,7 @@ module PaperTrail
           end
         end
 
-        if PaperTrail.serialized_attributes?
+        model.class.unserialize_attributes_for_paper_trail! attrs
-          model.class.unserialize_attributes_for_paper_trail! attrs
-        end
 
         # Set all the attributes in this version on the model.
         attrs.each do |k, v|

lib/paper_trail/version_concern.rb

@@ -260,9 +260,7 @@ module PaperTrail
     # @api private
     def load_changeset
       changes = HashWithIndifferentAccess.new(object_changes_deserialized)
-      if PaperTrail.serialized_attributes?
+      item_type.constantize.unserialize_attribute_changes_for_paper_trail!(changes)
-        item_type.constantize.unserialize_attribute_changes_for_paper_trail!(changes)
-      end
       changes
     rescue # TODO: Rescue something specific
       {}

test/test_helper.rb

@@ -43,6 +43,36 @@ class ActiveSupport::TestCase
     DatabaseCleaner.clean if using_mysql?
     Thread.current[:paper_trail] = nil
   end
+
+  private
+
+  def assert_attributes_equal(expected, actual)
+    if using_mysql?
+      expected, actual = expected.dup, actual.dup
+
+      # Adjust timestamps for missing fractional seconds precision.
+      %w(created_at updated_at).each do |timestamp|
+        expected[timestamp] = expected[timestamp].change(usec: 0)
+        actual[timestamp]   = actual[timestamp].change(usec: 0)
+      end
+    end
+
+    assert_equal expected, actual
+  end
+
+  def assert_changes_equal(expected, actual)
+    if using_mysql?
+      expected, actual = expected.dup, actual.dup
+
+      # Adjust timestamps for missing fractional seconds precision.
+      %w(created_at updated_at).each do |timestamp|
+        expected[timestamp][1] = expected[timestamp][1].change(usec: 0)
+        actual[timestamp][1]   = actual[timestamp][1].change(usec: 0)
+      end
+    end
+
+    assert_equal expected, actual
+  end
 end
 
 #

test/unit/model_test.rb

@@ -228,8 +228,8 @@ class HasPaperTrailModelTest < ActiveSupport::TestCase
           'id'         => [nil, @widget.id]
         }
 
-        assert_equal Time, @widget.versions.last.changeset['updated_at'][1].class
+        assert_kind_of Time, @widget.versions.last.changeset['updated_at'][1]
-        assert_equal changes, @widget.versions.last.changeset
+        assert_changes_equal changes, @widget.versions.last.changeset
       end
 
       context 'and then updated without any changes' do
@@ -375,7 +375,7 @@ class HasPaperTrailModelTest < ActiveSupport::TestCase
 
           should 'be available in its previous version' do
             assert_equal @widget.id, @reified_widget.id
-            assert_equal @widget.attributes, @reified_widget.attributes
+            assert_attributes_equal @widget.attributes, @reified_widget.attributes
           end
 
           should 'be re-creatable from its previous version' do
@@ -978,100 +978,94 @@ class HasPaperTrailModelTest < ActiveSupport::TestCase
     end
   end
 
-  # `serialized_attributes` is deprecated in ActiveRecord 5.0
+  context 'When an attribute has a custom serializer' do
-  if ::ActiveRecord::VERSION::MAJOR < 5
+    setup do
-    context 'When an attribute has a custom serializer' do
+      @person = Person.new(:time_zone => "Samoa")
+    end
+
+    should "be an instance of ActiveSupport::TimeZone" do
+      assert_equal ActiveSupport::TimeZone, @person.time_zone.class
+    end
+
+    context 'when the model is saved' do
       setup do
-        PaperTrail.config.serialized_attributes = true
+        @changes_before_save = @person.changes.dup
-        @person = Person.new(:time_zone => "Samoa")
+        @person.save!
       end
 
-      teardown { PaperTrail.config.serialized_attributes = false }
+      # Test for serialization:
-
+      should 'version.object_changes should not have stored the default, ridiculously long (to_yaml) serialization of the TimeZone object' do
-      should "be an instance of ActiveSupport::TimeZone" do
+        assert @person.versions.last.object_changes.length < 105, "object_changes length was #{@person.versions.last.object_changes.length}"
-        assert_equal ActiveSupport::TimeZone, @person.time_zone.class
+      end
+      # It should store the serialized value.
+      should 'version.object_changes attribute should have stored the value returned by the attribute serializer' do
+        as_stored_in_version = HashWithIndifferentAccess[YAML::load(@person.versions.last.object_changes)]
+        assert_equal [nil, 'Samoa'], as_stored_in_version[:time_zone]
+        serialized_value = Person::TimeZoneSerializer.dump(@person.time_zone)
+        assert_equal serialized_value, as_stored_in_version[:time_zone].last
       end
 
-      context 'when the model is saved' do
+      # Tests for unserialization:
+      should 'version.changeset should convert the attribute value back to its original, unserialized value' do
+        unserialized_value = Person::TimeZoneSerializer.load(@person.time_zone)
+        assert_equal unserialized_value, @person.versions.last.changeset[:time_zone].last
+      end
+      should "record.changes (before save) returns the original, unserialized values" do
+        assert_equal [NilClass, ActiveSupport::TimeZone], @changes_before_save[:time_zone].map(&:class)
+      end
+      should 'version.changeset should be the same as record.changes was before the save' do
+        assert_equal @changes_before_save, @person.versions.last.changeset.delete_if { |key, val| key.to_sym == :id }
+        assert_equal [NilClass, ActiveSupport::TimeZone], @person.versions.last.changeset[:time_zone].map(&:class)
+      end
+
+      context 'when that attribute is updated' do
         setup do
+          @attribute_value_before_change = @person.time_zone
+          @person.assign_attributes({ :time_zone => 'Pacific Time (US & Canada)' })
           @changes_before_save = @person.changes.dup
           @person.save!
         end
 
-        # Test for serialization:
+        # Tests for serialization:
-        should 'version.object_changes should not have stored the default, ridiculously long (to_yaml) serialization of the TimeZone object' do
+        # Before the serialized attributes fix, the object/object_changes value that was stored was ridiculously long (58723).
-          assert @person.versions.last.object_changes.length < 105, "object_changes length was #{@person.versions.last.object_changes.length}"
+        should 'version.object should not have stored the default, ridiculously long (to_yaml) serialization of the TimeZone object' do
+          assert @person.versions.last.object.length < 105, "object length was #{@person.versions.last.object.length}"
+        end
+        # Need an additional clause to detect what version of ActiveRecord is being used for this test because AR4 injects the `updated_at` column into the changeset for updates to models
+        should 'version.object_changes should not have stored the default, ridiculously long (to_yaml) serialization of the TimeZone object' do
+          assert @person.versions.last.object_changes.length < (ActiveRecord::VERSION::MAJOR < 4 ? 105 : 118), "object_changes length was #{@person.versions.last.object_changes.length}"
+        end
+        # But now it stores the short, serialized value.
+        should 'version.object attribute should have stored the value returned by the attribute serializer' do
+          as_stored_in_version = HashWithIndifferentAccess[YAML::load(@person.versions.last.object)]
+          assert_equal 'Samoa', as_stored_in_version[:time_zone]
+          serialized_value = Person::TimeZoneSerializer.dump(@attribute_value_before_change)
+          assert_equal serialized_value, as_stored_in_version[:time_zone]
         end
-        # It should store the serialized value.
         should 'version.object_changes attribute should have stored the value returned by the attribute serializer' do
           as_stored_in_version = HashWithIndifferentAccess[YAML::load(@person.versions.last.object_changes)]
-          assert_equal [nil, 'Samoa'], as_stored_in_version[:time_zone]
+          assert_equal ['Samoa', 'Pacific Time (US & Canada)'], as_stored_in_version[:time_zone]
           serialized_value = Person::TimeZoneSerializer.dump(@person.time_zone)
           assert_equal serialized_value, as_stored_in_version[:time_zone].last
         end
 
         # Tests for unserialization:
+        should 'version.reify should convert the attribute value back to its original, unserialized value' do
+          unserialized_value = Person::TimeZoneSerializer.load(@attribute_value_before_change)
+          assert_equal unserialized_value, @person.versions.last.reify.time_zone
+        end
         should 'version.changeset should convert the attribute value back to its original, unserialized value' do
           unserialized_value = Person::TimeZoneSerializer.load(@person.time_zone)
           assert_equal unserialized_value, @person.versions.last.changeset[:time_zone].last
         end
         should "record.changes (before save) returns the original, unserialized values" do
-          assert_equal [NilClass, ActiveSupport::TimeZone], @changes_before_save[:time_zone].map(&:class)
+          assert_equal [ActiveSupport::TimeZone, ActiveSupport::TimeZone], @changes_before_save[:time_zone].map(&:class)
         end
         should 'version.changeset should be the same as record.changes was before the save' do
-          assert_equal @changes_before_save, @person.versions.last.changeset.delete_if { |key, val| key.to_sym == :id }
+          assert_equal @changes_before_save, @person.versions.last.changeset
-          assert_equal [NilClass, ActiveSupport::TimeZone], @person.versions.last.changeset[:time_zone].map(&:class)
+          assert_equal [ActiveSupport::TimeZone, ActiveSupport::TimeZone], @person.versions.last.changeset[:time_zone].map(&:class)
         end
 
-        context 'when that attribute is updated' do
-          setup do
-            @attribute_value_before_change = @person.time_zone
-            @person.assign_attributes({ :time_zone => 'Pacific Time (US & Canada)' })
-            @changes_before_save = @person.changes.dup
-            @person.save!
-          end
-
-          # Tests for serialization:
-          # Before the serialized attributes fix, the object/object_changes value that was stored was ridiculously long (58723).
-          should 'version.object should not have stored the default, ridiculously long (to_yaml) serialization of the TimeZone object' do
-            assert @person.versions.last.object.length < 105, "object length was #{@person.versions.last.object.length}"
-          end
-          # Need an additional clause to detect what version of ActiveRecord is being used for this test because AR4 injects the `updated_at` column into the changeset for updates to models
-          should 'version.object_changes should not have stored the default, ridiculously long (to_yaml) serialization of the TimeZone object' do
-            assert @person.versions.last.object_changes.length < (ActiveRecord::VERSION::MAJOR < 4 ? 105 : 118), "object_changes length was #{@person.versions.last.object_changes.length}"
-          end
-          # But now it stores the short, serialized value.
-          should 'version.object attribute should have stored the value returned by the attribute serializer' do
-            as_stored_in_version = HashWithIndifferentAccess[YAML::load(@person.versions.last.object)]
-            assert_equal 'Samoa', as_stored_in_version[:time_zone]
-            serialized_value = Person::TimeZoneSerializer.dump(@attribute_value_before_change)
-            assert_equal serialized_value, as_stored_in_version[:time_zone]
-          end
-          should 'version.object_changes attribute should have stored the value returned by the attribute serializer' do
-            as_stored_in_version = HashWithIndifferentAccess[YAML::load(@person.versions.last.object_changes)]
-            assert_equal ['Samoa', 'Pacific Time (US & Canada)'], as_stored_in_version[:time_zone]
-            serialized_value = Person::TimeZoneSerializer.dump(@person.time_zone)
-            assert_equal serialized_value, as_stored_in_version[:time_zone].last
-          end
-
-          # Tests for unserialization:
-          should 'version.reify should convert the attribute value back to its original, unserialized value' do
-            unserialized_value = Person::TimeZoneSerializer.load(@attribute_value_before_change)
-            assert_equal unserialized_value, @person.versions.last.reify.time_zone
-          end
-          should 'version.changeset should convert the attribute value back to its original, unserialized value' do
-            unserialized_value = Person::TimeZoneSerializer.load(@person.time_zone)
-            assert_equal unserialized_value, @person.versions.last.changeset[:time_zone].last
-          end
-          should "record.changes (before save) returns the original, unserialized values" do
-            assert_equal [ActiveSupport::TimeZone, ActiveSupport::TimeZone], @changes_before_save[:time_zone].map(&:class)
-          end
-          should 'version.changeset should be the same as record.changes was before the save' do
-            assert_equal @changes_before_save, @person.versions.last.changeset
-            assert_equal [ActiveSupport::TimeZone, ActiveSupport::TimeZone], @person.versions.last.changeset[:time_zone].map(&:class)
-          end
-
-        end
       end
     end
   end

test/unit/protected_attrs_test.rb

@@ -38,7 +38,7 @@ class ProtectedAttrsTest < ActiveSupport::TestCase
         # For some reason this test seems to be broken in JRuby 1.9 mode in the
         # test env even though it works in the console. WTF?
         unless ActiveRecord::VERSION::MAJOR >= 4 && defined?(JRUBY_VERSION)
-          assert_equal @widget.previous_version.attributes, @initial_attributes
+          assert_attributes_equal @widget.previous_version.attributes, @initial_attributes
         end
       end
     end