From 32d1bebd9dc51c3614022f4a939d113137f9831d Mon Sep 17 00:00:00 2001 From: Vlad Date: Fri, 8 May 2026 13:57:15 +0700 Subject: [PATCH 1/3] Don't soft-destroy nested records before save --- Gemfile | 1 + lib/mongoid/paranoia/monkey_patches.rb | 2 +- spec/mongoid/nested_attributes_spec.rb | 81 ++++++++++++++++++++++---- 3 files changed, 73 insertions(+), 11 deletions(-) diff --git a/Gemfile b/Gemfile index 998a9f9..b87624e 100644 --- a/Gemfile +++ b/Gemfile @@ -15,3 +15,4 @@ end gem 'rake' gem 'rspec' gem 'rubocop' +gem 'ostruct' diff --git a/lib/mongoid/paranoia/monkey_patches.rb b/lib/mongoid/paranoia/monkey_patches.rb index 76163c3..8532305 100644 --- a/lib/mongoid/paranoia/monkey_patches.rb +++ b/lib/mongoid/paranoia/monkey_patches.rb @@ -36,7 +36,7 @@ class Many # @since 3.0.10 def destroy(parent, relation, doc) doc.flagged_for_destroy = true - if !doc.embedded? || parent.new_record? || doc.paranoid? + if !doc.embedded? || parent.new_record? destroy_document(relation, doc) else parent.flagged_destroys.push(-> { destroy_document(relation, doc) }) diff --git a/spec/mongoid/nested_attributes_spec.rb b/spec/mongoid/nested_attributes_spec.rb index b6c07b6..7c92b01 100644 --- a/spec/mongoid/nested_attributes_spec.rb +++ b/spec/mongoid/nested_attributes_spec.rb @@ -68,24 +68,35 @@ } end - it "removes the first document from the relation" do - expect(persisted.paranoid_phones.size).to eq(2) + # The destroy is deferred until parent save (matching stock + # Mongoid behavior for non-paranoid embedded docs). Pre-save the + # in-memory collection still contains the doc flagged for destruction. + + it "flags the marked document for destruction" do + expect(phone_one.flagged_for_destroy?).to be true end - it "does not delete the unmarked document" do - expect(persisted.paranoid_phones.first.number).to eq("3") + it "does not soft-delete the marked document until save" do + expect(phone_one).not_to be_destroyed + expect(phone_one.reload.deleted_at).to be_nil end - it "adds the new document to the relation" do - expect(persisted.paranoid_phones.last.number).to eq("4") + it "keeps the marked document in the relation pending save" do + expect(persisted.paranoid_phones.size).to eq(3) end - it "has the proper persisted count" do - expect(persisted.paranoid_phones.count).to eq(1) + it "applies the update to the unmarked document" do + expect(persisted.paranoid_phones.find(phone_two.id).number).to eq("3") + end + + it "adds the new document to the relation" do + expect(persisted.paranoid_phones.last.number).to eq("4") end - it "soft deletes the removed document" do - expect(phone_one).to be_destroyed + it "counts only persisted (non-pending) docs" do + # phone_one and phone_two are persisted; the new phone is not + # persisted until parent save runs. + expect(persisted.paranoid_phones.count).to eq(2) end context "when saving the parent" do @@ -112,6 +123,56 @@ end end + context "regression: deferred destroy on parent validation failure" do + # Before the fix, assigning _destroy: true on a paranoid embedded doc + # immediately persisted a soft-delete via update_one, regardless of + # whether the parent's subsequent save succeeded. This left orphaned + # soft-deletes if the parent was rejected by validations or if save + # was never called (e.g. a read-only preview endpoint). + + before(:all) do + Person.send(:undef_method, :paranoid_phones_attributes=) + Person.accepts_nested_attributes_for :paranoid_phones, allow_destroy: true + end + + after(:all) do + Person.send(:undef_method, :paranoid_phones_attributes=) + Person.accepts_nested_attributes_for :paranoid_phones + end + + let!(:persisted) do + Person.create do |p| + p.paranoid_phones << ParanoidPhone.new(number: "1") + end + end + let(:phone) { persisted.paranoid_phones.first } + + it "does not soft-delete when assign_attributes is not followed by save" do + persisted.assign_attributes(paranoid_phones_attributes: [{ id: phone.id, _destroy: "1" }]) + expect(phone.reload.deleted_at).to be_nil + expect(persisted.reload.paranoid_phones.count).to eq(1) + end + + it "does not soft-delete when the parent save fails validation" do + invalid = Class.new(StandardError) + Person.validate { errors.add(:base, "nope") if @reject_save } + persisted.instance_variable_set(:@reject_save, true) + expect { + persisted.update_attributes!(paranoid_phones_attributes: [{ id: phone.id, _destroy: "1" }]) + }.to raise_error(Mongoid::Errors::Validations) + expect(phone.reload.deleted_at).to be_nil + expect(persisted.reload.paranoid_phones.count).to eq(1) + Person._validate_callbacks.clear + end + + it "soft-deletes when the parent save succeeds" do + persisted.update_attributes!(paranoid_phones_attributes: [{ id: phone.id, _destroy: "1" }]) + expect(phone.reload.deleted_at).not_to be_nil + expect(persisted.reload.paranoid_phones.count).to eq(0) + expect(persisted.reload.paranoid_phones.unscoped.count).to eq(1) + end + end + context "when the child has defaults" do before(:all) do From 3c42469fa5781397dbd8c16c123b1b0514d90a4d Mon Sep 17 00:00:00 2001 From: Vlad Date: Fri, 8 May 2026 22:43:36 +0700 Subject: [PATCH 2/3] Remove monkey patch for Mongoid::Association::Nested::Many as it's the same as mongoid code now --- lib/mongoid/paranoia/monkey_patches.rb | 30 -------------------------- 1 file changed, 30 deletions(-) diff --git a/lib/mongoid/paranoia/monkey_patches.rb b/lib/mongoid/paranoia/monkey_patches.rb index 8532305..2a398f2 100644 --- a/lib/mongoid/paranoia/monkey_patches.rb +++ b/lib/mongoid/paranoia/monkey_patches.rb @@ -17,36 +17,6 @@ module Document Mongoid::Document.include Mongoid::Paranoia::Document -module Mongoid - module Association - module Nested - class Many - # Destroy the child document, needs to do some checking for embedded - # relations and delay the destroy in case parent validation fails. - # - # @api private - # - # @example Destroy the child. - # builder.destroy(parent, relation, doc) - # - # @param [ Document ] parent The parent document. - # @param [ Proxy ] relation The relation proxy. - # @param [ Document ] doc The doc to destroy. - # - # @since 3.0.10 - def destroy(parent, relation, doc) - doc.flagged_for_destroy = true - if !doc.embedded? || parent.new_record? - destroy_document(relation, doc) - else - parent.flagged_destroys.push(-> { destroy_document(relation, doc) }) - end - end - end - end - end -end - module Mongoid module Association module Embedded From 2b3d4a0b5e2f8e71ec1a4353264fa751b2c951c0 Mon Sep 17 00:00:00 2001 From: Vlad Date: Mon, 11 May 2026 09:57:55 +0700 Subject: [PATCH 3/3] Fix regression on object_already_related? on objects with equality overrides --- Gemfile | 2 +- lib/mongoid/paranoia/monkey_patches.rb | 23 +++++ spec/mongoid/nested_attributes_spec.rb | 137 ++++++++++++++++--------- 3 files changed, 112 insertions(+), 50 deletions(-) diff --git a/Gemfile b/Gemfile index b87624e..9a0563d 100644 --- a/Gemfile +++ b/Gemfile @@ -12,7 +12,7 @@ else gem 'mongoid', version end +gem 'ostruct' gem 'rake' gem 'rspec' gem 'rubocop' -gem 'ostruct' diff --git a/lib/mongoid/paranoia/monkey_patches.rb b/lib/mongoid/paranoia/monkey_patches.rb index 2a398f2..0f83e0b 100644 --- a/lib/mongoid/paranoia/monkey_patches.rb +++ b/lib/mongoid/paranoia/monkey_patches.rb @@ -12,10 +12,33 @@ module Document class_attribute :paranoid end end + + # Skip paranoid docs flagged for destruction when checking whether a + # candidate is already related, so they do not block a new sibling with + # the same `==` key during a destroy-and-re-add nested attributes update. + # Non-paranoid candidates fall through to stock Mongoid behavior via + # `super`, so any future upstream fix is inherited automatically. + module EmbedsManyProxyExtensions + private + + # @example Check if a document is already related. + # relation.send(:object_already_related?, document) + # + # @param [ Document ] document The candidate document to check. + # + # @return [ true, false ] If a non-flagged sibling matches. + def object_already_related?(document) + return super unless document.paranoid? + # rubocop:disable Style/CaseEquality -- matches upstream Mongoid's dedup check + _target.any? {|existing| existing._id && !existing.flagged_for_destroy? && existing === document } + # rubocop:enable Style/CaseEquality + end + end end end Mongoid::Document.include Mongoid::Paranoia::Document +Mongoid::Association::Embedded::EmbedsMany::Proxy.prepend(Mongoid::Paranoia::EmbedsManyProxyExtensions) module Mongoid module Association diff --git a/spec/mongoid/nested_attributes_spec.rb b/spec/mongoid/nested_attributes_spec.rb index 7c92b01..09989c4 100644 --- a/spec/mongoid/nested_attributes_spec.rb +++ b/spec/mongoid/nested_attributes_spec.rb @@ -68,10 +68,6 @@ } end - # The destroy is deferred until parent save (matching stock - # Mongoid behavior for non-paranoid embedded docs). Pre-save the - # in-memory collection still contains the doc flagged for destruction. - it "flags the marked document for destruction" do expect(phone_one.flagged_for_destroy?).to be true end @@ -93,9 +89,7 @@ expect(persisted.paranoid_phones.last.number).to eq("4") end - it "counts only persisted (non-pending) docs" do - # phone_one and phone_two are persisted; the new phone is not - # persisted until parent save runs. + it "counts only persisted documents" do expect(persisted.paranoid_phones.count).to eq(2) end @@ -117,59 +111,104 @@ expect(persisted.reload.paranoid_phones.last.number).to eq("4") end end + + context "when saving the parent fails validation" do + + before do + Person.class_eval { validate { errors.add(:base, "nope") } } + persisted.save + end + + after do + Person._validate_callbacks.clear + end + + it "does not soft-delete the marked document" do + expect(phone_one.reload.deleted_at).to be_nil + end + + it "leaves the persisted collection intact" do + expect(persisted.reload.paranoid_phones.count).to eq(2) + end + + it "does not persist the new document" do + expect(persisted.reload.paranoid_phones.where(number: "4")).to be_empty + end + end end end end end - end - context "regression: deferred destroy on parent validation failure" do - # Before the fix, assigning _destroy: true on a paranoid embedded doc - # immediately persisted a soft-delete via update_one, regardless of - # whether the parent's subsequent save succeeded. This left orphaned - # soft-deletes if the parent was rejected by validations or if save - # was never called (e.g. a read-only preview endpoint). + context "when the child overrides equality" do - before(:all) do - Person.send(:undef_method, :paranoid_phones_attributes=) - Person.accepts_nested_attributes_for :paranoid_phones, allow_destroy: true - end - - after(:all) do - Person.send(:undef_method, :paranoid_phones_attributes=) - Person.accepts_nested_attributes_for :paranoid_phones - end + before(:all) do + ParanoidPhone.class_eval do + def ==(other) + other.is_a?(self.class) && number == other.number + end + alias_method :eql?, :== + end + end - let!(:persisted) do - Person.create do |p| - p.paranoid_phones << ParanoidPhone.new(number: "1") + after(:all) do + ParanoidPhone.send(:remove_method, :==) + ParanoidPhone.send(:remove_method, :eql?) end - end - let(:phone) { persisted.paranoid_phones.first } - it "does not soft-delete when assign_attributes is not followed by save" do - persisted.assign_attributes(paranoid_phones_attributes: [{ id: phone.id, _destroy: "1" }]) - expect(phone.reload.deleted_at).to be_nil - expect(persisted.reload.paranoid_phones.count).to eq(1) - end + context "when the parent is persisted" do - it "does not soft-delete when the parent save fails validation" do - invalid = Class.new(StandardError) - Person.validate { errors.add(:base, "nope") if @reject_save } - persisted.instance_variable_set(:@reject_save, true) - expect { - persisted.update_attributes!(paranoid_phones_attributes: [{ id: phone.id, _destroy: "1" }]) - }.to raise_error(Mongoid::Errors::Validations) - expect(phone.reload.deleted_at).to be_nil - expect(persisted.reload.paranoid_phones.count).to eq(1) - Person._validate_callbacks.clear - end + let!(:persisted) do + Person.create do |p| + p.paranoid_phones << [ phone_one, phone_two ] + end + end - it "soft-deletes when the parent save succeeds" do - persisted.update_attributes!(paranoid_phones_attributes: [{ id: phone.id, _destroy: "1" }]) - expect(phone.reload.deleted_at).not_to be_nil - expect(persisted.reload.paranoid_phones.count).to eq(0) - expect(persisted.reload.paranoid_phones.unscoped.count).to eq(1) + context "when destroying then re-adding a sibling with the same key" do + + before do + persisted.paranoid_phones_attributes = + { + "bar" => { "id" => phone_one.id, "_destroy" => "1" }, + "baz" => { "number" => "1" } + } + end + + it "keeps the new sibling in the relation" do + fresh = persisted.paranoid_phones.send(:_target).reject(&:flagged_for_destroy?) + expect(fresh.map(&:number)).to include("1") + end + + context "when saving the parent" do + + before do + persisted.save + end + + it "soft-deletes the original sibling" do + expect(phone_one.reload.deleted_at).not_to be_nil + end + + it "persists the new sibling" do + reloaded = persisted.reload.paranoid_phones + expect(reloaded.map(&:number)).to contain_exactly("1", "2") + expect(reloaded.where(number: "1").first.id).not_to eq(phone_one.id) + end + end + end + + context "when pushing a duplicate of a live sibling" do + + before do + persisted.paranoid_phones.push(ParanoidPhone.new(number: "1")) + end + + it "does not add the duplicate to the relation" do + target = persisted.paranoid_phones.send(:_target) + expect(target.count {|p| p.number == "1" }).to eq(1) + end + end + end end end