Relation#where build BoundSqlLiteral rather than eagerly interpolate

Ref: #50793

To make not caching connection checkout viable, we need to reduced
the amount of places where we need a connection.

Once big source of this is query/relation building, where in many
cases it eagerly quote and interpolation bound values in SQL fragments.

Doing this requires an active connection because both MySQL and Postgres
may quote values differently based on the connection settings.

Instead of eagerly doing all this, we can instead just insert these
as bound values in the Arel AST. For adapters with prepared statements
this is better anyway as it will avoid leaking statements, and for those
that don't support it, it will simply delay the quoting to just
before the query is executed.

However, the `%` API (`where("title = %s", something)`) can't realistically
be fixed this way, but I don't see much value in it and it probably should
be deprecated and removed.

activerecord/lib/active_record/connection_adapters/mysql/quoting.rb

@@ -21,9 +21,6 @@ def cast_bound_value(value)
             "1"
           when false
             "0"
-          when ActiveSupport::Duration
-            warn_quote_duration_deprecated
-            value.to_s
           else
             value
           end

activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb

@@ -141,6 +141,8 @@ def type_cast(value) # :nodoc:
             encode_array(value)
           when Range
             encode_range(value)
+          when Rational
+            value.to_f
           else
             super
           end

activerecord/lib/active_record/connection_adapters/sqlite3/quoting.rb

@@ -63,7 +63,7 @@ def quote_default_expression(value, column) # :nodoc:
 
         def type_cast(value) # :nodoc:
           case value
-          when BigDecimal
+          when BigDecimal, Rational
             value.to_f
           when String
             if value.encoding == Encoding::ASCII_8BIT

activerecord/lib/active_record/relation/query_methods.rb

@@ -1512,9 +1512,21 @@ def build_subquery(subquery_alias, select_value) # :nodoc:
       def build_where_clause(opts, rest = []) # :nodoc:
         opts = sanitize_forbidden_attributes(opts)
 
+        if opts.is_a?(Array)
+          opts, *rest = opts
+        end
+
         case opts
-        when String, Array
-          parts = [klass.sanitize_sql(rest.empty? ? opts : [opts, *rest])]
+        when String
+          if rest.empty?
+            parts = [Arel.sql(opts)]
+          elsif rest.first.is_a?(Hash) && /:\w+/.match?(opts)
+            parts = [build_named_bound_sql_literal(opts, rest.first)]
+          elsif opts.include?("?")
+            parts = [build_bound_sql_literal(opts, rest)]
+          else
+            parts = [klass.sanitize_sql(rest.empty? ? opts : [opts, *rest])]
+          end
         when Hash
           opts = opts.transform_keys do |key|
             if key.is_a?(Array)
@@ -1550,6 +1562,46 @@ def async
         spawn.async!
       end
 
+      def build_named_bound_sql_literal(statement, values)
+        bound_values = values.transform_values do |value|
+          if ActiveRecord::Relation === value
+            Arel.sql(value.to_sql)
+          elsif value.respond_to?(:map) && !value.acts_like?(:string)
+            values = value.map { |v| v.respond_to?(:id_for_database) ? v.id_for_database : v }
+            values.empty? ? nil : values
+          else
+            value = value.id_for_database if value.respond_to?(:id_for_database)
+            value
+          end
+        end
+
+        begin
+          Arel::Nodes::BoundSqlLiteral.new("(#{statement})", nil, bound_values)
+        rescue Arel::BindError => error
+          raise ActiveRecord::PreparedStatementInvalid, error.message
+        end
+      end
+
+      def build_bound_sql_literal(statement, values)
+        bound_values = values.map do |value|
+          if ActiveRecord::Relation === value
+            Arel.sql(value.to_sql)
+          elsif value.respond_to?(:map) && !value.acts_like?(:string)
+            values = value.map { |v| v.respond_to?(:id_for_database) ? v.id_for_database : v }
+            values.empty? ? nil : values
+          else
+            value = value.id_for_database if value.respond_to?(:id_for_database)
+            value
+          end
+        end
+
+        begin
+          Arel::Nodes::BoundSqlLiteral.new("(#{statement})", bound_values, nil)
+        rescue Arel::BindError => error
+          raise ActiveRecord::PreparedStatementInvalid, error.message
+        end
+      end
+
       def lookup_table_klass_from_join_dependencies(table_name)
         each_join_dependencies do |join|
           return join.base_klass if table_name == join.table_name

activerecord/lib/active_record/testing/query_assertions.rb

@@ -89,19 +89,31 @@ def assert_no_queries_match(match, include_schema: false, &block)
       end
 
       class SQLCounter # :nodoc:
-        attr_reader :log, :log_all
+        attr_reader :log_full, :log_all
 
         def initialize
-          @log = []
+          @log_full = []
           @log_all = []
         end
 
+        def log
+          @log_full.map(&:first)
+        end
+
         def call(*, payload)
           return if payload[:cached]
 
           sql = payload[:sql]
           @log_all << sql
-          @log << sql unless payload[:name] == "SCHEMA"
+
+          unless payload[:name] == "SCHEMA"
+            bound_values = payload[:binds].map do |value|
+              value = value.value_for_database if value.respond_to?(:value_for_database)
+              value
+            end
+
+            @log_full << [sql, bound_values]
+          end
         end
       end
     end

activerecord/lib/arel/nodes/bound_sql_literal.rb

@@ -6,13 +6,17 @@ class BoundSqlLiteral < NodeExpression
       attr_reader :sql_with_placeholders, :positional_binds, :named_binds
 
       def initialize(sql_with_placeholders, positional_binds, named_binds)
-        if !positional_binds.empty? && !named_binds.empty?
-          raise BindError.new("cannot mix positional and named binds", sql_with_placeholders)
-        elsif !positional_binds.empty?
+        has_positional = !(positional_binds.nil? || positional_binds.empty?)
+        has_named = !(named_binds.nil? || named_binds.empty?)
+
+        if has_positional
+          if has_named
+            raise BindError.new("cannot mix positional and named binds", sql_with_placeholders)
+          end
           if positional_binds.size != (expected = sql_with_placeholders.count("?"))
             raise BindError.new("wrong number of bind variables (#{positional_binds.size} for #{expected})", sql_with_placeholders)
           end
-        elsif !named_binds.empty?
+        elsif has_named
           tokens_in_string = sql_with_placeholders.scan(/:(?<!::)([a-zA-Z]\w*)/).flatten.map(&:to_sym).uniq
           tokens_in_hash = named_binds.keys.map(&:to_sym).uniq
 
@@ -26,7 +30,7 @@ def initialize(sql_with_placeholders, positional_binds, named_binds)
         end
 
         @sql_with_placeholders = sql_with_placeholders
-        if !positional_binds.empty?
+        if has_positional
           @positional_binds = positional_binds
           @named_binds = nil
         else

activerecord/lib/arel/update_manager.rb

@@ -16,7 +16,8 @@ def table(table)
     end
 
     def set(values)
