Inconsistent behavior for array nullability creates invalid schema when using collection resolvers

[zaneclaes]

Describe the bug HotChocolate (appears) to treat ICollection<T> differently for models and for resolvers.

To Reproduce Using EFCore5, create a ParentModel and ChildModel with a N:M (many-to-many) relationship:

class ParentModel {
  public ICollection<ChildModel> children { get; set; } = new List<ChildModel>();
}

(and vice versa).

Note that the schema is of type [ChildModel!]!. This is correct, in that neither the array nor the child models may be null, per the lack of nullability in the source code.

However, now try to add a resolver to the parent model. E.g.,

descriptor.Field(p => p.children).ResolveWith(r => new List<ChildModel>());

In this case, the generated schema is [ChildModel]. This subsequently causes downstream errors with clients which depend on correct schema, such as graphql-codegen, requiring a ton of nullability boilerplate to fix.

Expected behavior Both code paths should respect nullability in the same fashion.

Desktop (please complete the following information):

• OS: Mac OSX
• Version: 11.0.0-rc.1

[zaneclaes]

I was able to fix the schema by explicitly adding .Type<NonNullType<ListType<NonNullType<ChildModelObj>>>(), fwiw.

[gojanpaolo]

Note that we also need to explicitly handle null in the resolver function, at least when using DataLoader. A Cannot return null for non-nullable field error will be thrown if null is not handled explicitly,

e.g.

descriptor
    .Field(p => p.Children)
    .ResolveWith<MyResolver>(r => r.GetChildrenAsync(default!, default!, default))
    .Type<NonNullType<ListType<NonNullType<ChildModelObj>>>>();

// MyResolver
public async Task<IEnumerable<ChildModel>> GetChildrenAsync(
    Parent parent,
    ChildByParentIdDataLoader childByParentId,
    CancellationToken cancellationToken) =>
    await childByParentId.LoadAsync(parent.Id, cancellationToken)
    ?? Enumerable.Empty<ChildModel>(); // notice explicit null handling

[michaelstaib]

Completely overlooked this one.

[michaelstaib]

So this are different issues.... the first issue

ResolveWith(r => new List) is actually not meant be used with an expression .... ResolveWith is meant to refer to member expressions.

-> .ResolveWith(r => r.GetChildrenAsync(default!, default!, default))

the above is correct.

Resolve is for delegates.

[michaelstaib]

@gojanpaolo also this is correct. If you return null this is per spec invalid. That you want an empty list in this case is implementation detail to your specific case.

[gojanpaolo]

@michaelstaib I think the issue on my example is that the DataLoader.LoadAsync returns Task<TValue> where TValue is not marked as nullable. I believe it should be marked nullable to follow C#'s nullable reference types.

Here is what my DataLoader implementation looks like

public sealed class ChildrenByParentIdDataLoader : BatchDataLoader<Guid, IGrouping<int, ChildModel>>
{
    protected async override Task<IReadOnlyDictionary<int, IGrouping<int, ChildModel>>> LoadBatchAsync(IReadOnlyList<int> keys, CancellationToken cancellationToken)
    {
        ...
    }
}

... and the ChildrenByParentIdDataLoader.LoadAsync says it returns non-nullable Task<IGrouping<int, ChildModel>>

[Odonno]

Facing the same problem here. Adding the .Type() was my only idea, like @zaneclaes you did.

It's really annoying that it does not infer the NonNull or Null type from the Field() function...

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.