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

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

Conversation

AnthonyMDev
Copy link

Fix the Document Validation Failure examples to match the wording of the other examples.

Fix the Document Validation Failure examples to match the wording of the other examples.
Copy link
Contributor

@Shane32 Shane32 left a comment

Choose a reason for hiding this comment

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

Fine with me

Comment on lines +718 to +719
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).
Copy link
Member

@benjie benjie Feb 24, 2025

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?

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

Successfully merging this pull request may close these issues.

5 participants