Merge pull request #549 from iaddict/master

Fix ordering on an aggregate in MSSQL

Author: kares

lib/arjdbc/mssql/limit_helpers.rb

@@ -4,6 +4,7 @@ module LimitHelpers
 
       # @private
       FIND_SELECT = /\b(SELECT(\s+DISTINCT)?)\b(.*)/mi
+      FIND_AGGREGATE_FUNCTIONS = /AVG|COUNT|COUNT_BIG|MAX|MIN|SUM|STDDEV|STDEVP|VAR|VARP/
 
       module SqlServerReplaceLimitOffset
 
@@ -28,8 +29,10 @@ def replace_limit_offset!(sql, limit, offset, order)
             #   ActiveRecord::StatementInvalid: ActiveRecord::JDBCError: Column 'users.id' is invalid in the select list because it is not contained in either an aggregate function or the GROUP BY clause.
             #   SELECT t.* FROM ( SELECT ROW_NUMBER() OVER(ORDER BY users.id) AS _row_num, [users].[lft], COUNT([users].[lft]) FROM [users] GROUP BY [users].[lft] HAVING COUNT([users].[lft]) > 1 ) AS t WHERE t._row_num BETWEEN 1 AND 1
             if rest_of_query.downcase.include?('group by')
-              if order.count(',') == 0
-                order.gsub!(/ORDER BY (.*)/, 'ORDER BY MIN(\1)')
+              if order.match(/^ORDER +BY +(#{FIND_AGGREGATE_FUNCTIONS})\(/i)
+                # do nothing
+              elsif order.count(',') == 0
+                order.gsub!(/ORDER +BY +([^\s]+)(\s+ASC|\s+DESC)?/i, 'ORDER BY MIN(\1)\2')
               else
                 raise('Only one order condition allowed.')
               end

test/db/mssql/limit_offset_test.rb

@@ -182,6 +182,24 @@ def test_limit_with_group_by
     assert_equal ['three', 'four'], ships.map(&:name)
   end if ar_version('3.0')
 
+  def test_limit_with_group_by_and_aggregate_in_order_clause
+    %w( one two three four five six seven eight ).each_with_index do |name, i|
+      LongShip.create!(:name => name, :width => (i+1)*10)
+    end
+
+    ships = LongShip.select(:name).group(:name).limit(2).order('width').all
+    assert_equal ['one', 'two'], ships.map(&:name)
+
+    ships = LongShip.select(:name).group(:name).limit(2).order('width DESC').all
+    assert_equal ['eight', 'seven'], ships.map(&:name)
+
+    ships = LongShip.select(:name).group(:name).limit(2).order('MAX(width)').all
+    assert_equal ['one', 'two'], ships.map(&:name)
+
+    ships = LongShip.select(:name).group(:name).limit(2).order('MAX(width) DESC').all
+    assert_equal ['eight', 'seven'], ships.map(&:name)
+  end if ar_version('3.0')
+
   def test_select_distinct_with_limit
     %w(c a b a b a c d c d).each do |name|
       LongShip.create!(:name => name)