Deprecate locking of dirty records #25873

Merged
merged 1 commit into from Feb 7, 2017

Projects

None yet

6 participants

@schuetzm
Contributor

lock! and with_lock reload the record before doing the actual locking. If there were any unsaved changes, they will be discarded without any warning. IMO this behaviour is dangerous and error prone.

See also #19743

@schneems schneems was assigned by rails-bot Jul 18, 2016
@rails-bot

r? @schneems

(@rails-bot has picked a reviewer for you, use r? to override)

@rafaelfranca
Member

Thank you for the pull request. What is the next step after this deprecation? When we remove the deprecation how the code will prevent this to happen again?

@schuetzm
Contributor

I'd suggest to raise an exception, because that's easy to implement.

But thinking about it, there is a way that would just prevent the record from being dirty in the first place:

MyModel.where(attr: 42).first_with_lock! do |record|
    # change record
    record.save!
end

On the other hand, this is more complicated, not as handy to use (you can't load the object, pass it to some other method, and lock it there), and might require major refactoring for the end users. So I'd still go with the exception.

@rafaelfranca rafaelfranca commented on an outdated diff Jul 20, 2016
activerecord/lib/active_record/locking/pessimistic.rb
@@ -59,7 +59,15 @@ module Pessimistic
# or pass true for "FOR UPDATE" (the default, an exclusive row lock). Returns
# the locked record.
def lock!(lock = true)
- reload(:lock => lock) if persisted?
+ if persisted?
+ if changed?
+ ActiveSupport::Deprecation.warn(<<-MSG.squish)
+ WARNING! Locking a record with unpersisted changes is deprecated. Use
@rafaelfranca
rafaelfranca Jul 20, 2016 Member

No need to WARNING! here.

@rafaelfranca
rafaelfranca Jul 20, 2016 Member

Locking a record with unpersisted changes is deprecated and will raise an exception in Rails 5.1. Use

@rafaelfranca rafaelfranca commented on an outdated diff Jul 20, 2016
activerecord/lib/active_record/locking/pessimistic.rb
@@ -59,7 +59,15 @@ module Pessimistic
# or pass true for "FOR UPDATE" (the default, an exclusive row lock). Returns
# the locked record.
def lock!(lock = true)
- reload(:lock => lock) if persisted?
+ if persisted?
+ if changed?
+ ActiveSupport::Deprecation.warn(<<-MSG.squish)
+ WARNING! Locking a record with unpersisted changes is deprecated. Use
+ `save` to persist the changes, or `reload` to discard them explicitly.
+ MSG
+ end
+ reload(:lock => lock)
@rafaelfranca
rafaelfranca Jul 20, 2016 Member

Use Ruby 1.9 hash syntax here.

@schuetzm
Contributor

@rafaelfranca I addressed your comments.

@schuetzm
Contributor

@rafaelfranca Rebased.

@rafaelfranca

Could you add a CHANGELOG entry?

+ if changed?
+ ActiveSupport::Deprecation.warn(<<-MSG.squish)
+ Locking a record with unpersisted changes is deprecated and will raise an
+ exception in Rails 5.1. Use `save` to persist the changes, or `reload` to
@schuetzm
Contributor

Added a changelog entry and changed the version in the deprecation notice to 5.2.

@schuetzm
Contributor
schuetzm commented Feb 7, 2017

@rafaelfranca Rebased again.

@@ -59,7 +59,16 @@ module Pessimistic
# or pass true for "FOR UPDATE" (the default, an exclusive row lock). Returns
# the locked record.
def lock!(lock = true)
- reload(lock: lock) if persisted?
+ if persisted?
+ if changed?
@rafaelfranca
rafaelfranca Feb 7, 2017 Member

Sorry I forgot to ask before but can you add a test for this deprecation?

@schuetzm
schuetzm Feb 7, 2017 Contributor

Done.

@schuetzm schuetzm Deprecate locking of dirty records
578f283
@rafaelfranca rafaelfranca merged commit 578f283 into rails:master Feb 7, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@schuetzm schuetzm deleted the schuetzm:warn_about_dirty_lock branch Feb 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment