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 SQL expressions in @orderBy #2056

Open
guillaumeboussion opened this issue Feb 8, 2022 · 7 comments
Open

Allow SQL expressions in @orderBy #2056

guillaumeboussion opened this issue Feb 8, 2022 · 7 comments
Labels
enhancement A feature or improvement

Comments

@guillaumeboussion
Copy link

What problem does this feature proposal attempt to solve?

This feature would allow requests to be sorted by null column values.

In example : to order a list of DateTime in SQL, providing
ISNULL(created_at) ASC, created_at ASC
allows date to be ordered right after non-null date values.

For the moment, it's only possible to do it by setting a @builder directive, and chain a raw SQL ordering. So it's ok when ordering on a simple table, but this is not as easy with a relation ordering

Which possible solutions should be considered?

In the OrderByDirection enum, maybe add 2 attributes ? NULL_ASC and NULL_DESC ? I'll stick to what is the simplest implementation to you guys

(and once again, thank's for the package)

@spawnia
Copy link
Collaborator

spawnia commented Feb 8, 2022

It could be necessary do have arbitrary ordering in the SQL clause, for example ordering by month and ignoring the year:

SUBSTR(created_at, 5, 2) ASC

Adding special support for one specific use case seems to be too narrow of a solution.

@spawnia spawnia added the discussion Requires input from multiple people label Feb 8, 2022
@guillaumeboussion
Copy link
Author

I get the point, but as you just showed, it could be really useful to handle a custom operator ('SUBSTR', 'ISNULL'...) in the builder, I mean, at least for relations (otherwise, the @builder is enough for this need)
If you think of a solution to identify the relation's column to sort by, I take it too :)

@spawnia
Copy link
Collaborator

spawnia commented Feb 9, 2022

These are not operators though, they are functions. As shown in the example, they can take multiple arguments and are thus not simply addded through some boolean flags. Actually, they can be composed in arbitrarily complex ways - too much power to expose directly to the end user (risk of SQL injection, resource exhaustion).

I could see allowing the definition of custom order expressions in @orderBy and exposing them to the client in some way.

type Query {
    users(
        orderBy: _ @orderBy(expressions: [{ name: "month", builder: "UserBuilder@createdAtMonth" }])
    ): [User!]! @all
}

users(orderBy: { column: MONTH, direction: ASC }) { id }

@guillaumeboussion
Copy link
Author

guillaumeboussion commented Feb 9, 2022

Well, looks great, but for this use I could just do a @builder directive I guess ?

Here is my concrete business example (so that you can understand) :
We're selling employee advantages such as discounts in many markets. So, on a brand page, we're displaying all related offers (our partners pay us to have the top position in the query result). Then, we query the Brand type, and join the Offer type as a @belongsTo relation. But, we need to order the main query on the $brand->offer->order attribute.

Of course, I need to first order the non-null value in the order column (so a ISNULL('order') ASC, 'order' ASC) to make them appear this way :
1
2
3
4
null
null

And I just can't find a way to @orderBy the offer relation, when querying the Brand type.

Do someone know a simple way to achieve this ?
Thanks again :)

@spawnia
Copy link
Collaborator

spawnia commented Feb 9, 2022

You can always order on the client 😉

My feature proposal could be extended to work on relations too. Adapted to your example:

type Query {
    brands(
        orderBy: _ @orderBy(
          relations: [
            { relation: "offer", expressions: [{ name: "ORDER_NULL_LAST", builder: "Offer@orderNullLast" }])
          ]
        )
    ): [User!]! @all
}

brands(orderBy: { offer: { column: ORDER_NULL_LAST, direction: ASC }}) { ... }

@guillaumeboussion
Copy link
Author

guillaumeboussion commented Feb 9, 2022

Looks great to me! Thanks for your work.
I guess in the Offer::orderNullLast(), all I'll have to do is

public function orderNullLast(Builder $builder) : Builder
{
    return $builder->orderByRaw('ISNULL(`order`) ASC, `order` ASC');
}

?

@spawnia spawnia changed the title [Ordering] Add ISNULL(column) ASC/DESC to OrderByDirection enum Allow SQL expressions in @orderBy Feb 9, 2022
@spawnia spawnia added enhancement A feature or improvement and removed discussion Requires input from multiple people labels Feb 9, 2022
@spawnia
Copy link
Collaborator

spawnia commented Feb 9, 2022

Looks like we have a good idea of how this could work. Some of the finer details will have to be fleshed out in the implementation:

  • exact name of the arguments for @orderBy
  • how to determine the name for the expression on the client, should probably be an enum
  • should expressions be merged with columns on the client or be separate?
  • does it make sense to allow clients to combine expressions with column and direction?

I am not looking to implement this myself at the moment, but am open to reviewing a PR or doing it as paid work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or improvement
Projects
None yet
Development

No branches or pull requests

2 participants