This project is archived and is in readonly mode.
PostgreSQL Adapter Breaks on Complex 'Order By' Clauses
The distinct and add_order_by_for_association_limiting methods in postgresql_adaptor.rb 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,
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.
Attached is a patch that fixes the problem through an improved parsing of the column expressions along with an update to the test_limited_eager_with_multiple_order_columns unit test so that it also tests for this case.
-
jayI believe this may be a dupe of ticket #1207 and that the issue itself was originally posted in the old Trac system here: http://dev.rubyonrails.org/ticke...
-
John Weathers
This issue does appear to overlap with ticket #1207.
Has that issue received feedback from Rails core and moved further along the path towards being committed to Rails?
I have applied for feedback for my patch, and the result of that feedback is in the following attachment which includes tests that focus on the column parsing - including one that breaks with the patch attached to #1207, but not with my updated patch.
-
Michael Koziarski- Assigned user set to Tarmo Tänav
assigning to tarmo to take a look. He's had the most experience in the postgresql adapter of late
-
Tarmo TänavThe patch looks generally good, I've got a couple of suggestions though. Firstly use "token << char" not "token += char" as the former modifies an existing string instead of generating a new one on each iteration. And the cases for '"' and "'" can be combined easily.
Also, it would be good to have specific tests for the parse_columns method covering a variety of ORDER BY cases that it's now supposed to support.
Thanks for your work
-
John Weathers
OK. I've take care of replacing the "+=" method call with "<<" in constructing the token string. I've also combined the single and double quote cases. Finally, I've added some additional tests to 'order_by_clause_parsing_test_postgresql.rb' that test for specific ORDER BY cases.
I'm attaching the updated patch file.
-
Michael Koziarski
- Milestone changed from 2.x to 2.3.3
The only thing which jumps out at me is that couldn't we avoid some of this expense for the really simple cases? e.g. if there's no ( or ) in the string, just split on ','?
-
Tarmo Tänav
Good point, indeed it is quite a bit less expensive for the common case to just split if there are no characters that the complex case is looking for. I'll make the change.
-
Michael Koziarski
- Milestone changed from 2.3.3 to 2.3.4
Moving to 2.3.4 rather than 2.3.3 as this isn't a regression.
-
-
Rizwan Reza- Tag changed from associations, columns, distinct, limit, order, parse, patch, postgresql to associations, bugmash, columns, distinct, limit, order, parse, patch, postgresql
-
-
-
-
-
rails- State changed from new to open
This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails core team are limited, and so we are asking for your help. If you can still reproduce this error on the 3-0-stable branch or on master, please reply with all of the information you have about it and add "[state:open]" to your comment. This will reopen the ticket for review. Likewise, if you feel that this is a very important feature for Rails to include, please reply with your explanation so we can consider it.
Thank you for all your contributions, and we hope you will understand this step to focus our efforts where they are most helpful.
-
rails
- State changed from open to stale