allow Arel.sql() for pluck

1 files changed, 53 insertions, 17 deletions

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