Fix the HasManyAssociation#count method so it uses the new ActiveRecord::Base#count syntax, while maintaining backwards compatibility. [Rick]

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@4237 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

5 files changed, 50 insertions, 25 deletions

diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 52cad79484..56368f4198 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -36,15 +36,18 @@ module ActiveRecord
       end
 
       # Count the number of associated records. All arguments are optional.
-      def count(runtime_conditions = nil)
+      def count(*args)
         if @reflection.options[:counter_sql]
           @reflection.klass.count_by_sql(@counter_sql)
         elsif @reflection.options[:finder_sql]
           @reflection.klass.count_by_sql(@finder_sql)
         else
-          sql = @finder_sql
-          sql += " AND (#{sanitize_sql(runtime_conditions)})" if runtime_conditions
-          @reflection.klass.count(sql)
+          column_name, options = @reflection.klass.send(:construct_count_options_from_legacy_args, *args)          
+          options[:conditions] = options[:conditions].nil? ?
+            @finder_sql :
+            @finder_sql + " AND (#{sanitize_sql(options[:conditions])})"
+
+          @reflection.klass.count(column_name, options)
         end
       end
 
diff --git a/activerecord/lib/active_record/calculations.rb b/activerecord/lib/active_record/calculations.rb
index 863aedbf9c..acab77870f 100644
--- a/activerecord/lib/active_record/calculations.rb
+++ b/activerecord/lib/active_record/calculations.rb
@@ -42,23 +42,7 @@ module ActiveRecord
       #
       # Note: Person.count(:all) will not work because it will use :all as the condition.  Use Person.count instead.
       def count(*args)
-        options     = {}
-        column_name = :all
-        # For backwards compatibility, we need to handle both count(conditions=nil, joins=nil) or count(options={}) or count(column_name=:all, options={}).
-        if args.size >= 0 && args.size <= 2
-          if args.first.is_a?(Hash)
-            options     = args.first
-          elsif args[1].is_a?(Hash)
-            options     = args[1]
-            column_name = args.first
-          else
-            # Handle legacy paramter options: def count(conditions=nil, joins=nil)
-            options.merge!(:conditions => args[0]) if args.length > 0
-            options.merge!(:joins => args[1])      if args.length > 1
-          end
-        else
-          raise(ArgumentError, "Unexpected parameters passed to count(*args): expected either count(conditions=nil, joins=nil) or count(options={})")
-        end
+        column_name, options = construct_count_options_from_legacy_args(*args)
 
         if options[:include] || scope(:find, :include)
           count_with_associations(options)
@@ -146,6 +130,27 @@ module ActiveRecord
       end
 
       protected
+        def construct_count_options_from_legacy_args(*args)
+          options     = {}
+          column_name = :all
+          # For backwards compatibility, we need to handle both count(conditions=nil, joins=nil) or count(options={}) or count(column_name=:all, options={}).
+          if args.size >= 0 && args.size <= 2
+            if args.first.is_a?(Hash)
+              options     = args.first
+            elsif args[1].is_a?(Hash)
+              options     = args[1]
+              column_name = args.first
+            else
+              # Handle legacy paramter options: def count(conditions=nil, joins=nil)
+              options.merge!(:conditions => args[0]) if args.length > 0
+              options.merge!(:joins => args[1])      if args.length > 1
+            end
+          else
+            raise(ArgumentError, "Unexpected parameters passed to count(*args): expected either count(conditions=nil, joins=nil) or count(options={})")
+          end
+          [column_name, options]
+        end
+      
         def construct_calculation_sql(aggregate, aggregate_alias, options) #:nodoc:
           scope = scope(:find)
           sql  = "SELECT #{aggregate} AS #{aggregate_alias}"
diff --git a/activerecord/test/associations_test.rb b/activerecord/test/associations_test.rb
index 173808c370..f94908ced1 100755
--- a/activerecord/test/associations_test.rb
+++ b/activerecord/test/associations_test.rb
@@ -283,10 +283,26 @@ class HasManyAssociationsTest < Test::Unit::TestCase
     companies(:first_firm).clients_of_firm.each {|f| }
   end
 
-  def test_counting
+  def test_counting_with_counter_sql
     assert_equal 2, Firm.find(:first).clients.count
   end
-  
+
+  def test_counting
+    assert_equal 2, Firm.find(:first).plain_clients.count
+  end
+
+  def test_counting_with_single_conditions
+    assert_equal 2, Firm.find(:first).plain_clients.count('1=1')
+  end
+
+  def test_counting_with_single_hash
+    assert_equal 2, Firm.find(:first).plain_clients.count(:conditions => '1=1')
+  end
+
+  def test_counting_with_column_name_and_hash
+    assert_equal 2, Firm.find(:first).plain_clients.count(:all, :conditions => '1=1')
+  end
+
   def test_finding
     assert_equal 2, Firm.find(:first).clients.length
   end
diff --git a/activerecord/test/fixtures/company.rb b/activerecord/test/fixtures/company.rb
index 59a7cdd13e..526748784e 100755
--- a/activerecord/test/fixtures/company.rb
+++ b/activerecord/test/fixtures/company.rb
@@ -28,6 +28,7 @@ class Firm < Company
   has_many :no_clients_using_counter_sql, :class_name => "Client",
            :finder_sql  => 'SELECT * FROM companies WHERE client_of = 1000',
            :counter_sql => 'SELECT COUNT(*) FROM companies WHERE client_of = 1000'
+  has_many :plain_clients, :class_name => 'Client'
 
   has_one :account, :foreign_key => "firm_id", :dependent => :destroy
 end
diff --git a/activerecord/test/reflection_test.rb b/activerecord/test/reflection_test.rb
index 64689595fe..4882ca246f 100644
--- a/activerecord/test/reflection_test.rb
+++ b/activerecord/test/reflection_test.rb
@@ -137,8 +137,8 @@ class ReflectionTest < Test::Unit::TestCase
   end
   
   def test_reflection_of_all_associations
-    assert_equal 13, Firm.reflect_on_all_associations.size
-    assert_equal 11, Firm.reflect_on_all_associations(:has_many).size
+    assert_equal 14, Firm.reflect_on_all_associations.size
+    assert_equal 12, Firm.reflect_on_all_associations(:has_many).size
     assert_equal 2, Firm.reflect_on_all_associations(:has_one).size
     assert_equal 0, Firm.reflect_on_all_associations(:belongs_to).size
   end