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.

6 files changed, 27 insertions, 10 deletions

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