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

@semanticNonNull/@catch: default levels to [0] instead of null #42

Merged
merged 6 commits into from
Jan 29, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions __index__.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,6 @@
- **[kotlin_labs/v0.2](/kotlin_labs/v0.2)** ([📄 graphql](kotlin_labs/v0.2/kotlin_labs-v0.2.graphql))
- **[link/v1.0](/link/v1.0)** ([📄 graphql](link/v1.0/link-v1.0.graphql))
- **[nullability/v0.1](/nullability/v0.1)** ([📄 graphql](nullability/v0.1/nullability-v0.1.graphql))
- **[nullability/v0.2](/nullability/v0.2)** ([📄 graphql](nullability/v0.2/nullability-v0.2.graphql))
- **[tag/v0.1](/tag/v0.1)** ([📄 graphql](tag/v0.1/tag-v0.1.graphql))
- **[tag/v0.2](/tag/v0.2)** ([📄 graphql](tag/v0.2/tag-v0.2.graphql))
4 changes: 2 additions & 2 deletions index.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@

[kotlin_labs v0.2](kotlin_labs/v0.2) incubating directives supported by the Apollo Kotlin client

## nullability v0.1
## nullability v0.2

[nullability v0.1](nullability/v0.1) incubating directives to work with nullability
[nullability v0.2](nullability/v0.2) incubating directives to work with nullability

# All Schemas

Expand Down
100 changes: 100 additions & 0 deletions nullability/v0.2/nullability-v0.2.graphql
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
"""
Indicates that a position is semantically non null: it is only null if there is a matching error in the `errors` array.
In all other cases, the position is non-null.

Tools doing code generation may use this information to generate the position as non-null:
martinbonnin marked this conversation as resolved.
Show resolved Hide resolved

```graphql
type User {
# email is semantically non-null and can be generated as non-null by error-handling clients.
email: String @semanticNonNull
}
```

The `field` argument is the name of the field if `@semanticNonNull` is applied to an object type extension:

```graphql
# extend the base schema to make User.email semantically non-null.
extend type User @semanticNonNull(field: "email")
```

`field` is used for client applications that do not control the base schema and must use type extensions.
When used on a field definition, `field` must be null.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to the main change in this PR, but...

I'm guessing you considered this already, but it would be possible to let regular GraphQL schema validation enforce this invariant for us by making a separate directive for the case where you specify a field, and then have that directive require a field be specified, and removing the argument from the field version.

As it stands we need ad-hoc validation that you do specify a field when used on a type, and that you don't specify a field when used on a field.

directive @semanticNonNull(level: Int = 0) repeatable on FIELD_DEFINITION
directive @semanticNonNullField(field: String!, level: Int = 0) repeatable on OBJECT

That said, I can see that having two directives is also cumbersome.

Copy link
Contributor Author

@martinbonnin martinbonnin Jan 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. The less custom validation I have to do, the happier I am. @semanticNonNullField(field: String!) feels like a mouthful of "fields" though. Other suggestions:

extend type User @semanticNonNullField(name: "friends", level: 0)
extend type User @addSemanticNonNull(forField: "friends", level: 0)

Also thinking about this, we could also remove the repeatable from the regular @semanticNonNull:

# pass all your levels all at once
directive @semanticNonNull(levels: [Int] = [0]) on FIELD_DEFINITION
# this ones needs to be repeatable for different fields. Which means we need validation that different directives for the same field do not conflict but 🤷 
directive @semanticNonNullField(name: String, levels: [Int] = [0]) repeatable on OBJECT | INTERFACE

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if we make levels plural, then we can remove the repeatable for the FIELD_DEFINITION variant. @addSemanticNonNull seem fine!

So the remaining validation we need to do:

  1. The field/level(s) being referenced exist and are currently nullable
  2. For @addSemanticNonNull, validate that the field exists
  3. Validate type compatibility with interfaces

In theory you could end up with a type getting marked as semantically non null in multiple ways. Probably that's okay? As long as it does not confuse tools:

type User @addSemanticNonNull(forField: "name") {
  name: String @semanticNonNull
}

Or

type User {
  friends: [User] @semanticNonNull(levels: [1,1,1,])
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory you could end up with a type getting marked as semantically non null in multiple ways.

More validation to be done but I think it's ok.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name nit: I find the word add to be a bit off for a directive because in a way all directives add something. semanticNonNullField works better for me but not a strong opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree directives are declarative and shouldn't start with a verb. I settled with @semanticNonNullField(name: String, levels: [Int]) in ba09b50, let me know what you think.

Also nitpick but it should really be @semanticNonNullPosition(field: String, levels: [Int]) but the spec uses field instead of position (or something else) in a lot of places so I kept Field for consistency even if it's a bit wrong.


The `level` argument indicates what level is semantically non null in case of lists:

```graphql
type User {
# friends is semantically non null
friends: [User] @semanticNonNull # same as @semanticNonNull(level: 0)

# friends[0], ... are semantically non null
friends: [User] @semanticNonNull(level: 1)

# friends, friends[0], ... are all semantically non null
friends: [User] @semanticNonNull(level: 0) @semanticNonNull(level: 1)
BoD marked this conversation as resolved.
Show resolved Hide resolved
}
```

`level` is zero indexed.
Passing a negative level or a level greater than the list dimension is an error.
martinbonnin marked this conversation as resolved.
Show resolved Hide resolved

"""
directive @semanticNonNull(field: String = null, level: Int = 0) repeatable on FIELD_DEFINITION | OBJECT
martinbonnin marked this conversation as resolved.
Show resolved Hide resolved

