after_commit on: :destroy hooks are broken in 2.1.0

[tamird]

2b0db84 broke after_commit on: :destroy callbacks because rails checks destroyed? after running a transaction to determine if it should run those callbacks: https://github.com/rails/rails/blob/4-2-stable/activerecord/lib/active_record/transactions.rb#L408

@huoxito @jhawthorn

This effectively makes this gem unusable starting with 2.1.0 :(

[radar]

Please provide a patch to fix this issue.

On 25 Feb 2015, at 19:18, Tamir Duberstein [email protected] wrote:

2b0db84 broke after_commit on: :destroy callbacks because rails checks destroyed? after running a transaction to determine if it should run those callbacks: https://github.com/rails/rails/blob/4-2-stable/activerecord/lib/active_record/transactions.rb#L408

@huoxito @jhawthorn

This makes effectively makes this gem unusable starting with 2.1.0 :(

—
Reply to this email directly or view it on GitHub.

[tamird]

git revert 2b0db84?

[radar]

What behaviour does that break though?

On 25 Feb 2015, at 21:30, Tamir Duberstein [email protected] wrote:

get revert 2b0db84?

—
Reply to this email directly or view it on GitHub.

[aaronjensen]

Curious about this too, we can't use 2.1.0 at the moment. It seems like rails has kind of painted us into a corner, if we revert 2b0db84, then you can't update a paranoia destroyed record. as mentioned in 2b0db84.

As it stands though, it breaks sunspot, because we're using after_commit on: :destroy to remove things from the index...

[jasoncox]

You could add to your model:

alias :destroyed? :paranoia_destroyed?

But keep in mind this will break update_column and update_columns the direct SQL insert methods which bypass validations and callbacks - you have other options for skipping these in rails.

This does not prevent you from updating a paranoia destroyed model.

@aaronjensen reverting 2b0db84 does not prevent updating a paranoia destroyed record only breaks update_column and update_columns.

[aaronjensen]

@jasoncox that has other problems (persisted? would need to be redefined too, it's a can of worms.)

This seems to fix the problem mentioned in this issue in a fairly clean way:

  # copied and modified from:  
  # https://github.com/rails/rails/blob/ec712fb4ca5289a22872eabf32c74459a304115d/activerecord/lib/active_record/transactions.rb#L410
  def transaction_include_any_action?(actions)
    actions.any? do |action|
      case action
      when :create
        transaction_record_state(:new_record)
      when :destroy
        paranoia_destroyed?
      when :update
        !(transaction_record_state(:new_record) || paranoia_destroyed?)
      end
    end
  end

[aaronjensen]

The fix above will fire after_commit :hook, on: :destroy callbacks whenever a paranoia destroyed model is updated. Maybe that's ok, i'm not sure. It works in our particular case, as it would be bad if after updates were fired for paranoia destroyed models and our destroy callbacks are idempotent.

[jasoncox]

@aaronjensen I agree, your fix is better/cleaner... I wonder what the solution is for the gem though, it's probably not great to monkey patch activerecord like this?

Perhaps we should write an after commit callback for paranoia?

[aaronjensen]

I'm not sure. It could just patch. It was patching other methods before.

By after commit callback for paranoia do you mean something like after_commit :whatever, on: :paranoia_destroy? That could be helpful, but it wouldn't solve my particular case with sunspot-rails which hardcodes the on: :destroy portion. I guess I'd just write my own callback that did the same thing that it did, so I could manually remove the item from the index on paranoia destroy. I'd also have to disable their after update callback as well since that would just add it back into the index.

Either way you shake it, it's messy.

The way it worked before wasn't really ever surprising, it behaved like a substitute for destroy exactly as I would have expected. The monkey patch restores part of that illusion, which is nice... but it has its own problems.

[radar]

Would a version of paranoia which doesn't enforce this scope by default, I.e. opt-in scope like Model.paranoid, fix these problems?

On 29 Mar 2015, at 15:46, Aaron Jensen [email protected] wrote:

I'm not sure. It could just patch. It was patching other methods before.

By after commit callback for paranoia do you mean something like after_commit :whatever, on: :paranoia_destroy? That could be helpful, but it wouldn't solve my particular case with sunspot-rails which hardcodes the on: :destroy portion. I guess I'd just write my own callback that did the same thing that it did, so I could manually remove the item from the index on paranoia destroy. I'd also have to disable their after update callback as well since that would just add it back into the index.

Either way you shake it, it's messy.

The way it worked before wasn't really ever surprising, it behaved like a substitute for destroy exactly as I would have expected. The monkey patch restores part of that illusion, which is nice... but it has its own problems.

—
Reply to this email directly or view it on GitHub.

[aaronjensen]

I don't think so, that'd introduce more problems for me.

What I really liked about paranoia was that I could drop it in and everything would be exactly the same as it was, except that I could recover a model or find it and inspect it after it was "destroyed".

2b0db84 has, unfortunately, broken that experience.

In upgrading to 2.1.0, I had to add the above monkey patch and I had to update several tests that were testing if something was destroyed to test if it was paranoia_destroyed. I'm less concerned about the work of updating the tests (I actually don't mind having to explicitly differentiate between something being destroyed and soft destroyed), the problem is that I don't know what other libraries or other places in rails that depend on destroyed? to work in the way I would expect it to work.

[aaronjensen]

The more I think about it, the more I think I'd rather just see 2b0db84 reverted and something else put into place to deal with #update_columns not working. For example:

def update_columns(*)
  @_updating_columns  = true
  super
ensure
  @_updating_columns = false
end

def destroyed?
  if @_updating_columns
    super
  else
    send(paranoia_column) != paranoia_sentinel_value
  end
end

[prashantdubey]

Is anyone actively working on fixing this problem anymore? I see PRs pending on Paranoia for months now with no single comment.

[radar]

Thank you for volunteering your time to do just that @prashantdubbey. I look forward to working with you :)

[jwg2s]

@radar definitely a 👍 on this - I'd be happy to look into. Do you have a general direction you'd like to go in order to resolve it that I should look into? Otherwise I'll just make it work and add a spec

[lukes]

Note, PR #228 is an attempt to fix this, but the conversation on that PR's stopped.

[Martin288]

I am using paranoia and sunspot together, and I now have to declare auto_remove_callback :after_destroy instead of :after_commit to remove index in solr. https://github.com/rubysherpas/paranoia#callbacks

[printercu]

What do you think about just adding deleted_at to previous_changes inside #delete (along with #update_columns) so it will be at least possible to run after_commit on: :update, if: -> { previous_changes.key?(:deleted_at) }. Will it break anything?

Maybe it'll be even possible to add this callback in gem and trigger after_commit on: :destroy (or :paranoia_destroy) and after_commit on: :restore from them.