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

Query sorting approach may be incorrect #573

Open
2 tasks done
kinglozzer opened this issue Feb 20, 2024 · 1 comment
Open
2 tasks done

Query sorting approach may be incorrect #573

kinglozzer opened this issue Feb 20, 2024 · 1 comment

Comments

@kinglozzer
Copy link
Member

kinglozzer commented Feb 20, 2024

Module version(s) affected

v4, v5

Description

Currently sort arguments are passed as an object:

{
    "created": "asc",
    "name": "asc"
}

According to the GraphQL spec, there’s no guarantee about the order that these will be interpreted, so the sort order might be interpreted as either Created ASC, Name ASC or Name ASC, Created ASC. It appears that webonyx/graphql-php will interpret them in the order they’re defined in the schema, not in the query or inputs.

We’ve managed to work around this by manually parsing the query in #563, but that only works if the sort order is baked into the query itself. When the sort order is passed as an argument to the query, the original order is lost as soon as the query is parsed (more info: #571 (comment)). That aside, manually inspecting the query AST to try to “fix” the sort order is probably an indication we’re approaching this the wrong way.

One alternative approach suggested is to use an array instead (a “list” in GraphQL speak). That might look something like:

[
    {"created": "asc"},
    {"name": "asc"}
]

If we could use that approach, I think it’d be important to preserve backward compatibility if at all possible. I haven’t looked at how that might be possible yet - perhaps interfaces or union types could help.

How to reproduce

Possible Solution

No response

Additional Context

No response

Validations

  • Check that there isn't already an issue that reports the same bug
  • Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)
@kinglozzer
Copy link
Member Author

I’ve been trying to think how we might be able to set up types for this while preserving some kind of backward compatibility with the existing sort fields. The only way I can come up with is this:

type Query {
  readMembers(
    filter: MemberFilterFields
    sort: MemberSortOptions
  ): [Member!]!
}

# New union type to support both old input (for bc) and new input
union MemberSortOptions = MemberSortFields | MemberSortExpressions

# Already exists
enum SortDirection {
  ASC
  DESC
}

# Already exists
input MemberSortFields {
  created: SortDirection
  name: SortDirection
}

# New enum of available fields to sort by
enum MemberSortField {
  name
  created
}

# New type
# Necessary because it's not possible to do a union type like: MemberSortFields | [MemberSortExpression]
type MemberSortExpressions {
  expressions: [MemberSortExpression!]!
}

# New type
type MemberSortExpression {
  field: MemberSortField,
  direction: SortDirection
}

So args for a sorted query with the new sorting approach would look like:

{
    "filter": {},
    "sort": {
        "expressions": [
            {"field": "created", "direction": "asc"},
            {"field": "name", "direction": "asc"}
        ]
    }
}

What’s not clear to me, however, is if backward compatibility is really feasible. If people are doing introspection on the schema, and expecting e.g. the existing MemberSortFields input type to be returned, would switching that to a union type break things for them? How would we “deprecate” the old approach?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants