Merge pull request rails#54951 from skipkayhil/hm-where-sql-literal

Enable passing retryable `SqlLiteral`s to `#where`

Author: byroot

activerecord/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Enable passing retryable SqlLiterals to `#where`.
+
+    *Hartley McGuire*
+
 *   Set default for primary keys in `insert_all`/`upsert_all`.
 
     Previously in Postgres, updating and inserting new records in one upsert wasn't possible

activerecord/lib/active_record/relation/predicate_builder.rb

@@ -82,7 +82,7 @@ def with(table)
       attr_writer :table
 
       def expand_from_hash(attributes, &block)
-        return ["1=0"] if attributes.empty?
+        return [Arel.sql("1=0", retryable: true)] if attributes.empty?
 
         attributes.flat_map do |key, value|
           if key.is_a?(Array) && key.size == 1

activerecord/lib/active_record/relation/query_methods.rb

@@ -1630,7 +1630,7 @@ def build_where_clause(opts, rest = []) # :nodoc:
           elsif opts.include?("?")
             parts = [build_bound_sql_literal(opts, rest)]
           else
-            parts = [model.sanitize_sql(rest.empty? ? opts : [opts, *rest])]
+            parts = [Arel.sql(model.sanitize_sql([opts, *rest]))]
           end
         when Hash
           opts = opts.transform_keys do |key|

activerecord/lib/active_record/relation/where_clause.rb

@@ -188,7 +188,7 @@ def predicates_with_wrapped_sql_literals
           non_empty_predicates.map do |node|
             case node
             when Arel::Nodes::SqlLiteral, ::String
-              wrap_sql_literal(node)
+              Arel::Nodes::Grouping.new(node)
             else node
             end
           end
@@ -199,13 +199,6 @@ def non_empty_predicates
           predicates - ARRAY_WITH_EMPTY_STRING
         end
 
-        def wrap_sql_literal(node)
-          if ::String === node
-            node = Arel.sql(node)
-          end
-          Arel::Nodes::Grouping.new(node)
-        end
-
         def extract_node_value(node)
           if node.respond_to?(:value_before_type_cast)
             node.value_before_type_cast

activerecord/lib/arel.rb

@@ -50,7 +50,9 @@ module Arel
   # Use this option only if the SQL is idempotent, as it could be executed
   # more than once.
   def self.sql(sql_string, *positional_binds, retryable: false, **named_binds)
-    if positional_binds.empty? && named_binds.empty?
+    if Arel::Nodes::SqlLiteral === sql_string
+      sql_string
+    elsif positional_binds.empty? && named_binds.empty?
       Arel::Nodes::SqlLiteral.new(sql_string, retryable: retryable)
     else
       Arel::Nodes::BoundSqlLiteral.new sql_string, positional_binds, named_binds

activerecord/test/cases/adapter_test.rb

@@ -706,6 +706,7 @@ def teardown
         notifications = capture_notifications("sql.active_record") do
           assert (a = Author.first)
           assert Post.where(id: [1, 2]).first
+          assert Post.where(Arel.sql("id IN (1,2)", retryable: true)).first
           assert Post.find(1)
           assert Post.find_by(title: "Welcome to the weblog")
           assert_predicate Post, :exists?
@@ -714,7 +715,7 @@ def teardown
           Author.group(:name).count
         end.select { |n| n.payload[:name] != "SCHEMA" }
 
-        assert_equal 8, notifications.length
+        assert_equal 9, notifications.length
 
         notifications.each do |n|
           assert n.payload[:allow_retry], "#{n.payload[:sql]} was not retryable"
@@ -727,6 +728,7 @@ def teardown
         notifications = capture_notifications("sql.active_record") do
           assert_not_nil (a = Author.first)
           assert_not_nil Post.where(id: [1, 2]).first
+          assert Post.where(Arel.sql("id IN (1,2)", retryable: true)).first
           assert_not_nil Post.find(1)
           assert_not_nil Post.find_by(title: "Welcome to the weblog")
           assert_predicate Post, :exists?
@@ -735,7 +737,7 @@ def teardown
           Author.group(:name).count
         end.select { |n| n.payload[:name] != "SCHEMA" }
 
