* Fix pagination problems when using include

* Introduce Unit Tests for pagination
* Allow count to work with :include by using count distinct.

[Kevin Clark & Jeremy Hopple]


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

actionpack/CHANGELOG

@@ -1,5 +1,11 @@
 *SVN*
 
+* Fix problems with pagination and :include.  [Kevin Clark]
+
+* Add ActiveRecordTestCase for testing AR integration. [Kevin Clark]
+
+* Add Unit Tests for pagination [Kevin Clark]
+
 * Add :html option for specifying form tag options in form_for. [Sam Stephenson]
 
 * Replace dubious controller parent class in filter docs. #3655, #3722  [info@rhalff.com, eigentone@gmail.com]

actionpack/Rakefile

@@ -31,6 +31,13 @@ Rake::TestTask.new { |t|
   t.verbose = true
 }
 
+desc 'ActiveRecord Integration Tests'
+Rake::TestTask.new(:test_active_record_integration) do |t|
+  t.libs << "test"
+  t.test_files = Dir.glob("test/activerecord/*_test.rb")
+  t.verbose = true
+end
+
 
 # Genereate the RDoc documentation
 

actionpack/lib/action_controller/pagination.rb

@@ -163,10 +163,11 @@ module ActionController
     # Returns the total number of items in the collection to be paginated for
     # the +model+ and given +conditions+. Override this method to implement a
     # custom counter.
-    def count_collection_for_pagination(model, conditions, joins)
-      model.count(conditions,joins)
+    def count_collection_for_pagination(model, options)
+      model.count(:conditions => options[:conditions],
+                  :joins => options[:join] || options[:joins], :include => options[:include])
     end
-  
+    
     # Returns a collection of items for the given +model+ and +options[conditions]+,
     # ordered by +options[order]+, for the current page in the given +paginator+.
     # Override this method to implement a custom finder.
@@ -185,12 +186,10 @@ module ActionController
     def paginator_and_collection_for(collection_id, options) #:nodoc:
       klass = options[:class_name].constantize
       page  = @params[options[:parameter]]
-      count = count_collection_for_pagination(klass, options[:conditions],
-                                              options[:join] || options[:joins])
-
+      count = count_collection_for_pagination(klass, options)
       paginator = Paginator.new(self, count, options[:per_page], page)
       collection = find_collection_for_pagination(klass, options, paginator)
-      
+    
       return paginator, collection 
     end
       

actionpack/test/active_record_unit.rb

