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.

Sign up for GitHub

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 consistency of examples #332

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix consistency of examples #332

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea13318
Fix consistency of examples
AnthonyMDev Feb 20, 2025
ac611c4
Update GraphQLOverHTTP.md
AnthonyMDev Feb 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions spec/GraphQLOverHTTP.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -599,7 +599,7 @@ code `200` (Okay).

##### Document validation failure

Requests that fail to pass _GraphQL validation_, the server SHOULD NOT execute
If a request fails to pass _GraphQL validation_, the server SHOULD NOT execute
the request and SHOULD return a status code of `200` (Okay).

##### Operation cannot be determined
Expand Down Expand Up @@ -715,8 +715,8 @@ code `400` (Bad Request).

##### Document validation failure

Requests that fail to pass _GraphQL validation_ SHOULD be denied execution with
a status code of `400` (Bad Request).
If a request fails to pass _GraphQL validation_, the server SHOULD NOT execute
the request and SHOULD return a status code of `400` (Bad Request).
Comment on lines +718 to +719
Copy link
Member

@benjie benjie Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has turned from one compliance statement to two, both of which are "recommended". I don't think that's ideal - ideally we recommend that you "both refuse to execute the request and return a 400 status to say as much". Also now I come to think about it "fails to pass" is a bit redundant. Maybe something like:

Suggested change
If a request fails to pass _GraphQL validation_, the server SHOULD NOT execute
the request and SHOULD return a status code of `400` (Bad Request).
If a request fails _GraphQL validation_, the server SHOULD return a status code
of `400` (Bad Request) without proceeding to GraphQL execution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: in traditional JSON we want two compliance statements here: should not execute, and SHOULD use 200, both of which are independent of each other (well... mostly. If you did execute then returning non-200 would be weird). You should always return 200, even on validation error; and you should not execute after validation error.

The same is not true with application/graphql-response+json - here the failure to execute and the 400 status are linked together. Failing to execute but returning non-4xx (e.g. 200) would be unexpected/undesired.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat of a side question but since we're in the "examples" section here, shouldn't we "just" use regular verbs (not capitalized). Examples shouldn't really be normative?


##### Operation cannot be determined

Expand Down