Merge pull request rails#54679 from issyl0/activerecord-implicit_order_column-should-not-always-add-pk-to-order-clause

activerecord: Don't always append primary keys etc. to order conditions

Author: matthewd

activerecord/CHANGELOG.md

@@ -1,3 +1,13 @@
+*   Allow bypassing primary key/constraint addition in `implicit_order_column`
+
+    When specifying multiple columns in an array for `implicit_order_column`, adding
+    `nil` as the last element will prevent appending the primary key to order
+    conditions. This allows more precise control of indexes used by
+    generated queries. It should be noted that this feature does introduce the risk
+    of API misbehavior if the specified columns are not fully unique.
+
+    *Issy Long*
+
 *   Allow setting the `schema_format` via database configuration.
 
     ```

activerecord/lib/active_record/model_schema.rb

@@ -113,17 +113,19 @@ module ModelSchema
     # :singleton-method: implicit_order_column
     # :call-seq: implicit_order_column
     #
-    # The name of the column records are ordered by if no explicit order clause
+    # The name of the column(s) records are ordered by if no explicit order clause
     # is used during an ordered finder call. If not set the primary key is used.
 
     ##
     # :singleton-method: implicit_order_column=
     # :call-seq: implicit_order_column=(column_name)
     #
-    # Sets the column to sort records by when no explicit order clause is used
-    # during an ordered finder call. Useful when the primary key is not an
-    # auto-incrementing integer, for example when it's a UUID. Records are subsorted
-    # by the primary key if it exists to ensure deterministic results.
+    # Sets the column(s) to sort records by when no explicit order clause is used
+    # during an ordered finder call. Useful for models where the primary key isn't an
+    # auto-incrementing integer (such as UUID).
+    #
+    # By default, records are subsorted by primary key to ensure deterministic results.
+    # To disable this subsort behavior, set `implicit_order_column` to `["column_name", nil]`.
 
     ##
     # :singleton-method: immutable_strings_by_default=

activerecord/lib/active_record/relation/finder_methods.rb

@@ -646,16 +646,13 @@ def ordered_relation
       end
 
       def _order_columns
-        oc = []
+        columns = Array(model.implicit_order_column)
 
-        oc << model.implicit_order_column if model.implicit_order_column
-        oc << model.query_constraints_list if model.query_constraints_list
+        return columns.compact if columns.length.positive? && columns.last.nil?
 
-        if model.primary_key && model.query_constraints_list.nil?
-          oc << model.primary_key
-        end
+        columns += Array(model.query_constraints_list || model.primary_key)
 
-        oc.flatten.uniq.compact
+        columns.uniq.compact
       end
   end
 end

activerecord/test/cases/finder_test.rb

@@ -1106,7 +1106,7 @@ def test_first_have_determined_order_by_default
     assert_equal expected, clients.order(nil).first(2)
   end
 
-  def test_implicit_order_column_is_configurable
+  def test_implicit_order_column_is_configurable_with_a_single_value
     old_implicit_order_column = Topic.implicit_order_column
     Topic.implicit_order_column = "title"
 
@@ -1120,6 +1120,28 @@ def test_implicit_order_column_is_configurable
     Topic.implicit_order_column = old_implicit_order_column
   end
 
+  def test_implicit_order_column_is_configurable_with_multiple_values
+    old_implicit_order_column = Topic.implicit_order_column
+    Topic.implicit_order_column = ["title", "author_name"]
+
+    assert_queries_match(/ORDER BY #{Regexp.escape(quote_table_name("topics.title"))} DESC, #{Regexp.escape(quote_table_name("topics.author_name"))} DESC, #{Regexp.escape(quote_table_name("topics.id"))} DESC LIMIT/i) {
+      Topic.last
+    }
+  ensure
+    Topic.implicit_order_column = old_implicit_order_column
+  end
+
+  def test_ordering_does_not_append_primary_keys_or_query_constraints_if_passed_an_implicit_order_column_array_ending_in_nil
+    old_implicit_order_column = Topic.implicit_order_column
+    Topic.implicit_order_column = ["author_name", nil]
+
+    assert_queries_match(/ORDER BY #{Regexp.escape(quote_table_name("topics.author_name"))} DESC LIMIT/i) {
+      Topic.last
+    }
+  ensure
+    Topic.implicit_order_column = old_implicit_order_column
+  end
+
   def test_implicit_order_set_to_primary_key
     old_implicit_order_column = Topic.implicit_order_column
     Topic.implicit_order_column = "id"