Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: SemanticNonNull type (null only on error) #1065

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

benjie
Copy link
Member

@benjie benjie commented Nov 24, 2023

TL;DR: Introduces a new type wrapper, Semantic-Non-Null, which represents that a value will not be null unless an error happens, and if an error does happen then this null does not bubble.

The problem

GraphQL schema designers must use non-nullable types sparingly because if a non-nullable type were to raise an error then the entire selection set it is within will be destroyed, leading to clients receiving less usable data and making writing the results to a normalized cache a dangerous action. Because of this, nullable-by-default is a best practice in GraphQL, and non-null type wrappers should only be used for fields that the schema designer is confident will never raise an error - not just in the current schema, but in all future schemas.

Many GraphQL consumers choose to ignore the entire response from the server when any error happens, one reason for this is because the null bubbling behavior makes writing to normalized caches dangerous. For these users, when an error doesn't happen, the nullable fields they are dealing with can be frustrating because their type generation requires them to handle the null case even if it may never happen in practice, which can lead to a lot of unnecessary code that will never execute. There is currently no way for the type generators to know that a field will never be null unless there's an associated error.

The solution

We can categorise that there are effectively two types of null:

  • Error null: where a position is null and there's a related error (with matching or prefixed path) in the errors list - indicates that something went wrong.
  • Semantic null: where a position is null and there is no related error - this data truly is null (e.g. a user having not yet set their avatar may have avatar: null; this is not an error).

This PR introduces a new wrapper type in addition to List and Non-Null, called Semantic-Non-Null. The Semantic-Non-Null type indicates that the field will never be a semantic null - it will not be null in the normal course of business, but can be null only if accompanied by an error in the errors list (i.e. an "error null"). Thus a client that throws out all responses with errors will never see a null in this position. Also, critically, any null raised by this field will not bubble and thus if an error is found with the exact path to this null then it is safe to store the result (including the error) into a normalized cache.

In SDL the Semantic-Non-Null wrapper is currently represented by a ! prefix (as opposed to the ! suffix for a strict Non-Null).

Thus we have the following:

# Type description Syntax Result values
1 Unadorned String String string, or error null, or semantic null
2 Semantic-Non-Null String !String string, or error null
3 (Strict-)Non-Null String String! string

Note that 1 and 3 above are exactly the same as in the current GraphQL specification, this PR introduces 2 which sits in the middle.

Backwards compatibility

All existing schemas are automatically supported because the meaning of String and String! is unchanged.

To ensure that all existing clients are automatically supported, this PR introduces the includeSemanticNonNull argument on __Field.type which defaults to false. Clients that do not pass includeSemanticNonNull: true will see all Semantic-Non-Null types stripped, which will have the effect of making them appear as if they were the unadorned types. This is safe, since it means these clients will need to handle both error nulls and semantic nulls (as they traditionally would have) even though we know that a semantic null will never happen in practice.

All existing GraphQL documentation, tutorials, examples, and everything else we've built over the last 8 years remains valid since the meaning of String and String! are unchanged.

History

This PR is almost identical to #1048, but it changes the name of the new type wrapper from Null-Only-On-Error to Semantic-Non-Null, and changes the syntax from String* to !String. It addresses the True Nullability Schema discussion raised by @captbaritone and incorporates/adapts some of the terminology from @leebyron's Strict Semantic Nullability proposal.

Copy link

netlify bot commented Nov 24, 2023

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit bd038f2
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/6595e04ffa373d0008cbff71
😎 Deploy Preview https://deploy-preview-1065--graphql-spec-draft.netlify.app/draft
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

facebook-github-bot pushed a commit to facebook/relay that referenced this pull request Feb 5, 2024
Summary:
This PR adds experimental support for `semanticNonNull` as described in apollographql/specs#42. Which is part of a broader effort to explore semantic nullability in GraphQL as explored in [RFC: SemanticNonNull type (null only on error) ](graphql/graphql-spec#1065). This directive-based approach should allow us to experiment with the concepts and identify issues as we work to understand the viability of semantic nullability in GraphQL.

## Experimental

As this is still an experimental implementation, it's designed to be minimally invasive rather than ideal in terms of architecture or performance. As the feature/RFCs stabilize I would imagine we would bake this into the schema crate and data structures as a first class concept.

This flag will not be broadly safe to enable in Relay since by default fields that are null due to error are still surfaced to the user. This is only safe to enable if:

1. Your network layer discards all payloads that have any field errors
2. You enable our [explicit error handling feature](#4416), which is still itself experimental.

## Missing Pieces

- [ ] Documentation about how to use this feature (purposeful, since this is experimental)
- [ ] Support for `semanticNonNullField` which allows a patching an existing type to define it's field's semantic nullability
- [ ] Validation
  - [ ] Invalid use of `levels` will simply panic.
  - [ ] Uses of `semanticNonNullField` will simply be ignored
  - [ ] There is no schema validation ensuring interface types have type-compatible semantic nullability

Pull Request resolved: #4601

Test Plan:
```
cargo test
```

I also spun up a version of [`grats-relay-example`](https://github.com/captbaritone/grats-relay-example) using Grat's [experimental support for `semanticNonNull`](https://grats.capt.dev/docs/guides/strict-semantic-nullability/) and was able to see it working end to end.

https://github.com/facebook/relay/assets/162735/dc979a58-95f3-4e55-9d9b-577afdd798ca

Reviewed By: alunyov

Differential Revision: D53191255

Pulled By: captbaritone

fbshipit-source-id: c09333f2b9475315d81792d33947fd908001c021
@captbaritone
Copy link

One outstanding question regarding this approach is "will we be able to find an additive syntax to denote 'null only on error' that is palatable?".

Some folks from the Nullability Working Group met in an ad-hoc meeting to brainstorm the options available and itentify viable syntax candidates. (Meeting notes can be found here under the heading "Ad-hoc discussion on syntax")

We arrived at a top three which seemed to pull away from the rest as clear leaders. ~! is the favorite of the group, but all three are worth sharing with a larger group. All three have a basis in written English in which the additive symbol qualifies the ! in some way.

The three leaders, including the properties that make them more or less desirable:

  • ~!
    • Interpreted as “approximately not null”
    • Might be slightly preferable during lexing (no need to look ahead) but we already do look ahead, so not a meaningful difference
    • ~ is used as “bitwise not” in some languages which may imply a type of negation (likely not a practical issue since most GraphQL adjacent code is likely not doing bit bashing)
  • !*
    • Interpreted as “not null, with some caveat”
    • Visually too heavy in monospace
  • !^
    • Interpreted as “not null, with some caveat”
    • Harder to say aloud
    • Name may be unclear/ambiguous: “Bang carrot” or “Bang circumflex”
    • Often seen as a secondary “footnote” character to be used after * has already been taken. That said, commonly used for footnotes in Markdown

@bignimbus
Copy link
Contributor

bignimbus commented Feb 9, 2024

These are all really thoughtful. The only thing I have to add is that !^ reminds me of bitwise XOR, which feels conceptually adjacent to semantic non-null in a way that I can't fully express on a Friday afternoon.

@michaelstaib
Copy link
Member

Also "Bang carrot" sounds kind of great :D

@glen-84
Copy link

glen-84 commented Feb 11, 2024

friends: [User~!]~!

You won't make any friends with syntax like that. 🙃

Adding a multi-character affix would be a big mistake, in my opinion.

With @leebyron's proposal, this would simply be friends: [User], right? How can that not be the goal?

@benjie
Copy link
Member Author

benjie commented Feb 12, 2024

With Lee's proposal, this would simply be friends: [User], right?

Along with schema @strictNullablility, yes.

How can that not be the goal?

There's a number of reasons why using the unadorned type to represent a semantically non-null type can not be the goal; here's a few:

  1. The unadorned type currently means nullable. Changing its meaning (even if that's correlated with a directive added to the schema) is a HUGE shift:
    • Every single SDL file that exists currently may have it's meaning changed if somewhere in the pipeline the schema gains a @strictNullability directive
      • When people add @strictNullability to their schema declaration they would need to change every single Foo type in their SDL to be Foo? in order to be safe; this is a huge shift and something very likely to have fields missed due to merge conflicts/etc as a codebase continues to evolve whilst this transition is going on
      • If they miss this transition in one place, that place is now forever semantically non-nullable because transitioning back to "nullable" is a breaking change.
    • Every piece of documentation that the community has written in the last 9 years which says the unadorned type is nullable will need to be revised (a frankly impossible task)
  2. The default behavior should be the most forward-compatible behavior.
    • Moving from nullable to non-nullable for output types is non-breaking, so starting with User (nullable) and later changing to User! (non-nullable) is an easy and safe transition.
    • Moving from non-nullable to semantically-non-nullable is not breaking. Moving from semantically-non-nullable to strict non-nullable is not breaking.
    • With Lee's proposal, the unadorned type now means "semantically non-nullable" (but only if the @strictNullability directive is present). If you realise that this is in fact a semantically nullable field, you need to change the type to User? which is a breaking change.
    • We should make it as easy as possible for people to write and maintain schemas in a versionless way, and defaulting to types that can be iterated is safer and, I'd argue, better for the ecosystem.
    • In this proposal (RFC: SemanticNonNull type (null only on error) #1065) the unadorned type is the most forwards-compatible type - you can move to semantically non-nullable or strict-non-nullable in a non-breaking way
  3. Having User mean two almost opposite things, "nullable" and "semantically non-nullable", depending on the presence of a directive which could be in a completely different folder of the project seems hugely confusing and undesirable.
    • In my opinion, this will increase the complexity of GraphQL and make it much harder for people to learn, reducing adoption

My personal opinion is that Type and Type! should continue to mean what they have always meant, and the new type we're introducing, the semantically-non-nullable type, should get a different syntax. So far, Type~! is the preferred syntax for this, but if you have a better suggestion please post it here.

@glen-84
Copy link

glen-84 commented Feb 12, 2024

The unadorned type currently means nullable.

And that's a mistake IMO.

  • There are usually more non-nullable fields in a schema than nullable fields.
    • Why not make the most common case the simplest to write?
  • This is completely at odds with numerous other languages like TypeScript, C#, Kotlin, and Swift.
    • Why invent different (and downright ugly) syntax when many developers are already familiar with using ! (not-null) and ? (nullable)?

Changing its meaning (even if that's correlated with a directive added to the schema) is a HUGE shift

... in the right direction.

When people add @strictNullability to their schema declaration they would need to change every single Foo type in their SDL to be Foo? in order to be safe; this is a huge shift and something very likely to have fields missed due to merge conflicts/etc as a codebase continues to evolve whilst this transition is going on

This is covered in Lee's proposal ("How to adopt this incrementally?"). You don't enable @strictNullability until you're done adding the semantically-nullable modifier (?).

If they miss this transition in one place, that place is now forever semantically non-nullable because transitioning back to "nullable" is a breaking change.

Then they best be careful. 🙂

Every piece of documentation that the community has written in the last 9 years which says the unadorned type is nullable will need to be revised (a frankly impossible task)

So nothing can ever be improved because people have written about GraphQL before? The language will become progressively worse while trying to achieve infinite BC. Is versioning really an impossibility here?

Moving from nullable to non-nullable for output types is non-breaking, so starting with User (nullable) and later changing to User! (non-nullable) is an easy and safe transition.

It means a client can continue to use optional chaining, sure, but how does the API suddenly start returning data that doesn't exist, if the field was previously nullable?

If you realise that this is in fact a semantically nullable field, you need to change the type to User? which is a breaking change.

This is a migration concern. It's an issue once.

We should make it as easy as possible for people to write and maintain schemas in a versionless way, and defaulting to types that can be iterated is safer and, I'd argue, better for the ecosystem.

Part of the reason for these proposals is clients having to write code like a?.b?.c?.d – if nullable is the default, that just means more of this?

In this proposal (RFC: SemanticNonNull type (null only on error) #1065) the unadorned type is the most forwards-compatible type - you can move to semantically non-nullable or strict-non-nullable in a non-breaking way

Again, I'm curious how this works in practice, and more importantly, how often it happens. What data do you return for a field that has been changed to non-nullable? A confusing default like example@example.com? An empty string?

Having User mean two almost opposite things, "nullable" and "semantically non-nullable", depending on the presence of a directive which could be in a completely different folder of the project seems hugely confusing and undesirable.

You're thinking of a scenario where the two coexist. You do the migration once, and align your mental model with many other languages, then you forget about the inconsistent syntax that GraphQL currently has (in relation to these languages and common conventions).

Also, the behaviour might be toggled via a setting, and not necessarily via a schema directive hidden deep inside your project.

In my opinion, this will increase the complexity of GraphQL and make it much harder for people to learn, reducing adoption

To the contrary, something like friends: [User~!]~! (aka ASCII art) is more likely to alienate developers. If anything, aligning the syntax with common language conventions (which are far cleaner and more intuitive) is more likely to do the opposite.

My personal opinion is that Type and Type! should continue to mean what they have always meant, and the new type we're introducing, the semantically-non-nullable type, should get a different syntax.

This is adding more cognitive overhead – instead of just picking between nullable and non-nullable, we have to consider "semantically non-nullable" as well. How will that be less confusing?

So far, Type~! is the preferred syntax for this, but if you have a better suggestion please post it here.

Just Don't Do It™

It will make by-hand schema authoring a confusing mess. Multi-character affixes are awful. There exists a convention used commonly – adopt that, and align GraphQL with a common way of thinking. Accept the short-term pain for longer-term simplicity.

If anything, involve the community – poll developers on:

  • friends: [User] (nullable) and friends: [User~!]~! (non-nullable) [with GraphQL-specific syntax], vs
  • friends: [User] (non-nullable) and friends: [User?]? (nullable) [and alignment with common languages and conventions].

@tobias-tengler
Copy link

tobias-tengler commented Feb 12, 2024

@benjie I see your point regarding backwards compatibility and already produced GraphQL content.
I have to agree with @glen-84 though. If we make changes to this, it should make things easier long-term.

? makes sense to every developer and having the shortest syntax for the most common case (null-only-on-error) is also nice.

type User {
  name: String
  communityName: String?
}

The above is easily understood without knowing the historic context.

type User {
  id: ID!
  name: String~!
  communityName: String
}

This is not.

The introduction of the String? and the changed meaning of String wouldn't break existing consumers, since null-on-error is already handled by the type being nullable previously. The only breaking change would be String! to String, but this would also be the case with String! to String~!.

Similarly to how String~! would return String for old introspection queries, String? could also return String.

Besides the point that learning material would be outdated, I don't see a real reason why ~! should be preferred over ?. The outdated info that String means nullable wouldn't cause any harm, because even if you write queries with this assumption, you're handling the null-on-error case.

For me this would be an okay trade-off for an easier understood nullable syntax and shorter syntax for the common case. Contrary to you I think the new syntax would make adoption easier. For existing projects it's an optional, one time schema migration and for new projects you get an easier understood syntax out-of-the-box.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants