Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

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

The combination of a :uniq => true association and the #distinct call
in #construct_limited_ids_condition combine to create invalid SQL, because
we're explicitly selecting DISTINCT, and also sending #distinct on to AREL,
via the relation#distinct_value.

Rather than build a select distinct clause in #construct_limited_ids_condition,
I set #distinct! and pass just the columns into the select statement.
This requires introducing a #columns_for_distinct method to return the
select columns but not the statement itself.
  • Loading branch information...
commit 15d6e4dce7126fe24bce5cdb91d2ffee68648420 1 parent 0593c00
@Empact Empact authored
View
14 activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -706,12 +706,20 @@ def add_column_options!(sql, options) #:nodoc:
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.
View
9 activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
@@ -467,14 +467,9 @@ def type_to_sql(type, limit = nil, precision = nil, scale = nil)
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 @@ def distinct(columns, orders) #:nodoc:
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
View
4 activerecord/lib/active_record/relation/finder_methods.rb
@@ -245,9 +245,9 @@ def apply_join_dependency(relation, join_dependency)
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]}
View
34 activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb
@@ -230,21 +230,41 @@ def test_distinct_zero_orders
@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 @@ def order.to_sql
@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) }
View
12 activerecord/test/cases/finder_test.rb
@@ -98,6 +98,18 @@ def test_exists_with_includes_limit_and_empty_result
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?
Please sign in to comment.
Something went wrong with that request. Please try again.