From 744e37a8efeed3a93cb8dd515f06a21a289de6f3 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Tue, 13 Feb 2018 15:45:59 -0800 Subject: [PATCH 1/7] Add #create_or_find_by to lean on unique constraints --- activerecord/lib/active_record/querying.rb | 2 +- activerecord/lib/active_record/relation.rb | 48 ++++++++++++++++------ activerecord/test/cases/relations_test.rb | 9 ++++ 3 files changed, 46 insertions(+), 13 deletions(-) diff --git a/activerecord/lib/active_record/querying.rb b/activerecord/lib/active_record/querying.rb index 3996d5661fc5a..7c8f794910a14 100644 --- a/activerecord/lib/active_record/querying.rb +++ b/activerecord/lib/active_record/querying.rb @@ -5,7 +5,7 @@ module Querying delegate :find, :take, :take!, :first, :first!, :last, :last!, :exists?, :any?, :many?, :none?, :one?, to: :all delegate :second, :second!, :third, :third!, :fourth, :fourth!, :fifth, :fifth!, :forty_two, :forty_two!, :third_to_last, :third_to_last!, :second_to_last, :second_to_last!, to: :all delegate :first_or_create, :first_or_create!, :first_or_initialize, to: :all - delegate :find_or_create_by, :find_or_create_by!, :find_or_initialize_by, to: :all + delegate :find_or_create_by, :find_or_create_by!, :create_or_find_by, :create_or_find_by!, :find_or_initialize_by, to: :all delegate :find_by, :find_by!, to: :all delegate :destroy_all, :delete_all, :update_all, to: :all delegate :find_each, :find_in_batches, :in_batches, to: :all diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index d71c430045f96..c384665f691f8 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -147,18 +147,7 @@ def first_or_initialize(attributes = nil, &block) # :nodoc: # or processes there is a race condition between both calls and it could # be the case that you end up with two similar records. # - # Whether that is a problem or not depends on the logic of the - # application, but in the particular case in which rows have a UNIQUE - # constraint an exception may be raised, just retry: - # - # begin - # CreditAccount.transaction(requires_new: true) do - # CreditAccount.find_or_create_by(user_id: user.id) - # end - # rescue ActiveRecord::RecordNotUnique - # retry - # end - # + # If this might be a problem for your application, please see #create_or_find_by. def find_or_create_by(attributes, &block) find_by(attributes) || create(attributes, &block) end @@ -170,6 +159,41 @@ def find_or_create_by!(attributes, &block) find_by(attributes) || create!(attributes, &block) end + # Attempts to create a record with the given attributes in a table that has a unique constraint + # on one or several of its columns. If a row already exists with one or several of these + # unique constraints, the exception such an insertion would normally raise is caught, + # and the existing record with those attributes is sought found using #find_by. + # + # This is similar to #find_or_create_by, but avoids the problem of stale reads, as that methods needs + # to first query the table, then attempt to insert a row if none is found. That leaves a timing gap + # between the SELECT and the INSERT statements that can cause problems in high throughput applications. + # + # There are several drawbacks to #create_or_find_by, though: + # + # * The underlying table must have the relevant columns defined with unique constraints. + # * A unique constraint violation may be triggered by only one, or at least less than all, + # of the given attributes. This means that the subsequent #find_by may fail to find a + # matching record, which will then return nil, rather than a record will the given attributes. + # * It relies on exception handling to handle control flow, which may be marginally slower. And + # + # This method will always returns a record if all given attributes are covered by unique constraints, + # but if creation was attempted and failed due to validation errors it won't be persisted, you get what + # #create returns in such situation. + def create_or_find_by(attributes, &block) + create(attributes, &block) + rescue ActiveRecord::RecordNotUnique + find_by(attributes) + end + + # Like #create_or_find_by, but calls + # {create!}[rdoc-ref:Persistence::ClassMethods#create!] so an exception + # is raised if the created record is invalid. + def create_or_find_by!(attributes, &block) + create!(attributes, &block) + rescue ActiveRecord::RecordNotUnique + find_by(attributes) + end + # Like #find_or_create_by, but calls {new}[rdoc-ref:Core#new] # instead of {create}[rdoc-ref:Persistence::ClassMethods#create]. def find_or_initialize_by(attributes, &block) diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index bc7c54dbe0a89..57baf6470f86a 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -1349,6 +1349,15 @@ def test_find_or_create_by! assert_raises(ActiveRecord::RecordInvalid) { Bird.find_or_create_by!(color: "green") } end + def test_create_or_find_by + assert_nil Subscriber.find_by(nick: "bob") + + subscriber = Subscriber.create!(nick: "bob") + + assert_equal subscriber, Subscriber.create_or_find_by(nick: "bob") + assert_not_equal subscriber, Subscriber.create_or_find_by(nick: "cat") + end + def test_find_or_initialize_by assert_nil Bird.find_by(name: "bob") From ee7a4536a793e88d6259621c443c2694c1b3cc58 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Tue, 13 Feb 2018 16:02:24 -0800 Subject: [PATCH 2/7] Raise an exception if no record can be found Better than nil. --- activerecord/lib/active_record/relation.rb | 7 ++++--- activerecord/test/cases/relations_test.rb | 8 ++++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index c384665f691f8..ef9bcbd4b49be 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -173,7 +173,8 @@ def find_or_create_by!(attributes, &block) # * The underlying table must have the relevant columns defined with unique constraints. # * A unique constraint violation may be triggered by only one, or at least less than all, # of the given attributes. This means that the subsequent #find_by may fail to find a - # matching record, which will then return nil, rather than a record will the given attributes. + # matching record, which will then raise an `ActiveRecord::NotFound` exception, + # rather than a record will the given attributes. # * It relies on exception handling to handle control flow, which may be marginally slower. And # # This method will always returns a record if all given attributes are covered by unique constraints, @@ -182,7 +183,7 @@ def find_or_create_by!(attributes, &block) def create_or_find_by(attributes, &block) create(attributes, &block) rescue ActiveRecord::RecordNotUnique - find_by(attributes) + find_by!(attributes) end # Like #create_or_find_by, but calls @@ -191,7 +192,7 @@ def create_or_find_by(attributes, &block) def create_or_find_by!(attributes, &block) create!(attributes, &block) rescue ActiveRecord::RecordNotUnique - find_by(attributes) + find_by!(attributes) end # Like #find_or_create_by, but calls {new}[rdoc-ref:Core#new] diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 57baf6470f86a..5ec2f135d9230 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -1358,6 +1358,14 @@ def test_create_or_find_by assert_not_equal subscriber, Subscriber.create_or_find_by(nick: "cat") end + def test_create_or_find_by_with_non_unique_attributes + Subscriber.create!(nick: "bob", name: "the builder") + + assert_raises(ActiveRecord::RecordNotFound) do + Subscriber.create_or_find_by(nick: "bob", name: "the cat") + end + end + def test_find_or_initialize_by assert_nil Bird.find_by(name: "bob") From cde52088ad5132cb761ee30eeb76a666b597d671 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Tue, 13 Feb 2018 16:40:25 -0800 Subject: [PATCH 3/7] Address the race condition between INSERT and SELECT --- activerecord/lib/active_record/relation.rb | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index ef9bcbd4b49be..2e18963c13fb8 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -164,9 +164,9 @@ def find_or_create_by!(attributes, &block) # unique constraints, the exception such an insertion would normally raise is caught, # and the existing record with those attributes is sought found using #find_by. # - # This is similar to #find_or_create_by, but avoids the problem of stale reads, as that methods needs - # to first query the table, then attempt to insert a row if none is found. That leaves a timing gap - # between the SELECT and the INSERT statements that can cause problems in high throughput applications. + # This is similar to #find_or_create_by, but avoids the problem of stale reads between the SELECT + # and the INSERT, as that methods needs to first query the table, then attempt to insert a row + # if none is found. # # There are several drawbacks to #create_or_find_by, though: # @@ -175,11 +175,16 @@ def find_or_create_by!(attributes, &block) # of the given attributes. This means that the subsequent #find_by may fail to find a # matching record, which will then raise an `ActiveRecord::NotFound` exception, # rather than a record will the given attributes. - # * It relies on exception handling to handle control flow, which may be marginally slower. And - # - # This method will always returns a record if all given attributes are covered by unique constraints, - # but if creation was attempted and failed due to validation errors it won't be persisted, you get what - # #create returns in such situation. + # * While we avoid the race condition between SELECT -> INSERT from #find_or_create_by, + # we actually have another race condition between INSERT -> SELECT, which can be triggered + # if a DELETE between those two statements is run by another client. But for most applications, + # that's a significantly less likely condition to hit. + # * It relies on exception handling to handle control flow, which may be marginally slower. + # + # This method will return a record if all given attributes are covered by unique constraints + # (unless the INSERT -> DELETE -> SELECT race condition is triggered), but if creation was attempted + # and failed due to validation errors it won't be persisted, you get what #create returns in + # such situation. def create_or_find_by(attributes, &block) create(attributes, &block) rescue ActiveRecord::RecordNotUnique From 03374e9c634888cebc724690003a137d66a7cdcd Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Wed, 14 Feb 2018 15:36:14 -0800 Subject: [PATCH 4/7] Require new transaction in case unique constraint causes rollback Otherwise PG will complain with "PG::InFailedSqlTransaction: ERROR: current transaction is aborted, commands ignored until end of transaction block". Thanks @matthewd --- activerecord/lib/active_record/relation.rb | 4 ++-- activerecord/test/cases/relations_test.rb | 12 ++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 2e18963c13fb8..5d18c37ee14ef 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -186,7 +186,7 @@ def find_or_create_by!(attributes, &block) # and failed due to validation errors it won't be persisted, you get what #create returns in # such situation. def create_or_find_by(attributes, &block) - create(attributes, &block) + transaction(requires_new: true) { create(attributes, &block) } rescue ActiveRecord::RecordNotUnique find_by!(attributes) end @@ -195,7 +195,7 @@ def create_or_find_by(attributes, &block) # {create!}[rdoc-ref:Persistence::ClassMethods#create!] so an exception # is raised if the created record is invalid. def create_or_find_by!(attributes, &block) - create!(attributes, &block) + transaction(requires_new: true) { create!(attributes, &block) } rescue ActiveRecord::RecordNotUnique find_by!(attributes) end diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 5ec2f135d9230..18e0bed5b61fa 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -22,6 +22,7 @@ require "models/category" require "models/categorization" require "models/edge" +require "models/subscriber" class RelationTest < ActiveRecord::TestCase fixtures :authors, :author_addresses, :topics, :entrants, :developers, :companies, :developers_projects, :accounts, :categories, :categorizations, :categories_posts, :posts, :comments, :tags, :taggings, :cars, :minivans @@ -1366,6 +1367,17 @@ def test_create_or_find_by_with_non_unique_attributes end end + def test_create_or_find_by_within_transaction + assert_nil Subscriber.find_by(nick: "bob") + + subscriber = Subscriber.create!(nick: "bob") + + Subscriber.transaction do + assert_equal subscriber, Subscriber.create_or_find_by(nick: "bob") + assert_not_equal subscriber, Subscriber.create_or_find_by(nick: "cat") + end + end + def test_find_or_initialize_by assert_nil Bird.find_by(name: "bob") From 0768324360085fd0a7284327b84371cd3f531d84 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Wed, 14 Feb 2018 15:37:47 -0800 Subject: [PATCH 5/7] Documentation fixes Thanks @tjschuck --- activerecord/lib/active_record/relation.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 5d18c37ee14ef..f25a8c466301b 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -162,10 +162,10 @@ def find_or_create_by!(attributes, &block) # Attempts to create a record with the given attributes in a table that has a unique constraint # on one or several of its columns. If a row already exists with one or several of these # unique constraints, the exception such an insertion would normally raise is caught, - # and the existing record with those attributes is sought found using #find_by. + # and the existing record with those attributes is found using #find_by. # # This is similar to #find_or_create_by, but avoids the problem of stale reads between the SELECT - # and the INSERT, as that methods needs to first query the table, then attempt to insert a row + # and the INSERT, as that method needs to first query the table, then attempt to insert a row # if none is found. # # There are several drawbacks to #create_or_find_by, though: @@ -173,8 +173,8 @@ def find_or_create_by!(attributes, &block) # * The underlying table must have the relevant columns defined with unique constraints. # * A unique constraint violation may be triggered by only one, or at least less than all, # of the given attributes. This means that the subsequent #find_by may fail to find a - # matching record, which will then raise an `ActiveRecord::NotFound` exception, - # rather than a record will the given attributes. + # matching record, which will then raise an ActiveRecord::RecordNotFound exception, + # rather than a record with the given attributes. # * While we avoid the race condition between SELECT -> INSERT from #find_or_create_by, # we actually have another race condition between INSERT -> SELECT, which can be triggered # if a DELETE between those two statements is run by another client. But for most applications, From 72cbe0c9755de562ccc737ed9110cb9e60a7063d Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Wed, 14 Feb 2018 15:41:56 -0800 Subject: [PATCH 6/7] Add changelog entry --- activerecord/CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index d73c0859ee0ed..bbb0ec575e192 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,9 @@ +## Rails 6.0.0.alpha (Unreleased) ## + +* Add ActiveRecord::Base.create_or_find_by/! to deal with the SELECT/INSERT race condition in + ActiveRecord::Base.find_or_create_by/! by leaning on unique constraints in the database. + + *DHH* Please check [5-2-stable](https://github.com/rails/rails/blob/5-2-stable/activerecord/CHANGELOG.md) for previous changes. From ba45394236077798c94d88e2deec38ecc8da4d9b Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Wed, 14 Feb 2018 15:47:50 -0800 Subject: [PATCH 7/7] Remove trailing whitespace --- activerecord/lib/active_record/relation.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index f25a8c466301b..4e9d10700cb04 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -161,17 +161,17 @@ def find_or_create_by!(attributes, &block) # Attempts to create a record with the given attributes in a table that has a unique constraint # on one or several of its columns. If a row already exists with one or several of these - # unique constraints, the exception such an insertion would normally raise is caught, + # unique constraints, the exception such an insertion would normally raise is caught, # and the existing record with those attributes is found using #find_by. # - # This is similar to #find_or_create_by, but avoids the problem of stale reads between the SELECT - # and the INSERT, as that method needs to first query the table, then attempt to insert a row - # if none is found. + # This is similar to #find_or_create_by, but avoids the problem of stale reads between the SELECT + # and the INSERT, as that method needs to first query the table, then attempt to insert a row + # if none is found. # # There are several drawbacks to #create_or_find_by, though: # # * The underlying table must have the relevant columns defined with unique constraints. - # * A unique constraint violation may be triggered by only one, or at least less than all, + # * A unique constraint violation may be triggered by only one, or at least less than all, # of the given attributes. This means that the subsequent #find_by may fail to find a # matching record, which will then raise an ActiveRecord::RecordNotFound exception, # rather than a record with the given attributes. @@ -181,9 +181,9 @@ def find_or_create_by!(attributes, &block) # that's a significantly less likely condition to hit. # * It relies on exception handling to handle control flow, which may be marginally slower. # - # This method will return a record if all given attributes are covered by unique constraints + # This method will return a record if all given attributes are covered by unique constraints # (unless the INSERT -> DELETE -> SELECT race condition is triggered), but if creation was attempted - # and failed due to validation errors it won't be persisted, you get what #create returns in + # and failed due to validation errors it won't be persisted, you get what #create returns in # such situation. def create_or_find_by(attributes, &block) transaction(requires_new: true) { create(attributes, &block) }