Merge pull request #2128 from sikachu/master-dynamic_finder

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

4 files changed, 49 insertions, 16 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index 4113a16c12..ef202a3573 100644
--- a/activerecord/CHANGELOG
+++ b/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.
diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index deff1c65ef..25074c2d4f 100644
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -1052,20 +1052,15 @@ module ActiveRecord #:nodoc:
         # 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 @@ module ActiveRecord #:nodoc:
                 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
diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb
index 3d2a03d2b9..5dc5f99582 100644
--- a/activerecord/test/cases/finder_test.rb
+++ b/activerecord/test/cases/finder_test.rb
@@ -666,6 +666,10 @@ class FinderTest < ActiveRecord::TestCase
     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 @@ class FinderTest < ActiveRecord::TestCase
     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
diff --git a/activerecord/test/cases/named_scope_test.rb b/activerecord/test/cases/named_scope_test.rb
index 2afe3c8f32..ed0240cada 100644
--- a/activerecord/test/cases/named_scope_test.rb
+++ b/activerecord/test/cases/named_scope_test.rb
@@ -497,14 +497,24 @@ end
 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