A helper method equivalent to the [GraphQLNonNullType] attribute for IObjectFieldDescriptor

[marksmithhh]

Currently in Hot Chocolate, unless you use C# 8.0 nullable reference types (which is not really a nice-to-use and pretty feature, and has its own limitations and problems), you would have to explicitly specify which properties/methods that have a reference type should be non-nullable.

You could currently do this using attributes like this:

[GraphQLNonNullType]
public string Title { get; set; }

But to achieve the same thing using IObjectFieldDescriptor's fluent API, this is what you would have to do:

descriptor.Field(p => p.Title).Type<NonNullType<StringType>>();

which is:

• Ugly and long
• The StringType part is effectively unnecessary and redundant
• Looking at that line of code, you're not sure whether the type itself is being changed or it's just being set to non-nullable.

I'm sure you would agree that having a nice little helper method like NonNull() would make the code above way cleaner, more readable, and more understandable at first sight.

descriptor.Field(p => p.Title).NonNull();

It also makes sense to have such a method since we already have the attribute that does the exact same thing.

What do you think?

[aahmadi458]

Agreed. I'm sick of .Type<NonNullType<Type...s at this point! :D

[aradalvand]

I initially thought descriptor.Field(b => b.Author).Type<NonNullType>() would work, but it apparently doesn't.

+ I would also like a method for lists, that is, a shorthand for:

descriptor.Field(p => p.Title).Type<NonNullType<ListType<NonNullType<SomeType>>>();

which is even uglier ! maybe a helper like:

descriptor.Field(p => p.Title).NonNullList();

[michaelstaib]

While I agree, I think this needs a bit more discussion.

There are a lot of edge cases here, for instance, ListsOfLists. Why should NonNullList for instance translate to .Type<NonNullType<ListType<NonNullType<SomeType>>>().

I will add this to the December release and hope we get some more ideas in here. Once we have this API refined we might also rework the attributes to align them.

[aradalvand]

@michaelstaib

Why should NonNullList for instance translate to .Type<NonNullType<ListType<NonNullType<SomeType>>>()?

I assume you mean why not instead translate to .Type<NonNullType<ListType<SomeType>>() (non-nullable list of nullable elements)? Well, simply because you should use NonNull() for that. That would make it go from (the default) Type<ListType<X>> to Type<NonNullType<ListType<X>>. That's what the attribute [GraphQLNonNullType] does too.

In general, we know that there are 3 common scenarios to cover:

1. .Type<NonNullType<X>>
2. .Type<NonNullType<ListType<X>>
3. .Type<NonNullType<ListType<NonNull<X>>>

Now, the first 2 could/should be handled by .NonNull(), but the last one could be handled by another method, which I suggested we could call .NonNullList() or if you want it to be more descriptive .NonNullListOfNonNull() perhaps. But if you find that verbose, how about .NonNull(nonNullElements: true)?

What do you think?

In the case of more complex scenarios, such as a list of lists like you mentioned, maybe the user should express what they need explicitly with the .Type<> method, but for other simpler scenarios (which take up about 90% of scenarios :D) those helper methods would be extremely useful.

[marksmithhh]

@michaelstaib

What about just allowing:

.Type<NonNullType>()

Pros:

• No need for extra helper methods
• Can handle ALL edge cases (e.g. list of lists, list of list of lists, etc.)

Cons:

• More verbose than .NonNull() obviously, but I think the advantages outweigh this disadvantage.

I myself originally suggested adding .NonNull() but given the fact that there are more edge cases than I initially thought, as @michaelstaib mentioned, now I would say .Type<NonNullType> is a good choice too.

Since our problem was basically the fact that SomeType in .Type<NonNullType<SomeType>> was redundant, now with this, our original problem is solved, in addition, any complex edge cases can also be handled. For example, non-nullable list of nullable list of non-nulls:

.Type<NonNullType<ListType<ListType<NonNullType>>>()

You guys @AradAral @aahmadi458 also tell us what you think about this.

[PascalSenn]

Another option would be:

