Merge pull request #18928 from bogdan/unreversable-order

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

activerecord/CHANGELOG.md

@@ -1,3 +1,17 @@
+*   `ActiveRecord::Relation#reverse_order` throws `ActiveRecord::IrreversibleOrderError`
+    when the order can not be reversed using current trivial algorithm.
+    Also raises the same error when `#reverse_order` is called on
+    relation without any order and table has no primary key:
+
+        Topic.order("concat(author_name, title)").reverse_order
+          # Before: SELECT `topics`.* FROM `topics` ORDER BY concat(author_name DESC, title) DESC
+          # After: raises ActiveRecord::IrreversibleOrderError
+        Edge.all.reverse_order
+          # Before: SELECT `edges`.* FROM `edges` ORDER BY `edges`.`` DESC
+          # After: raises ActiveRecord::IrreversibleOrderError
+
+    *Bogdan Gusiev*
+
 *   Improve schema_migrations insertion performance by inserting all versions
     in one INSERT SQL.
 

activerecord/lib/active_record/errors.rb

@@ -275,4 +275,9 @@ module ActiveRecord
   # The mysql2 and postgresql adapters support setting the transaction isolation level.
   class TransactionIsolationError < ActiveRecordError
   end
+
+  # IrreversibleOrderError is raised when a relation's order is too complex for
+  # +reverse_order+ to automatically reverse.
+  class IrreversibleOrderError < ActiveRecordError
+  end
 end

activerecord/lib/active_record/relation/query_methods.rb

@@ -1104,14 +1104,21 @@ module ActiveRecord
     end
 
     def reverse_sql_order(order_query)
-      order_query = ["#{quoted_table_name}.#{quoted_primary_key} ASC"] if order_query.empty?
+      if order_query.empty?
+        return [table[primary_key].desc] if primary_key
+        raise IrreversibleOrderError,
+          "Relation has no current order and table has no primary key to be used as default order"
+      end
 
       order_query.flat_map do |o|
         case o
         when Arel::Nodes::Ordering
           o.reverse
         when String
-          o.to_s.split(',').map! do |s|
+          if does_not_support_reverse?(o)
+            raise IrreversibleOrderError, "Order #{o.inspect} can not be reversed automatically"
+          end
+          o.split(',').map! do |s|
             s.strip!
             s.gsub!(/\sasc\Z/i, ' DESC') || s.gsub!(/\sdesc\Z/i, ' ASC') || s.concat(' DESC')
           end
@@ -1121,6 +1128,13 @@ module ActiveRecord
       end
     end
 
+    def does_not_support_reverse?(order)
+      #uses sql function with multiple arguments
+      order =~ /\([^()]*,[^()]*\)/ ||
+        # uses "nulls first" like construction
+        order =~ /nulls (first|last)\Z/i
+    end
+
     def build_order(arel)
       orders = order_values.uniq
       orders.reject!(&:blank?)

activerecord/test/cases/relations_test.rb

@@ -19,6 +19,7 @@ require 'models/aircraft'
 require "models/possession"
 require "models/reader"
 require "models/categorization"
+require "models/edge"
 
 class RelationTest < ActiveRecord::TestCase
   fixtures :authors, :topics, :entrants, :developers, :companies, :developers_projects, :accounts, :categories, :categorizations, :posts, :comments,
@@ -223,6 +224,39 @@ class RelationTest < ActiveRecord::TestCase
     assert_equal topics(:fifth).title, topics.first.title
   end
 
+  def test_reverse_order_with_function
+    topics = Topic.order("length(title)").reverse_order
+    assert_equal topics(:second).title, topics.first.title
+  end
+
+  def test_reverse_order_with_function_other_predicates
+    topics = Topic.order("author_name, length(title), id").reverse_order
+    assert_equal topics(:second).title, topics.first.title
+    topics = Topic.order("length(author_name), id, length(title)").reverse_order
+    assert_equal topics(:fifth).title, topics.first.title
+  end
+
+  def test_reverse_order_with_multiargument_function
+    assert_raises(ActiveRecord::IrreversibleOrderError) do
+      Topic.order("concat(author_name, title)").reverse_order
+    end
+  end
+
+  def test_reverse_order_with_nulls_first_or_last
+    assert_raises(ActiveRecord::IrreversibleOrderError) do
+      Topic.order("title NULLS FIRST").reverse_order
+    end
+    assert_raises(ActiveRecord::IrreversibleOrderError) do
+      Topic.order("title nulls last").reverse_order
+    end
+  end
+
+  def test_default_reverse_order_on_table_without_primary_key
+    assert_raises(ActiveRecord::IrreversibleOrderError) do
+      Edge.all.reverse_order
+    end
+  end
+
   def test_order_with_hash_and_symbol_generates_the_same_sql
     assert_equal Topic.order(:id).to_sql, Topic.order(:id => :asc).to_sql
   end