Saving "last_column" in visit_Arel_Attributes_Attributes may break Arel::Nodes::NamedFunction #153

gama opened this Issue Dec 14, 2012 · 1 comment


None yet
3 participants

gama commented Dec 14, 2012

Maybe I'm not "doing it right", but I cound't find any solutions by browsing through the source code or checking
Google... So, here it goes:

When calling a named function such as "CONCAT" with table columns/attributes among the parameters, the type of the column/attribute is used to convert the following attribute. As far as I could understand, this is required to properly convert the type of the right hand side of binary visitors, and is acomplished by saving the "last_column" instance attribute when visiting a Arel::Attribute::Atribute (which is later used by "quote").

However, when supplying a list of values to a named function (or any other arel node which uses "{}" I believe), the evaluation of the nodes sequentially breaks due to this type convertion.

An example always helps, so:

class T < ActiveRecord::Base
ts = T.arel_table'CONCAT', [ts['id'], '#']).to_sql

"CONCAT(, 0)"

Not that the '#' string is converted to 0, because the previous value in the array is an Arel::Attributes::Attribute whose type is "integer".

elia referenced this issue Jul 30, 2015

Make visitors threadsafe by removing @last_column
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

This is a first pass at fixing this by changing the signature of the
visit methods to accept the currently-relevant attribute, if any.

This comment has been minimized.

Show comment
Hide comment

elia Jul 30, 2015

This can be closed as per rails/rails#10655 (comment)

elia commented Jul 30, 2015

This can be closed as per rails/rails#10655 (comment)

@matthewd matthewd closed this Jul 31, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment