Merge pull request #18928 from bogdan/unreversable-order

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

2 files changed, 21 insertions, 2 deletions

diff --git a/activerecord/lib/active_record/errors.rb b/activerecord/lib/active_record/errors.rb
index e5906b6756..87f32c042c 100644
--- a/activerecord/lib/active_record/errors.rb
+++ b/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
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 716b1e8505..8ef9f9f627 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/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?)