"""
Indicates how clients should handle errors on a given position.

When used on the schema definition, `@catch` applies to every position that can return an error.

The `level` argument indicates where to catch errors in case of lists:

```graphql
{
user {
# friends catches errors
friends @catch { name } # same as @catch(level:0)

# friends[0], ... catch errors
friends @catch(level: 0) { name }

# friends, friends[0], ... all catch errors
friends @catch(level: 0) @catch(level: 1) { name }
}
}
```

`level` is zero indexed.
Passing a negative level or a level greater than the list dimension is an error.

See `CatchTo` for more details.
"""
directive @catch(to: CatchTo! = RESULT, level: Int = 0) repeatable on FIELD | SCHEMA

enum CatchTo {
"""
Catch the error and map the position to a result type that can contain either
a value or an error.
"""
RESULT,
"""
Catch the error and map the position to a nullable type that will be null
in the case of error.
This does not allow to distinguish between semantic null and error null but
can be simpler in some cases.
"""
NULL,
"""
Throw the error.
Parent positions can recover using `RESULT` or `NULL`.
If no parent position recovers, the parsing stops.
"""
THROW
}

"""
Never throw on field errors.

This is used for backward compatibility for clients where this was the default behaviour.
"""
directive @ignoreErrors on QUERY | MUTATION | SUBSCRIPTION
31 changes: 31 additions & 0 deletions nullability/v0.2/nullability-v0.2.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# nullability v0.2

<h2>Experimental nullability directives</h2>

```raw html
<table class=spec-data>
<tr><td>Status</td><td>Release</td>
<tr><td>Version</td><td>0.2</td>
</table>
<link rel=stylesheet href=https://specs.apollo.dev/apollo-light.css>
<script type=module async defer src=https://specs.apollo.dev/inject-logo.js></script>
```

This specification provides a list of directives to help dealing with nullability. For more information, see [the nullability working group GitHub repository.](https://github.com/graphql/nullability-wg)


#! @semanticNonNull

:::[definition](nullability-v0.2.graphql#@semanticNonNull)

#! @catch

:::[definition](nullability-v0.2.graphql#@catch)

#! CatchTo

:::[definition](nullability-v0.2.graphql#@CatchTo)

#! @ignoreErrors

:::[definition](nullability-v0.2.graphql#@ignoreErrors)