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

Add prepend_order as a new query method to prepend the specified order onto any existing order #40401

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

agrobbin
Copy link
Contributor

@agrobbin agrobbin commented Oct 16, 2020

Summary

I recently ran into a situation where it would've been great to prepend a new ORDER BY clause to a query. In our case, it was when using a scope that itself had something to the effect of:

class Profile < ApplicationRecord
  belongs_to :user

  scope :recently_updated, -> { where(arel_table[:updated_at].gteq(7.days.ago)).order(updated_at: :desc) }
end

class User < ApplicationRecord
  has_one :profile

  scope :recently_updated, -> { joins(:profile).merge(Profile.recently_updated) }
end

We wanted to use User.recently_updated, but needed to put a specific group of users at the top of the list. The only way to do that without something like prepend_order would be to reimplement the recently_updated scopes defined on the model.

With prepend_order, we can simply call User.recently_updated.prepend_order('...'), and we're done!

Another way we could use this would be in conjunction with ActiveRecord::FinderMethods#ordered_relation:

class CustomerLegalEntity < ApplicationRecord
  scope :ordered, -> { ordered_relation.prepend_order(default: :desc) }
end

@p8
Copy link
Member

p8 commented Oct 16, 2020

Hi @agrobbin. Thanks for the PR.
There already is a method (with almost the same name): reorder https://api.rubyonrails.org/classes/ActiveRecord/QueryMethods.html#method-i-reorder

   User.order('email DESC').reorder('id ASC') # generated SQL has 'ORDER BY id ASC'

@agrobbin
Copy link
Contributor Author

@p8 I looked at reorder, and that's not quite the same as this:

User.order('email DESC').reorder('id ASC') # generated SQL has `ORDER BY id ASC`
User.order('email DESC').preorder('id ASC') # generated SQL has `ORDER BY id ASC, email DESC`

@p8
Copy link
Member

p8 commented Oct 16, 2020

@agrobbin Ah that makes sense.

@simi
Copy link
Contributor

simi commented Oct 16, 2020

🤔 my initial idea regarding this was to provide block to reorder with current order as a argument to be able to enhance it. preorder can just use this as well.

@agrobbin
Copy link
Contributor Author

@simi I'm definitely open to other approaches here, this just felt like the simplest one with the least likelihood of impacting existing applications! I bet we could achieve the same result by changing reorder to optionally accept a block, but there was something nice about keeping the interface here identical to the other AR query methods, and not changing the existing implementation of any of the other query methods.

Base automatically changed from master to main January 14, 2021 17:02
@rails-bot
Copy link

rails-bot bot commented Apr 14, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Apr 14, 2021
@agrobbin
Copy link
Contributor Author

I'd love to get some input on this contribution if anyone from the core team has some time!

@rails-bot rails-bot bot removed the stale label Apr 14, 2021
@rails-bot
Copy link

rails-bot bot commented Jul 13, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Jul 13, 2021
@ghiculescu
Copy link
Member

@p8 I looked at reorder, and that's not quite the same as this:

User.order('email DESC').reorder('id ASC') # generated SQL has `ORDER BY id ASC`
User.order('email DESC').preorder('id ASC') # generated SQL has `ORDER BY id ASC, email DESC`

I think in this case you'd want to do

User.order('email DESC').reorder('ORDER BY id ASC, email DESC')

I don't mind this new addition. My main concern is the name. To me preorder looks like a typo of reorder.

@rails-bot rails-bot bot removed the stale label Jul 13, 2021
@agrobbin
Copy link
Contributor Author

@ghiculescu I'm totally open to a different name! I do like the simplicity of order, reorder, and preorder, but I see the concern about potential typos. A more ... expressive term could be prefix_order, or something else!

@rails-bot
Copy link

rails-bot bot commented Oct 17, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Oct 17, 2021
@agrobbin
Copy link
Contributor Author

I'd still love to get some 👀 on this for potential inclusion!

@rails-bot rails-bot bot removed the stale label Oct 18, 2021
@rails-bot
Copy link

rails-bot bot commented Jan 16, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Jan 16, 2022
@agrobbin
Copy link
Contributor Author

I'd still love to get this into Rails core!

@rails-bot rails-bot bot removed the stale label Jan 16, 2022
@rails-bot
Copy link

rails-bot bot commented Apr 16, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Apr 16, 2022
@agrobbin
Copy link
Contributor Author

I'd still love to get this into Rails core!

@rails-bot rails-bot bot removed the stale label Apr 16, 2022
@p8
Copy link
Member

p8 commented Apr 17, 2022

@agrobbin Maybe prepend_order would be a clearer name? Similar to reverse_order…

@agrobbin
Copy link
Contributor Author

@p8 updated! I've also rebased off of the latest main, to resolve the outstanding merge conflicts.

@agrobbin agrobbin changed the title Add preorder as a new query method to prepend the specified order onto any existing order Add prepend_order as a new query method to prepend the specified order onto any existing order Apr 17, 2022
@p8
Copy link
Member

p8 commented Apr 20, 2022

Thanks @agrobbin !
Looking at similar PR's it seems like similar PR's were rejected 😞 but that was a long time ago...
#16051 Provide prepend_order and append_order relation methods
#18245 Adding preorder to change order presedence

@agrobbin
Copy link
Contributor Author

I hadn't seen those @p8! I'm hopeful there is openness to this feature now, particularly since based on what was said back in #16051, the opposition wasn't to the feature itself but to the changing of the default over time. Since this is purely additive, I've got my 🤞🏻 that this is acceptable! If order.prepend is more palatable than prepend_order, I'm happy to change it to that. I'm not sure if it is perfectly symmetrical with WHERE NOT, but I definitely get the interest in that kind of DSL. Let me know what you'd suggest!

…der onto any existing order

    ```ruby
    class User < ApplicationRecord
      scope :recent, -> { order(name: :desc) }
    end

    most_popular_then_recent = User.recent.prepend_order(popularity: :desc)
    ```

This is useful if you are using a scope that has an order that you want use as a fallback.
@agrobbin
Copy link
Contributor Author

@p8 I know it's been awhile (actually, almost exactly a year!), but I'm wondering if you have a few moments to take another look at this.

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.

None yet

4 participants