* 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

4 files changed, 122 insertions, 21 deletions

diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index ea442021c5..6b461b5514 100755
--- a/activerecord/lib/active_record/associations.rb
+++ b/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_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, reflections) if !using_limitable_reflections?(reflections) && options[:limit]
+
+          add_limit!(sql, options) if using_limitable_reflections?(reflections)
+
+          if !Base.connection.supports_count_distinct?
+            sql << ")"
+          end
+
+          return sanitize_sql(sql)          
+        end
 
         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) if !using_limitable_reflections?(reflections) && options[:limit]
+          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)
-          unless (id_list = select_limited_ids_list(options)).empty?
+ 
+        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?
diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index dd0c3e5c79..deb3c5fc1f 100755
--- a/activerecord/lib/active_record/base.rb
+++ b/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.
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
index f120a50f8f..b01ef89dfc 100755
--- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
+++ b/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
diff --git a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb
index 44011a09fd..579bc471d2 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb
+++ b/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:
         {