Graphql-dotnet: Type comparison appears to be wrong in InputObjectGraphType

[Shane32]

InputObjectGraphType is defined as:

``` C#
public class InputObjectGraphType : ComplexGraphType, IInputObjectGraphType
{
public override FieldType AddField(FieldType fieldType)
{
if (fieldType.Type == typeof(ObjectGraphType))
{
throw new ArgumentException("InputObjectGraphType cannot have fields containing a ObjectGraphType.", nameof(fieldType.Type));
}

        return base.AddField(fieldType);
    }
}

It would appear that this line of code:

``` C#
if (fieldType.Type == typeof(ObjectGraphType))

should have been written as:
C# if ((fieldType.Type != null && typeof(ObjectGraphType).IsAssignableFrom(fieldType.Type)) || ((fieldType.ResolvedType as ObjectGraphType) != null))

Is that correct? If so, I can submit a pull request

[Shane32]

(I'm not exactly familiar with how input graphs deserialize input data into models.)

[sungam3r]

The direction of thoughts is right. I have long wanted to add an implementation of this algorithm from the specification to check in/out types.

[sungam3r]

Also note what ObjectGraphType is :
c# public class ObjectGraphType : ObjectGraphType<object> { }
so ObjectGraphType<T> is more suitable for check against.

[Shane32]

Good reference. I was just thinking that my code would not perform sufficient checks for NonNullGraphType<>

[joemcbride]

There is the following:

https://github.com/graphql-dotnet/graphql-dotnet/blob/fe0368f15dcea1dd578e83c521b985798e1a2214/src/GraphQL/GraphQLExtensions.cs#L44-L48

Though it only works with objects that have a ResolvedType. It could be updated to also check the Type, if there is no ResolvedType.

I would suggest to use the interface IObjectGraphType vs. the specific type.

[joemcbride]

There is a GetNamedType, which handled the NonNullGraphType, and ListGraphType which works with both Type and ResolvedType cases.

https://github.com/graphql-dotnet/graphql-dotnet/blob/fe0368f15dcea1dd578e83c521b985798e1a2214/src/GraphQL/GraphQLExtensions.cs#L50

https://github.com/graphql-dotnet/graphql-dotnet/blob/fe0368f15dcea1dd578e83c521b985798e1a2214/src/GraphQL/GraphQLExtensions.cs#L96

[Shane32]

so we can just add this:

``` C#
public static bool IsInputObjectType(this Type type)
{
var namedType = type.GetNamedType();
return typeof(IInputObjectGraphType).IsAssignableFrom(namedType);
}

correct?  and maybe this to the InputObjectGraphType class:

``` C#
        private bool ValidateFieldType(FieldType fieldType)
        {
            if (fieldType.ResolvedType != null && fieldType.ResolvedType.IsInputObjectType()) return true;
            if (fieldType.Type != null && fieldType.Type.IsInputObjectType()) return true;
            return false;
        }

[joemcbride]

Looks right to me.

[Shane32]

So the class becomes:

``` C#
public class InputObjectGraphType : ComplexGraphType, IInputObjectGraphType
{
public override FieldType AddField(FieldType fieldType)
{
if (ValidateFieldType(fieldType))
{
throw new ArgumentException("InputObjectGraphType cannot have fields containing a ObjectGraphType.", nameof(fieldType.Type));
}

        return base.AddField(fieldType);
    }

    private static bool ValidateFieldType(FieldType fieldType)
    {
        if (fieldType.ResolvedType != null && fieldType.ResolvedType.IsInputObjectType()) return true;
        if (fieldType.Type != null && fieldType.Type.IsInputObjectType()) return true;
        return false;
    }
}

```

[Shane32]

I'll post a pull request

[Shane32]

well, !ValidateFieldType lol

[Shane32]

Ok well that breaks just about every test since StringGraphType is not an IInputObjectType ...

[joemcbride]

Only need to do the check on the field if it is a IObjectGraphType.

[Shane32]

Makes sense. Do you think there should there be a different extension method like IsInputType which checks for objects, scalars and enums as well? Or not needed?

[Shane32]

perhaps IsInputGraphType

[Shane32]

nevermind, scalars don't have an interface like IScalarGraphType. so I'll just write it as you suggested

[joemcbride]

Have methods like that already, though they target the ResolvedType (in general, in most cases only need to deal with the ResolvedType during execution).

https://github.com/graphql-dotnet/graphql-dotnet/blob/master/src/GraphQL/GraphQLExtensions.cs#L30-L42

[Shane32]

The SchemaBuilder won't execute with the check in place, as GraphQLTypeReference is an IObjectGraphType but not an IInputObjectGraphType. Do you have a recommendation how this should be handled? I'm not really familiar with the SchemaBuilder at all, so it's a bit beyond me. I can post my suggested changes, but they won't pass tests until the issue with the SchemaBuilder is fixed.

[Shane32]

The posted code fixes as much as I can. It then disables the validation (which wasn't working anyway) until someone can review the SchemaBuilder and fixes the issue there.

[joemcbride]

GraphQLTypeReference is a special case. Technically GraphQLTypeReference could refer to anything, including scalars. So GraphQLTypeReference should just be ignored.

[Shane32]

Makes sense. That leaves 3 options;

1) Add an IsProxyType function to check if the type is a GraphQLTypeReference
2) Code IsInputType to also check for GraphQLTypeReference -- but, then if someone were checking for IsInputType == false, it would return a false negative
3) Add explicit code to InputObjectGraphType.ValidateFieldType to check for and ignore GraphQLTypeReference

I recommend option 1, but I'm not sure what the name of the extension function should be. IsProxyType? IsReferenceType? IsProxyGraphType? Perhaps an interface such as IProxyGraphType should also be added to GraphQLTypeReference, but that would be pointless if proxies are hard-coded into the codebase.

[sungam3r]

Technically GraphQLTypeReference could refer to anything, including scalars.

Yes.

So GraphQLTypeReference should just be ignored.

I disagree. GraphQLTypeReference can point to invalid type and should be checked.

[joemcbride]

I disagree. GraphQLTypeReference can point to invalid type and should be checked.

At this point in time, when the field is added, the referenced type may not even be created yet. So that would be impossible to do.

[sungam3r]

I'm against IsProxyType. GraphQLTypeReference is a special internal thing and can be handled explicitly.

[sungam3r]

At this point in time, when the field is added, the referenced type may not even be created yet.

I guess. We need to think.

[joemcbride]

It would also have to have access to the Schema, and it would have to assume that all of the types were currently available in the Schema. Things like the SchemaBuilder don't always have a fully built schema yet when fields are being added to a type.

This check is mostly for convenience to help make sure you don't accidentally do something wrong and enforce the rule as early in the process as possible. At this point in the process you can't do that check with GraphQLTypeReference. You _could_ do that check when the GraphQLTypeReference is resolved to its real type.

[sungam3r]

You could do that check when the GraphQLTypeReference is resolved to its real type.

Then it’s worth doing just that.

[Shane32]

Validating GraphQLTypeReference types would seem to me to be a separate, though related, issue from the validation bug I referenced above. I would suggest creating another issue to track that if indeed invalid GraphQLTypeReferences are not validated once they are resolved to their real type.

[sungam3r]

OK

[sungam3r]

Outdated and fixed in #1436
@Shane32 can you confirm that the problem is resolved?

[Shane32]

I ran a few other tests against it to be sure, and everything looks good to me!