Merge pull request #36448 from kamipo/allow_column_name_with_simple_function_call

Allow column name with function (e.g. `length(title)`) as safe SQL string

7 files changed, 67 insertions, 42 deletions

diff --git a/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb b/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb
index e34f4f745f..1b6ba8ce97 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb
@@ -158,7 +158,10 @@ module ActiveRecord
       COLUMN_NAME = /
         \A
         (
-          (?:\w+\.)?\w+
+          (?:
+            # table_name.column_name | function(one or no argument)
+            ((?:\w+\.)?\w+) | \w+\((?:|\g<2>)\)
+          )
           (?:(?:\s+AS)?\s+\w+)?
         )
         (?:\s*,\s*\g<1>)*
@@ -179,7 +182,10 @@ module ActiveRecord
       COLUMN_NAME_WITH_ORDER = /
         \A
         (
-          (?:\w+\.)?\w+
+          (?:
+            # table_name.column_name | function(one or no argument)
+            ((?:\w+\.)?\w+) | \w+\((?:|\g<2>)\)
+          )
           (?:\s+ASC|\s+DESC)?
           (?:\s+NULLS\s+(?:FIRST|LAST))?
         )
diff --git a/activerecord/lib/active_record/connection_adapters/mysql/quoting.rb b/activerecord/lib/active_record/connection_adapters/mysql/quoting.rb
index a0829b1115..dfed5471f4 100644
--- a/activerecord/lib/active_record/connection_adapters/mysql/quoting.rb
+++ b/activerecord/lib/active_record/connection_adapters/mysql/quoting.rb
@@ -43,7 +43,10 @@ module ActiveRecord
         COLUMN_NAME = /
           \A
           (
-            (?:\w+\.|`\w+`\.)?(?:\w+|`\w+`)
+            (?:
+              # `table_name`.`column_name` | function(one or no argument)
+              ((?:\w+\.|`\w+`\.)?(?:\w+|`\w+`)) | \w+\((?:|\g<2>)\)
+            )
             (?:(?:\s+AS)?\s+(?:\w+|`\w+`))?
           )
           (?:\s*,\s*\g<1>)*
@@ -53,7 +56,10 @@ module ActiveRecord
         COLUMN_NAME_WITH_ORDER = /
           \A
           (
-            (?:\w+\.|`\w+`\.)?(?:\w+|`\w+`)
+            (?:
+              # `table_name`.`column_name` | function(one or no argument)
+              ((?:\w+\.|`\w+`\.)?(?:\w+|`\w+`)) | \w+\((?:|\g<2>)\)
+            )
             (?:\s+ASC|\s+DESC)?
           )
           (?:\s*,\s*\g<1>)*
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb b/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb
index d18c5c5c12..0c800dca83 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb
@@ -89,7 +89,10 @@ module ActiveRecord
         COLUMN_NAME = /
           \A
           (
-            (?:\w+\.|"\w+"\.)?(?:\w+|"\w+")(?:::\w+)?
+            (?:
+              # "table_name"."column_name"::type_name | function(one or no argument)::type_name
+              ((?:\w+\.|"\w+"\.)?(?:\w+|"\w+")(?:::\w+)?) | \w+\((?:|\g<2>)\)(?:::\w+)?
+            )
             (?:(?:\s+AS)?\s+(?:\w+|"\w+"))?
           )
           (?:\s*,\s*\g<1>)*
@@ -99,7 +102,10 @@ module ActiveRecord
         COLUMN_NAME_WITH_ORDER = /
           \A
           (
-            (?:\w+\.|"\w+"\.)?(?:\w+|"\w+")(?:::\w+)?
+            (?:
+              # "table_name"."column_name"::type_name | function(one or no argument)::type_name
+              ((?:\w+\.|"\w+"\.)?(?:\w+|"\w+")(?:::\w+)?) | \w+\((?:|\g<2>)\)(?:::\w+)?
+            )
             (?:\s+ASC|\s+DESC)?
             (?:\s+NULLS\s+(?:FIRST|LAST))?
           )
diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb b/activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb
index 5d6932e4ca..54808de714 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb
@@ -56,7 +56,10 @@ module ActiveRecord
         COLUMN_NAME = /
           \A
           (
-            (?:\w+\.|"\w+"\.)?(?:\w+|"\w+")
+            (?:
+              # "table_name"."column_name" | function(one or no argument)
+              ((?:\w+\.|"\w+"\.)?(?:\w+|"\w+")) | \w+\((?:|\g<2>)\)
+            )
             (?:(?:\s+AS)?\s+(?:\w+|"\w+"))?
           )
           (?:\s*,\s*\g<1>)*
@@ -66,7 +69,10 @@ module ActiveRecord
         COLUMN_NAME_WITH_ORDER = /
           \A
           (
-            (?:\w+\.|"\w+"\.)?(?:\w+|"\w+")
+            (?:
+              # "table_name"."column_name" | function(one or no argument)
+              ((?:\w+\.|"\w+"\.)?(?:\w+|"\w+")) | \w+\((?:|\g<2>)\)
+            )
             (?:\s+ASC|\s+DESC)?
           )
           (?:\s*,\s*\g<1>)*
diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb
index 38723b6c19..cb46f9e053 100644
--- a/activerecord/test/cases/associations/eager_test.rb
+++ b/activerecord/test/cases/associations/eager_test.rb
@@ -968,14 +968,14 @@ class EagerAssociationTest < ActiveRecord::TestCase
       posts(:thinking, :sti_comments),
       Post.all.merge!(
         includes: [:author, :comments], where: { "authors.name" => "David" },
-        order: Arel.sql("UPPER(posts.title)"), limit: 2, offset: 1
+        order: "UPPER(posts.title)", limit: 2, offset: 1
       ).to_a
     )
     assert_equal(
       posts(:sti_post_and_comments, :sti_comments),
       Post.all.merge!(
         includes: [:author, :comments], where: { "authors.name" => "David" },
-        order: Arel.sql("UPPER(posts.title) DESC"), limit: 2, offset: 1
+        order: "UPPER(posts.title) DESC", limit: 2, offset: 1
       ).to_a
     )
   end
@@ -985,14 +985,14 @@ class EagerAssociationTest < ActiveRecord::TestCase
       posts(:thinking, :sti_comments),
       Post.all.merge!(
         includes: [:author, :comments], where: { "authors.name" => "David" },
-        order: [Arel.sql("UPPER(posts.title)"), "posts.id"], limit: 2, offset: 1
+        order: ["UPPER(posts.title)", "posts.id"], limit: 2, offset: 1
       ).to_a
     )
     assert_equal(
       posts(:sti_post_and_comments, :sti_comments),
       Post.all.merge!(
         includes: [:author, :comments], where: { "authors.name" => "David" },
-        order: [Arel.sql("UPPER(posts.title) DESC"), "posts.id"], limit: 2, offset: 1
+        order: ["UPPER(posts.title) DESC", "posts.id"], limit: 2, offset: 1
       ).to_a
     )
   end
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 732bca4bf5..a5c162af33 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -298,7 +298,7 @@ class RelationTest < ActiveRecord::TestCase
   end
 
   def test_reverse_order_with_function
-    topics = Topic.order(Arel.sql("length(title)")).reverse_order
+    topics = Topic.order("length(title)").reverse_order
     assert_equal topics(:second).title, topics.first.title
   end
 
@@ -1696,7 +1696,7 @@ class RelationTest < ActiveRecord::TestCase
     scope = Post.order("comments.body asc")
     assert_equal ["comments"], scope.references_values
 
-    scope = Post.order(Arel.sql("foo(comments.body)"))
+    scope = Post.order("foo(comments.body)")
     assert_equal [], scope.references_values
   end
 
@@ -1721,7 +1721,7 @@ class RelationTest < ActiveRecord::TestCase
     scope = Post.reorder("comments.body asc")
     assert_equal %w(comments), scope.references_values
 
-    scope = Post.reorder(Arel.sql("foo(comments.body)"))
+    scope = Post.reorder("foo(comments.body)")
     assert_equal [], scope.references_values
   end
 
diff --git a/activerecord/test/cases/unsafe_raw_sql_test.rb b/activerecord/test/cases/unsafe_raw_sql_test.rb
index d4cb51afba..87edb163f2 100644
--- a/activerecord/test/cases/unsafe_raw_sql_test.rb
+++ b/activerecord/test/cases/unsafe_raw_sql_test.rb
@@ -141,7 +141,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
   test "order: disallows invalid column name" do
     with_unsafe_raw_sql_disabled do
       assert_raises(ActiveRecord::UnknownAttributeReference) do
-        Post.order("len(title) asc").pluck(:id)
+        Post.order("REPLACE(title, 'misc', 'zzzz') asc").pluck(:id)
       end
     end
   end
@@ -157,7 +157,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
   test "order: disallows invalid column with direction" do
     with_unsafe_raw_sql_disabled do
       assert_raises(ActiveRecord::UnknownAttributeReference) do
-        Post.order("len(title)" => :asc).pluck(:id)
+        Post.order("REPLACE(title, 'misc', 'zzzz')" => :asc).pluck(:id)
       end
     end
   end
@@ -190,7 +190,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
   test "order: disallows invalid Array arguments" do
     with_unsafe_raw_sql_disabled do
       assert_raises(ActiveRecord::UnknownAttributeReference) do
-        Post.order(["author_id", "length(title)"]).pluck(:id)
+        Post.order(["author_id", "REPLACE(title, 'misc', 'zzzz')"]).pluck(:id)
       end
     end
   end
