allow Arel.sql() for pluck

3 files changed, 69 insertions, 33 deletions

diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb
index 66bc14b5ab..20dcb0080b 100644
--- a/activerecord/test/cases/calculations_test.rb
+++ b/activerecord/test/cases/calculations_test.rb
@@ -144,7 +144,7 @@ class CalculationsTest < ActiveRecord::TestCase
   end
 
   def test_should_order_by_calculation
-    c = Account.group(:firm_id).order("sum_credit_limit desc, firm_id").sum(:credit_limit)
+    c = Account.group(:firm_id).order(Arel.sql("sum_credit_limit desc, firm_id")).sum(:credit_limit)
     assert_equal [105, 60, 53, 50, 50], c.keys.collect { |k| c[k] }
     assert_equal [6, 2, 9, 1], c.keys.compact
   end
@@ -644,7 +644,7 @@ class CalculationsTest < ActiveRecord::TestCase
   end
 
   def test_pluck_with_qualified_column_name
-    assert_equal [1, 2, 3, 4, 5], Topic.order(:id).pluck("topics.id")
+    assert_equal [1, 2, 3, 4, 5], Topic.order(:id).pluck(Arel.sql("topics.id"))
   end
 
   def test_pluck_auto_table_name_prefix
@@ -659,18 +659,18 @@ class CalculationsTest < ActiveRecord::TestCase
 
   def test_pluck_not_auto_table_name_prefix_if_column_joined
     Company.create!(name: "test", contracts: [Contract.new(developer_id: 7)])
-    assert_equal [7], Company.joins(:contracts).pluck(:developer_id)
+    assert_equal [7], Company.joins(:contracts).pluck(Arel.sql("developer_id"))
   end
 
   def test_pluck_with_selection_clause
-    assert_equal [50, 53, 55, 60], Account.pluck("DISTINCT credit_limit").sort
-    assert_equal [50, 53, 55, 60], Account.pluck("DISTINCT accounts.credit_limit").sort
-    assert_equal [50, 53, 55, 60], Account.pluck("DISTINCT(credit_limit)").sort
+    assert_equal [50, 53, 55, 60], Account.pluck(Arel.sql("DISTINCT credit_limit")).sort
+    assert_equal [50, 53, 55, 60], Account.pluck(Arel.sql("DISTINCT accounts.credit_limit")).sort
+    assert_equal [50, 53, 55, 60], Account.pluck(Arel.sql("DISTINCT(credit_limit)")).sort
 
     # MySQL returns "SUM(DISTINCT(credit_limit))" as the column name unless
     # an alias is provided.  Without the alias, the column cannot be found
     # and properly typecast.
-    assert_equal [50 + 53 + 55 + 60], Account.pluck("SUM(DISTINCT(credit_limit)) as credit_limit")
+    assert_equal [50 + 53 + 55 + 60], Account.pluck(Arel.sql("SUM(DISTINCT(credit_limit)) as credit_limit"))
   end
 
   def test_plucks_with_ids
@@ -684,7 +684,7 @@ class CalculationsTest < ActiveRecord::TestCase
 
   def test_pluck_not_auto_table_name_prefix_if_column_included
     Company.create!(name: "test", contracts: [Contract.new(developer_id: 7)])
-    ids = Company.includes(:contracts).pluck(:developer_id)
+    ids = Company.includes(:contracts).pluck(Arel.sql("developer_id"))
     assert_equal Company.count, ids.length
     assert_equal [7], ids.compact
   end
@@ -704,12 +704,12 @@ class CalculationsTest < ActiveRecord::TestCase
 
   def test_pluck_with_multiple_columns_and_selection_clause
     assert_equal [[1, 50], [2, 50], [3, 50], [4, 60], [5, 55], [6, 53]],
-      Account.pluck("id, credit_limit")
+      Account.pluck(Arel.sql("id, credit_limit"))
   end
 
   def test_pluck_with_multiple_columns_and_includes
     Company.create!(name: "test", contracts: [Contract.new(developer_id: 7)])
-    companies_and_developers = Company.order("companies.id").includes(:contracts).pluck(:name, :developer_id)
+    companies_and_developers = Company.order(Arel.sql("companies.id")).includes(:contracts).pluck(:name, Arel.sql("developer_id"))
 
     assert_equal Company.count, companies_and_developers.length
     assert_equal ["37signals", nil], companies_and_developers.first
@@ -731,7 +731,7 @@ class CalculationsTest < ActiveRecord::TestCase
   def test_pluck_columns_with_same_name
     expected = [["The First Topic", "The Second Topic of the day"], ["The Third Topic of the day", "The Fourth Topic of the day"]]
     actual = Topic.joins(:replies)
