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

Deprecate `.reorder(nil)` with `.first` / `.first!` taking non-deterministic result #36889

Merged

Conversation

@kamipo
Copy link
Member

commented Aug 8, 2019

Actually, first taking non-deterministic result is considered a bug to
me. But changing the behavior should not be happened in rc2.

I've restored the behavior for 6.0, and then will deprecate that in 6.1.

To continue taking non-deterministic result, use .take / .take!
instead.

Fixes #36802.

kamipo added some commits Aug 8, 2019

Restore an ability that `reorder` with `first` taking non-determinist…
…ic result

Actually, `first` taking non-deterministic result is considered a bug to
me. But changing the behavior should not be happened in rc2.

I've restored the behavior for 6.0, and then will deprecate that in 6.1.

Fixes #36802.
Deprecate `.reorder(nil)` with `.first` / `.first!` taking non-determ…
…inistic result

To continue taking non-deterministic result, use `.take` / `.take!`
instead.

@rails-bot rails-bot bot added the activerecord label Aug 8, 2019

@kamipo kamipo merged commit bdfd470 into rails:master Aug 9, 2019

2 checks passed

buildkite/rails Build #62938 passed (9 minutes, 9 seconds)
Details
codeclimate All good!
Details

@kamipo kamipo deleted the kamipo:deprecate_reorder_with_non_deterministic_first branch Aug 9, 2019

Edouard-chin added a commit to Shopify/identity_cache that referenced this pull request Aug 12, 2019

Remove cosmetic `reorder(nil)`, it triggers too many deprecations:
- ActiveRecord made a recent change to deprecate usage of `first`
  returning a non deterministic result rails/rails#36889

  The change upstream was orginally done in rails/rails@1340498
  but had to be reverted because it shouldn't be part of a release
  candidate.
  For the reference, I had a quick chat about that change rails/rails@1340498#r33986510

  In IdentityCache we call `reorder(nil).first` and the reason was
  purely cosmetic #148

  > I also added some reorder(nil) calls here and there to prevent useless ORDER clauses to be included in the queries

Edouard-chin added a commit to Shopify/identity_cache that referenced this pull request Aug 12, 2019

Remove cosmetic `reorder(nil)`, it triggers too many deprecations:
- ActiveRecord made a recent change to deprecate usage of `first`
  returning a non deterministic result rails/rails#36889

  The change upstream was orginally done in rails/rails@1340498
  but had to be reverted because it shouldn't be part of a release
  candidate.
  For the reference, I had a quick chat about that change rails/rails@1340498#r33986510

  In IdentityCache we call `reorder(nil).first` and the reason was
  purely cosmetic #148

  > I also added some reorder(nil) calls here and there to prevent useless ORDER clauses to be included in the queries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.