Refactoring, testing and documenting pg_connection.distinct

activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb

@@ -429,20 +429,17 @@ def type_to_sql(type, limit = nil, precision = nil, scale = nil)
         # PostgreSQL requires the ORDER BY columns in the select list for distinct queries, and
         # requires that the ORDER BY include the distinct column.
         #
-        #   distinct("posts.id", "posts.created_at desc")
+        #   distinct("posts.id", ["posts.created_at desc"])
+        #   # => "DISTINCT posts.id, posts.created_at AS alias_0"
         def distinct(columns, orders) #:nodoc:
-          return "DISTINCT #{columns}" if orders.empty?
-
-          # Construct a clean list of column names from the ORDER BY clause, removing
-          # any ASC/DESC modifiers
-          order_columns = orders.collect do |s|
-            s = s.to_sql unless s.is_a?(String)
-            s.gsub(/\s+(ASC|DESC)\s*(NULLS\s+(FIRST|LAST)\s*)?/i, '')
-          end
-          order_columns.delete_if { |c| c.blank? }
-          order_columns = order_columns.zip((0...order_columns.size).to_a).map { |s,i| "#{s} AS alias_#{i}" }
-
-          "DISTINCT #{columns}, #{order_columns * ', '}"
+          order_columns = orders.map{ |s|
+              # Convert Arel node to string
+              s = s.to_sql unless s.is_a?(String)
+              # Remove any ASC/DESC modifiers
+              s.gsub(/\s+(ASC|DESC)\s*(NULLS\s+(FIRST|LAST)\s*)?/i, '')
+            }.reject(&:blank?).map.with_index { |column, i| "#{column} AS alias_#{i}" }
+
+          [super].concat(order_columns).join(', ')
         end
       end
     end

activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb

@@ -216,6 +216,35 @@ def test_partial_index
         assert_equal "(number > 100)", index.where
       end
 
+      def test_distinct_zero_orders
+        assert_equal "DISTINCT posts.id",
+          @connection.distinct("posts.id", [])
+      end
+
+      def test_distinct_one_order
+        assert_equal "DISTINCT posts.id, posts.created_at AS alias_0",
+          @connection.distinct("posts.id", ["posts.created_at desc"])
+      end
+
+      def test_distinct_few_orders
+        assert_equal "DISTINCT posts.id, posts.created_at AS alias_0, posts.position AS alias_1",
+          @connection.distinct("posts.id", ["posts.created_at desc", "posts.position asc"])
+      end
+
+      def test_distinct_blank_not_nil_orders
+        assert_equal "DISTINCT posts.id, posts.created_at AS alias_0",
+          @connection.distinct("posts.id", ["posts.created_at desc", "", "   "])
+      end
+
+      def test_distinct_with_arel_order
+        order = Object.new
+        def order.to_sql
+          "posts.created_at desc"
+        end
+        assert_equal "DISTINCT posts.id, posts.created_at AS alias_0",
+          @connection.distinct("posts.id", [order])
+      end
+
       def test_distinct_with_nulls
         assert_equal "DISTINCT posts.title, posts.updater_id AS alias_0", @connection.distinct("posts.title", ["posts.updater_id desc nulls first"])
         assert_equal "DISTINCT posts.title, posts.updater_id AS alias_0", @connection.distinct("posts.title", ["posts.updater_id desc nulls last"])