- Notifications
You must be signed in to change notification settings - Fork22.1k
Add #create_or_find_by to lean on unique constraints#31989
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
nynhex commentedFeb 13, 2018
This is really good. I have multiple use cases for this. 👍 |
| defcreate_or_find_by!(attributes, &block) | ||
| create!(attributes, &block) | ||
| rescueActiveRecord::RecordNotUnique | ||
| find_by(attributes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should raise in the fallbackfind case? It'd be surprising to ever get anil result (for whatever reason, including a missing unique index) from these methods.
There was a problem hiding this comment.
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!.
Better than nil.
| Subscriber.create_or_find_by(nick:"bob",name:"the cat") | ||
| end | ||
| end | ||
There was a problem hiding this comment.
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 withcreate_with (to change the behavior of thecreate) and withwhere (to change the behavior offind!
I'm not sure if those method should be used withcreate_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.
There was a problem hiding this comment.
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 asfind_or_create_by, except instead of an|| we're using arescue. But the flow is the same.
There was a problem hiding this comment.
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 fortest_find_or_create_by_with_create_with.
There was a problem hiding this comment.
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
| # 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This isn'ttotally 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.
There was a problem hiding this comment.
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.
| # #create returns in such situation. | ||
| defcreate_or_find_by(attributes, &block) | ||
| create(attributes, &block) | ||
| rescueActiveRecord::RecordNotUnique |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This needstransaction(requires_new: true) do around thecreate to work in an ongoing surrounding transaction (on at least PostgreSQL)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 stickON 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.
| defcreate_or_find_by!(attributes, &block) | ||
| create!(attributes, &block) | ||
| rescueActiveRecord::RecordNotUnique | ||
| find_by!(attributes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we want toretry here instead of raising if the find fails? Seems confusing forcreate_or_find_by! to raise RecordNotFound, just because of a racing delete.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 racingdelete; insert; delete.
There was a problem hiding this comment.
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.
dhh commentedFeb 14, 2018 via email
TIL! Do you have an idea for a good test for that? …On Tue, Feb 13, 2018 at 4:49 PM, Matthew Draper ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In activerecord/lib/active_record/relation.rb <#31989 (comment)>: > + # + # 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, + # 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 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. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#31989 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAKtbTMH-JrUyizELV10shNLxOgesnAks5tUi2lgaJpZM4SElV8> . |
matthewd commentedFeb 14, 2018
I think this should do it: deftest_create_or_find_by_within_transactionassert_nilSubscriber.find_by(nick:"bob")subscriber=Subscriber.create!(nick:"bob")Subscriber.transactiondoassert_equalsubscriber,Subscriber.create_or_find_by(nick:"bob")assert_not_equalsubscriber,Subscriber.create_or_find_by(nick:"cat")endend |
tjschuck left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Documentation edits.
| # * 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, |
There was a problem hiding this comment.
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.
| # * 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
will =>with
| # 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. |
There was a problem hiding this comment.
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.
| # 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 between the SELECT | ||
| # and the INSERT, as that methods needs to first query the table, then attempt to insert a row |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
methods =>method
Otherwise PG will complain with "PG::InFailedSqlTransaction: ERROR: current transaction is aborted, commands ignored until end of transaction block".Thanks@matthewd
AnalyzePlatypus commentedFeb 17, 2018
Awesome! |
subhashsaran commentedFeb 18, 2018
Good stuff. |
dhh commentedFeb 21, 2018 via email
Happy to explore that as a PG 9.5 level-up somehow. But the whole feature shouldn’t depend on it imo. … On Feb 21, 2018, at 12:47, Sean Griffin ***@***.***> wrote:@sgrif commented on this pull request. In activerecord/lib/active_record/relation.rb: > + # + # 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, + # 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 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. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread. |
sikachu commentedFeb 21, 2018
yhirano55 commentedApr 17, 2018
I want to use these awesome class methods on Rails 5.2, so much. |
bogdanvlviv commentedApr 17, 2018
I think we don't, "New features are only added to the master branch and will not be made available in point releases." byMaintenance Policy for Ruby on Rails. |
yhirano55 commentedApr 17, 2018
I see, thank you for your explanation. |
djezzzl commentedDec 7, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hi everybody, As we start relying on the database constraints more, I'd suggest (and few people like the idea) to have the improved versions of I just opened the issue#34650 where we can discuss the topic. Any feedback is very appreciated! |
create_or_find_by will be implemented in Rails 6.0 (rails/rails#31989)However we've decided to use it before it comes out, and would love it to be in rubocop scope as well.I guess we're not the first ones to do it, so it could benefit to most.
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:
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.
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.