Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Use nil? instead of blank? to check dynamic finder result #8204

Merged
merged 1 commit into from

3 participants

@nikitug

It's safe to use nil? instead of blank? because it's impossible to get an array on finder with bang; all_by fi

Fixes #7238

@carlosantoniodasilva

It needs a changelog entry. Also, I think it'll be good to have the tests on master as well, so that we can ensure it works perfectly fine there. Thanks!

@carlosantoniodasilva

Sorry, just noticed the other pull request for master :smile:, thanks!

@carlosantoniodasilva

I think it'll need a rebase - again due to the changelog :smile:

@carlosantoniodasilva

Can you also do like this, removing the nothing raised thing? https://github.com/rails/rails/pull/8202/files#r2112067 Thanks.

@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
@rafaelfranca rafaelfranca merged commit b56376b into rails:3-2-stable
@rafaelfranca

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 13, 2012
  1. @nikitug

    Use nil? instead of blank? to check dynamic finder result

    nikitug authored
    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
This page is out of date. Refresh to see the latest.
View
5 activerecord/CHANGELOG.md
@@ -1,5 +1,10 @@
## Rails 3.2.10 (unreleased)
+* Use `nil?` instead of `blank?` to check whether dynamic finder with a bang
+ should raise RecordNotFound. Fixes #7238.
+
+ *Nikita Afanasenko*
+
* Use query cache/uncache when using ENV["DATABASE_URL"].
Fixes #6951. [Backport #8074]
View
2  activerecord/lib/active_record/relation/finder_methods.rb
@@ -263,7 +263,7 @@ def find_by_attributes(match, attributes, *args)
conditions = Hash[attributes.map {|a| [a, args[attributes.index(a)]]}]
result = where(conditions).send(match.finder)
- if match.bang? && result.blank?
+ if match.bang? && result.nil?
raise RecordNotFound, "Couldn't find #{@klass.name} with #{conditions.to_a.collect {|p| p.join(' = ')}.join(', ')}"
else
yield(result) if block_given?
View
5 activerecord/test/cases/finder_test.rb
@@ -652,6 +652,11 @@ def test_find_by_one_attribute_bang
assert_raise(ActiveRecord::RecordNotFound) { Topic.find_by_title!("The First Topic!") }
end
+ def test_find_by_one_attribute_bang_with_blank_defined
+ blank_topic = BlankTopic.create(:title => "The Blank One")
+ assert_equal blank_topic, BlankTopic.find_by_title!("The Blank One")
+ end
+
def test_find_by_one_attribute_with_order_option
assert_equal accounts(:signals37), Account.find_by_credit_limit(50, :order => 'id')
assert_equal accounts(:rails_core_account), Account.find_by_credit_limit(50, :order => 'id DESC')
View
6 activerecord/test/models/topic.rb
@@ -112,6 +112,12 @@ class ImportantTopic < Topic
serialize :important, Hash
end
+class BlankTopic < Topic
+ def blank?
+ true
+ end
+end
+
module Web
class Topic < ActiveRecord::Base
has_many :replies, :dependent => :destroy, :foreign_key => "parent_id", :class_name => 'Web::Reply'
Something went wrong with that request. Please try again.