Merge pull request #917 from Dynamoid/ak/fix-sending-partition-key-and-always-dump-it-in-transactions

Fix transactional operations when primary key is of non-native DynamoDB type

Author: andrykonchin

lib/dynamoid/transaction_write/delete_with_instance.rb

@@ -35,10 +35,10 @@ def observable_by_user_result
       end
 
       def action_request
-        key = { @model_class.hash_key => @model.hash_key }
+        key = { @model_class.hash_key => dump_attribute(@model_class.hash_key, @model.hash_key) }
 
         if @model_class.range_key?
-          key[@model_class.range_key] = @model.range_value
+          key[@model_class.range_key] = dump_attribute(@model_class.range_key, @model.range_value)
         end
 
         {
@@ -55,6 +55,11 @@ def validate_model!
         raise Dynamoid::Errors::MissingHashKey if @model.hash_key.nil?
         raise Dynamoid::Errors::MissingRangeKey if @model_class.range_key? && @model.range_value.nil?
       end
+
+      def dump_attribute(name, value)
+        options = @model_class.attributes[name]
+        Dumping.dump_field(value, options)
+      end
     end
   end
 end

lib/dynamoid/transaction_write/delete_with_primary_key.rb

@@ -34,10 +34,10 @@ def observable_by_user_result
       end
 
       def action_request
-        key = { @model_class.hash_key => @hash_key }
+        key = { @model_class.hash_key => dump_attribute(@model_class.hash_key, @hash_key) }
 
         if @model_class.range_key?
-          key[@model_class.range_key] = @range_key
+          key[@model_class.range_key] = dump_attribute(@model_class.range_key, @range_key)
         end
 
         {
@@ -54,6 +54,11 @@ def validate_primary_key!
         raise Dynamoid::Errors::MissingHashKey if @hash_key.nil?
         raise Dynamoid::Errors::MissingRangeKey if @model_class.range_key? && @range_key.nil?
       end
+
+      def dump_attribute(name, value)
+        options = @model_class.attributes[name]
+        Dumping.dump_field(value, options)
+      end
     end
   end
 end

lib/dynamoid/transaction_write/destroy.rb

@@ -54,10 +54,10 @@ def observable_by_user_result
       end
 
       def action_request
-        key = { @model_class.hash_key => @model.hash_key }
+        key = { @model_class.hash_key => dump_attribute(@model_class.hash_key, @model.hash_key) }
 
         if @model_class.range_key?
-          key[@model_class.range_key] = @model.range_value
+          key[@model_class.range_key] = dump_attribute(@model_class.range_key, @model.range_value)
         end
 
         {
@@ -74,6 +74,11 @@ def validate_model!
         raise Dynamoid::Errors::MissingHashKey if @model.hash_key.nil?
         raise Dynamoid::Errors::MissingRangeKey if @model_class.range_key? && @model.range_value.nil?
       end
+
+      def dump_attribute(name, value)
+        options = @model_class.attributes[name]
+        Dumping.dump_field(value, options)
+      end
     end
   end
 end

lib/dynamoid/transaction_write/save.rb

@@ -121,8 +121,8 @@ def action_request_to_update
         changes_dumped = Dynamoid::Dumping.dump_attributes(changes, @model_class.attributes)
 
         # primary key to look up an item to update
-        key = { @model_class.hash_key => @model.hash_key }
-        key[@model_class.range_key] = @model.range_value if @model_class.range_key?
+        key = { @model_class.hash_key => dump_attribute(@model_class.hash_key, @model.hash_key) }
+        key[@model_class.range_key] = dump_attribute(@model_class.range_key, @model.range_value) if @model_class.range_key?
 
         # Build UpdateExpression and keep names and values placeholders mapping
         # in ExpressionAttributeNames and ExpressionAttributeValues.
@@ -159,6 +159,11 @@ def touch_model_timestamps(skip_created_at:)
         @model.updated_at = timestamp unless @options[:touch] == false && !@was_new_record
         @model.created_at ||= timestamp unless skip_created_at
       end
+
+      def dump_attribute(name, value)
+        options = @model_class.attributes[name]
+        Dumping.dump_field(value, options)
+      end
     end
   end
 end

lib/dynamoid/transaction_write/update_fields.rb

@@ -46,8 +46,8 @@ def action_request
         builder = UpdateRequestBuilder.new(@model_class)
 
         # primary key to look up an item to update
-        builder.hash_key = @hash_key
-        builder.range_key = @range_key if @model_class.range_key?
+        builder.hash_key = dump_attribute(@model_class.hash_key, @hash_key)
+        builder.range_key = dump_attribute(@model_class.range_key, @range_key) if @model_class.range_key?
 
         # changed attributes to persist
         changes = @attributes.dup
@@ -111,6 +111,11 @@ def add_timestamps(attributes, skip_created_at: false)
         result
       end
 
+      def dump_attribute(name, value)
+        options = @model_class.attributes[name]
+        Dumping.dump_field(value, options)
+      end
+
       class UpdateRequestBuilder
         attr_writer :hash_key, :range_key, :condition_expression
 

lib/dynamoid/transaction_write/upsert.rb

@@ -44,8 +44,13 @@ def action_request
         changes_dumped = Dynamoid::Dumping.dump_attributes(changes, @model_class.attributes)
 
         # primary key to look up an item to update
-        key = { @model_class.hash_key => @hash_key }
-        key[@model_class.range_key] = @range_key if @model_class.range_key?
+        partition_key_dumped = dump(@model_class.hash_key, @hash_key)
+        key = { @model_class.hash_key => partition_key_dumped }
+
+        if @model_class.range_key?
+          sort_key_dumped = dump(@model_class.range_key, @range_key)
+          key[@model_class.range_key] = sort_key_dumped
+        end
 
         # Build UpdateExpression and keep names and values placeholders mapping
         # in ExpressionAttributeNames and ExpressionAttributeValues.
@@ -91,6 +96,11 @@ def add_timestamps(attributes, skip_created_at: false)
         result[:updated_at] ||= timestamp
         result
       end
+
+      def dump(name, value)
+        options = @model_class.attributes[name]
+        Dumping.dump_field(value, options)
+      end
     end
   end
 end

spec/dynamoid/finders_spec.rb

@@ -14,7 +14,7 @@
       end
     end
 
-    context 'one primary key provided' do
+    context 'a single primary key provided' do
       context 'simple primary key' do
         it 'finds' do
           obj = klass.create
@@ -103,21 +103,21 @@
           end
         end
 
-        context 'when true' do
+        context 'when false' do
           it 'leads to not raising exception if model not found' do
             expect(klass.find('blah-blah', raise_error: false)).to eq nil
           end
         end
       end
 
-      it 'uses dumped value of partition key to update item' do
+      it 'uses dumped value of partition key' do
         klass = new_class(partition_key: { name: :published_on, type: :date })
 
         obj = klass.create!(published_on: '2018-10-07'.to_date)
         expect(klass.find(obj.published_on)).to eql(obj)
       end
 
-      it 'uses dumped value of sort key to update item' do
+      it 'uses dumped value of sort key' do
         klass = new_class do
           range :published_on, :date
         end
@@ -226,19 +226,6 @@
           expect(klass_with_composite_key.find([[obj1.id, '1'], [obj2.id, '2']])).to match_array(objects)
         end
 
-        it 'dumps a sort key value' do
-          klass_with_date = new_class do
-            range :published_on, :date
-          end
-
-          obj1 = klass_with_date.create(published_on: '2018/07/26'.to_date)
-          obj2 = klass_with_date.create(published_on: '2018/07/27'.to_date)
-
-          expect(
-            klass_with_date.find([[obj1.id, obj1.published_on], [obj2.id, obj2.published_on]])
-          ).to contain_exactly(obj1, obj2)
-        end
-
         it 'raises MissingRangeKey when range key is not specified' do
           obj1, obj2 = klass_with_composite_key.create([{ age: 1 }, { age: 2 }])
 
@@ -274,25 +261,23 @@
           end
         end
 
-        context 'when true' do
+        context 'when false' do
           it 'leads to not raising exception if model not found' do
-            obj = klass.create
-
-            # expect(klass.find([obj.id, 'blah-blah'], raise_error: false)).to eq [obj]
-            expect(klass.find_all([obj.id, 'blah-blah'])).to eq [obj]
+            obj = klass.create!
+            expect(klass.find([obj.id, 'blah-blah'], raise_error: false)).to eq [obj]
           end
         end
       end
 
-      it 'uses dumped value of partition key to update item' do
+      it 'uses dumped value of partition key' do
         klass = new_class(partition_key: { name: :published_on, type: :date })
         objects = (1..2).map { |i| klass.create(published_on: '2018-10-07'.to_date + i) }
         obj1, obj2 = objects
 
         expect(klass.find([obj1.published_on, obj2.published_on])).to match_array(objects)
       end
 
-      it 'uses dumped value of sort key to update item' do
+      it 'uses dumped value of sort key' do
         klass = new_class do
           range :published_on, :date
         end

spec/dynamoid/persistence_spec.rb

@@ -2651,7 +2651,7 @@ def around_update_callback
 
     context 'primary key dumping' do
       context 'new model' do
-        it 'uses dumped value of partition key to update item' do
+        it 'uses dumped value of partition key to save item' do
           klass = new_class(partition_key: { name: :published_on, type: :date }) do
             field :title
           end
@@ -2663,7 +2663,7 @@ def around_update_callback
           expect(obj_loaded.title).to eq 'Some title'
         end
 
-        it 'uses dumped value of sort key to update item' do
+        it 'uses dumped value of sort key to save item' do
           klass = new_class do
             range :published_on, :date
             field :title
@@ -2678,7 +2678,7 @@ def around_update_callback
       end
 
       context 'persised model' do
-        it 'uses dumped value of partition key to update item' do
+        it 'uses dumped value of partition key to save item' do
           klass = new_class(partition_key: { name: :published_on, type: :date }) do
             field :title
           end
@@ -2691,7 +2691,7 @@ def around_update_callback
           expect(obj_loaded.title).to eq 'New'
         end
 
-        it 'uses dumped value of sort key to update item' do
+        it 'uses dumped value of sort key to save item' do
           klass = new_class do
             range :published_on, :date
             field :title