public IObjectFieldDescriptor NonNull(
    bool scalar = true,
    bool list = true,
    bool nestedList= true);

For scalars list and nested lists would the default behaviour of NonNull be that everything is non null. It would be possible to override specific parts of the result.

Like:

foo: [[Bar!]!]!

descriptor.Field(x => x.Foo).NonNull();

foo: [[Bar]!]!

descriptor.Field(x => x.Foo).NonNull(scalar: false);

foo: [[Bar]]!

descriptor.Field(x => x.Foo).NonNull(scalar: false, nestedList: false);

foo: [[Bar]]

descriptor.Field(x => x.Foo).NonNull(scalar: false, nestedList: false, list: false);

foo: [[Bar!]]

descriptor.Field(x => x.Foo).NonNull(nestedList: false, list: false);

[aahmadi458]

Y'know I'm fine with both @PascalSenn's parameterized .NonNull(), and @marksmithhh's .Type<NonNullType>(). As long as I can get rid of the ugly long .Type<NonNullType<Something>>() lol. But seriously they both look pretty neat to me.

I have a question though, it might be irrelevant but how would someone achieve say a [[[[Bar]]!]] (list of list of non-nullable list of list of Bars) with @PascalSenn's .NonNull()? I know it's almost ridiculous and it's extremely rare to have a three-or-four-dimensional list, but if we're talking edge cases then... :P In other words, what would nestedList be referring to exactly if we had a more than 2 dimensional list? So does that mean that .Type<NonNullType>() is more flexible in that aspect?

[PascalSenn]

@aahmadi458 we do not have to worry about 3 or more dimensions because in GraphQl only two dimensional lists are supported. This is defined by the offical specification

[aahmadi458]

@PascalSenn Oh okay, I didn't know that. There's no problem then. What you suggested can handles edge cases, basically. Would you guys then consider this for the HC-11.0.0 milestone, instead of HC-11.1.0, perhaps?

[aradalvand]

As a reply to myself:

In general, we know that there are 3 common scenarios to cover:

I realized there is actually another common scenario which I missed: A nullable list of non-nullable elements:

1. .Type<ListType<NonNullType<X>> And this is not achievable with either my .NonNull() or .NonNullList(), except if NonNullList() received a parameter (like nonNullList) which I think would make it a little unintuitive. So, maybe we can rule out my proposals!

Eventually, I've come to like both .Type<NonNullType>(), AND a single .NonNull() method with optional parameters, which would default to everything being non-nullable, as @PascalSenn proposed

They're both intuitive.

So, I'd say whichever you guys @michaelstaib @PascalSenn finally go for is a matter of difficulty of implementation and other under-the-hood factors maybe. The external interface of both is equally nice.

I also believe these 2 are the only reasonable ways of implementing this helper. So, case closed!

[ztolley]

Did this make it into code yet?

I have a scenario where I has a Basket type and a BasketItem type. I've written the descriptor and resolver for items but that generates a type in the schema showing the list as nullable.

I cant figure out for the life of me how to tell the descriptor that the 'items' property in Basket is a non-nullable list of Item and that each of those items is non-nullable.

type Basket { items: [BasketItem!]! }

Isn't this a common requirement?

[aradalvand]

@ztolley Why can't you do the following?!

descriptor.Field(b => b.BasketItems).Type<NonNullType<ListType<NonNullType<BasketItem>>>>()

[ztolley]

ok. Trying that, just have to get around

Solution OrderApi.sln Project GraphQL GraphQL\Baskets\BasketType.cs:24 The type 'OrderAPI.GraphQL.Data.BasketItem' must be convertible to 'HotChocolate.Types.IType' in order to use it as parameter 'T' in the generic class 'HotChocolate.Types.NonNullType<T>'

Thanks for the pointer

[aradalvand]

You should have a schema type for BasketItem. Something like BasketItemType that derives from ObjectType<BasketItem>. Replace BasketItem with BasketItemType in .Type<NonNullType<ListType<NonNullType<BasketItem>>>>() in order to get rid of the error.

[ztolley]

That worked. Looking forward to a sweeter syntax :)

[aradalvand]

Any updates on this?

[michaelstaib]

Not yet... at the moment focus is on the client API. We will have a look at this once we have implemented the main features for 11.1

[aradalvand]

@michaelstaib Ok, fine. Happy to hear that the focus is on Strawberry Shake!

[aradalvand]

Any updates on this one, guys?

[michaelstaib]

We have resisted this one but did not find a good suggestion in this thread. All of the suggestions do not hold up for all cases. I think if we put new API in it should work well in all the use-cases. I also think it is not a pressing issue since it is easy to extend the descriptors with some local extension methods.

We will revisit this with the August release and wait for more input. Anybody got a better idea?

[aradalvand]

All of the suggestions do not hold up for all cases.

@michaelstaib Would you elaborate on that a bit? What's wrong with @PascalSenn's suggestion in this comment, for example? What use case would it not cover?

[aradalvand]

@michaelstaib @PascalSenn Hi! No updates yet?

[aradalvand]

@michaelstaib Hi. No updates on this one yet? Any approximate date as to when we can expect this?

[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.

[aradalvand]

.

[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.

[aradalvand]

(comment for stale bot)

[michaelstaib]

I removed the ping comments to get the issue cleaner again.

[jarlef]

As a current workaround i have used the following ObjectFieldDescriptor extension method based on similar approach in FilterFieldDescriptorExtensions

public static TFieldDescriptor AsNonNull<TFieldDescriptor>(this TFieldDescriptor descriptor)
        where TFieldDescriptor : IDescriptor<FieldDefinitionBase>
    {
        descriptor.Extend().OnBeforeCreate((c, def) => def.Type = RewriteTypeToNonNullType(def, c.TypeInspector));
        return descriptor;
    }

    public static TFieldDescriptor AsNonNullList<TFieldDescriptor>(this TFieldDescriptor descriptor)
        where TFieldDescriptor : IDescriptor<FieldDefinitionBase>
    {
        descriptor.Extend().OnBeforeCreate((c, def) => def.Type = RewriteListTypeToNonNullType(def, c.TypeInspector));
        return descriptor;
    }

    private static ITypeReference RewriteTypeToNonNullType(FieldDefinitionBase definition, ITypeInspector typeInspector)
    {
        var reference = definition.Type;

        if (reference is ExtendedTypeReference extendedTypeRef)
        {
            return extendedTypeRef.WithType(typeInspector.ChangeNullability(extendedTypeRef.Type, false));
        }

        throw new NotSupportedException();
    }

    private static ITypeReference RewriteListTypeToNonNullType(FieldDefinitionBase definition, ITypeInspector typeInspector)
    {
        var reference = definition.Type;

        if (reference is ExtendedTypeReference extendedTypeRef)
        {

            return extendedTypeRef.WithType(typeInspector.ChangeNullability(extendedTypeRef.Type, false, false));
        }

        throw new NotSupportedException();
    }

protected override void Configure(IObjectTypeDescriptor<Asset> descriptor)
{
    descriptor.Field(x => x.Id).AsNonNull();
    descriptor.Field(x => x.SubAssets).AsNonNullList();
}

[fleed]

any updates on this one? I just accidentally realized that the resolved properties are marked as nullable.

Using @jarlef 's extensions to fix it.

[AntonC9018]

This requires a bit of an internal refactor as well. If we don't get the complete type passed into Type, we'll have to rewrite it later rather than find it immediately, which is what seems to happen currently. I personally like the non-generic-last-type approach. I've put together a prototype method that deals with the reflection, but there might be a better way than constructing the type with reflection to get it out of cache. https://github.com/ChilliCream/graphql-platform/blob/2e28bb203c68a8dbcc1b59a11fd5e6eef689f5cb/src/HotChocolate/Core/src/Types/Types/Helpers/ImplyTypeHelper.cs#L27 I'll work on it some more tomorrow. Using NativeType here instead of the object type might just work actually, I'll try that next.