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

Additionally order by primary key if implicit_order_column is not uniq #37626

Merged
merged 1 commit into from Nov 10, 2019

Conversation

@pawurb
Copy link
Contributor

pawurb commented Nov 2, 2019

As mentioned here https://github.com/rails/rails/pull/34480/files#r234781773 the current implementation of implicit_order_column does not guarantee that the objects will always be returned in the same order if implicit_order_column contains duplicate values.

This patch adds a secondary sort by primary key (if it's not set as a implicit_order_column already) to guarantee the consistent output.

@rails-bot rails-bot bot added the activerecord label Nov 2, 2019
@pawurb

This comment has been minimized.

Copy link
Contributor Author

pawurb commented Nov 2, 2019

The reasoning behind this patch is that with UUID used as a primary key setting created_at as implicit_order_column can become a common practice. Batch created objects will all have the same created_at value, causing the inconsistent behaviour forlast and first methods.

@@ -560,7 +560,11 @@ def find_last(limit)

def ordered_relation
if order_values.empty? && (implicit_order_column || primary_key)
order(arel_attribute(implicit_order_column || primary_key).asc)
if implicit_order_column && implicit_order_column != primary_key

This comment has been minimized.

Copy link
@eugeneius

eugeneius Nov 2, 2019

Member
Suggested change
if implicit_order_column && implicit_order_column != primary_key
if implicit_order_column && primary_key && implicit_order_column != primary_key

If a model has no primary key and has implicit_order_column configured, we shouldn't add the secondary sort.

Both cases need a test.

This comment has been minimized.

Copy link
@pawurb

pawurb Nov 2, 2019

Author Contributor

@eugeneius I've added tests covering the cases I could think of.

@pawurb pawurb force-pushed the pawurb:implicit_ordering_primary_key branch 4 times, most recently from 6ba961b to 517deb2 Nov 2, 2019
@pawurb pawurb requested a review from eugeneius Nov 3, 2019
Copy link
Member

eugeneius left a comment

Could you add a changelog entry? Otherwise this is looking good 👍


c = Topic.connection
table_name = c.quote_table_name("topics")
assert_sql(/ORDER BY #{Regexp.escape(table_name)}.#{Regexp.escape(c.quote_column_name("title"))} DESC, #{Regexp.escape(table_name)}.#{Regexp.escape(c.quote_column_name("id"))} DESC LIMIT/i) {

This comment has been minimized.

Copy link
@eugeneius

eugeneius Nov 3, 2019

Member

These lines could be shorter and more readable if the table names and column names were quoted together:

Suggested change
assert_sql(/ORDER BY #{Regexp.escape(table_name)}.#{Regexp.escape(c.quote_column_name("title"))} DESC, #{Regexp.escape(table_name)}.#{Regexp.escape(c.quote_column_name("id"))} DESC LIMIT/i) {
assert_sql(/ORDER BY #{Regexp.escape(c.quote_table_name("topics.title"))} DESC, #{Regexp.escape(c.quote_table_name("topics.id"))} DESC LIMIT/i) {

It's admittedly a little strange to quote a column name with the quote_table_name method, but it works, and we already rely on it elsewhere:

assert_sql(/ORDER BY #{Regexp.escape(c.quote_table_name("posts.id"))}/i) do

assert_sql(/GROUP BY #{Regexp.escape(Account.connection.quote_table_name("accounts.firm_id"))}/i) do

@@ -115,8 +115,8 @@ module ModelSchema
#
# 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.
# auto-incrementing integer, for example when it's a UUID. Query is subsorted

This comment has been minimized.

Copy link
@eugeneius

eugeneius Nov 3, 2019

Member
Suggested change
# auto-incrementing integer, for example when it's a UUID. Query is subsorted
# auto-incrementing integer, for example when it's a UUID. Records are subsorted
# auto-incrementing integer, for example when it's a UUID. Note that using
# a non-unique column can result in non-deterministic results.
# auto-incrementing integer, for example when it's a UUID. Query is subsorted
# by a primary key column if it is available to ensure deterministic results.

This comment has been minimized.

Copy link
@eugeneius

eugeneius Nov 3, 2019

Member
Suggested change
# by a primary key column if it is available to ensure deterministic results.
# by the primary key if it exists to ensure deterministic results.
@pawurb pawurb force-pushed the pawurb:implicit_ordering_primary_key branch from 517deb2 to 98f2e50 Nov 3, 2019
@pawurb pawurb force-pushed the pawurb:implicit_ordering_primary_key branch from 98f2e50 to 31d31fc Nov 3, 2019
@pawurb pawurb requested a review from eugeneius Nov 3, 2019
Copy link
Member

eugeneius left a comment

LGTM, but this will need a committer's review before it can be merged.

@kaspth kaspth merged commit 9a6df0e into rails:master Nov 10, 2019
2 checks passed
2 checks passed
build
Details
buildkite/rails Build #64716 passed (16 minutes, 14 seconds)
Details
@kaspth

This comment has been minimized.

Copy link
Member

kaspth commented Nov 10, 2019

Sweet! Thanks for the fix @pawurb and the review @eugeneius ❤️

@pawurb pawurb deleted the pawurb:implicit_ordering_primary_key branch Nov 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.