[RFC] Require selection of error type in data

[cmonty]

The GraphQL community has been pretty vocal that the errors dictionary is confusing for representing domain-level errors. At the same time @leebyron has stated the position that errors is only for exceptional things (e.g. GraphQL parsing errors, systems being down, etc) and not for user triggered errors (e.g. malformed input). See #135 & #391

As Braintree has been transitioning our public API in GraphQL, we've found that there are pros and cons with using errors to represent user triggered errors. The most obvious pro is all of the errors are co-located so that a client does not need to know about another concept of error. If something didn't work, you look in errors.

While being co-located is nice, using errors to represent user errors becomes difficult and loses the benefits of GraphQL, most notably documentation and types. The spec allows for "extensions" to errors, which becomes a dumping ground of undocumented, untyped keys and values. How do users know the structure, types, and cause of these errors in standard tooling besides trial and error?

On the other hand, putting these errors in data means a user not only has to handle errors by looking at two different locations, but the user could not know they need to select domain errors as part of the mutation, causing clients to miss out on information that is challenging to replicate. You'll probably only make this mistake once, but if you're integrating with a few different mutations over a larger period of time or across teams, the mistake may be repeated multiple times. In the end, this forces people to always select these error fields, which leads to my proposal.

I think we could use the GraphQL validation system to ensure errors are selected for the response. Note the following schema definition:

type Mutation {
  createUser(input: UserInput!): UserPayload
}

input UserInput {
  name: String
  email: String
  ...
}

type UserPayload {
  id: ID
  name: String
  ...
  mutationErrors: [MutationError]
}

error MutationError {
  message: String
  code: String

  """
  Input field on which caused the error
  example: ["input", "name"]
  """
  inputPath: [String]
}

If the client were to execute a query without selecting the mutationErrors field, the GraphQL validation system would fail the query, placing the error in errors:

mutation CreateUser($input: UserInput!) {
  createUser(input: $input) {
    id
  }
}

Executing results in:

{
  "data": null,
  "errors": [{
    "message": "Field 'mutationErrors' required to be selected for 'createUser' mutation",
    "path": "createUser"
  }]
}

To resolve the GraphQL validation error select the mutationErrors field with any or all sub-fields:

mutation CreateUser($input: UserInput!) {
  createUser(input: $input) {
    id
    mutationErrors {
      code
    }
  }
}

The use of a new top-level keyword error is to tell the validation system that the field is required to be selected and included in part of the response.

This may open a slippery slope that goes against a core ideal of GraphQL: the client selects what is to be returned. I think, though, in some instances having the server give some restrictions, especially around the case of user error handling, this could make sense. And, it's not required that you use the error keyword or even errors in your Mutations at all, in which case the client can keep behaving as it does today.

Note that adding an error type to an existing Mutation would be backwards-incompatible in the same way as making an input field non-nullable.

[alloy]

Thanks for filing this! I gave a talk at GraphQL Finland the other day on this very subject, you can find a blog post version of it here http://artsy.github.io/blog/2018/10/19/where-art-thou-my-error/

I like the idea of expressing error types explicitly, as something that can’t be overlooked, and distinctly from other domain models. When I spoke with @IvanGoncharov about this he also mentioned that maybe errors warrant their own specification (and syntax), but iirc it was mostly to distinguish errors from domain models as otherwise being noise when looking at a schema.

My issue with this current proposal is that it only caters to ‘user errors’ in the sense that a mutation input was invalid, whereas there can be many more expected errors that may occur, such as requests to upstream services failing. Our GraphQL service is an aggregation service over backend services and so for any operation (both mutations and queries) part of the selections may fail to resolve. (Consider a subset of the query to result in an authorization denied error, we may not want that to fail the entire operation.) When considering queries and providing error information at potentially any node, the usage of unions over additional error fields to make it explicit either success or a failure was the case has thus far been my preference.

Additionally, I would prefer to keep (some of) the semantics around GraphQL types in general, specifically interfaces. (Interfaces are great to be able to have more generic clients.) It is not clear to me how an “error” type in your proposal would differ (or not) from other output object types, besides being marked as an error. Can you elaborate on that?

[cmonty]

Thanks for the feedback @alloy and awesome talk at GraphQL Finland!

My initial proposal is probably too narrow because we also run into the same situations where an upstream service could fail or any other situation (there are numerous!) where the user did not cause the failure. I feel this proposal could be more broad to include those cases, but the mechanics would remain the same.

