aboutsummaryrefslogtreecommitdiffstats
path: root/activerecord
diff options
context:
space:
mode:
authorKasper Timm Hansen <kaspth@gmail.com>2019-07-19 03:41:05 +0200
committerKasper Timm Hansen <kaspth@gmail.com>2019-07-31 05:59:23 +0200
commitcea392eb9683fe5545c968817e039c72c69fd3f9 (patch)
tree9b0588208b54e5369b308ccc949a3e3ff1cd7f67 /activerecord
parent65dcc9d1597b5472ed59ea6c73686d48942f76f6 (diff)
downloadrails-cea392eb9683fe5545c968817e039c72c69fd3f9.tar.gz
rails-cea392eb9683fe5545c968817e039c72c69fd3f9.tar.bz2
rails-cea392eb9683fe5545c968817e039c72c69fd3f9.zip
Polymorphic has_one touch: Reset association cache result after create transaction
In case of a polymorphic association there's no automatic inverse_of to assign the inverse record. So to get the record there needs to be a query executed, however, if the query fires within the transaction that's trying to create the associated record, no record can be found. And worse, the nil result is cached on the association so after the transaction commits the record can't be found. That's what happens if touch is enabled on a polymorphic has_one association. Consider a Comment with a commentable association that needs to be touched. For `Comment.create(commentable: Post.new)`, the existing code essentially does `commentable.send(:comment)` within the create transaction for the comment and thus not finding the comment. Now we're purposefully clearing the cache in case we've tried accessing the association within the transaction and found no object. Before: ``` kaspth-imac 2.6.3 ~/code/rails/activerecord master *= ARCONN=postgresql bin/test test/cases/associations/has_one_associations_test.rb -n /commit/ Using postgresql Run options: -n /commit/ --seed 46022 D, [2019-07-19T03:30:37.864537 #96022] DEBUG -- : Chef Load (0.2ms) SELECT "chefs".* FROM "chefs" WHERE "chefs"."employable_id" = $1 AND "chefs"."employable_type" = $2 LIMIT $3 [["employable_id", 1], ["employable_type", "DrinkDesignerWithPolymorphicTouchChef"], ["LIMIT", 1]] D, [2019-07-19T03:30:37.865013 #96022] DEBUG -- : Chef Create (0.2ms) INSERT INTO "chefs" ("employable_id", "employable_type") VALUES ($1, $2) RETURNING "id" [["employable_id", 1], ["employable_type", "DrinkDesignerWithPolymorphicTouchChef"]] D, [2019-07-19T03:30:37.865201 #96022] DEBUG -- : TRANSACTION (0.1ms) RELEASE SAVEPOINT active_record_1 D, [2019-07-19T03:30:37.874136 #96022] DEBUG -- : TRANSACTION (0.1ms) ROLLBACK D, [2019-07-19T03:30:37.874323 #96022] DEBUG -- : TRANSACTION (0.1ms) ROLLBACK F Failure: HasOneAssociationsTest#test_polymorphic_has_one_with_touch_option_on_create_wont_cache_assocation_so_fetching_after_transaction_commit_works [/Users/kaspth/code/rails/activerecord/test/cases/associations/has_one_associations_test.rb:716]: --- expected +++ actual @@ -1 +1 @@ -#<Chef id: 1, employable_id: 1, employable_type: "DrinkDesignerWithPolymorphicTouchChef", department_id: nil, employable_list_type: nil, employable_list_id: nil> +nil ``` After: ``` kaspth-imac 2.6.3 ~/code/rails/activerecord master *= ARCONN=postgresql bin/test test/cases/associations/has_one_associations_test.rb -n /commit/ Using postgresql Run options: -n /commit/ --seed 46022 D, [2019-07-19T03:30:22.479387 #95973] DEBUG -- : Chef Create (0.3ms) INSERT INTO "chefs" ("employable_id", "employable_type") VALUES ($1, $2) RETURNING "id" [["employable_id", 1], ["employable_type", "DrinkDesignerWithPolymorphicTouchChef"]] D, [2019-07-19T03:30:22.479574 #95973] DEBUG -- : TRANSACTION (0.1ms) RELEASE SAVEPOINT active_record_1 D, [2019-07-19T03:30:22.482051 #95973] DEBUG -- : Chef Load (0.1ms) SELECT "chefs".* FROM "chefs" WHERE "chefs"."employable_id" = $1 AND "chefs"."employable_type" = $2 LIMIT $3 [["employable_id", 1], ["employable_type", "DrinkDesignerWithPolymorphicTouchChef"], ["LIMIT", 1]] D, [2019-07-19T03:30:22.482317 #95973] DEBUG -- : TRANSACTION (0.1ms) ROLLBACK D, [2019-07-19T03:30:22.482437 #95973] DEBUG -- : TRANSACTION (0.1ms) ROLLBACK . Finished in 0.088498s, 11.2997 runs/s, 22.5994 assertions/s. 1 runs, 2 assertions, 0 failures, 0 errors, 0 skips ``` Notice the select now fires after the commit.
Diffstat (limited to 'activerecord')
-rw-r--r--activerecord/lib/active_record/associations/association.rb4
-rw-r--r--activerecord/lib/active_record/associations/builder/has_one.rb19
-rw-r--r--activerecord/test/cases/associations/has_one_associations_test.rb18
-rw-r--r--activerecord/test/models/drink_designer.rb6
-rw-r--r--activerecord/test/schema/schema.rb2
5 files changed, 37 insertions, 12 deletions
diff --git a/activerecord/lib/active_record/associations/association.rb b/activerecord/lib/active_record/associations/association.rb
index cf22b850b9..705a5571ee 100644
--- a/activerecord/lib/active_record/associations/association.rb
+++ b/activerecord/lib/active_record/associations/association.rb
@@ -56,6 +56,10 @@ module ActiveRecord
@inversed = false
end
+ def reset_negative_cache # :nodoc:
+ reset if loaded? && target.nil?
+ end
+
# Reloads the \target and returns +self+ on success.
# The QueryCache is cleared if +force+ is true.
def reload(force = false)
diff --git a/activerecord/lib/active_record/associations/builder/has_one.rb b/activerecord/lib/active_record/associations/builder/has_one.rb
index 27ebe8cb71..db8393930e 100644
--- a/activerecord/lib/active_record/associations/builder/has_one.rb
+++ b/activerecord/lib/active_record/associations/builder/has_one.rb
@@ -32,15 +32,12 @@ module ActiveRecord::Associations::Builder # :nodoc:
end
end
- def self.touch_record(o, name, touch)
- record = o.send name
+ def self.touch_record(record, name, touch)
+ instance = record.send(name)
- return unless record && record.persisted?
-
- if touch != true
- record.touch(touch)
- else
- record.touch
+ if instance&.persisted?
+ touch != true ?
+ instance.touch(touch) : instance.touch
end
end
@@ -48,11 +45,9 @@ module ActiveRecord::Associations::Builder # :nodoc:
name = reflection.name
touch = reflection.options[:touch]
- callback = lambda { |record|
- HasOne.touch_record(record, name, touch)
- }
-
+ callback = -> (record) { HasOne.touch_record(record, name, touch) }
model.after_create callback, if: :saved_changes?
+ model.after_create_commit { association(name).reset_negative_cache }
model.after_update callback, if: :saved_changes?
model.after_destroy callback
model.after_touch callback
diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb
index 3ef25c7027..b1fcf47528 100644
--- a/activerecord/test/cases/associations/has_one_associations_test.rb
+++ b/activerecord/test/cases/associations/has_one_associations_test.rb
@@ -712,6 +712,24 @@ class HasOneAssociationsTest < ActiveRecord::TestCase
}
end
+ def test_polymorphic_has_one_with_touch_option_on_create_wont_cache_association_so_fetching_after_transaction_commit_works
+ assert_queries(4) {
+ chef = Chef.create(employable: DrinkDesignerWithPolymorphicTouchChef.new)
+ employable = chef.employable
+
+ assert_equal chef, employable.chef
+ }
+ end
+
+ def test_polymorphic_has_one_with_touch_option_on_update_will_touch_record_by_fetching_from_database_if_needed
+ DrinkDesignerWithPolymorphicTouchChef.create(chef: Chef.new)
+ designer = DrinkDesignerWithPolymorphicTouchChef.last
+
+ assert_queries(3) {
+ designer.update(name: "foo")
+ }
+ end
+
def test_has_one_with_touch_option_on_update
new_club = Club.create(name: "1000 Oaks")
new_club.create_membership
diff --git a/activerecord/test/models/drink_designer.rb b/activerecord/test/models/drink_designer.rb
index 8258408f35..fe7f5c9e03 100644
--- a/activerecord/test/models/drink_designer.rb
+++ b/activerecord/test/models/drink_designer.rb
@@ -10,5 +10,11 @@ class DrinkDesignerWithPolymorphicDependentNullifyChef < ActiveRecord::Base
has_one :chef, as: :employable, dependent: :nullify
end
+class DrinkDesignerWithPolymorphicTouchChef < ActiveRecord::Base
+ self.table_name = "drink_designers"
+
+ has_one :chef, as: :employable, touch: true
+end
+
class MocktailDesigner < DrinkDesigner
end
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index dd0ff759b6..cae2890c9e 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -1070,6 +1070,7 @@ ActiveRecord::Schema.define do
create_table :cake_designers, force: true do |t|
end
create_table :drink_designers, force: true do |t|
+ t.string :name
end
create_table :chefs, force: true do |t|
t.integer :employable_id
@@ -1077,6 +1078,7 @@ ActiveRecord::Schema.define do
t.integer :department_id
t.string :employable_list_type
t.integer :employable_list_id
+ t.timestamps
end
create_table :recipes, force: true do |t|
t.integer :chef_id