-        assert_equal 8, notifications.length
+        assert_equal 9, notifications.length
 
         notifications.each do |n|
           assert n.payload[:allow_retry], "#{n.payload[:sql]} was not retryable"

activerecord/test/cases/relation/where_clause_test.rb

@@ -15,9 +15,9 @@ class WhereClauseTest < ActiveRecord::TestCase
     end
 
     test "+ is associative, but not commutative" do
-      a = WhereClause.new(["a"])
-      b = WhereClause.new(["b"])
-      c = WhereClause.new(["c"])
+      a = WhereClause.new([Arel.sql("a")])
+      b = WhereClause.new([Arel.sql("b")])
+      c = WhereClause.new([Arel.sql("c")])
 
       assert_equal a + (b + c), (a + b) + c
       assert_not_equal a + b, b + a
@@ -76,7 +76,7 @@ class WhereClauseTest < ActiveRecord::TestCase
 
     test "a clause knows if it is empty" do
       assert_empty WhereClause.empty
-      assert_not_empty WhereClause.new(["anything"])
+      assert_not_empty WhereClause.new([Arel.sql("anything")])
     end
 
     test "invert cannot handle nil" do
@@ -99,7 +99,7 @@ class WhereClauseTest < ActiveRecord::TestCase
         table["id"].lteq(2),
         table["id"].is_not_distinct_from(1),
         table["id"].is_distinct_from(2),
-        "sql literal"
+        Arel.sql("sql literal"),
       ])
       expected = WhereClause.new([
         Arel::Nodes::Not.new(
@@ -114,7 +114,7 @@ class WhereClauseTest < ActiveRecord::TestCase
             table["id"].lteq(2),
             table["id"].is_not_distinct_from(1),
             table["id"].is_distinct_from(2),
-            Arel::Nodes::Grouping.new("sql literal")
+            Arel::Nodes::Grouping.new(Arel.sql("sql literal")),
           ])
         )
       ])
@@ -161,7 +161,7 @@ class WhereClauseTest < ActiveRecord::TestCase
       random_object = Object.new
       where_clause = WhereClause.new([
         table["id"].in([1, 2, 3]),
-        "foo = bar",
+        Arel.sql("foo = bar"),
         random_object,
       ])
       expected = Arel::Nodes::And.new([
@@ -175,7 +175,7 @@ class WhereClauseTest < ActiveRecord::TestCase
 
     test "ast removes any empty strings" do
       where_clause = WhereClause.new([table["id"].in([1, 2, 3])])
-      where_clause_with_empty = WhereClause.new([table["id"].in([1, 2, 3]), ""])
+      where_clause_with_empty = WhereClause.new([table["id"].in([1, 2, 3]), Arel.sql("")])
 
       assert_equal where_clause.ast, where_clause_with_empty.ast
     end
@@ -232,12 +232,12 @@ class WhereClauseTest < ActiveRecord::TestCase
     test "or will use only common conditions if one side only has common conditions" do
       only_common = WhereClause.new([
         table["id"].eq(bind_param(1)),
-        "foo = bar",
+        Arel.sql("foo = bar"),
       ])
 
       common_with_extra = WhereClause.new([
         table["id"].eq(bind_param(1)),
-        "foo = bar",
+        Arel.sql("foo = bar"),
         table["extra"].eq(bind_param("pluto")),
       ])
 
@@ -247,13 +247,13 @@ class WhereClauseTest < ActiveRecord::TestCase
 
     test "supports hash equality" do
       h = Hash.new(0)
-      h[WhereClause.new(["a"])] += 1
-      h[WhereClause.new(["a"])] += 1
-      h[WhereClause.new(["b"])] += 1
+      h[WhereClause.new([Arel.sql("a")])] += 1
+      h[WhereClause.new([Arel.sql("a")])] += 1
+      h[WhereClause.new([Arel.sql("b")])] += 1
 
       expected = {
-        WhereClause.new(["a"]) => 2,
-        WhereClause.new(["b"]) => 1
+        WhereClause.new([Arel.sql("a")]) => 2,
+        WhereClause.new([Arel.sql("b")]) => 1
       }
       assert_equal expected, h
     end