Handle partial data and error paths

[hugo-vrijswijk]

This is a complicated change, and open to discussion if it is actually wanted.

Failed fields now propagate their null to the nearest nullable position, rather than a Result.Failure which discards all data. Example:

Before:

query {
  ping
  viaEffect {
    name
  }
}
# result:
{
  "errors": [
    { "message": "boom" }
  ]
}

After:

query {
  ping
  viaEffect {
    name
  }
}
# result:
{
  "data": {
    "ping": "pong",
    "viaEffect": null
  },
  "errors": [
    { "message": "boom", "path": ["viaEffect"] }
  ]
}
# or, if all fields are non-nullable:
{
  "data": null,
  "errors": [
    { "message": "boom", "path": ["viaEffect"] }
  ]
}

More examples are in the tests, which show the difference clearly. This is a big breaking change for users that expect data to be complete if there is data at the root. Release notes should mention this very clearly. But it is in line with the GraphQL specification, which says that a failed field should not discard its siblings' data. This partially closes a few conformance suites, except for locations in errors, which are not (yet) implemented.

---

The interpreter now tracks the response position of each value:

• A failure at a nullable position completes as null and keeps the data of its siblings.
• A failure at a non-null position propagates its null to the nearest enclosing nullable position.
• A null which reaches the root leaves data as null.

Each problem carries the response path of its own position, with the alias of the field and the index of the list entry.

Both error policies now complete the deferred positions of a failed batch as null and keep the rest of the response. An internal error stays a request error and aborts the completion.

BREAKING: Problem.path changes type from List[String] to List[Problem.PathSegment], so that a path can hold list indexes. A segment is a Name(String) or an Index(Int).

[milessabin]

I think the spec is open to interpretation here. The relevant sections are 6.3.3, 6.4.4, 7.1, esp. 7.1.6 and example 209. I can't see explicit text there which says that a conforming implementation MUST return a partial result, I see more of an assumption that it typically will. I think that fits with the execution model of the Javascript reference implementation, where every field is backed by it's own effectful "resolver" each of which could fail independently. That said, I read the spec as saying that if a partial result is returned, then it MUST propagate nulls up to nullable positions in the way described.

I think on balance I'm comfortable with this. Grackle's execution model is different, in as much as typically if the base mapping fails (eg. due to DB query failure) that will typically be sufficiently catastrophic that a partial result won't be possible. But I think it should be possible to have the failure of an isolated effect handler nested at the leaf of a query result in a partial result being returned. In fact, I think I had assumed that this was already at least partly possible because a failing effect handler can already return a null result with problems attached ... the missing part would be the upwards null propagation.

I'm not too concerned about "users that expect data to be complete if there is data at the root" ... thanks to the null propagation to a nullable field, the result is always schema-valid, so the client should be able to handle results of that form.

Would it be practical to make it possible to opt in to a "no partial failures" mode which matches the current behaviour?

[hugo-vrijswijk]

I agree part of the spec seems written from a somewhat implementation-oriented perspective. Which doesn't help. But I don't know how open to interpretation it is. From 6.3.3:

If during ExecuteCollectedFields() a response position with a non-null type raises an execution error then that error must propagate to the parent response position, either resolving to null if allowed or being further propagated to a parent response position.

ExecuteCollectedFields can be read as completing the batches of effects in completeAll in Grackle. It specifically says it must propagate to the parent response position (or to its parent).

In 6.4.4:

An execution error is an error raised during field execution, value resolution or coercion, at a specific response position. While these errors must be reported in the response, they are “handled” by producing partial "data" in the response.
{...}
Since Non-Null response positions cannot be null, execution errors are propagated to be handled by the parent response position. If the parent response position may be null then it resolves to null, otherwise if it is a Non-Null type, the execution error is further propagated to its parent response position.

From 7.1.6:

An execution error is an error raised during the execution of a particular field which results in partial response data.
...
When an execution error is raised at a given response position, then that response position must not be present within the response "data" entry (except null), and the "errors" entry must include the error. Nested execution is halted and sibling execution attempts to continue, producing partial result (see Handling Execution Errors).

So execution errors (AKA during effect resolution) have to result in partial data, without halting sibling continuation (ErrorPolicy.Accumulate). And any errors have to bubble up to the nearest nullable parent field in the tree.

---

For Grackle, if the base mapping is a Skunk mapping and the database is down then the execution error appears at the base, so the base of the result is null ("data": null, or "data": { "user": null } if user is nullable). In the current release result has no data field at all, only errors. The ErrorPolicy only dictates if more effects are run and errors will be accumulated.

With this PR, if the base mapping has a Skunk field foo mapping, and an unrelated Circe mapping for another field bar, the database being down would still result in bar field being returned (if the skunk field is nullable). But root mapping errors result in errors and null at the root. Which is perfectly valid.

But I think it should be possible to have the failure of an isolated effect handler nested at the leaf of a query result in a partial result being returned

That's exactly the only thing this PR does 😁 (plus path in the error).

---

Would it be practical to make it possible to opt in to a "no partial failures" mode which matches the current behaviour?

It might be. The JavaScript GraphQL library has recently introduced an experimental directive @experimental_disableErrorPropagation which does exactly that (RFC here). We could adopt support for the directive too. That way, Grackle aligns with other GraphQL libraries. It seems cleaner to me in a request, instead of as a mapping-constant. Though another option on ErrorPolicy would prevent any mix-and-match possible error handling strategies. Unless you feel strongly about this I'd rather have this in a different PR, though. Since it moves behaviour away from the spec.

---

I know this is a long comment, for an already complicated PR, so I apologize 😅. But I think it is important to understand the impact and actually agree on if this is what Grackle is intended to do.

[milessabin]

Yes, I think you're right.

As you point out, there's ongoing discussion around controlling this behaviour. Even in we don't expose an experimental directive, would it make sense to include the plumbing to enable the choice now, rather than wait and retrofit it later?

FWIW, the discussions seem to be focussed on client controls on null propagation. It seems at least as reasonable to think about server controls (which I could imaging being implemented via a schema directive).

I must admit, my intuitions would be to favour a default "fail all, fail fast" semantics determined by the server, but then I'm a server implementor, not a client 😉

[hugo-vrijswijk]

I'm not sure what you mean by introducing the plumbing, if there's no way to control it. I must be missing something because it sounds like dead code 😅.

I think the error propagation comes from the idea of wanting GraphQL to have a "mega-query" that gets everything you need to render a page, rather than a million little requests. If you have that, different parts of a query aren't necessarily related to each other, so if one service is down most of the query will still work. For me most queries in GraphQL have been a single thing per query, so maybe I'm doing something wrong. Maybe I just need more microservices or something 🤷

[milessabin]

I'm not sure what you mean by introducing the plumbing, if there's no way to control it.

I'm imagining something along the lines of an additional argument to compileAndRun similar to introspectionLevel which could be threaded through to control the behaviour. If nothing else, this could be used by anyone who needs to preserve the existing behaviour.

But that isn't a blocker. I'm happy to approve this without it.

[milessabin]

LGTM!