Merge pull request #31405 from bogdanvlviv/fix-conflicts-counter_cach…

…e-with-touch-by-optimistic_locking Fix conflicts `counter_cache` with `touch: true` by optimistic locking.
rails · Dec 12, 2017 · 185a30f · 185a30f
2 parents 2eee8c3 + 6ddba9b
commit 185a30f
Show file tree

Hide file tree

Showing 6 changed files with 128 additions and 8 deletions.
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
@@ -1,3 +1,86 @@
+*   Fix conflicts `counter_cache` with `touch: true` by optimistic locking.
+
+    ```
+    # create_table :posts do |t|
+    #   t.integer :comments_count, default: 0
+    #   t.integer :lock_version
+    #   t.timestamps
+    # end
+    class Post < ApplicationRecord
+    end
+
+    # create_table :comments do |t|
+    #   t.belongs_to :post
+    # end
+    class Comment < ApplicationRecord
+      belongs_to :post, touch: true, counter_cache: true
+    end
+    ```
+
+    Before:
+    ```
+    post = Post.create!
+    # => begin transaction
+         INSERT INTO "posts" ("created_at", "updated_at", "lock_version")
+         VALUES ("2017-12-11 21:27:11.387397", "2017-12-11 21:27:11.387397", 0)
+         commit transaction
+
+    comment = Comment.create!(post: post)
+    # => begin transaction
+         INSERT INTO "comments" ("post_id") VALUES (1)
+
+         UPDATE "posts" SET "comments_count" = COALESCE("comments_count", 0) + 1,
+         "lock_version" = COALESCE("lock_version", 0) + 1 WHERE "posts"."id" = 1
+
+         UPDATE "posts" SET "updated_at" = '2017-12-11 21:27:11.398330',
+         "lock_version" = 1 WHERE "posts"."id" = 1 AND "posts"."lock_version" = 0
+         rollback transaction
+    # => ActiveRecord::StaleObjectError: Attempted to touch a stale object: Post.
+
+    Comment.take.destroy!
+    # => begin transaction
+         DELETE FROM "comments" WHERE "comments"."id" = 1
+
+         UPDATE "posts" SET "comments_count" = COALESCE("comments_count", 0) - 1,
+         "lock_version" = COALESCE("lock_version", 0) + 1 WHERE "posts"."id" = 1
+
+         UPDATE "posts" SET "updated_at" = '2017-12-11 21:42:47.785901',
+         "lock_version" = 1 WHERE "posts"."id" = 1 AND "posts"."lock_version" = 0
+         rollback transaction
+    # => ActiveRecord::StaleObjectError: Attempted to touch a stale object: Post.
+    ```
+
+    After:
+    ```
+    post = Post.create!
+    # => begin transaction
+         INSERT INTO "posts" ("created_at", "updated_at", "lock_version")
+         VALUES ("2017-12-11 21:27:11.387397", "2017-12-11 21:27:11.387397", 0)
+         commit transaction
+
+    comment = Comment.create!(post: post)
+    # => begin transaction
+         INSERT INTO "comments" ("post_id") VALUES (1)
+
+         UPDATE "posts" SET "comments_count" = COALESCE("comments_count", 0) + 1,
+         "lock_version" = COALESCE("lock_version", 0) + 1,
+         "updated_at" = '2017-12-11 21:37:09.802642' WHERE "posts"."id" = 1
+         commit transaction
+
+    comment.destroy!
+    # => begin transaction
+         DELETE FROM "comments" WHERE "comments"."id" = 1
+
+         UPDATE "posts" SET "comments_count" = COALESCE("comments_count", 0) - 1,
+         "lock_version" = COALESCE("lock_version", 0) + 1,
+         "updated_at" = '2017-12-11 21:39:02.685520' WHERE "posts"."id" = 1
+         commit transaction
+    ```
+
+    Fixes #31199.
+
+    *bogdanvlviv*
+
 *   Add support for PostgreSQL operator classes to `add_index`.
 
     Example:

