Fix decrementing counter caches for parent records using optimistic locking

activerecord/lib/active_record/locking/optimistic.rb

@@ -97,7 +97,6 @@ def _update_row(attribute_names, attempted_action = "update")
             lock_attribute_was = @attributes[locking_column]
 
             update_constraints = _query_constraints_hash
-            update_constraints[locking_column] = _lock_value_for_database(locking_column)
 
             attribute_names = attribute_names.dup if attribute_names.frozen?
             attribute_names << locking_column
@@ -123,16 +122,9 @@ def _update_row(attribute_names, attempted_action = "update")
         end
 
         def destroy_row
-          return super unless locking_enabled?
+          affected_rows = super
 
-          locking_column = self.class.locking_column
-
-          delete_constraints = _query_constraints_hash
-          delete_constraints[locking_column] = _lock_value_for_database(locking_column)
-
-          affected_rows = self.class._delete_record(delete_constraints)
-
-          if affected_rows != 1
+          if locking_enabled? && affected_rows != 1
             raise ActiveRecord::StaleObjectError.new(self, "destroy")
           end
 
@@ -152,6 +144,13 @@ def _clear_locking_column
           clear_attribute_change(self.class.locking_column)
         end
 
+        def _query_constraints_hash
+          return super unless locking_enabled?
+
+          locking_column = self.class.locking_column
+          super.merge(locking_column => _lock_value_for_database(locking_column))
+        end
+
         module ClassMethods
           DEFAULT_LOCKING_COLUMN = "lock_version"
 

activerecord/test/cases/counter_cache_test.rb

@@ -2,6 +2,7 @@
 
 require "cases/helper"
 require "models/topic"
+require "models/bulb"
 require "models/car"
 require "models/aircraft"
 require "models/wheel"
@@ -248,6 +249,15 @@ class ::SpecialReply < ::Reply
     end
   end
 
+  test "removing association updates counter" do
+    michael = people(:michael)
+    car = cars(:honda)
+
+    assert_difference -> { michael.reload.cars_count }, -1 do
+      car.destroy
+    end
+  end
+
   test "update counters doesn't touch timestamps by default" do
     @topic.update_column :updated_at, 5.minutes.ago
     previously_updated_at = @topic.updated_at

activerecord/test/fixtures/cars.yml

@@ -2,8 +2,10 @@ honda:
   id: 1
   name: honda
   engines_count: 0
+  person_id: 1
 
 zyke:
   id: 2
   name: zyke
   engines_count: 0
+  person_id: 2

activerecord/test/fixtures/people.yml

@@ -6,6 +6,7 @@ michael:
   gender: M
   followers_count: 1
   friends_too_count: 1
+  cars_count: 1
 david:
   id: 2
   first_name: David
@@ -14,6 +15,7 @@ david:
   gender: M
   followers_count: 1
   friends_too_count: 1
+  cars_count: 1
 susan:
   id: 3
   first_name: Susan

activerecord/test/models/car.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 class Car < ActiveRecord::Base
+  belongs_to :person, counter_cache: true
   has_many :bulbs
   has_many :all_bulbs, -> { unscope(where: :name) }, class_name: "Bulb"
   has_many :all_bulbs2, -> { unscope(:where) }, class_name: "Bulb"

activerecord/test/schema/schema.rb

@@ -191,6 +191,7 @@
   end
 
   create_table :cars, force: true do |t|
+    t.belongs_to :person
     t.string  :name
     t.integer :engines_count
     t.integer :wheels_count, default: 0, null: false
@@ -910,6 +911,7 @@
     t.references :best_friend_of
     t.integer    :insures, null: false, default: 0
     t.timestamp :born_at
+    t.integer :cars_count, default: 0
     t.timestamps null: false
   end