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

Allow order to be given expressions as hash keys #28191

Merged
merged 1 commit into from Mar 17, 2017

Conversation

@eugeneius
Copy link
Member

@eugeneius eugeneius commented Feb 26, 2017

Related to #28117.

When order is given a hash, the keys are currently assumed to be attribute names and are quoted as such in the query, which makes it impossible to pass an expression instead:

Post.order("LENGTH(title)" => :asc).last
# SELECT  `posts`.* FROM `posts` ORDER BY `posts`.`LENGTH(title)` DESC LIMIT 1

If the key is an Arel::Nodes::SqlLiteral, we now use it directly in the query. This provides a way to build a relation with a complex order clause that can still be reversed with reverse_order or last.

@matthewd
Copy link
Member

@matthewd matthewd commented Feb 27, 2017

This seems like a good idea, but we should probably coordinate it with #27947: allow an Arel.sql expression, but not start treating a plain string as such.

When `order` is given a hash, the keys are currently assumed to be
attribute names and are quoted as such in the query, which makes it
impossible to pass an expression instead:

    Post.order("LENGTH(title)" => :asc).last
    # SELECT  `posts`.* FROM `posts` ORDER BY `posts`.`LENGTH(title)` DESC LIMIT 1

If the key is an `Arel::Nodes::SqlLiteral`, we now use it directly in
the query. This provides a way to build a relation with a complex order
clause that can still be reversed with `reverse_order` or `last`.
@eugeneius eugeneius force-pushed the eugeneius:string_assoc_order branch to 6cf4835 Feb 27, 2017
@eugeneius
Copy link
Member Author

@eugeneius eugeneius commented Feb 27, 2017

I've restricted the behaviour to only apply to Arel SQL literals - thanks @matthewd!

@rafaelfranca rafaelfranca requested a review from matthewd Feb 28, 2017
Copy link
Member

@rafaelfranca rafaelfranca left a comment

LGTM.

@rafaelfranca rafaelfranca added this to the 5.1.0 milestone Feb 28, 2017
@rafaelfranca rafaelfranca merged commit bac40b9 into rails:master Mar 17, 2017
2 checks passed
2 checks passed
codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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

4 participants
You can’t perform that action at this time.