In my initial thinking, the "error" type I presented above would only be a mechanism for the GraphQL validator to enforce that the client think about errors being returned outside of the top-level errors dictionary. Interfaces and unions could still work in order to provide the most flexibility.

Building on what you already presented:

interface IGenericError {
   message: String
}

interface IHttpError {
   message: String
   statusCode: String
}

error HttpError implements IGenericError & IHttpError {
   message: String
   statusCode: String
}

union UserOrError = User | HttpError

type Mutation {
  createUser(input: UserInput!): UserOrError
}

The following request would be invalid based on my proposal:

mutation CreateUser($input: UserInput!) {
  createUser(input: $input) {
    ... on User {
      id
    }
  }
}

{
  "data": null,
  "errors": [{
    "message": "Type 'HttpError' required to be selected for 'createUser' mutation",
    "path": "createUser"
  }]
}

But, if you additionally selected all of the error types in the union, the request would pass the validator:

mutation CreateUser($input: UserInput!) {
  createUser(input: $input) {
    ... on User {
      id
    }
    ... on HttpError {
      statusCode
    }
  }
}

[HenrikFricke]

For me putting validation errors in the response of a mutation feels always like a workaround for a missing feature. If I get data from the mutation (or a status code 200), then I want to trust that the mutation has been successful. For all kind of errors - including validation errors, I'd like to use the concept of throwing exceptions.

I recently opened the ticket #532 and closed it to add my thoughts to this ticket. In my ticket, I compared the GraphQL schema with a thrift file. Not that I am a huge fan of Thrift, but I really like that I can define exceptions for every method.

Like this:

struct Profile {                            
    1: required string id;
}

service ProfileService {
    Profile changePassword(
        1: string current,
        2: string new
    ) throws (
        1: InvalidCurrentPasswordException invalidCurrentPasswordException
    2: InvalidNewPasswordException invalidNewPasswordException 
    )
}

I wish we could have something similar for the GraphQL schema:

input ChangePasswordInput {
  current: String!
  new: String!
}

error InvalidNewPassword {
  lowercaseLetterMissing: Boolean!
  uppercaseLetterMissing: Boolean!
  numberMissing: Boolean!
  minimumCharactersReached: Boolean!
}

mutation {
  changePassword(password: ChangePasswordInput!): Profile throws InvalidNewPassword
}

Benefits would be, that we can generate also TypeScript definitions for the errors and validate the integration between the front-end and the GraphQL API with every deployment. Currently, we just use the extension feature of errors in our projects. That's quite nice, but we don't have any TypeScript definitions for the extensions which could lead to unwanted behaviour.

[cmonty]

@HenrikFricke For your example, what would be returned to the client in the case of errors? Would you select the fields you want from InvalidNewPassword type? Would the field values be returned in data or errors? Curious to hear your thoughts, thanks!

[HenrikFricke]

@cmonty Good questions, so basically I would love to get them as errors, so I can easily identify in the consumer that something went wrong and I didn't get any valid data. The idea of also providing a selector for the errors is interesting, but then we would need something like inline fragments to distinguish between the errors and the fields we want. Maybe that's too complex for error handling. I could imagine that we just describe the extension payload of the error in the GraphQL schema.

So, if this would be the GraphQL schema:

error InvalidNewPassword {
  lowercaseLetterMissing: Boolean!
  uppercaseLetterMissing: Boolean!
  numberMissing: Boolean!
  minimumCharactersReached: Boolean!
}

Then we could have an implementation like this (example in React with Apollo client):

const MyComponent = () => (
  <Query>
    {({ data, error }) => {
      if (error) {
        // We would still need some type casting here
        const invalidNewPasswordError = error.graphQLErrors[0];

        if (invalidNewPasswordError.extension.lowercaseLetterMissing) {
          return <p>Add at least one lowercase letter.</p>;
        }

        return <p>Something went wrong</p>;
      }

      return <input type="text" />;
    }}
  </Query>
);

For the typecasting, we also need to think about to add a __typename property to the error object.

What's your opinion about it? To be honest I am also not hundred percent sure if this is actually a good approach, but I can see the benefit of having type definitions for the extension payload.

[cmonty]

I think having one place for errors is a benefit of your approach. I tend to think of errors as a place for "compiler" errors and data as a place for "runtime" errors. It's not entirely clear the fields will end up in errors.extensions. Having explicitly defined and introspectable fields is a big win either way.