Skip to content
This repository

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

Closed
wants to merge 1 commit into from

3 participants

Ben Woosley Rafael Mendonça França Timothy J. Raymond
Ben Woosley
Empact commented

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#uniq_value.

I unset relation#uniq_value in the limited ids lookup so that Arel doesn't apply the DISTINCT that we're already applying. This only affects the limited ids lookup query, and only in the problematic case where uniq_value was true. Everything else is unchanged.

Bug is present on 3-2-stable as well.

Previously failed with:

ActiveRecord::StatementInvalid: SQLite3::SQLException: near "DISTINCT": syntax error: SELECT  DISTINCT DISTINCT "posts".id FROM "posts" LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id" AND "comments"."type" IN ('SpecialComment', 'SubSpecialComment') INNER JOIN "categorizations" ON "posts"."id" = "categorizations"."post_id" WHERE "categorizations"."author_id" = ? LIMIT 0
    sqlite3-1.3.6/lib/sqlite3/database.rb:91:in `initialize'
    sqlite3-1.3.6/lib/sqlite3/database.rb:91:in `new'
    sqlite3-1.3.6/lib/sqlite3/database.rb:91:in `prepare'
    rails/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb:288:in `block in exec_query'
    rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:288:in `block in log'
    rails/activesupport/lib/active_support/notifications/instrumenter.rb:18:in `instrument'
    rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:283:in `log'
    rails/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb:277:in `exec_query'
    rails/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb:495:in `select'
    rails/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:18:in `select_all'
    rails/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb:63:in `select_all'
    rails/activerecord/lib/active_record/querying.rb:40:in `block in find_by_sql'
    rails/activerecord/lib/active_record/explain.rb:37:in `logging_query_plan'
    rails/activerecord/lib/active_record/querying.rb:39:in `find_by_sql'
    rails/activerecord/lib/active_record/relation.rb:174:in `exec_queries'
    rails/activerecord/lib/active_record/relation.rb:164:in `block in to_a'
    rails/activerecord/lib/active_record/explain.rb:37:in `logging_query_plan'
    rails/activerecord/lib/active_record/relation.rb:163:in `to_a'
    rails/activerecord/lib/active_record/relation/delegation.rb:6:in `collect'
    rails/activerecord/lib/active_record/relation/finder_methods.rb:241:in `construct_limited_ids_condition'
    rails/activerecord/lib/active_record/relation/finder_methods.rb:226:in `apply_join_dependency'
    rails/activerecord/lib/active_record/relation/finder_methods.rb:215:in `construct_relation_for_association_find'
    rails/activerecord/lib/active_record/relation/finder_methods.rb:177:in `exists?'
    test/cases/finder_test.rb:68:in `test_exists_with_distinct_association_includes_and_limit'
Ben Woosley Empact 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#uniq_value.

I unset relation#uniq_value in the limited ids lookup so that Arel doesn't
apply the DISTINCT that we're already applying. This only affects the limited
ids lookup query, and only in the problematic case where uniq_value was true.
8aeeef0
Ben Woosley
Empact commented

Alternative solution is to set uniq_value = true here and passing the primary_key + order columns to arel, rather than writing our own distinct SQL via @klass.connection.distinct. If this works out, it would remove some literal SQL from activerecord and allow us to remove the distinct method from connection, as it isn't used anywhere else.

Rafael Mendonça França
Owner

@tenderlove could you review this one please?

Ben Woosley Empact referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Ben Woosley Empact referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Timothy J. Raymond

+1 I've reproduced this in 4.0.0 beta. Applying this patch fixed it.

Ben Woosley Empact referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
Ben Woosley

Closing this in favor of #6792, which is a more correct patch and is since revised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Jun 11, 2012
Ben Woosley Empact 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#uniq_value.

I unset relation#uniq_value in the limited ids lookup so that Arel doesn't
apply the DISTINCT that we're already applying. This only affects the limited
ids lookup query, and only in the problematic case where uniq_value was true.
8aeeef0
This page is out of date. Refresh to see the latest.
1  activerecord/lib/active_record/relation/finder_methods.rb
@@ -240,6 +240,7 @@ def construct_limited_ids_condition(relation)
240 240 values = @klass.connection.distinct("#{@klass.connection.quote_table_name table_name}.#{primary_key}", orders)
241 241
242 242 relation = relation.dup
  243 + relation.uniq_value = false # avoids SELECT DISTINCT DISTINCT
243 244
244 245 ids_array = relation.select(values).collect {|row| row[primary_key]}
245 246 ids_array.empty? ? raise(ThrowResult) : table[primary_key].in(ids_array)
12 activerecord/test/cases/finder_test.rb
@@ -74,6 +74,18 @@ def test_exists_with_includes_limit_and_empty_result
74 74 assert !Topic.includes(:replies).limit(1).where('0 = 1').exists?
75 75 end
76 76
  77 + def test_exists_with_distinct_association_includes_and_limit
  78 + author = Author.first
  79 + assert !author.unique_categorized_posts.includes(:special_comments).limit(0).exists?
  80 + assert author.unique_categorized_posts.includes(:special_comments).limit(1).exists?
  81 + end
  82 +
  83 + def test_exists_with_distinct_association_includes_limit_and_order
  84 + author = Author.first
  85 + assert !author.unique_categorized_posts.includes(:special_comments).order('comments.taggings_count DESC').limit(0).exists?
  86 + assert author.unique_categorized_posts.includes(:special_comments).order('comments.taggings_count DESC').limit(1).exists?
  87 + end
  88 +
77 89 def test_exists_with_empty_table_and_no_args_given
78 90 Topic.delete_all
79 91 assert !Topic.exists?

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.