conventions icon indicating copy to clipboard operation
conventions copied to clipboard

"GraphQL.ExecutionError: Null value provided for non-nullable argument 'field1'" - Even though it is actually not null.

Open floge07 opened this issue 1 year ago • 9 comments

Hello again,

after I worked around #267 and then rolled out the new version, I discovered more obscure problems. Some operations suddenly fail with the error mentioned in the title. No changes to the actual query or backend types were made.

The "ensure non null" validation seems to throw some new false positives. If I remove the NonNull<> wrapper from that field it executes again, and there actually is a value!

This only happens on some operations. The thing that stands out about them is their complexity. Looking at the request body, it's an operation with variables, which calls a query with one of the variables, and includes a fragment, which also uses a variable on one of it's methods.

An example:

query getStuffAndMore($var1: [String!]!, $var2: [String!]!, $var3: String) {
    getStuff(path: $var2, filter: $var3) {
        ...Fragment1
        ...on ErrorInfo {
            errorMessage
            errorCode
            __typename
        }
        __typename
    }
}

fragment Fragment1 on UnionType1 {
    more(field1: $var1)
}

I worked around it for now by removing the NonNull<> wrapper on the effected methods on the server side and instead added ArgumentNullException.ThrowIfNull() to the first line.

GraphQL: 8.2.1 GraphQL.Server.Transports.AspNetCore: 8.2.0 GraphQL.Convetions: 8.0.0

floge07 avatar Dec 13 '24 10:12 floge07

This one is probably a bug in GraphQL.NET’s new argument processing and not due to the use of the conventions project. Probably has to do with the use of the argument within the fragment. I’d be happy to fix it if we can reproduce the problem (with or without the conventions project).

Shane32 avatar Dec 13 '24 13:12 Shane32

Attempting to replicate the test, can you see if this schema would somewhat match what you're attempting?

# The Query type
type Query {
    getStuff(path: [String!]!, filter: String): GetStuffResponse!
}

# Union type definition used in the fragment
union GetStuffResponse = UnionType1 | ErrorInfo

# Union member type with the fragment field
type UnionType1 {
    more(field1: [String!]!): String!
}

# Error information type
type ErrorInfo {
    errorMessage: String!
    errorCode: String!
}

If getStuff returns a union, then it can only ever return either the UnionType1 object graph type, or the ErrorInfo type, correct?

Shane32 avatar Dec 13 '24 14:12 Shane32

So far I cannot reproduce the problem within GraphQL.NET alone that fails using the above sample query. My code so far:

using GraphQL.Types;
using Microsoft.Extensions.DependencyInjection;

namespace GraphQL.Tests.Bugs;

public class Bug268
{
    [Theory]
    [InlineData(
        """{ "var1": ["test"], "var2": ["test"], "var3": "test" }""",
        """{ "data": { "getStuff": { "more": "test", "__typename": "UnionType1" } } }""")]
    [InlineData(
        """{ "var1": ["test432"], "var2": ["test"], "var3": "test" }""",
        """{ "data": { "getStuff": { "more": "test432", "__typename": "UnionType1" } } }""")]
    [InlineData(
        """{ "var1": [], "var2": [], "var3": null }""",
        """{ "data": { "getStuff": { "more": null, "__typename": "UnionType1" } } }""")]
    [InlineData(
        """{ "var1": ["test"], "var2": ["test"], "var3": "error" }""",
        """{ "data": { "getStuff": { "errorMessage": "Test1", "errorCode": "TestCode", "__typename": "ErrorInfo" }}}""")]
    public async Task Sample_Matrix(string variablesJson, string expectedResponseJson)
    {
        var serviceCollection = new ServiceCollection();
        serviceCollection.AddGraphQL(b => b
            .AddSchema<MySchema>()
            .AddGraphTypes()
        );
        using var service = serviceCollection.BuildServiceProvider();
        var schema = service.GetRequiredService<MySchema>();
        var actualResponseJson = await schema.ExecuteAsync(_ =>
        {
            _.Query = """
                query getStuffAndMore($var1: [String!]!, $var2: [String!]!, $var3: String) {
                    getStuff(path: $var2, filter: $var3) {
                        ...Fragment1
                        ... on ErrorInfo {
                            errorMessage
                            errorCode
                            __typename
                        }
                        __typename
                    }
                }

                fragment Fragment1 on UnionType1 {
                    more(field1: $var1)
                }
                """;
            _.Variables = variablesJson.ToInputs();
        });
        actualResponseJson.ShouldBeCrossPlatJson(expectedResponseJson);
    }

    public class MySchema : Schema
    {
        public MySchema(IServiceProvider provider) : base(provider)
        {
            Query = provider.GetRequiredService<MyQuery>();
        }
    }

