[Fix rails#54591] Raise an error in order dependent finder methods when the model has no order columns

Author: joshuay03

activerecord/CHANGELOG.md

@@ -70,6 +70,19 @@
 
     *Eileen M. Uchitelle*
 
+*   Raise `ActiveRecord::MissingRequiredOrderError` when order dependent finder methods (e.g. `#first`, `#last`) are
+    called without `order` values on the relation, and the model does not have any order columns (`implicit_order_column`,
+    `query_constraints`, or `primary_key`) to fall back on.
+
+    This change will be introduced with a new framework default for Rails 8.1, and the current behavior of not raising
+    an error has been deprecated with the aim of removing the configuration option in Rails 8.2.
+
+    ```ruby
+    config.active_record.raise_on_missing_required_finder_order_columns = true
+    ```
+
+    *Joshua Young*
+
 *   `:class_name` is now invalid in polymorphic `belongs_to` associations.
 
     Reason is `:class_name` does not make sense in those associations because

activerecord/lib/active_record.rb

@@ -355,6 +355,9 @@ def self.permanent_connection_checkout=(value)
   singleton_class.attr_accessor :run_after_transaction_callbacks_in_order_defined
   self.run_after_transaction_callbacks_in_order_defined = false
 
+  singleton_class.attr_accessor :raise_on_missing_required_finder_order_columns
+  self.run_after_transaction_callbacks_in_order_defined = false
+
   singleton_class.attr_accessor :application_record_class
   self.application_record_class = nil
 

activerecord/lib/active_record/errors.rb

@@ -552,6 +552,11 @@ class SerializationFailure < TransactionRollbackError
   class Deadlocked < TransactionRollbackError
   end
 
+  # MissingRequiredOrderError is raised when a relation requires ordering but
+  # lacks any +order+ values in scope or any model order columns to use.
+  class MissingRequiredOrderError < ActiveRecordError
+  end
+
   # IrreversibleOrderError is raised when a relation's order is too complex for
   # +reverse_order+ to automatically reverse.
   class IrreversibleOrderError < ActiveRecordError

activerecord/lib/active_record/relation/finder_methods.rb

@@ -638,8 +638,27 @@ def find_last(limit)
       end
 
       def ordered_relation
-        if order_values.empty? && !_order_columns.empty?
-          order(_order_columns.map { |column| table[column].asc })
+        if order_values.empty?
+          if !_order_columns.empty?
+            return order(_order_columns.map { |column| table[column].asc })
+          end
+
+          if ActiveRecord.raise_on_missing_required_finder_order_columns
+            raise MissingRequiredOrderError, <<~MSG.squish
+              Relation has no order values, and #{model} has no order columns to use as a default.
+              Set at least one of `implicit_order_column`, `query_constraints` or `primary_key` on
+              the model when no `order `is specified on the relation.
+            MSG
+          else
+            ActiveRecord.deprecator.warn(<<~MSG)
+              Calling order dependent finder methods (e.g. `#first`, `#second`) without `order` values on the relation,
+              and on a model (#{model}) that does not have any order columns (`implicit_order_column`, `query_constraints`,
+              or `primary_key`) to fall back on is deprecated and will raise `ActiveRecord::MissingRequiredOrderError`
+              in Rails 8.2.
+            MSG
+
+            self
+          end
         else
           self
         end

activerecord/lib/active_record/relation/query_methods.rb

@@ -2018,8 +2018,11 @@ def reverse_sql_order(order_query)
             return _reverse_order_columns.map { |column| table[column].desc }
           end
 
-          raise IrreversibleOrderError,
-            "Relation has no current order and table has no order columns to be used as default order"
+          raise IrreversibleOrderError, <<~MSG.squish
+            Relation has no order values, and #{model} has no order columns to use as a default.
+            Set at least one of `implicit_order_column`, or `primary_key` on the model when no
+            `order `is specified on the relation.
+          MSG
         end
 
         order_query.flat_map do |o|

activerecord/test/cases/connection_adapters/connection_handlers_sharding_db_test.rb

@@ -369,8 +369,8 @@ def test_same_shards_across_clusters
           ShardConnectionTestModelB.lease_connection.execute("CREATE TABLE `shard_connection_test_model_bs` (shard_key VARCHAR (255))")
           ShardConnectionTestModelB.create!(shard_key: "test_model_b_default")
 
-          assert_equal "test_model_default", ShardConnectionTestModel.where(shard_key: "test_model_default").first.shard_key
-          assert_equal "test_model_b_default", ShardConnectionTestModelB.where(shard_key: "test_model_b_default").first.shard_key
+          assert_equal "test_model_default", ShardConnectionTestModel.where(shard_key: "test_model_default").take.shard_key
+          assert_equal "test_model_b_default", ShardConnectionTestModelB.where(shard_key: "test_model_b_default").take.shard_key
         end
       end
 

activerecord/test/cases/finder_test.rb

@@ -26,6 +26,7 @@
 require "models/non_primary_key"
 require "models/clothing_item"
 require "models/cpk"
+require "models/edge"
 require "support/stubs/strong_parameters"
 require "support/async_helper"
 
@@ -1055,10 +1056,120 @@ def test_last_on_loaded_relation_should_not_use_sql
     end
   end
 
-  def test_last_with_irreversible_order
-    assert_raises(ActiveRecord::IrreversibleOrderError) do
+  def test_first_without_order_columns
+    assert_nil Edge.primary_key
+    assert_nil Edge.implicit_order_column
+    assert_nil Edge.query_constraints_list
+    error = assert_raises(ActiveRecord::MissingRequiredOrderError) do
+      Edge.all.first
+    end
+    assert_match(/Relation has no order values/, error.message)
+  end
+
+  # TODO: Remove this test when we remove the deprecated `raise_on_missing_required_finder_order_columns`
+  def test_first_without_order_columns_and_raise_on_missing_required_finder_order_columns_disabled
+    raise_on_missing_required_finder_order_columns_before = ActiveRecord.raise_on_missing_required_finder_order_columns
+    ActiveRecord.raise_on_missing_required_finder_order_columns = false
+
+    assert_nil Edge.primary_key
+    assert_nil Edge.implicit_order_column
+    assert_nil Edge.query_constraints_list
+    assert_nothing_raised do
+      assert_deprecated(/Calling order dependent finder methods/, ActiveRecord.deprecator) do
+        Edge.all.first
+      end
+    end
+  ensure
+    ActiveRecord.raise_on_missing_required_finder_order_columns = raise_on_missing_required_finder_order_columns_before
+  end
+
+  def test_first_with_at_least_primary_key
+    ordered_edge = Class.new(Edge) do
+      self.primary_key = "source_id"
+    end
+    assert_nothing_raised do
+      ordered_edge.all.first
+    end
+  end
+
+  def test_first_with_at_least_implict_order_column
+    ordered_edge = Class.new(Edge) do
+      self.implicit_order_column = "source_id"
+    end
+    assert_nothing_raised do
+      ordered_edge.all.first
+    end
+  end
+
+  def first_with_at_least_query_constraints
+    ordered_edge = Class.new(Edge) do
+      query_constraints "source_id"
+    end
+    assert_nothing_raised do
+      ordered_edge.all.first
+    end
+  end
+
+  def test_last_without_order_columns
+    assert_nil Edge.primary_key
+    assert_nil Edge.implicit_order_column
+    assert_nil Edge.query_constraints_list
+    error = assert_raises(ActiveRecord::MissingRequiredOrderError) do
+      Edge.all.last
+    end
+    assert_match(/Relation has no order values/, error.message)
+  end
+
+  # TODO: Remove this test when we remove `raise_on_missing_required_finder_order_columns`
+  def test_last_without_order_columns_and_raise_on_missing_required_finder_order_columns_disabled
+    raise_on_missing_required_finder_order_columns_before = ActiveRecord.raise_on_missing_required_finder_order_columns
+    ActiveRecord.raise_on_missing_required_finder_order_columns = false
+
+    assert_nil Edge.primary_key
+    assert_nil Edge.implicit_order_column
+    assert_nil Edge.query_constraints_list
+    error = assert_raises(ActiveRecord::IrreversibleOrderError) do
+      assert_deprecated(/Calling order dependent finder methods/, ActiveRecord.deprecator) do
+        Edge.all.last
+      end
+    end
+    assert_match(/Relation has no order values/, error.message)
+  ensure
+    ActiveRecord.raise_on_missing_required_finder_order_columns = raise_on_missing_required_finder_order_columns_before
+  end
+
+  def test_last_with_at_least_primary_key
+    ordered_edge = Class.new(Edge) do
+      self.primary_key = "source_id"
+    end
+    assert_nothing_raised do
+      ordered_edge.all.last
+    end
+  end
+
+  def test_last_with_at_least_implict_order_column
+    ordered_edge = Class.new(Edge) do
+      self.implicit_order_column = "source_id"
+    end
+    assert_nothing_raised do
+      ordered_edge.all.last
+    end
+  end
+
+  def last_with_at_least_query_constraints
+    ordered_edge = Class.new(Edge) do
+      query_constraints "source_id"
+    end
+    assert_nothing_raised do
+      ordered_edge.all.last
+    end
+  end
+
+  def test_last_with_irreversible_order_value
+    error = assert_raises(ActiveRecord::IrreversibleOrderError) do
       Topic.order(Arel.sql("coalesce(author_name, title)")).last
     end
+    assert_match(/Order .* cannot be reversed automatically/, error.message)
   end
 
   def test_last_on_relation_with_limit_and_offset

activerecord/test/cases/fixtures_test.rb

@@ -1467,7 +1467,7 @@ def test_supports_polymorphic_belongs_to
 
   def test_only_generates_a_pk_if_necessary
     assert_nothing_raised do
-      m = Matey.first
+      m = Matey.take
       m.pirate = pirates(:blackbeard)
       m.target = pirates(:redbeard)
     end

activerecord/test/cases/helper.rb

@@ -42,6 +42,10 @@
 ActiveRecord.raise_on_assign_to_attr_readonly = true
 ActiveRecord.belongs_to_required_validates_foreign_key = false
 
+# Ensure that required finder order columns are present in the test suite until
+# this deprecated configuration is removed.
+ActiveRecord.raise_on_missing_required_finder_order_columns = true
+
 ActiveRecord::ConnectionAdapters.register("abstract", "ActiveRecord::ConnectionAdapters::AbstractAdapter", "active_record/connection_adapters/abstract_adapter")
 ActiveRecord::ConnectionAdapters.register("fake", "FakeActiveRecordAdapter", File.expand_path("../support/fake_adapter.rb", __dir__))
 

activerecord/test/cases/relations_test.rb

@@ -368,9 +368,10 @@ def test_reverse_order_with_nulls_first_or_last
   end if current_adapter?(:PostgreSQLAdapter)
 
   def test_default_reverse_order_on_table_without_primary_key
-    assert_raises(ActiveRecord::IrreversibleOrderError) do
+    error = assert_raises(ActiveRecord::IrreversibleOrderError) do
       Edge.all.reverse_order
     end
+    assert_match(/Relation has no order values/, error.message)
   end
 
   def test_default_reverse_order_on_table_without_primary_key_with_implicit_order_column