Refactor substitute_binds to perform substitution immediately

I'm honestly not sure if replacing bind params with their concrete
values is something that belongs in Arel at all, as it's something that
will need to be coupled to the quoting mechanism of the caller, and
could just be accomplished by using `Quoted` instead.

Still, with the new structure we can provide a much simpler API around
substitution. The expectation of the quoter responding to `quote` is a
reasonably minimal API.

I originally used `DelegateClass` here, with the one line override of
`add_bind`, but realized that we have some funky code going on where the
collector returns the next collector to use (in practice `self` is
always returned, and I don't see why we'd ever want to do this).
Removing that would likely be worthwhile, but would be a larger
refactoring

lib/arel/collectors/substitute_binds.rb

@@ -2,36 +2,27 @@
 module Arel
   module Collectors
     class SubstituteBinds
-      def initialize
-        @parts = []
+      def initialize(quoter, delegate_collector)
+        @quoter = quoter
+        @delegate = delegate_collector
       end
 
       def << str
-        @parts << str
+        delegate << str
         self
       end
 
       def add_bind bind
-        @parts << bind
-        self
+        self << quoter.quote(bind)
       end
 
-      def value; @parts; end
-
-      def substitute_binds bvs
-        bvs = bvs.dup
-        @parts.map do |val|
-          if Arel::Nodes::BindParam === val
-            bvs.shift
-          else
-            val
-          end
-        end
+      def value
+        delegate.value
       end
 
-      def compile bvs
-        substitute_binds(bvs).join
-      end
+      protected
+
+      attr_reader :quoter, :delegate
     end
   end
 end

lib/arel/visitors/oracle.rb

@@ -145,7 +145,7 @@ def split_order_string(string)
       end
 
       def visit_Arel_Nodes_BindParam o, collector
-        collector.add_bind(o) { |i| ":a#{i}" }
+        collector.add_bind(o.value) { |i| ":a#{i}" }
       end
 
     end

lib/arel/visitors/oracle12.rb

@@ -53,7 +53,7 @@ def visit_Arel_Nodes_UpdateStatement o, collector
       end
 
       def visit_Arel_Nodes_BindParam o, collector
-        collector.add_bind(o) { |i| ":a#{i}" }
+        collector.add_bind(o.value) { |i| ":a#{i}" }
       end
     end
   end

lib/arel/visitors/postgresql.rb

@@ -46,7 +46,7 @@ def visit_Arel_Nodes_DistinctOn o, collector
       end
 
       def visit_Arel_Nodes_BindParam o, collector
-        collector.add_bind(o) { |i| "$#{i}" }
+        collector.add_bind(o.value) { |i| "$#{i}" }
       end
 
       def visit_Arel_Nodes_GroupingElement o, collector

lib/arel/visitors/to_sql.rb

@@ -737,7 +737,7 @@ def visit_Arel_Attributes_Attribute o, collector
       def literal o, collector; collector << o.to_s; end
 
       def visit_Arel_Nodes_BindParam o, collector
-        collector.add_bind(o) { "?" }
+        collector.add_bind(o.value) { "?" }
       end
 
       alias :visit_Arel_Nodes_SqlLiteral :literal

test/collectors/test_substitute_bind_collector.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 require 'helper'
 require 'arel/collectors/substitute_binds'
+require 'arel/collectors/sql_string'
 
 module Arel
   module Collectors
@@ -11,61 +12,36 @@ def setup
         super
       end
 
-      def collect node
-        @visitor.accept(node, Collectors::SubstituteBinds.new)
-      end
-
-      def compile node
-        collect(node).value
-      end
-
-      def ast_with_binds bv
+      def ast_with_binds
         table = Table.new(:users)
         manager = Arel::SelectManager.new table
-        manager.where(table[:age].eq(bv))
-        manager.where(table[:name].eq(bv))
+        manager.where(table[:age].eq(Nodes::BindParam.new("hello")))
+        manager.where(table[:name].eq(Nodes::BindParam.new("world")))
         manager.ast
       end
 
-      def test_leaves_binds
-        node = Nodes::BindParam.new(nil)
-        list = compile node
-        assert_equal node, list.first
-        assert_equal node.class, list.first.class
-      end
-
-      def test_adds_strings
-        bv = Nodes::BindParam.new(nil)
-        list = compile ast_with_binds bv
-        assert_operator list.length, :>, 0
-        assert_equal bv, list.grep(Nodes::BindParam).first
-        assert_equal bv.class, list.grep(Nodes::BindParam).first.class
-      end
-
-      def test_substitute_binds
-        bv = Nodes::BindParam.new(nil)
-        collector = collect ast_with_binds bv
-
-        values = collector.value
-
-        offsets = values.map.with_index { |v,i|
-          [v,i]
-        }.find_all { |(v,_)| Nodes::BindParam === v }.map(&:last)
-
-        list = collector.substitute_binds ["hello", "world"]
-        assert_equal "hello", list[offsets[0]]
-        assert_equal "world", list[offsets[1]]
-
-        assert_equal 'SELECT FROM "users" WHERE "users"."age" = hello AND "users"."name" = world', list.join
+      def compile(node, quoter)
+        collector = Collectors::SubstituteBinds.new(quoter, Collectors::SQLString.new)
+        @visitor.accept(node, collector).value
       end
 
       def test_compile
-        bv = Nodes::BindParam.new(nil)
-        collector = collect ast_with_binds bv
-
-        sql = collector.compile ["hello", "world"]
+        quoter = Object.new
+        def quoter.quote(val)
+          val.to_s
+        end
+        sql = compile(ast_with_binds, quoter)
         assert_equal 'SELECT FROM "users" WHERE "users"."age" = hello AND "users"."name" = world', sql
       end
+
+      def test_quoting_is_delegated_to_quoter
+        quoter = Object.new
+        def quoter.quote(val)
+          val.inspect
+        end
+        sql = compile(ast_with_binds, quoter)
+        assert_equal 'SELECT FROM "users" WHERE "users"."age" = "hello" AND "users"."name" = "world"', sql
+      end
     end
   end
 end