Client controlled nullability: inconsistency of assertions?

[benjie]

@twof Now that CCN doesn't simply change the nullability of a field, the intent of how certain cases should be handled is less clear to me. The current spec edits state:

In addition to being Non-Nullable, if a field marked with ! resolves to null, it propagates to the nearest parent field marked with a ? or to data if there is not a parent marked with a ?. An error is added to the errors array identical to if the field had been Non-Nullable in the schema.

Consider the following:

type Query { a: A }
type A { b: B }
type B { c: Int }

query {
  a? {
    b {
      c!
    }
  }
}

Should a.b.c be null, then the result should be {a: null} due to the throw/catch nature of ! / ?.

However, if a.b is null the spec seems to indicate that {a:{b: null}} acceptable even though a.b.c is not present. It seems confusing that if this is acceptable, why is it not also an acceptable solution to the situation where a.b.c alone was null?

Previously we've discussed the idea that ! might make all field from the nearest ? (or the root) to the ! field non-nullable also - i.e. ! would guarantee that if the container (? field, or root) is present, then the full path exists, gaining consistency. Maybe this would be a good solution to this seeming inconsistency?

Personally, I'd be happy if ! and ? went back to just setting the effective nullability of the field and having no further behaviours associated.

[twof]

@benjie I think you're right. If a query author doesn't believe their feature works when c is missing, then they should also reject responses with missing b since that means c would also be missing.

@captbaritone @josephsavona does it still work with Relay's fragment isolation plan if all fields between ! and ? are non-null?

[LunaticMuch]

I've lost a bit of context

It's a key point. I am late at this party 🙇🏻 so reading it with a fresh perspective I end with:

1. it feels a bit overengineering a problem which is not 100% crystal clear (or it's party included in this section from Relay)
2. while a CCN can have some interesting benefits, why shall it be also include additional behaviours (clearly odd in some cases?)

A lot in GraphQL is with the contract and setting the contract right. Adding CCN with additional behaviours seems to invalidate this basic principle.

[AnthonyMDev]

BestFriendProfileComponent might not know how to render without a name. Since this is an error (according to the business logic of BestFriendProfileComponent), bestfriend should not be nulled out. Instead, this error should bubble up to the nearest error-handling boundary (me), and we could display an error fallback in place of a User Profile.
However, it's perfectly valid for a user to not have a best friend.

I agree with @benjie. This seems to lead to unintuitive situations in a lot of cases. I'm strongly in favor of errors only bubbling up to the next nullable parent (whether that parent is nullable in the schema or by use of ?).

@mofeiZ It feels like what you want here is too closely tied to client specific behaviors, but it feels like it would make CCN become a lot more than CCN (as @LunaticMuch) mentioned. Making the ? operator an explicit error boundary makes CCN a lot more complicated to implement and for client developers to use and reason about.

---

Please let me know if I'm misunderstanding something about what you're goals are here in this example.

In your example, I'm not convinced that making the user object null is actually the best case scenario either. Ideally, you would likely still want to get the user's avatar and only display an error where the bestfriend component would be (instead of for the entire user profile).

I think the struggle here is that the proposed response data does not give you a good understanding of why the bestfriend field is null.

bestfriend could be null because:

1. The user has no best friend
2. The required name! field was null, so the best friend object was invalid.

While there is no indication of which of these is the cause at the site of the bestfriend field, that information can still be obtained by looking for the CCN error for the name field.

This isn't always easy to do though.

Firstly, view components usually don't have a reference to the entire GraphQL request and it's errors, so you'd need to build something to pass that around, this could get messy and detract from code quality. Additionally, if there are many fields that have ! CCN annotations, you would need to look through all of the errors and see if any of them indicate that the bestfriend field should be null due to an null child field.

---

Thinking through what the client would need to do to glean this information does make me think there is something missing here still. However I'm not convinced that making the ? the explicit error boundary is the right choice. I am more inclined to look for how we can better indicate the necessary information to the client when a CCN error occurs.

Overall, the issue seems to be more about being able to understand why the bestfriend field is null.

Even though this may be a bit messy, I would argue that this is a concern of the client. All of the information necessary to deduce if this null object is really null or due to a CCN error is provided in the response errors. Constructing some client-side infrastructure that parses the errors and then propagates information about them to the consuming models/components is definitely possible.

[AnthonyMDev]

After reading back and forth here, I think I largely agree with the arguments that @benjie has made, and I'm leaning towards making everything between an assertion and a catch operator a required field.

@twof Maybe I'm missing something, but it sounds like that is the opposite of what @benjie is in favor of?

[twof]

@AnthonyMDev Yeah you're right. You can read more of Benjie's thoughts here. graphql/nullability-wg#2 (comment)

We're going to be discussing this at the CCN WG meeting on the 26th, and again at the main WG meeting on August 3rd if you'd like to be a part of those discussions. https://github.com/graphql/client-controlled-nullability-wg/blob/main/agendas/2023/2023-07-26.md

[AnthonyMDev]

I'll be at both meetings :)

[IvanGoncharov]

I agree with @benjie that the semantics of error boundaries is strange.
As I understand it originated from the relay's @required directives and was boosted by the observation that in the absolute majority of cases clients just used the THROW argument.
https://relay.dev/docs/guides/required-directive/#throw-unrecoverable

My point is that Relay's @required(action: THROW) makes a lot of sense since it uses existing react error boundaries that will handle any exception and not only GraphQL ones.
That behavior exactly matches the business use case suggested here #1009 (reply in thread)

However, I don't think we should create parallel error boundaries (parallel to client code, e.g. react error boundaries) in GraphQL itself I think correct relay behavior is optimal and we should preserve it.

[AnthonyMDev]

Yes, there is no reason why adoption of CCN into the spec prevents Relay from continuing to use the @required directive, right?

My understanding is that in the original proposal, the ? operator really only became an error boundary as a welcome side-effect of the error boundary being the next nullable field, and that could make a field nullable.

If the ? must be used for an error boundary to exist at all, then this really is much more than just "CCN". You would need to use the ? operator even if the field you wanted to be the error boundary is already nullable.

If we are going to explicitly include syntax for setting error boundaries, I'd suggest that we explore options for different syntax. Because it feels like we are overloading the functionality of the ? operator. I would say we could start proposing syntax for defining an explicit error boundary as an additional RFC. It doesn't seem necessary that we include that functionality for CCN to be implemented.

[benjie]

I wrote my concerns up in more detail here: graphql/nullability-wg#2 (comment)