From 00c86f928d73b08bbf1921b6c973ca0111945f45 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Wed, 7 Jun 2023 07:51:43 -0600 Subject: [PATCH] MONGOID-5560 alias delete_one to delete for `Many` proxies (#5633) * MONGOID-5560 alias EmbedsMany#delete_one to EmbedsMany#delete * MONGOID-5560 alias HasMany::Proxy#delete_one to HasMany::Proxy#delete * MONGOID-5560 alias HasAndBelongsToMany::Proxy#delete_one to HasAndBelongsToMany::Proxy#delete * rubocop appeasement * turns out, EmbedsMany::Proxy#delete_one *was* being used. Let's pull that out of private visibility, make it @api private, and give it a better name, besides. Should make it clearer that this has internal use outside of this class. --- .../association/embedded/embeds_many/proxy.rb | 32 +- .../has_and_belongs_to_many/proxy.rb | 4 + .../association/referenced/has_many/proxy.rb | 4 + lib/mongoid/threaded.rb | 4 - lib/mongoid/traversable.rb | 2 +- .../embedded/embeds_many/proxy_spec.rb | 69 ++-- .../has_and_belongs_to_many/proxy_spec.rb | 340 ++++++++---------- .../referenced/has_many/proxy_spec.rb | 148 ++++---- spec/mongoid/attributes_spec.rb | 4 +- 9 files changed, 283 insertions(+), 324 deletions(-) diff --git a/lib/mongoid/association/embedded/embeds_many/proxy.rb b/lib/mongoid/association/embedded/embeds_many/proxy.rb index 8629fe55ee..b09b9953c7 100644 --- a/lib/mongoid/association/embedded/embeds_many/proxy.rb +++ b/lib/mongoid/association/embedded/embeds_many/proxy.rb @@ -155,6 +155,23 @@ def delete(document) end end + # Mongoid::Extensions::Array defines Array#delete_one, so we need + # to make sure that method behaves reasonably on proxies, too. + alias delete_one delete + + # Removes a single document from the collection *in memory only*. + # It will *not* persist the change. + # + # @param [ Document ] document The document to delete. + # + # @api private + def _remove(document) + _target.delete_one(document) + _unscoped.delete_one(document) + update_attributes_hash + reindex + end + # Delete all the documents in the association without running callbacks. # # @example Delete all documents from the association. @@ -399,21 +416,6 @@ def criteria _association.criteria(_base, _target) end - # Deletes one document from the target and unscoped. - # - # @api private - # - # @example Delete one document. - # relation.delete_one(doc) - # - # @param [ Document ] document The document to delete. - def delete_one(document) - _target.delete_one(document) - _unscoped.delete_one(document) - update_attributes_hash - reindex - end - # Integrate the document into the association. will set its metadata and # attempt to bind the inverse. # diff --git a/lib/mongoid/association/referenced/has_and_belongs_to_many/proxy.rb b/lib/mongoid/association/referenced/has_and_belongs_to_many/proxy.rb index 907c2625cc..701baf4b46 100644 --- a/lib/mongoid/association/referenced/has_and_belongs_to_many/proxy.rb +++ b/lib/mongoid/association/referenced/has_and_belongs_to_many/proxy.rb @@ -138,6 +138,10 @@ def delete(document) doc end + # Mongoid::Extensions::Array defines Array#delete_one, so we need + # to make sure that method behaves reasonably on proxies, too. + alias delete_one delete + # Removes all associations between the base document and the target # documents by deleting the foreign keys and the references, orphaning # the target documents in the process. diff --git a/lib/mongoid/association/referenced/has_many/proxy.rb b/lib/mongoid/association/referenced/has_many/proxy.rb index fdee24a3fc..95c895f58b 100644 --- a/lib/mongoid/association/referenced/has_many/proxy.rb +++ b/lib/mongoid/association/referenced/has_many/proxy.rb @@ -106,6 +106,10 @@ def delete(document) end end + # Mongoid::Extensions::Array defines Array#delete_one, so we need + # to make sure that method behaves reasonably on proxies, too. + alias delete_one delete + # Deletes all related documents from the database given the supplied # conditions. # diff --git a/lib/mongoid/threaded.rb b/lib/mongoid/threaded.rb index 7c78799f2a..57079745f0 100644 --- a/lib/mongoid/threaded.rb +++ b/lib/mongoid/threaded.rb @@ -34,10 +34,6 @@ module Threaded # executed on documents. EXECUTE_CALLBACKS = '[mongoid]:execute-callbacks' - # The key storing the default value for whether or not callbacks are - # executed on documents. - EXECUTE_CALLBACKS = '[mongoid]:execute-callbacks' - extend self # Begin entry into a named thread local stack. diff --git a/lib/mongoid/traversable.rb b/lib/mongoid/traversable.rb index da0f8746b9..1be7f1544d 100644 --- a/lib/mongoid/traversable.rb +++ b/lib/mongoid/traversable.rb @@ -238,7 +238,7 @@ def remove_child(child) remove_ivar(name) else relation = send(name) - relation.send(:delete_one, child) + relation._remove(child) end end diff --git a/spec/mongoid/association/embedded/embeds_many/proxy_spec.rb b/spec/mongoid/association/embedded/embeds_many/proxy_spec.rb index a6d700b370..ae501c84c6 100644 --- a/spec/mongoid/association/embedded/embeds_many/proxy_spec.rb +++ b/spec/mongoid/association/embedded/embeds_many/proxy_spec.rb @@ -1863,51 +1863,56 @@ class TrackingIdValidationHistory end end - describe "#delete" do + %i[ delete delete_one ].each do |method| + describe "\##{method}" do + let(:address_one) { Address.new(street: "first") } + let(:address_two) { Address.new(street: "second") } - let(:person) do - Person.new - end - - let(:address_one) do - Address.new(street: "first") - end + before do + person.addresses << [ address_one, address_two ] + end - let(:address_two) do - Address.new(street: "second") - end + shared_examples_for 'deleting from the collection' do + context 'when the document exists in the relation' do + let!(:deleted) do + person.addresses.send(method, address_one) + end - before do - person.addresses << [ address_one, address_two ] - end + it 'deletes the document' do + expect(person.addresses).to eq([ address_two ]) + expect(person.reload.addresses).to eq([ address_two ]) if person.persisted? + end - context "when the document exists in the relation" do + it 'deletes the document from the unscoped' do + expect(person.addresses.send(:_unscoped)).to eq([ address_two ]) + end - let!(:deleted) do - person.addresses.delete(address_one) - end + it 'reindexes the relation' do + expect(address_two._index).to eq(0) + end - it "deletes the document" do - expect(person.addresses).to eq([ address_two ]) - end + it 'returns the document' do + expect(deleted).to eq(address_one) + end + end - it "deletes the document from the unscoped" do - expect(person.addresses.send(:_unscoped)).to eq([ address_two ]) + context 'when the document does not exist' do + it 'returns nil' do + expect(person.addresses.send(method, Address.new)).to be_nil + end + end end - it "reindexes the relation" do - expect(address_two._index).to eq(0) - end + context 'when the root document is unpersisted' do + let(:person) { Person.new } - it "returns the document" do - expect(deleted).to eq(address_one) + it_behaves_like 'deleting from the collection' end - end - context "when the document does not exist" do + context 'when the root document is persisted' do + let(:person) { Person.create } - it "returns nil" do - expect(person.addresses.delete(Address.new)).to be_nil + it_behaves_like 'deleting from the collection' end end end diff --git a/spec/mongoid/association/referenced/has_and_belongs_to_many/proxy_spec.rb b/spec/mongoid/association/referenced/has_and_belongs_to_many/proxy_spec.rb index 69fc1aa8c4..1bccfb1229 100644 --- a/spec/mongoid/association/referenced/has_and_belongs_to_many/proxy_spec.rb +++ b/spec/mongoid/association/referenced/has_and_belongs_to_many/proxy_spec.rb @@ -2086,283 +2086,229 @@ end end - describe "#delete" do + %i[ delete delete_one ].each do |method| + describe "\##{method}" do + let(:person) { Person.create! } + let(:preference_one) { Preference.create!(name: "Testing") } + let(:preference_two) { Preference.create!(name: "Test") } - let(:person) do - Person.create! - end - - let(:preference_one) do - Preference.create!(name: "Testing") - end - - let(:preference_two) do - Preference.create!(name: "Test") - end - - before do - person.preferences << [ preference_one, preference_two ] - end - - context "when the document exists" do - - let!(:deleted) do - person.preferences.delete(preference_one) - end - - it "removes the document from the relation" do - expect(person.preferences).to eq([ preference_two ]) - end - - it "returns the document" do - expect(deleted).to eq(preference_one) - end - - it "removes the document key from the foreign key" do - expect(person.preference_ids).to eq([ preference_two.id ]) - end - - it "removes the inverse reference" do - expect(deleted.reload.people).to be_empty - end - - it "removes the base id from the inverse keys" do - expect(deleted.reload.person_ids).to be_empty + before do + person.preferences << [ preference_one, preference_two ] end - context "and person and preferences are reloaded" do - - before do - person.reload - preference_one.reload - preference_two.reload + context 'when the document exists' do + let!(:deleted) do + person.preferences.send(method, preference_one) end - it "nullifies the deleted preference" do + it 'removes the document from the relation' do expect(person.preferences).to eq([ preference_two ]) end - it "retains the ids for one preference" do - expect(person.preference_ids).to eq([ preference_two.id ]) + it 'returns the document' do + expect(deleted).to eq(preference_one) end - end - end - - context "when the document does not exist" do - - let!(:deleted) do - person.preferences.delete(Preference.new) - end - - it "returns nil" do - expect(deleted).to be_nil - end - - it "does not modify the relation" do - expect(person.preferences).to eq([ preference_one, preference_two ]) - end - - it "does not modify the keys" do - expect(person.preference_ids).to eq([ preference_one.id, preference_two.id ]) - end - end - context "when :dependent => :nullify is set" do - - context "when :inverse_of is set" do - - let(:event) do - Event.create! + it 'removes the document key from the foreign key' do + expect(person.preference_ids).to eq([ preference_two.id ]) end - before do - person.administrated_events << [ event ] + it 'removes the inverse reference' do + expect(deleted.reload.people).to be_empty end - it "deletes the document" do - expect(event.delete).to be true + it 'removes the base id from the inverse keys' do + expect(deleted.reload.person_ids).to be_empty end - end - end - - context "when the relationships are self referencing" do - let(:tag_one) do - Tag.create!(text: "one") - end + context 'and person and preferences are reloaded' do + before do + person.reload + preference_one.reload + preference_two.reload + end - let(:tag_two) do - Tag.create!(text: "two") - end + it 'nullifies the deleted preference' do + expect(person.preferences).to eq([ preference_two ]) + end - before do - tag_one.related << tag_two + it 'retains the ids for one preference' do + expect(person.preference_ids).to eq([ preference_two.id ]) + end + end end - context "when deleting without reloading" do - + context 'when the document does not exist' do let!(:deleted) do - tag_one.related.delete(tag_two) - end - - it "deletes the document from the relation" do - expect(tag_one.related).to be_empty + person.preferences.send(method, Preference.new) end - it "deletes the foreign key from the relation" do - expect(tag_one.related_ids).to be_empty + it 'returns nil' do + expect(deleted).to be_nil end - it "removes the reference from the inverse" do - expect(deleted.related).to be_empty + it 'does not modify the relation' do + expect(person.preferences).to eq([ preference_one, preference_two ]) end - it "removes the foreign keys from the inverse" do - expect(deleted.related_ids).to be_empty + it 'does not modify the keys' do + expect(person.preference_ids).to eq([ preference_one.id, preference_two.id ]) end end - context "when deleting with reloading" do + context 'when :dependent => :nullify is set' do + context 'when :inverse_of is set' do + let(:event) { Event.create! } - context "when deleting from the front side" do - - let(:reloaded) do - tag_one.reload - end - - let!(:deleted) do - reloaded.related.delete(tag_two) - end - - it "deletes the document from the relation" do - expect(reloaded.related).to be_empty + before do + person.administrated_events << [ event ] end - it "deletes the foreign key from the relation" do - expect(reloaded.related_ids).to be_empty + it 'deletes the document' do + expect(event.delete).to be true end + end + end - it "removes the reference from the inverse" do - expect(deleted.related).to be_empty - end + context 'when the relationships are self referencing' do + let(:tag_one) { Tag.create!(text: "one") } + let(:tag_two) { Tag.create!(text: "two") } - it "removes the foreign keys from the inverse" do - expect(deleted.related_ids).to be_empty - end + before do + tag_one.related << tag_two end - context "when deleting from the inverse side" do - - let(:reloaded) do - tag_two.reload - end + context 'when deleting without reloading' do + let!(:deleted) { tag_one.related.send(method, tag_two) } - let!(:deleted) do - reloaded.related.delete(tag_one) + it 'deletes the document from the relation' do + expect(tag_one.related).to be_empty end - it "deletes the document from the relation" do - expect(reloaded.related).to be_empty + it 'deletes the foreign key from the relation' do + expect(tag_one.related_ids).to be_empty end - it "deletes the foreign key from the relation" do - expect(reloaded.related_ids).to be_empty + it 'removes the reference from the inverse' do + expect(deleted.related).to be_empty end - it "removes the foreign keys from the inverse" do + it 'removes the foreign keys from the inverse' do expect(deleted.related_ids).to be_empty end end - end - end - context "when the association has callbacks" do + context 'when deleting with reloading' do + context "when deleting from the front side" do + let(:reloaded) { tag_one.reload } + let!(:deleted) { reloaded.related.send(method, tag_two) } - let(:post) do - Post.new - end + it 'deletes the document from the relation' do + expect(reloaded.related).to be_empty + end - let(:tag) do - Tag.new - end + it 'deletes the foreign key from the relation' do + expect(reloaded.related_ids).to be_empty + end - before do - post.tags << tag - end + it 'removes the reference from the inverse' do + expect(deleted.related).to be_empty + end - context "when the callback is a before_remove" do + it 'removes the foreign keys from the inverse' do + expect(deleted.related_ids).to be_empty + end + end - context "when there are no errors" do + context 'when deleting from the inverse side' do + let(:reloaded) { tag_two.reload } + let!(:deleted) { reloaded.related.send(method, tag_one) } - before do - post.tags.delete tag - end + it 'deletes the document from the relation' do + expect(reloaded.related).to be_empty + end - it "executes the callback" do - expect(post.before_remove_called).to be true - end + it 'deletes the foreign key from the relation' do + expect(reloaded.related_ids).to be_empty + end - it "removes the document from the relation" do - expect(post.tags).to be_empty + it 'removes the foreign keys from the inverse' do + expect(deleted.related_ids).to be_empty + end end end + end - context "when errors are raised" do - - before do - expect(post).to receive(:before_remove_tag).and_raise - begin; post.tags.delete(tag); rescue; end - end + context 'when the association has callbacks' do + let(:post) { Post.new } + let(:tag) { Tag.new } - it "does not remove the document from the relation" do - expect(post.tags).to eq([ tag ]) - end + before do + post.tags << tag end - end - context "when the callback is an after_remove" do + context 'when the callback is a before_remove' do + context 'when there are no errors' do + before do + post.tags.send(method, tag) + end - context "when no errors are raised" do + it 'executes the callback' do + expect(post.before_remove_called).to be true + end - before do - post.tags.delete(tag) + it 'removes the document from the relation' do + expect(post.tags).to be_empty + end end - it "executes the callback" do - expect(post.after_remove_called).to be true - end + context "when errors are raised" do + before do + expect(post).to receive(:before_remove_tag).and_raise + begin; post.tags.send(method, tag); rescue; end + end - it "removes the document from the relation" do - expect(post.tags).to be_empty + it 'does not remove the document from the relation' do + expect(post.tags).to eq([ tag ]) + end end end - context "when errors are raised" do + context 'when the callback is an after_remove' do + context 'when no errors are raised' do + before do + post.tags.send(method, tag) + end - before do - expect(post).to receive(:after_remove_tag).and_raise - begin; post.tags.delete(tag); rescue; end + it 'executes the callback' do + expect(post.after_remove_called).to be true + end + + it 'removes the document from the relation' do + expect(post.tags).to be_empty + end end - it "removes the document from the relation" do - expect(post.tags).to be_empty + context 'when errors are raised' do + before do + expect(post).to receive(:after_remove_tag).and_raise + begin; post.tags.send(method, tag); rescue; end + end + + it 'removes the document from the relation' do + expect(post.tags).to be_empty + end end end end end end - [ :delete_all, :destroy_all ].each do |method| - - describe "##{method}" do - - context "when the relation is not polymorphic" do - - context "when conditions are provided" do - - let(:person) do - Person.create! - end + %i[ delete_all destroy_all ].each do |method| + describe "\##{method}" do + context 'when the relation is not polymorphic' do + context 'when conditions are provided' do + let(:person) { Person.create! } let!(:preference_one) do person.preferences.create!(name: "Testing") diff --git a/spec/mongoid/association/referenced/has_many/proxy_spec.rb b/spec/mongoid/association/referenced/has_many/proxy_spec.rb index 14b73d9931..8e4e3629ef 100644 --- a/spec/mongoid/association/referenced/has_many/proxy_spec.rb +++ b/spec/mongoid/association/referenced/has_many/proxy_spec.rb @@ -1665,110 +1665,112 @@ def with_transaction_via(model, &block) end end - describe '#delete' do - let!(:person) { Person.create!(username: 'arthurnn') } + %i[ delete delete_one ].each do |method| + describe "##{method}" do + let!(:person) { Person.create!(username: 'arthurnn') } - context 'when the document is found' do - context 'when no dependent option is set' do - context 'when we are assigning attributes' do - let!(:drug) { person.drugs.create! } - let(:deleted) { person.drugs.delete(drug) } + context 'when the document is found' do + context 'when no dependent option is set' do + context 'when we are assigning attributes' do + let!(:drug) { person.drugs.create! } + let(:deleted) { person.drugs.send(method, drug) } - before do - Mongoid::Threaded.begin_execution(:assign) - end + before do + Mongoid::Threaded.begin_execution(:assign) + end - after do - Mongoid::Threaded.exit_execution(:assign) - end + after do + Mongoid::Threaded.exit_execution(:assign) + end - it 'does not cascade' do - expect(deleted.changes.keys).to eq([ 'person_id' ]) + it 'does not cascade' do + expect(deleted.changes.keys).to eq([ 'person_id' ]) + end end - end - context 'when the document is loaded' do - let!(:drug) { person.drugs.create! } - let!(:deleted) { person.drugs.delete(drug) } + context 'when the document is loaded' do + let!(:drug) { person.drugs.create! } + let!(:deleted) { person.drugs.send(method, drug) } - it 'returns the document' do - expect(deleted).to eq(drug) - end + it 'returns the document' do + expect(deleted).to eq(drug) + end - it 'deletes the foreign key' do - expect(drug.person_id).to be_nil - end + it 'deletes the foreign key' do + expect(drug.person_id).to be_nil + end - it 'removes the document from the association' do - expect(person.drugs).not_to include(drug) + it 'removes the document from the association' do + expect(person.drugs).not_to include(drug) + end end - end - context 'when the document is not loaded' do - let!(:drug) { Drug.create!(person_id: person.username) } - let!(:deleted) { person.drugs.delete(drug) } + context 'when the document is not loaded' do + let!(:drug) { Drug.create!(person_id: person.username) } + let!(:deleted) { person.drugs.send(method, drug) } - it 'returns the document' do - expect(deleted).to eq(drug) - end + it 'returns the document' do + expect(deleted).to eq(drug) + end - it 'deletes the foreign key' do - expect(drug.person_id).to be_nil - end + it 'deletes the foreign key' do + expect(drug.person_id).to be_nil + end - it 'removes the document from the association' do - expect(person.drugs).not_to include(drug) + it 'removes the document from the association' do + expect(person.drugs).not_to include(drug) + end end end - end - context 'when dependent is delete' do - context 'when the document is loaded' do - let!(:post) { person.posts.create!(title: 'test') } - let!(:deleted) { person.posts.delete(post) } + context 'when dependent is delete' do + context 'when the document is loaded' do + let!(:post) { person.posts.create!(title: 'test') } + let!(:deleted) { person.posts.send(method, post) } - it 'returns the document' do - expect(deleted).to eq(post) - end + it 'returns the document' do + expect(deleted).to eq(post) + end - it 'deletes the document' do - expect(post).to be_destroyed - end + it 'deletes the document' do + expect(post).to be_destroyed + end - it 'removes the document from the association' do - expect(person.posts).not_to include(post) + it 'removes the document from the association' do + expect(person.posts).not_to include(post) + end end - end - context 'when the document is not loaded' do - let!(:post) { Post.create!(title: 'foo', person_id: person.id) } - let!(:deleted) { person.posts.delete(post) } + context 'when the document is not loaded' do + let!(:post) { Post.create!(title: 'foo', person_id: person.id) } + let!(:deleted) { person.posts.send(method, post) } - it 'returns the document' do - expect(deleted).to eq(post) - end + it 'returns the document' do + expect(deleted).to eq(post) + end - it 'deletes the document' do - expect(post).to be_destroyed - end + it 'deletes the document' do + expect(post).to be_destroyed + end - it 'removes the document from the association' do - expect(person.posts).not_to include(post) + it 'removes the document from the association' do + expect(person.posts).not_to include(post) + end end end end - end - context 'when the document is not found' do - let!(:post) { Post.create!(title: 'foo') } - let!(:deleted) { person.posts.delete(post) } + context 'when the document is not found' do + let!(:post) { Post.create!(title: 'foo') } + let!(:deleted) { person.posts.send(method, post) } - it 'returns nil' do - expect(deleted).to be_nil - end + it 'returns nil' do + expect(deleted).to be_nil + end - it 'does not delete the document' do - expect(post).to be_persisted + it 'does not delete the document' do + expect(post).to be_persisted + end end end end diff --git a/spec/mongoid/attributes_spec.rb b/spec/mongoid/attributes_spec.rb index 2449782cef..18f99c26aa 100644 --- a/spec/mongoid/attributes_spec.rb +++ b/spec/mongoid/attributes_spec.rb @@ -2495,7 +2495,7 @@ end end - context "when doing delete_one" do + context "when doing _remove" do let(:doc) { NestedBook.create! } let(:page) { NestedPage.new } before do @@ -2503,7 +2503,7 @@ doc.pages << NestedPage.new doc.pages << NestedPage.new - doc.pages.send(:delete_one, page) + doc.pages._remove(page) end it "updates the attributes" do