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

feat(typescript): expose type property for errors in graphql response errors #314

Merged
merged 2 commits into from
Aug 31, 2021

Conversation

devversion
Copy link
Contributor

GraphQL response errors can have a type field that can be useful
for error detection. e.g. if a pull request does not exist there is
an error with the NOT_FOUND type.

@gr2m maybe you could help with this here. I have not found any official specification from Github on this, but I see feature requests for this in other Github GraphQL libraries and I can see it in various requests from Github. We could also type this as potentially undefined? I lack information here unfortunately.

…se errors

GraphQL response errors can have a `type` field that can be useful
for error detection. e.g. if a pull request does not exist there is
an error with the `NOT_FOUND` type.
@ghost ghost added this to Inbox in JS Aug 30, 2021
@gr2m
Copy link
Contributor

gr2m commented Aug 30, 2021

Hi Paul, I just checked and confirmed the type property in the errors array

Example query

{
  repository(owner: "gr2m", name: "sandbox") {
    pullRequest(number: 999) {
      id
    }
  }
}

yields

{
  "data": {
    "repository": {
      "pullRequest": null
    }
  },
  "errors": [
    {
      "type": "NOT_FOUND",
      "path": [
        "repository",
        "pullRequest"
      ],
      "locations": [
        {
          "line": 3,
          "column": 5
        }
      ],
      "message": "Could not resolve to a PullRequest with the number of 999."
    }
  ]
}

But you say the type is not always set? In that case we should do type?: string. Can you share a query that yields a response with an .errors response key that has an error without the type key?

@gr2m gr2m added Type: Bug Something isn't working as documented typescript Relevant to TypeScript users only labels Aug 30, 2021
@devversion
Copy link
Contributor Author

devversion commented Aug 30, 2021

@gr2m Thanks for checking! (yeah I was checking some queries, like for the pull request and saw this property)

Sorry for the confusion. I was proposing we could make it type?: string in case we will not have any official confirmation on whether this property always exists or not. Sounds like it's always set though? so this PR is ready for review.

@gr2m
Copy link
Contributor

gr2m commented Aug 30, 2021

I was proposing we could make it type?: string in case we will not have any official confirmation on whether this property always exists or not. Sounds like it's always set though? so this PR is ready for review.

I think it's probably always set. Can you please ask in https://github.community/c/github-ecosystem/37, or send a message to support at https://support.github.com/contact? Either should give us an answer from GitHub. You can reference your pull request

@devversion
Copy link
Contributor Author

I've just sent a ticket. I'll keep you updated.

@ghost ghost moved this from Inbox to Awaiting response in JS Aug 30, 2021
@devversion
Copy link
Contributor Author

@gr2m I just received the response:

Thanks for reaching out to GitHub Support!

Yes, the type field is going to be returned with every error. The error format is described below:

https://spec.graphql.org/draft/#sec-Errors.Error-result-format

Hope this helps!
Regards,

It sounds like type is always set, according to the message. It surprises me though that the support linked to the GraphQL specification because errors only have the code field here. GraphQL also states the following:

GraphQL services should not provide any additional entries to the error format since they could conflict with additional entries that may be added in future versions of this specification. https://spec.graphql.org/draft/#sec-Errors.Error-result-format

I've replied and checked on that, but I assume that will not change any time soon, as it would/might break some services relying on the GraphQL API returning type instead of code.

src/types.ts Show resolved Hide resolved
@gr2m gr2m enabled auto-merge (squash) August 31, 2021 17:27
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Thanks a lot 💐

@gr2m gr2m merged commit ffa3428 into octokit:master Aug 31, 2021
JS automation moved this from Awaiting response to Done Aug 31, 2021
@github-actions
Copy link

🎉 This PR is included in version 4.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented typescript Relevant to TypeScript users only
Projects
No open projects
JS
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants