Nullability of reference type dictionary values is not correctly translated to schema

[thekatze]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

In a Project with nullable reference types enabled and a GraphQL mutation endpoint like this:

public class Mutation {
    public record WorkItem(string Id, string? Description);
    public Task<Dictionary<string, WorkItem?>> DoWork(Dictionary<string, WorkItem?> workItems)
    {
        // Do Work ...
        return Task.FromResult(workItems);
    }
}

The generated GraphQL Schema looks like this:

type Mutation {
  doWork(input: DoWorkInput!): DoWorkPayload!
}

input DoWorkInput {
  workItems: [KeyValuePairOfStringAndWorkItemInput!]!
}

input KeyValuePairOfStringAndWorkItemInput {
  key: String!
  value: WorkItemInput!
}

input WorkItemInput {
  id: String!
  description: String
}

The value: WorkItemInput! in KeyValuePairOfStringAndWorkItemInput should be nullable as is defined in Dictionary<string, WorkItem?>, but it is non-nullable.

Steps to reproduce

Full Reproduction in Repository: https://github.com/TheKatze/HotChocolateNullableRepro

Product

Hot Chocolate

Version

12.14.0, 13.0.0-preview.66

[michaelstaib]

Thanks for reporting, I will have a look at this.

[BickelLukas]

Any update on this @michaelstaib ?

[nir-legit]

Hi, I encountered the same issue. I have a Dictionary<string, string>, and sometimes the value string is null, but I get an error when returning it that says, "Cannot return null for non-nullable field." (while string should be nullable)
Are there any updates on this issue? @michaelstaib

[san-legit]

Any updates here? :)

[michaelstaib]

I have scheduled it on 15.2

Just from a schema design perspective I would not expose a dictionary directly but create a better GraphQL representation.

[N-Olbert]

Just wanted to share my analysis results after debugging this for about two hours (so not a too deep analysis at all - it might be partly wrong / incomplete)

First of all this has nothing to do with dictionary and instead is a side effect of the the nullability info erasure for reference type-generics.
For example

[Fact]
public async Task GenericTypeArguments_Nullability_Inference()
{
    var schema =
        await new ServiceCollection()
            .AddGraphQL()
            .AddQueryType<WithGenericType>()
            .BuildSchemaAsync();

    // assert
    schema.MatchSnapshot();
}

public class WithGenericType
{
    public Foo<string?>? Test => throw new NotImplementedException();
}

public record Foo<T>(T Bar);

results in

schema {
  query: WithGenericType
}

type FooOfString {      # WRONG, should be "FooOfNullableString" 
  bar: String!          # WRONG, should be bar: String
}

type WithGenericType {
  test: FooOfString     # WRONG, should be "FooOfNullableString"
}

It is a fact that at compile time typeof(Foo<string?>) becomes typeof(Foo<string>), therefore Assert.Equal(typeof(Foo<string?>), typeof(Foo<string> )); passes.

The nullability info is only stored with Nullable-attributes, see sharplab
Anyway, at the point in time when the field definition/descriptor is created, the only thing we have is the type of the property/method itself (-> aka Foo.Bar) and from this, the nullability can't be inferred, since the attibutes are defined within the type that contains the actual property / method of that generic type(-> WithGenericType in the sample). This makes totally sense since Foo<string?> Member1 is different from Foo<string> Member2 since the nullability differs, but at compile time they share the same generic type definition.

To infer the nullability correctly, the ObjectFieldDescriptor (and the other descriptors too, of course) would need the something like the ExtendedTypeInfo of the type or field that caused their creation, since this structure includes the nullability-info (within ExtendedTypeInfo.TypeArguments).
While this may be passed along with some - in my opinion and at first sight - major changes, it seems like a lot of effort for little benefit.

During the name creation of generic types, nullability of reference types is currently not considered at all.

In general there seem to be a lot of edge cases like a type whose generic type argument may not be exposed by a member at all (which means it would only affect the name of the type) or generics within generics etc.

The Hotfix is quite simple... rebind the dictionary to a custom KVP whose Value is nullable, and expose as a list of that KVP.

public class Query
{
    public IEnumerable<KeyNullableValuePair<string, string>> Echo(
        IEnumerable<KeyNullableValuePair<string, string>> dict) =>
        dict.ToDictionary(x => x.Key, x => x.Value)
            .Select(x => new KeyNullableValuePair<string, string>(x.Key, x.Value));
}

public record KeyNullableValuePair<TKey, TValue>(TKey Key, TValue? Value) where TKey : notnull; // Note that TValue is always nullable

internal class KeyNullableValuePairType<TKey, TValue> : ObjectType<KeyNullableValuePair<TKey, TValue>>
    where TKey : notnull
{
    protected override void Configure(IObjectTypeDescriptor<KeyNullableValuePair<TKey, TValue>> descriptor)
    {
        descriptor.BindFieldsImplicitly();
        descriptor.Name($"KeyValuePairOf{typeof(TKey).Name}AndNullable{typeof(TValue).Name}");
    }
}