@@ -0,0 +1,88 @@
+require  File.dirname(__FILE__) + '/abstract_unit'
+
+# Define the essentials
+class ActiveRecordTestConnector
+  cattr_accessor :able_to_connect
+  cattr_accessor :connected
+  
+  # Set our defaults
+  self.connected = false
+  self.able_to_connect = true
+end
+
+# Try to grab AR
+begin
+  PATH_TO_AR = File.dirname(__FILE__) + '/../../activerecord'
+  require "#{PATH_TO_AR}/lib/active_record" unless Object.const_defined?(:ActiveRecord)
+  require "#{PATH_TO_AR}/lib/active_record/fixtures" unless Object.const_defined?(:Fixtures)
+rescue Object => e
+  $stderr.puts "\nSkipping ActiveRecord assertion tests: #{e}"
+  ActiveRecordTestConnector.able_to_connect = false
+end
+
+# Define the rest of the connector
+class ActiveRecordTestConnector  
+  def self.setup
+    unless self.connected || !self.able_to_connect
+      setup_connection
+      load_schema
+      self.connected = true
+    end
+  rescue Object => e
+    $stderr.puts "\nSkipping ActiveRecord assertion tests: #{e}"
+    #$stderr.puts "  #{e.backtrace.join("\n  ")}\n"
+    self.able_to_connect = false
+  end
+  
+  private
+  
+  def self.setup_connection
+    if Object.const_defined?(:ActiveRecord)
+          
+      begin
+        ActiveRecord::Base.establish_connection(:adapter => 'sqlite3', :dbfile => ':memory:')
+        ActiveRecord::Base.connection
+      rescue Object
+        $stderr.puts 'SQLite 3 unavailable; falling to SQLite 2.'
+        ActiveRecord::Base.establish_connection(:adapter => 'sqlite', :dbfile => ':memory:')
+        ActiveRecord::Base.connection
+      end
+    
+      Object.send(:const_set, :QUOTED_TYPE, ActiveRecord::Base.connection.quote_column_name('type')) unless Object.const_defined?(:QUOTED_TYPE)
+    else
+      raise "Couldn't locate ActiveRecord."
+    end
+  end
+  
+  # Load actionpack sqlite tables
+  def self.load_schema
+    File.read(File.dirname(__FILE__) + "/fixtures/db_definitions/sqlite.sql").split(';').each do |sql|
+      ActiveRecord::Base.connection.execute(sql) unless sql.blank?
+    end
+  end
+end
+  
+# Test case for inheiritance  
+class ActiveRecordTestCase < Test::Unit::TestCase
+  # Set our fixture path
+  self.fixture_path = "#{File.dirname(__FILE__)}/fixtures/"
+  
+  def setup
+    abort_tests unless ActiveRecordTestConnector.connected = true
+  end
+  
+  # Default so Test::Unit::TestCase doesn't complain
+  def test_truth
+  end
+  
+  private
+  
+  # If things go wrong, we don't want to run our test cases. We'll just define them to test nothing.
+  def abort_tests
+    self.class.public_instance_methods.grep(/^test./).each do |method|
+      self.class.class_eval { define_method(method.to_sym){} }
+    end
+  end
+end
+
+ActiveRecordTestConnector.setup

actionpack/test/activerecord/active_record_assertions_test.rb

@@ -1,45 +1,6 @@
-require "#{File.dirname(__FILE__)}/../abstract_unit"
-
-# Unfurl the safety net.
-path_to_ar = File.dirname(__FILE__) + '/../../../activerecord'
-if Object.const_defined?(:ActiveRecord) || File.exist?(path_to_ar)
-  begin
-
-# These tests require Active Record, so you're going to need AR in a
-# sibling directory to AP and have SQLite installed.
-
-unless Object.const_defined?(:ActiveRecord)
-  require "#{path_to_ar}/lib/active_record"
-end
-
-begin
-  ActiveRecord::Base.establish_connection(:adapter => 'sqlite3', :dbfile => ':memory:')
-  ActiveRecord::Base.connection
-rescue Object
-  $stderr.puts 'SQLite 3 unavailable; falling to SQLite 2.'
-  ActiveRecord::Base.establish_connection(:adapter => 'sqlite', :dbfile => ':memory:')
-  ActiveRecord::Base.connection
-end
-
-# Set up company fixtures.
-$LOAD_PATH << "#{path_to_ar}/test"
-QUOTED_TYPE = ActiveRecord::Base.connection.quote_column_name('type') unless Object.const_defined?(:QUOTED_TYPE)
+require "#{File.dirname(__FILE__)}/../active_record_unit"
 require 'fixtures/company'
-File.read("#{path_to_ar}/test/fixtures/db_definitions/sqlite.sql").split(';').each do |sql|
-  ActiveRecord::Base.connection.execute(sql) unless sql.blank?
-end
 
-# Add some validation rules to trip up the assertions.
-class Company
-  protected
-    def validate
-      errors.add_on_empty('name')
-      errors.add('rating', 'rating should not be 2') if rating == 2
-      errors.add_to_base('oh oh') if rating == 3
-    end  
-end
-
-# A controller to host the assertions.
 class ActiveRecordAssertionsController < ActionController::Base
   self.template_root = "#{File.dirname(__FILE__)}/../fixtures/"
 
@@ -76,12 +37,15 @@ class ActiveRecordAssertionsController < ActionController::Base
   # the safety dance......
   def rescue_action(e) raise; end
 end
