#2622 PostgreSQL Adapter Breaks on Complex 'Order By' Clauses - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#2622 ✓stale

PostgreSQL Adapter Breaks on Complex 'Order By' Clauses

Reported by John Weathers | May 8th, 2009 @ 03:29 AM | in 2.3.10

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.

patch_postgresql_column_parsing.diff 4.9 KB

Comments and changes to this ticket

You flagged this item as spam.
jay May 11th, 2009 @ 03:38 PM
I 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...
You flagged this item as spam.
John Weathers May 27th, 2009 @ 09:29 PM
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.
- postgresql_order_by_clause_parsing_patch.diff 7.7 KB
Michael Koziarski June 1st, 2009 @ 01:06 AM
- 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änav June 1st, 2009 @ 02:43 AM
The 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
You flagged this item as spam.
John Weathers June 2nd, 2009 @ 08:32 PM
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.
- postgresql_order_by_clause_parsing_patch.diff 8.6 KB
Michael Koziarski June 8th, 2009 @ 08:56 AM
- 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 June 8th, 2009 @ 09:56 AM
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 June 9th, 2009 @ 08:45 AM
- 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.
Jeremy Kemper September 11th, 2009 @ 11:04 PM
- Milestone changed from “2.3.4” to “2.3.6”
[milestone:id#50064 bulk edit command]
Rizwan Reza May 16th, 2010 @ 02:41 AM
- Tag changed from “associations, columns, distinct, limit, order, parse, patch, postgresql” to “associations, bugmash, columns, distinct, limit, order, parse, patch, postgresql”
[bulk edit]
Jeremy Kemper May 23rd, 2010 @ 05:54 PM
- Milestone changed from “2.3.6” to “2.3.7”
[bulk edit]
Jeremy Kemper May 24th, 2010 @ 09:40 AM
- Milestone changed from “2.3.7” to “2.3.8”
[bulk edit]
Jeremy Kemper May 25th, 2010 @ 11:45 PM
- Milestone changed from “2.3.8” to “2.3.9”
[bulk edit]
Jeremy Kemper August 30th, 2010 @ 02:28 AM
- Milestone changed from “2.3.9” to “2.3.10”
- Importance changed from “” to “High”
[bulk edit]
rails February 26th, 2011 @ 12:00 AM
- 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 February 26th, 2011 @ 12:00 AM
- State changed from “open” to “stale”