Allow resolvers to return multiple errors

[stanishev]

This PR is to enable resolvers to return an array of GraphQL::ExecutionErrors and to have the same behavior we see when a single GraphQL::ExecutionError is returned from a resolver.

More detailed description here: #1230

[rmosolgo]

Thanks for the detailed description in the issue and for proposing this fix, I'll take a close look soon!

[rmosolgo]

This fix looks good to me, do you mind adding a test case for it? You can either extend one of the (giant) fixture schemas that are already in the test suite, or you can write a small schema for testing this behavior in isolation, then demonstrate the new behavior that you added. That way, we can be sure it will keep working in the future!

[stanishev]

Will do, thanks for the suggestion (I thought about adding a test case with the original commit but felt disoriented within the test suite and wasn't sure where to start, I can go back and look at it now with more focus).

update: done. I went for what I think made sense, but happy to fix/modify if you think the test belongs elsewhere or should look differently.

[cabello]

I am curious, does this violate the spec?

If the field returns null because of an error which has already been added to the "errors" list in the response, the "errors" list must not be further affected. That is, only one error should be added to the errors list per field.

http://facebook.github.io/graphql/October2016/#sec-Errors-and-Non-Nullability

[stanishev]

I am honestly confused by the wording in that spec. The errors key is a list of errors, the only required key is message. To not have multiple errors per field (as that final sentence on its own implies) would be very limiting in my opinion, but I also can't make any sense of that statement because there is nothing in the graphql errors spec that ties the error to a field.

My take is that neither this PR (nor the existing add_error method) are a violation of the graphql specs, but @rmosolgo is the final arbiter in this.

[cjoudrey]

Here's a related issue in the reference implementation: graphql/graphql-js#205

[rmosolgo]

Hmm, I suppose graphql-ruby already violates the spec in two ways:

If an error is thrown while resolving a field, it should be treated as though the field returned null...

In fact, graphql-ruby allows adding errors via add_error and still returning a response. (add_error was added for this very purpose a long time ago)

That is, only one error should be added to the errors list per field.

As @stanishev points out, this is already possible via multiple calls to add_error.

So, I don't think this PR makes the violation any worse 😆 And of course, any user who doesn't wish to violate the spec in this way can simply use raise to ensure that:

• only one error field; AND
• fields with errors return null.