graphql-dotnet / conventions

GraphQL Conventions Library for .NET
MIT License
233 stars 63 forks source link

After updating to 7.0.0, problems with executing operations that include union types #234

Closed floge07 closed 1 year ago

floge07 commented 1 year ago

GraphQL 7.1.1 GraphQL.NewtonsoftJson 7.0.1 GraphQL.Server.Transports.AspNetCore 7.1.1 GraphQL.Conventions 7.0.0

After Updating to the newest Version and after adjusting for the breaking changes by reading the migration guides, I still have a problem with Union types that I'm not able to fix.

When I try to run a query that returns an union type, the ExecutionResult contains this error:

Unable to cast object of type 'GraphQL.Conventions.Adapters.Types.UnionGraphType`1[GqlExampleResult]' to type 'GraphQL.Types.IComplexGraphType'.

The mentioned type looks like this:

[Name("ExampleResult")]
public class GqlExampleResult : Union<GqlExampleInformation, GqlErrorInfo>
{
    public GqlExampleResult(GqlExampleInformation information)
    {
        base.Instance = information;
    }

    public GqlExampleResult(GqlErrorInfo gqlErrorInfo)
    {
        base.Instance = gqlErrorInfo;
    }
}

Second problem: When I use a different query, it works only once, the second execution fails with:

Error executing document. This graph type 'X' has already been initialized. Make sure that you do not use the same instance of a graph type in multiple schemas. It may be so if you registered this graph type as singleton; see https://graphql-dotnet.github.io/docs/getting-started/dependency-injection/ for more info.

X in this case is a possible type of another Union type that was not even used in this query. Edit: This second error seems to have been some configuration mistake that I can't reproduce anymore.

Are these some obvious errors for you, or do I need to dig out more debug data?

sungam3r commented 1 year ago

Regarding the first error - could you provide exception stack trace? Regarding the second error - do you share your graph types across multiple schemas?

floge07 commented 1 year ago

Sadly I have no stack trace for it. This error is already serialized into the ExecutionResult (in the errors list as a message). I am currently trying to debug the conventions package, so maybe I'm able to extract the stack trace somewhere...

I'm afraid I'm not quite sure what exactly "multiple schemas" are... I add Graphql with this:

services.AddGraphQL(configure =>
{
    configure.AddSystemTextJson();
    configure.AddSchema(schema); // result of GraphQLEngine.New().WithQuery<>().....BuildSchema().GetSchema()
    configure.AddDocumentExecuter<GraphQL.Conventions.DocumentExecuter>();
});

So there is only one ISchema registered. But the type 'X' that is mentioned in the error is used in Query, Mutation, and Subscription.

sungam3r commented 1 year ago

I think this is a question of how GraphQLEngine from this project creates schema with graph types inside. I don't know since I don't use it.

This error is already serialized into the ExecutionResult (in the errors list as a message).

Try enable ExecutionOptions.ThrowOnUnhandledException

Shane32 commented 1 year ago

I suggest adding configure.AddErrorInfoProvider(o => o.ExposeExceptionDetails = true); to expose stack traces in the responses.

Shane32 commented 1 year ago

@sungam3r I'm not sure there are any tests in the Conventions project that use IGraphQLBuilder (or Conventions.DocumentExecuter for that matter). Assuming not, we should add some.

@floge07 I don't use the Conventions project either, but I found some tests for unions, which all seemed to run fine. I altered the tests so queries would run twice, with no ill effect either.

floge07 commented 1 year ago

Yes, that worked. So here is the stack trace:

System.InvalidCastException: Unable to cast object of type 'GraphQL.Conventions.Adapters.Types.UnionGraphType`1[GqlExampleResult]' to type 'GraphQL.Types.IComplexGraphType'.
   at GraphQL.Execution.ExecutionStrategy.<CollectFieldsFrom>g__CollectFields|9_0(ExecutionContext context, IGraphType specificType, GraphQLSelectionSet selectionSet, Dictionary`2 fields, ROM[] visitedFragmentNames, Int32 foundFragments) in /_/src/GraphQL/Execution/ExecutionStrategy.cs:line 260
   at GraphQL.Execution.ExecutionStrategy.CollectFieldsFrom(ExecutionContext context, IGraphType specificType, GraphQLSelectionSet selectionSet, Dictionary`2 fields) in /_/src/GraphQL/Execution/ExecutionStrategy.cs:line 232
   at GraphQL.Execution.ExecutionStrategy.GetSubFields(ExecutionContext context, ExecutionNode node) in /_/src/GraphQL/Execution/ExecutionStrategy.cs:line 207
   at GraphQL.ReadonlyResolveFieldContext.get_SubFields() in /_/src/GraphQL/ResolveFieldContext/ReadonlyResolveFieldContext.cs:line 119
   at GraphQL.ResolveFieldContext..ctor(IResolveFieldContext context) in /_/src/GraphQL/ResolveFieldContext/ResolveFieldContext.cs:line 116
   at GraphQL.Conventions.Adapters.ResolutionContext.SetArgument(String name, Object value)
   at GraphQL.Conventions.Handlers.ExecutionFilterAttributeHandler.ResolveArgument(GraphArgumentInfo argument, IResolutionContext resolutionContext)
   at GraphQL.Conventions.Handlers.ExecutionFilterAttributeHandler.Execute(IResolutionContext resolutionContext, Func`2 executor)
   at GraphQL.Conventions.Adapters.FieldResolver.ResolveAsync(IResolveFieldContext context)
   at GraphQL.Execution.ExecutionStrategy.ExecuteNodeAsync(ExecutionContext context, ExecutionNode node) in /_/src/GraphQL/Execution/ExecutionStrategy.cs:line 458
   at GraphQL.Execution.ExecutionStrategy.ExecuteNodeAsync(ExecutionContext context, ExecutionNode node) in /_/src/GraphQL/Execution/ExecutionStrategy.cs:line 486
   at GraphQL.Execution.ParallelExecutionStrategy.ExecuteNodeTreeAsync(ExecutionContext context, ExecutionNode rootNode) in /_/src/GraphQL/Execution/ParallelExecutionStrategy.cs:line 49
   at GraphQL.Execution.ParallelExecutionStrategy.ExecuteNodeTreeAsync(ExecutionContext context, ExecutionNode rootNode) in /_/src/GraphQL/Execution/ParallelExecutionStrategy.cs:line 115
   at GraphQL.Execution.ExecutionStrategy.ExecuteAsync(ExecutionContext context) in /_/src/GraphQL/Execution/ExecutionStrategy.cs:line 27
   at GraphQL.DocumentExecuter.CoreExecuteAsync(ExecutionOptions options) in /_/src/GraphQL/Execution/DocumentExecuter.cs:line 186
   at GraphQL.DocumentExecuter.CoreExecuteAsync(ExecutionOptions options) in /_/src/GraphQL/Execution/DocumentExecuter.cs:line 211
   at GraphQL.Conventions.GraphQLEngine.ExecuteAsync(Object rootObject, String query, String operationName, Inputs variables, IUserContext userContext, IDependencyInjector dependencyInjector, ComplexityConfiguration complexityConfiguration, Boolean enableValidation, Boolean enableProfiling, IEnumerable`1 rules, CancellationToken cancellationToken, IEnumerable`1 listeners)
   at GraphQL.Conventions.DocumentExecuter.ExecuteAsync(ExecutionOptions options)
   at GraphQL.Server.Transports.AspNetCore.GraphQLHttpMiddleware.ExecuteRequestAsync(HttpContext context, GraphQLRequest request, IServiceProvider serviceProvider, IDictionary`2 userContext) in /_/src/Transports.AspNetCore/GraphQLHttpMiddleware.cs:line 420
   at GraphQL.Server.Transports.AspNetCore.GraphQLHttpMiddleware.HandleRequestAsync(HttpContext context, RequestDelegate next, GraphQLRequest gqlRequest) in /_/src/Transports.AspNetCore/GraphQLHttpMiddleware.cs:line 299
   at GraphQL.Server.Transports.AspNetCore.GraphQLHttpMiddleware.InvokeAsync(HttpContext context) in /_/src/Transports.AspNetCore/GraphQLHttpMiddleware.cs:line 177
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Builder.Extensions.UsePathBaseMiddleware.InvokeCore(HttpContext context, String matchedPath, String remainingPath)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
sungam3r commented 1 year ago

