Skip to content

[Fix #54591] Raise an error in order dependent finder methods when the model has no order columns #54608

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

Merged
merged 1 commit into from
Jul 15, 2025

Conversation

joshuay03
Copy link
Contributor

@joshuay03 joshuay03 commented Feb 24, 2025

Motivation / Background

Fixes #54591
Follow-up to #54607

Detail

  1. Adds a new error ActiveRecord::MissingRequiredOrderError which gets raised in ActiveRecord::FinderMethods#ordered_relation (used by #last and and all find_nth* methods) when there are no order_values and no order columns to be used for the default order.
  2. Updates a similar case where ActiveRecord::IrreversibleOrderError is raised in ActiveRecord::QueryMethods#reverse_sql_order to have a message consistent with this new error.

cc @byroot

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@joshuay03 joshuay03 force-pushed the fix-54591 branch 4 times, most recently from 693debd to 75126e8 Compare February 24, 2025 14:32
@joshuay03 joshuay03 changed the title [Fix #54591] Raise an error on #first and #last when the model has no order columns [Fix #54591] Raise an error on order dependent finder methods when the model has no order columns Feb 24, 2025
@joshuay03 joshuay03 force-pushed the fix-54591 branch 2 times, most recently from a45d7c3 to 13d308c Compare February 28, 2025 07:53
@joshuay03 joshuay03 force-pushed the fix-54591 branch 3 times, most recently from 7e979b0 to 7219e68 Compare February 28, 2025 21:47
@joshuay03 joshuay03 force-pushed the fix-54591 branch 2 times, most recently from 064e48e to 4979968 Compare March 19, 2025 08:30
@joshuay03 joshuay03 force-pushed the fix-54591 branch 3 times, most recently from c12418c to 45b2d42 Compare May 17, 2025 03:44
@joshuay03 joshuay03 changed the title [Fix #54591] Raise an error on order dependent finder methods when the model has no order columns [Fix #54591] Raise an error in order dependent finder methods when the model has no order columns May 17, 2025
@joshuay03 joshuay03 requested a review from skipkayhil May 17, 2025 03:45
Copy link
Member

@byroot byroot left a comment

Choose a reason for hiding this comment

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

I like this a lot. I'll wait a couple days to see if someone opposes though.

@byroot
Copy link
Member

byroot commented May 17, 2025

I wonder if this should be considered breaking enough to warrant a framework default and a deprecation.

Which is debatable because this is solving a bug, but prior you'd get a somewhat unpredictable behavior but "working" behavior, now you'd get a straight error.

@joshuay03 joshuay03 force-pushed the fix-54591 branch 2 times, most recently from 076c286 to af7637b Compare May 19, 2025 01:16
@joshuay03
Copy link
Contributor Author

I wonder if this should be considered breaking enough to warrant a framework default and a deprecation.

Which is debatable because this is solving a bug, but prior you'd get a somewhat unpredictable behavior but "working" behavior, now you'd get a straight error.

Hmm, yeah, that's a good point, I can see the argument for a deprecation notice. I will action soon 👍🏽

@joshuay03 joshuay03 force-pushed the fix-54591 branch 3 times, most recently from 3ddc41a to f9c8b4f Compare May 25, 2025 01:43
@joshuay03 joshuay03 force-pushed the fix-54591 branch 4 times, most recently from 7ef2e6a to ed25ba1 Compare May 25, 2025 02:01
@joshuay03 joshuay03 force-pushed the fix-54591 branch 2 times, most recently from 790d22b to 332a33e Compare May 25, 2025 02:12
@joshuay03
Copy link
Contributor Author

I wonder if this should be considered breaking enough to warrant a framework default and a deprecation.
Which is debatable because this is solving a bug, but prior you'd get a somewhat unpredictable behavior but "working" behavior, now you'd get a straight error.

Hmm, yeah, that's a good point, I can see the argument for a deprecation notice. I will action soon 👍🏽

Action-ed! Hopefully I've done all the things right...

@byroot
Copy link
Member

byroot commented Jul 15, 2025

Uh, just realized this fell through the cracks, I'll try to rebase it.

@byroot byroot merged commit 79711de into rails:main Jul 15, 2025
3 checks passed
@joshuay03
Copy link
Contributor Author

Thank you!

@joshuay03 joshuay03 deleted the fix-54591 branch July 15, 2025 09:41
@joshuay03 joshuay03 moved this from In Progress / Pending Review to Done in Open Source Jul 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent finder method behaviour without primary key and implicit_order_column
3 participants