From 8b8579a81a626f9a2cf83d4179994b7710161cbd Mon Sep 17 00:00:00 2001 From: Tyler Rick Date: Mon, 5 Nov 2012 11:09:06 -0800 Subject: [PATCH 1/5] When building the :object_changes metadata, use the serialized value for any attributes that are serialized --- lib/paper_trail/has_paper_trail.rb | 22 ++++++++++++++---- test/dummy/app/models/person.rb | 23 +++++++++++++++++++ .../20110208155312_set_up_test_tables.rb | 1 + test/unit/model_test.rb | 23 +++++++++++++++++++ 4 files changed, 65 insertions(+), 4 deletions(-) diff --git a/lib/paper_trail/has_paper_trail.rb b/lib/paper_trail/has_paper_trail.rb index 95fb28f4c..407289ec5 100644 --- a/lib/paper_trail/has_paper_trail.rb +++ b/lib/paper_trail/has_paper_trail.rb @@ -175,15 +175,29 @@ def record_update :whodunnit => PaperTrail.whodunnit } if version_class.column_names.include? 'object_changes' - # The double negative (reject, !include?) preserves the hash structure of self.changes. - data[:object_changes] = self.changes.reject do |key, value| - !notably_changed.include?(key) - end.to_yaml + data[:object_changes] = changes_for_paper_trail.to_yaml end send(self.class.versions_association_name).build merge_metadata(data) end end + def changes_for_paper_trail + # The double negative (reject, !include?) preserves the hash structure of self.changes. + self.changes.reject do |key, value| + !notably_changed.include?(key) + end.tap do |changes_hash| + # Use serialized value for attributes that are serialized + changes_hash.each do |key, (old_value, new_value)| + if serialized_attributes.include?(key) + # serializer.dump(new_value) is the same as @attributes[key].serialized_value + serializer = @attributes[key].coder + changes_hash[key] = [serializer.dump(old_value), + serializer.dump(new_value)] + end + end + end + end + def record_destroy if switched_on? and not new_record? version_class.create merge_metadata(:item_id => self.id, diff --git a/test/dummy/app/models/person.rb b/test/dummy/app/models/person.rb index ab77d1c44..7a173093e 100644 --- a/test/dummy/app/models/person.rb +++ b/test/dummy/app/models/person.rb @@ -2,4 +2,27 @@ class Person < ActiveRecord::Base has_many :authorships, :dependent => :destroy has_many :books, :through => :authorships has_paper_trail + + # Convert strings to TimeZone objects when assigned + def time_zone=(value) + if value.is_a? ActiveSupport::TimeZone + super + else + zone = ::Time.find_zone(value) # nil if can't find time zone + super zone + end + end + + # Store TimeZone objects as strings when serialized to database + class TimeZoneSerializer + def dump(zone) + zone.try(:name) + end + + def load(value) + ::Time.find_zone!(value) rescue nil + end + end + + serialize :time_zone, TimeZoneSerializer.new end diff --git a/test/dummy/db/migrate/20110208155312_set_up_test_tables.rb b/test/dummy/db/migrate/20110208155312_set_up_test_tables.rb index 0381e68b8..9eade1de1 100644 --- a/test/dummy/db/migrate/20110208155312_set_up_test_tables.rb +++ b/test/dummy/db/migrate/20110208155312_set_up_test_tables.rb @@ -80,6 +80,7 @@ def self.up create_table :people, :force => true do |t| t.string :name + t.string "time_zone" end create_table :songs, :force => true do |t| diff --git a/test/unit/model_test.rb b/test/unit/model_test.rb index 485005eb9..f649aa584 100644 --- a/test/unit/model_test.rb +++ b/test/unit/model_test.rb @@ -906,6 +906,29 @@ def without(&block) end end + context 'When an attribute has a custom serializer' do + setup { @person = Person.create(:time_zone => "UTC") } + + should "be an instance of ActiveSupport::TimeZone" do + assert_equal ActiveSupport::TimeZone, @person.time_zone.class + end + + context 'when that attribute is updated' do + setup { @person.update_attributes({ :time_zone => 'Pacific Time (US & Canada)' }) } + + should 'have used the value returned by the serializer' do + assert_equal ['UTC', 'Pacific Time (US & Canada)'], @person.versions.last.changeset[:time_zone] + assert_equal @person.instance_variable_get(:@attributes)['time_zone'].serialized_value, @person.versions.last.changeset[:time_zone].last + end + + should 'not have stored the default, ridiculously long (to_yaml) serialization of the time_zone object' do + # Before the serialized attributes fix, the object_changes 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}" + end + end + end + + context 'A new model instance which uses a custom Version class' do setup { @post = Post.new } From ae2350b14775b46f4e8314f9bcfabd2455ebbf4c Mon Sep 17 00:00:00 2001 From: Tyler Rick Date: Mon, 5 Nov 2012 12:07:50 -0800 Subject: [PATCH 2/5] Changed #changeset so that it automatically unserializes any serialized attributes. Now version.changeset returns the same thing that record.changes returned before the save. --- lib/paper_trail/has_paper_trail.rb | 8 ++++---- lib/paper_trail/version.rb | 10 +++++++++- test/unit/model_test.rb | 32 ++++++++++++++++++++++++------ 3 files changed, 39 insertions(+), 11 deletions(-) diff --git a/lib/paper_trail/has_paper_trail.rb b/lib/paper_trail/has_paper_trail.rb index 407289ec5..12d149b5f 100644 --- a/lib/paper_trail/has_paper_trail.rb +++ b/lib/paper_trail/has_paper_trail.rb @@ -189,10 +189,10 @@ def changes_for_paper_trail # Use serialized value for attributes that are serialized changes_hash.each do |key, (old_value, new_value)| if serialized_attributes.include?(key) - # serializer.dump(new_value) is the same as @attributes[key].serialized_value - serializer = @attributes[key].coder - changes_hash[key] = [serializer.dump(old_value), - serializer.dump(new_value)] + # coder.dump(new_value) is the same as @attributes[key].serialized_value + coder = @attributes[key].coder + changes_hash[key] = [coder.dump(old_value), + coder.dump(new_value)] end end end diff --git a/lib/paper_trail/version.rb b/lib/paper_trail/version.rb index 7e4b4cc34..90f1e8c9f 100644 --- a/lib/paper_trail/version.rb +++ b/lib/paper_trail/version.rb @@ -103,7 +103,15 @@ def reify(options = {}) def changeset if self.class.column_names.include? 'object_changes' if changes = object_changes - HashWithIndifferentAccess[YAML::load(changes)] + HashWithIndifferentAccess[YAML::load(changes)].tap do |changes| + item_type.constantize.serialized_attributes.each do |key, coder| + if changes.key?(key) + old_value, new_value = changes[key] + changes[key] = [coder.load(old_value), + coder.load(new_value)] + end + end + end else {} end diff --git a/test/unit/model_test.rb b/test/unit/model_test.rb index f649aa584..09f2aa2be 100644 --- a/test/unit/model_test.rb +++ b/test/unit/model_test.rb @@ -914,17 +914,37 @@ def without(&block) end context 'when that attribute is updated' do - setup { @person.update_attributes({ :time_zone => 'Pacific Time (US & Canada)' }) } - - should 'have used the value returned by the serializer' do - assert_equal ['UTC', 'Pacific Time (US & Canada)'], @person.versions.last.changeset[:time_zone] - assert_equal @person.instance_variable_get(:@attributes)['time_zone'].serialized_value, @person.versions.last.changeset[:time_zone].last + setup do + @person.assign_attributes({ :time_zone => 'Pacific Time (US & Canada)' }) + @changes_before_save = @person.changes.dup + @person.save! end - should 'not have stored the default, ridiculously long (to_yaml) serialization of the time_zone object' do + # Tests for serialization: + should 'object_changes have stored the value returned by the attribute serializer' do + as_stored = HashWithIndifferentAccess[YAML::load(@person.versions.last.object_changes)] + assert_equal ['UTC', 'Pacific Time (US & Canada)'], as_stored[:time_zone] + assert_equal @person.instance_variable_get(:@attributes)['time_zone'].serialized_value, as_stored[:time_zone].last + end + should 'not have stored the default, ridiculously long (to_yaml) serialization of the TimeZone object' do # Before the serialized attributes fix, the object_changes 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}" end + + # Tests for unserialization: + should '#changeset should convert the attribute value back to its original, unserialized value' do + as_stored = HashWithIndifferentAccess[YAML::load(@person.versions.last.object_changes)] + assert_equal ['UTC', 'Pacific Time (US & Canada)'], as_stored[:time_zone] + assert_equal @person.instance_variable_get(:@attributes)['time_zone'].serialized_value, as_stored[: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 From 5e2cd92688c8332cbf28606053775f6f322f7817 Mon Sep 17 00:00:00 2001 From: Tyler Rick Date: Tue, 6 Nov 2012 15:09:44 -0800 Subject: [PATCH 3/5] Slightly more efficient to loop through serialized_attributes than to loop through the changes hash --- lib/paper_trail/has_paper_trail.rb | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/paper_trail/has_paper_trail.rb b/lib/paper_trail/has_paper_trail.rb index 12d149b5f..00cb9b800 100644 --- a/lib/paper_trail/has_paper_trail.rb +++ b/lib/paper_trail/has_paper_trail.rb @@ -185,14 +185,13 @@ def changes_for_paper_trail # The double negative (reject, !include?) preserves the hash structure of self.changes. self.changes.reject do |key, value| !notably_changed.include?(key) - end.tap do |changes_hash| + end.tap do |changes| # Use serialized value for attributes that are serialized - changes_hash.each do |key, (old_value, new_value)| - if serialized_attributes.include?(key) - # coder.dump(new_value) is the same as @attributes[key].serialized_value - coder = @attributes[key].coder - changes_hash[key] = [coder.dump(old_value), - coder.dump(new_value)] + serialized_attributes.each do |key, coder| + if changes.key?(key) + old_value, new_value = changes[key] + changes[key] = [coder.dump(old_value), + coder.dump(new_value)] end end end From 25a98808dd5b9ca29e2d7d112b8856ce73e4febe Mon Sep 17 00:00:00 2001 From: Tyler Rick Date: Tue, 6 Nov 2012 15:15:31 -0800 Subject: [PATCH 4/5] Cleaned up the code by adding serialize_attribute_changes/unserialize_attribute_changes helper methods, which also do a better job of showing the parallel logic between serializing and unserializing. --- lib/paper_trail/has_paper_trail.rb | 27 ++++++++++++++++++++------- lib/paper_trail/version.rb | 8 +------- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/lib/paper_trail/has_paper_trail.rb b/lib/paper_trail/has_paper_trail.rb index 00cb9b800..d0cb658d6 100644 --- a/lib/paper_trail/has_paper_trail.rb +++ b/lib/paper_trail/has_paper_trail.rb @@ -88,6 +88,25 @@ def paper_trail_off def paper_trail_on self.paper_trail_enabled_for_model = true end + + def serialize_attribute_changes(changes) + serialized_attributes.each do |key, coder| + if changes.key?(key) + old_value, new_value = changes[key] + changes[key] = [coder.dump(old_value), + coder.dump(new_value)] + end + end + end + def unserialize_attribute_changes(changes) + serialized_attributes.each do |key, coder| + if changes.key?(key) + 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 @@ -187,13 +206,7 @@ def changes_for_paper_trail !notably_changed.include?(key) end.tap do |changes| # Use serialized value for attributes that are serialized - serialized_attributes.each do |key, coder| - if changes.key?(key) - old_value, new_value = changes[key] - changes[key] = [coder.dump(old_value), - coder.dump(new_value)] - end - end + self.class.serialize_attribute_changes(changes) end end diff --git a/lib/paper_trail/version.rb b/lib/paper_trail/version.rb index 90f1e8c9f..c1a8a5f80 100644 --- a/lib/paper_trail/version.rb +++ b/lib/paper_trail/version.rb @@ -104,13 +104,7 @@ def changeset if self.class.column_names.include? 'object_changes' if changes = object_changes HashWithIndifferentAccess[YAML::load(changes)].tap do |changes| - item_type.constantize.serialized_attributes.each do |key, coder| - if changes.key?(key) - old_value, new_value = changes[key] - changes[key] = [coder.load(old_value), - coder.load(new_value)] - end - end + item_type.constantize.unserialize_attribute_changes(changes) end else {} From 748eab5034a82818643d5e61101d410cd706e10d Mon Sep 17 00:00:00 2001 From: Tyler Rick Date: Tue, 6 Nov 2012 15:41:38 -0800 Subject: [PATCH 5/5] Custom attribute serializers should also be used when storing attribute values in version.object (that is, in object_to_string) and when loading attribute values out of version.object (that is, in reify). --- lib/paper_trail/has_paper_trail.rb | 21 ++++++++++++++++++- lib/paper_trail/version.rb | 1 + test/unit/model_test.rb | 33 ++++++++++++++++++++---------- 3 files changed, 43 insertions(+), 12 deletions(-) diff --git a/lib/paper_trail/has_paper_trail.rb b/lib/paper_trail/has_paper_trail.rb index d0cb658d6..3926b0a30 100644 --- a/lib/paper_trail/has_paper_trail.rb +++ b/lib/paper_trail/has_paper_trail.rb @@ -89,6 +89,23 @@ def paper_trail_on self.paper_trail_enabled_for_model = true end + # Used for Version#object attribute + def serialize_attributes(attributes) + serialized_attributes.each do |key, coder| + if attributes.key?(key) + attributes[key] = coder.dump(attributes[key]) + end + end + end + def unserialize_attributes(attributes) + serialized_attributes.each do |key, coder| + if attributes.key?(key) + attributes[key] = coder.load(attributes[key]) + end + end + end + + # Used for Version#object_changes attribute def serialize_attribute_changes(changes) serialized_attributes.each do |key, coder| if changes.key?(key) @@ -255,7 +272,9 @@ def item_before_change end def object_to_string(object) - object.attributes.except(*self.class.skip).to_yaml + object.attributes.except(*self.class.skip).tap do |attributes| + self.class.serialize_attributes attributes + end.to_yaml end def changed_notably? diff --git a/lib/paper_trail/version.rb b/lib/paper_trail/version.rb index c1a8a5f80..7f991b297 100644 --- a/lib/paper_trail/version.rb +++ b/lib/paper_trail/version.rb @@ -79,6 +79,7 @@ def reify(options = {}) model = klass.new end + model.class.unserialize_attributes attrs attrs.each do |k, v| if model.respond_to?("#{k}=") model.send :write_attribute, k.to_sym, v diff --git a/test/unit/model_test.rb b/test/unit/model_test.rb index 09f2aa2be..20160a0c8 100644 --- a/test/unit/model_test.rb +++ b/test/unit/model_test.rb @@ -907,7 +907,7 @@ def without(&block) end context 'When an attribute has a custom serializer' do - setup { @person = Person.create(:time_zone => "UTC") } + setup { @person = Person.create(:time_zone => "Samoa") } should "be an instance of ActiveSupport::TimeZone" do assert_equal ActiveSupport::TimeZone, @person.time_zone.class @@ -915,27 +915,38 @@ def without(&block) context 'when that attribute is updated' do setup do + @attribute_value_before_change = @person.instance_variable_get(:@attributes)['time_zone'] @person.assign_attributes({ :time_zone => 'Pacific Time (US & Canada)' }) @changes_before_save = @person.changes.dup @person.save! end # Tests for serialization: - should 'object_changes have stored the value returned by the attribute serializer' do - as_stored = HashWithIndifferentAccess[YAML::load(@person.versions.last.object_changes)] - assert_equal ['UTC', 'Pacific Time (US & Canada)'], as_stored[:time_zone] - assert_equal @person.instance_variable_get(:@attributes)['time_zone'].serialized_value, as_stored[:time_zone].last + # 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 - should 'not have stored the default, ridiculously long (to_yaml) serialization of the TimeZone object' do - # Before the serialized attributes fix, the object_changes that was stored was ridiculously long (58723) + 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 + # 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] + assert_equal @attribute_value_before_change.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] + assert_equal @person.instance_variable_get(:@attributes)['time_zone'].serialized_value, as_stored_in_version[:time_zone].last + end # Tests for unserialization: - should '#changeset should convert the attribute value back to its original, unserialized value' do - as_stored = HashWithIndifferentAccess[YAML::load(@person.versions.last.object_changes)] - assert_equal ['UTC', 'Pacific Time (US & Canada)'], as_stored[:time_zone] - assert_equal @person.instance_variable_get(:@attributes)['time_zone'].serialized_value, as_stored[:time_zone].last + should 'version.reify should convert the attribute value back to its original, unserialized value' do + assert_equal @attribute_value_before_change.unserialized_value, @person.versions.last.reify.time_zone + end + should 'version.changeset should convert the attribute value back to its original, unserialized value' do + assert_equal @person.instance_variable_get(:@attributes)['time_zone'].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)