Add order to BindParams in the ToSql collector

This removes the need for us to do the re-ordering by walking the AST in
ActiveRecord. We're using a block to communicate with the collector,
since the collector needs to be the thing which knows about the index,
while the visitor is the thing that needs to know the syntax. The
BindParam needs to know about neither of these things, so it's been
changed to stop being a subclass of SqlLiteral

I could also see an alternative implementation using format strings if
for some reason blocks cause a problem.

12 files changed, 48 insertions, 16 deletions

diff --git a/lib/arel/collectors/sql_string.rb b/lib/arel/collectors/sql_string.rb
index 8ca89ca7bd..fd2faaef3a 100644
--- a/lib/arel/collectors/sql_string.rb
+++ b/lib/arel/collectors/sql_string.rb
@@ -5,8 +5,14 @@ require 'arel/collectors/plain_string'
 module Arel
   module Collectors
     class SQLString < PlainString
+      def initialize(*)
+        super
+        @bind_index = 1
+      end
+
       def add_bind bind
-        self << bind.to_s
+        self << yield(@bind_index)
+        @bind_index += 1
         self
       end
 
diff --git a/lib/arel/nodes.rb b/lib/arel/nodes.rb
index ccccd471e2..2c3c48881b 100644
--- a/lib/arel/nodes.rb
+++ b/lib/arel/nodes.rb
@@ -4,6 +4,7 @@ require 'arel/nodes/select_statement'
 require 'arel/nodes/select_core'
 require 'arel/nodes/insert_statement'
 require 'arel/nodes/update_statement'
+require 'arel/nodes/bind_param'
 
 # terminal
 
diff --git a/lib/arel/nodes/bind_param.rb b/lib/arel/nodes/bind_param.rb
new file mode 100644
index 0000000000..160bc21b91
--- /dev/null
+++ b/lib/arel/nodes/bind_param.rb
@@ -0,0 +1,6 @@
+module Arel
+  module Nodes
+    class BindParam < Node
+    end
+  end
+end
diff --git a/lib/arel/nodes/sql_literal.rb b/lib/arel/nodes/sql_literal.rb
index b43288b29c..2c56644b99 100644
--- a/lib/arel/nodes/sql_literal.rb
+++ b/lib/arel/nodes/sql_literal.rb
@@ -10,8 +10,5 @@ module Arel
         coder.scalar = self.to_s
       end
     end
-
-    class BindParam < SqlLiteral
-    end
   end
 end
diff --git a/lib/arel/visitors/postgresql.rb b/lib/arel/visitors/postgresql.rb
index 60878ddd20..f55aaf30fe 100644
--- a/lib/arel/visitors/postgresql.rb
+++ b/lib/arel/visitors/postgresql.rb
@@ -23,6 +23,10 @@ module Arel
         collector << "DISTINCT ON ( "
         visit(o.expr, collector) << " )"
       end
+
+      def visit_Arel_Nodes_BindParam o, collector
+        collector.add_bind(o) { |i| "$#{i}" }
+      end
     end
   end
 end
diff --git a/lib/arel/visitors/to_sql.rb b/lib/arel/visitors/to_sql.rb
index a3f8cb565d..884076d987 100644
--- a/lib/arel/visitors/to_sql.rb
+++ b/lib/arel/visitors/to_sql.rb
@@ -186,7 +186,8 @@ module Arel
 
         len = o.expressions.length - 1
         o.expressions.zip(o.columns).each_with_index { |(value, attr), i|
-          if Nodes::SqlLiteral === value
+          case value
+          when Nodes::SqlLiteral, Nodes::BindParam
             collector = visit value, collector
           else
             collector << quote(value, attr && column_for(attr)).to_s
@@ -713,7 +714,7 @@ module Arel
       def literal o, collector; collector << o.to_s; end
 
       def visit_Arel_Nodes_BindParam o, collector
-        collector.add_bind o
+        collector.add_bind(o) { "?" }
       end
 
       alias :visit_Arel_Nodes_SqlLiteral :literal
diff --git a/test/collectors/test_bind_collector.rb b/test/collectors/test_bind_collector.rb
index 60532f061c..da55244a82 100644
--- a/test/collectors/test_bind_collector.rb
+++ b/test/collectors/test_bind_collector.rb
@@ -27,14 +27,14 @@ module Arel
       end
 
       def test_leaves_binds
-        node = Nodes::BindParam.new 'omg'
+        node = Nodes::BindParam.new
         list = compile node
         assert_equal node, list.first
         assert_equal node.class, list.first.class
       end
 
       def test_adds_strings
-        bv = Nodes::BindParam.new('?')
+        bv = Nodes::BindParam.new
         list = compile ast_with_binds bv
         assert_operator list.length, :>, 0
         assert_equal bv, list.grep(Nodes::BindParam).first
@@ -42,7 +42,7 @@ module Arel
       end
 
       def test_substitute_binds
-        bv = Nodes::BindParam.new('?')
+        bv = Nodes::BindParam.new
         collector = collect ast_with_binds bv
 
         values = collector.value
@@ -59,7 +59,7 @@ module Arel
       end
 
       def test_compile
-        bv = Nodes::BindParam.new('?')
+        bv = Nodes::BindParam.new
         collector = collect ast_with_binds bv
 
         sql = collector.compile ["hello", "world"]
diff --git a/test/collectors/test_sql_string.rb b/test/collectors/test_sql_string.rb
index 6d2e23151b..cd121a1364 100644
--- a/test/collectors/test_sql_string.rb
+++ b/test/collectors/test_sql_string.rb
@@ -27,7 +27,7 @@ module Arel
       end
 
       def test_compile
-        bv = Nodes::BindParam.new('?')
+        bv = Nodes::BindParam.new
         collector = collect ast_with_binds bv
 
         sql = collector.compile ["hello", "world"]
diff --git a/test/test_update_manager.rb b/test/test_update_manager.rb
index f1a019970d..d636ab548f 100644
--- a/test/test_update_manager.rb
+++ b/test/test_update_manager.rb
@@ -12,7 +12,7 @@ module Arel
       table = Table.new(:users)
       um = Arel::UpdateManager.new Table.engine
       um.table table
-      um.set [[table[:name], (Arel::Nodes::BindParam.new '?')]]
+      um.set [[table[:name], Arel::Nodes::BindParam.new]]
       um.to_sql.must_be_like %{ UPDATE "users" SET "name" =  ? }
     end
 
diff --git a/test/visitors/test_bind_visitor.rb b/test/visitors/test_bind_visitor.rb
index 333636ed51..79d340e5cd 100644
--- a/test/visitors/test_bind_visitor.rb
+++ b/test/visitors/test_bind_visitor.rb
@@ -18,7 +18,7 @@ module Arel
       def test_assignment_binds_are_substituted
         table = Table.new(:users)
         um = Arel::UpdateManager.new Table.engine
-        bp = Nodes::BindParam.new '?'
+        bp = Nodes::BindParam.new
         um.set [[table[:name], bp]]
         visitor = Class.new(Arel::Visitors::ToSql) {
           include Arel::Visitors::BindVisitor
@@ -38,7 +38,7 @@ module Arel
           include Arel::Visitors::BindVisitor
         }.new nil
 
-        bp = Nodes::BindParam.new 'omg'
+        bp = Nodes::BindParam.new
         called = false
         visitor.accept(bp, collector) { called = true }
         assert called
diff --git a/test/visitors/test_postgres.rb b/test/visitors/test_postgres.rb
index 3d646a7324..368feb5977 100644
--- a/test/visitors/test_postgres.rb
+++ b/test/visitors/test_postgres.rb
@@ -117,6 +117,16 @@ module Arel
           }
         end
       end
+
+      describe "Nodes::BindParam" do
+        it "increments each bind param" do
+          query = @table[:name].eq(Arel::Nodes::BindParam.new)
+            .and(@table[:id].eq(Arel::Nodes::BindParam.new))
+          compile(query).must_be_like %{
+            "users"."name" = $1 AND "users"."id" = $2
+          }
+        end
+      end
     end
   end
 end
diff --git a/test/visitors/test_to_sql.rb b/test/visitors/test_to_sql.rb
index 9c18d74827..7895866809 100644
--- a/test/visitors/test_to_sql.rb
+++ b/test/visitors/test_to_sql.rb
@@ -16,9 +16,16 @@ module Arel
       end
 
       it 'works with BindParams' do
-        node = Nodes::BindParam.new 'omg'
+        node = Nodes::BindParam.new
         sql = compile node
-        sql.must_be_like 'omg'
+        sql.must_be_like '?'
+      end
+
+      it 'does not quote BindParams used as part of a Values' do
+        bp = Nodes::BindParam.new
+        values = Nodes::Values.new([bp])
+        sql = compile values
+        sql.must_be_like 'VALUES (?)'
       end
 
       it 'can define a dispatch method' do