Dynamic Finder Deprecation Notice on documented behavior #2404

Closed
baroquebobcat opened this Issue Aug 2, 2011 · 12 comments

Projects

None yet

7 participants

@baroquebobcat

I upgraded a project from rc4 to rc5 and got a new deprecation notice.

When I call a dynamic finder with one argument that is a hash, eg:

  Post.find_or_create_by_title_and_author :title => some_title, :author => some_author

I get a warning about it

DEPRECATION WARNING: Calling dynamic finder with less number of arguments than the number of attributes in method name is deprecated and will raise an ArguementError in the next version of Rails. Please passing `nil' to the argument you want it to be nil.

Calling a dynamic finder with a hash is a documented behavior:

  # To find by a subset of the attributes to be used for instantiating a new object, pass a hash instead of
  # a list of parameters.
  #
  # Tag.find_or_create_by_name(:name => "rails", :creator => current_user)

https://github.com/sikachu/rails/blob/fa7dd345c306f2a3605c8148c30194d9b721e1a3/activerecord/lib/active_record/base.rb#L215

The warning was added in #2129

@spastorino
Ruby on Rails member
@vijaydev
Ruby on Rails member

@sikachu: On a diff note, we should change "Please passing `nil' to the argument you want it to be nil" to "Please pass nil to the missing argument" (or something to that effect).

@sikachu
Ruby on Rails member

Ha! Ok, that's the regression then. I'll work on it.

@sikachu sikachu was assigned Aug 3, 2011
@jonleighton
Ruby on Rails member

I think we should have a method find_or_create which takes a hash for this use case, and then deprecate this method as it is totally illogical and confusing.

However, this would technically be a new feature so I think it's too late for 3.1.

So I think we should:

  • Support this behaviour without warnings in 3-1-stable
  • Deprecate this behaviour and implement a find_or_create method in master

Happy to review any patches if you can work on it @sikachu.

@jonleighton
Ruby on Rails member

To be clear, I am suggesting that we deprecate find_or_create_by_foo taking a hash, not deprecate find_or_create_by_foo methods entirely (although I would be in favour of that if we had find_or_create, but that's a different question).

@baroquebobcat

How would find_or_create with a hash specify which attributes are keys and which are to be used to fill in values when lookup fails? Maybe use the current block syntax?

Baz.find_or_create :foo => 1, :bar => 2 do |baz|
  baz.quux = :awesome
end

The problem I see is that cases where someone passes a params hash to find_or_create_by_username_and_email, where they want to create the new one with the non-matching options. Requiring a block might make that feel a little awkward.

User.find_or_create params[:user].slice(:username,:email) do |user|
  user.coupon = params[:user][:coupon]
  user.first_awesome_thing = params[:user][:awesome_thing]
# or
  user.attributes= params[:user]
end

On the other hand, maybe it should feel awkward, because it's an odd thing to do, and you want people to be sure that's the behavior they want.

@jonleighton
Ruby on Rails member

IMO we should not have a way for users to find with one set of attributes and then create with a superset. That seems like a really weird feature to me, and I would rather users just type the code explicitly if they want to do that.

So I would envisage find_or_create finding or creating with exactly the same attributes.

How does Tag.find_or_create_by_name(:name => "rails", :creator => current_user) work now? Is it:

  1. Tag.where(:name => "rails").first || Tag.create(name => "rails", :creator => current_user), or
  2. Tag.where(:name => "rails", :creator => current_user).first || Tag.create(name => "rails", :creator => current_user)

Whichever it is, both are messed up in different ways IMO ;)

@tilsammans

To answer your question Jon, it's the former.

No argument from me about its messed-up-edness.

@tenderlove tenderlove closed this in c1b85ed Aug 3, 2011
@tenderlove
Ruby on Rails member

@jonleighton please make the find_or_create on master, or open a ticket for Rails 3.2 so we don't forget. Thanks!

@baroquebobcat

Thanks for resolving this so quickly. You guys rock!

@jonleighton
Ruby on Rails member

Done #2420

@sikachu
Ruby on Rails member

I'll work on that too. Thank you @tenderlove for fixing this one up for me. 👍

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