search

rubysherpas / paranoia

acts_as_paranoid for Rails 5, 6 and 7
Other
2.89k stars 529 forks source link

Paranoia does not preserve optimistic locking behaviour #242

Open elyzion opened 9 years ago

elyzion commented 9 years ago

Summary

Paranoia makes use of the touch method to update the deleted_at column. This breaks existing destroy behaviour as described in the API documentation at http://api.rubyonrails.org/classes/ActiveRecord/Locking/Optimistic.html.

Detail

The web based service I am working on uses optimistic locking to prevent a user from overwriting a record or deleting a record after is had been updating by another user. This worked fine while not using acts_as_paranoid. Enabling acts_as_paranoid resulted in the lock_version being ignored on destroy calls. I took a look at the source and it turned out the paranoia uses the touch method to update the deleted_at column.

It seems like changing the paranoia implementation to make use of an update call, and not a touch call, would be the best way to go about resolving this issue. Any suggestions?

radar commented 9 years ago

Would this be fixed by the changes in #245? /cc @Empact.

Empact commented 9 years ago

Don't think #245 would help - optimistic locking is implemented via _update_record, which is run on save but not touch or update_columns.

Empact commented 9 years ago

Ok I might see a fix - overriding destroy_row rather than destroy itself. Will take a look at implementing it as well.

elyzion commented 9 years ago

Thank you for taking the time to look into this @Empact !

Empact commented 9 years ago

I don't think there's an easy fix due to order of inclusion - I would recommend paranoia make use of "destroy_row" and then re-include other extension modules over itself - any module that extends a method defined in ActiveRecord::Persistence e.g. the counter cached associations code could just be re-included via ActiveRecord::CounterCache, once we've re-defined destroy_row.

There's some risk in this so I can't guarantee it would be successful, but it does mean paranoia has to do less reimplementation and can support more of the existing extensions in other libraries and in ActiveRecord itself.