Add SemanticNonNull support by XiNiHa · Pull Request #2180 · ghostdogpr/caliban

XiNiHa · 2024-04-05T12:58:50Z

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 · 2024-04-05T15:36:38Z

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

caliban/core/src/main/scala/caliban/schema/Annotations.scala

Line 77 in 7ddba86

case class GQLNonNullable() extends StaticAnnotation

XiNiHa · 2024-04-05T16:02:47Z

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 · 2024-04-06T02:51:35Z

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`

ghostdogpr

Looks okay to me.

@kyri-petrou can you check too?

kyri-petrou · 2024-04-09T10:31:01Z

@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 graphql/graphql-spec#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 · 2024-04-09T10:41:49Z

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 · 2024-04-09T11:16:47Z

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 · 2024-04-09T11:26:26Z

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

XiNiHa · 2024-04-09T14:23:59Z

@kyri-petrou Done!

kyri-petrou

Reviewed from my phone, but I want to test it a bit more on some services at $WORK later just to make sure we're not introducing any derivation regressions

kyri-petrou · 2024-04-09T22:57:06Z

core/src/main/scala/caliban/schema/Schema.scala

  ): Schema[R1, ZStream[R1, Nothing, A]] =
    new Schema[R1, ZStream[R1, Nothing, A]] {
      override def optional: Boolean                                         = false
+      override def canFail: Boolean                                          = ev.canFail


Shouldn't this be false?

IMO if A is a non-effectful faillible type, these should also be faillible. However since we have none of them, it'll always be false anyways. But semantically I see this more correct.

kyri-petrou · 2024-04-09T22:57:49Z

core/src/main/scala/caliban/schema/Schema.scala

  ): Schema[R0, ZQuery[R1, Nothing, A]] =
    new Schema[R0, ZQuery[R1, Nothing, A]] {
      override def optional: Boolean                                         = ev.optional
+      override def canFail: Boolean                                          = ev.canFail


Similarly, shouldn't this be false?

kyri-petrou · 2024-04-09T22:58:15Z

core/src/main/scala/caliban/schema/Schema.scala

  implicit def infallibleEffectSchema[R0, R1 >: R0, R2 >: R0, A](implicit ev: Schema[R2, A]): Schema[R0, URIO[R1, A]] =
    new Schema[R0, URIO[R1, A]] {
      override def optional: Boolean                                         = ev.optional
+      override def canFail: Boolean                                          = ev.canFail


Yeah, shouldn't this one be false?

Ah, saw your comment below

core/src/main/scala/caliban/schema/Schema.scala

kyri-petrou

Looks great overall, although I just tested this PR locally with a service at $WORK (Scala 3), and it seems that there is a bug where top-level query and mutation fields that can fail are derived as non-null. For some reason, this doesn't seem to affect any other fields - just top-level ones

Can you please try and reproduce the issue in a test suite and fix the source of it?

Schema rendered with v2.6.0 vs this PR:

kyri-petrou · 2024-04-24T08:35:59Z

core/src/main/scala/caliban/schema/SchemaUtils.scala

+  /**
+   * Directive used to mark a field as semantically non-nullable.
+   */
+  val SemanticNonNull = Directive("semanticNonNull")


Perhaps this is better placed in caliban.parsing.adt.Directive as we have other directives defined in there

Since those are all name strings instead of actual directive instances, I'm worried that adding this causes some confusion 🤔

kyri-petrou · 2024-04-24T08:36:50Z

core/src/main/scala-3/caliban/schema/SchemaDerivation.scala

+   *
+   * Override this method and return `true` to enable the feature.
+   */
+  def enableSemanticNonNull: Boolean = false


May I recommend we use a DerivationConfig case class instead? I can see us wanting to add more derivation customization options in the future

kyri-petrou · 2024-04-24T12:35:46Z

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

XiNiHa · 2024-04-24T13:13:03Z

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 · 2024-04-25T10:35:28Z

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 · 2024-04-25T10:59:40Z

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

kyri-petrou · 2024-05-09T00:20:28Z

core/src/main/scala-2/caliban/schema/SchemaDerivation.scala

                val hasNullableAnn = p.annotations.contains(GQLNullable())
                val hasNonNullAnn  = p.annotations.contains(GQLNonNullable())
-                !hasNonNullAnn && (hasNullableAnn || p.typeclass.optional)
+                (!hasNonNullAnn && (hasNullableAnn || p.typeclass.nullable), hasNullableAnn || hasNonNullAnn)


I think this should be optional?

Suggested change

(!hasNonNullAnn && (hasNullableAnn || p.typeclass.nullable), hasNullableAnn || hasNonNullAnn)

(!hasNonNullAnn && (hasNullableAnn || p.typeclass.optional), hasNullableAnn || hasNonNullAnn)

I don't think the code was wrong, but I agree that was somewhat confusing to read. I've updated the logic to represent it better what I've meant.

kyri-petrou · 2024-05-09T00:21:39Z

core/src/main/scala-3/caliban/schema/DerivationUtils.scala

        val hasNullableAnn = fieldAnnotations.contains(GQLNullable())
        val hasNonNullAnn  = fieldAnnotations.contains(GQLNonNullable())
-        !hasNonNullAnn && (hasNullableAnn || schema.optional)
+        (!hasNonNullAnn && (hasNullableAnn || schema.nullable), hasNullableAnn || hasNonNullAnn)


Same as above

ghostdogpr · 2024-05-09T00:21:46Z

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

kyri-petrou

@XiNiHa could you please add a test for the use-case that I previously posted? I think the bug is still there at the moment

….nullable

kyri-petrou

Thanks for putting up with all my requests and delays in reviews! Happy to have this merged once CI is passing

XiNiHa · 2024-05-11T14:53:41Z

Looks like CI is finally passing 🎉

ghostdogpr · 2024-05-12T00:46:00Z

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

XiNiHa force-pushed the semantic-non-null branch from ee81b99 to faa4a2c Compare April 5, 2024 13:51

XiNiHa force-pushed the semantic-non-null branch 5 times, most recently from 695dac6 to 09d4d12 Compare April 8, 2024 15:56

ghostdogpr approved these changes Apr 9, 2024

View reviewed changes

XiNiHa force-pushed the semantic-non-null branch 4 times, most recently from 2d04c44 to a0f82da Compare April 9, 2024 14:01

kyri-petrou reviewed Apr 9, 2024

View reviewed changes

XiNiHa force-pushed the semantic-non-null branch 2 times, most recently from a84bbfe to 85e67f1 Compare April 23, 2024 11:38

XiNiHa requested a review from kyri-petrou April 23, 2024 11:46

kyri-petrou requested changes Apr 24, 2024

View reviewed changes

XiNiHa force-pushed the semantic-non-null branch 2 times, most recently from d8d3da7 to 7d8e546 Compare April 24, 2024 12:58

kyri-petrou reviewed May 9, 2024

View reviewed changes

XiNiHa added 7 commits May 11, 2024 16:00

Add semanticNonNull support

306b5cb

Disable feature by default with an overridable flag

1abee63

Adjust schema definition to contain canFail instead of semanticNonNull

b77a059

No semanticNonNull transform on hand-written schema

4c515dc

Reuse semanticNonNull directive definition across files

a63acce

Wrap enableSemanticNonNull option with a config object

4107b2e

Revert changing the semantic of Schema.optional by introducing Schema…

fddcc3b

….nullable

XiNiHa force-pushed the semantic-non-null branch from d710a70 to fddcc3b Compare May 11, 2024 07:00

XiNiHa added 2 commits May 11, 2024 16:02

Change deprecation warning version to 2.7.0

6fd85ce

Clear nullability determination logic

82662cb

XiNiHa force-pushed the semantic-non-null branch from e4878c4 to 82662cb Compare May 11, 2024 09:11

Add test to ensure semantic of .optional for custom schemas

61f3c6d

XiNiHa force-pushed the semantic-non-null branch from 0b74e56 to 5c20b50 Compare May 11, 2024 10:09

XiNiHa requested a review from kyri-petrou May 11, 2024 10:09

XiNiHa force-pushed the semantic-non-null branch from 5c20b50 to bb949dd Compare May 11, 2024 10:18

kyri-petrou approved these changes May 11, 2024

View reviewed changes

XiNiHa added 2 commits May 11, 2024 23:40

Fix not keeping semantics of .optional on custom schema

066a821

Add supplementary tests to make things sure

7aea83b

XiNiHa force-pushed the semantic-non-null branch from bb949dd to 7aea83b Compare May 11, 2024 14:41

ghostdogpr approved these changes May 12, 2024

View reviewed changes

ghostdogpr merged commit 63f4d6d into ghostdogpr:series/2.x May 12, 2024

XiNiHa deleted the semantic-non-null branch May 12, 2024 06:16

XiNiHa mentioned this pull request May 12, 2024

Add docs for SemanticNonNull support #2229

Merged

satorg mentioned this pull request Jun 19, 2024

Broken backward compatibility in CatsInterop since v2.7.0 #2295

Closed

	(!hasNonNullAnn && (hasNullableAnn \|\| p.typeclass.nullable), hasNullableAnn \|\| hasNonNullAnn)
	(!hasNonNullAnn && (hasNullableAnn \|\| p.typeclass.optional), hasNullableAnn \|\| hasNonNullAnn)

Uh oh!

Conversation

XiNiHa commented Apr 5, 2024

Uh oh!

kyri-petrou commented Apr 5, 2024

Uh oh!

XiNiHa commented Apr 5, 2024

Uh oh!

ghostdogpr commented Apr 6, 2024

Uh oh!

ghostdogpr left a comment

Choose a reason for hiding this comment

Uh oh!

kyri-petrou commented Apr 9, 2024

Uh oh!

XiNiHa commented Apr 9, 2024

Uh oh!

kyri-petrou commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

XiNiHa commented Apr 9, 2024

Uh oh!

XiNiHa commented Apr 9, 2024

Uh oh!

kyri-petrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kyri-petrou left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyri-petrou Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XiNiHa Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyri-petrou commented Apr 24, 2024

Uh oh!

XiNiHa commented Apr 24, 2024

Uh oh!

kyri-petrou commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

XiNiHa commented Apr 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghostdogpr commented May 9, 2024

Uh oh!

kyri-petrou left a comment

Choose a reason for hiding this comment

Uh oh!

kyri-petrou left a comment

Choose a reason for hiding this comment

Uh oh!

XiNiHa commented May 11, 2024

kyri-petrou commented Apr 9, 2024 •

edited

Loading

kyri-petrou left a comment •

edited

Loading

kyri-petrou Apr 24, 2024 •

edited

Loading

XiNiHa Apr 24, 2024 •

edited

Loading

kyri-petrou commented Apr 25, 2024 •

edited

Loading