PostgreSQL Adapter Breaks on Complex 'Order By' Clauses

The 'distinct' and 'add_order_by_for_association_limiting' methods in
the PostgreSQL adaptor assume that incoming 'order by' clauses can be
parsed simply by splitting on commas. This fails whenever a more
complex expression is passed involving multiple columns with one or
more function calls having multiple arguments.

For example, the code

Post.find(:all, :include => [:author, :comments], :conditions =>
" = 'David'", :order => "COALESCE(UPPER(posts.title), 'No
Title') DESC,", :limit => 2, :offset => 1)

will result in broken SQL as ActiveRecord tries to associate aliases
with all the "columns" that it finds including the arguments to
COALESCE which it will see as two columns instead of one column that
is modified by a function.

I have documented the issue and submitted a patch that resolves it

I now need feedback for the patch.


Probably an improvement to the current code, but it still fails for some inputs:

  irb(main):045:0> parse_columns("'a\\',b', b")
  => ["'a\\'", "b', b"]

This assumes you don't have standard_conforming_strings set for the
connection. Of course, to be correct in all cases, your parser needs
to check the standard_conforming_strings setting and operate
appropriately based on it.

The patch also has a problem with arrays:

  irb(main):047:0> parse_columns("ARRAY[1,2,3+4]")
  => ["ARRAY[1", "2", "3+4]"]

Also, your add_order_by_for_association_limiting! change doesn't
consider whitespace after the ASC/DESC modifier:

  irb(main):052:0> parse_columns("a DESC , b DESC , c").map { |s| m =
s.match(/(?:\b(?:asc|desc))?$/i)[0]; (m=='') ? ' ASC' : m.upcase }
  => [" ASC", " ASC", " ASC"]
  irb(main):053:0> parse_columns("a DESC, b DESC, c").map { |s| m =
s.match(/(?:\b(?:asc|desc))?$/i)[0]; (m=='') ? ' ASC' : m.upcase }
  => ["DESC", "DESC", " ASC"]

Of course, you can modify parse_columns and related code to improve
the parsing. Considering the very limited scope of what you are
trying to do (separate an expression string into an array of
expressions), you might even be able to get something that works for
all possible inputs.


Thanks for the feedback for issue #2622.

I have updated the patch to address the cases below where the patched version still failed. I have also added some more tests that specifically target the parsing rather than the side effects of the parsing.