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

Incorrect response when errors occurred #2470

Closed
atzedus opened this issue Dec 14, 2022 · 2 comments
Closed

Incorrect response when errors occurred #2470

atzedus opened this issue Dec 14, 2022 · 2 comments

Comments

@atzedus
Copy link
Contributor

atzedus commented Dec 14, 2022

What happened?

When querying multiple fields at once and at least one of it returns error, then whole response become null with no any description where it null is.

What did you expect?

List of errors and data with fields name queried.

Minimal graphql.schema and models to reproduce

Schema:

type Query {
    Kids: [ID!]
    Martials: [ID!]
}

Query:

query Lists {
  	Kids
  	Martials
}

Result:

{
  "errors": [
    {
      "message": "some error",
      "path": [
        "Kids"
      ]
    }
  ],
  "data": null
}

What expected?:

{
  "errors": [
    {
      "message": "some error",
      "path": [
        "Kids"
      ]
    }
  ],
  "data": {
       "Kids": null,
       "Martials": [1, 2, 3, 4]
  }
}

versions

  • go run github.com/99designs/gqlgen v.0.17.22?
  • go 1.19?

P.S. I will send pull request to fix this soon.

atzedus pushed a commit to atzedus/gqlgen that referenced this issue Dec 14, 2022
atzedus pushed a commit to atzedus/gqlgen that referenced this issue Dec 14, 2022
StevenACoffman added a commit that referenced this issue Dec 14, 2022
* Fix issue #2470

* go generate ./...

* regenerate examples

Signed-off-by: Steve Coffman <steve@khanacademy.org>

Signed-off-by: Steve Coffman <steve@khanacademy.org>
Co-authored-by: Roman A. Grigorovich <ragrigorov@mts.ru>
Co-authored-by: Steve Coffman <steve@khanacademy.org>
@atzedus atzedus closed this as completed Dec 14, 2022
@jwatte
Copy link

jwatte commented Jan 24, 2023

This is a major behavior change, breaking a lot of existing code.
Making this part of a bugfix version release is quite surprising.

@pksieminski
Copy link

pksieminski commented Mar 6, 2023

@atzedus are you sure that behaviour that you described in your issue is correct for the schema you listed?

I rolled back gqlgen to version v0.17.22 and tested it with your schema:

extend type Query {
  Kids: [ID!]
  Martials: [ID!]
}

Query:

query {
  Kids
  Martials
}

Returned nil, errors.New("some error") in Kids resolver and received this response:

{
  "errors": [
    {
      "message": "some error",
      "path": [
        "Kids"
      ],
    }
  ],
  "data": {
    "Kids": null,
    "Martials": ["1", "2", "3", "4"]
  }
}

To receive result as claimed in your issue, Kids list would have to be Non-Null type as follows for KidsNN:

extend type Query {
  Kids: [ID!]
  KidsNN: [ID!]!
  Martials: [ID!]
}

Query:

query {
  Kids
  Martials
  KidsNN
}

Now with same nil, errors.New("some error") return value for KidsNN resolver, you will receive response:

{
  "errors": [
    {
      "message": "some error",
      "path": [
        "Kids"
      ],
    },
    {
      "message": "some error",
      "path": [
        "KidsNN"
      ],
    }
  ],
  "data": null
}

And above behaviour is correct according to Graphql specification:

  • Since Kids list type is not Non-Null, in case of null value and error, it propagates it at most up to the list
  • Since KidsNN list type is Non-Null, in case of null value and error, it propagates further up to data root field, since KidsNN is Non-Null and returning it null would break schema contract and work against GraphQL spec

According to this spec previous behaviour was correct:

If a List type wraps a Non-Null type, and one of the elements of that list resolves to null, then the entire list must resolve to null. If the List type is also wrapped in a Non-Null, the field error continues to propagate upwards.

If all fields from the root of the request to the source of the field error return Non-Null types, then the "data" entry in the response should be null.

Based on the above points I believe that #2471 should be reverted. Right now it is also causing issues in more complex scenarios, where type wrapped in a list is a structure. When one of its fields resolves to Null then it does not propagate up properly - see #2541.

atzedus pushed a commit to atzedus/gqlgen that referenced this issue Mar 6, 2023
StevenACoffman pushed a commit that referenced this issue Mar 7, 2023
* Revert "Fix issue #2470: Incorrect response when errors occurred (#2471)"

This reverts commit 5cb6e3e.

* Closes #2523 #2541

* misspell lint fix

---------

Co-authored-by: Roman A. Grigorovich <ragrigorov@mts.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants