Merge pull request #851 from Dynamoid/ak/don-require-custom-type-to-implement-equality-method

Fix Dirty API to compare objects of custom type using dumps

Author: andrykonchin

README.md

@@ -471,6 +471,27 @@ method, which would return either `:string` or `:number`.
 DynamoDB may support some other attribute types that are not yet
 supported by Dynamoid.
 
+If a custom type implements `#==` method you can specify `comparable:
+true` option in a field declaration to specify that an object is safely
+comparable for the purpose of detecting changes. By default old and new
+objects will be compared by their serialized representation.
+
+```ruby
+class Money
+  # ...
+
+  def ==(other)
+    # comparison logic
+  end
+end
+
+class User
+  # ...
+
+  field :balance, Money, comparable: true
+end
+```
+
 ### Sort key
 
 Along with partition key table may have a sort key. In order to declare

lib/dynamoid/dirty.rb

@@ -301,7 +301,19 @@ def attribute_changed_in_place?(name)
       return false if value_from_database.nil?
 
       value = read_attribute(name)
-      value != value_from_database
+      type_options = self.class.attributes[name.to_sym]
+
+      unless type_options[:type].is_a?(Class) && !type_options[:comparable]
+        # common case
+        value != value_from_database
+      else
+        # objects of a custom type that does not implement its own `#==` method
+        # (that's declared by `comparable: false` or just not specifying the
+        # option `comparable`) are compared by comparing their dumps
+        dump = Dumping.dump_field(value, type_options)
+        dump_from_database = Dumping.dump_field(value_from_database, type_options)
+        dump != dump_from_database
+      end
     end
 
     module DeepDupper
@@ -318,26 +330,25 @@ def self.dup_attribute(value, type_options)
 
         case value
         when NilClass, TrueClass, FalseClass, Numeric, Symbol, IO
-          # till Ruby 2.4 these immutable objects could not be duplicated
-          # IO objects cannot be duplicated - is used for binary fields
+          # Till Ruby 2.4 these immutable objects could not be duplicated.
+          # IO objects (used for the binary type) cannot be duplicated as well.
           value
-        when String
-          value.dup
         when Array
-          if of.is_a? Class # custom type
+          if of.is_a? Class
+            # custom type
             value.map { |e| dup_attribute(e, type: of) }
           else
             value.deep_dup
           end
         when Set
           Set.new(value.map { |e| dup_attribute(e, type: of) })
-        when Hash
-          value.deep_dup
         else
-          if type.is_a? Class # custom type
-            Marshal.load(Marshal.dump(value)) # dup instance variables
+          if type.is_a? Class
+            # custom type
+            dump = Dumping.dump_field(value, type_options)
+            Undumping.undump_field(dump.deep_dup, type_options)
           else
-            value.dup # date, datetime
+            value.deep_dup
           end
         end
       end

spec/dynamoid/dirty_spec.rb

@@ -646,7 +646,7 @@
 
     let(:klass_with_set_of_custom_type) do
       new_class do
-        field :users, :set, of: DirtySpec::User
+        field :users, :set, of: DirtySpec::UserWithEquality
       end
     end
 
@@ -686,6 +686,12 @@
       end
     end
 
+    let(:klass_with_comparable_custom_type) do
+      new_class do
+        field :user, DirtySpec::UserWithEquality, comparable: true
+      end
+    end
+
     context 'string type' do
       it 'detects in-place modifying a String value' do
         obj = klass_with_string.create!(name: +'Alex')
@@ -711,10 +717,15 @@
       end
 
       it 'detects in-place modifying of a Set element' do
-        obj = klass_with_set_of_custom_type.create!(users: [DirtySpec::User.new(+'Alex')])
+        obj = klass_with_set_of_custom_type.create!(users: [DirtySpec::UserWithEquality.new(+'Alex')])
         obj.users.map { |u| u.name.upcase! }
 
-        expect(obj.changes).to eq('users' => [Set[DirtySpec::User.new('Alex')], Set[DirtySpec::User.new('ALEX')]])
+        expect(obj.changes).to eq(
+          'users' => [
+            Set[DirtySpec::UserWithEquality.new('Alex')],
+            Set[DirtySpec::UserWithEquality.new('ALEX')]
+          ]
+        )
       end
     end
 
@@ -792,11 +803,56 @@
     end
 
     context 'custom type' do
-      it 'detects in-place modifying a String value' do
+      it 'detects in-place modifying' do
         obj = klass_with_custom_type.create!(user: DirtySpec::User.new(+'Alex'))
         obj.user.name.upcase!
+        ScratchPad.record []
+
+        old_value, new_value = obj.changes['user']
+
+        expect(old_value.name).to eq 'Alex'
+        expect(new_value.name).to eq 'ALEX'
+        expect(ScratchPad.recorded).to eq([])
+      end
+
+      it 'detects in-place modifying when custom type is safely comparable' do
+        obj = klass_with_comparable_custom_type.create!(user: DirtySpec::UserWithEquality.new(+'Alex'))
+        obj.user.name.upcase!
+        ScratchPad.record []
+
+        old_value, new_value = obj.changes['user']
+
+        expect(old_value.name).to eq 'Alex'
+        expect(new_value.name).to eq 'ALEX'
+
+        expect(ScratchPad.recorded.size).to eq(1)
+        record = ScratchPad.recorded[0]
+
+        expect(record[0]).to eq('==')
+        expect(record[1]).to equal(new_value)
+        expect(record[2]).to equal(old_value)
+      end
+
+      it 'reports no in-place changes when field is not modified' do
+        obj = klass_with_custom_type.create!(user: DirtySpec::User.new('Alex'))
+
+        ScratchPad.record []
+        expect(obj.changes['user']).to eq(nil)
+        expect(ScratchPad.recorded).to eq([])
+      end
+
+      it 'reports no in-place changes when field is not modified and custom type is safely comparable' do
+        obj = klass_with_comparable_custom_type.create!(user: DirtySpec::UserWithEquality.new('Alex'))
+        ScratchPad.record []
+
+        expect(obj.changes['user']).to eq(nil)
+
+        expect(ScratchPad.recorded.size).to eq(1)
+        record = ScratchPad.recorded[0]
 
-        expect(obj.changes).to eq('user' => [DirtySpec::User.new('Alex'), DirtySpec::User.new('ALEX')])
+        expect(record[0]).to eq('==')
+        expect(record[1]).to equal(obj.user)
+        expect(record[2]).to eq(obj.user) # an implicit 'from-database' copy
       end
     end
   end

spec/fixtures/dirty.rb

@@ -12,20 +12,30 @@ def dynamoid_dump
       @name
     end
 
+    def self.dynamoid_load(string)
+      new(string.to_s)
+    end
+  end
+
+  class UserWithEquality < User
     def eql?(other)
+      if ScratchPad.recorded.is_a? Array
+        ScratchPad << ['eql?', self, other]
+      end
+
       @name == other.name
     end
 
     def ==(other)
+      if ScratchPad.recorded.is_a? Array
+        ScratchPad << ['==', self, other]
+      end
+
       @name == other.name
     end
 
     def hash
       @name.hash
     end
-
-    def self.dynamoid_load(string)
-      new(string.to_s)
-    end
   end
 end

spec/fixtures/dumping.rb

@@ -37,14 +37,6 @@ def dynamoid_dump
       "#{name} (dumped with #dynamoid_dump)"
     end
 
-    def eql?(other)
-      name == other.name
-    end
-
-    def hash
-      name.hash
-    end
-
     def self.dynamoid_dump(user)
       "#{user.name} (dumped with .dynamoid_dump)"
     end