Input argument error paths in error response

[ivome]

Current limitation:

With the proposed changes to the RFC in #230 that are already implemented in graphql-js, it is only possible to determine which field caused an error. (Unless I am missing something?)
If you want to implement input validation with custom constraints for specific arguments, there is no way for the client to tell which input argument value is invalid with the current spec, especially if we have input values that contain lists or other complex objects.

Possible usecases:

If we would have a standardized way to process input errors, this would open up the possibility to create standard compliant libraries with server side form validation or generate complete forms and admin interfaces based on introspection queries.

Example:

Given the following schema:

# Input object type for createUser mutation
input MutationInput {
   username: String!
   categories: [ID!]!
}

# Mutation type
type Mutation {
  createUser(input: MutationInput!): ID!
}

Executed query:

# Query:
mutation CreateUser($input: MutationInput!) {
  createUser(input: $input)
}

# Variables
{
  input: {
    username: "invalidusername",
    categories: [ "1", "2", "someinvalidid" ]
  }
}

This could result in a response like this:

{
  "data": null,
  "errors": [
    {
       "message": "Input validation failed",
       "path": ["createUser"],
        // If the error was caused by one or more argument values on that field
        // we have a list with errors that each contain a human readable message
        // and the path within the field arguments to the value that caused the error
       "argumentErrors": [
          {
             "message": "Please provide a valid category ID",
              // We use 0-indexed path values for list input values
             "path": [ "input", "categories", 2 ]
          },
          {
             "message": "Please provide a valid username",
             "path": [ "input", "username" ]
          }
       ]
    }
  ]
}

This is just a quick draft. I would like to know what you think of the idea.

[calebmer]

The best place for this logic is in your GraphQL schema, and not in the GraphQL specification 😊

If we take your example:

input MutationInput {
   username: String!
   categories: [ID!]!
}

type Mutation {
  createUser(input: MutationInput!): ID!
}

…and have it return an object type:

input MutationInput {
   username: String!
   categories: [ID!]!
}

type Mutation {
  createUser(input: MutationInput!): CreateUserPayload
}

type CreateUserPayload {
  createdUserID: ID
}

You could also add a field for user validation errors. (Note how createdUserID is nullable so that if the mutation failed you don’t need to return a value.)

input MutationInput {
   username: String!
   categories: [ID!]!
}

type Mutation {
  createUser(input: MutationInput!): CreateUserPayload
}

type CreateUserPayload {
  createdUserID: ID
  errors: [MutationInputError]
}

type MutationInputError {
  message: String
  field: String
}

This would be much easier to use as well considering that most GraphQL clients today do not have sophisticated GraphQL error handling mechanisms.

I’ve also written a post with more recommendations on designing an effective GraphQL mutation system if you’re interested.

[ivome]

Thanks @calebmer for your reply! I like the article you posted!

This is exactly the approach that I am using in my current APIs. However, this approach seems to me to have some major limitations and inconsistencies:

• When a mutation field succeeds (returns a valid response object), I expect it to return the defined output. If a mutation fails, for whatever reason, I expect it to return an error. With the payload approach we then have two error scenarios that we need to handle in the client (GraphQL error and userError as payload), which adds another layer of complexity.
• All response fields in the payload object have to be nullable in case the mutation fails with user errors. This adds unnecessary checks for each mutation payload on the client. Generated flow types of the mutation payload object could potentially require tons of checks in the codebase. The proposed change would eliminate this need since we can have payload objects with non nullable fields.
• The error handling through a mutation payload is limited to mutations. What about validation of input arguments on query or subscription fields? The proposed change to the GraphQL API would cover that as well.
• The payload approach bloats the schema with redundant fields that are not even part of the actual response that we need, but rather a vehicle to expose error messages to the client.
• We get fragmented implementations across the GraphQL community regarding error handling. Every API would implement custom types with varying implementations, naming conventions etc. It is for example not possible to create form libraries that work across all GraphQL endpoints.

This would be much easier to use as well considering that most GraphQL clients today do not have sophisticated GraphQL error handling mechanisms.

I think it would be beneficial to the community to add such error handling to the spec. If this would be added to the standard, the GraphQL clients can adopt this and we can build other functionality on top of it that is reusable across all GraphQL endpoints, like generic form libraries.

[calebmer]

When a mutation field succeeds (returns a valid response object), I expect it to return the defined output. If a mutation fails, for whatever reason, I expect it to return an error. With the payload approach we then have two error possibilities that we need to handle in the client (GraphQL error and userError as payload), which adds another layer of complexity.

