Add SemanticNonNull support

[XiNiHa]

Adds SemanticNonNull support for every effectful types.

Currently, the support is implemented to add @semanticNonNull directives to fields that are semantically non-null. However, as the spec evolves and merges, it's very likely to have a separate syntax to annotate the semantic nullability of a field.

The implementation is very clear for Caliban since we know all the possibilities of errors and nulls. @semanticNonNull is only applied when 1. the resolver is faillible 2. the field is optional 3. the result type is not optional.

Although the spec also supports more detailed configuration of semantic nullability for list fields, it was not applicable to Caliban's case since as it currently doesn't resolve error inside stream to null. (actually it feels a bit odd considering the decision of having effects with E <: Throwable as nullable)

While I think that the feature should be blocked by default with a feature flag, I have no idea how that would be implemented. Any ideas?

[kyri-petrou]

I haven't looked into this PR in depth, but is this similar to this annotation?

https://github.com/ghostdogpr/caliban/blob/7ddba86ea5881543df3eed69b74bc4eb0ead5983/core/src/main/scala/caliban/schema/Annotations.scala#L77

[XiNiHa]

Nope, it's about having 3 types of nullability instead of two - NonNull, SemanticNonNull, and Nullable. The directive you linked only (and correctly, even with the PR) overrides the nullability to be NonNull, while this PR separates out a new state, "it's NonNull unless it errors".

[ghostdogpr]

While I think that the feature should be blocked by default with a feature flag, I have no idea how that would be implemented. Any ideas?

Hmm, good question. We have a Feature concept already for Defer and we also FiberRef for configuration, but both are unapplicable here because this change affects schema derivation, which is a pure operation.

Maybe adding a method in SchemaDerivation that you can override and then requiring something like that:

object MySchema extends Schema.SchemaDerivation[Env] {
  override def enableSemanticNonNull: Boolean = true
}

// then use `MySchema.gen`

[kyri-petrou]

@XiNiHa thank you very much for your contribution! I'm trying to understand a bit the spec around @semanticNonNull and what is this PR meant to be implementing but I might need some help to understand what's going on

Although the spec also supports more detailed configuration of semantic nullability for list fields

The spec linked here that comes from Apollo seems to use a field directive to add the @semanticallyNonNull directive to individual fields, irrespective of type. If I'm understanding this spec correctly, that means we would need to use field annotations for this instead of binding this to the type itself

However, as https://github.com/graphql/graphql-spec/pull/1065 evolves and merges

Now this is the part that really confuses me. This RFC proposes an extension to the type system, for SemanticallyNonNull to be represented via syntax (e.g., !String). For this, using the type system like in the PR makes more sense, but then the PR doesn't do any of the changes proposed in the RFC but instead uses a field directive.

@XiNiHa could you please clarify this for me, which spec is it that this PR is implementing?

[XiNiHa]

The spec linked here that comes from Apollo seems to use a field directive to add the @semanticallyNonNull directive to individual fields, irrespective of type. If I'm understanding this spec correctly, that means we would need to use field annotations for this instead of binding this to the type itself

What I meant for the implementation was to add the @semanticNonNull annotation on FIELD_DEFINITIONs, not for OBJECTs. Did you find anything that I did wrong? At least the tests are working correctly.

Now this is the part that really confuses me. This RFC proposes an extension to the type system, for SemanticallyNonNull to be represented via syntax (e.g., !String). For this, using the type system like in the PR makes more sense, but then the PR doesn't do any of the changes proposed in the RFC but instead uses a field directive.

Fully implementing the spec (with the syntax addition) requires much more work for both Caliban and the ecosystem, and AFAIK none of the ecosystem members have implemented the syntax part. Instead, ecosystem members like Relay, Grats, and Apollo Kotlin, are experimenting with only the semantics, by using the directives instead of a separate syntax. Maybe this document from Grats would be more helpful in understanding the surroundings.

[kyri-petrou]

Did you find anything that I did wrong? At least the tests are working correctly.

I'm a little bit worried about tying the implementation of a field-specific directive to the Schema / type system, as I fear it'll probably going to bite us down the line in some way. There is already some work underway to be able to do Schema transformations, and I feel this might add another dimension to the things we'll need to cater for.

I need to think this through a bit more, but instead of adding def semanticNonNull: Boolean to Schema, how about we have something like def canFail: Boolean? While it might be similar to what semanticNonNull is, it doesn't tie the Schema to field-specific features. Plus this opens up possibilities for other future features as it's much more generic.

In derivation, we can then decide whether we'll add the @semanticNonNull annotation to fields depending on the combination of canFail, optional and enableSemanticNonNull from SchemaDerivation.

[XiNiHa]

Sounds great. I'll adjust the implementation to reflect what you've proposed!

[XiNiHa]

@kyri-petrou Done!

[kyri-petrou]

By the way before sending you off down some rabbithole, this might be just a rendering issue. So I'd check that first!

[XiNiHa]

it seems that there is a bug where top-level query and mutation fields that can fail are derived as non-null

I was not able to reproduce this ;(

[kyri-petrou]

Ok I managed to track down the source of the issue. It seems that I had a custom schema defined in the application like this:

  given [A](using ev: Schema[R, A]): Schema[R, FieldOps => A] with {
    override def arguments: List[__InputValue]                    = ev.arguments
    override def optional: Boolean                                = ev.optional
    def toType(isInput: Boolean, isSubscription: Boolean): __Type = ev.toType_(isInput, isSubscription)

    def resolve(value: FieldOps => A): Step[R] = MetadataFunctionStep { f =>
      val fops = new FieldOps(f)
      ev.resolve(value(fops))
    }
  }

Since now the underlying ev.optional is different, that led to a different schema derivation

While this is not a common use-case, we can't assume that other users aren't doing the same thing. @ghostdogpr open to suggestions on naming, but I suggest we do something along these lines (although I'm kind of inclined towards just making it final and let it break source compatibility)

  @deprecatedOverriding("this method will be made final. Override canFail and nullable instead", "2.6.1")
  def optional: Boolean = canFail || nullable

  def canFail: Boolean  = false
  def nullable: Boolean = false // Happy for other name suggestions

Then the majority of the code should remain the same and use optional except for derivation. Thoughts?

[XiNiHa]

@kyri-petrou applied your suggestion! (I named it nullable for now)

[ghostdogpr]

Hmm there is a Mima failure, shall we just exclude it?

[ghostdogpr]

I disabled Mima on the main branch so if you rebase that will solve that issue.

[XiNiHa]

Looks like CI is finally passing :tada:

[ghostdogpr]

Thanks for adding this! Would you be able to submit a PR for adding a few docs about it? Maybe a new section in https://github.com/ghostdogpr/caliban/blob/series/2.x/vuepress/docs/docs/schema.md