Errors best practices

[majkelcc]

I have some issues and thoughts on error handling I wanted to share. This part of implementing my own GraphQL API is the most time consuming for me, probably due to not being covered too well by GraphQL specification.

It would be a good idea to standarize error handling in this gem and update the docs on best practices.

Issues

1. Latest specification added an extensions key to errors to avoid potential confclicts in the future: https://facebook.github.io/graphql/June2018/#example-fce18. Currently some errors return additional information, like value or problems in case of GraphQL::CoercionError. Are there plans for wrapping these additional fields under this new extensions key?
2. When writing a custom type and throwing a GraphQL::CoercionError exception from coerce_input, path key is empty, but default coercion errors, like thrown when passing a string as Boolean type does. This is probably a separate bug, but I noticed that path key even when present, doesn't always contain the full path to the mentioned field. Like here:

mutation($changelog: changelogInput!) {
  createChangelog(changelog: $changelog) {

...

{
  errors: [{
    "message": "Variable changelog of type changelogInput! was provided invalid value",
    "locations": [
      {
        "line": 1,
        "column": 10
      }
    ],
    "value": {
      ...
      "published": "true",
    },
    "problems": [
      {
        "path": [
          "published"
        ],
        "explanation": "Could not coerce value \"true\" to Boolean"
      }
    ]
  }]
}

It looks like the path should contain both changelog and published keys?
3. Is there a reason for stopping further validation of input values after first error occurs?

Quick thougts/ideas

1. In new class-based API (great job!) I can define a custom type, for example:

Types::ChangelogType < ApplicationType
  object_alias :changelog

  field :id, GraphQL::Types::ID
  field :title, String
  field :published, GraphQL::Types::Boolean

  def title
    ...
  end
end

And in it, this title method acts like an accessor to the title argument. It would be great if from such method I could raise an exception in case of invalid data. Building a separate type for such field and raising CoercionError is not ideal in some cases.
2. https://github.com/rmosolgo/graphql-ruby/blob/master/guides/mutations/mutation_errors.md#errors-as-data contains an interesting idea of returning errors as data. Personally I like it, but it seems to be inconsistent with behaviour in case of other errors, like mentioned coercion. I think it would be better to have one way of dealing with input errors.
3. In schema I can define a method handling type_errors. I think it's a very cool ability and maybe that's a good way to go for handling other types of errors, if not all?
4. In examples errors are usually returned as full messages, which makes it harder to control the presentation layer. I think that Rails got it right by returning errors in a structure like { "title" => ["is invalid"]}. I would argue that we should head into a similar direction.

Sorry if it's too long, but I wanted to write down most of the things I found a problem with.

[rmosolgo]

additional fields under this new extensions key?

Yes, this is implemented in 1.9+, see #2077

I noticed that path key even when present, doesn't always contain the full path to the mentioned field

Try adding error_bubbling(false) to your schema definition. This new behavior was added in #2013 and made the default in #2069. Let me know what you find in that case.

a reason for stopping further validation

I'm not exactly sure what you mean by that, but if you'd like to open an issue with an example the current behavior and an example of the desired behavior, I'm happy to discuss it!

raise an exception

Did you try raising GraphQL::ExecutionError? It's made for this purpose. https://graphql-ruby.org/errors/execution_errors.html#adding-errors-to-the-array

I think it would be better to have one way of dealing with input errors... I think [type_error is] a good way to go for handling other types of errors, if not all? ... I think that Rails got it right by returning errors in a structure...

Thanks for sharing your thoughts. These suggestions are interesting but not a priority for me personally. Please feel free to try building a system like that and let me know if anything can be improved in the library to support it!