Fix SqlLiterals overriding query's retryable value

Most `visit` functions in the `ToSql` visitor that modify the
`collector`'s `retryable` value will change it unconditionally to
`false`. For example, if the visitor ever encounters a
`DeleteStatement`, we never want to retry the query, so `retryable =
false`. However, the `SqlLiteral` `visit` method would copy the
`SqlLiteral`s `retryable` attribute to the `collector`, whether it was
`true` or `false`. This is an issue because the `collector`'s final
`retryable` value ends up dependent on the order of `SqlLiteral`s
visited and can be incorrectly changed from `false` to `true`.

This commit fixes the issue by changing the `SqlLiteral` `visit` method
to only change the `collector`'s `retryable` value to `false` (and only if
the `SqlLiteral`'s `retryable` value is also `false`). This ensures that
`retryable` `SqlLiteral`s can never override a `collector`s `false`
`retryable` value.

A new test was added to exercise the bug, specifically that a
`retryable: true` `SqlLiteral` is visited after a `Node` that marks the
`collector` `retryable: false`. Additionally, the fix exposed the fact
that one of the tests incorrectly relied on the previous behavior
because none of the tests used a `collector` with `retryable: true`. To
be more realistic, `collector.retryable = true` was added to all of the
tests concerned with `retryable`, even though most of them are valid
tests without it (since `retryable: nil` is the default, asserting
`retryable: false` still correctly covers `ToSql` changing the value to
`false`).

Author: skipkayhil

activerecord/lib/arel/visitors/to_sql.rb

@@ -763,7 +763,7 @@ def visit_Arel_Nodes_BindParam(o, collector)
 
         def visit_Arel_Nodes_SqlLiteral(o, collector)
           collector.preparable = false
-          collector.retryable = o.retryable
+          collector.retryable &&= o.retryable
           collector << o.to_s
         end
 

activerecord/test/cases/arel/visitors/to_sql_test.rb

@@ -73,6 +73,7 @@ def dispatch
         it "should mark collector as non-retryable when visiting named function" do
           function = Nodes::NamedFunction.new("ABS", [@table])
           collector = Collectors::SQLString.new
+          collector.retryable = true
           @visitor.accept(function, collector)
 
           assert_equal false, collector.retryable
@@ -81,22 +82,37 @@ def dispatch
         it "should mark collector as non-retryable when visiting SQL literal" do
           node = Nodes::SqlLiteral.new("COUNT(*)")
           collector = Collectors::SQLString.new
+          collector.retryable = true
           @visitor.accept(node, collector)
 
           assert_equal false, collector.retryable
         end
 
-        it "should mark collector as retryable if SQL literal is marked as retryable" do
+        it "should not change retryable if SQL literal is marked as retryable" do
           node = Nodes::SqlLiteral.new("COUNT(*)", retryable: true)
           collector = Collectors::SQLString.new
+          collector.retryable = true
           @visitor.accept(node, collector)
 
-          assert collector.retryable
+          assert_predicate collector, :retryable
+        end
+
+        it "should mark collector as non-retryable if SQL literal is not retryable" do
+          node = Nodes::As.new(
+            Nodes::SqlLiteral.new("`product.id`"),
+            Nodes::SqlLiteral.new("`product.id`", retryable: true)
+          )
+          collector = Collectors::SQLString.new
+          collector.retryable = true
+          @visitor.accept(node, collector)
+
+          assert_equal false, collector.retryable
         end
 
         it "should mark collector as non-retryable when visiting bound SQL literal" do
           node = Nodes::BoundSqlLiteral.new("id IN (?)", [[1, 2, 3]], {})
           collector = Collectors::SQLString.new
+          collector.retryable = true
           @visitor.accept(node, collector)
 
           assert_equal false, collector.retryable
@@ -105,6 +121,7 @@ def dispatch
         it "should mark collector as non-retryable when visiting insert statement node" do
           statement = Arel::Nodes::InsertStatement.new(@table)
           collector = Collectors::SQLString.new
+          collector.retryable = true
           @visitor.accept(statement, collector)
 
           assert_equal false, collector.retryable
@@ -113,6 +130,7 @@ def dispatch
         it "should mark collector as non-retryable when visiting update statement node" do
           statement = Arel::Nodes::UpdateStatement.new(@table)
           collector = Collectors::SQLString.new
+          collector.retryable = true
           @visitor.accept(statement, collector)
 
           assert_equal false, collector.retryable
@@ -121,6 +139,7 @@ def dispatch
         it "should mark collector as non-retryable when visiting delete statement node" do
           statement = Arel::Nodes::DeleteStatement.new(@table)
           collector = Collectors::SQLString.new
+          collector.retryable = true
           @visitor.accept(statement, collector)
 
           assert_equal false, collector.retryable