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

Be strict about error paths format #1073

Merged
merged 3 commits into from
Apr 4, 2024

Conversation

martinbonnin
Copy link
Contributor

@martinbonnin martinbonnin commented Jan 25, 2024

Replace should with must in the description of error paths: This field must be a list of path segments starting...

    {
      "message": "Name for character with ID 1002 could not be fetched.",
      "locations": [{ "line": 6, "column": 7 }],
      "path": ["hero", "heroFriends", 1, "name"]
    }

Anything else will make it impossible to handle errors in error-aware clients. This is especially important for strict-nullability or semantic-non-nullability

Copy link

netlify bot commented Jan 25, 2024

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 88d99e5
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/65bbef231a82d50009549022
😎 Deploy Preview https://deploy-preview-1073--graphql-spec-draft.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

This is technically a breaking change; but it seems more of a bugfix to me - this should (haha) have been a must all along, and I'm not aware of any implementation that is breaking these rules. I'm not going to mark this as an approval until we've discussed at the WG, but I am heavily in favour of it.

@leebyron Was this deliberately a should, or was it a case where natural language accidentally made its way in rather than RFC2119 language?

@martinbonnin Please add this to an upcoming WG for discussion.

cc @graphql/implementers - is anyone's implementation going to break if we change this from should to must?

BoD added a commit to BoD/graphql-wg that referenced this pull request Jan 30, 2024
With @martinbonnin 's permission, I've added back the discussion about  graphql/graphql-spec#1073 to the agenda
@captbaritone
Copy link

cc @mjmahone in case you have context on why this is not "must". You mentioned that this might have been an intentional choice to allow servers to use errors to silently handle things like permissions?

I can see a parallel here with things like returning 404 errors when you try to access a route that you don't have permission to view in order to prevent you from using the status code to determine if a private entity exists or not.

@leebyron leebyron added the 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) label Feb 1, 2024
@mjmahone
Copy link
Contributor

mjmahone commented Feb 1, 2024

@captbaritone I was misunderstanding the context. I don't have an objection to the path being required to be a list of strings/ints if it exists.

spec/Section 7 -- Response.md Outdated Show resolved Hide resolved
@leebyron leebyron added 📄 Draft (RFC 2) RFC Stage 2 (See CONTRIBUTING.md) and removed 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) labels Feb 1, 2024
@leebyron
Copy link
Collaborator

leebyron commented Feb 1, 2024

@graphql/implementers - please review before we accept

@leebyron
Copy link
Collaborator

leebyron commented Feb 1, 2024

Radiating intent here (and action item): We intend to accept this and merge it next month barring any feedback.

@andimarek
Copy link
Contributor

From a GraphQL Java POV we are ok with that

@mjmahone
Copy link
Contributor

mjmahone commented Mar 7, 2024

At the Mar 7 WG meeting we established that we intend to merge this PR after the April WG meeting, unless there are objections.

If you implement a GraphQL server or client and do not implement the error paths array as specified, please let us know!

@leebyron leebyron removed the 📄 Draft (RFC 2) RFC Stage 2 (See CONTRIBUTING.md) label Apr 4, 2024
@leebyron leebyron added the 🏁 Accepted (RFC 3) RFC Stage 3 (See CONTRIBUTING.md) label Apr 4, 2024
@leebyron leebyron merged commit 32d24f6 into graphql:main Apr 4, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏁 Accepted (RFC 3) RFC Stage 3 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants