Change find_or_create to do INSERT before SELECT #10670

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@jcoglan
Contributor
jcoglan commented May 17, 2013

This makes find_or_create() safe to use with concurrent requests by
relying on unique indexes to raise errors if the INSERT fails. By doing
the SELECT first, we leave this method open to the same race conditions
that affect uniqueness validation.

@jcoglan jcoglan Change find_or_create to do INSERT before SELECT.
This makes find_or_create() safe to use with concurrent requests by
relying on unique indexes to raise errors if the INSERT fails. By doing
the SELECT first, we leave this method open to the same race conditions
that affect uniqueness validation.
6c6167f
@steveklabnik
Member

Nice.

@josevalim josevalim commented on the diff May 17, 2013
activerecord/lib/active_record/relation.rb
@@ -179,12 +179,16 @@ def first_or_initialize(attributes = nil, &block) # :nodoc:
# end
# # => <User id: 2, first_name: 'Scarlett', last_name: 'Johansson'>
def find_or_create_by(attributes, &block)
- find_by(attributes) || create(attributes, &block)
+ create(attributes, &block)
+ rescue RecordNotUnique
@josevalim
josevalim May 17, 2013 Member

This will potentially rescue an exception raised by a model created while creating the model given by those attributes.

@josevalim
Member

Thanks @jcoglan!

This is a major change from how the method currently works though and calling create can generate side effects which would be surprising given the function name (i.e. why it is attempting to create before it finds?). If we are going to have this functionality, I suggest to provide a create_or_find_by and document explicitly its use case and advantages over the current approach.

@jcoglan
Contributor
jcoglan commented May 17, 2013

So, I was half-trolling with this, in that I think Rails should encourage you to do the right thing for database integrity (i.e. use unique indexes rather than client-side query-then-insert), but I don't know if this is the right way to do it. I'm aware it entirely breaks back-compat and won't work unless you've indexed your DB correctly. Maybe the right thing to do is invent a new API and discourage use of this, like how the docs for validates_uniqueness_of tell you not to use it for concurrent requests.

So having said that I'd like to discuss what we can do to avoid DB consistency problems when people have > 1 server.

@schneems
Member

👍 database constraints and encouraging their use. It's the number 1 criticism I hear from our postgres team about AR. What if we have the uniqueness validator take some kind of flag db_constraint: true and all it does is check for a unique index and raise an error if not present. As @jcoglan mentioned, don't think this can go into AR as is for now.

@fxn
Member
fxn commented Jun 16, 2013

On one hand, database constraints are not required, therefore Active Record cannot assume they are set.

On the other hand, find_or_create works with any set of attributes, not only supposedly unique attributes.

The API in 4.0.0 explains the race condition and how to deal with it given a unique constraint: http://edgeapi.rubyonrails.org/classes/ActiveRecord/Relation.html#method-i-find_or_create_by (I personally wrote that one.)

Let's close this one, can't be merged.

@fxn fxn closed this Jun 16, 2013
@fxn
Member
fxn commented Jun 16, 2013

Oh, let me add that the proposal has a race condition, since between the exception is raised and the find call happens, the record could be deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment