From 0ef7e73f0af752002c6150bd1cf29586d8bd21c9 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Sat, 30 May 2015 12:26:41 -0600 Subject: [PATCH] Ensure symbols passed to `select` are always quoted Our general contract in Active Record is that strings are assumed to be SQL literals, and symbols are assumed to reference a column. If a from clause is given, we shouldn't include the table name, but we should still quote the value as if it were a column. Upon fixing this, the tests were still failing on SQLite. This was because the column name being returned by the query was `"\"join\""` instead of `"join"`. This is actually a bug in SQLite that was fixed a long time ago, but I was using the version of SQLite included by OS X which has this bug. Since I'm guessing this will be a common case for contributors, I also added an explicit check with a more helpful error message. Fixes #20360 --- Gemfile.lock | 2 +- activerecord/CHANGELOG.md | 7 ++++++ .../active_record/relation/query_methods.rb | 16 ++++++-------- activerecord/test/cases/relation_test.rb | 22 +++++++++++++++++++ 4 files changed, 37 insertions(+), 10 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 6c900835cb..3cc20f1c9c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -308,4 +308,4 @@ DEPENDENCIES w3c_validators BUNDLED WITH - 1.10.1 + 1.10.2 diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index fff1f75567..2cd9791aa0 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,10 @@ +* Ensure symbols passed to `ActiveRecord::Relation#select` are always treated + as columns. + + Fixes #20360. + + *Sean Griffin* + * Do not set `sql_mode` if `strict: :default` is specified. ``` diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index fd78db2e95..f85dc35e89 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -1001,15 +1001,13 @@ module ActiveRecord end def arel_columns(columns) - if from_clause.value - columns - else - columns.map do |field| - if (Symbol === field || String === field) && columns_hash.key?(field.to_s) - arel_table[field] - else - field - end + columns.map do |field| + if (Symbol === field || String === field) && columns_hash.key?(field.to_s) && !from_clause.value + arel_table[field] + elsif Symbol === field + connection.quote_table_name(field.to_s) + else + field end end end diff --git a/activerecord/test/cases/relation_test.rb b/activerecord/test/cases/relation_test.rb index 9353be1ba7..24ed9e6638 100644 --- a/activerecord/test/cases/relation_test.rb +++ b/activerecord/test/cases/relation_test.rb @@ -242,6 +242,13 @@ module ActiveRecord assert_equal false, post.respond_to?(:title), "post should not respond_to?(:body) since invoking it raises exception" end + def test_select_quotes_when_using_from_clause + ensure_sqlite3_version_doesnt_include_bug + quoted_join = ActiveRecord::Base.connection.quote_table_name("join") + selected = Post.select(:join).from(Post.select("id as #{quoted_join}")).map(&:join) + assert_equal Post.pluck(:id), selected + end + def test_relation_merging_with_merged_joins_as_strings join_string = "LEFT OUTER JOIN #{Rating.quoted_table_name} ON #{SpecialComment.quoted_table_name}.id = #{Rating.quoted_table_name}.comment_id" special_comments_with_ratings = SpecialComment.joins join_string @@ -276,5 +283,20 @@ module ActiveRecord assert_equal "type cast from database", UpdateAllTestModel.first.body end + + private + + def ensure_sqlite3_version_doesnt_include_bug + if current_adapter?(:SQLite3Adapter) + selected_quoted_column_names = ActiveRecord::Base.connection.exec_query( + 'SELECT "join" FROM (SELECT id AS "join" FROM posts) subquery' + ).columns + assert_equal ["join"], selected_quoted_column_names, <<-ERROR.squish + You are using an outdated version of SQLite3 which has a bug in + quoted column names. Please update SQLite3 and rebuild the sqlite3 + ruby gem + ERROR + end + end end end