From e7e9b16416c22f80234a53b485e4c2b8c95a25a1 Mon Sep 17 00:00:00 2001
From: John Weathers <john.weathers@systino.net>
Date: Thu, 7 May 2009 20:14:13 -0400
Subject: [PATCH] Improved the parsing of column expressions in the PostgreSQL adapter so that "order by" clauses containing multi-argument functions like COALESCE do not generate invalid SQL queries with mistaken attempts at applying aliases to arguments inside the function

---
 .../connection_adapters/postgresql_adapter.rb      |   47 +++++++++++++++++---
 activerecord/test/cases/associations/eager_test.rb |    3 +-
 2 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
index 4961793..5e8cd74 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -864,6 +864,43 @@ module ActiveRecord
         execute "DROP INDEX #{quote_table_name(index_name(table_name, options))}"
       end
 
+      # Parses column expressions out of an "order by" clause
+      def parse_columns(order_by)
+        columns = []
+        token = ''
+        stack = []
+        order_by.each_byte do |byte|
+          char = byte.chr
+          case char
+          when '('
+            stack.push('(')
+          when ')'
+            stack.pop if stack.last == '('
+          when '"'
+            if stack.last == '"'
+              stack.pop
+            else
+              stack.push('"')
+            end
+          when "'"
+            if stack.last == "'"
+              stack.pop
+            else
+              stack.push("'")
+            end
+          when ","
+            if stack.empty?
+              columns << token
+              token = ''
+              next
+            end
+          end
+          
+          token += char
+        end
+        columns << token.strip
+      end
+
       # Maps logical Rails types to PostgreSQL-specific data types.
       def type_to_sql(type, limit = nil, precision = nil, scale = nil)
         return super unless type.to_s == 'integer'
@@ -886,10 +923,9 @@ module ActiveRecord
         return "DISTINCT #{columns}" if order_by.blank?
 
         # 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.delete_if &:blank?
-        order_columns = order_columns.zip((0...order_columns.size).to_a).map { |s,i| "#{s} AS alias_#{i}" }
+        # any ASC/DESC modifiers and adding enumerated aliases
+        order_columns = parse_columns(order_by)
+        order_columns = order_columns.zip((0...order_columns.size).to_a).map { |c,i| "#{c.sub(/\b(?:asc|desc)$/i, '')} 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.
@@ -904,8 +940,7 @@ module ActiveRecord
       def add_order_by_for_association_limiting!(sql, options) #:nodoc:
         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 = parse_columns(options[:order]).map { |s| m = s.match(/(?:\b(?:asc|desc))?$/i)[0]; (m=='') ? ' ASC' : m.upcase }
         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}"
diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb
index 4072381..76acf16 100644
--- a/activerecord/test/cases/associations/eager_test.rb
+++ b/activerecord/test/cases/associations/eager_test.rb
@@ -573,10 +573,11 @@ class EagerAssociationTest < ActiveRecord::TestCase
   end
 
   def test_limited_eager_with_multiple_order_columns
-    assert_equal posts(:thinking, :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(:thinking, :sti_comments), Post.find(:all, :include => [:author, :comments], :conditions => "authors.name = 'David'", :order => 'COALESCE(UPPER(posts.title), \'Test Title\'), posts.id', :limit => 2, :offset => 1)
     assert_equal posts(:sti_post_and_comments, :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_preload_with_interpolation
     assert_equal [comments(:greetings)], Post.find(posts(:welcome).id, :include => :comments_with_interpolated_conditions).comments_with_interpolated_conditions
   end
-- 
1.6.0.4