Rails core - May 2009 - 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 =>
"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!

On Thu, May 7, 2009 at 7:46 PM, John  Weathers <johnweathers@gmail.com>
wrote:
>
> 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.
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

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

--
John Weathers
johnweathers@gmail.com

On Mon, May 11, 2009 at 12:58 PM, Jeremy Evans
<jeremyevans0@gmail.com>wrote:
>
> On Thu, May 7, 2009 at 7:46 PM, John  Weathers
<johnweathers@gmail.com>
> wrote:
> >
> > 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.
>
> 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
>
> >
>
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---