Yep, this can be frustrating. However, this is nothing new in the realm of API development 😊. For example, in HTTP you can have 4xx errors and 5xx errors. One represents an error that is fixable by the user and the other represents a fatal error that a user cannot fix. I think this distinction is useful in GraphQL as well. This pattern also allows you to be as detailed as you want with 4xx errors whereas throwing everything in errors means you will always need to adhere to the format that the specification defines at the time.

All response fields in the payload object have to be nullable in case the mutation fails with user errors. This adds unnecessary checks for each mutation payload on the client. Generated flow types of the mutation payload object could potentially require tons of checks in the codebase. The proposed change would eliminate this need since we can have payload objects with non nullable fields.

I hold the opinion that using non-null fields are a bad practice in GraphQL unless they absolutely positively make sense which to me is basically just an id field and maybe a name field. (Fun fact: the first version of GraphQL at Facebook did not allow you to define non-null fields!) There are a few reasons for this:

1. You can always make a field non-null if was nullable. This will not break your GraphQL clients. However, you can never make a non-null field nullable. This will break your GraphQL clients which won’t be expecting null values. This means that non-null values make it very hard for you to change the design of your API in the future. If you ever decide that you want to rename a field, that some objects should not have data for that field, or that you need to remove a field you can’t! If the field were nullable then you would have much more flexibility as your schema evolves.
2. The way error propagation works with non-null values means it makes more sense to go nullable wherever possible. For example, take the following two array types: [User] and [User!]. The latter requires that no item in the array can be null. Now let’s say when everything is working normally the response you would get back is: [User:1, User:2, User:3, User:4]. Now what if for whatever reason your backend can’t fetch User:3? If your type were [User] then the result would be [User:1, User:2, null, User:4], but if it were [User!] then the null value would propagate up one level and your result would just be null. This applies for object types as well. By making a field non-null you loose the ability to handle errors for that field specifically and must instead handle errors at the first point in your tree that returns a nullable value.

That’s my manifesto on nullable vs. non-null, however there is nothing stopping you from using non-null if you really want to! Just use a union type 😊

We would change my above example to:

union CreateUserPayload = CreateUserPayloadSuccess | CreateUserPayloadError

type CreateUserPayloadSuccess {
  createdUserID: ID!
}

type CreateUserPayloadError {
  errors: [MyCustomError!]!
}

…then you could query it like so:

mutation {
  createUser(...) {
    __typename
    ... on CreateUserPayloadSuccess { createdUserID }
    ... on CreateUserPayloadError { errors }
  }
}

This would result in types that look like:

type X = {
  createUser: {
    __typename: 'CreateUserPayloadSuccess',
    createdUserID: ...,
  } | {
    __typename: 'CreateUserPayloadError',
    errors: ...,
  } | {
    // In case you add more types to the union in the future.
    __typename: string,
  },
};

A tagged union type. Exactly what you want! If this type isn’t generated from that query then I’m going to be bold and say that is a bug in whatever is generating your types 😊

The error handling through a mutation payload is limited to mutations. What about validation of input arguments on query or subscription fields? The proposed change to the GraphQL API would cover that as well.

You could do a similar pattern in your queries and subscriptions. For instance:

type Query {
 search(...): SearchResult
}

type Subscription {
  search(...): SearchResult
}

union SearchResult = SearchResultSuccess | SearchResultError

I would argue that this is also better then the proposal because it keeps separate 4xx style user errors and 5xx style server errors. You can also taylor your 4xx style errors to the exact needs of your UI using the full power of GraphQL 😊

The payload approach bloats the schema with redundant fields that are not even part of the actual response that we need, but rather a vehicle to expose error messages to the client.

Expand on this idea? There is should be no shame in bloating your schema because the ideal is for our schemas to be versionless. This means that all “versions” of your API should live together in the schema at once. GraphQL then gives you the power to pick and choose a small subset from the bloat.

We get fragmented implementations across the GraphQL community regarding error handling. Every API would implement custom types with varying implementations, naming conventions etc. It is for example not possible to create form libraries that work across all GraphQL endpoints.

Your right that this could be sad, however to be fair I have never seen a good error handling approach that works for everyone 😊

Everybody has their own needs for the information the need from errors and how to render that information in the UI. I think of the validation errors you describe as mainly a UI concern (that’s why I feel it should be in GraphQL) and very few people have the same UI. Community standards might not be the right tool here, but company standards definitely could be. It’s worth noting that Relay Modern allows you to build custom handlers for certain patterns in your GraphQL API. I haven’t seen these used outside of connections, but this could be a perfect application!

Finally, if it really is super important that there is a shared standard for UI validation errors then you could write a specification for GraphQL like the Relay specifications.

---

Ultimately, I think using GraphQL is going to more powerful and provide more optionality then any specification we could write that modifies GraphQL’s error handling behavior 😊

[smolinari]

If GraphQL is going to be ubiquitous, as @leebyron would like it to be (and me too!), then there needs to be a section in the spec on standard error handling. It needs to be certainly more than what is currently in the spec right now. Thankfully, it is clear from that section that the concern of error handling is left open for future development. So now, this suggestion comes at a good time and it is just a matter of the community coming up with a proper spec for errors. 😄

Scott

[IvanGoncharov]

@ivome I really interested in this functionality, since we started to work on middleware for validating input args and having the standard way to report errors will increase the usability of such middleware.

I suggest you make PR which adds RFC into rfcs folder.
Here is a video where Lee Byron explains RFC process in details:
https://youtu.be/zVNrqo9XGOs?t=35m23s
And here are set of questions that RFC should answer:

IMHO, your initial comment answers all of them except 4 & 5.
In your example message says Input validation failed and it will be the only message that will be shown by existing clients. It severely impacts debugging experience on older clients used with newer servers. I think info from argumentErrors should be duplicated inside message as plain text.

I just have a few minor technical suggestions:

1. having the name of argument at the start of path is confusing so I would suggest splitting it out into argumentName property.
2. I would think about internationalization in advance especially with use cases like admin panel and forms generation. I would suggest adding errorCode for a string that can be used to map error to localized template of the error message and messageArgs for arguments of such template.

[leebyron]

I'm going to move this RFC to Rejected for a handful of reasons:

• Since being discussed, a common best practice has been to include user errors as part of the schema itself so they can contain domain specific information. Caleb mentioned this pattern above.

• Argument values are checked either at validation time, or before execution time when coercing variable values. Providing more metadata about developer errors for use in tools could be welcome, but it doesn't seem like that is the focus of this issue.

[AlbertMarashi]

I think this should be relooked at, this would be a great feature

[shoe-diamente]

@leebyron

• Since being discussed, a common best practice has been to include user errors as part of the schema itself so they can contain domain specific information. Caleb mentioned this pattern above.

What about errors that can happen at any node in the graph? Such as lack of permissions to read a specific node?

[dantman]

@leebyron @calebmer I do not recall ever reading this "best practice" in any of the GraphQL documentation on mutation or errors or in any articles I've read. In fact this is the first time I've ever heard that I should be writing my mutations with MutationResult | MutationError style unions instead of throwing errors. In fact the Apollo developers were probably also never given this message considering the presence of the UserInputError error type.

This is a big problem, because to my understanding taking an api like createReview(episode: $ep, review: $review): Review (from the docs) and rewriting it to be createReview(episode: $ep, review: $review): CreateReviewPayload whether CreateReviewPayload is type CreateReviewPayload { review: Review, error: Error } or union CreateReviewPayload = Review | CreateReviewError would be a schema breaking change for an API that had already created mutations in the style shown by the official documentation.

[benjie]

Following the Relay specifications is often a best practice; in particular the Relay Input Object Mutations Specification advises a shape of mutation that would allow extending the payload to add an error field without being a breaking change. Maybe we should link to the Relay specs from the best practices section?

https://facebook.github.io/relay/graphql/mutations.htm

[dantman]

The GraphQL documentation already has best practices on pagination. If this is the expected way to handle user input errors in GraphQL and not a Relay-ism then the official site should have best practices on handling user input errors. Not everyone follows the exact patterns Relay uses.

The mutation documentation should also use examples that follow the best practice.

[benjie]

Agreed; however I think the best practices on handling errors in mutations are still emerging.

I think having the payload type for mutations is definitely a best practice though, since without this you cannot add "meta" fields without breaking the schema or having to add a new mutation. We should definitely advise at least using mutation payloads.

The input object also makes a lot of sense when it comes to client applications though, IMO; for example I use Apollo with graphql-code-generator to generate my mutations, and being able to pass around the $input: MyMutationInput input variable as a single strongly typed object is very convenient.

clientMutationId, OTOH, is unnecessary unless your client needs it (although it is one way of meeting the "at least one field must be defined" requirement on the payload type for a mutation that doesn't currently return anything).