Merge pull request #485 from Dynamoid/update-associations-on-object-delete

Update associations on object delete

Author: andrykonchin

CHANGELOG.md

@@ -14,6 +14,7 @@
 ## Fixes
 
 * [#480](https://github.com/Dynamoid/dynamoid/pull/480) Repair `.consistent`/`.delete_all`/`.destroy_all` calls directly on a model class
+* [#484](https://github.com/Dynamoid/dynamoid/pull/484) Fix broken foreign keys after model deleting (@kkan)
 * Fixes in Readme.md: [#470](https://github.com/Dynamoid/dynamoid/pull/470) (@rromanchuk), [#473](https://github.com/Dynamoid/dynamoid/pull/473) (@Rulikkk)
 
 ---

lib/dynamoid/associations/association.rb

@@ -58,6 +58,12 @@ def declaration_field_type
         :set
       end
 
+      def disassociate_source
+        Array(target).each do |target_entry|
+          target_entry.send(target_association).disassociate(source.hash_key) if target_entry && target_association
+        end
+      end
+
       private
 
       # The target class name, either inferred through the association's name or specified in options.

lib/dynamoid/associations/many_association.rb

@@ -25,7 +25,9 @@ def initialize(*args)
       # @private
       # @since 0.2.0
       def find_target
-        Array(target_class.find(source_ids.to_a))
+        return [] if source_ids.empty?
+
+        Array(target_class.find(source_ids.to_a, raise_error: false))
       end
 
       # @private

lib/dynamoid/associations/single_association.rb

@@ -28,7 +28,7 @@ def setter(object)
       # unsaved changes will be saved. Doesn't delete an associated model from
       # DynamoDB.
       def delete
-        target.send(target_association).disassociate(source.hash_key) if target && target_association
+        disassociate_source
         disassociate
         target
       end
@@ -90,7 +90,7 @@ def empty?
 
       # @private
       def associate(hash_key)
-        target.send(target_association).disassociate(source.hash_key) if target && target_association
+        disassociate_source
         source.update_attribute(source_attribute, Set[hash_key])
       end
 
@@ -109,7 +109,7 @@ def disassociate(_hash_key = nil)
       def find_target
         return if source_ids.empty?
 
-        target_class.find(source_ids.first)
+        target_class.find(source_ids.first, raise_error: false)
       end
 
       def target=(object)

lib/dynamoid/persistence.rb

@@ -802,6 +802,10 @@ def delete
       @destroyed = true
 
       Dynamoid.adapter.delete(self.class.table_name, hash_key, options)
+
+      self.class.associations.each do |name, options|
+        send(name).disassociate_source
+      end
     rescue Dynamoid::Errors::ConditionalCheckFailedException
       raise Dynamoid::Errors::StaleObjectError.new(self, 'delete')
     end

spec/dynamoid/persistence_spec.rb

@@ -2559,6 +2559,10 @@ def log_message
     end
   end
 
+  context 'destroy' do
+    # TODO: adopt test cases for the `delete` method
+  end
+
   context 'delete' do
     it 'uses dumped value of sort key to call DeleteItem' do
       klass = new_class do
@@ -2597,6 +2601,146 @@ def log_message
         expect { a1.destroy }.to_not raise_error
       end
     end
+
+    context 'when model has associations' do
+      context 'when belongs_to association' do
+        context 'when has_many on the other side' do
+          let!(:source_model) { User.create }
+          let!(:target_model) { source_model.camel_case.create }
+
+          it 'disassociates self' do
+            expect do
+              source_model.delete
+            end.to change { CamelCase.find(target_model.id).users.target }.from([source_model]).to([])
+          end
+
+          it 'updates cached ids list in associated model' do
+            source_model.delete
+            expect(CamelCase.find(target_model.id).users_ids).to eq nil
+          end
+
+          it 'behaves correctly when associated model is linked with several models' do
+            source_model2 = User.create
+            target_model.users << source_model2
+
+            expect(CamelCase.find(target_model.id).users.target).to contain_exactly(source_model, source_model2)
+            source_model.delete
+            expect(CamelCase.find(target_model.id).users.target).to contain_exactly(source_model2)
+            expect(CamelCase.find(target_model.id).users_ids).to eq [source_model2.id].to_set
+          end
+
+          it 'does not raise exception when foreign key is broken' do
+            source_model.update_attributes!(camel_case_ids: ['fake_id'])
+
+            expect { source_model.delete }.not_to raise_error
+            expect(CamelCase.find(target_model.id).users.target).to eq []
+          end
+        end
+
+        context 'when has_one on the other side' do
+          let!(:source_model) { Sponsor.create }
+          let!(:target_model) { source_model.camel_case.create }
+
+          it 'disassociates self' do
+            expect do
+              source_model.delete
+            end.to change { CamelCase.find(target_model.id).sponsor.target }.from(source_model).to(nil)
+          end
+
+          it 'updates cached ids list in associated model' do
+            source_model.delete
+            expect(CamelCase.find(target_model.id).sponsor_ids).to eq nil
+          end
+
+          it 'does not raise exception when foreign key is broken' do
+            source_model.update_attributes!(camel_case_ids: ['fake_id'])
+
+            expect { source_model.delete }.not_to raise_error
+            expect(CamelCase.find(target_model.id).sponsor.target).to eq nil
+          end
+        end
+      end
+
+      context 'when has_many association' do
+        let!(:source_model) { User.create }
+        let!(:target_model) { source_model.books.create }
+
+        it 'disassociates self' do
+          expect do
+            source_model.delete
+          end.to change { Magazine.find(target_model.title).owner.target }.from(source_model).to(nil)
+        end
+
+        it 'updates cached ids list in associated model' do
+          source_model.delete
+          expect(Magazine.find(target_model.title).owner_ids).to eq nil
+        end
+
+        it 'does not raise exception when cached foreign key is broken' do
+          books_ids_new = source_model.books_ids + ['fake_id']
+          source_model.update_attributes!(books_ids: books_ids_new)
+
+          expect { source_model.delete }.not_to raise_error
+          expect(Magazine.find(target_model.title).owner).to eq nil
+        end
+      end
+
+      context 'when has_one association' do
+        let!(:source_model) { User.create }
+        let!(:target_model) { source_model.monthly.create }
+
+        it 'disassociates self' do
+          expect do
+            source_model.delete
+          end.to change { Subscription.find(target_model.id).customer.target }.from(source_model).to(nil)
+        end
+
+        it 'updates cached ids list in associated model' do
+          source_model.delete
+          expect(Subscription.find(target_model.id).customer_ids).to eq nil
+        end
+
+        it 'does not raise exception when cached foreign key is broken' do
+          source_model.update_attributes!(monthly_ids: ['fake_id'])
+
+          expect { source_model.delete }.not_to raise_error
+        end
+      end
+
+      context 'when has_and_belongs_to_many association' do
+        let!(:source_model) { User.create }
+        let!(:target_model) { source_model.subscriptions.create }
+
+        it 'disassociates self' do
+          expect do
+            source_model.delete
+          end.to change { Subscription.find(target_model.id).users.target }.from([source_model]).to([])
+        end
+
+        it 'updates cached ids list in associated model' do
+          source_model.delete
+          expect(Subscription.find(target_model.id).users_ids).to eq nil
+        end
+
+        it 'behaves correctly when associated model is linked with several models' do
+          source_model2 = User.create
+          target_model.users << source_model2
+
+          expect(Subscription.find(target_model.id).users.target).to contain_exactly(source_model, source_model2)
+          source_model.delete
+          expect(Subscription.find(target_model.id).users.target).to contain_exactly(source_model2)
+          expect(Subscription.find(target_model.id).users_ids).to eq [source_model2.id].to_set
+        end
+
+        it 'does not raise exception when foreign key is broken' do
+          subscriptions_ids_new = source_model.subscriptions_ids + ['fake_id']
+          source_model.update_attributes!(subscriptions_ids: subscriptions_ids_new)
+
+          expect { source_model.delete }.not_to raise_error
+          expect(Subscription.find(target_model.id).users_ids).to eq nil
+        end
+      end
+    end
   end
 
   describe '.import' do