Fixed: Optimistic locking does not work correctly #26050

Merged
merged 2 commits into from Oct 21, 2016

Projects

None yet

6 participants

@bogdanvlviv
Contributor
bogdanvlviv commented Aug 4, 2016 edited

Fixed: Optimistic locking does not work well with null in the database.

Added ability update locking_column value.
Ignore optimistic locking if update with new locking column value.

Fix for #26024

@rails-bot

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@bogdanvlviv bogdanvlviv changed the title from Fixed: Optimistic locking does not work well with null in the database #26024 to Fixed: Optimistic locking does not work well with null in the database Aug 4, 2016
@maclover7
Member

Can you add a regression test? Thanks!

@vsizov
vsizov commented Aug 4, 2016

YAY, Thanks @bogdanvlviv

@vsizov
vsizov commented Aug 5, 2016

@maclover7 I suppose the tests have been added!?

@bogdanvlviv
Contributor

Should i also backport it to 4-2-stable and 5-0-stable?

@vsizov
vsizov commented Aug 19, 2016

Any idea on when it will be merged?

@vsizov
vsizov commented Aug 22, 2016

There are two bugs in this functionality (rails master and 4.2.*) I prepared two test cases

require 'active_record'
require 'minitest/autorun'
require 'sqlite3'
require 'logger'

ActiveRecord::Base.establish_connection 'sqlite3::memory:'

ActiveRecord::Schema.define do
  create_table :posts do |t|
    t.string :name
    t.integer :lock_version
  end
end

class Post < ActiveRecord::Base
end

class BugTest < Minitest::Test
  def test_lock_version_without_default_value
    #Post.create!(name: 'First')
    ActiveRecord::Base.connection.execute("insert into posts(name) VALUES('Mark')")

    p1 = Post.find(1)
    p1.name = "Michael"
    p1.save
  end

  def test_lock_version_without_default_value_when_form_submit
    ActiveRecord::Base.connection.execute("insert into posts(name) VALUES('Mark')")

    p1 = Post.find(1)
    p1.update(name: "Bob", lock_version: "0")
    p1.save
  end
end

This PR fixes first one. Anyway this PR can be considered as a fix and merged separately

@vsizov
vsizov commented Aug 22, 2016 edited

The problem is that read_attribute_before_type_cast(lock_col) returns '0' for case I mentioned above while database has NULL value

@vsizov vsizov commented on an outdated diff Aug 22, 2016
activerecord/lib/active_record/locking/optimistic.rb
@@ -80,7 +80,8 @@ def _update_record(attribute_names = self.attribute_names) #:nodoc:
return 0 if attribute_names.empty?
lock_col = self.class.locking_column
- previous_lock_value = send(lock_col).to_i
+ previous_lock_value = read_attribute_before_type_cast(lock_col)
@vsizov
vsizov Aug 22, 2016

I left this line, but added after it following line:

previous_lock_value = nil if previous_lock_value == '0'

and removed (overwrote) _create_record method above. But in this case we never need to set default value. Just consider it as a possible fix.

@bogdanvlviv
Contributor

@vsizov thanks. I added some changes to fix it.

@bogdanvlviv
Contributor

@rafaelfranca, this pull request needs your feedback.

@vsizov
vsizov commented Aug 23, 2016

@bogdanvlviv The current implementation presupposes performing two SQL queries. I think there should be another solution.

@bogdanvlviv
Contributor
bogdanvlviv commented Aug 23, 2016 edited

@vsizov are you about?

relation = relation.where(self.class.primary_key => id)
relation = relation.where(lock_col => previous_lock_value) unless update_with_lock_col

In logs i see only one SQL query when running line with

relation.update_all
# UPDATE "posts" SET "title" = 'new title', "lock_version" = 1 WHERE "posts"."id" = ? AND "posts"."lock_version" IS NULL

i believe in it, but I have to make sure.

@vsizov
vsizov commented Aug 23, 2016 edited

@bogdan Ah, yes, lazy load.... Sorry than

@vsizov
vsizov commented Aug 23, 2016

Thanks @bogdanvlviv I'm looking forward when it's merged

@bogdanvlviv
Contributor
bogdanvlviv commented Aug 23, 2016 edited

@vsizov thanks! I added tests to make sure that executes only one SQL query. Also i changed current implementation #_update_record.

@bogdanvlviv bogdanvlviv changed the title from Fixed: Optimistic locking does not work well with null in the database to Fixed: Optimistic locking does not work well Aug 23, 2016
@bogdanvlviv bogdanvlviv changed the title from Fixed: Optimistic locking does not work well to Fixed: Optimistic locking does not work correctly Aug 23, 2016
@bogdanvlviv
Contributor

Please review.

@tenderlove
Member

@bogdanvlviv I think this is OK. Can you rebase and I'll merge?

bogdanvlviv added some commits Aug 4, 2016
@bogdanvlviv bogdanvlviv Fixed: Optimistic locking does not work well with null in the database 22a822e
@bogdanvlviv bogdanvlviv Added ability update locking_column value
a60a20b
@tenderlove tenderlove merged commit 0aed0bb into rails:master Oct 21, 2016

0 of 2 checks passed

codeclimate Code Climate is analyzing this code.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@bogdanvlviv
Contributor

@tenderlove Thanks! Should i also backport it to 4-2-stable?

@tenderlove
Member

@bogdanvlviv I am slightly afraid of backporting. This feature of AR is something that we've historically had problems with, so I'd rather not put the stable branches at risk.

@bogdanvlviv
Contributor

@tenderlove Got it. Thanks!

@bogdanvlviv bogdanvlviv deleted the bogdanvlviv:optimistic_locking branch Oct 21, 2016
@bogdanvlviv bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Oct 23, 2016
@bogdanvlviv bogdanvlviv Add info about updating locking column value
[ci skip]
Follow #26050
99c305e
@bogdanvlviv bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Oct 23, 2016
@bogdanvlviv bogdanvlviv Add info about updating locking column value
[ci skip]
Follow #26050
2bb74c9
@bogdanvlviv bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Nov 10, 2016
@bogdanvlviv bogdanvlviv Update guides/source/active_record_querying.md
Add info about updating locking column value

Follow #26050 and #26871

[ci skip]
25eed41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment