PostgreSQL Adapter Breaks on Complex 'Order By' Clauses

rubyonrails-core
1

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 => "authors.name = 'David'", :order => "COALESCE(UPPER(posts.title), 'No Title') DESC, posts.id", :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 here: https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/2622-postgresql-adapter-breaks-on-complex-order-by-clauses

I now need feedback for the patch.

Thanks!

2

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.

Jeremy

3

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.

https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/2622-postgresql-adapter-breaks-on-complex-order-by-clauses