Skip to content

Commit c327b19

Browse files
authored
activerecord: Don't always append primary keys etc. to order conditions
- If `nil` is the last element of an array passed to `implicit_order_column`, do not append the primary key or the query constraints. For example, `self.implicit_order_column = ["author_name", nil]` will generate `ORDER BY author_name DESC` and not `ORDER BY author_name DESC, id DESC` (with or without a LIMIT). - There wasn't a test for `implicit_order_column` supporting arrays for ordering by multiple columns, so I added one. - The reasoning is that in some cases, like database sharding, the primary key is not the best column to order by for performance reasons and thus users should have the choice of whether to append it by default.
1 parent c3f1f39 commit c327b19

File tree

4 files changed

+44
-13
lines changed

4 files changed

+44
-13
lines changed

activerecord/CHANGELOG.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,13 @@
1+
* Allow bypassing primary key/constraint addition in `implicit_order_column`
2+
3+
When specifying multiple columns in an array for `implicit_order_column`, adding
4+
`nil` as the last element will prevent appending the primary key to order
5+
conditions. This allows more precise control of indexes used by
6+
generated queries. It should be noted that this feature does introduce the risk
7+
of API misbehavior if the specified columns are not fully unique.
8+
9+
*Issy Long*
10+
111
* Allow setting the `schema_format` via database configuration.
212

313
```

activerecord/lib/active_record/model_schema.rb

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -113,17 +113,19 @@ module ModelSchema
113113
# :singleton-method: implicit_order_column
114114
# :call-seq: implicit_order_column
115115
#
116-
# The name of the column records are ordered by if no explicit order clause
116+
# The name of the column(s) records are ordered by if no explicit order clause
117117
# is used during an ordered finder call. If not set the primary key is used.
118118

119119
##
120120
# :singleton-method: implicit_order_column=
121121
# :call-seq: implicit_order_column=(column_name)
122122
#
123-
# Sets the column to sort records by when no explicit order clause is used
124-
# during an ordered finder call. Useful when the primary key is not an
125-
# auto-incrementing integer, for example when it's a UUID. Records are subsorted
126-
# by the primary key if it exists to ensure deterministic results.
123+
# Sets the column(s) to sort records by when no explicit order clause is used
124+
# during an ordered finder call. Useful for models where the primary key isn't an
125+
# auto-incrementing integer (such as UUID).
126+
#
127+
# By default, records are subsorted by primary key to ensure deterministic results.
128+
# To disable this subsort behavior, set `implicit_order_column` to `["column_name", nil]`.
127129

128130
##
129131
# :singleton-method: immutable_strings_by_default=

activerecord/lib/active_record/relation/finder_methods.rb

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -646,16 +646,13 @@ def ordered_relation
646646
end
647647

648648
def _order_columns
649-
oc = []
649+
columns = Array(model.implicit_order_column)
650650

651-
oc << model.implicit_order_column if model.implicit_order_column
652-
oc << model.query_constraints_list if model.query_constraints_list
651+
return columns.compact if columns.length.positive? && columns.last.nil?
653652

654-
if model.primary_key && model.query_constraints_list.nil?
655-
oc << model.primary_key
656-
end
653+
columns += Array(model.query_constraints_list || model.primary_key)
657654

658-
oc.flatten.uniq.compact
655+
columns.uniq.compact
659656
end
660657
end
661658
end

activerecord/test/cases/finder_test.rb

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1106,7 +1106,7 @@ def test_first_have_determined_order_by_default
11061106
assert_equal expected, clients.order(nil).first(2)
11071107
end
11081108

1109-
def test_implicit_order_column_is_configurable
1109+
def test_implicit_order_column_is_configurable_with_a_single_value
11101110
old_implicit_order_column = Topic.implicit_order_column
11111111
Topic.implicit_order_column = "title"
11121112

@@ -1120,6 +1120,28 @@ def test_implicit_order_column_is_configurable
11201120
Topic.implicit_order_column = old_implicit_order_column
11211121
end
11221122

1123+
def test_implicit_order_column_is_configurable_with_multiple_values
1124+
old_implicit_order_column = Topic.implicit_order_column
1125+
Topic.implicit_order_column = ["title", "author_name"]
1126+
1127+
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) {
1128+
Topic.last
1129+
}
1130+
ensure
1131+
Topic.implicit_order_column = old_implicit_order_column
1132+
end
1133+
1134+
def test_ordering_does_not_append_primary_keys_or_query_constraints_if_passed_an_implicit_order_column_array_ending_in_nil
1135+
old_implicit_order_column = Topic.implicit_order_column
1136+
Topic.implicit_order_column = ["author_name", nil]
1137+
1138+
assert_queries_match(/ORDER BY #{Regexp.escape(quote_table_name("topics.author_name"))} DESC LIMIT/i) {
1139+
Topic.last
1140+
}
1141+
ensure
1142+
Topic.implicit_order_column = old_implicit_order_column
1143+
end
1144+
11231145
def test_implicit_order_set_to_primary_key
11241146
old_implicit_order_column = Topic.implicit_order_column
11251147
Topic.implicit_order_column = "id"

0 commit comments

Comments
 (0)