Skip to content

Loading…

Raise an ArgumentError if user passing less number of argument in the dynamic finder #2128

Merged
merged 3 commits into from

2 participants

@sikachu
Ruby on Rails member

The previous behavior was unintentional, and some people was relying on it. Now the dynamic finder will always expecting the number of arguments to be equal or greater (so you can still pass the options to it.)

So if you were doing this and expecting the second argument to be nil:

    User.find_by_username_and_group("sikachu")

You'll now get ArgumentError: wrong number of arguments (1 for 2). You'll then have to do this:

    User.find_by_username_and_group("sikachu", nil)
sikachu added some commits
@sikachu sikachu Raise an ArgumentError if user passing less number of argument in the…
… dynamic finder

The previous behavior was unintentional, and some people was relying on it. Now the dynamic finder will always expecting the number of arguments to be equal or greater (so you can still pass the options to it.)

So if you were doing this and expecting the second argument to be nil:

    User.find_by_username_and_group("sikachu")

You'll now get `ArgumentError: wrong number of arguments (1 for 2).` You'll then have to do this:

    User.find_by_username_and_group("sikachu", nil)
6e69949
@sikachu sikachu Refactor the code a bit to reduce the duplication 1ccca1b
@sikachu sikachu Refactor test case to use anonymous class - Thank you @tenderlove 4443905
@jonleighton jonleighton merged commit 50941ec into rails:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 17, 2011
  1. @sikachu

    Raise an ArgumentError if user passing less number of argument in the…

    sikachu committed
    … dynamic finder
    
    The previous behavior was unintentional, and some people was relying on it. Now the dynamic finder will always expecting the number of arguments to be equal or greater (so you can still pass the options to it.)
    
    So if you were doing this and expecting the second argument to be nil:
    
        User.find_by_username_and_group("sikachu")
    
    You'll now get `ArgumentError: wrong number of arguments (1 for 2).` You'll then have to do this:
    
        User.find_by_username_and_group("sikachu", nil)
  2. @sikachu
Commits on Jul 18, 2011
  1. @sikachu
View
14 activerecord/CHANGELOG
@@ -1,3 +1,17 @@
+*Rails 3.2.0 (unreleased)*
+
+* Active Record's dynamic finder will now raise the error if you passing in less number of arguments than what you call in method signature.
+
+ So if you were doing this and expecting the second argument to be nil:
+
+ User.find_by_username_and_group("sikachu")
+
+ You'll now get `ArgumentError: wrong number of arguments (1 for 2).` You'll then have to do this:
+
+ User.find_by_username_and_group("sikachu", nil)
+
+ [Prem Sichanugrist]
+
*Rails 3.1.0 (unreleased)*
* ActiveRecord::MacroReflection::AssociationReflection#build_record has a new method signature.
View
23 activerecord/lib/active_record/base.rb
@@ -1052,20 +1052,15 @@ def compute_table_name
# Each dynamic finder using <tt>scoped_by_*</tt> is also defined in the class after it
# is first invoked, so that future attempts to use it do not run through method_missing.
def method_missing(method_id, *arguments, &block)
- if match = DynamicFinderMatch.match(method_id)
+ if match = (DynamicFinderMatch.match(method_id) || DynamicScopeMatch.match(method_id))
attribute_names = match.attribute_names
super unless all_attributes_exists?(attribute_names)
- if match.finder?
- options = arguments.extract_options!
- relation = options.any? ? scoped(options) : scoped
- relation.send :find_by_attributes, match, attribute_names, *arguments, &block
- elsif match.instantiator?
- scoped.send :find_or_instantiator_by_attributes, match, attribute_names, *arguments, &block
+ if arguments.size < attribute_names.size
+ method_trace = "#{__FILE__}:#{__LINE__}:in `#{method_id}'"
+ backtrace = [method_trace] + caller
+ raise ArgumentError, "wrong number of arguments (#{arguments.size} for #{attribute_names.size})", backtrace
end
- elsif match = DynamicScopeMatch.match(method_id)
- attribute_names = match.attribute_names
- super unless all_attributes_exists?(attribute_names)
- if match.scope?
+ if match.respond_to?(:scope?) && match.scope?
self.class_eval <<-METHOD, __FILE__, __LINE__ + 1
def self.#{method_id}(*args) # def self.scoped_by_user_name_and_password(*args)
attributes = Hash[[:#{attribute_names.join(',:')}].zip(args)] # attributes = Hash[[:user_name, :password].zip(args)]
@@ -1074,6 +1069,12 @@ def self.#{method_id}(*args) # def self.scope
end # end
METHOD
send(method_id, *arguments)
+ elsif match.finder?
+ options = arguments.extract_options!
+ relation = options.any? ? scoped(options) : scoped
+ relation.send :find_by_attributes, match, attribute_names, *arguments, &block
+ elsif match.instantiator?
+ scoped.send :find_or_instantiator_by_attributes, match, attribute_names, *arguments, &block
end
else
super
View
8 activerecord/test/cases/finder_test.rb
@@ -666,6 +666,10 @@ def test_find_by_two_attributes
assert_nil Topic.find_by_title_and_author_name("The First Topic", "Mary")
end
+ def test_find_by_two_attributes_but_passing_only_one
+ assert_raise(ArgumentError) { Topic.find_by_title_and_author_name("The First Topic") }
+ end
+
def test_find_last_by_one_attribute
assert_equal Topic.last, Topic.find_last_by_title(Topic.last.title)
assert_nil Topic.find_last_by_title("A title with no matches")
@@ -947,6 +951,10 @@ def test_find_or_initialize_from_two_attributes
assert !another.persisted?
end
+ def test_find_or_initialize_from_two_attributes_but_passing_only_one
+ assert_raise(ArgumentError) { Topic.find_or_initialize_by_title_and_author_name("Another topic") }
+ end
+
def test_find_or_initialize_from_one_aggregate_attribute_and_one_not
new_customer = Customer.find_or_initialize_by_balance_and_name(Money.new(123), "Elizabeth")
assert_equal 123, new_customer.balance.amount
View
20 activerecord/test/cases/named_scope_test.rb
@@ -497,14 +497,24 @@ def test_scoped_by
class DynamicScopeTest < ActiveRecord::TestCase
fixtures :posts
+ def setup
+ @test_klass = Class.new(Post) do
+ def self.name; "Post"; end
+ end
+ end
+
def test_dynamic_scope
- assert_equal Post.scoped_by_author_id(1).find(1), Post.find(1)
- assert_equal Post.scoped_by_author_id_and_title(1, "Welcome to the weblog").first, Post.find(:first, :conditions => { :author_id => 1, :title => "Welcome to the weblog"})
+ assert_equal @test_klass.scoped_by_author_id(1).find(1), @test_klass.find(1)
+ assert_equal @test_klass.scoped_by_author_id_and_title(1, "Welcome to the weblog").first, @test_klass.find(:first, :conditions => { :author_id => 1, :title => "Welcome to the weblog"})
end
def test_dynamic_scope_should_create_methods_after_hitting_method_missing
- assert_blank Developer.methods.grep(/scoped_by_created_at/)
- Developer.scoped_by_created_at(nil)
- assert_present Developer.methods.grep(/scoped_by_created_at/)
+ assert_blank @test_klass.methods.grep(/scoped_by_type/)
+ @test_klass.scoped_by_type(nil)
+ assert_present @test_klass.methods.grep(/scoped_by_type/)
+ end
+
+ def test_dynamic_scope_with_less_number_of_arguments
+ assert_raise(ArgumentError){ @test_klass.scoped_by_author_id_and_title(1) }
end
end
Something went wrong with that request. Please try again.