-                    
-class ActiveRecordAssertionsControllerTest < Test::Unit::TestCase
+                  
+class ActiveRecordAssertionsControllerTest < ActiveRecordTestCase
+  fixtures :companies
+  
   def setup
     @request = ActionController::TestRequest.new
     @response = ActionController::TestResponse.new
     @controller = ActiveRecordAssertionsController.new
+    super
   end
 
   # test for 1 bad apple column
@@ -117,11 +81,4 @@ class ActiveRecordAssertionsControllerTest < Test::Unit::TestCase
     assert_success
     assert_invalid_record 'company'
   end
-end 
-
-# End of safety net.
-  rescue Object => e
-    $stderr.puts "Skipping Active Record assertion tests: #{e}"
-    #$stderr.puts "  #{e.backtrace.join("\n  ")}"
-  end
-end
+end

actionpack/test/activerecord/pagination_test.rb

@@ -0,0 +1,146 @@
+require File.dirname(__FILE__) + '/../active_record_unit'
+
+require 'fixtures/topic'
+require 'fixtures/reply'
+require 'fixtures/developer'
+require 'fixtures/project'
+
+class PaginationTest < ActiveRecordTestCase
+  fixtures :topics, :replies, :developers, :projects, :developers_projects
+
+  class PaginationController < ActionController::Base
+    self.template_root = "#{File.dirname(__FILE__)}/../fixtures/"
+    
+    def simple_paginate
+      @topic_pages, @topics = paginate(:topics)
+      render :nothing => true
+    end
+    
+    def paginate_with_per_page
+      @topic_pages, @topics = paginate(:topics, :per_page => 1)
+      render :nothing => true
+    end
+    
+    def paginate_with_order
+      @topic_pages, @topics = paginate(:topics, :order => 'created_at asc')
+      render :nothing => true
+    end
+    
+    def paginate_with_order_by
+      @topic_pages, @topics = paginate(:topics, :order_by => 'created_at asc')
+      render :nothing => true
+    end
+    
+    def paginate_with_include_and_order
+      @topic_pages, @topics = paginate(:topics, :include => :replies, :order => 'replies.created_at asc, topics.created_at asc')
+      render :nothing => true
+    end
+    
+    def paginate_with_conditions
+      @topic_pages, @topics = paginate(:topics, :conditions => ["created_at > ?", 30.minutes.ago])
+      render :nothing => true
+    end
+    
+    def paginate_with_class_name
+      @developer_pages, @developers = paginate(:developers, :class_name => "DeVeLoPeR")
+      render :nothing => true
+    end
+    
+    def paginate_with_singular_name
+      @developer_pages, @developers = paginate()
+      render :nothing => true
+    end
+    
+    def paginate_with_joins
+      @developer_pages, @developers = paginate(:developers, 
+                                             :joins => 'LEFT JOIN developers_projects ON developers.id = developers_projects.developer_id',
+                                             :conditions => 'project_id=1')        
+      render :nothing => true
+    end
+    
+    def paginate_with_join
+      @developer_pages, @developers = paginate(:developers, 
+                                             :join => 'LEFT JOIN developers_projects ON developers.id = developers_projects.developer_id',
+                                             :conditions => 'project_id=1')        
+      render :nothing => true
+    end
+    
+    def rescue_errors(e) raise e end
+
+    def rescue_action(e) raise end
+    
+  end
+  
+  def setup
+    @controller = PaginationController.new
+    @request    = ActionController::TestRequest.new
+    @response   = ActionController::TestResponse.new
+    super
+  end
+
+  # Single Action Pagination Tests
+
+  def test_simple_paginate
+    get :simple_paginate
+    assert_equal 1, assigns(:topic_pages).page_count
+    assert_equal 3, assigns(:topics).size
+  end
+  
+  def test_paginate_with_per_page
+    get :paginate_with_per_page
+    assert_equal 1, assigns(:topics).size
+    assert_equal 3, assigns(:topic_pages).page_count
+  end
+  
+  def test_paginate_with_order
+    get :paginate_with_order
+    expected = [topics(:futurama),
+               topics(:harvey_birdman),
+               topics(:rails)]
+    assert_equal expected, assigns(:topics)
+    assert_equal 1, assigns(:topic_pages).page_count
+  end
+  
+  def test_paginate_with_order_by
+    get :paginate_with_order
+    expected = assigns(:topics)
+    get :paginate_with_order_by
+    assert_equal expected, assigns(:topics)  
+    assert_equal 1, assigns(:topic_pages).page_count    
+  end
+  
+  def test_paginate_with_conditions
+    get :paginate_with_conditions
+    expected = [topics(:rails)]
+    assert_equal expected, assigns(:topics)
+    assert_equal 1, assigns(:topic_pages).page_count
+  end
+  
+  def test_paginate_with_class_name
+    get :paginate_with_class_name
+    
+    assert assigns(:developers).size > 0
+    assert_equal DeVeLoPeR, assigns(:developers).first.class
+  end
+      
+  def test_paginate_with_joins
+    get :paginate_with_joins
+    assert_equal 2, assigns(:developers).size
+    developer_names = assigns(:developers).map { |d| d.name }
+    assert developer_names.include?('David')
+    assert developer_names.include?('Jamis')
+  end
+  
+  def test_paginate_with_join_and_conditions
+    get :paginate_with_joins
+    expected = assigns(:topics)
+    get :paginate_with_join
+    assert_equal expected, assigns(:topics)      
+  end
+  
+  def test_paginate_with_include_and_order
+    get :paginate_with_include_and_order
+    expected = Topic.find(:all, :include => 'replies', :order => 'replies.created_at asc, topics.created_at asc', :limit => 10)
+    assert_equal expected, assigns(:topics)
+  end
+end

