Raise AR::IrreversibleOrderError when #reverse_order can not do it's job #18928

bogdan · 2015-02-13T14:19:41Z

Current #reverse_order algorithm is very simple and doesn't try to
handle complicated cases of reversing order given as string.
This patch makes AR to raise exception instead of generating invalid SQL
inside #reverse_order.

Fixes #8225, #11571

This patch is alternative to #16779 and #18754.

sgrif · 2015-02-13T15:26:08Z

activerecord/lib/active_record/relation/query_methods.rb


      order_query.flat_map do |o|
        case o
        when Arel::Nodes::Ordering
          o.reverse
        when String
-          o.to_s.split(',').map! do |s|
+          if o =~ /\([^()]*,[^()]*\)/


Can we pull this out into a method that states what it's looking for, like contains_functions_or_complex_sql_expressions_or_whatever_i_dont_thing_regexes_are_great_at_expressing_intent?(o)

Maybe a comment is better here?

I'd prefer a method, which is less likely to get out of sync with the code.

sgrif · 2015-02-13T15:28:05Z

This is definitely my preferred solution to the problem. I think we might need to have a deprecation cycle though, if there was previously a case where this could generate valid SQL, even if that SQL was incorrect.

bogdan · 2015-02-13T15:58:03Z

@sgrif current regexp basically checks if there is a function call that has at least one comma inside like:

# match and raises
concat(field1, field2)
length('hello, world')
# not match and generates reverse
length(field1), length(field2)

Lets try to find a case when , inside ( ) can still result in valid SQL.

kaspth · 2015-02-13T16:57:28Z

activerecord/CHANGELOG.md

@@ -1,3 +1,17 @@
+*   `AR::Relation#reverse_order` throwns `AR::UnreversableOrderError` when the order can not be reversed
+    using current trival algorithm. Also raises the same errow when `#reverse_order` is called on


Typos in trivial and error here 😉

bogdan · 2015-02-13T19:58:33Z

Renamed exception to IrreversibleOrderError
Fixed typos in docs
Renamed method

Thanks everyone for good comments.

vipulnsward · 2015-02-13T20:06:28Z

activerecord/CHANGELOG.md

@@ -1,3 +1,17 @@
+*   `AR::Relation#reverse_order` throwns `AR::IrreversableOrderError` when the order can not be reversed


egilburg · 2015-02-13T20:18:43Z

activerecord/CHANGELOG.md

@@ -1,3 +1,17 @@
+*   `AR::Relation#reverse_order` throws `AR::IrreversableOrderError` when the order can not be reversed


sorry but this is still typo should be Irreversible, not Irreversable :-)

fixed. 👍

egilburg · 2015-02-13T20:40:09Z

activerecord/test/cases/relations_test.rb

+
+  def test_reverse_order_with_multiargument_function
+    assert_raises(ActiveRecord::IrreversibleOrderError) do
+      Topic.order("concat(author_name, title)").reverse_order


Why is a concat too complex to reverse? I understand the example with the nested DESC, but not this one. Unless I misread, this should still work if you swap the single ASC with DESC or vice versa, without changing anything in the function.

The following in stable rails: Topic.order("concat(author_name, title)").reverse_order.to_sql will give you order by concat(author_name desc, title) desc.

The goal of this patch is not to improve a way how rails reverse order but clearly mark cases that rails can not handle.

Better to fix reverse_orderimho

@slbug no matter how hard you try, you will not be able to support all use cases. This patch is not about making reverse_order better, but about confirming it is not perfect and will never be.

matthewd · 2015-02-18T07:45:24Z

Is as much as this resembles #16779 (comment), I'm clearly 👍.

But I suspect the current might-this-be-bad regexp is too narrow.

sgrif · 2015-02-20T17:26:45Z

We should check for NULLS FIRST and friends as well.

bogdan · 2015-02-23T15:16:20Z

@sgrif added /nulls (first|last)\Z/i. Something else?

bogdan · 2016-01-26T15:44:20Z

@sgrif any time to come back to this issue?

sgrif · 2016-01-26T19:30:14Z

It looks like you never pushed up the nulls first/last change. Can you do that and squash/rebase than ping me again?

Raises when #reverse_order can not process SQL order instead of making invalid SQL before this patch

bogdan · 2016-01-27T11:03:53Z

@sgrif done.

Raise AR::IrreversibleOrderError when #reverse_order can not do it's job

sgrif reviewed Feb 13, 2015
View reviewed changes

bogdan force-pushed the unreversable-order branch from aee0039 to 5a9352f Compare February 13, 2015 16:21