-      .pluck("topics.title", "replies_topics.title")
+      .pluck(Arel.sql("topics.title"), Arel.sql("replies_topics.title"))
     assert_equal expected, actual
   end
 
@@ -772,7 +772,7 @@ class CalculationsTest < ActiveRecord::TestCase
     companies = Company.order(:name).limit(3).load
 
     assert_queries 1 do
-      assert_equal ["37signals", "Apex", "Ex Nihilo"], companies.pluck("DISTINCT name")
+      assert_equal ["37signals", "Apex", "Ex Nihilo"], companies.pluck(Arel.sql("DISTINCT name"))
     end
   end
 
diff --git a/activerecord/test/cases/relation/merging_test.rb b/activerecord/test/cases/relation/merging_test.rb
index b68b3723f6..953e0fee76 100644
--- a/activerecord/test/cases/relation/merging_test.rb
+++ b/activerecord/test/cases/relation/merging_test.rb
@@ -118,7 +118,7 @@ class MergingDifferentRelationsTest < ActiveRecord::TestCase
 
   test "merging where relations" do
     hello_by_bob = Post.where(body: "hello").joins(:author).
-      merge(Author.where(name: "Bob")).order("posts.id").pluck("posts.id")
+      merge(Author.where(name: "Bob")).order("posts.id").pluck(Arel.sql("posts.id"))
 
     assert_equal [posts(:misc_by_bob).id,
                   posts(:other_by_bob).id], hello_by_bob
@@ -126,19 +126,19 @@ class MergingDifferentRelationsTest < ActiveRecord::TestCase
 
   test "merging order relations" do
     posts_by_author_name = Post.limit(3).joins(:author).
-      merge(Author.order(:name)).pluck("authors.name")
+      merge(Author.order(:name)).pluck(Arel.sql("authors.name"))
 
     assert_equal ["Bob", "Bob", "David"], posts_by_author_name
 
     posts_by_author_name = Post.limit(3).joins(:author).
-      merge(Author.order("name")).pluck("authors.name")
+      merge(Author.order("name")).pluck(Arel.sql("authors.name"))
 
     assert_equal ["Bob", "Bob", "David"], posts_by_author_name
   end
 
   test "merging order relations (using a hash argument)" do
     posts_by_author_name = Post.limit(4).joins(:author).
-      merge(Author.order(name: :desc)).pluck("authors.name")
+      merge(Author.order(name: :desc)).pluck(Arel.sql("authors.name"))
 
     assert_equal ["Mary", "Mary", "Mary", "David"], posts_by_author_name
   end
diff --git a/activerecord/test/cases/unsafe_raw_sql_test.rb b/activerecord/test/cases/unsafe_raw_sql_test.rb
index d05f0f12d9..89eb02594a 100644
--- a/activerecord/test/cases/unsafe_raw_sql_test.rb
+++ b/activerecord/test/cases/unsafe_raw_sql_test.rb
@@ -13,7 +13,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     end
 
     assert_equal enabled, disabled
-    assert_equal disabled, Post.unsafe_raw_order("title").pluck(:id)
+    assert_equal disabled, Post.order(Arel.sql("title")).pluck(:id)
   end
 
   test "order: allows symbol column name" do
@@ -22,7 +22,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     end
 
     assert_equal enabled, disabled
-    assert_equal disabled, Post.unsafe_raw_order(:title).pluck(:id)
+    assert_equal disabled, Post.order(Arel.sql("title")).pluck(:id)
   end
 
   test "order: allows downcase symbol direction" do
@@ -31,7 +31,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     end
 
     assert_equal enabled, disabled
-    assert_equal disabled, Post.unsafe_raw_order(title: :asc).pluck(:id)
+    assert_equal disabled, Post.order(Arel.sql("title") => Arel.sql("asc")).pluck(:id)
   end
 
   test "order: allows upcase symbol direction" do
@@ -40,7 +40,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     end
 
     assert_equal enabled, disabled
-    assert_equal disabled, Post.unsafe_raw_order(title: :ASC).pluck(:id)
+    assert_equal disabled, Post.order(Arel.sql("title") => Arel.sql("ASC")).pluck(:id)
   end
 
   test "order: allows string direction" do
@@ -49,7 +49,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     end
 
     assert_equal enabled, disabled
-    assert_equal disabled, Post.unsafe_raw_order(title: "asc").pluck(:id)
+    assert_equal disabled, Post.order(Arel.sql("title") => Arel.sql("asc")).pluck(:id)
   end
 
   test "order: allows multiple columns" do
@@ -58,7 +58,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     end
 
     assert_equal enabled, disabled
-    assert_equal disabled, Post.unsafe_raw_order(:author_id, :title).pluck(:id)
+    assert_equal disabled, Post.order(Arel.sql("author_id"), Arel.sql("title")).pluck(:id)
   end
 
   test "order: allows mixed" do