actionpack/test/fixtures/companies.yml

@@ -0,0 +1,24 @@
+thirty_seven_signals:
+  id: 1
+  name: 37Signals
+  rating: 4
+
+TextDrive:
+  id: 2
+  name: TextDrive
+  rating: 4
+
+PlanetArgon:
+  id: 3
+  name: Planet Argon
+  rating: 4
+
+Google:
+  id: 4
+  name: Google
+  rating: 4
+  
+Ionist:
+  id: 5
+  name: Ioni.st
+  rating: 4

actionpack/test/fixtures/company.rb

@@ -0,0 +1,9 @@
+class Company < ActiveRecord::Base
+  attr_protected :rating
+  set_sequence_name :companies_nonstd_seq
+
+  validates_presence_of :name
+  def validate
+    errors.add('rating', 'rating should not be 2') if rating == 2
+  end  
+end

actionpack/test/fixtures/db_definitions/sqlite.sql

@@ -0,0 +1,42 @@
+CREATE TABLE 'companies' (
+  'id' INTEGER PRIMARY KEY NOT NULL,
+  'name' TEXT DEFAULT NULL,
+  'rating' INTEGER DEFAULT 1
+);
+
+CREATE TABLE 'replies' (
+  'id' INTEGER PRIMARY KEY NOT NULL, 
+  'content' text, 
+  'created_at' datetime, 
+  'updated_at' datetime, 
+  'topic_id' integer
+);
+
+CREATE TABLE 'topics' (
+  'id' INTEGER PRIMARY KEY NOT NULL, 
+  'title' varchar(255), 
+  'subtitle' varchar(255), 
+  'content' text, 
+  'created_at' datetime, 
+  'updated_at' datetime
+);
+
+CREATE TABLE 'developers' (
+  'id' INTEGER PRIMARY KEY NOT NULL,
+  'name' TEXT DEFAULT NULL,
+  'salary' INTEGER DEFAULT 70000,
+  'created_at' DATETIME DEFAULT NULL,
+  'updated_at' DATETIME DEFAULT NULL
+);
+
+CREATE TABLE 'projects' (
+  'id' INTEGER PRIMARY KEY NOT NULL,
+  'name' TEXT DEFAULT NULL
+);
+
+CREATE TABLE 'developers_projects' (
+  'developer_id' INTEGER NOT NULL,
+  'project_id' INTEGER NOT NULL,
+  'joined_on' DATE DEFAULT NULL,
+  'access_level' INTEGER DEFAULT 1
+);

