From 22a822e5813ef7ea9ab6dbbb670a363899a083af Mon Sep 17 00:00:00 2001 From: bogdanvlviv Date: Thu, 4 Aug 2016 03:36:30 +0300 Subject: Fixed: Optimistic locking does not work well with null in the database --- activerecord/CHANGELOG.md | 6 ++ .../lib/active_record/locking/optimistic.rb | 10 ++-- activerecord/test/cases/locking_test.rb | 66 ++++++++++++++++++---- activerecord/test/schema/schema.rb | 2 + 4 files changed, 69 insertions(+), 15 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 15b49e0a0b..db9c394a65 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +* Fixed: Optimistic locking does not work well with null in the database. + + Fixes #26024 + + *bogdanvlviv* + * Fixed support for case insensitive comparisons of `text` columns in PostgreSQL. diff --git a/activerecord/lib/active_record/locking/optimistic.rb b/activerecord/lib/active_record/locking/optimistic.rb index 8e8a97990a..fa9e789b24 100644 --- a/activerecord/lib/active_record/locking/optimistic.rb +++ b/activerecord/lib/active_record/locking/optimistic.rb @@ -60,6 +60,7 @@ module ActiveRecord end private + def increment_lock lock_col = self.class.locking_column previous_lock_value = send(lock_col).to_i @@ -80,7 +81,8 @@ module ActiveRecord 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) + increment_lock attribute_names += [lock_col] @@ -91,7 +93,7 @@ module ActiveRecord affected_rows = relation.where( self.class.primary_key => id, - lock_col => previous_lock_value, + lock_col => previous_lock_value ).update_all( attributes_for_update(attribute_names).map do |name| [name, _read_attribute(name)] @@ -104,9 +106,9 @@ module ActiveRecord affected_rows - # If something went wrong, revert the version. + # If something went wrong, revert the locking_column value. rescue Exception - send(lock_col + "=", previous_lock_value) + send(lock_col + "=", previous_lock_value.to_i) raise end end diff --git a/activerecord/test/cases/locking_test.rb b/activerecord/test/cases/locking_test.rb index 13b6f6daaf..8b6a969c9e 100644 --- a/activerecord/test/cases/locking_test.rb +++ b/activerecord/test/cases/locking_test.rb @@ -161,14 +161,6 @@ class OptimisticLockingTest < ActiveRecord::TestCase assert_equal(error.record.object_id, p2.object_id) end - def test_lock_new_with_nil - p1 = Person.new(first_name: "anika") - p1.save! - p1.lock_version = nil # simulate bad fixture or column with no default - p1.save! - assert_equal 1, p1.lock_version - end - def test_lock_new_when_explicitly_passing_nil p1 = Person.new(first_name: "anika", lock_version: nil) p1.save! @@ -222,22 +214,73 @@ class OptimisticLockingTest < ActiveRecord::TestCase def test_lock_without_default_sets_version_to_zero t1 = LockWithoutDefault.new + assert_equal 0, t1.lock_version + assert_nil t1.lock_version_before_type_cast + + t1.save! + t1.reload - t1.save - t1 = LockWithoutDefault.find(t1.id) assert_equal 0, t1.lock_version + assert_equal 0, t1.lock_version_before_type_cast + end + + def test_lock_without_default_should_work_with_null_in_the_database + ActiveRecord::Base.connection.execute("INSERT INTO lock_without_defaults(title) VALUES('title1')") + t1 = LockWithoutDefault.last + t2 = LockWithoutDefault.last + + assert_equal 0, t1.lock_version + assert_nil t1.lock_version_before_type_cast + assert_equal 0, t2.lock_version + assert_nil t2.lock_version_before_type_cast + + t1.title = "new title1" + t2.title = "new title2" + + assert_nothing_raised { t1.save! } + assert_equal 1, t1.lock_version + assert_equal "new title1", t1.title + + assert_raise(ActiveRecord::StaleObjectError) { t2.save! } + assert_equal 0, t2.lock_version + assert_equal "new title2", t2.title end def test_lock_with_custom_column_without_default_sets_version_to_zero t1 = LockWithCustomColumnWithoutDefault.new + assert_equal 0, t1.custom_lock_version assert_nil t1.custom_lock_version_before_type_cast t1.save! t1.reload + assert_equal 0, t1.custom_lock_version - assert [0, "0"].include?(t1.custom_lock_version_before_type_cast) + assert_equal 0, t1.custom_lock_version_before_type_cast + end + + def test_lock_with_custom_column_without_default_should_work_with_null_in_the_database + ActiveRecord::Base.connection.execute("INSERT INTO lock_without_defaults_cust(title) VALUES('title1')") + + t1 = LockWithCustomColumnWithoutDefault.last + t2 = LockWithCustomColumnWithoutDefault.last + + assert_equal 0, t1.custom_lock_version + assert_nil t1.custom_lock_version_before_type_cast + assert_equal 0, t2.custom_lock_version + assert_nil t2.custom_lock_version_before_type_cast + + t1.title = "new title1" + t2.title = "new title2" + + assert_nothing_raised { t1.save! } + assert_equal 1, t1.custom_lock_version + assert_equal "new title1", t1.title + + assert_raise(ActiveRecord::StaleObjectError) { t2.save! } + assert_equal 0, t2.custom_lock_version + assert_equal "new title2", t2.title end def test_readonly_attributes @@ -461,6 +504,7 @@ unless in_memory_db? end protected + def duel(zzz = 5) t0, t1, t2, t3 = nil, nil, nil, nil diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index d2fb090118..983ac076a9 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -436,10 +436,12 @@ ActiveRecord::Schema.define do end create_table :lock_without_defaults, force: true do |t| + t.column :title, :string t.column :lock_version, :integer end create_table :lock_without_defaults_cust, force: true do |t| + t.column :title, :string t.column :custom_lock_version, :integer end -- cgit v1.2.3 From a60a20bb7faae4d0758676b865f625275c500e4e Mon Sep 17 00:00:00 2001 From: bogdanvlviv Date: Mon, 22 Aug 2016 21:16:08 +0300 Subject: Added ability update locking_column value --- activerecord/CHANGELOG.md | 5 ++ .../lib/active_record/locking/optimistic.rb | 14 ++-- activerecord/test/cases/dirty_test.rb | 2 +- activerecord/test/cases/locking_test.rb | 76 ++++++++++++++++++++++ 4 files changed, 90 insertions(+), 7 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index db9c394a65..f847c71420 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,8 @@ +* Optimistic locking: Added ability update locking_column value. + Ignore optimistic locking if update with new locking_column value. + + *bogdanvlviv* + * Fixed: Optimistic locking does not work well with null in the database. Fixes #26024 diff --git a/activerecord/lib/active_record/locking/optimistic.rb b/activerecord/lib/active_record/locking/optimistic.rb index fa9e789b24..d39b7181f4 100644 --- a/activerecord/lib/active_record/locking/optimistic.rb +++ b/activerecord/lib/active_record/locking/optimistic.rb @@ -78,17 +78,19 @@ module ActiveRecord def _update_record(attribute_names = self.attribute_names) #:nodoc: return super unless locking_enabled? - return 0 if attribute_names.empty? lock_col = self.class.locking_column - previous_lock_value = read_attribute_before_type_cast(lock_col) - - increment_lock - attribute_names += [lock_col] - attribute_names.uniq! + return super if attribute_names.include?(lock_col) + return 0 if attribute_names.empty? begin + previous_lock_value = read_attribute_before_type_cast(lock_col) + + increment_lock + + attribute_names.push(lock_col) + relation = self.class.unscoped affected_rows = relation.where( diff --git a/activerecord/test/cases/dirty_test.rb b/activerecord/test/cases/dirty_test.rb index 09bd00291d..3bd8475bb0 100644 --- a/activerecord/test/cases/dirty_test.rb +++ b/activerecord/test/cases/dirty_test.rb @@ -341,7 +341,7 @@ class DirtyTest < ActiveRecord::TestCase def test_partial_update_with_optimistic_locking person = Person.new(first_name: "foo") - old_lock_version = 1 + old_lock_version = person.lock_version with_partial_writes Person, false do assert_queries(2) { 2.times { person.save! } } diff --git a/activerecord/test/cases/locking_test.rb b/activerecord/test/cases/locking_test.rb index 8b6a969c9e..0579df0a07 100644 --- a/activerecord/test/cases/locking_test.rb +++ b/activerecord/test/cases/locking_test.rb @@ -247,6 +247,44 @@ class OptimisticLockingTest < ActiveRecord::TestCase assert_equal "new title2", t2.title end + def test_lock_without_default_should_update_with_lock_col + t1 = LockWithoutDefault.create(title: "title1", lock_version: 6) + + assert_equal 6, t1.lock_version + + t1.update(lock_version: 0) + t1.reload + + assert_equal 0, t1.lock_version + end + + def test_lock_without_default_queries_count + t1 = LockWithoutDefault.create(title: "title1") + + assert_equal "title1", t1.title + assert_equal 0, t1.lock_version + + assert_queries(1) { t1.update(title: "title2") } + + t1.reload + assert_equal "title2", t1.title + assert_equal 1, t1.lock_version + + assert_queries(1) { t1.update(title: "title3", lock_version: 6) } + + t1.reload + assert_equal "title3", t1.title + assert_equal 6, t1.lock_version + + t2 = LockWithoutDefault.new(title: "title1") + + assert_queries(1) { t2.save! } + + t2.reload + assert_equal "title1", t2.title + assert_equal 0, t2.lock_version + end + def test_lock_with_custom_column_without_default_sets_version_to_zero t1 = LockWithCustomColumnWithoutDefault.new @@ -283,6 +321,44 @@ class OptimisticLockingTest < ActiveRecord::TestCase assert_equal "new title2", t2.title end + def test_lock_with_custom_column_without_default_should_update_with_lock_col + t1 = LockWithCustomColumnWithoutDefault.create(title: "title1", custom_lock_version: 6) + + assert_equal 6, t1.custom_lock_version + + t1.update(custom_lock_version: 0) + t1.reload + + assert_equal 0, t1.custom_lock_version + end + + def test_lock_with_custom_column_without_default_queries_count + t1 = LockWithCustomColumnWithoutDefault.create(title: "title1") + + assert_equal "title1", t1.title + assert_equal 0, t1.custom_lock_version + + assert_queries(1) { t1.update(title: "title2") } + + t1.reload + assert_equal "title2", t1.title + assert_equal 1, t1.custom_lock_version + + assert_queries(1) { t1.update(title: "title3", custom_lock_version: 6) } + + t1.reload + assert_equal "title3", t1.title + assert_equal 6, t1.custom_lock_version + + t2 = LockWithCustomColumnWithoutDefault.new(title: "title1") + + assert_queries(1) { t2.save! } + + t2.reload + assert_equal "title1", t2.title + assert_equal 0, t2.custom_lock_version + end + def test_readonly_attributes assert_equal Set.new([ "name" ]), ReadonlyNameShip.readonly_attributes -- cgit v1.2.3