search

graphql / graphql-spec

GraphQL is a query language and execution engine tied to any backend service.
https://spec.graphql.org
14.31k stars 1.13k forks source link

Be strict about error paths format #1073

Closed martinbonnin closed 7 months ago

martinbonnin commented 10 months ago

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

netlify[bot] commented 10 months ago

Deploy Preview for graphql-spec-draft ready!

Name Link
Latest commit 88d99e550b39c29c611989928fd53c9407d1d00b
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.

captbaritone commented 9 months ago

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.

mjmahone commented 9 months ago

@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.

leebyron commented 9 months ago

@graphql/implementers - please review before we accept

leebyron commented 9 months ago

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

andimarek commented 9 months ago

From a GraphQL Java POV we are ok with that

mjmahone commented 8 months ago

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!