From 5be48e491edd702950f2e85e46b4d9125c0070b2 Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Wed, 9 Dec 2015 16:58:27 +0100 Subject: [PATCH 1/2] Use Active Record's type system from 4.2 onwards. Rails 4.2 deprecates `serialized_attributes` without replacement. However, it also introduces a type system which lets us treat all attributes the same. Rails 4.2 has `type_for_attribute` which knows how to serialize and deserialize itself from a database through `type_cast_for_database` and `type_cast_from_database`. (In Rails 5 they will be `serialize` and `deserialize` respectively.) Thus we no longer need the `PaperTrail.config.serialized_attributes` toggle, and this change makes it do nothing. It's still kept around for backwardscompatibility. --- README.md | 16 --- lib/paper_trail/attributes_serialization.rb | 89 ++++++++++++++ lib/paper_trail/config.rb | 29 ++--- lib/paper_trail/has_paper_trail.rb | 78 +----------- lib/paper_trail/reifier.rb | 4 +- lib/paper_trail/version_concern.rb | 4 +- test/unit/model_test.rb | 130 ++++++++++---------- 7 files changed, 169 insertions(+), 181 deletions(-) create mode 100644 lib/paper_trail/attributes_serialization.rb diff --git a/README.md b/README.md index b143bfaf..46119627 100644 --- a/README.md +++ b/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 diff --git a/lib/paper_trail/attributes_serialization.rb b/lib/paper_trail/attributes_serialization.rb new file mode 100644 index 00000000..3022ce67 --- /dev/null +++ b/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 diff --git a/lib/paper_trail/config.rb b/lib/paper_trail/config.rb index 2729b2fe..af4901f2 100644 --- a/lib/paper_trail/config.rb +++ b/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) - if ::ActiveRecord::VERSION::MAJOR >= 5 - ::ActiveSupport::Deprecation.warn( - "ActiveRecord 5.0 deprecated `serialized_attributes` " + - "without replacement, so this PaperTrail config setting does " + - "nothing with this version, and is always turned off" - ) - end - @serialized_attributes = value + def serialized_attributes + ActiveSupport::Deprecation.warn( + "PaperTrail.config.serialized_attributes is deprecated without " \ + "replacement and no longer has any effect." + ) + end + + def serialized_attributes=(_) + ActiveSupport::Deprecation.warn( + "PaperTrail.config.serialized_attributes= is deprecated without " \ + "replacement and no longer has any effect." + ) end def track_associations diff --git a/lib/paper_trail/has_paper_trail.rb b/lib/paper_trail/has_paper_trail.rb index 10476207..4636e9fb 100644 --- a/lib/paper_trail/has_paper_trail.rb +++ b/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) - end + self.class.serialize_attribute_changes_for_paper_trail!(_changes) _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) - end + self.class.serialize_attributes_for_paper_trail!(attrs) attrs end diff --git a/lib/paper_trail/reifier.rb b/lib/paper_trail/reifier.rb index 4f9d59cc..a8c7c037 100644 --- a/lib/paper_trail/reifier.rb +++ b/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 - end + model.class.unserialize_attributes_for_paper_trail! attrs # Set all the attributes in this version on the model. attrs.each do |k, v| diff --git a/lib/paper_trail/version_concern.rb b/lib/paper_trail/version_concern.rb index 104e44a6..ef10c390 100644 --- a/lib/paper_trail/version_concern.rb +++ b/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) - end + item_type.constantize.unserialize_attribute_changes_for_paper_trail!(changes) changes rescue # TODO: Rescue something specific {} diff --git a/test/unit/model_test.rb b/test/unit/model_test.rb index 5a03f2e5..70d47f93 100644 --- a/test/unit/model_test.rb +++ b/test/unit/model_test.rb @@ -228,7 +228,7 @@ 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 end @@ -978,100 +978,94 @@ class HasPaperTrailModelTest < ActiveSupport::TestCase end end - # `serialized_attributes` is deprecated in ActiveRecord 5.0 - if ::ActiveRecord::VERSION::MAJOR < 5 - context 'When an attribute has a custom serializer' do + context 'When an attribute has a custom serializer' do + setup 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 - @person = Person.new(:time_zone => "Samoa") + @changes_before_save = @person.changes.dup + @person.save! end - teardown { PaperTrail.config.serialized_attributes = false } - - should "be an instance of ActiveSupport::TimeZone" do - assert_equal ActiveSupport::TimeZone, @person.time_zone.class + # Test for serialization: + 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 < 105, "object_changes length was #{@person.versions.last.object_changes.length}" + 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: - 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 < 105, "object_changes length was #{@person.versions.last.object_changes.length}" + # 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 - # 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 [NilClass, ActiveSupport::TimeZone], @person.versions.last.changeset[:time_zone].map(&:class) + assert_equal @changes_before_save, @person.versions.last.changeset + 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 From 01837bf374ecef37d71e852507517bd97d57584a Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Mon, 21 Dec 2015 14:00:57 +0100 Subject: [PATCH 2/2] Add assertions to adjust timestamps on MySQL. Tests were failing on MySQL due to timestamps with differing usec flunking `assert_equal`. Happened because Rails 4.2 added support for fractional seconds precision. Travis, however, doesn't support the required MySQL 5.6 for it. Bend the timestamps to strip out fractional seconds when tests are run with MySQL. --- test/test_helper.rb | 30 ++++++++++++++++++++++++++++++ test/unit/model_test.rb | 4 ++-- test/unit/protected_attrs_test.rb | 2 +- 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/test/test_helper.rb b/test/test_helper.rb index 42590ed6..193ccf71 100644 --- a/test/test_helper.rb +++ b/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 # diff --git a/test/unit/model_test.rb b/test/unit/model_test.rb index 70d47f93..c6e2af02 100644 --- a/test/unit/model_test.rb +++ b/test/unit/model_test.rb @@ -229,7 +229,7 @@ class HasPaperTrailModelTest < ActiveSupport::TestCase } 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 diff --git a/test/unit/protected_attrs_test.rb b/test/unit/protected_attrs_test.rb index 36a0ffdc..68f3be59 100644 --- a/test/unit/protected_attrs_test.rb +++ b/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