[PARENT] Improve documentation and static checks regarding mutation errors conventions
Summary
Our mutations currently have two distinct error modes:
- for argument errors, syntax errors and the like, the service will fail
and the top level object will be something like:
{errors: [Error]}
These arenon-recoverable
errors, and they should not be shown to the user. - for errors encountered during execution, such as model validation errors,
git errors, etc, if anticipated they will be returned as part of the result
of the mutation, which will (from the perspective of the client), return
succesfully, with a result of the form:
{data: {mutationName: {errors: [Error]}}}
Ideally these are meaningful errors that should be shown to the user, either as explanation, or so they can fix something. These areuser-errors
(of interest to the user, not necessarily caused by them), orrecoverable-errors
.
This has downsides - the most obvious, from looking at our front end code, is
that we never check for the second of these two error conditions. This means
we have brittle code which will cause reference errors when the update handlers
try to reach deeply into objects that do not exist. (for example, an update
handler may evaluate data.mutationName.something.somethingElse
, which will
fail with essentially a null-pointer exception on when there is no something
on the result.
Adding the appropriate level of error handling gets tedious quickly, and makes for messy code that obscures the logic.
Improvements
We should be much clearer amongst ourselves about what these errors mean, and
when something is non-recoverable
vs recoverable
. This should be audited
during code-review on back-end MRs, and all front-end code must include tests for
the error states.
This is a documentation issue (we need to improve doc/development/api_graphql_styleguide.md) as well as a code-quality issue, where we need to hold ourselves to account about how errors are handled throughout the stack.
Involved components
Our GraphQL mutations.
Tasks
-
add section on errors to the mutations section of doc/development/api_graphql_styleguide.md. -
create global error fragment -
statically enforce its usage in every mutation -
add a graphql reviewer category (similar to database) -
have danger enforce that graphql MRs (touching code in app/graphql
or touching*.graphql
files) have a gitlab-ee~4133451 review
/cc @.luke