Fix UnhandledError when resolving top-level subscription field

[sungam3r]

Also see #3213

Yesterday one of our team members found a bug when using field middleware and calling subscription.

Before fix:

    Shouldly.ShouldAssertException : errors
        should be null but was
    [GraphQL.Execution.UnhandledError: Error trying to resolve field 'newMessageContent'.
     ---> System.InvalidOperationException: Expected to find property or method 'newMessageContent' on type 'String' but it does not exist.
       at GraphQL.Resolvers.NameFieldResolver.CreateResolver(Type target, String name) in C:\_GITHUB_\graphql-dotnet\src\GraphQL\Resolvers\NameFieldResolver.cs:line 83
       at GraphQL.Resolvers.NameFieldResolver.<>c.<Resolve>b__6_0(ValueTuple`2 t) in C:\_GITHUB_\graphql-dotnet\src\GraphQL\Resolvers\NameFieldResolver.cs:line 38
       at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
       at GraphQL.Resolvers.NameFieldResolver.Resolve(IResolveFieldContext context, String name) in C:\_GITHUB_\graphql-dotnet\src\GraphQL\Resolvers\NameFieldResolver.cs:line 38
       at GraphQL.Resolvers.NameFieldResolver.ResolveAsync(IResolveFieldContext context) in C:\_GITHUB_\graphql-dotnet\src\GraphQL\Resolvers\NameFieldResolver.cs:line 28
       at GraphQL.Tests.Subscription.SubscriptionTests.MyMiddleware.ResolveAsync(IResolveFieldContext context, FieldMiddlewareDelegate next) in C:\_GITHUB_\graphql-dotnet\src\GraphQL.Tests\Subscription\SubscriptionTests.cs:line 58
       at GraphQL.Tests.Subscription.SubscriptionTests.<>c__DisplayClass3_1.<SubscribeToContent>b__1(IResolveFieldContext context) in C:\_GITHUB_\graphql-dotnet\src\GraphQL.Tests\Subscription\SubscriptionTests.cs:line 84
       at GraphQL.Resolvers.FuncFieldResolver`1.ResolveAsync(IResolveFieldContext context) in C:\_GITHUB_\graphql-dotnet\src\GraphQL\Resolvers\FuncFieldResolver.cs:line 40
       at GraphQL.Execution.ExecutionStrategy.ExecuteNodeAsync(ExecutionContext context, ExecutionNode node) in C:\_GITHUB_\graphql-dotnet\src\GraphQL\Execution\ExecutionStrategy.cs:line 477
       --- End of inner exception stack trace ---]

ApplyMiddleware method sets NameFieldResolver.Instance for all fields, even for subscriptions. I have no ideas why we haven't found it before.

@Shane32 I need your help. I do not like current implementation. The main problem is complex == schema?.Subscription comparison. SchemaTypes has no reference to schema but in fact only Schema calls all methods on SchemaTypes, so I just added two methods and delegated work to them. I would like to change design - either adding reference to Schema or in some other way.

[Shane32]

How about this?

var inner = field.Resolver ?? ((field.StreamResolver != null) ? SourceFieldResolver.Instance : NameFieldResolver.Instance));

[Shane32]

ApplyMiddleware method sets NameFieldResolver.Instance for all fields, even for subscriptions. I have no ideas why we haven't found it before.

I believe the schema-first and auto-registering code both set the Resolver property correctly. It is only code-first that would have an issue.

Now, it is not mandatory that the SourceFieldResolver is used. The event data from the IObservable is passed in as the root value at SubscriptionExecutionStrategy.cs:182. However, setting a custom resolver could apply a transform to this data. It is much more intuitive to apply the transform to the IObservable, and as far as I know this is how subscriptions are typically used. NameFieldResolver would never be used.

Also if we look at the FieldSubscribe and FieldSubscribeAsync code, we note that we have a todo comment to remove the resolver argument. We should probably remove it in v8. Perhaps for now we should set it to SourceFieldResolver if it is null (although you are doing this through the middleware code). People can manually set the Resolver property if they need to override SourceFieldResolver.Instance, which I believe would be rare.

Also within those same two methods, note that the resolve is typed as Func<IResolveFieldContext<TSourceType>, object?>. Specifically consider TSourceType. This is most definitely incorrect; it would match the IObservable event data type, not the type of the graph type's source on which the field was added. Since few(??) people use the ExecutionOptions.Root property, and hence are unlikely to strongly type their root Subcription type, TSourceType is probably almost always object, matching IObservable<object> used here.

[Shane32]

FieldSubscribe and FieldSubscribeAsync are already deprecated in favor of the field builder methods, so no need to mess with those methods now.

We could add FieldType.Resolver = SourceFieldResolver.Instance; to the field builder ResolveStream methods....

[sungam3r]

Thanks. I would like to publish v7.3.1, OK?

[codecov-commenter]

Codecov Report

Merging #3568 (a0faad3) into master (2c6ebdc) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #3568      +/-   ##
==========================================
- Coverage   83.83%   83.83%   -0.01%     
==========================================
  Files         381      381              
  Lines       16840    16839       -1     
  Branches     2697     2697              
==========================================
- Hits        14118    14117       -1     
  Misses       2076     2076              
  Partials      646      646              

Impacted Files	Coverage Δ
src/GraphQL/Execution/ExecutionStrategy.cs	85.49% <100.00%> (-0.04%)	⬇️
src/GraphQL/Types/Collections/SchemaTypes.cs	85.33% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[Shane32]

Thanks. I would like to publish v7.3.1, OK?

Sure