Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fixe #reverse_order incorrect behavior #8225

Open
wants to merge 1 commit into from

7 participants

@shemerey

Fixed incorrect behavior in situations when the string containing function comes in method order. for example SUBSTR()

  Sybase:     SUBSTR(field, 1, 10)
  MySql:      SUBSTR(field, 1, 10)
  Postgresql: SUBSTR(field, 1, 10)
  Sqlite:     SUBSTR(field, 1, 10)
  Oracle:     SUBSTR(field, 1, 10)
  DB2:        SUBSTR(field, 1, 10)

“,” was used as means for dividing fields where sorting takes place, as a result of that invalid sql was generated, example:

Post.order('SUBSTR(name, 1, 10)').reverse_order.to_sql

expected:

SELECT "posts".* FROM "posts" ORDER BY SUBSTR(name, 1, 10) DESC

got

SELECT "posts".* FROM "posts" ORDER BY SUBSTR(name DESC, 1 DESC, 10) DESC

This fixes the incorrect behavior. for that purpose i added private method (#split_order_expressions) that split only by expected "," Other behavior was not affected.

@shemerey

you could check my benchmarks for different implementations here https://gist.github.com/4079000

activerecord/test/cases/relation_scoping_test.rb
@@ -31,6 +31,10 @@ def test_double_reverse_order_produces_original_order
assert_equal Developer.order("name DESC"), Developer.order("name DESC").reverse_order.reverse_order
end
+ def test_reverse_order_with_function_in_order
+ assert_equal Developer.order('SUBSTR(name, 1, 100)').to_a, Developer.order("substr(name, 1, 100) DESC").reverse_order

Remove extra space after assert_equal

thx. removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
activerecord/lib/active_record/relation/query_methods.rb
@@ -791,6 +791,24 @@ def build_select(arel, selects)
end
end
+ def split_order_expressions(str)
+ depth, index, expressions = 0, 0, []
+ current = expressions[index] = ''
+
+ str.each_char do |char|
+ depth += 1 if char == '('
+ depth -= 1 if char == ')'
+ if depth == 0 && char == ','
+ index += 1
+ current = expressions[index] = ''

Extra space after =

thx. removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva

It also needs a changelog entry.. The overall idea looks good, but we're back to parsing sql strings as @tenderlove commented some time ago =/. Thanks!

@shemerey

changelog updated

@shemerey

sorry for bother you, but maybe any news on this?

@carlosantoniodasilva

@shemerey I'm going to take a look again soon, and you ping you back, sorry for the delay.

@shemerey

no problems just let me know what will be your decision

@shemerey shemerey Fixed #reverse_order incorect behavior
Fixed incorrect behavior in situations when the string containing function comes in method order. for example SUBSTR()

  Sybase:     SUBSTR(field, 1, 10)
  MySql:      SUBSTR(field, 1, 10)
  Postgresql: SUBSTR(field, 1, 10)
  Sqlite:     SUBSTR(field, 1, 10)
  Oracle:     SUBSTR(field, 1, 10)
  DB2:        SUBSTR(field, 1, 10)

  “,” was used as means for dividing fields where sorting takes place, as a result of that invalid sql was generated
  example:
      Post.order('SUBSTR(name, 1, 10)').reverse_order.to_sql
  expected:   #=> "SELECT \"posts\".* FROM \"posts\"   ORDER BY SUBSTR(name, 1, 10) DESC"
  got:        #=> "SELECT \"posts\".* FROM \"posts\"   ORDER BY SUBSTR(name DESC, 1 DESC, 10) DESC"

This fixes the incorrect behavior. for that purpose i added private method (#split_order_expressions)
that split only by expected "," Other behavior was not affected.
09bcbeb
@shemerey

Hi i've just add Improvements to this bugfix

return str.split(',') unless str =~ /[)(]/

new benchmarks here https://gist.github.com/4079000#gistcomment-618458

@carlosantoniodasilva

@shemerey thanks!

@tenderlove could you give some input here please? <3

@shemerey

do you have any news about this commit ?

@al2o3cr

This seems quite close to the "implement a SQL parser" thing that it sounds like the core team would prefer not to do.

@matthewd
Collaborator

Yeah... I could see treating it as a good-enough improvement: explicitly not handling strings, for example, but allowing one to solve a decent subset of issues by wrapping the sort expression with parens.

That said, even if this is desirable/acceptable/tolerable, the implementation here leaves quite a bit to be desired.

@nathany

I just discovered reverse_order and promptly ran into this issue.

to_char(work_orders.number,'0000000000')
becomes to_char(work_orders.number DESC,'0000000000') DESC

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/query_methods.rb#L1060

To avoid parsing SQL (even just splitting on commas in SQL), maybe it could just append DESC on the end of the string?

Maybe order("a, b") shouldn't be supported/encouraged, but order("a").order("b") or order(:a, :b) would continue to become ORDER BY a DESC, b DESC. Thoughts?

@nathany

Should I open a new issue for this where other solutions could be proposed?

Unfortunately fixing this bug will likely be a breaking change.

@rafaelfranca

I believe the best thing we can do is disallow reverse_order when the argument is string. For these cases people can use reorder.

@rafaelfranca

#7423 and #11571 are related and will be fixed if we follow this plan.

@nathany

@rafaelfranca I was hoping to clean up some of our reporting code using reverse_order, but some of our sorting relies on strings containing SQL functions (eg. UPPER). Appending DESC on the end of the string would work for us, so long as we never have multiple columns in a single string.

Your proposal will leave us appending "DESC" to the strings manually.

You know the Rails codebase far better than I, so I trust your judgement. I just wanted to let you know our use case. Thanks.

@notlaforge

I was able to sidestep this bug by giving the function an alias in the SELECT clause, like so:

Model.select('*, some_func(foo, bar) AS sort_col').order("sort_col ASC")
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 7, 2012
  1. @shemerey

    Fixed #reverse_order incorect behavior

    shemerey authored
    Fixed incorrect behavior in situations when the string containing function comes in method order. for example SUBSTR()
    
      Sybase:     SUBSTR(field, 1, 10)
      MySql:      SUBSTR(field, 1, 10)
      Postgresql: SUBSTR(field, 1, 10)
      Sqlite:     SUBSTR(field, 1, 10)
      Oracle:     SUBSTR(field, 1, 10)
      DB2:        SUBSTR(field, 1, 10)
    
      “,” was used as means for dividing fields where sorting takes place, as a result of that invalid sql was generated
      example:
          Post.order('SUBSTR(name, 1, 10)').reverse_order.to_sql
      expected:   #=> "SELECT \"posts\".* FROM \"posts\"   ORDER BY SUBSTR(name, 1, 10) DESC"
      got:        #=> "SELECT \"posts\".* FROM \"posts\"   ORDER BY SUBSTR(name DESC, 1 DESC, 10) DESC"
    
    This fixes the incorrect behavior. for that purpose i added private method (#split_order_expressions)
    that split only by expected "," Other behavior was not affected.
This page is out of date. Refresh to see the latest.
View
8 activerecord/CHANGELOG.md
@@ -1,4 +1,12 @@
## Rails 4.0.0 (unreleased) ##
+* Fixed #reverse_order incorect behavior
+ in situations when the string containing function comes in method order.
+
+ Post.order('SUBSTR(name, 1, 10)').reverse_order.to_sql
+ expected: #=> "SELECT \"posts\".* FROM \"posts\" ORDER BY SUBSTR(name, 1, 10) DESC"
+ got: #=> "SELECT \"posts\".* FROM \"posts\" ORDER BY SUBSTR(name DESC, 1 DESC, 10) DESC"
+
+ Shemerey Anton
* `#pluck` can be used on a relation with `select` clause
Fix #7551
View
22 activerecord/lib/active_record/relation/query_methods.rb
@@ -791,6 +791,26 @@ def build_select(arel, selects)
end
end
+ def split_order_expressions(str)
+ return str.split(',') unless str =~ /[)(]/
+
+ depth, index, expressions = 0, 0, []
+ current = expressions[index] = ''
+
+ str.each_char do |char|
+ depth += 1 if char == '('
+ depth -= 1 if char == ')'
+ if depth == 0 && char == ','
+ index += 1
+ current = expressions[index] = ''
+ else
+ current << char
+ end
+ end
+
+ expressions
+ end
+
def reverse_sql_order(order_query)
order_query = ["#{quoted_table_name}.#{quoted_primary_key} ASC"] if order_query.empty?
@@ -799,7 +819,7 @@ def reverse_sql_order(order_query)
when Arel::Nodes::Ordering
o.reverse
when String
- o.to_s.split(',').collect do |s|
+ split_order_expressions(o.to_s).collect do |s|
s.strip!
s.gsub!(/\sasc\Z/i, ' DESC') || s.gsub!(/\sdesc\Z/i, ' ASC') || s.concat(' DESC')
end
View
4 activerecord/test/cases/relation_scoping_test.rb
@@ -31,6 +31,10 @@ def test_double_reverse_order_produces_original_order
assert_equal Developer.order("name DESC"), Developer.order("name DESC").reverse_order.reverse_order
end
+ def test_reverse_order_with_function_in_order
+ assert_equal Developer.order('SUBSTR(name, 1, 100)').to_a, Developer.order("substr(name, 1, 100) DESC").reverse_order
+ end
+
def test_scoped_find
Developer.where("name = 'David'").scoping do
assert_nothing_raised { Developer.find(1) }
Something went wrong with that request. Please try again.