Add header to disable null-bubbling

[tobias-tengler]

No description provided.

[github-actions]

Qodana for .NET

1 new problem were found

Inspection name	Severity	Problems
Return type of a function can be non-nullable	🔶 Warning	1

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at [email protected]

• Or via our issue tracker: https://jb.gg/qodana-issue
• Or share your feedback: https://jb.gg/qodana-discussions

[codecov]

Codecov Report

Attention: Patch coverage is 75.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 70.21%. Comparing base (69d7955) to head (69e564d).
Report is 1 commits behind head on main.

Files	Patch %	Lines
.../Execution/Pipeline/OperationResolverMiddleware.cs	40.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6877      +/-   ##
==========================================
+ Coverage   70.04%   70.21%   +0.16%     
==========================================
  Files        2586     2594       +8     
  Lines      129039   129434     +395     
==========================================
+ Hits        90384    90876     +492     
+ Misses      38655    38558      -97     

Flag	Coverage Δ
unittests	70.21% <75.00%> (+0.16%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[michaelstaib]

This would create a non-spec compliant GraphQL server. We have discussed this in the working group and all agreed that it should not be a default in a GraphQL server. However we could allow it with a header, its the same idea as with the directive ... the client opts into this behaviour.

[tobias-tengler]

@michaelstaib We're in full control of all the clients and can ensure that they can handle it. So the server not being spec-compliant wouldn't be an issue for us.

Of course we could also use the current @nullBubbling directive, but then devs have to remember to add it and we'd like it to be the default.

Switching to the current spec proposal with the @semanticNonNull directive (later a type wrapper like ~!) would also work for us. Would this be preferable?

[michaelstaib]

You can adopt the new spec directive, but having it as default is not a good idea.

[michaelstaib]

However, with a header you can move this into the network layer and no one has to think about it.

[tobias-tengler]

Okay, then I'll switch this from a schema option to a header.

[michaelstaib]

Can you post a link to the rfc?

[tobias-tengler]

I thought this was the current proposal people were getting behind: graphql/graphql-spec#1065

[michaelstaib]

Lets see ... what I can include is a header approach at the moment. I can see that we include the draft with V14 but the new syntax !String seems still a bit odd :)

The header we can keep in even after the proposal is merged and is only implemented once in the client network layer.

[glen-84]

but the new syntax !String seems still a bit odd :)

Ya, I really dislike it.

I prefer Lee's proposal (though I may not fully understand all of the finer details).

[tobias-tengler]

Me too. I'd love ? for nullable and unadorned for non-null (can be null for error). But I guess the new proposal is the most backwards compatible...

[michaelstaib]

@glen-84 @tobias-tengler in the spec working group we need to make sure that every graphql query that ever was created works when upgrading to a newer GraphQL spec.

While when exploring features its good to explore without compromise once you decide for an improvement it must make sure that older queries against the same type system are not breaking.

This is especially painful with introspection.

If you want to make a breaking change it must be worth the cost of the ecosystem upgrading and breaking lots of applications.

[michaelstaib]

Do not get hung up on syntax yet. I will do a prototype for V14 tonight ...

[michaelstaib]

I have started implementing the proposal today ... this will be a high impact change.

We will have a new schema option that specifies the NonNull inference. By default we will infer them as strict NonNull types to keep compatibility.

Newer servers should opt into the new behavior.

services
    .AddGraphQLServer()
    .ModifyOptions(o => o.NonNullInference = NonNullInferenceKind.SemanticNonNull);

With this set all non-null C# types are now inferred as semantic non-null types. In order to explicitly make something strict non-null in this case we need a new annotation.

[StrictNonNull]
public string Hello() => "Hello";

New attributes:

• StrictNonNullAttribute
• StrictNonNullAttribute<T>
• SemanticNonNullAttribute
• SemanticNonNullAttribute<T>

Generic Marker Types:

• StrictNonNull<T>
• SemanticNonNull<T>
• NonNull<T>

[StrictNonNull<List<StringType>>]
public string Hello() => "Hello";

[tobias-tengler]

Very exciting! A semantic non-null will only be tagged with the @semanticNonNull directive in the schema for now though?

type Product {
  name: String @semanticNonNull
}

It would be great to also have the proposed ability to just disable null bubbling, since code generation and IDEs won't be able to handle the directive or the new type modifier yet.

[glen-84]

@glen-84 @tobias-tengler in the spec working group we need to make sure that every graphql query that ever was created works when upgrading to a newer GraphQL spec.

I get that, but Lee's proposal is backwards-compatible – if you don't apply the schema directive (@strictNullability), then the server behaves the same way that it did before.

Even if you added versioning to GraphQL, so that Strict Semantic Nullability could become the default, old queries would simply default to version 1.0/October2021.

IMO, the best possible future outcome is what Lee suggests:

Type! → Type (no null values allowed)
Type? → Type | SemanticNull | ErrorNull (differentiation must be possible)
Type → Type | ErrorNull (differentiation unnecessary)

For example:

type User {
  id: ID!           # Strict non-null (neither semantic- nor error-null).
  username: String? # Nullable, but take note of any field errors (semantic-null vs error-null).
  age: Int          # Non-nullable (Int or error-null).
}

[tobias-tengler]

There will hopefully soon be a prototype for strict nulls (semantic non-null), so this is probably obsolete. Closing.