Feature request: Resilient Parsing

[BenSchwab]

Feature Request: Resilient Parsing

The graphql spec introduces the concept of error bubbling, where partial errors are allowed by bubbling up to the first element that is nullable.

Protect against backwards incompatible changes

If a server changes a field from being non-null to nullable existing clients that have already been shipped would completely break, even if the client appropriately handles nullability at an object higher up in the response.

Implementation

When parsing object types catch errors and replace the parsed value with null. This achieves a bubble effect where the error will propagate up the parse chain until we hit a nullable object.

When parsing lists a null value is read, if the list is of non-null types the entire list should resolve to null and bubble up the error.

override fun <T : Any> readObject(field: ResponseField, objectReader: ResponseReader.ObjectReader<T>): T? {
    if (shouldSkip(field)) {
      return null
    }
    val value: R? = fieldValueResolver.valueFor(recordSet, field)
    checkValue(field, value)
    willResolve(field, value)
    resolveDelegate.willResolveObject(field, value)
    val parsedValue: T?
    parsedValue = if (value == null) {
      resolveDelegate.didResolveNull()
      null
    } else {
      // The read is wrapped in a try catch, and we try to substitute null.
      // If the field is non-null, it will just throw a new error when checkValue
      // is called
     * **try {
        objectReader.read(RealResponseReader(operationVariables, errorCollector, value, fieldValueResolver, scalarTypeAdapters, resolveDelegate))
      } catch (exception: Exception) {
        errorCollector.errors += exception
        null
      }*
    }
    resolveDelegate.didResolveObject(field, value)
    didResolve(field)
    return parsedValue
  }

We could consider doing the same thing for custom scalar types, which would allow faulty custom scalar logic to resolve to null for a nullable custom scalar type.

Errors are collected in a partial error response object on the root operation, next to the standard graphql errors:

data class Response<T>(
       val operation: Operation<*, *, *>,
        val data: T?, 
        val errors: List<Error>? = null,
        // A new field containing errors that occurred during parsing, but may be recoverable.
        val parseErrors: List<ParseError>? = null

The ParseError object should form a chain of failures, if the error included multiple bubbled up response. Like the standard Error, this ParseError should include in a location in the query where the failure occurred.

In addition to surfacing the errors at the RootResponse we should be able opt-into generating an errors property on any object element in the response via a generateErrorAccessor directive. This seems possible by having the ErrorCollector track an “ongoing” error object, that “resolves” when the response reader is able to set a value to null.

Examples:

Simple:

Data? -------------                           
        scalar1                        
        object1? -------                        
                scalar2 <--- parsing error here                   

• object1 resolves to null

• error present in data

Object bubble:

Data? -------------                           
        scalar1                        
        object1? -------                        
                object2 -------- 
                        scalar2 <---parsing error here
        

* object 2 resolves to null causing error to bubble to object 1 which successfully resolves to null
* error present in data

Non-null List:

Data? -------------                           
        scalar1                        
        object1? -------                        
                scalar2                 
        list<T>? ------
                object2 <------ parsing error here
                object3 

List resolves to null

Nullable list

Data? -------------                           
        scalar1                        
        object1? -------                        
                scalar2                 
        list<T?>? ------
                object2 <------ parsing error here
                object3 

List contains null element for object 2

Other uses: Allow client “required” directive

An often requested client directive is the ability to mark a field on the client as “required” transforming it from nullable to non-null. However, as in the backwards incompatible case, if this requirement fails, it would be ideal to allow to response to still partially succeed.

[martinbonnin]

Thanks for the detailed feature request!

A few early thoughts:

• My understanding of the spec is that the bubbling implementation is meant for the server, not the clients? Doing it on the client feels like a spec extension, should that be opt-in if we end up doing it?
• Does this has cache implications? I guess we'll treat that as server errors and not cache them so maybe it's not too bad.
• generateErrorAccessor will also have to account for name clashes. Maybe it's enough to have the path in the root error?

Also, I've been thinking about changing the way we parse the json. We currently parse in 2 steps, first to a Map and then to the actual generated models. I've been thinking about changing that to a 1-step process like Moshi. That could have benefits such as performance and more type safety. Maybe it'll make sense to do this before adding more parsing logic. I'll open a separate issue to discuss this.

[BenSchwab]

Thanks for the questions!

My understanding of the spec is that the bubbling implementation is meant for the server, not the clients? Doing it on the client feels like a spec extension, should that be opt-in if we end up doing it?

Correct, the error bubbling spec I linked is for the server. But I feel like this part of the client is an implementation choice, and it seems like giving developers the ability to leverage GraphQLs nullability/error system (which the server spec demonstrates there is a sane model for) to have more resilient old clients seems great. We could make this behavior opt in / opt out.

Does this has cache implications?

Yes. I have been playing around with an implementation and it needs to make sure the normalized cache stays consistent. But it seems reasonable to just put a null in the cache during a parsing error.

generateErrorAccessor will also have to account for name clashes. Maybe it's enough to have the path in the root error?

Good point - I think with a specific enough name apolloParseErrors or something the chance of a clash is a small, and if there is one I feel like it would not be terrible for the compiler to through an exception and say that if you select a field named apolloParseErrors you need to alias it to something else.

I would be okay starting off with just the root errors, but our hypothesis is that for some large aggregate queries, it will be helpful to know at any arbitrary point if there were parsing errors, and I can think of a good design for the root errors that is not stringly typed.

[BenSchwab]

Separately, I would love to see us switch to a streaming parsing model. I do believe it would make a meaningful difference in parsing performance.

[martinbonnin]

Follow up issue there for the streaming parser: #2523

[martinbonnin]

Anyone reading this might be interested in @catch and @nullOnlyOnError support (You can read the current pull request here and associated documentation here.)

@catch allows catching GraphQL errors at any point in a query. Writing this I realize that it does not address the initial issue of non-null => nullable schema change because @catch only handles GraphQL errors.

But it could be modified to handle parsing errors (such as NullInNonNullPosition) if needed.