aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorbogdanvlviv <bogdanvlviv@gmail.com>2017-12-11 23:56:07 +0200
committerbogdanvlviv <bogdanvlviv@gmail.com>2017-12-12 00:32:50 +0200
commit6ddba9b46c1d307ea7e03d753d0988183b9cb80c (patch)
tree3317877c34bc72aa6711d6a0419c9ad6290d041a
parent02e058a2f26097d095061567e597a7aaee4d5c9e (diff)
downloadrails-6ddba9b46c1d307ea7e03d753d0988183b9cb80c.tar.gz
rails-6ddba9b46c1d307ea7e03d753d0988183b9cb80c.tar.bz2
rails-6ddba9b46c1d307ea7e03d753d0988183b9cb80c.zip
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.
-rw-r--r--activerecord/CHANGELOG.md83
-rw-r--r--activerecord/lib/active_record/associations/belongs_to_association.rb4
-rw-r--r--activerecord/lib/active_record/associations/builder/belongs_to.rb10
-rw-r--r--activerecord/test/cases/locking_test.rb34
-rw-r--r--activerecord/test/models/wheel.rb2
-rw-r--r--activerecord/test/schema/schema.rb3
6 files changed, 128 insertions, 8 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 89a12d4223..ceef240257 100644
--- 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
index ba54cd8f49..68c608df13 100644
--- a/activerecord/lib/active_record/associations/belongs_to_association.rb
+++ b/activerecord/lib/active_record/associations/belongs_to_association.rb
@@ -49,9 +49,9 @@ module ActiveRecord
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
index 9904ee4bed..c161454c1a 100644
--- a/activerecord/lib/active_record/associations/builder/belongs_to.rb
+++ b/activerecord/lib/active_record/associations/builder/belongs_to.rb
@@ -114,9 +114,13 @@ module ActiveRecord::Associations::Builder # :nodoc:
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
index e857180bd1..437a5a38a3 100644
--- a/activerecord/test/cases/locking_test.rb
+++ b/activerecord/test/cases/locking_test.rb
@@ -399,11 +399,43 @@ class OptimisticLockingTest < ActiveRecord::TestCase
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
index e05fb64477..8db57d181e 100644
--- 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
index bf66846840..3205c4c20a 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -123,7 +123,7 @@ ActiveRecord::Schema.define do
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 @@ ActiveRecord::Schema.define do
end
create_table :wheels, force: true do |t|
+ t.integer :size
t.references :wheelable, polymorphic: true
end