GraphQL errors when passing sort parameters as separate variables

kinglozzer commented 6 months ago

Module version(s) affected

4.3.5+

Description

I’ve been seeing the following warning from some projects with GraphQL, have finally decided to stop ignoring them and investigate:

E_WARNING: Undefined property: GraphQL\Language\AST\VariableNode::$fields

from this line specifically:

https://github.com/silverstripe/silverstripe-graphql/blob/4.3.7/src/Schema/Traits/SortTrait.php#L49

I assume the code isn’t expecting a VariableNode instance

Appears to be a regression from https://github.com/silverstripe/silverstripe-graphql/pull/563 (4.3.4 doesn’t produce the warning, 4.3.5 and up does).

I don’t fully understand the code here, so I’m not sure why this is occurring yet. I’ll try to do some more testing tomorrow.

I’ve only confirmed this against SS4 (GraphQL v4) but I suspect the same issue would apply to SS5.

How to reproduce

The best I can offer at this stage is an example of the schema:

SilverStripe\Blog\Model\BlogPost:
  fields:
    id: true
    title: true
    urlSegment: true
    link: true
    editorsPick: true
    timeToRead: true
    publishDate: true
    timesViewed: true
    parentId: true
    epochPublishDate:
      type: 'int'
    cardImageSrc:
      type: 'string'
    minutesToRead:
      type: 'string'
    estimatedTimeToConsume:
      type: 'string'
    renderCard:
      type: 'string'
  operations:
    read: true

And an example query:

{
"operationName": "ReadBlogPosts", 
"query": 
"query ReadBlogPosts($filter: BlogPostFilterFields, $sort: BlogPostSortFields, $offset: Int, $limit: Int) {
  readBlogPosts(filter: $filter, sort: $sort, offset: $offset, limit: $limit) {
    edges: nodes {
      id
      title
      editorsPick
      publishDate
      cardImageSrc
      estimatedTimeToConsume
      link
      epochPublishDate
      renderCard
      __typename
    }
    pageInfo {
      totalCount
      hasNextPage
      hasPreviousPage
      __typename
    }
    __typename
  }
}"
, 
"variables": {
"filter": {}, 
"limit": 6, 
"offset": 0, 
"sort": []
}
}

Possible Solution

No response

Additional Context

No response

Validations

[X] Check that there isn't already an issue that reports the same bug
[X] Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

PRs

https://github.com/silverstripe/silverstripe-graphql/pull/577 (avoids the warning but doesn't fix the underlying problem)

kinglozzer commented 6 months ago

I’ve got a failing test case set up for this - the last two items in the data provider here: https://github.com/kinglozzer/silverstripe-graphql/commit/51c7589136cfc9fdde0a597032d52b1edeb534dc#diff-185c0371539557bb7be75e037e0082fd661eafa9d5a2c59ef11886da20f66f5eR782

A workaround to fix the warning is simple enough, but the original issue (sort fields coming through in the wrong order) will still be present for sort args passed in this way. I don’t think the original implementation of the fix took this method of passing sort fields into account

kinglozzer commented 6 months ago

I’ve just been digging into this, I’m not sure we really can fix this fully. Take this case:

query ($sort: DataObjectFakeSortFields, $fileSort: FilesSimpleSortFields) {
  readDataObjectFakes(sort: $sort) {
    nodes {
      myField
      files(sort: $fileSort) {
        title
      }
    }
  }
}

With these args:

'args' => [
    'sort' => ['myField' => 'ASC'],
    'fileSort' => ['ParentID' => 'DESC', 'name' => 'ASC'],
]

Debugging the ResolveInfo object that’s currently being used to fix the order, ParentID is only available in one place (ResolveInfo::$variableValues) and it’s already in the “wrong” order at that point. It seems there’s no way to actually access the original order the arg was provided in.

kinglozzer commented 6 months ago

I’ve posted about this here, and someone has pointed out a section in the GraphQL spec which makes me think the way we currently handle sorting might be wrong. I’ll post a separate issue about that.

For this issue, I think the only solution is to not try to correct the sort order if it is provided via an argument. I’ll create a PR soon

silverstripe / silverstripe-graphql