I suppose it is line 262 (not 260) : var fieldType = GetFieldDefinition(context.Schema, (IComplexGraphType)specificType, field);

floge07 commented 1 year ago

Is it even correct that the UnionGraphType is getting to that function? I would expect that that function should be called on the possible types it wraps, or not?

Btw, am I assuming correctly that you will look into this? Or do I have to do it?

sungam3r commented 1 year ago

The ideal option is to make PR with a test that demonstrates the problem. Then we will fix the error ourselves. Without repro, it is difficult to help judging by scattered info and will take more time.

Shane32 commented 1 year ago

@floge07 Ditto here. I don't use the conventions project, and while the issue may be something simple, I'm not familiar enough with the codebase to diagnose it without a repro.

floge07 commented 1 year ago

I see, that makes a lot of sense. Will do it tomorrow.

Edit: It works in the test... but why...?! Somewhere is a difference... What am I missing...?!

floge07 commented 1 year ago

I figured it out. It's not the Union in itself, you have to select __typename on the Union to cause this error.

this works: image

this does not: image

test is a Union that returns TypeA or TypeB test2 just returns TypeA

It took me so long to notice this because I never used __typename anywhere. It's automatically injected into the query in every object by the graphql-client we use.

So the question is: Is this something that should be possible (which it was in previous versions) on the server-side or is the client violating some graphql specs by doing this? Depending on the answer I would finish with the unit test or otherwise open an Issue with the graphql-client

sungam3r commented 1 year ago

I didn't manage to reproduce it. See UnionInterfaceTests class that uses __typename a lot.

floge07 commented 1 year ago

What are these edge cases?! It only happens when the method that returns the union has a variable... image

Btw I wrote the test in conventions because I thought that's where the problem is. But I will try to write one in the base project next.

Shane32 commented 1 year ago

@floge07 Thanks for working on this! I've also tried to reproduce the issue without success, so a test -- either in the conventions project and/or in the main project -- would be very helpful in tracking this down.

floge07 commented 1 year ago

I too was not successful in reproducing this problem in the base project. So it seems this is specific to the way the conventions project builds the schema or something, which is annoying as the knowledge of this project seems to be limited...

Edit: Also, we can ignore the second error I wrote about. Can't reproduce it anymore.

Shane32 commented 1 year ago

The bug is reproducible in base GraphQL.NET, when attempting to retrieve IResolveFieldContext.SubFields from within a field resolver for a union graph type field. This Conventions project clones ResolveFieldContext via the copy constructor, which copies the SubFields property (and in so doing, materializes it, triggering the bug).

Shane32 commented 1 year ago

This bug is confirmed to be a GraphQL.NET issue and will be tracked here:

No changes are necessary in this repo. Once the issue is fixed in GraphQL.NET, simply reference the updated nuget package and the issue should be resolved. Please reopen this issue if that is not the case.

floge07 commented 1 year ago

@Shane32 Now that the pr is merged, do you have a time estimate for the release that will contain the fix? Btw, huge thank you all for working through this bug so fast!

Shane32 commented 1 year ago

Waiting on https://github.com/graphql-dotnet/graphql-dotnet/pull/3417 to release 7.2

sungam3r commented 1 year ago

I will review ASAP.