Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add #create_or_find_by to lean on unique constraints #31989

Merged
merged 7 commits into from
Feb 15, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/querying.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
48 changes: 36 additions & 12 deletions activerecord/lib/active_record/relation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is sought found using #find_by — "sought" should be deleted.

#
# 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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't totally true: another client could update or delete the row between the rejected INSERT and the subsequent SELECT. This race condition is complementary to the one in #find_or_create_by.

That caveat probably belongs in the "drawbacks" section, but even still I don't think we can make this claim here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Will add that point. It's a much rarer race condition in many apps, I'd say.

# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs transaction(requires_new: true) do around the create to work in an ongoing surrounding transaction (on at least PostgreSQL)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this not necessary for find_or_create_by?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it doesn't cause an SQL error and then attempt to recover. PostgreSQL remembers when an error has occurred inside a transaction, and disallows all further operations until that transaction has been rolled back.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are willing to require PG 9.5 or later for this (when using the PG adapter), we could just stick ON CONFLICT DO NOTHING on the end instead. This would let us wrap the whole thing in a transaction instead of just the create, which eliminates any possibility of race conditions.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should raise in the fallback find case? It'd be surprising to ever get a nil result (for whatever reason, including a missing unique index) from these methods.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point. Let's change that to find_by!.

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)
Expand Down
9 changes: 9 additions & 0 deletions activerecord/test/cases/relations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Expand Down