PostgreSQL: use a subselect to correctly perform eager finds with :limit and :order. Closes #4668.

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

5 files changed, 41 insertions, 5 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index 435aeac9b9..1c6988765c 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* PostgreSQL: use a subselect to correctly perform eager finds with :limit and :order.  #4668 [eventualbuddha]
+
 * Pass a range in :conditions to use the SQL BETWEEN operator.  #6974 [dcmanges]
     Student.find(:all, :conditions => { :grade => 9..12 })
 
diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index 8a1768dba6..56086693d2 100755
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -1205,7 +1205,13 @@ module ActiveRecord
           end
 
           add_conditions!(sql, options[:conditions], scope)
-          sql << "ORDER BY #{options[:order]} " if options[:order]
+          if options[:order]
+            if is_distinct
+              connection.add_order_by_for_association_limiting!(sql, options)
+            else
+              sql << "ORDER BY #{options[:order]}"
+            end
+          end
           add_limit!(sql, options, scope)
           return sanitize_sql(sql)
         end
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
index 599d04e593..8a8d956ca8 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -287,6 +287,12 @@ module ActiveRecord
       def distinct(columns, order_by)
         "DISTINCT #{columns}"
       end
+      
+      # ORDER BY clause for the passed order option.
+      # PostgreSQL overrides this due to its stricter standards compliance.
+      def add_order_by_for_association_limiting!(sql, options)
+        sql << "ORDER BY #{options[:order]}"
+      end
     end
   end
 end
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
index c0efd1a341..5ef117a749 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -383,17 +383,29 @@ module ActiveRecord
 
         # construct a clean list of column names from the ORDER BY clause, removing
         # any asc/desc modifiers
-        order_columns = order_by.split(',').collect! { |s| s.split.first }
+        order_columns = order_by.split(',').collect { |s| s.split.first }
         order_columns.delete_if &:blank?
-
-        # add the DISTINCT columns to the start of the ORDER BY clause
-        order_by.replace "#{columns}, #{order_by}"
+        order_columns = order_columns.zip((0...order_columns.size).to_a).map { |s,i| "#{s} AS alias_#{i}" }
 
         # return a DISTINCT ON() clause that's distinct on the columns we want but includes
         # all the required columns for the ORDER BY to work properly
         sql = "DISTINCT ON (#{columns}) #{columns}, "
         sql << order_columns * ', '
       end
+      
+      # ORDER BY clause for the passed order option.
+      # 
+      # PostgreSQL does not allow arbitrary ordering when using DISTINCT ON, so we work around this
+      # by wrapping the sql as a sub-select and ordering in that query.
+      def add_order_by_for_association_limiting!(sql, options)
+        return sql if options[:order].blank?
+        
+        order = options[:order].split(',').collect { |s| s.strip }.reject(&:blank?)
+        order.map! { |s| 'DESC' if s =~ /\bdesc$/i }
+        order = order.zip((0...order.size).to_a).map { |s,i| "id_list.alias_#{i} #{s}" }.join(', ')
+        
+        sql.replace "SELECT * FROM (#{sql}) AS id_list ORDER BY #{order}"
+      end
 
       private
         BYTEA_COLUMN_TYPE_OID = 17
diff --git a/activerecord/test/associations/eager_test.rb b/activerecord/test/associations/eager_test.rb
index b303363f00..0d69af1c10 100644
--- a/activerecord/test/associations/eager_test.rb
+++ b/activerecord/test/associations/eager_test.rb
@@ -290,6 +290,16 @@ class EagerAssociationTest < Test::Unit::TestCase
   def find_all_ordered(className, include=nil)
     className.find(:all, :order=>"#{className.table_name}.#{className.primary_key}", :include=>include)
   end
+  
+  def test_limited_eager_with_order
+    assert_equal [posts(:thinking), posts(:sti_comments)], Post.find(:all, :include => [:author, :comments], :conditions => "authors.name = 'David'", :order => 'UPPER(posts.title)', :limit => 2, :offset => 1)
+    assert_equal [posts(:sti_post_and_comments), posts(:sti_comments)], Post.find(:all, :include => [:author, :comments], :conditions => "authors.name = 'David'", :order => 'UPPER(posts.title) DESC', :limit => 2, :offset => 1)
+  end
+  
+  def test_limited_eager_with_multiple_order_columns
+    assert_equal [posts(:thinking), posts(:sti_comments)], Post.find(:all, :include => [:author, :comments], :conditions => "authors.name = 'David'", :order => 'UPPER(posts.title), posts.id', :limit => 2, :offset => 1)
+    assert_equal [posts(:sti_post_and_comments), posts(:sti_comments)], Post.find(:all, :include => [:author, :comments], :conditions => "authors.name = 'David'", :order => 'UPPER(posts.title) DESC, posts.id', :limit => 2, :offset => 1)
+  end
 
   def test_eager_with_multiple_associations_with_same_table_has_many_and_habtm
     # Eager includes of has many and habtm associations aren't necessarily sorted in the same way