Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Field produces both value and error #769

Open
tomgatzgates opened this issue Aug 27, 2020 · 5 comments
Open

Field produces both value and error #769

tomgatzgates opened this issue Aug 27, 2020 · 5 comments

Comments

@tomgatzgates
Copy link

I'm looking to clarify whether it should be valid for a field to be resolved with both a value and an error in the global errors list.

This scenario came up when resolving a field to an external service. In the event that the external service is down, the server would like to provide a sensible default response whilst still notifying the client that the value is a fallback value.

Its technical possible to achive but I'm wondering whether it is/should be valid as per the spec?

cc @eapache

@eapache
Copy link
Member

eapache commented Aug 27, 2020

The motivating use case makes sense to me.

The relevant line in the spec I see is

If an error is thrown while resolving a field, it should be treated as though the field returned null, and an error must be added to the "errors" list in the response.

from http://spec.graphql.org/draft/#sec-Errors-and-Non-Nullability. Depending on how you read this though, it could only refer to unexpected errors (exceptions, not flow-control-errors).

Also, http://spec.graphql.org/draft/#sec-Errors which deals with these kinds of validations more directly doesn't really mention it. The closest reference is

If an error can be associated to a particular field in the GraphQL result, it must contain an entry with the key path that details the path of the response field which experienced the error. This allows clients to identify whether a null result is intentional or caused by a runtime error.

But it doesn't require the result to be null.

@benjie
Copy link
Member

benjie commented Aug 28, 2020

I'm certain that the first link above is stating that if a field throws an error, the field must resolve as if the resolver returned null (i.e. it should be null, or if the field is non-nullable it should cascade to the first nullable parent field). If the field resolves to a non-null value, then it follows that no error can have been thrown, and thus nothing added to the "errors" list for that field. i.e. I don't believe this is allowed as per the current GraphQL specification.

The solution to this is to instead indicate the error via the schema itself (rather than via the "errors" list), by modelling the schema in such a way that this event can be indicated via the output of regular types/fields.

@andimarek
Copy link
Contributor

It would be good to get a clarification of the original intent (cc @leebyron @dschafer maybe?), even if I agree with @benjie take that it probably means error and non null data is not allowed together.

In GraphQL Java we actually allow data and error to be mixed, but not sure how often this is really used.

@bbakerman
Copy link

The GraphQL Java has a discussion on this and we have strong evidence (from teams asking for it) that being able to (sometimes) return errors and data is useful.

We do have a path where if a field resolver (we call it a DataFetcher) throws an exception then this is exceptionally handled and the data will be null.

However we also allow a resolver to return a compound object DataFetcherResult that allows both data and an error in a success case.

That is we have an "exceptional" path and a "successful path" in place

The use case we have is around clients that "understand" the nature of errors via "error extensions"

eg imagine an error like

{
   message : "The foo system is down right now - a default value has been provided"
   extensions : {
     statusCode : 503
  }
}

In the case above the the client has been given a default value AND it could warn the end user that things are unstable and they might want to try again or treat it carefully.

If the field resolves to a non-null value, then it follows that no error can have been thrown

We are argue that this is not true - that it does not follow that no error can be present.

While the most common situation might be that "exceptions" in resolvers result in null field data and an error, we don't think the specification should encode that and say its always required to be true.

@leebyron
Copy link
Collaborator

leebyron commented May 10, 2023

If an error is thrown while resolving a field, it should be treated as though the field returned null, and an error must be added to the "errors" list in the response.

I agree that this is ambiguous. We need to address this clause to be more clear. In particular I don't like "should be treated as though the field returned null" - this is a bit confusing since what that section is trying to articulate is the error propagation behavior and not a "return" behavior.

Adding some more detail on original intent:

"Error" handling as written in the spec does in fact refer to "exceptional" behavior and is distinct from a field normally producing a value (null or otherwise). Errors are then handled using this defined propagation, which as quoted requires producing null in the response at some point, so a section of the response can be removed to provide a partial response.

The spec had some recent edits to attempt to make this a bit more clear in that it does not say return, or produce an error, but instead "raise an error - the raise term is there to disambiguate that errors are not part of normal value generation and trigger this exceptional behavior.

If the field resolves to a non-null value, then it follows that no error can have been thrown

With the assumption that the only specification defined mechanism for adding to the errors list (during execution) is a field error, then this logic is sound. An error in the list implies that field raise that error which means the field error handling logic applied and either that response field itself must be null, or it propagated in which case one of the parent response fields is null.


Where to go from here?

  1. We should make it more clear how field error raising and handling works!

  2. Aside from that, there is in fact unstated ambiguity about where errors in the errors lists can come from and what assumptions can be made about them. We should resolve this as well.

  3. There are clear examples of a need for more control over both field exceptional behavior and errors collection. @bbakerman highlighted some key ones above, and we've had a (now stale, unfortunately) proposal about client-side error handling in the query text ("client controlled nullability") - we should explore more proposals for extended behavior for the WG to consider - either specifically scoped to "default value in the case of error" scenario described above, or a more general expansion of what is allowed to be placed in the errors list

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants