Report GraphQL system errors as expected #4811
Conversation
Coverage report
Show files with reduced coverage 🔻
Test suite run success1890 tests passing in 865 suites. Report generated by 🧪jest coverage report action from e35b8ab |
| } else { | ||
| mappedError = new AbortError(errorMessage) | ||
| } | ||
| const mappedError: Error = new GraphQLClientError(errorMessage, status, error.response.errors) |
There was a problem hiding this comment.
Not strictly necessary, but using the same type here allows us to retain information that is otherwise lost. This includes the specific error messages, the status code, and stack information.
Not sure of the original intent of "Client" here. Is it meant to convey a 4xx class error, or an error received by the GraphQL client? If the former, then this new usage would require a rename, at least as currently written,.
There was a problem hiding this comment.
I think its the latter but this is one of the older corners of things, I don't think there's any issue with making larger changes here if you see fit
I don't think we need to say mappedError: Error if its only going to ever be a GraphQLClientError now.
| @@ -234,3 +237,31 @@ function errorMessageImpliesEnvironmentIssue(message: string): boolean { | |||
| const anyMatches = environmentIssueMessages.some((issueMessage) => message.includes(issueMessage)) | |||
| return anyMatches | |||
| } | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
There was a problem hiding this comment.
I don't like using any here and would appreciate any help in removing it.
There was a problem hiding this comment.
unknown would be better and reflects what this gets called with better I think. If you combine it with the type narrowing below I think it'll work nicely -- you check at runtime that this is a client error and then you do your extra logic around the nature of the error
| return false | ||
| } | ||
|
|
||
| const extensions = error?.errors?.[0]?.extensions |
There was a problem hiding this comment.
Here and below, we are looking at only the first error, so this still needs work. The logic shoujld be that the error is internal if any of the errors in the array are internal.
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| function isGraphQLError(error: any): boolean { | ||
| return error?.name === 'GraphQLClientError' |
There was a problem hiding this comment.
My attempts to use something like instanceof did not work. This another place where I could use some help with typing.
There was a problem hiding this comment.
I think what you want here is a narrowing function. This is something that takes some input at runtime and if true, makes some implication about the provided value https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates
So in this case, maybe roughly:
function isGraphQLError(error: unknown): error is GraphQLClientError {
return error !== null && typeof error === 'object' && 'name' in error && error.name === 'GraphQLClientError'
}
then elsewhere you can do
if (isGraphQLError(error)) {
// safe to treat this as a GraphQLClientError
error.someGraphQLThing.whatever()
}
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| function isGraphQLError(error: any): boolean { | ||
| return error?.name === 'GraphQLClientError' |
There was a problem hiding this comment.
I think what you want here is a narrowing function. This is something that takes some input at runtime and if true, makes some implication about the provided value https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates
So in this case, maybe roughly:
function isGraphQLError(error: unknown): error is GraphQLClientError {
return error !== null && typeof error === 'object' && 'name' in error && error.name === 'GraphQLClientError'
}
then elsewhere you can do
if (isGraphQLError(error)) {
// safe to treat this as a GraphQLClientError
error.someGraphQLThing.whatever()
}
| @@ -234,3 +237,31 @@ function errorMessageImpliesEnvironmentIssue(message: string): boolean { | |||
| const anyMatches = environmentIssueMessages.some((issueMessage) => message.includes(issueMessage)) | |||
| return anyMatches | |||
| } | |||
|
|
|||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
There was a problem hiding this comment.
unknown would be better and reflects what this gets called with better I think. If you combine it with the type narrowing below I think it'll work nicely -- you check at runtime that this is a client error and then you do your extra logic around the nature of the error
| } else { | ||
| mappedError = new AbortError(errorMessage) | ||
| } | ||
| const mappedError: Error = new GraphQLClientError(errorMessage, status, error.response.errors) |
There was a problem hiding this comment.
I think its the latter but this is one of the older corners of things, I don't think there's any issue with making larger changes here if you see fit
I don't think we need to say mappedError: Error if its only going to ever be a GraphQLClientError now.
WHY are these changes introduced?
Improve accuracy of the CLI error count SLOs by marking GraphQL failures due to internal server errors as
expected.Internal server errors are recorded as unexpected events by the back-end that generates them. From the point of view of the CLI, we expect such errors will occur and we communicate the failure to the user. That's all we can do, so they should not count against the error count SLOs
WHAT is this pull request doing?
Uses a common type to identify all GraphQL. Interrogates the error response info to determine if the error is internal, and reports the error as expected if it is internal.
How to test your changes?
Post-release steps
Measuring impact
How do we know this change was effective? Please choose one:
Checklist