actionpack/test/fixtures/developer.rb

@@ -0,0 +1,7 @@
+class Developer < ActiveRecord::Base
+  has_and_belongs_to_many :projects
+end
+
+class DeVeLoPeR < ActiveRecord::Base
+  set_table_name "developers"
+end

actionpack/test/fixtures/developers.yml

@@ -0,0 +1,21 @@
+david:
+  id: 1
+  name: David
+  salary: 80000
+
+jamis:
+  id: 2
+  name: Jamis
+  salary: 150000
+
+<% for digit in 3..10 %>
+dev_<%= digit %>:
+  id: <%= digit %>
+  name: fixture_<%= digit %>
+  salary: 100000
+<% end %>
+
+poor_jamis:
+  id: 11
+  name: Jamis
+  salary: 9000

actionpack/test/fixtures/developers_projects.yml

@@ -0,0 +1,13 @@
+david_action_controller:
+  developer_id: 1
+  project_id: 2
+  joined_on: 2004-10-10
+
+david_active_record:
+  developer_id: 1
+  project_id: 1
+  joined_on: 2004-10-10
+
+jamis_active_record:
+  developer_id: 2
+  project_id: 1

actionpack/test/fixtures/project.rb

@@ -0,0 +1,3 @@
+class Project < ActiveRecord::Base
+  has_and_belongs_to_many :developers, :uniq => true
+end

actionpack/test/fixtures/projects.yml

@@ -0,0 +1,7 @@
+action_controller:
+  id: 2
+  name: Active Controller
+
+active_record:
+  id: 1
+  name: Active Record

actionpack/test/fixtures/replies.yml

@@ -0,0 +1,13 @@
+witty_retort:
+  id: 1
+  topic_id: 1
+  content: Birdman is better!
+  created_at: <%= 6.hours.ago.to_s(:db) %>
+  updated_at: nil
+  
+another:
+  id: 2
+  topic_id: 2
+  content: Nuh uh!
+  created_at: <%= 1.hour.ago.to_s(:db) %>
+  updated_at: nil

actionpack/test/fixtures/reply.rb

@@ -0,0 +1,5 @@
+class Reply < ActiveRecord::Base
+  belongs_to :topic, :include => [:replies]
+  
+  validates_presence_of :content
+end

actionpack/test/fixtures/topic.rb

@@ -0,0 +1,3 @@
+class Topic < ActiveRecord::Base
+  has_many :replies, :include => [:user], :dependent => true
+end

actionpack/test/fixtures/topics.yml

@@ -0,0 +1,22 @@
+futurama:
+  id: 1
+  title: Isnt futurama awesome?
+  subtitle: It really is, isnt it.
+  content: I like futurama
+  created_at: <%= 1.day.ago.to_s(:db) %>
+  updated_at:
+  
+harvey_birdman:
+  id: 2
+  title: Harvey Birdman is the king of all men
+  subtitle: yup
+  content: It really is
+  created_at: <%= 2.hours.ago.to_s(:db) %>
+  updated_at:
+
+rails:
+  id: 3
+  title: Rails is nice
+  subtitle: It makes me happy
+  content: except when I have to hack internals to fix pagination. even then really.
+  created_at: <%= 20.minutes.ago.to_s(:db) %>

activerecord/CHANGELOG

@@ -1,5 +1,7 @@
 *SVN*
 
+* Fix problems with count when used with :include [Jeremy Hopple and Kevin Clark]
+
 * ActiveRecord::RecordInvalid now states which validations failed in its default error message [Tobias Luetke]
 
 * Using AssociationCollection#build with arrays of hashes should call build, not create [DHH]

activerecord/lib/active_record/associations.rb

@@ -779,6 +779,11 @@ module ActiveRecord
             end
           end
         end
+        
+        def count_with_associations(options = {})
+          reflections = reflect_on_included_associations(options[:include])
+          return count_by_sql(construct_counter_sql_with_included_associations(options, reflections))
+        end
 
         def find_with_associations(options = {})
           reflections  = reflect_on_included_associations(options[:include])
@@ -996,40 +1001,72 @@ module ActiveRecord
             "#{name} Load Including Associations"
           )
         end
-
-        def construct_finder_sql_with_included_associations(options, schema_abbreviations, reflections)
-          sql = "SELECT #{column_aliases(schema_abbreviations)} FROM #{table_name} "
+        
+        def construct_counter_sql_with_included_associations(options, reflections)
+          sql = "SELECT COUNT(DISTINCT #{table_name}.#{primary_key})"
+          
+          # A (slower) workaround if we're using a backend, like sqlite, that doesn't support COUNT DISTINCT.
+          if !Base.connection.supports_count_distinct?
+            sql = "SELECT COUNT(*) FROM (SELECT DISTINCT #{table_name}.#{primary_key}"
+          end
+          
+          sql << " FROM #{table_name} "
           sql << reflections.collect { |reflection| association_join(reflection) }.to_s
           sql << "#{options[:joins]} " if options[:joins]
 
           add_conditions!(sql, options[:conditions])
           add_sti_conditions!(sql, reflections)
-          add_limited_ids_condition!(sql, options) if !using_limitable_reflections?(reflections) && options[:limit]
-
-          sql << "ORDER BY #{options[:order]} " if options[:order]
+          add_limited_ids_condition!(sql, options, reflections) if !using_limitable_reflections?(reflections) && options[:limit]
 
           add_limit!(sql, options) if using_limitable_reflections?(reflections)
 
-          return sanitize_sql(sql)
+          if !Base.connection.supports_count_distinct?
+            sql << ")"
+          end
+
+          return sanitize_sql(sql)          
         end
 
-        def add_limited_ids_condition!(sql, options)
-          unless (id_list = select_limited_ids_list(options)).empty?
+        def construct_finder_sql_with_included_associations(options, schema_abbreviations, reflections)
+          sql = "SELECT #{column_aliases(schema_abbreviations)} FROM #{table_name} "
+          sql << reflections.collect { |reflection| association_join(reflection) }.to_s
+          sql << "#{options[:joins]} " if options[:joins]
+ 
+          add_conditions!(sql, options[:conditions])
+          add_sti_conditions!(sql, reflections)
+          add_limited_ids_condition!(sql, options, reflections) if !using_limitable_reflections?(reflections) && options[:limit]
+
+          sql << "ORDER BY #{options[:order]} " if options[:order]
+ 
+          add_limit!(sql, options) if using_limitable_reflections?(reflections)
+ 
+          return sanitize_sql(sql)
+        end
+ 
+        def add_limited_ids_condition!(sql, options, reflections)
+          unless (id_list = select_limited_ids_list(options, reflections)).empty?
             sql << "#{condition_word(sql)} #{table_name}.#{primary_key} IN (#{id_list}) "
           end
         end
-
-        def select_limited_ids_list(options)
+ 
+        def select_limited_ids_list(options, reflections)
           connection.select_values(
-            construct_finder_sql_for_association_limiting(options),
+            construct_finder_sql_for_association_limiting(options, reflections),
             "#{name} Load IDs For Limited Eager Loading"
           ).collect { |id| connection.quote(id) }.join(", ")
         end
-
-        def construct_finder_sql_for_association_limiting(options)
-          raise(ArgumentError, "Limited eager loads and conditions on the eager tables is incompatible") if include_eager_conditions?(options)
+ 
+        def construct_finder_sql_for_association_limiting(options, reflections)
+          #sql = "SELECT DISTINCT #{table_name}.#{primary_key} FROM #{table_name} "
+          sql = "SELECT "
+          sql << "DISTINCT #{table_name}." if include_eager_conditions?(options) || include_eager_order?(options)
+          sql << "#{primary_key} FROM #{table_name} "
+          
+          if include_eager_conditions?(options) || include_eager_order?(options)
+            sql << reflections.collect { |reflection| association_join(reflection) }.to_s
+            sql << "#{options[:joins]} " if options[:joins]
+          end
           
-          sql = "SELECT #{primary_key} FROM #{table_name} "
           add_conditions!(sql, options[:conditions])
           sql << "ORDER BY #{options[:order]} " if options[:order]
           add_limit!(sql, options)
@@ -1044,6 +1081,14 @@ module ActiveRecord
             condition_table_name != table_name
           end
         end
+        
+        def include_eager_order?(options)
+          order = options[:order]
+          return false unless order
+          order.scan(/(\w+)\.\w+/).flatten.any? do |order_table_name|
+            order_table_name != table_name
+          end
+        end
 
         def using_limitable_reflections?(reflections)
           reflections.reject { |r| [ :belongs_to, :has_one ].include?(r.macro) }.length.zero?

activerecord/lib/active_record/base.rb

@@ -495,13 +495,59 @@ module ActiveRecord #:nodoc:
         connection.delete(sql, "#{name} Delete all")
       end
 
-      # Returns the number of records that meet the +conditions+. Zero is returned if no records match. Example:
-      #   Product.count "sales > 1"
-      def count(conditions = nil, joins = nil)
-        sql  = "SELECT COUNT(*) FROM #{table_name} "
-        sql << " #{joins} " if joins
-        add_conditions!(sql, conditions)
-        count_by_sql(sql)
+      # Count operates using three different approaches. 
+      #
+      # * Count all: By not passing any parameters to count, it will return a count of all the rows for the model.
+      # * Count by conditions or joins: For backwards compatibility, you can pass in +conditions+ and +joins+ as individual parameters.
+      # * Count using options will find the row count matched by the options used.
+      #
+      # The last approach, count using options, accepts an option hash as the only parameter. The options are:
+      #
+      # * <tt>:conditions</tt>: An SQL fragment like "administrator = 1" or [ "user_name = ?", username ]. See conditions in the intro.
+      # * <tt>:joins</tt>: An SQL fragment for additional joins like "LEFT JOIN comments ON comments.post_id = id". (Rarely needed).
+      #   The records will be returned read-only since they will have attributes that do not correspond to the table's columns.
+      # * <tt>:include</tt>: Named associations that should be loaded alongside using LEFT OUTER JOINs. The symbols named refer
+      #   to already defined associations. When using named associations count returns the number DISTINCT items for the model you're counting.
+      #   See eager loading under Associations.
+      #
+      # Examples for counting all:
+      #   Person.count         # returns the total count of all people
+      #
+      # Examples for count by +conditions+ and +joins+ (for backwards compatibility):
+      #   Person.count("age > 26")  # returns the number of people older than 26
+      #   Person.find("age > 26 AND job.salary > 60000", "LEFT JOIN jobs on jobs.person_id = person.id") # returns the total number of rows matching the conditions and joins fetched by SELECT COUNT(*).
+      #
+      # Examples for count with options:
+      #   Person.count(:conditions => "age > 26")
+      #   Person.count(:conditions => "age > 26 AND job.salary > 60000", :include => :job) # because of the named association, it finds the DISTINCT count using LEFT OUTER JOIN.
+      #   Person.count(:conditions => "age > 26 AND job.salary > 60000", :joins => "LEFT JOIN jobs on jobs.person_id = person.id") # finds the number of rows matching the conditions and joins. 
+      def count(*args) 
+        options = {}
+        
+        #For backwards compatibility, we need to handle both count(conditions=nil, joins=nil) or count(options={}).
+        if args.size >= 0 and args.size <= 2
+          if args.first.is_a?(Hash)
+            options = args.first
+            #should we verify the options hash???
+          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
+        
+        options[:include] ? count_with_associations(options) : count_by_sql(construct_counter_sql(options))
+      end
+      
+      def construct_counter_sql(options)
+        sql  = "SELECT COUNT(" 
+        sql << "DISTINCT " if options[:distinct]
+        sql << "#{table_name}.#{primary_key}) FROM #{table_name} "
+        sql << " #{options[:joins]} " if options[:joins]
+        add_conditions!(sql, options[:conditions])
+        sql
       end
 
       # Returns the result of an SQL statement that should only include a COUNT(*) in the SELECT part.

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

