graphql-dotnet / relay

A toolset for creating Relay.js compatible GraphQL servers in dotnet.
MIT License
75 stars 29 forks source link

When implementing AsyncNodeGraphType ExecutionStrategy throws InvalidOperationException #171

Open wtorricos opened 2 years ago

wtorricos commented 2 years ago

I found that when implementing AsyncNodeGraphType the ExecutionStrategy class will throw an InvalidOperationException since the following call returns null: https://github.com/graphql-dotnet/graphql-dotnet/blob/ed720f46e26704ed457b723801637d9d726f45bd/src/GraphQL/Execution/ExecutionStrategy.cs#L636

The problem seems to be that the execution strategy is not awaiting the result and is trying to find the GraphType of Task instead of just T.

Our implementation broke after we upgraded GraphQL, here you can see the version of our packages:

<PackageReference Include="GraphQL" Version="5.3.3" />
<PackageReference Include="GraphQL.DataLoader" Version="5.3.3" />
<PackageReference Include="GraphQL.MemoryCache" Version="5.3.3" />
<PackageReference Include="GraphQL.MicrosoftDI" Version="5.3.3" />
<PackageReference Include="GraphQL.SystemTextJson" Version="5.3.3" />
<PackageReference Include="GraphQL.Relay" Version="0.8.0" />

Hope this gives you enough information to fix it

Shane32 commented 2 years ago

Most likely you need to call FieldAsync rather than Field for some of your field definitions.

See: https://graphql-dotnet.github.io/docs/migrations/migration5#32-valuetask-execution-pipeline-support-changes

Shane32 commented 2 years ago

I'd further guess that the field type in this case is an interface or union, and what happened is that IsTypeOf returned false for the Task<T> returned value, causing the type to be null. With this change here in proposed PR graphql-dotnet/graphql-dotnet#3270 : https://github.com/graphql-dotnet/graphql-dotnet/pull/3270#discussion_r939766433 the code will use the interface type and not cause a null reference exception. Then you would at least see a better exception than what is seen currently, but it would not solve your issue.

Without seeing more of your code, this is just a guess, of course.

wtorricos commented 2 years ago

Thank you for the reply.

Regarding FieldAsync, I don't think that's the problem as even some really simple types are failing, we have many like the one below:

public sealed class CharacterType : AsyncNodeGraphType<CharacterModel>
{
    public CharacterType()
    {
        Name = "Character";

        Id(characterModel => characterModel.Id);

        Field<NonNullGraphType<StringGraphType>>()
            .Name("name")
            .Resolve(context => context.Source.Name ?? "None");
    }

    public override async Task<CharacterModel> GetById(IResolveFieldContext<object> context, string id)
    {
        var characterService = context.RequestServices.GetRequiredService<CharacterService>();
        Guid characterId = Guid.Parse(id);
        return characterService.GetByIdAsync(characterId, context.CancellationToken);
    }
}

The workaround that is working at the moment is to extend NodeGraphType instead of AsyncNodeGraphType

Shane32 commented 2 years ago

Right I see. AsyncNodeGraphType<T> (which is in the relay project) inherits from NodeGraphType<Task<T>>, and when you call Id it calls Field instead of FieldAsync. The relay project's AsyncNodeGraphType needs to be fixed so that it does not call Field but instead calls FieldAsync.

Shane32 commented 2 years ago

See faulty code here: (when used by the derived AsyncNodeGraphType -- the code is fine as NodeGraphType)

https://github.com/graphql-dotnet/relay/blob/5ad6b1c6367a2cfd83894a1f2f1f761a62c36b4d/src/GraphQL.Relay/Types/NodeGraphType.cs#L91..L98

There's nobody that actively works on this project, but if you'd like to open a pull request @tico321 I would be happy to review it, and when merged, issue an updated release.

wtorricos commented 2 years ago

Thank you for the fast response, I'll try to take a look over the weekend

wtorricos commented 2 years ago

Supporting NodeGraphType as well as AsyncNodeGraphtype seems to be a little bit trickier than what I thought, using FieldAsync wan't enough, but I found that awaiting the result in QueryGraphType fixes the problem. https://github.com/graphql-dotnet/relay/blob/7e5d8e4a6365194b68f282239b31f36137ace5aa/src/GraphQL.Relay/Types/QueryGraphType.cs#L24 Since I'm only using AsyncNodeGraphType I'm going to use the following class in my project until I can find a way to make it work with both Node and AsyncNode.

public class AsyncQueryGraphType : ObjectGraphType
{
    public AsyncQueryGraphType()
    {
        Name = "Query";

        Field<NodeInterface>()
            .Name("node")
            .Description("Fetches an object given its global Id")
            .Argument<NonNullGraphType<IdGraphType>>("id", "The global Id of the object")
            .ResolveAsync(ResolveObjectFromGlobalId);
    }

    private async Task<object> ResolveObjectFromGlobalId(IResolveFieldContext<object> context)
    {
        var globalId = context.GetArgument<string>("id");
        var parts = Node.FromGlobalId(globalId);
        var node = context.Schema.AllTypes[parts.Type];
        var getById = node!.GetType().GetMethod("GetById");

        dynamic awaitable = getById!.Invoke(node, new object[] { context, parts.Id });
        await awaitable!;
        return awaitable.GetAwaiter().GetResult();
    }
}

Having said that please let me know if you have any suggestions that may work and I'll take a look when I have some time.