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 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
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
49 changes: 37 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,42 @@ 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 raise an `ActiveRecord::NotFound` exception,
Copy link
Contributor

Choose a reason for hiding this comment

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

`ActiveRecord::NotFound` needs <tt> for Rdoc code formatting, not backticks.

# rather than a record will the given attributes.
Copy link
Contributor

Choose a reason for hiding this comment

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

will => with

# * 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.

Do we want to retry here instead of raising if the find fails? Seems confusing for create_or_find_by! to raise RecordNotFound, just because of a racing delete.

Copy link
Member Author

Choose a reason for hiding this comment

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

You can't retry this failure as it should only occur when you use attributes where not all of them are covered by unique constraints. It's more like a INVALID QUERY kind of failure.

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, I was assuming the delete race, rather than operator error. I guess we could retry once, so we're safe from a poorly-timed simple delete. That way we'd only be tripped up by a racing delete; insert; delete.

Copy link
Member Author

Choose a reason for hiding this comment

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

True. I'm not sure whether having just a single retry is a good warranty, though. Since it's hard for us to tell the difference between legit race condition and invalid query.

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
17 changes: 17 additions & 0 deletions activerecord/test/cases/relations_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1349,6 +1349,23 @@ 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_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

Copy link
Member

Choose a reason for hiding this comment

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

I think this is missing a test with create_with (to change the behavior of the create) and with where (to change the behavior of find!

I'm not sure if those method should be used with create_or_find_by but right now they can and I have no idea of the effect they would have, so better to test them to make sure it is what we expect.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see how that differs? create_or_find_by uses the same flow as find_or_create_by, except instead of an || we're using a rescue. But the flow is the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, it's not that we can't test it, just that I don't think it teaches us anything interesting. The test would be identical to the one for test_find_or_create_by_with_create_with.

Copy link
Member

Choose a reason for hiding this comment

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

If it is the same behavior we want so I guess it is fine being tested in that test

def test_find_or_initialize_by
assert_nil Bird.find_by(name: "bob")

Expand Down