Add @orderBy argument directive

[megawubs]

• Added or updated tests
• Added Docs for all relevant versions

Related Issue/Intent

Resolves #580

Changes

• Added the @orderBy argument directive
• Added the OrderByClause input type
• Added the SortOrder enum type

input OrderByClause{
    field: String!
    order: SortOrder!
}

enum SortOrder {
    ASC
    DESC
}

Open for Discussion
I'm not entirely sure about the naming of the input field type and the enum type yet.

Other possibilities i've tried:

• input SortOption & enum SortOrder
• input OrderByOption & enum OrderByOrder
• input OrderByClause & enum Order

Any other ideas?

[robsontenorio]

How about multiple OrderByClause?

[megawubs]

@robsontenorio What I currently have to get multiple orderBy clauses working is the following:

type Query {
   suppliers(orderBy: [OrderByClause!]! @orderBy): [Supplier!]  @paginate
}

I also added an example like this to the docs.

Your commend however makes me think that the @orderBy directive will break when not given an array of SortOption because it requires it. Should we keep it this way, or check if it's given an array or one item?

[spawnia]

@megawubs i think its better for consistency to always have it as an array.

[megawubs]

@spawnia I agree. But do we require the schema writer to typehint the type, or can we do something like @paginate where the arguments are added to the query in a consistent way?

[spawnia]

What usecase would such a transformation serve, how would it make things easier? Can you give an example?

[lucasmichot]

@megawubs this is a nice addition, nevertheless I am wondering a bit about the field type
Don't you feel it should be an enum instead?

As you might want to control on which field (hopefully indexed) the sorting should be applied to...

As an example, here is the implementation used for Github:

{
  repository(owner: "nuwave", name: "lighthouse") {
    issues(last: 100, orderBy: {field: CREATED_AT, direction: DESC}) {
      edges {
        node {
          databaseId
          title
          createdAt
        }
      }
    }
  }
}

Checking at:

query enumValuesOfIssueOrderField {
  __type(name: "IssueOrderField") {
    name
    enumValues {
      name
    }
  }
}

You get:

{
  "data": {
    "__type": {
      "name": "IssueOrderField",
      "enumValues": [
        {
          "name": "CREATED_AT"
        },
        {
          "name": "UPDATED_AT"
        },
        {
          "name": "COMMENTS"
        }
      ]
    }
  }
}

What about adding a @orderable(OrderByEnum) on each field that can be used for sorting, when retrieved by pagination?

[lucasmichot]

A schema is sometimes easier to describe things 😉

# The default order by direction
enum OrderDirection {
    ASC
    DESC
}

# A custom order by direction
enum AnotherOrderDirection {
    FOO
    BAR
}

type Query {
    # If orderableEnum is not set, it would default to "OrderDirection"
    users: [User!]! @paginate(type: "paginator" model: "App\\User" orderableDirection: "AnotherOrderDirection")
    user(id: ID @eq): User @find(model: "App\\User")
}

type User {
    id: ID!
    name: String!
    email: String!
    # The default field name will be "CREATED_AT"
    created_at: DateTime! @orderable()
    # The field name can be overriden
    updated_at: DateTime! @orderable(field: "CUSTOM_UPDATED_AT_FIELD_NAME")
}

# This would generate the following enum:
enum UserOrderField {
    CREATED_AT @enum(value: "created_at")
    CUSTOM_UPDATED_AT_FIELD_NAME @enum(value: "updated_at")
}

[spawnia]

@lucasmichot you bring up some good points.

I see the value in having a foolproof schema that uses ENUM's upfront to prevent usage mistakes. The way you propose to define them is quite nice as well. As you correctly noted, it can be beneficiary/necessary for high traffic API's to limit the possible queries.

Still, i think the implementation proposed here is close to optimal when it comes to self-consumed API's that should be rapidly developed. A quite common use for Laravel, i think.

We can add something like you proposed in a later PR, this one looks fine to me as is.

[olivernybroe]

Just to come with another alternative

This will allow all fields to be ordered

type Query {
    posts: [Post] @orderBy
}

type Post {
    id: ID!
    title: String!
    description: String!
}

And will generate the following

type Query {
    posts(orderBy: PostsOrderByInput) : [Post]
}
input PostsOrderByInput {
   id: OrderBy
   title: OrderBy
   description: OrderBy
}

enum OrderBy {
   ASC
   DESC
}

I can then also just do the following to only order some fields

type Query {
    posts: [Post] @orderBy(fields: ['id', 'title'])
}

And it will generate

type Query {
    posts(orderBy: PostsOrderByInput) : [Post]
}
input PostsOrderByInput {
   id: OrderBy
   title: OrderBy
}

enum OrderBy {
   ASC
   DESC
}

This approach will also solve the problem of being able to order the same field twice, as that can give confusion to which one of them will run first and last.

In theory you could also extend this to also limit if a field should only be able to limit by ASC. A use case for this would be to limit your indexes to not have both ASC and DESC.
Just a quick example:

type Query {
    posts: [Post] @orderBy(fields: {id: 'all',  title: 'asc'})
}

And it will generate

type Query {
    posts(orderBy: PostsOrderByInput) : [Post]
}
input PostsOrderByInput {
   id: OrderBy
   title: AscOrderBy
}

enum AscOrderBy {
  ASC
}

enum OrderBy {
   ASC
   DESC
}

And to extend on this a little more, we can then also force a user to choose a orderBy on a field

type Query {
    posts: [Post] @orderBy(fields: {id: 'all|required',  title: 'asc'})
}

And it will generate

type Query {
    posts(orderBy: PostsOrderByInput!) : [Post]
}
input PostsOrderByInput {
   id: OrderBy!
   title: AscOrderBy
}

enum AscOrderBy {
  ASC
}

enum OrderBy {
   ASC
   DESC
}

[megawubs]

I agree with @spawnia. Ordering by specific fields and directions is a nice thing to have. But for most cases I think it's too much.

[olivernybroe]

The directive itself does not require that the type should be OrderByClause.
If that is the case, then it has to check that the order is a valid order and also the bigger issue is how does it handle when I input an invalid field.

[spawnia]

Added validation for a correct definition.

Don't see how invalid fields could be handled gracefully?

[olivernybroe]

The test scenarios looks great. However you are only testing for best case scenarios, we for example also need tests for handling invalid field supplied.

[spawnia]

@olivernybroe i like how the fields can be deduced from the model or defined manually in your proposal.

However, the spec says that arguments are unordered, so the proposed final schema falls apart there unfortunately.

[olivernybroe]

@spawnia Ah damn it. But nice catch.

Well I guess we need to go with the list approach then as they are ordered.
However we could still easily implement support for this @orderBy(fields: ['id', 'title']) and it then creates a enum with those values and uses that enum. Of course we are now removing the support for required and ASC only fields.

[spawnia]

@olivernybroe yeah, we can add this in a seperate PR and with a different name, like @orderable

[megawubs]

Changes look good to me 😄

[chrissm79]

Thank you @megawubs, great work on this one! 👍