-
Notifications
You must be signed in to change notification settings - Fork 65
Use 294 as the partial status code #346
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
Open
benjie
wants to merge
5
commits into
main
Choose a base branch
from
partial-status-code
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be simplified. Whether {data} is {null} should have no bearing on response semantics and should be removed to simplify the specification -- if {data} exists, then there is no request error and 2xx is required, 200 if no {errors}. If {data} exists alongside {errors}, that's a 294; but if there's {errors} without {data}, that's a 4xx. I would not add specifications for an illegal response, where {data} is {null} and {errors} does not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://spec.graphql.org/October2021/#sec-Response-Format
If the request "executes", then it's 2xx.
If it doesn't execute, it's a 4xx / 5xx.
Neither depend on whether
data
isnull
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have this:
So we can only know that execution may have started. Seems like 2xx to me, anyway. And it makes the spec simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the simplification
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your diff reads really confusing because of some weird GitHub glitch, but I think what you’re saying is to change the requirement for when data is null from “allowed to use any status code” to “required that it be a 2xx (and preferably 294)”. This has been discussed in the past and some people felt that data:null should code as an error (there’s no data so there’s no partial success!), whereas other felt it should code as a partial success (execution occurred!). The original express-graphql coded it as a 400 IIRC (maybe a 500? It definitely wasn’t 2xx whatever it was) and many servers followed that pattern, hence why the spec text carefully excludes that case allowing the implementer to make their own choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no specification for this, the schema adds a non-normative note indicating it’s not possible in case the reader isn’t aware and wonders what to do in that perceived edge case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I see it,
data
being null is a simple result of null bubbling, as is the case anywhere else in the schema. So if your schema is:And you request
{ dog(id: "3") { name } }
but either the dog's name isn't defined or the dog can't be found, null bubbling occurs anddata
ends up beingnull
. This is no different than if it were to occur anywhere else in the schema. Ifdog
returned a nullableDog
type, the client must check ifdog
is null. Basically, any GraphQL consumer must know how to handle nullable types (including in this case the root type itself) and/or look for theerrors
property to see if any errors were returned.I just don't see why there is an exception to the rule for a root type, and further, an exception that only occurs in the scenario where the GraphQL server were to provide an illegal response (a null
data
without anerrors
key).Understood but this does not apply. Any backwards compatibility should be defined for
application/json
, which we have defined to always return 200 for reasons already discussed. The language here is for the new response type which uses a different set of rules for status codes.While there's no partial data, it cannot be known that there's no partial success. Granted this may be a bad server design, but if we extended the above sample to mutations, it is certainly possible that a partial save occurred, but null bubbling occurred and obscured the result. Sample:
Here null bubbling would have occurred causing
data: null
even thoughsetDogNameA
executed successfully.