Updated ActiveRecord::Base.find_or_create_by for 2 attributes when 1 is n #1276

Closed
wants to merge 1 commit into
from

4 participants

@jeremyf

Updated ActiveRecord::Base.find_or_create_by for 2 attributes when 1 is nil

Updated the find_or_create_by_foo_and_bar('hello') to process the find portion
similar to find_by_foo_and_bar('hello'). In the original
find_by_foo_and_bar('hello'), the resulting query was something like
"foo = 'hello' AND bar IS NULL". In the original
find_or_create_by_foo_and_bar('hello'), the resulting query was something like
"foo = 'hello'"

This patch aligns those two methods expected behaviors on the "find" portion
of the method call.

@jeremyf jeremyf Updated ActiveRecord::Base.find_or_create_by for 2 attributes when 1 …
…is nil

Updated the find_or_create_by_foo_and_bar('hello') to process the find portion
similar to find_by_foo_and_bar('hello').  In the original
find_by_foo_and_bar('hello'), the resulting query was something like
"foo = 'hello' AND bar IS NULL".  In the original
find_or_create_by_foo_and_bar('hello'), the resulting query was something like
"foo = 'hello'"

This patch aligns those two methods expected behaviors on the "find" portion
of the method call.
4c67e39
@sikachu
Ruby on Rails member

Interesting. I feel like it should instead raise an error if you're doing find_by_foo_and_bar('hello') and passing only one argument. I think if you're using _and_, you actually have to pass two arguments to it.

/cc @tenderlove

@jeremyf

Given that find_by_foo_and_bar will not raise an error if only one param is passed, I'd expect find_or_create_by_foo_and_bar to behave the same. Having these two related functions behave differently on missing parameters is a big no no for me. And altering the find_by_foo_and_bar to raise an error on a missing parameter will likely break existing applications.

@isaacsanders

@jeremyf Is this still an issue?

@rafaelfranca
Ruby on Rails member

Yeah. I don't remember but seems like @jonleighton want to remove the dynamic finders in the next version. If so, please close this one @jonleighton

@jeremyf

If we are removing dynamic finders, then I'll close it.

@jeremyf jeremyf closed this Apr 28, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment