Nullability/error awareness improvements

[swizzlr]

Use case

I would like to have a similar feature set to Apollo Kotlin which is adopting semanticNonNull in order to more accurately describe the domain of null because nothing, null because of error, and null because of bubbled error. I would also like the solution to provide ergonomic access to field errors for existing nullable types

Describe the solution you'd like

• Improved accessors. One or a choice of the following (the choice could be configured through codegen or directive parameters):
  • for a semantically non-null field:
    • var field: String { get throws(GraphQLError) } or
    • var field: Result<String, GraphQLError> or
    • @SemanticNonNull var field: String? where @SemanticNonNull is a property wrapper providing a projected value $field that is of type GraphQLError?
  • for a semantically nullable field:
    • the above accessors, except with optionals where appropriate
• A way to opt-in to this behavior incrementally via schema directives. For consistency's sake, the Kotlin implementation seems like a sensible starting point: https://www.apollographql.com/docs/kotlin/advanced/nullability#introduction.

Given the spec is still in flux, dropping the server directive and simply offering something like _semanticNonNullField might be a good way to scare off uninitiated users who aren't following the workgroup discussions.

I understand Swift 6 is a big priority right now but at least having this on the roadmap would be a good start, I think!

[swizzlr]

Or perhaps

@SemanticNonNull var field: String? where @SemanticNonNull is a property wrapper providing a projected value $field that is of type Result<String?, GraphQLError

[calvincestari]

HI Thomas, thanks for creating the issue.

I understand Swift 6 is a big priority right now but at least having this on the roadmap would be a good start, I think!

Yes you're correct Swift 6 is the big priority, but has been on my mind to get something experimental into Apollo iOS that we can test and learn from to inform the direction of the proposals. I agree that getting visibility on the roadmap of our intention is a good idea. Client Controlled Nullability used to be available but was removed once it was determined the proposal wasn't going to advance.

Given the spec is still in flux, dropping the server directive and simply offering something like _semanticNonNullField might be a good way to scare off uninitiated users who aren't following the workgroup discussions.

All of this still has a reliance on a server implementation though because it needs to understand that sending null is possible/supported for a field instead of bubbling up and potentially blowing out the entire operation.

[swizzlr]

All of this still has a reliance on a server implementation though because it needs to understand that sending null is possible/supported for a field instead of bubbling up and potentially blowing out the entire operation.

Oh, I must be mistaken/not explaining myself correctly.

What I'm requesting here is that a server-defined nullable field can be declared by the client to be semantically non null, and therefore when the server returns a null in that field Apollo should make it easy for me to grab the error, if any, and type it in swift as a non-optional type. Obviously if the field is null and there is no field error then it's a programming error on my part. My understanding is that that server behavior is already part of the standard (returning nulls and using the errors array).

[calvincestari]

Thanks for clarifying, I better understand what you're asking for now.

What I'm requesting here is that a server-defined nullable field can be declared by the client to be semantically non null, and therefore when the server returns a null in that field Apollo should make it easy for me to grab the error, if any, and type it in swift as a non-optional type.

When I first read this I read it as just a simpler way to access the field error, instead of having to search through the field errors array and find the matching path, etc. Yes - this would be a nice improvement. Changing the type to non-optional I'm not so sure on, but I do like the throwing getter suggestion; that should work since we use DataDict as the backing store and don't actually have to store a non-optional value.

Obviously if the field is null and there is no field error then it's a programming error on my part. My understanding is that that server behavior is already part of the standard (returning nulls and using the errors array).

I'm not certain we can rely on this assumption though because I'm not 100% sure on the field error behaviour of nullable fields. The GraphQL spec makes specific mention of non-null types but nothing that states a field error must be raised for nullable types. I think you can infer that a field error should be raised but nothing to state that it must be. It would be good to clarify that behaviour with the working group.

[calvincestari]

Obviously if the field is null and there is no field error then it's a programming error on my part. My understanding is that that server behavior is already part of the standard (returning nulls and using the errors array).

I'm not certain we can rely on this assumption though because I'm not 100% sure on the field error behaviour of nullable fields. The GraphQL spec makes specific mention of non-null types but nothing that states a field error must be raised for nullable types. I think you can infer that a field error should be raised but nothing to state that it must be. It would be good to clarify that behaviour with the working group.

I've read and re-read this section of the spec countless times and while the language could be clearer I believe we're to assume that a field error is required in the "errors" list regardless of the type. I think that's going to be the common understanding of when a field error must be raised.

[swizzlr]

I've been thinking about this a ton over the past few weeks. My response got very long, so instead I decided to see about having a crack at implementation. I learned a lot.

Big picture: this issue I've filed here is way too broad. Improving error awareness and enhancing the handling of partial results is not something that can be solved overnight. The semantics of it are hard to pin down, as evidenced by the long road the nullability workgroup has walked.

What I'd like to do is close this issue and break it up into a few smaller ones, but I have one concrete action item that would provide the underlying infrastructure to support whatever eventually comes out of this.

Goal

As a user I would like to be able to convert the keypath of a SelectionSet-conforming data type to a GraphQL path.

In other words, I need some infrastructure in ApolloAPI that's the moral equivalent of:
func convert<T: SelectionSet, U>(keyPath: KeyPath<T, U>) -> [PathComponent]?
so that I can do:

// assume i've unwrapped data and errors from a GraphQLResult, let data and let errors
guard let field = data.entity.nullableField else {
    let nullableFieldPath: [GraphQLError.PathEntry] = convert(\MyOperation.Data.entity.nullableField)!
    let nullableFieldErrors = errors.filter { $0.path == nullableFieldPath }
    throw nullableFieldErrors.first ?? MyError.unknownError
}

- GraphQLError.PathEntry == PathComponent. Providing a PathComponent instead could be more generally useful, as long as this typealias is a stable public API - since U is unconstrained there are keypaths that aren't selections (e.g. `__data`), and we can't guarantee the conversion, hence optional return type. I greatly prefer these ergonomics.

Would you be up for me to make a ticket to track this? It would enable future work on error awareness.

[calvincestari]

Thanks for continuing to consider this @swizzlr.

Big picture: this issue I've filed here is way too broad.

We don't need to resolve it all at once though. There is place for a larger Nullability Task List where we carve out smaller pieces that can be done independently and incrementally to eventually get us to where we want to be.

Improving error awareness and enhancing the handling of partial results is not something that can be solved overnight. The semantics of it are hard to pin down, as evidenced by the long road the nullability workgroup has walked.

Exactly. The working group has been at this for a while now and gone through a few iterations. I think we're narrowing in on a workable proposal though.

As a user I would like to be able to convert the keypath of a SelectionSet-conforming data type to a GraphQL path

Give me some time to think through this a bit more..

[calvincestari]

// assume i've unwrapped data and errors from a GraphQLResult, let data and let errors
guard let field = data.entity.nullableField else {
    let nullableFieldPath: [GraphQLError.PathEntry] = convert(\MyOperation.Data.entity.nullableField)!
    let nullableFieldErrors = errors.filter { $0.path == nullableFieldPath }
    throw nullableFieldErrors.first ?? MyError.unknownError
}

Returning MyError.unknownError doesn't seem correct here as you'd never know whether null is the correct and valid value. Unless you're wanting to go directly to a faux-strict-non-null equivalent in your implementation?

As for the conversion func convert<T: SelectionSet, U>(keyPath: KeyPath<T, U>) -> [PathComponent]? I recall having some issues in getting to this at runtime which is why defer-based operations produce static metadata for field path so we don't have to pay this cost at runtime. It ends up generating code like this:

  enum DeferredFragmentIdentifiers {
    static let parent_deferred = DeferredFragmentIdentifier(label: "parent_deferred", fieldPath: ["allProducts"])
    static let child_deferred = DeferredFragmentIdentifier(label: "child_deferred", fieldPath: ["allProducts", "dimensions"])
  }

[swizzlr]

Unless you're wanting to go directly to a faux-strict-non-null equivalent in your implementation?

Basically, yes. Unfortunately that's a contract that has to be documented in the graph, until/if semanticNonNull lands in the spec. There are quite a few places already in our graph where this occurs, most notably in lists of entities. Because we don't usually want to lose everything in the list, just because one of those entities had a resolution error, so we leave them nullable, even though they should, for example always possess a name.

The code block you've included there is one way a user of this library could make their lives easier. Right now it's stringly-typed. If I had this primitive available to me I could tailor my API calls to the documented semantics of our graph. I certainly wouldn't think it's the right behavior in all cases, but it's definitely useful to me here, and I think it would probably be helpful to others.

[swizzlr]

I haven't been able to get this to happen at runtime with what I have. I started reaching for Mirror and gave up. If we could agree on a public API shape that could be shipped as an experimental opt in feature for the codegen, would you support a PR?