Improve GraphQL error handling #6358
Conversation
Coverage report
Test suite run success3228 tests passing in 1358 suites. Report generated by 🧪jest coverage report action from 1580630 |
| mappedError = new GraphQLClientError(errorMessage, status, error.response.errors) | ||
| } else { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| if (error.response.errors?.every((graphqlError: any) => graphqlError.type === 'GraphApi::UserError')) { |
There was a problem hiding this comment.
This seems reversed? Wouldn't we want user errors to return GraphQlClientError and any other error to abort?
There was a problem hiding this comment.
That would make sense looking at the function names, but in reality I think that:
- AbortError reports as handled
- GraphQLClientError reports as unhandled
So maybe we need a bigger refactor here to simplify and make it clearer
There was a problem hiding this comment.
I feel like it's hard, without context of the specific call, to qualify a 4xx error as always expected. If the CLI code is expecting to hit a server, and the CLI thinks the user is authenticated for example, then a 401 is probably unexpected since from the CLI's point of view, it assumed the user was authenticated.
So I don't know if we can globally categorize all these errors in one pass or not, or if we have to be a bit more precise and tackle each individual 401/403 case by case
There was a problem hiding this comment.
In general, I believe we shouldn't report errors that may be caused by users as unhandled. For example, a user with a wrong token in CI could artificially inflate our stats.
Using that same example, there's no way for the CLI to know if that 401 is expected or not. Maybe we can in some specific cases, like right after a full auth flow. I'll give it a try to distinguish them, but I think it's better to not report by default.
WHY are these changes introduced?
WHAT is this pull request doing?
How to test your changes?
Measuring impact
How do we know this change was effective? Please choose one:
Checklist