    public class MyQuery : ObjectGraphType
    {
        public MyQuery()
        {
            Name = "Query";
            Field<GetStuffResponseType>("getStuff")
                .Argument<NonNullGraphType<ListGraphType<NonNullGraphType<StringGraphType>>>>("path")
                .Argument<StringGraphType>("filter")
                .Resolve(context => context.GetArgument<string>("filter") == "error"
                    ? new ErrorInfo { ErrorMessage = "Test1", ErrorCode = "TestCode" }
                    : new UnionType1());
        }
    }

    public class GetStuffResponseType : UnionGraphType
    {
        public GetStuffResponseType()
        {
            Name = "GetStuffResponse";
            Type<UnionType1Type>();
            Type<ErrorInfoType>();
        }
    }

    public class UnionType1Type : ObjectGraphType<UnionType1>
    {
        public UnionType1Type()
        {
            Field<StringGraphType>("more")
                .Argument<NonNullGraphType<ListGraphType<NonNullGraphType<StringGraphType>>>>("field1")
                .Resolve(context => context.GetArgument<List<string>>("field1").FirstOrDefault());
        }
    }

    public class UnionType1
    {
    }

    public class ErrorInfoType : ObjectGraphType<ErrorInfo>
    {
        public ErrorInfoType()
        {
            Field(x => x.ErrorMessage);
            Field(x => x.ErrorCode);
        }
    }

    public class ErrorInfo
    {
        public string ErrorMessage { get; set; }
        public string ErrorCode { get; set; }
    }
}

Shane32 avatar Dec 13 '24 14:12 Shane32

I'm looking at the Conventions project and I see ResolutionContext.cs:41 throws an error message of the exact type you described. So this problem may be specific to the Conventions project.

Shane32 avatar Dec 13 '24 14:12 Shane32

If you can write a sample that reproduces the bug using the Conventions project, I can look further at this for you.

Shane32 avatar Dec 13 '24 15:12 Shane32

Thanks for the support, and yeah, I will for sure do that. But sadly I'm on vacation starting next week, so it will probably take a few weeks to get back to this.

floge07 avatar Dec 13 '24 16:12 floge07

So I spend some time reproducing this error.

It seems when I simplified my schema to include in the issue I removed the actual problematic aspect from it, which was a nested field on child items of the same type.

I wrote a test to reproduce the exact error message -> https://github.com/graphql-dotnet/conventions/commit/e38bcf0f39555a5ca457d1d60dc5cb1e5b252b41

Interestingly the first variant I did by copying another test, which looked like this:

var schema = Schema<SchemaTypeWithDecimal>();
var result = await schema.ExecuteAsync((e) => e.Query = "query { test }");

resulted in a new error message: Error trying to resolve field 'field'.

That is why I also wrote a variant using the GraphQLEngine, which is how we currently execute graphql requests. That way the original error message occurs.

Though this created a new question for me of whether we even use the Conventions project correctly...

Well in any case, while testing I observed another new error: When doing the exact same schema and query but instead of NonNull<List<string>>, just simply NonNull<string> it results in: Constructor on type 'GraphQL.Conventions.NonNull`1[[System.String, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]' not found.

floge07 avatar Jan 07 '25 16:01 floge07

ExecutionFilterAttributeHandler.cs:73 calls resolutionContext.GetArgument(argument). ResolutionContext.cs:27 pulls the argument from FieldContext.Arguments and the value is a NonNull<List<string>>. But ExecutionFilterAttributeHandler.cs:74 calls Wrapper.Wrap and eventually gets to CollectionWrapper.cs:20 which cannot coerce NonNull<List<string>> to IEnumerable and so returns null instead of iterating and wrapping the list or whatever it's supposed to do.

@tlil What's supposed to be happening here? Is the problem within CollectionWrapper or should the FieldContext.Arguments have never contained the list wrapped with NonNull<T> to begin with? I find it odd that the argument already has a NonNull<T> wrapper and yet the handler is calling the Wrapper.Wrap method, as if perhaps it was already wrapped.

Here's the branch with the test added:

  • https://github.com/floge07/dotnet-graphql-conventions/tree/test-268-null-false-positive

Here's the link to the new test:

  • https://github.com/floge07/dotnet-graphql-conventions/blob/test-268-null-false-positive/test/GraphQL.Conventions.Tests/Execution/SchemaExecutionTests.cs#L36

Shane32 avatar Jan 07 '25 16:01 Shane32

Note: I wouldn't be surprised if this bug is partially due to GraphQL.NET v8, which parses all variables once upon initial execution rather than upon demand. For example, maybe the variables get parsed and 'wrapped' according to the variable definition, but the Conventions project is expecting the raw data and to rewrap them according to the field's definition (which may be compatible but different). So perhaps the variables need to be unwrapped before being rewrapped for the field's definition or something. I guess this test doesn't use variables, but something like that. (I'm just sorta guessing here, and could be way off.)

Shane32 avatar Jan 07 '25 17:01 Shane32