@@ -67,12 +67,12 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     end
 
     assert_equal enabled, disabled
-    assert_equal disabled, Post.unsafe_raw_order(:author_id, title: :asc).pluck(:id)
+    assert_equal disabled, Post.order(Arel.sql("author_id"), Arel.sql("title") => Arel.sql("asc")).pluck(:id)
   end
 
   test "order: disallows invalid column name" do
     with_config(:disabled) do
-      assert_raises(ArgumentError) do
+      assert_raises(ActiveRecord::UnknownAttributeReference) do
         Post.order("title asc").pluck(:id)
       end
     end
@@ -88,19 +88,37 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
 
   test "order: disallows invalid column with direction" do
     with_config(:disabled) do
-      assert_raises(ArgumentError) do
+      assert_raises(ActiveRecord::UnknownAttributeReference) do
         Post.order(foo: :asc).pluck(:id)
       end
     end
   end
 
+  test "order: always allows Arel" do
+    enabled, disabled = with_configs(:enabled, :disabled) do
+      Post.order(Arel.sql("length(title)")).pluck(:title)
+    end
+
+    assert_equal enabled, disabled
+  end
+
+  test "order: logs deprecation warning for unrecognized column" do
+    with_config(:deprecated) do
+      ActiveSupport::Deprecation.expects(:warn).with do |msg|
+        msg =~ /\ADangerous query method used with .*length\(title\)/
+      end
+
+      Post.order("length(title)")
+    end
+  end
+
   test "pluck: allows string column name" do
     enabled, disabled = with_configs(:enabled, :disabled) do
       Post.pluck("title")
     end
 
     assert_equal enabled, disabled
-    assert_equal disabled, Post.unsafe_raw_pluck("title")
+    assert_equal disabled, Post.pluck(Arel.sql("title"))
   end
 
   test "pluck: allows symbol column name" do
@@ -109,7 +127,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     end
 
     assert_equal enabled, disabled
-    assert_equal disabled, Post.unsafe_raw_pluck(:title)
+    assert_equal disabled, Post.pluck(Arel.sql("title"))
   end
 
   test "pluck: allows multiple column names" do
@@ -118,7 +136,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     end
 
     assert_equal enabled, disabled
-    assert_equal disabled, Post.unsafe_raw_pluck(:title, :id)
+    assert_equal disabled, Post.pluck(Arel.sql("title"), Arel.sql("id"))
   end
 
   test "pluck: allows column names with includes" do
@@ -127,7 +145,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     end
 
     assert_equal enabled, disabled
-    assert_equal disabled, Post.includes(:comments).unsafe_raw_pluck(:title, :id)
+    assert_equal disabled, Post.includes(:comments).pluck(Arel.sql("title"), Arel.sql("id"))
   end
 
   test "pluck: allows auto-generated attributes" do
@@ -136,12 +154,12 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     end
 
     assert_equal enabled, disabled
-    assert_equal disabled, Post.unsafe_raw_pluck(:tags_count)
+    assert_equal disabled, Post.pluck(Arel.sql("tags_count"))
   end
 
   test "pluck: disallows invalid column name" do
     with_config(:disabled) do
-      assert_raises(ArgumentError) do
+      assert_raises(ActiveRecord::UnknownAttributeReference) do
         Post.pluck("length(title)")
       end
     end
@@ -149,7 +167,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
 
   test "pluck: disallows invalid column name amongst valid names" do
     with_config(:disabled) do
-      assert_raises(ArgumentError) do
+      assert_raises(ActiveRecord::UnknownAttributeReference) do
         Post.pluck(:title, "length(title)")
       end
     end
@@ -157,12 +175,30 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
 
   test "pluck: disallows invalid column names with includes" do
     with_config(:disabled) do
-      assert_raises(ArgumentError) do
+      assert_raises(ActiveRecord::UnknownAttributeReference) do
         Post.includes(:comments).pluck(:title, "length(title)")
       end
     end
   end
 
+  test "pluck: always allows Arel" do
+    enabled, disabled = with_configs(:enabled, :disabled) do
+      Post.includes(:comments).pluck(:title, Arel.sql("length(title)"))
+    end
+
+    assert_equal enabled, disabled
+  end
+
+  test "pluck: logs deprecation warning" do
+    with_config(:deprecated) do
+      ActiveSupport::Deprecation.expects(:warn).with do |msg|
+        msg =~ /\ADangerous query method used with .*length\(title\)/
+      end
+
+      Post.includes(:comments).pluck(:title, "length(title)")
+    end
+  end
+
   def with_configs(*new_values, &blk)
     new_values.map { |nv| with_config(nv, &blk) }
   end