kaspth reviewed Feb 13, 2015
View reviewed changes

bogdan force-pushed the unreversable-order branch from 5a9352f to 1c1b52a Compare February 13, 2015 19:55

vipulnsward reviewed Feb 13, 2015
View reviewed changes

bogdan force-pushed the unreversable-order branch from 1c1b52a to 1c3d9de Compare February 13, 2015 20:14

egilburg reviewed Feb 13, 2015
View reviewed changes

bogdan force-pushed the unreversable-order branch from 1c3d9de to 9011ddd Compare February 13, 2015 20:33

bogdan changed the title ~~Raise AR::UnreversableOrderError when #reverse_order can not do it's job~~ Raise AR::IrreversibleOrderError when #reverse_order can not do it's job Feb 13, 2015

egilburg reviewed Feb 13, 2015
View reviewed changes

bogdan force-pushed the unreversable-order branch from 9011ddd to 115422b Compare February 16, 2015 15:43

rafaelfranca added the activerecord label Feb 19, 2015

bogdan force-pushed the unreversable-order branch from 115422b to 1264d9d Compare February 23, 2015 14:54

bogdan force-pushed the unreversable-order branch from 1264d9d to 9011ddd Compare June 13, 2015 09:30

rails-bot assigned sgrif Oct 20, 2015

sgrif added this to the 5.0.0 milestone Jan 26, 2016

Introduce ActiveRecord::IrreversibleOrderError

14efb42

Raises when #reverse_order can not process SQL order instead of making invalid SQL before this patch

bogdan force-pushed the unreversable-order branch from 9011ddd to 14efb42 Compare January 27, 2016 10:54

sgrif added a commit that referenced this pull request Jan 28, 2016

Merge pull request #18928 from bogdan/unreversable-order

74bee83

Raise AR::IrreversibleOrderError when #reverse_order can not do it's job

sgrif merged commit 74bee83 into rails:master Jan 28, 2016

bogdan mentioned this pull request Jan 28, 2016

Always use SQL limit in Relation#last when limit argument given #16400

Merged

bogdan deleted the unreversable-order branch January 29, 2016 10:43

mjhoy mentioned this pull request Apr 18, 2017

WIP - disallow reverse_order when the argument is string. #16779

Closed

2 tasks

kamipo mentioned this pull request Feb 14, 2019

Make reverse_sql_order work correctly if "nulls first/last" is in the middle of "order by". #35267

Closed

ghiculescu mentioned this pull request Mar 18, 2022

reverse_sql_order fails to reverse reversible order #44719

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise AR::IrreversibleOrderError when #reverse_order can not do it's job #18928

Raise AR::IrreversibleOrderError when #reverse_order can not do it's job #18928

bogdan commented Feb 13, 2015

sgrif Feb 13, 2015

bogdan Feb 13, 2015

sgrif Feb 13, 2015

bogdan Feb 13, 2015

sgrif commented Feb 13, 2015

bogdan commented Feb 13, 2015

kaspth Feb 13, 2015

bogdan commented Feb 13, 2015

vipulnsward Feb 13, 2015

bogdan Feb 13, 2015

egilburg Feb 13, 2015

bogdan Feb 13, 2015

egilburg Feb 13, 2015

bogdan Feb 13, 2015

slbug Jan 28, 2016

bogdan Jan 28, 2016

matthewd commented Feb 18, 2015

sgrif commented Feb 20, 2015

bogdan commented Feb 23, 2015

bogdan commented Jan 26, 2016

sgrif commented Jan 26, 2016

bogdan commented Jan 27, 2016

		@@ -1,3 +1,17 @@
		* `AR::Relation#reverse_order` throwns `AR::UnreversableOrderError` when the order can not be reversed
		using current trival algorithm. Also raises the same errow when `#reverse_order` is called on

		@@ -1,3 +1,17 @@
		* `AR::Relation#reverse_order` throws `AR::IrreversableOrderError` when the order can not be reversed

Raise AR::IrreversibleOrderError when #reverse_order can not do it's job #18928

Raise AR::IrreversibleOrderError when #reverse_order can not do it's job #18928

Conversation

bogdan commented Feb 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgrif commented Feb 13, 2015

bogdan commented Feb 13, 2015

Choose a reason for hiding this comment

bogdan commented Feb 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewd commented Feb 18, 2015

sgrif commented Feb 20, 2015

bogdan commented Feb 23, 2015

bogdan commented Jan 26, 2016

sgrif commented Jan 26, 2016

bogdan commented Jan 27, 2016