Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it possible to override the implicit order column #34480

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,20 @@
* Make the implicit order column configurable.

When calling ordered finder methods such as +first+ or +last+ without an
explicit order clause, ActiveRecord sorts records by primary key. This can
result in unpredictable and surprising behaviour when the primary key is
not an auto-incrementing integer, for example when it's a UUID. This change
makes it possible to override the column used for implicit ordering such
that +first+ and +last+ will return more predictable results.

Example:

class Project < ActiveRecord::Base
self.implicit_order_column = "created_at"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would be clearer to use UUID as the example column? Like we talked about at RubyConf created_at might not be a good column for someone to use because there could be duplicates.

What would happen in the case there are duplicates? Can we make sure that Rails orders first by the set column and second by the ID if that column isn't a unique value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The particular use case for this feature is when the primary key itself is a UIID, and thus first/last return records in a seemingly arbitrary fashion because they aren’t sequential so I’m not sure if that would make things clearer. In such a case you would be able to specify the created_at as the implicit order column and thus get results that are closer to what happens with numeric incrementing primary keys. As you point out though, this would mean potentially inconsistent records being returned if the column isn’t unique and there are two with the same value. Adding a secondary sort by the primary key is one option to ensure the same record is always returned. The other alternative is to accept that if the user specifies a non-unique column they do so at their own risk. I prefer the later to keep things simple, but I’m happy to work up a patch that also includes the primary key as a secondary sort. What do you think?

end

*Tekin Suleyman*

* Bump minimum PostgreSQL version to 9.3.

*Yasuo Honda*
Expand Down
16 changes: 16 additions & 0 deletions activerecord/lib/active_record/model_schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,21 @@ module ModelSchema
# If true, the default table name for a Product class will be "products". If false, it would just be "product".
# See table_name for the full rules on table/class naming. This is true, by default.

##
# :singleton-method: implicit_order_column
# :call-seq: implicit_order_column
#
# The name of the column 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. Note that using
# a non-unique column can result in non-deterministic results.
included do
mattr_accessor :primary_key_prefix_type, instance_writer: false

Expand All @@ -110,6 +125,7 @@ module ModelSchema
class_attribute :schema_migrations_table_name, instance_accessor: false, default: "schema_migrations"
class_attribute :internal_metadata_table_name, instance_accessor: false, default: "ar_internal_metadata"
class_attribute :pluralize_table_names, instance_writer: false, default: true
class_attribute :implicit_order_column, instance_accessor: false

self.protected_environments = ["production"]
self.inheritance_column = "type"
Expand Down
4 changes: 2 additions & 2 deletions activerecord/lib/active_record/relation/finder_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -550,8 +550,8 @@ def find_last(limit)
end

def ordered_relation
if order_values.empty? && primary_key
order(arel_attribute(primary_key).asc)
if order_values.empty? && (implicit_order_column || primary_key)
order(arel_attribute(implicit_order_column || primary_key).asc)
else
self
end
Expand Down
10 changes: 10 additions & 0 deletions activerecord/test/cases/finder_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -741,6 +741,16 @@ def test_first_have_determined_order_by_default
assert_equal expected, clients.limit(5).first(2)
end

def test_implicit_order_column_is_configurable
old_implicit_order_column = Topic.implicit_order_column
Topic.implicit_order_column = "title"

assert_equal topics(:fifth), Topic.first
assert_equal topics(:third), Topic.last
ensure
Topic.implicit_order_column = old_implicit_order_column
end

def test_take_and_first_and_last_with_integer_should_return_an_array
assert_kind_of Array, Topic.take(5)
assert_kind_of Array, Topic.first(5)
Expand Down