@@ -38,6 +38,12 @@ module ActiveRecord
       def supports_migrations?
         false
       end
+      
+      # Does this adapter support using DISTINCT within COUNT?  This is +true+
+      # for all adapters except sqlite.
+      def supports_count_distinct?
+        true
+      end
 
       # Should primary key values be selected from their corresponding
       # sequence before the insert statement?  If true, next_sequence_value

activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb

@@ -98,6 +98,10 @@ module ActiveRecord
       def supports_migrations? #:nodoc:
         true
       end
+      
+      def supports_count_distinct? #:nodoc:
+        false
+      end
 
       def native_database_types #:nodoc:
         {

activerecord/test/associations_go_eager_test.rb

@@ -120,9 +120,11 @@ class EagerAssociationTest < Test::Unit::TestCase
   end
 
   def test_eager_with_has_many_and_limit_and_conditions_array_on_the_eagers
-    assert_raises(ArgumentError) do
-      posts = Post.find(:all, :include => [ :author, :comments ], :limit => 2, :conditions => [ "authors.name = ?", 'David' ])
-    end
+    posts = Post.find(:all, :include => [ :author, :comments ], :limit => 2, :conditions => [ "authors.name = ?", 'David' ])
+    assert_equal 2, posts.size
+    
+    count = Post.count(:include => [ :author, :comments ], :limit => 2, :conditions => [ "authors.name = ?", 'David' ])
+    assert_equal count, posts.size
   end
 
   def test_eager_with_has_many_and_limit_with_no_results
@@ -141,13 +143,19 @@ class EagerAssociationTest < Test::Unit::TestCase
   end
 
   def test_eager_with_has_many_and_limit_and_conditions_on_the_eagers
-    assert_raises(ArgumentError) do
-      posts = authors(:david).posts.find(:all, 
-        :include    => :comments, 
-        :conditions => "comments.body like 'Normal%' OR comments.#{QUOTED_TYPE}= 'SpecialComment'",
-        :limit      => 2
-      )
-    end
+    posts = authors(:david).posts.find(:all, 
+      :include    => :comments, 
+      :conditions => "comments.body like 'Normal%' OR comments.#{QUOTED_TYPE}= 'SpecialComment'",
+      :limit      => 2
+    )
+    assert_equal 2, posts.size
+    
+    count = Post.count(
+      :include    => [ :comments, :author ], 
+      :conditions => "authors.name = 'David' AND (comments.body like 'Normal%' OR comments.#{QUOTED_TYPE}= 'SpecialComment')",
+      :limit      => 2
+    )
+    assert_equal count, posts.size
   end
 
   def test_eager_association_loading_with_habtm

activerecord/test/base_test.rb

@@ -1056,6 +1056,13 @@ class BasicsTest < Test::Unit::TestCase
                         "LEFT JOIN comments ON posts.id=comments.post_id")
     end
     assert_equal res, res2
+    
+    res3 = res + 1
+    assert_nothing_raised do
+      res3 = Post.count(:conditions => "posts.#{QUOTED_TYPE} = 'Post'",
+                        :joins => "LEFT JOIN comments ON posts.id=comments.post_id")
+    end
+    assert_equal res, res3
   end
   
   def test_clear_association_cache_stored