Support BindParams in subqueries

[jgraichen]

When an Arel AST contains a SelectManager as a component, e.g. as a CTE
expression, the SelectManager is converted to SQL with `#to_sql`. This
uses a new collector that leads to invalid expressions when using
bind parameters. For example, when generating PostgreSQL queries, the
bind parameter number starts from one again. When using the
SubstituteBinds collector, binds in the subquery are not substituted.

This commit changes the ToSql visitor to visit the SelectManager ast
itself.

Steps to reproduce

begin
  require 'bundler/inline'
rescue LoadError => e
  $stderr.puts 'Bundler version 1.10 or later is required. Please update your Bundler'
  raise e
end

gemfile(true) do
  source 'https://rubygems.org'
  gem 'arel', github: 'rails/arel'
  gem 'minitest'
end

require 'arel'
require 'arel/collectors/bind'
require 'arel/collectors/composite'
require 'arel/collectors/substitute_binds'

require 'minitest/autorun'
require 'logger'

class DummyConnection
  attr_accessor :visitor

  def quote(name)
    name.to_s.inspect
  end

  alias quote_table_name quote
  alias quote_column_name quote
end

class BugTest < Minitest::Test
  attr_reader :connection, :visitor, :collector

  def setup
    @connection = DummyConnection.new
    @visitor = Arel::Visitors::PostgreSQL.new(connection)

    # This is required because the ToSql visitor calls to_sql on the
    # SelectManager from the subquery
    connection.visitor = visitor
    Arel::Table.engine = Struct.new(:connection).new(connection)
  end

  def query
    table1 = Arel::Table.new(:a)
    table2 = Arel::Table.new(:b)
    table3 = Arel::Table.new(:c)

    # CTE query
    composed_cte = Arel::Nodes::As.new(
      table2,
      table3.project(Arel.star).where(
        table3[:field].eq(Arel::Nodes::BindParam.new('val1'))
      )
    )

    table1
      .project(Arel.star)
      .with(composed_cte)
      .where(table2[:field].eq(Arel::Nodes::BindParam.new('val2')))
  end

  def sql
    visitor.accept(query.ast, collector).value
  end

  def test_bind_param_extraction_in_psql_visitor_subquery
    @collector = Arel::Collectors::SubstituteBinds.new(
      connection,
      Arel::Collectors::SQLString.new
    )

    assert_equal sql,
      'WITH "b" AS (SELECT * FROM "c" WHERE "c"."field" = "val1") SELECT * FROM "a" WHERE "b"."field" = "val2"'
  end

  def test_postgresql_bind_param_index_numbers
    @collector = Arel::Collectors::SQLString.new

    assert_equal sql,
      'WITH "b" AS (SELECT * FROM "c" WHERE "c"."field" = $1) SELECT * FROM "a" WHERE "b"."field" = $2'
  end
end

Output

  1) Failure:
BugTest#test_bind_param_extraction_in_psql_visitor_subquery [arel8.rb:75]:
--- expected
+++ actual
@@ -1 +1 @@
-"WITH \"b\" AS (SELECT * FROM \"c\" WHERE \"c\".\"field\" = $1) SELECT * FROM \"a\" WHERE \"b\".\"field\" = \"val2\""
+"WITH \"b\" AS (SELECT * FROM \"c\" WHERE \"c\".\"field\" = \"val1\") SELECT * FROM \"a\" WHERE \"b\".\"field\" = \"val2\""

  2) Failure:
BugTest#test_postgresql_bind_param_index_numbers [arel8.rb:82]:
--- expected
+++ actual
@@ -1 +1 @@
-"WITH \"b\" AS (SELECT * FROM \"c\" WHERE \"c\".\"field\" = $1) SELECT * FROM \"a\" WHERE \"b\".\"field\" = $1"
+"WITH \"b\" AS (SELECT * FROM \"c\" WHERE \"c\".\"field\" = $1) SELECT * FROM \"a\" WHERE \"b\".\"field\" = $2"

System configuration

Ruby version: ruby 2.4.2p198 (2017-09-14 revision 59899) [x86_64-linux]

A fix is attached and all existing specs are passing, but I'm still looking where to add the test from the example above.

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @tenderlove (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.