Skip to content

[PARENT] Improve documentation and static checks regarding mutation errors conventions

Summary

Our mutations currently have two distinct error modes:

  1. for argument errors, syntax errors and the like, the service will fail and the top level object will be something like: {errors: [Error]} These are non-recoverable errors, and they should not be shown to the user.
  2. 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 are user-errors (of interest to the user, not necessarily caused by them), or recoverable-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

Edited by Christen Dybenko