Support partial results about caliban HOT 5 CLOSED

Relevant specs:
https://graphql.github.io/graphql-spec/June2018/#sec-Errors-and-Non-Nullability
https://graphql.github.io/graphql-spec/June2018/#sec-Response

Thanks for opening this discussion! It's a very interesting problem.

A few questions/remarks:

• Your PR makes all effectful fields that are not UIO nullable. Is that the correct default for mutations? I'm afraid this might be unexpected for users. If we apply that new behavior to queries only, the inconsistency might be unexpected too 🤔WDYT?
• More generally, some users might want to fail and not have partial failures, I think there should be an easy way to do that if we change the default behavior.
• If we do that, the same logic should be applied to ZQuery.
• If we want to support this in an explicit manner, is there a type we could use that would determine which way to use? Or maybe an annotation?
• I think this should be released only with a solution to accumulate errors, otherwise you wouldn't be able to distinguish between an error and a simple null field.

from caliban.

Is that the correct default for mutations?

I think yes, mutations should behave the same (it's not very clear from the spec, but I think other GraphQL implementations execute all mutations, even if some of them fail). Also, for mutations, it's not always obvious whether the failure means a failure to mutate, or failure to compute the result.

some users might want to fail and not have partial failures

Maybe, although for me it's hard to imagine the case when we have some data but want to intentionally throw it away. Anyway, I think the partial failure should be the default behavior, and it also follows the spec.

I think this should be released only with a solution to accumulate errors

Absolutely. I guess I should have created my PR as a draft.

from caliban.

I think most other libraries rely on the user-defined schema to decide whether to return null or to bubble up the failure (as defined by the spec), so they don't really have any choice to make. What's a bit different here is that we make the choice for the user based on their schema types.

I tend to agree your proposal is a good default, but I think it would be nicer if we let users fail fast if they want to, for example with a mutation like this:

mutation {
  deleteA
  deleteB
}

If deleteA fails and is nullable, deleteB will be ran. If deleteA fails and is not nullable, the error will bubble up and the whole mutation will return null without running deleteB. I can imagine both cases to be needed.

from caliban.

We had some discussions about it on Discord: https://discordapp.com/channels/629491597070827530/633200096393166868/646147482153254913 (feel free to join 😄)

One idea would be to use the Die channel for the "fail fast" case. User would simply return UIO (field would be non-nullable) and add .orDie to make their error interrupt the whole query.

I'll play with the Executor to see if I can achieve the proposed error handling. Will let you know how it goes.

from caliban.

After discussing with @adamgfraser, it seems that to support this feature we first need ZQuery to support partial failures as well (with something like foldM or catchAll in ZIO). He will have a look at that.

Once this is done, the executor will need to collect the errors, set NullValue for those fields and return (List[E], ResponseValue). I can take care of that part.

from caliban.