Change `@semanticNonNull.level` and `@catch.level` default to be `0`?

[martinbonnin]

Follow up from comment from @captbaritone on discord.
The current description of @semanticNonNull states that:

"""
...
If `level` is null, all levels are semantically non null.
...
"""
directive @semanticNonNull(field: String = null, level: Int = null) repeatable on FIELD_DEFINITION | OBJECT

One advantage is that it's more compact in list cases. Compare:

type User {
  # make all levels nullable with a single directive
  friends: [Users] @semanticNonNull
}

to

type User {
  # making all levels nullable requires several directives
  friends: [Users] @semanticNonNull(level: 0) @semanticNonNull(level: 1)
}

A drawback is that it makes the directive definition more complex (level is now nullable, this is surprising).

Another drawback is that it opens the door to ambiguous behaviour:

type User {
  # should that be an error? a warning?
  friends: [Users!] @semanticNonNull
}

Since @semanticNonNull applies to all levels above, it's also applying to User!, should that be an error?

There's a tradeoff of compactness vs explicitness. Since @semanticNonNull is itself verbose already maybe the extra verbosity is worth it if it makes things more explicit.

See also: apollographql/apollo-kotlin#5462

[BoD]

My 2c: @semanticNonNull on a non null field is not worth a warning as both signals are consistent. It feels like (I could be wrong!) nulls inside list are a pretty rare use case? If that's true, most applications almost never want nulls at any levels, and so this being the default is sensible.

[captbaritone]

@semanticNonNull on a non null field is not worth a warning as both signals are consistent.

I'm not sure I follow. A type cannot be simultaneously non-nullable and semantically-non-nullable. In other words, following the syntax suggested in @GeNJie's RFC it would be invalid SDL to make a field both non-null and semantic non-null:

type User {
  name: !String! # Presumably this would be an error
}

Following from that observation, and your assessment that nullable list items are rare (I'm inclined to agree), having the default be that we apply to all levels seems likely to invite that conflicting state where we are declaring that a type is both non-nullable and semantically-non-nullable.

type User {
  # The ! on the list item conflicts with @semanticNonNull applying to all levels
  friends: [User!] @semanticNonNull
}

As I've looked at supporting @semanticNonNull in Grats, I've discovered that semantically non-null list items are a bit less straight forward than fields. Even schemas which opt for default nullability in order to achieve more resiliency generally don't make list items nullable since resiliency advantage is minimal: List items can only get into an error state due to validation, which is much less common than runtime exceptions.

The upshot is that I suspect it will be rare to make items within lists @semanticNonNull. If the items are not expected to be null, schema designers will likely choose explicitly non-null so that non-error handling clients can expect non null values. If the value is expected to be null, then they won't want @semanticNonNull.

For a more detailed breaking of this reasoning see this doc.

If my reasoning is sound, then semantic non-null list items will be rare and most schema authors will opt for explicit non-nullable or nullable. But, schema authors will likely still want to mark most plural fields as @semanticNonNull. This means they will need to remember to add level: 0 whenever annotating a list field.

I think a more sensible mental model for schema authors is: "Slap a @semanticNonNull on any field you want to be semantically non-nullable. In the rare case that you want semantically non-nullable list items add additional directives to specify that as well".

[BoD]

A type cannot be simultaneously non-nullable and semantically-non-nullable.

Ah that actually makes sense - I was seeing this from the lense of our codegen which will generate the field as non nullable in both cases. But it’s true the semantics are incompatible and a warning at least is probably warranted indeed.

[martinbonnin]

Pull request to change default level to 0: #42

[martinbonnin]

Closed with #42