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

Already on GitHub? Sign in to your account

Add a ActiveRecord::Base#find_or_create method #2420

Closed
jonleighton opened this Issue Aug 4, 2011 · 23 comments

Comments

Projects
None yet
10 participants
Member

jonleighton commented Aug 4, 2011

To replace the dynamic find_or_create_by methods accepting a hash. See #2404 for explanation.

@ghost ghost assigned jonleighton Aug 4, 2011

I'll agree that at first glance the use described in 2404 might be less than obvious, and I look forward to having a better option available.
I also can't help but notice that both your comment
#2404 (comment) as well as preceding one
#2404 (comment)
both perfectly illustrate that the first-pass alternatives are more cumbersome without adding (much if any) clarity.

I hope this discussion leads to an altogether better idiom before getting too set on removing a (perhaps) slightly odd but useful feature that works as documented.
It's also completely obvious to me that green is the only sensible color for a bikeshed.

The current find_or_create_by_foo usage also feels a bit like a "rails 2" feature that missed out on the rails 3 goodness. Since we can already create record through relations, it seems like all we're missing is the "or". What if we had something along the lines of ...

Tag.where(:name => "rails", :color => "ruby").or_create(:creator => current_user, :exta => "stuff")

It seems like it might clarify any ambiguity over which options apply to finding vs. creating.

Contributor

iain commented Aug 19, 2011

+1 on @iconoclast's idea. Looks very clear!

Contributor

andmej commented Aug 27, 2011

+1 on @iconoclast's idea. Gorgeous syntax!

charly commented Aug 27, 2011

+1 on @iconoclast's.

Contributor

dmitriy-kiriyenko commented Aug 27, 2011

+1 on @iconoclast's. A really cool syntax.

I see an issue in this, however. The find_or_create_by returned a single object, while where returns a relation, which turns to array when we use it.

So for consistency we'll have to take one of the two ways: either or_create should return an array of one object (as the where would do), or or_create in case there were many objects should return the first of them (like find_or_create_by did). I'd vote for the second option - that the or_create method should work like where().first in case of scope returns something.

What do you think?

Contributor

andmej commented Aug 27, 2011

I also think it should return a single object.

EDIT:

In fact, I suggest this method be called first_or_create to avoid ambiguities. What do you guys think of that?

Tag.where(:name => "rails", :color => "ruby").first_or_create(:creator => current_user, :exta => "stuff")
Contributor

dmitriy-kiriyenko commented Aug 27, 2011

Sounds reasonable for me. +1 for that.

Member

jonleighton commented Aug 27, 2011

I like this proposal, and I think first_or_create is a good name (I definitely prefer it over just or_create).

hooopo commented Aug 27, 2011

+1 for first_or_create

Contributor

exviva commented Aug 27, 2011

Then there should probably also be first_or_initialize, right?

@andmej andmej added a commit to andmej/rails that referenced this issue Aug 28, 2011

@andmej andmej Adding first_or_create and first_or_create! methods to ActiveRecord::…
…Relation. Related to #2420.
4f22974
Contributor

andmej commented Aug 28, 2011

I've pushed a patch to my fork that implements first_or_create and first_or_create!:

ruby-1.9.2-p0 > Bird.all
 => [] 
ruby-1.9.2-p0 > Bird.where(:color => "green").first_or_create(:name => "parrot")
 => #<Bird id: 1, name: "parrot", color: "green", created_at: "2011-08-28 03:33:35", updated_at: "2011-08-28 03:33:35"> 
ruby-1.9.2-p0 > Bird.where(:color => "green").first_or_create(:name => "parakeet")
 => #<Bird id: 1, name: "parrot", color: "green", created_at: "2011-08-28 03:33:35", updated_at: "2011-08-28 03:33:35"> 

Can you guys take a look and tell me if the patch is mergeable? Also, what would be the appropriate place to document this?

Owner

guilleiguaran commented Aug 28, 2011

@andmej A good place to document it is the ActiveRecord Querying Guide

Member

jonleighton commented Aug 28, 2011

Hey @andmej, looks like a good start. Here are some comments:

  • Add a basic test for the version delegated from AR::Base, so we have that covered (rather than the respond_to? test you currently have).
  • Split up assertions which do not need to happen in succession into separate test methods
  • Add tests for the two methods with no parameters, and with a block parameter
  • Add inline documentation for the methods, and add to the querying guide too
  • Add a CHANGELOG entry
  • If you're up for also adding a first_or_initialize method, I think that'd be good too, although I am unsure about the naming since we do not have an initialize class method. Perhaps first_or_build is better as this is in common with associations.

Thanks!

Contributor

exviva commented Aug 28, 2011

@jonleighton I was actually thinking of first_or_build as well, and I personally prefer it, but if I'm not mistaken, dynamic finders had a find_or_initialize_by finder, that's why I suggested it.

Member

jonleighton commented Aug 28, 2011

@exviva yes, good point, but I think we should use this opportunity to avoid initialize as that was bad naming in the first place IMO. I think it should either be first_or_new or first_or_build. I think the former is more 'correct' because it aligns with the new method, but the latter perhaps reads a bit better. Opinions welcome.

Contributor

andmej commented Aug 28, 2011

@jonleighton all right, I will take care of that :)

The test for AR::Base should be in test/cases/base_test.rb instead of test/cases/relations_test.rb, right? Also, is there a way to test it's delegated to AR::Relation? Otherwise, it's just rewriting the same tests but removing the wherecalls and this doesn't feel very clean.

About the name for the other method, I like first_or_new.

Contributor

dmitriy-kiriyenko commented Aug 28, 2011

I'd say, first_or_build reads better. It's also consistent with has_many associations construction methods, which are create and build. If I am not making a mistake, there is also a build method on relations, which works similar to new.

So my vote is to first_or_build. Or both =) - can be a useful alias.

@andmej - you can mock to test the very delegation. However, not sure if here it's really better than just doing the basic test for AR::Base.

@jonleighton - maybe it could be useful to have a test helper method like assert_delegates "ActiveRecord::Base#first_or_create", :to => :relation?

Member

jonleighton commented Aug 28, 2011

@andmej

You could test it is delegated using stubbing (this is mocha syntax, which is used in AR tests):

scoped, record = stub, stub
Foo.stubs(:scoped).returns(scoped)
scoped.stubs(:first_or_create).returns(record)

assert_equal record, Foo.first_or_create

However, this test is intrinsically more linked to the exact implementation of AR::Base (e.g. it must know about the existence of the scoped method).

Personally, I would probably just choose to add a very simple usage of first_or_create in base_test.rb (rather than the exhaustive tests you will put in relations_test.rb) and be done with it. The advantage is that it's less coupled to the implementation, the disadvantage is that you are not so much testing that the delegation exists but a part of the implementation in Relation.

However, my feeling is that in this case the most important thing is simply to make sure the code path is covered by a test, so that if for whatever reason that delegation disappeared, a test would break.

@andmej andmej added a commit to andmej/rails that referenced this issue Aug 29, 2011

@andmej andmej Adding basic test for first_or_create and first_or_create! to AR::Bas…
…e. These methods are delegated to 'scoped' and tested thoroughly on relations_test.rb. Related to #2420.
4f45621

@andmej andmej added a commit to andmej/rails that referenced this issue Aug 29, 2011

@andmej andmej Splitting up assertions in RelationTest#test_first_or_create_bang whi…
…ch do not need to happen in succession into separate test methods.

Related to #2420.
e6cbfff

@andmej andmej added a commit to andmej/rails that referenced this issue Aug 29, 2011

@andmej andmej Testing first_or_create and first_or_create! with no parameters, with…
… a block and with an array.

Related to #2420.
c9ac222

@andmej andmej added a commit to andmej/rails that referenced this issue Aug 29, 2011

@andmej andmej Adding inline documentation for first_or_create and first_or_create!
Related to #2420.
7b90e44
Contributor

nhocki commented Aug 30, 2011

@andmej I'd love to help with the documentation at the AR Querying Guide as @guilleiguaran suggested.

About the name, I think both should exists (as build is an alias for new). So just create an alias for the name and let the developers decide which one they want to use.

@andmej andmej added a commit to andmej/rails that referenced this issue Aug 30, 2011

@andmej andmej Adding documentation for first_or_create and first_or_create! in AR Q…
…uerying Guide.

Related to #2420.
ff41cf3

@andmej andmej added a commit to andmej/rails that referenced this issue Aug 30, 2011

@andmej andmej Adding first_or_new to AR::Base and AR::Relation.
Related to #2420.
f6f8489
Contributor

andmej commented Aug 30, 2011

Hi guys,

Can you take a look at the progress I've done so far? I just implemented first_or_new and also pushed some more tests as per @jonleighton's suggestions.

Only a couple of things are missing:

  • Add a first_or_build alias for first_or_new and test it.
  • Add documentation for first_or_new to the AR Querying Guide.

@nhocki, can you take care of the second point while I tackle the first one? Thanks mate.

Member

jonleighton commented Aug 30, 2011

@andmej looking good. when you're ready, submit a pull request so I will be able to make inline coments on there as necessary.

@andmej andmej added a commit to andmej/rails that referenced this issue Aug 30, 2011

@andmej andmej Aliasing first_or_new as first_or_build.
Related to #2420.
e69e12c

@andmej andmej added a commit to andmej/rails that referenced this issue Aug 30, 2011

@andmej andmej Adding first_or_create, first_or_create!, first_or_new and first_or_b…
…uild to Active Record.

This let's you write things like:

    User.where(:first_name => "Scarlett").first_or_create!(:last_name => "Johansson", :hot => true)

Related to #2420.
84dad44
Contributor

andmej commented Aug 30, 2011

@jonleighton just sent the pull request. Let me know what's next :)

@jonleighton jonleighton closed this Sep 9, 2011

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