diff --git a/activerecord/lib/active_record/associations/belongs_to_association.rb b/activerecord/lib/active_record/associations/belongs_to_association.rb
@@ -49,9 +49,9 @@ def increment_counters # :nodoc:
         def update_counters(by)
           if require_counter_update? && foreign_key_present?
             if target && !stale_target?
-              target.increment!(reflection.counter_cache_column, by)
+              target.increment!(reflection.counter_cache_column, by, touch: reflection.options[:touch])
             else
-              klass.update_counters(target_id, reflection.counter_cache_column => by)
+              klass.update_counters(target_id, reflection.counter_cache_column => by, touch: reflection.options[:touch])
             end
           end
         end

diff --git a/activerecord/lib/active_record/associations/builder/belongs_to.rb b/activerecord/lib/active_record/associations/builder/belongs_to.rb
@@ -114,9 +114,13 @@ def self.add_touch_callbacks(model, reflection)
         BelongsTo.touch_record(record, record.send(changes_method), foreign_key, n, touch, belongs_to_touch_method)
       }}
 
-      model.after_save    callback.(:saved_changes), if: :saved_changes?
-      model.after_touch   callback.(:changes_to_save)
-      model.after_destroy callback.(:changes_to_save)
+      unless reflection.counter_cache_column
+        model.after_create callback.(:saved_changes), if: :saved_changes?
+        model.after_destroy callback.(:changes_to_save)
+      end
+
+      model.after_update callback.(:saved_changes), if: :saved_changes?
+      model.after_touch callback.(:changes_to_save)
     end
 
     def self.add_default_callbacks(model, reflection)

diff --git a/activerecord/test/cases/locking_test.rb b/activerecord/test/cases/locking_test.rb
@@ -399,11 +399,43 @@ def test_update_without_attributes_does_not_only_update_lock_version
     end
   end
 
+  def test_counter_cache_with_touch_and_lock_version
+    car = Car.create!
+
+    assert_equal 0, car.wheels_count
+    assert_equal 0, car.lock_version
+
+    previously_car_updated_at = car.updated_at
+    travel(1.second) do
+      Wheel.create!(wheelable: car)
+    end
+
+    assert_equal 1, car.reload.wheels_count
+    assert_not_equal previously_car_updated_at, car.updated_at
+    assert_equal 1, car.lock_version
+
+    previously_car_updated_at = car.updated_at
+    car.wheels.first.update(size: 42)
+
+    assert_equal 1, car.reload.wheels_count
+    assert_not_equal previously_car_updated_at, car.updated_at
+    assert_equal 2, car.lock_version
+
+    previously_car_updated_at = car.updated_at
+    travel(1.second) do
+      car.wheels.first.destroy!
+    end
+
+    assert_equal 0, car.reload.wheels_count
+    assert_not_equal previously_car_updated_at, car.updated_at
+    assert_equal 3, car.lock_version
+  end
+
   def test_polymorphic_destroy_with_dependencies_and_lock_version
     car = Car.create!
 
     assert_difference "car.wheels.count"  do
-      car.wheels << Wheel.create!
+      car.wheels.create
     end
     assert_difference "car.wheels.count", -1  do
       car.reload.destroy

diff --git a/activerecord/test/models/wheel.rb b/activerecord/test/models/wheel.rb
@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 class Wheel < ActiveRecord::Base
-  belongs_to :wheelable, polymorphic: true, counter_cache: true
+  belongs_to :wheelable, polymorphic: true, counter_cache: true, touch: true
 end
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
@@ -123,7 +123,7 @@
   create_table :cars, force: true do |t|
     t.string  :name
     t.integer :engines_count
-    t.integer :wheels_count
+    t.integer :wheels_count, default: 0
     t.column :lock_version, :integer, null: false, default: 0
     t.timestamps null: false
   end
@@ -964,6 +964,7 @@
   end
 
   create_table :wheels, force: true do |t|
+    t.integer :size
     t.references :wheelable, polymorphic: true
   end