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

Conversation

@tekin
Copy link
Contributor

@tekin tekin commented Nov 18, 2018

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 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"
  end

This takes a different approach to #34236, which automatically switches to using created_at if the primary key is a UUID and a created_at column exists.

@rails-bot
Copy link

@rails-bot rails-bot commented Nov 18, 2018

r? @rafaelfranca

(@rails-bot has picked a reviewer for you, use r? to override)

@tekin
Copy link
Contributor Author

@tekin tekin commented Nov 18, 2018

Example:

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

This comment has been minimized.

@eileencodes

eileencodes Nov 19, 2018
Member

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?

This comment has been minimized.

@tekin

tekin Nov 19, 2018
Author Contributor

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?

@eileencodes eileencodes added this to the 6.0.0 milestone Nov 19, 2018
# order clause is used. Defaults to the primary key.
def implicit_order_column
@implicit_order_column ||= primary_key
end

This comment has been minimized.

@tenderlove

tenderlove Nov 19, 2018
Member

Instead of doing this as a getter / setter, could we just let subclasses override?

For example, the implementation in active record is:

class ActiveRecord::Base
  def self.implicit_order_column
    primary_key
  end
end

If you want to override it in your subclass, just do:

class Project < ActiveRecord::Base
  def self.implicit_order_column
    "created_at"
  end
end

That way we don't have to deal with ivar / cache management.

This comment has been minimized.

@matthewd

matthewd Nov 20, 2018
Member

I think class_attribute would be better: it should be as simple as the override version, but is much more accessible for e.g. a future deprecation.

This comment has been minimized.

@tekin

tekin Nov 20, 2018
Author Contributor

I did look into class_attribute but it wasn't immediately obvious how I'd set the default value to primary_key.

This comment has been minimized.

@matthewd

matthewd Nov 20, 2018
Member

Hmmm.. I guess you'd set it to nil, then || it in the caller

This comment has been minimized.

@tekin

tekin Nov 26, 2018
Author Contributor

@eileencodes / @tenderlove any thoughts on this? should I update to use class_attribute instead?

This comment has been minimized.

@tenderlove

tenderlove Nov 26, 2018
Member

@tekin yes, make this a class attribute and then we'll merge it. 😊

This comment has been minimized.

@tekin

tekin Nov 27, 2018
Author Contributor

Updated to use class_attribute.

@tekin tekin force-pushed the tekin:configurable-implicit-ordering-column branch 2 times, most recently from 0e3792c to d1cc1e1 Nov 20, 2018
@tekin tekin changed the title Make implicit order column configurable Make it possible to override the implicit order column Nov 20, 2018
@tekin
Copy link
Contributor Author

@tekin tekin commented Nov 20, 2018

PR updated to use an overridable class method instead of getter/setter.

@tekin tekin force-pushed the tekin:configurable-implicit-ordering-column branch 2 times, most recently from 2bd52db to 9a8f5c8 Nov 20, 2018
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. For Example:

  class Project < ActiveRecord::Base
    self.implicit_order_column = "created_at"
  end
@tekin tekin force-pushed the tekin:configurable-implicit-ordering-column branch from 9a8f5c8 to 3b9982a Nov 27, 2018
Copy link
Member

@eileencodes eileencodes left a comment

Looks good, thanks 👍

@eileencodes eileencodes merged commit b9c7305 into rails:master Nov 27, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tekin
Copy link
Contributor Author

@tekin tekin commented Nov 27, 2018

🎉

suketa added a commit to suketa/rails_sandbox that referenced this pull request Aug 3, 2019
Make it possible to override the implicit order column
rails/rails#34480
suketa added a commit to suketa/rails_sandbox that referenced this pull request Aug 3, 2019
Make it possible to override the implicit order column
rails/rails#34480
tekin added a commit to DFE-Digital/claim-additional-payments-for-teaching that referenced this pull request Dec 3, 2019
One of the downsides to using UUIDs for primary keys is that we lose the
implicit ordering behaviour of the `first` and `last` finder methods,
which by default return the first and last records based on the primary
key as a proxy for creation date. When that is an incrementing integer,
things work as expected, but with UUIDs we end up with entirely
random records based on the string ordering of the UUIDs. Rails 6
introduced the ability to override this implicit order column[1], which
means we can set it to created_at and get back the natural and less
surprising behaviour of `first` and `last`.

[1]: rails/rails#34480
@kamipo kamipo mentioned this pull request Dec 9, 2019
0 of 2 tasks complete
mcmire added a commit to yaorlov/shoulda-matchers that referenced this pull request Jul 12, 2020
Add a matcher that can test the new [implicit_order_column][1] class
property that is available on ActiveRecord classes in Rails 6.

[1]: rails/rails#34480

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
mcmire added a commit to yaorlov/shoulda-matchers that referenced this pull request Jul 12, 2020
Add a matcher that can test the new [implicit_order_column][1] class
property that is available on ActiveRecord classes in Rails 6.

[1]: rails/rails#34480

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
Vin0uz pushed a commit to Vin0uz/Futsal that referenced this pull request Aug 2, 2020
Having uuids on all models messes things up a bit : default sorting by id is broken, and it's better to
ensure that we sort using `created_at` fields by default.

TDIL : [this commit on Rails 6 - add implicit order column](rails/rails#34480)
Vin0uz pushed a commit to Vin0uz/Futsal that referenced this pull request Aug 2, 2020
Having uuids on all models messes things up a bit : default sorting by id is broken, and it's better to
ensure that we sort using `created_at` fields by default.

TDIL : [this commit on Rails 6 - add implicit order column](rails/rails#34480)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants