When reviving dependent associations, query is not scoped to parent record

[masonhale]

I was just debugging an issue when trying to revive a deleted record, and I found that that when reviving the dependent associations for the record, the query is not scoped to the parent record. By that I mean the query does not include the foreign key. Rather it merely looks for any record of the given type that happened to be deleted within the timeframe of the deleted_at (plus/minus the depended_record_window).

The result is that completely unrelated records could be revived if they happened to be deleted around the same time (something quite likely on a high traffic app). Surely this isn't the way it is meant to work is it?

My expectation is that query generated by add_record_window method would include the foreign_key of the containing record. See: https://github.com/JackDanger/permanent_records/blob/master/lib/permanent_records.rb#L134-L144

I can try to create a patch for this issue, but first I would like to know if this is working 'as designed' if there is not reason the foreign key is not included?

[JackDanger]

That's definitely not the way it was intended! If you could put together a failing tests case for this all of the project members here would be much obliged, I'm sure. Thank you, @masonhale!

[masonhale]

@JackDanger I opened a PR #76 with a test spec that fails in the 3.2.0 release against activerecord 3.2 and activerecord 4.2. (The app I'm working with is still Rails 3.2).

However the spec does not fail against the master branch.

I see the master branch gemspec requires activerecord >= 4.2.0 so I am unable to test against activerecord 3.2.

Is the 3.2.0 release the last one expected to support activerecord 3.2?
When looking at releases it looks like it is the current official release, no? https://github.com/JackDanger/permanent_records/releases

[JackDanger]

The official releases are the ones on Rubygems.org: https://rubygems.org/gems/permanent_records

At some point the ActiveRecord source changed so significantly that we stopped supporting 3.x. So it looks like what you've found is a great candidate for backporting. Maybe start at ad58da2 (the last commit of the 3.3.x releases of permanent_records) and apply your test and your fix there? Then we can issue a 3.3.1 backported fix with your changes that everybody can use on Rails 3.x.

git checkout -b 3-x-dependent-revival
git reset --hard ad58da297b8d60911bae5a063e3f7259a05951ca
git cherry-pick aa14c02d103123ad5f1de5ff1bec6dec98a6ec97
git cherry-pick d0def7f917124eceb8e558fa89b64e0c1f87bffc

Thanks for digging into this! My apps at Square have all been upgraded to 4.x (thank goodness) so I hadn't noticed this problem.

[masonhale]

I pulled down ad58da2 and merged the test spec I wrote into it, but I'm getting lots and lots of test failures related to 'destroy' returning false in some instances.

A sample of the error I see (again and again)

  1) PermanentRecords revive dependent associations, and only dependent associations should revive muskrats of revived hole
     Failure/Error: hole_a.destroy
     NoMethodError:
       undefined method `attributes' for false:FalseClass
     # ./lib/permanent_records.rb:92:in `destroy_with_permanent_records'
     # ./lib/permanent_records.rb:47:in `block in destroy'
     # ./lib/permanent_records.rb:43:in `destroy'
     # ./spec/permanent_records/dependent_revival_spec.rb:17:in `block (2 levels) in <top (required)>'

...

  3) PermanentRecords#destroy returns the record
     Failure/Error: subject { record.destroy should_force }
     NoMethodError:
       undefined method `attributes' for false:FalseClass
     # ./lib/permanent_records.rb:92:in `destroy_with_permanent_records'
     # ./lib/permanent_records.rb:47:in `block in destroy'
     # ./lib/permanent_records.rb:43:in `destroy'
     # ./spec/permanent_records_spec.rb:22:in `block (3 levels) in <top (required)>'
     # ./spec/permanent_records_spec.rb:25:in `block (3 levels) in <top (required)>'

I checked and loads of tests fail in ad58da2 before I merge my changes too (so my changes did not trigger the error).

Rather than me tracking down the fix for this new error, I'm hoping you know the cause and hopefully the commit that fixes it?

Are you sure this is the last commit before the 3.3.0 release?

[JackDanger]

I do know the source of that error. I'll look at it tomorrow. Thanks for
working through this!

::Jack

On Mon, Jun 6, 2016 at 5:11 PM, Mason Hale [email protected] wrote:

I pulled down ad58da2
ad58da2
and merged the test spec I wrote into it, but I'm getting lots and lots of
test failures related to 'destroy' returning false in some instances.

A sample of the error I see (again and again)

1. PermanentRecords revive dependent associations, and only dependent associations should revive muskrats of revived hole
   Failure/Error: hole_a.destroy
   NoMethodError:
   undefined method `attributes' for false:FalseClass

   ./lib/permanent_records.rb:92:in`destroy_with_permanent_records'

   ./lib/permanent_records.rb:47:in `block in destroy'

   ./lib/permanent_records.rb:43:in`destroy'

   ./spec/permanent_records/dependent_revival_spec.rb:17:in `block (2 levels) in <top (required)>'

...

3. PermanentRecords#destroy returns the record
   Failure/Error: subject { record.destroy should_force }
   NoMethodError:
   undefined method `attributes' for false:FalseClass

   ./lib/permanent_records.rb:92:in`destroy_with_permanent_records'

   ./lib/permanent_records.rb:47:in `block in destroy'

   ./lib/permanent_records.rb:43:in`destroy'

   ./spec/permanent_records_spec.rb:22:in `block (3 levels) in <top (required)>'

   ./spec/permanent_records_spec.rb:25:in`block (3 levels) in <top (required)>'

I checked and loads of tests fail in ad58da2
ad58da2
before I merge my changes too (so my changes did not trigger the error).

Rather than me tracking down the fix for this new error, I'm hoping you
know the cause and hopefully the commit that fixes it?

Are you sure this is the last commit before the 3.3.0 release?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#75 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAAIF4SN9KQYvNoATgTYrp-T6y-UR5Tlks5qJLctgaJpZM4IvID4
.

[JackDanger]

On your branch you'll probably need to remove references to Rails 4+ in ./ci and .travis.yaml because this error occurs on modern versions of AR. Run bundle exec rspec -bc to be certain which version of the gem you're running against.

PermanentRecords has added a fix for the modern ActiveRecord API in a20e0f0 but I think you may not need that if you're building a fix for 3.x users.