westonganger / paper_trail-association_tracking

Plugin for the PaperTrail gem to track and reify associations
MIT License
128 stars 38 forks source link

Reifying has_one association with "dependent: :destroy" could destroy a live record #32

Closed bli closed 2 years ago

bli commented 2 years ago

To demonstrate the issue, just make a small change to an existing test case.

  1. Add line expect(widget0.reload.wotsit).not_to be_nil to the first test case in file has_one_spec.rb
  describe "widget, reified from a version prior to creation of wotsit" do
    it "has a nil wotsit" do
      widget = Widget.create(name: "widget_0")
      widget.update(name: "widget_1")
      widget.create_wotsit(name: "wotsit_0")
      widget0 = widget.versions.last.reify(has_one: true)
      expect(widget0.wotsit).to be_nil
      expect(widget0.reload.wotsit).not_to be_nil
    end
  end
  1. Run the test, it should still pass, which is correct.

  2. Add dependent: :destroy to has_one :wotsit in class Widget (spec/dummy_app/app/models/widget.rb)

class Widget < ActiveRecord::Base
  EXCLUDED_NAME = "Biglet"
  has_paper_trail
  has_one :wotsit, dependent: :destroy
  has_many(:fluxors, -> { order(:name) })
  has_many :whatchamajiggers, as: :owner
  validates :name, exclusion: { in: [EXCLUDED_NAME] }
end
  1. Now run the test above again, it should fail now
  1) PaperTrail widget, reified from a version prior to creation of wotsit has a nil wotsit
     Failure/Error: expect(widget0.reload.wotsit).not_to be_nil

       expected: not nil
            got: nil
     # ./spec/paper_trail/associations/has_one_spec.rb:17:in `block (3 levels) in <top (required)>'

Is this a known issue?

On top of my head, I thought of a simple fix by modifying method create_event in reifiers/has_one.rb , but not sure if it is a proper/complete one:

           if options[:mark_for_destruction]
             model.send(assoc.name).mark_for_destruction if model.send(assoc.name, true)
           else
-            model.paper_trail.appear_as_new_record do
-              model.send "#{assoc.name}=", nil
-            end
+            model.association(assoc.name).target = nil
           end
         end
westonganger commented 2 years ago

Thanks for reporting the issue. If you would like to create a PR for the issue please feel free to do so.

westonganger commented 2 years ago

This is now fixed in master via #38

westonganger commented 2 years ago

v2.2.1 is now released which contains the fix for this.