Merge pull request #6792 from Empact/postgres-distinct

Fix that #exists? can produce invalid SQL: "SELECT DISTINCT DISTINCT"

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

@@ -706,12 +706,20 @@ module ActiveRecord
       end
 
       # SELECT DISTINCT clause for a given set of columns and a given ORDER BY clause.
-      # Both PostgreSQL and Oracle overrides this for custom DISTINCT syntax.
       #
-      #   distinct("posts.id", "posts.created_at desc")
+      #   distinct("posts.id", ["posts.created_at desc"])
       #
       def distinct(columns, order_by)
-        "DISTINCT #{columns}"
+        "DISTINCT #{columns_for_distinct(columns, order_by)}"
+      end
+
+      # Given a set of columns and an ORDER BY clause, returns the columns for a SELECT DISTINCT.
+      # Both PostgreSQL and Oracle overrides this for custom DISTINCT syntax - they
+      # require the order columns appear in the SELECT.
+      #
+      #   columns_for_distinct("posts.id", ["posts.created_at desc"])
+      def columns_for_distinct(columns, orders)
+        columns
       end
 
       # Adds timestamps (+created_at+ and +updated_at+) columns to the named table.

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

@@ -467,14 +467,9 @@ module ActiveRecord
           end
         end
 
-        # Returns a SELECT DISTINCT clause for a given set of columns and a given ORDER BY clause.
-        #
         # 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 AS alias_0"
-        def distinct(columns, orders) #:nodoc:
+        def columns_for_distinct(columns, orders) #:nodoc:
           order_columns = orders.map{ |s|
               # Convert Arel node to string
               s = s.to_sql unless s.is_a?(String)
@@ -482,7 +477,7 @@ module ActiveRecord
               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(', ')
+          [super, *order_columns].join(', ')
         end
       end
     end

activerecord/lib/active_record/relation/finder_methods.rb

@@ -245,9 +245,9 @@ module ActiveRecord
 
     def construct_limited_ids_condition(relation)
       orders = relation.order_values.map { |val| val.presence }.compact
-      values = @klass.connection.distinct("#{quoted_table_name}.#{primary_key}", orders)
+      values = @klass.connection.columns_for_distinct("#{quoted_table_name}.#{quoted_primary_key}", orders)
 
-      relation = relation.dup.select(values)
+      relation = relation.dup.select(values).distinct!
 
       id_rows = @klass.connection.select_all(relation.arel, 'SQL', relation.bind_values)
       ids_array = id_rows.map {|row| row[primary_key]}

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

@@ -230,21 +230,41 @@ module ActiveRecord
           @connection.distinct("posts.id", [])
       end
 
+      def test_columns_for_distinct_zero_orders
+        assert_equal "posts.id",
+          @connection.columns_for_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_columns_for_distinct_one_order
+        assert_equal "posts.id, posts.created_at AS alias_0",
+          @connection.columns_for_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_columns_for_distinct_few_orders
+        assert_equal "posts.id, posts.created_at AS alias_0, posts.position AS alias_1",
+          @connection.columns_for_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_columns_for_distinct_blank_not_nil_orders
+        assert_equal "posts.id, posts.created_at AS alias_0",
+          @connection.columns_for_distinct("posts.id", ["posts.created_at desc", "", "   "])
+      end
+
       def test_distinct_with_arel_order
         order = Object.new
         def order.to_sql
@@ -254,11 +274,25 @@ module ActiveRecord
           @connection.distinct("posts.id", [order])
       end
 
+      def test_columns_for_distinct_with_arel_order
+        order = Object.new
+        def order.to_sql
+          "posts.created_at desc"
+        end
+        assert_equal "posts.id, posts.created_at AS alias_0",
+          @connection.columns_for_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"])
       end
 
+      def test_columns_for_distinct_with_nulls
+        assert_equal "posts.title, posts.updater_id AS alias_0", @connection.columns_for_distinct("posts.title", ["posts.updater_id desc nulls first"])
+        assert_equal "posts.title, posts.updater_id AS alias_0", @connection.columns_for_distinct("posts.title", ["posts.updater_id desc nulls last"])
+      end
+
       def test_raise_error_when_cannot_translate_exception
         assert_raise TypeError do
           @connection.send(:log, nil) { @connection.execute(nil) }

activerecord/test/cases/finder_test.rb

@@ -98,6 +98,18 @@ class FinderTest < ActiveRecord::TestCase
     assert !Topic.includes(:replies).limit(1).where('0 = 1').exists?
   end
 
+  def test_exists_with_distinct_association_includes_and_limit
+    author = Author.first
+    assert !author.unique_categorized_posts.includes(:special_comments).limit(0).exists?
+    assert author.unique_categorized_posts.includes(:special_comments).limit(1).exists?
+  end
+
+  def test_exists_with_distinct_association_includes_limit_and_order
+    author = Author.first
+    assert !author.unique_categorized_posts.includes(:special_comments).order('comments.taggings_count DESC').limit(0).exists?
+    assert author.unique_categorized_posts.includes(:special_comments).order('comments.taggings_count DESC').limit(1).exists?
+  end
+
   def test_exists_with_empty_table_and_no_args_given
     Topic.delete_all
     assert !Topic.exists?