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.
In commit 68a95542e1 the last_column feature of ToSql was removed. The visit_Arel_Nodes_Matches and visit_Arel_Nodes_DoesNotMatch methods are overwritten in the PostgreSQL class, but were not updated appropriately. This commit fixes the issue accordingly.
This bug affects at least all update_all statements in Rails 4.0.2 that have subqueries with ILIKE statements on PostgreSQL. The bug is present in Arel 4.0.1 and later, so it probably affects most Rails 4.0.2 projects.
It would be highly appreciated if Arel 4 could get a point release as well. Thanks for your continued work.
The last_column feature of the ToSql visitor and its descendants is what
enabled quoting based on the column last visited -- in other words, if
you have a standard condition like an equality with a string attribute
on the left side and an integer on the right side, then when ARel visits
the node, it'll first visit the left side attribute, setting the
column of the string attribute as the last column, and resulting in the
right side of the condition getting the appropriate quoting.
The downside is that this means that visitors can't be shared between
threads, because of the state mutation. It also makes for some really
weird behavior in the event that the visitor visits a node that happens
to contain an attribute you weren't expecting to be there, since it'll
potentially quote something based on that attribute. So, it prevents
reversing an equality condition. column = value will work, but not value
= column, since the last column wouldn't be the column you're hoping
for.
This is a first pass at fixing this by changing the signature of the
visit methods to accept the currently-relevant attribute, if any.