@@ -198,8 +198,8 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
   test "order: allows valid Array arguments" do
     ids_expected = Post.order(Arel.sql("author_id, length(title)")).pluck(:id)
 
-    ids_depr     = with_unsafe_raw_sql_deprecated { Post.order(["author_id", Arel.sql("length(title)")]).pluck(:id) }
-    ids_disabled = with_unsafe_raw_sql_disabled   { Post.order(["author_id", Arel.sql("length(title)")]).pluck(:id) }
+    ids_depr     = with_unsafe_raw_sql_deprecated { Post.order(["author_id", "length(title)"]).pluck(:id) }
+    ids_disabled = with_unsafe_raw_sql_disabled   { Post.order(["author_id", "length(title)"]).pluck(:id) }
 
     assert_equal ids_expected, ids_depr
     assert_equal ids_expected, ids_disabled
@@ -208,7 +208,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
   test "order: logs deprecation warning for unrecognized column" do
     with_unsafe_raw_sql_deprecated do
       assert_deprecated(/Dangerous query method/) do
-        Post.order("length(title)")
+        Post.order("REPLACE(title, 'misc', 'zzzz')")
       end
     end
   end
@@ -223,11 +223,11 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
     assert_equal titles_expected, titles_disabled
   end
 
-  test "pluck: allows string column name with alias" do
-    titles_expected = Post.pluck(Arel.sql("title"))
+  test "pluck: allows string column name with function and alias" do
+    titles_expected = Post.pluck(Arel.sql("UPPER(title)"))
 
-    titles_depr     = with_unsafe_raw_sql_deprecated { Post.pluck("title AS posts_title") }
-    titles_disabled = with_unsafe_raw_sql_disabled   { Post.pluck("title AS posts_title") }
+    titles_depr     = with_unsafe_raw_sql_deprecated { Post.pluck("UPPER(title) AS title") }
+    titles_disabled = with_unsafe_raw_sql_disabled   { Post.pluck("UPPER(title) AS title") }
 
     assert_equal titles_expected, titles_depr
     assert_equal titles_expected, titles_disabled
@@ -297,7 +297,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
   test "pluck: disallows invalid column name" do
     with_unsafe_raw_sql_disabled do
       assert_raises(ActiveRecord::UnknownAttributeReference) do
-        Post.pluck("length(title)")
+        Post.pluck("REPLACE(title, 'misc', 'zzzz')")
       end
     end
   end
@@ -305,7 +305,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
   test "pluck: disallows invalid column name amongst valid names" do
     with_unsafe_raw_sql_disabled do
       assert_raises(ActiveRecord::UnknownAttributeReference) do
-        Post.pluck(:title, "length(title)")
+        Post.pluck(:title, "REPLACE(title, 'misc', 'zzzz')")
       end
     end
   end
@@ -313,7 +313,7 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
   test "pluck: disallows invalid column names with includes" do
     with_unsafe_raw_sql_disabled do
       assert_raises(ActiveRecord::UnknownAttributeReference) do
-        Post.includes(:comments).pluck(:title, "length(title)")
+        Post.includes(:comments).pluck(:title, "REPLACE(title, 'misc', 'zzzz')")
       end
     end
   end
@@ -328,24 +328,25 @@ class UnsafeRawSqlTest < ActiveRecord::TestCase
   test "pluck: logs deprecation warning" do
     with_unsafe_raw_sql_deprecated do
       assert_deprecated(/Dangerous query method/) do
-        Post.includes(:comments).pluck(:title, "length(title)")
+        Post.includes(:comments).pluck(:title, "REPLACE(title, 'misc', 'zzzz')")
       end
     end
   end
 
-  def with_unsafe_raw_sql_disabled(&blk)
-    with_config(:disabled, &blk)
-  end
+  private
+    def with_unsafe_raw_sql_disabled(&block)
+      with_config(:disabled, &block)
+    end
 
-  def with_unsafe_raw_sql_deprecated(&blk)
-    with_config(:deprecated, &blk)
-  end
+    def with_unsafe_raw_sql_deprecated(&block)
+      with_config(:deprecated, &block)
+    end
 
-  def with_config(new_value, &blk)
-    old_value = ActiveRecord::Base.allow_unsafe_raw_sql
-    ActiveRecord::Base.allow_unsafe_raw_sql = new_value
-    blk.call
-  ensure
-    ActiveRecord::Base.allow_unsafe_raw_sql = old_value
-  end
+    def with_config(new_value, &block)
+      old_value = ActiveRecord::Base.allow_unsafe_raw_sql
+      ActiveRecord::Base.allow_unsafe_raw_sql = new_value
+      yield
+    ensure
+      ActiveRecord::Base.allow_unsafe_raw_sql = old_value
+    end
 end