From 6e6994994d9a8edea33720f0da32b04f9a0efa2f Mon Sep 17 00:00:00 2001 From: Prem Sichanugrist Date: Sun, 17 Jul 2011 18:41:02 -0400 Subject: [PATCH 1/3] 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) --- activerecord/CHANGELOG | 14 ++++++++++++++ activerecord/lib/active_record/base.rb | 10 ++++++++++ activerecord/test/cases/finder_test.rb | 8 ++++++++ activerecord/test/cases/named_scope_test.rb | 15 +++++++++++++++ 4 files changed, 47 insertions(+) diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index 4113a16c12003..ef202a3573fbd 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 deff1c65ef774..5808950715bda 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1055,6 +1055,11 @@ def method_missing(method_id, *arguments, &block) if match = DynamicFinderMatch.match(method_id) attribute_names = match.attribute_names super unless all_attributes_exists?(attribute_names) + 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 if match.finder? options = arguments.extract_options! relation = options.any? ? scoped(options) : scoped @@ -1065,6 +1070,11 @@ def method_missing(method_id, *arguments, &block) elsif match = DynamicScopeMatch.match(method_id) attribute_names = match.attribute_names super unless all_attributes_exists?(attribute_names) + 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 if match.scope? self.class_eval <<-METHOD, __FILE__, __LINE__ + 1 def self.#{method_id}(*args) # def self.scoped_by_user_name_and_password(*args) diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb index 3d2a03d2b9a55..5dc5f99582599 100644 --- a/activerecord/test/cases/finder_test.rb +++ b/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 diff --git a/activerecord/test/cases/named_scope_test.rb b/activerecord/test/cases/named_scope_test.rb index 2afe3c8f32076..01893e4dc37d6 100644 --- a/activerecord/test/cases/named_scope_test.rb +++ b/activerecord/test/cases/named_scope_test.rb @@ -497,6 +497,17 @@ def test_scoped_by class DynamicScopeTest < ActiveRecord::TestCase fixtures :posts + def setup + # Ensure we start with a clean model with no generated class method + Post.methods.select{ |c| c =~ /scoped_by_/ }.each do |method_name| + Post.class_eval <<-RUBY + class << self + remove_method :#{method_name} + end + RUBY + 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"}) @@ -507,4 +518,8 @@ def test_dynamic_scope_should_create_methods_after_hitting_method_missing Developer.scoped_by_created_at(nil) assert_present Developer.methods.grep(/scoped_by_created_at/) end + + def test_dynamic_scope_with_less_number_of_arguments + assert_raise(ArgumentError){ Post.scoped_by_author_id_and_title(1) } + end end From 1ccca1b9cb4c360b6d9784663da542df34ed3773 Mon Sep 17 00:00:00 2001 From: Prem Sichanugrist Date: Sun, 17 Jul 2011 18:53:15 -0400 Subject: [PATCH 2/3] Refactor the code a bit to reduce the duplication --- activerecord/lib/active_record/base.rb | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 5808950715bda..25074c2d4fd6f 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1052,7 +1052,7 @@ def compute_table_name # Each dynamic finder using scoped_by_* 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 arguments.size < attribute_names.size @@ -1060,22 +1060,7 @@ def method_missing(method_id, *arguments, &block) backtrace = [method_trace] + caller raise ArgumentError, "wrong number of arguments (#{arguments.size} for #{attribute_names.size})", backtrace end - 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 - end - elsif match = DynamicScopeMatch.match(method_id) - attribute_names = match.attribute_names - super unless all_attributes_exists?(attribute_names) - 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 - 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)] @@ -1084,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 From 4443905169b54845090b7f8f863dfc820513e469 Mon Sep 17 00:00:00 2001 From: Prem Sichanugrist Date: Sun, 17 Jul 2011 22:49:04 -0400 Subject: [PATCH 3/3] Refactor test case to use anonymous class - Thank you @tenderlove --- activerecord/test/cases/named_scope_test.rb | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/activerecord/test/cases/named_scope_test.rb b/activerecord/test/cases/named_scope_test.rb index 01893e4dc37d6..ed0240cada022 100644 --- a/activerecord/test/cases/named_scope_test.rb +++ b/activerecord/test/cases/named_scope_test.rb @@ -498,28 +498,23 @@ class DynamicScopeTest < ActiveRecord::TestCase fixtures :posts def setup - # Ensure we start with a clean model with no generated class method - Post.methods.select{ |c| c =~ /scoped_by_/ }.each do |method_name| - Post.class_eval <<-RUBY - class << self - remove_method :#{method_name} - end - RUBY + @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){ Post.scoped_by_author_id_and_title(1) } + assert_raise(ArgumentError){ @test_klass.scoped_by_author_id_and_title(1) } end end