-      if String === values
+      case values
+      when String, Nodes::BoundSqlLiteral
         @ast.values = [values]
       else
         @ast.values = values.map { |column, value|

activerecord/test/cases/adapters/abstract_mysql_adapter/bind_parameter_test.rb

@@ -49,7 +49,7 @@ def test_create_null_bytes
         end
 
         def test_where_with_string_for_string_column_using_bind_parameters
-          assert_quoted_as "'Welcome to the weblog'", "Welcome to the weblog"
+          assert_quoted_as "'Welcome to the weblog'", "Welcome to the weblog", match: 1
         end
 
         def test_where_with_integer_for_string_column_using_bind_parameters
@@ -73,14 +73,16 @@ def test_where_with_rational_for_string_column_using_bind_parameters
         end
 
         private
-          def assert_quoted_as(expected, value)
+          def assert_quoted_as(expected, value, match: 0)
             relation = Post.where("title = ?", value)
             assert_equal(
               %{SELECT `posts`.* FROM `posts` WHERE (title = #{expected})},
               relation.to_sql,
             )
-            assert_nothing_raised do # Make sure SQL is valid
-              relation.to_a
+            if match == 0
+              assert_empty relation.to_a
+            else
+              assert_equal match, relation.count
             end
           end
       end

activerecord/test/cases/adapters/postgresql/bind_parameter_test.rb

@@ -10,44 +10,40 @@ class BindParameterTest < ActiveRecord::PostgreSQLTestCase
         fixtures :posts
 
         def test_where_with_string_for_string_column_using_bind_parameters
-          assert_quoted_as "'Welcome to the weblog'", "Welcome to the weblog"
+          assert_quoted_as "'Welcome to the weblog'", "Welcome to the weblog", match: 1
         end
 
         def test_where_with_integer_for_string_column_using_bind_parameters
-          assert_quoted_as "0", 0, valid: false
+          assert_quoted_as "0", 0
         end
 
         def test_where_with_float_for_string_column_using_bind_parameters
-          assert_quoted_as "0.0", 0.0, valid: false
+          assert_quoted_as "0.0", 0.0
         end
 
         def test_where_with_boolean_for_string_column_using_bind_parameters
-          assert_quoted_as "FALSE", false, valid: false
+          assert_quoted_as "FALSE", false
         end
 
         def test_where_with_decimal_for_string_column_using_bind_parameters
-          assert_quoted_as "0.0", BigDecimal(0), valid: false
+          assert_quoted_as "0.0", BigDecimal(0)
         end
 
         def test_where_with_rational_for_string_column_using_bind_parameters
-          assert_quoted_as "0/1", Rational(0), valid: false
+          assert_quoted_as "0/1", Rational(0)
         end
 
         private
-          def assert_quoted_as(expected, value, valid: true)
+          def assert_quoted_as(expected, value, match: 0)
             relation = Post.where("title = ?", value)
             assert_equal(
               %{SELECT "posts".* FROM "posts" WHERE (title = #{expected})},
               relation.to_sql,
             )
-            if valid
-              assert_nothing_raised do # Make sure SQL is valid
-                relation.to_a
-              end
+            if match == 0
+              assert_empty relation.to_a
             else
-              assert_raises ActiveRecord::StatementInvalid do
-                relation.to_a
-              end
+              assert_equal match, relation.count
             end
           end
       end

activerecord/test/cases/adapters/sqlite3/bind_parameter_test.rb

@@ -10,7 +10,7 @@ class BindParameterTest < ActiveRecord::SQLite3TestCase
         fixtures :posts
 
         def test_where_with_string_for_string_column_using_bind_parameters
-          assert_quoted_as "'Welcome to the weblog'", "Welcome to the weblog"
+          assert_quoted_as "'Welcome to the weblog'", "Welcome to the weblog", match: 1
         end
 
         def test_where_with_integer_for_string_column_using_bind_parameters
@@ -34,14 +34,16 @@ def test_where_with_rational_for_string_column_using_bind_parameters
         end
 
         private
-          def assert_quoted_as(expected, value)
+          def assert_quoted_as(expected, value, match: 0)
             relation = Post.where("title = ?", value)
             assert_equal(
               %{SELECT "posts".* FROM "posts" WHERE (title = #{expected})},
               relation.to_sql,
             )
-            assert_nothing_raised do # Make sure SQL is valid
-              relation.to_a
+            if match == 0
+              assert_empty relation.to_a
+            else
+              assert_equal match, relation.count
             end
           end
       end

activerecord/test/cases/associations/belongs_to_associations_test.rb

@@ -252,9 +252,9 @@ def test_default_scope_on_relations_is_not_cached
     assert_equal 0, counter
     comment = comments.first
     assert_equal 0, counter
-    sql = capture_sql { comment.post }
+    queries = capture_sql_and_binds { comment.post }
     comment.reload
-    assert_not_equal sql, capture_sql { comment.post }
+    assert_not_equal queries, capture_sql_and_binds { comment.post }
   end
 
   def test_proxy_assignment

activerecord/test/cases/associations/has_many_associations_test.rb

@@ -171,9 +171,9 @@ def test_default_scope_on_relations_is_not_cached
     assert_equal 0, counter
     post = posts.first
     assert_equal 0, counter
-    sql = capture_sql { post.comments.to_a }
+    queries = capture_sql_and_binds { post.comments.to_a }
     post.comments.reset
-    assert_not_equal sql, capture_sql { post.comments.to_a }
+    assert_not_equal queries, capture_sql_and_binds { post.comments.to_a }
   end
 
   def test_has_many_build_with_options

activerecord/test/cases/bind_parameter_test.rb

@@ -102,7 +102,7 @@ def test_statement_cache_with_sql_string_literal
 
         topics = Topic.where("topics.id = ?", 1)
         assert_equal [1], topics.map(&:id)
-        assert_not_includes statement_cache, to_sql_key(topics.arel)
+        assert_includes statement_cache, to_sql_key(topics.arel)
       end
 
       def test_too_many_binds

activerecord/test/cases/finder_test.rb

@@ -200,7 +200,7 @@ def test_exists
     assert_equal false, Topic.exists?(9999999999999999999999999999999)
     assert_equal false, Topic.exists?(Topic.new.id)
 
-    assert_raise(NoMethodError) { Topic.exists?([1, 2]) }
+    assert_raise(ArgumentError) { Topic.exists?([1, 2]) }
   end
 
   def test_exists_with_scope

activerecord/test/cases/relation/merging_test.rb

@@ -146,14 +146,20 @@ def test_merge_doesnt_duplicate_same_clauses
 
     only_david = Author.where("#{author_id} IN (?)", david)
 
-    if current_adapter?(:Mysql2Adapter, :TrilogyAdapter)
-      assert_queries_match(/WHERE \(#{Regexp.escape(author_id)} IN \('1'\)\)\z/) do
-        assert_equal [david], only_david.merge(only_david)
+    matcher = if Author.connection.prepared_statements
+      if current_adapter?(:PostgreSQLAdapter)
+        /WHERE \(#{Regexp.escape(author_id)} IN \(\$1\)\)\z/
+      else
+        /WHERE \(#{Regexp.escape(author_id)} IN \(\?\)\)\z/
       end
+    elsif current_adapter?(:Mysql2Adapter, :TrilogyAdapter)
+      /WHERE \(#{Regexp.escape(author_id)} IN \('1'\)\)\z/
     else
-      assert_queries_match(/WHERE \(#{Regexp.escape(author_id)} IN \(1\)\)\z/) do
-        assert_equal [david], only_david.merge(only_david)
-      end
+      /WHERE \(#{Regexp.escape(author_id)} IN \(1\)\)\z/
+    end
+
+    assert_queries_match(matcher) do
+      assert_equal [david], only_david.merge(only_david)
     end
   end
 

activerecord/test/cases/relation_test.rb

@@ -204,7 +204,7 @@ def self.sanitize_sql(args)
 
       relation = Relation.new(klass)
       relation.merge!(where: ["foo = ?", "bar"])
-      assert_equal Relation::WhereClause.new(["foo = bar"]), relation.where_clause
+      assert_equal Relation::WhereClause.new([Arel.sql("(foo = ?)", "bar")]), relation.where_clause
     end
 
     def test_merging_readonly_false

activerecord/test/cases/relations_test.rb

@@ -476,9 +476,9 @@ def test_finding_with_complex_order
   def test_finding_with_sanitized_order
     query = Tag.order([Arel.sql("field(id, ?)"), [1, 3, 2]]).to_sql
     if current_adapter?(:Mysql2Adapter, :TrilogyAdapter)
-      assert_match(/field\(id, '1','3','2'\)/, query)
+      assert_match(/field\(id, '1',\s*'3',\s*'2'\)/, query)
     else
-      assert_match(/field\(id, 1,3,2\)/, query)
+      assert_match(/field\(id, 1,\s*3,\s*2\)/, query)
     end
 
     query = Tag.order([Arel.sql("field(id, ?)"), []]).to_sql