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

"Fix" for issue#2470 breaks years of tests and clients #2523

Closed
jwatte opened this issue Jan 24, 2023 · 4 comments
Closed

"Fix" for issue#2470 breaks years of tests and clients #2523

jwatte opened this issue Jan 24, 2023 · 4 comments

Comments

@jwatte
Copy link

jwatte commented Jan 24, 2023

What happened?

I upgraded from bugfix-release 0.17.22 to 0.17.24
Suddenly, clients that have worked for a long time, blow up, and unit tests that have passed for a long time, fail.
This is because the "fix" for issue #2470 breaks the API by returning data when also returning errors.

What did you expect?

Minor releases should not break major API behaviors.

Minimal graphql.schema and models to reproduce

Any schema will do this -- any handler that returns an error will now also return a (possibly empty) content.
The previous behavior was to return null for data when there are errors -- this behavior should be preserved.

versions

  • go run github.com/99designs/gqlgen version?
    v0.17.24-dev
  • go version?
    go version go1.18.5 linux/amd64
@omarkohl
Copy link

The new behavior is according to GraphQL specification I believe: https://spec.graphql.org/October2021/#sec-Errors.Error-result-format therefore the previous behavior and what #2470 fixes is in fact a bug.

@hdemirev
Copy link

I also found this change in behavior to be quite unexpected, besides being the issue with tests this also impacts production observability because the newly introduced must not be null error can dwarf the actual, relevant error returned by the server.

@vikstrous2
Copy link

In the original bug report, the root level fields were nullable. That's why the null shouldn't bubble up to the root. However, the fix didn't look at whether or not the root level fields are nullable. It made root level fields ALWAYS become null instead of bubbling up.

The spec here explains that this should depend on whether or not the fields are nullable https://spec.graphql.org/October2021/#sec-Handling-Field-Errors

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.

I think the problematic code is here https://github.com/99designs/gqlgen/pull/2471/files#diff-1a18bf3099cf00a138c2bd2e8757edc3addcf719e7bde166f94c62c3676cac1fR29-R31

@vikstrous2
Copy link

same thing reported in #2541

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

4 participants