This project is archived and is in readonly mode.

#6147 open
Brandon Dimcheff

find_or_create via has_many fails for hash parameters

Reported by Brandon Dimcheff | December 11th, 2010 @ 06:55 AM | in 2.x

I tried to add this to 1108, since it is related, but it failed without a flash message.

find_or_create is still messed up in 2-3-stable. Check out this failing test case, and you'll see that the proper values aren't being set when find_or_create is called on an association with a hash. It looks like it's sticking a yamlized version of the hash into the body field and not setting the type at all.

This effectively prevents you from doing anything along the lines of

object.association.find_or_create_by_name(:name => 'foo', :title => 'bar')

since it seems to mess up name and just ignore title.

Comments and changes to this ticket

  • Brandon Dimcheff

    Brandon Dimcheff December 11th, 2010 @ 06:57 AM

    I guess lighthouse won't let me upload a patch, so here it is

  • Piotr Sarnacki

    Piotr Sarnacki December 11th, 2010 @ 01:30 PM

    • Milestone set to 2.x
    • State changed from “new” to “open”
    • Tag set to bug activerecord find_or_create_by
    • Importance changed from “” to “Low”
  • Ryan Bigg

    Ryan Bigg December 11th, 2010 @ 11:53 PM

    This is not supported by Rails at the moment. Please try my gem find_by_hash: http://github.com/radar/find_by_hash to get this functionality.

  • Brandon Dimcheff

    Brandon Dimcheff December 12th, 2010 @ 03:34 AM

    @ryan - This as worked for me in rails 2 for a while. In the docs it shows that you can do this with an ActiveRecord class directly:

    Tag.find_or_create_by_name(:name => "rails", :creator => current_user)
    

    Now I realize that I'm doing this with an association instead of directly on the class, but why should the API be any different?

  • Brandon Dimcheff

    Brandon Dimcheff December 12th, 2010 @ 03:36 AM

    I just edited the ticket to fix an error - find_or_create works properly on a model class, as far as I can tell. It does not work properly when called on an association.

  • James Badger

    James Badger December 15th, 2010 @ 10:25 PM

    I have also experienced this bug, and I am working on a patch.

    I tried to use your test Brandon, but I was unable to properly set the 'type' attribute for the Comment model, it would always evaluate to nil. This error is also present in test_find_or_create_by_with_additional_parameters (from #1108), which does not check to see if the 'type' was actually set (so no error or failure is raised). As 'type' is reserved for STI in this context, and 'test' is not a valid subclass for Comment.

    For that reason, I rewrote them with the Author-Post association instead, which may fit the 'additional attributes setting' part of the test.

  • Brandon Dimcheff

    Brandon Dimcheff December 15th, 2010 @ 11:07 PM

    ah, ok yeah that makes sense. Since I never got that test passing, I didn't run into that problem. I just used the model objects that were already being used in the other activerecord tests. I think those test the type field, but maybe they do so without reloading the object, so the type is deceptively set correctly...

  • James Badger

    James Badger December 16th, 2010 @ 02:02 AM

    https://gist.github.com/742909

    Here is my patch, which (as the test case shows) attempts to fix a few scenarios:

    • Finding an existing model using find_or_create_by_title("A fine post")
    • Creating a new model using find_or_create_by_title(:title => "A wood post", :body => "It is made of wood.")
    • Creating a new model using find_or_create_by_title("An iron post", :body => "A heavy post made of iron.")
    • Creating a new model using find_or_create_by_title_and_body("A witness post", "The other post was near here.")

    The one in which I am not sure what type of behaviour would be preferred is the last one, where

    find_or_create_by_title_and_body("The last post", "If only they were this easy to find.", {:title => "Maybe not the last post", :body => "Another post."})

    will use the hash-defined values over the string values. This could be reversed to prefer the string values by using a custom merge! block in the patch.

  • Pat George

    Pat George February 9th, 2011 @ 05:57 PM

    1. I just updated an old Rails 2.3.4 project to 2.3.11 (for the security update) only to discover all our "object.association.find_or_create_by_attr1_and_attr2(hash)" calls no longer work.

    I went back to 2.3.5 and tested it each Rails version. It stopped working all together in 2.3.9 throwing an "ArgumentError: Unknown key(s)" error (which I believe is what issue 1108 was about). In 2.3.11 it no longer throws the error but instead assigns the entire hash to the first attribute:

    account = Account.first; params = { :url => "google2311.com", :token => "foo2311", :user_id => account.users.first.id } account.links.find_or_create_by_url_and_token params _.url => {:token=>"foo2311", :user_id=>1, :url=>"google2311.com"}

    If this isn't supported anymore, fine. But know that it's a regression.

  • Pat George

    Pat George February 9th, 2011 @ 05:59 PM

    Sorry for the bad code block.

    >> account = Account.first; params = { :url => "google2311.com", :token => "foo2311", :user_id => account.users.first.id }
    >> account.links.find_or_create_by_url_and_token params
    >> _.url
    => {:token=>"foo2311", :user_id=>1, :url=>"google2311.com"}
    
  • Brandon Dimcheff

    Brandon Dimcheff February 10th, 2011 @ 04:38 PM

    @pat, yes that was my experience as well. I stuck a git checkout of rails in vendor/rails and git bisected until I found the commit that caused the problem, which wasn't released until 2.3.9, I think. I haven't tried James' patch yet, but if it works, maybe we can get it accepted...

    This is definitely a regression, and it logically should work, considering it works when it's not using an association proxy.

  • Brian Finney

    Brian Finney March 8th, 2011 @ 02:58 AM

    Bump, also running into this issue. This is also blocking for us to update to the latest rails-2.3 from 2.3.8

    @James, That patch is close but misses a few edge cases that are handled by ActiveRecord::Base#find_or_create_by_*.

    Attaching a patch that incorporates James' tests, plus a few more edge cases, and fixes this bug.

    Also as pull request at https://github.com/rails/rails/pull/207

    Thanks
    Brian

  • Brian Finney
  • bingbing
  • jeff (at snowmoonsoftware)

    jeff (at snowmoonsoftware) May 4th, 2011 @ 06:16 PM

    Bump. Brian, I applied your patch to rails 2.3.11, and still see a problem with find_or_create_by with multiple parameters:

    p.niod_metrics.find_or_create_by_metric_type_id_and_month(666, Date.parse('2011-06-01'))
    
    NiodMetric Load (0.5ms)   SELECT * FROM 'niod_metrics' WHERE ('niod_metrics'.'month' IS NULL AND 
    'niod_metrics'.'metric_type_id' IN( 666,'2011-06-01' )) AND ('niod_metrics'.niodmetrics_id = 1750 AND 
    'niod_metrics'.niodmetrics_type = 'Profile') LIMIT 1
    
    
    NiodMetric Create (0.2ms)   INSERT INTO 'niod_metrics' ('created_at', 'month', 'updated_at', 'last_activity_at', 
    'niodmetrics_type', 'value', 'metric_type_id', 'niodmetrics_id') VALUES('2011-05-04 17:10:44', '2011-06-01', 
    '2011-05-04 17:10:44', NULL, 'Profile', NULL, 666, 1750)
    

    The insert is correct, but the find isn't. This is blocking a move from rails 2.3.4 to 2.3.11.

  • Brian Finney

    Brian Finney May 10th, 2011 @ 12:49 AM

    Hey Jeff

    I believe the issue you're seeing is unrelated to this patch. This bug is specific to using hash parameters to effect the create_by statement and the patch only effects the insert statement. To find the root cause of the select being incorrect, you need to follow the calls through pull_finder_args_from(DynamicFinderMatch.match(method).attribute_names, *args) on line 383 of activerecord/lib/active_record/associations/association_collection.rb.

    Thanks

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Pages