From 263f01d93da118dc150c6ac816e70dcf10de2608 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Mon, 5 Mar 2018 04:14:51 +0900 Subject: Fix that after commit callbacks on update does not triggered when optimistic locking is enabled This issue is caused by `@_trigger_update_callback` won't be updated due to `_update_record` in `Locking::Optimistic` doesn't call `super` when optimistic locking is enabled. Now optimistic locking concern when updating is supported by `_update_row` low level API, therefore overriding `_update_record` is no longer necessary. Removing the method just fix the issue. Closes #29096. Closes #29321. Closes #30823. --- .../lib/active_record/locking/optimistic.rb | 49 +++------------------- activerecord/lib/active_record/persistence.rb | 24 ++++++----- .../test/cases/transaction_callbacks_test.rb | 22 ++++++++++ 3 files changed, 42 insertions(+), 53 deletions(-) diff --git a/activerecord/lib/active_record/locking/optimistic.rb b/activerecord/lib/active_record/locking/optimistic.rb index af361a8d2d..7f096bb532 100644 --- a/activerecord/lib/active_record/locking/optimistic.rb +++ b/activerecord/lib/active_record/locking/optimistic.rb @@ -61,13 +61,6 @@ module ActiveRecord end private - - def increment_lock - lock_col = self.class.locking_column - previous_lock_value = send(lock_col) - send("#{lock_col}=", previous_lock_value + 1) - end - def _create_record(attribute_names = self.attribute_names, *) if locking_enabled? # We always want to persist the locking version, even if we don't detect @@ -77,41 +70,13 @@ module ActiveRecord super end - def _update_record(attribute_names = self.attribute_names) - attribute_names &= self.class.column_names - return super unless locking_enabled? - return 0 if attribute_names.empty? - - begin - lock_col = self.class.locking_column - - previous_lock_value = read_attribute_before_type_cast(lock_col) - - increment_lock - - attribute_names.push(lock_col) - - affected_rows = self.class._update_record( - attributes_with_values_for_update(attribute_names), - self.class.primary_key => id_in_database, - lock_col => previous_lock_value - ) - - unless affected_rows == 1 - raise ActiveRecord::StaleObjectError.new(self, "update") - end - - affected_rows - - # If something went wrong, revert the locking_column value. - rescue Exception - send("#{lock_col}=", previous_lock_value.to_i) - - raise - end + def _touch_row(attribute_names, time) + super + ensure + clear_attribute_change(self.class.locking_column) if locking_enabled? end - def _update_row(attribute_names, attempted_action) + def _update_row(attribute_names, attempted_action = "update") return super unless locking_enabled? begin @@ -135,10 +100,8 @@ module ActiveRecord # If something went wrong, revert the locking_column value. rescue Exception - self[locking_column] = previous_lock_value + self[locking_column] = previous_lock_value.to_i raise - ensure - clear_attribute_change(locking_column) end end diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index ce16587326..d4f4a5887a 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -656,18 +656,11 @@ module ActiveRecord MSG end - time ||= current_time_from_proper_timezone attribute_names = timestamp_attributes_for_update_in_model attribute_names.concat(names) unless attribute_names.empty? - attribute_names.each do |attr_name| - write_attribute(attr_name, time) - clear_attribute_change(attr_name) - end - - affected_rows = _update_row(attribute_names, "touch") - + affected_rows = _touch_row(attribute_names, time) @_trigger_update_callback = affected_rows == 1 else true @@ -688,7 +681,18 @@ module ActiveRecord self.class._delete_record(self.class.primary_key => id_in_database) end - def _update_row(attribute_names, attempted_action) + def _touch_row(attribute_names, time) + time ||= current_time_from_proper_timezone + + attribute_names.each do |attr_name| + write_attribute(attr_name, time) + clear_attribute_change(attr_name) + end + + _update_row(attribute_names, "touch") + end + + def _update_row(attribute_names, attempted_action = "update") self.class._update_record( attributes_with_values(attribute_names), self.class.primary_key => id_in_database @@ -712,7 +716,7 @@ module ActiveRecord affected_rows = 0 @_trigger_update_callback = true else - affected_rows = _update_row(attribute_names, "update") + affected_rows = _update_row(attribute_names) @_trigger_update_callback = affected_rows == 1 end diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb index 1c7cec4dad..e89ac53732 100644 --- a/activerecord/test/cases/transaction_callbacks_test.rb +++ b/activerecord/test/cases/transaction_callbacks_test.rb @@ -394,6 +394,28 @@ class TransactionCallbacksTest < ActiveRecord::TestCase end end +class TransactionAfterCommitCallbacksWithOptimisticLockingTest < ActiveRecord::TestCase + class PersonWithCallbacks < ActiveRecord::Base + self.table_name = :people + + after_create_commit { |record| record.history << :commit_on_create } + after_update_commit { |record| record.history << :commit_on_update } + after_destroy_commit { |record| record.history << :commit_on_destroy } + + def history + @history ||= [] + end + end + + def test_after_commit_callbacks_with_optimistic_locking + person = PersonWithCallbacks.create!(first_name: "first name") + person.update!(first_name: "another name") + person.destroy + + assert_equal [:commit_on_create, :commit_on_update, :commit_on_destroy], person.history + end +end + class CallbacksOnMultipleActionsTest < ActiveRecord::TestCase self.use_transactional_tests = false -- cgit v1.2.3