Merge pull request #36889 from kamipo/deprecate_reorder_with_non_deterministic_first

Deprecate `.reorder(nil)` with `.first` / `.first!` taking non-deterministic result

activerecord/CHANGELOG.md

@@ -1,3 +1,9 @@
+*   Deprecate `.reorder(nil)` with `.first` / `.first!` taking non-deterministic result.
+
+    To continue taking non-deterministic result, use `.take` / `.take!` instead.
+
+    *Ryuta Kamizono*
+
 *   Ensure custom PK types are casted in through reflection queries.
 
     *Gannon McGibbon*
@@ -78,4 +84,5 @@
 
     *Michael Duchemin*
 
+
 Please check [6-0-stable](https://github.com/rails/rails/blob/6-0-stable/activerecord/CHANGELOG.md) for previous changes.

activerecord/lib/active_record/relation/finder_methods.rb

@@ -114,6 +114,15 @@ def take!
     #   Person.first(3) # returns the first three objects fetched by SELECT * FROM people ORDER BY people.id LIMIT 3
     #
     def first(limit = nil)
+      if !order_values.empty? && order_values.all?(&:blank?)
+        blank_value = order_values.first
+        ActiveSupport::Deprecation.warn(<<~MSG.squish)
+          `.reorder(#{blank_value.inspect})` with `.first` / `.first!` no longer
+          takes non-deterministic result in Rails 6.2.
+          To continue taking non-deterministic result, use `.take` / `.take!` instead.
+        MSG
+      end
+
       if limit
         find_nth_with_limit(0, limit)
       else

activerecord/lib/active_record/relation/query_methods.rb

@@ -372,7 +372,7 @@ def reorder(*args)
 
     # Same as #reorder but operates on relation in-place instead of copying.
     def reorder!(*args) # :nodoc:
-      preprocess_order_args(args)
+      preprocess_order_args(args) unless args.all?(&:blank?)
 
       self.reordering_value = true
       self.order_values = args

activerecord/test/cases/relations_test.rb

@@ -1737,6 +1737,27 @@ def test_reverse_order_with_reorder_nil_removes_the_order
     assert_nil relation.order_values.first
   end
 
+  def test_reorder_with_first
+    sql_log = capture_sql do
+      message = <<~MSG.squish
+        `.reorder(nil)` with `.first` / `.first!` no longer
+        takes non-deterministic result in Rails 6.2.
+        To continue taking non-deterministic result, use `.take` / `.take!` instead.
+      MSG
+      assert_deprecated(message) do
+        assert Post.order(:title).reorder(nil).first
+      end
+    end
+    assert sql_log.all? { |sql| !/order by/i.match?(sql) }, "ORDER BY was used in the query: #{sql_log}"
+  end
+
+  def test_reorder_with_take
+    sql_log = capture_sql do
+      assert Post.order(:title).reorder(nil).take
+    end
+    assert sql_log.all? { |sql| !/order by/i.match?(sql) }, "ORDER BY was used in the query: #{sql_log}"
+  end
+
   def test_presence
     topics = Topic.all