find_by_ bang methods break if the model has an empty? method on Rails 3-2-x #7238

Closed
knaveofdiamonds opened this Issue Aug 2, 2012 · 7 comments

5 participants

@knaveofdiamonds

Given an ActiveRecord model like:

class Topic < ActiveRecord::Base
  def empty?
     true # actual implementation may be on stories in topic or similar
  end
end

Calling a method like find_by_name("A topic") will successfully return the topic, whereas the bang version, find_by_name!("A topic"), will raise a RecordNotFound exception. This is due to find_by_attributes checking result.blank? instead of result.nil?

@knaveofdiamonds

Start of a fix is here: https://github.com/knaveofdiamonds/rails/tree/fix_find_by_bang_error

Adding an empty? method to Topic also seems to cause association presence validation to fail, which I haven't addressed yet.

@pixeltrix
Ruby on Rails member

The reason the blank? is used is because the result may be an empty array when you're using find_all_by_name! - any fix needs to take account of that.

@senny
Ruby on Rails member

@knaveofdiamonds any progress on the patch?

@nikitug nikitug added a commit to nikitug/rails that referenced this issue Nov 11, 2012
@nikitug nikitug Use nil? instead of blank? to check dynamic finder result
It's safe to use `nil?` instead of `blank?` because it's impossible to get an array on finder with bang; `all_by` finder matches against regex without bang: `when /^find_(all_|last_)?by_([_a-zA-Z]\w*)$/`.

Fixes #7238
ff217ef
@nikitug

@pixeltrix @knaveofdiamonds it seems fine to use nil? instead of blank?, check out @ff217ef

@nikitug

Also it's not a problem in master because as I said there is no way to use all_by and bang simultaneously, and first! also checks for nil? neither than blank?.

@rafaelfranca
Ruby on Rails member

@nikitug have you already opened pull request with the fix to 3-2-stable and the regression tests to master? If not please do it.

@nikitug nikitug added a commit to nikitug/rails that referenced this issue Nov 13, 2012
@nikitug nikitug Regression test for #7238 8424d0d
@nikitug

@rafaelfranca was waiting for some feedback. Just did it

@rafaelfranca rafaelfranca pushed a commit that closed this issue Nov 13, 2012
@nikitug nikitug Use nil? instead of blank? to check dynamic finder result
It's safe to use `nil?` instead of `blank?` because it's impossible to get an array on finder with bang;
`all_by` finder matches against regex without bang: `when /^find_(all_|last_)?by_([_a-zA-Z]\w*)$/`.

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