Add IConfigureExecution

[Shane32]

This allows for 'middleware' or 'plugin' behavior to the document executer. This will be immediately useful for appending Apollo Tracing results, as well as implementation of Automatic Persisted Queries.

IConfigureExecution works similarly to "Apollo Link"s as seen below:

Within our library, these components are implemented similarly to this:

Apollo	GraphQL.NET
GraphQL operation	IDocumentExecuter
Link	IConfigureExecution
Terminating Link	GraphQLHttpMiddleware

(In the diagram above, Apollo Links are shown in the context of a client, but same design applies to a server.)

[sungam3r]

Suggested change

	public static IGraphQLBuilder ConfigureExecution(this IGraphQLBuilder builder, Func<Func<ExecutionOptions, Task<ExecutionResult>>, ExecutionOptions, Task<ExecutionResult>> action)
	public static IGraphQLBuilder ConfigureExecution(this IGraphQLBuilder builder, Func<ExecutionOptions, ExecutionDelegate, Task<ExecutionResult>> action)

[sungam3r]

@Shane32 Compare this with

 /// <summary>
        /// Adds a middleware delegate defined in-line to the application's request pipeline.
        /// If you aren't calling the next function, use <see cref="RunExtensions.Run(IApplicationBuilder, RequestDelegate)"/> instead.
        /// </summary>
        /// <param name="app">The <see cref="IApplicationBuilder"/> instance.</param>
        /// <param name="middleware">A function that handles the request and calls the given next function.</param>
        /// <returns>The <see cref="IApplicationBuilder"/> instance.</returns>
        public static IApplicationBuilder Use(this IApplicationBuilder app, Func<HttpContext, RequestDelegate, Task> middleware)
        {
            return app.Use(next => context => middleware(context, next));
        }

ConfigureExecution <-> Use
IGraphQLBuilder <-> IApplicationBuilder
action <-> middleware
ExecutionOptions <-> HttpContext
ExecutionDelegate <-> RequestDelegate
Task<ExecutionResult> <-> Task

Suggestions from me:

1. Maybe rename method name and argument name to align with ASP.NET stuff? I'm 50/50.
2. Add other overloads from ASP.NET:
   public static IApplicationBuilder Use(this IApplicationBuilder app, Func<HttpContext, Func<Task>, Task> middleware)
   and
   IApplicationBuilder Use(Func<RequestDelegate, RequestDelegate> middleware);
3. Add docs mentioning that GraphQL.NET document execution pipeline follows the very same design as ASP.NET http request pipeline.

[Shane32]

ASP.NET Core docs discourage the use of the first overload you suggested, so I would suggest we simply skip that one. The second overload requires a change in design to implement: it's an application builder, so instead of IConfigureExecution we would have IExecutionBuilder (or something named like that), where the interface method is ExecutionDelegate BuildExecution(ExecutionDelegate next).

Do you think it's worth it? Ideally, the start of the execution would initialize an ExecutionContext (similar to HttpContext), and everything - document parsing, document caching, document listeners, document validation, and even the execution strategy selection and execution - would be based on installable middleware, similar to ASP.NET Core. However, we would need a place to configure the builder pipeline in case of branches, as pulling a list from DI would be insufficient. I feel like that's too big of a change to tackle for v5, although it is worth considering for v6.

I would suggest we leave this a bit simpler for v5, tackling the ability to add middleware around the existing document executer, and if we wish to rewrite the execution pipeline from scratch, we do so for v6.

What do you think? If you still think we should make the changes you proposed, fine, but can you be a little more specific as to the interface definition that should be added, and builder methods to be added?

Example interface:

// now:
public interface IConfigureExecution
{
    Task<ExecutionResult> ExecuteAsync(ExecutionOptions options, ExecutionDelegate next);
}
public static IGraphQLBuilder ConfigureExecution(this IGraphQLBuilder builder, Func<ExecutionOptions, ExecutionDelegate, Task<ExecutionResult>> action);

// suggestion???
public interface IExecutionBuilder
{
    ExecutionDelegate Build(ExecutionDelegate next);
}
public static IGraphQLBuilder ConfigureExecution(this IGraphQLBuilder builder, Func<ExecutionOptions, ExecutionDelegate, Task<ExecutionResult>> middleware);
public static IGraphQLBuilder ConfigureExecution(this IGraphQLBuilder builder, Func<ExecutionDelegate, ExecutionDelegate> middleware);

Again, personally I think I'd just leave it as shown in this PR for v5.

One other option: we could leave the interface as-is and add the extra configuration method like shown below, but I don't like it - it seems like a hack.

public static IGraphQLBuilder ConfigureExecution(this IGraphQLBuilder builder, Func<ExecutionDelegate, ExecutionDelegate> middleware)
{
    builder.Services.Register<IConfigureExecution>(new ConfigureExecution((options, next) => middleware(next)(options)));
    return builder;
}

[Shane32]

@sungam3r bump - if we answer this one question, we may be able to merge this now or shortly.

[sungam3r]

I agree to postpone redesign to v6.

[sungam3r]

#3131

[sungam3r]

This is a very-very useful feature and at the same time it is quite simple to implement and support. 👍

[Shane32]

@sungam3r Thanks for the comments. They all look good. I bumped the apollo tracing method to a separate PR so we can discuss the best way to implement that. Also see above for my comments on IConfigureExecutionOptions. I think this PR is ready to merge now unless you have any other comments.

[sungam3r]

Only this one left - #3121 (comment)

[Shane32]

Only this one left - #3121 (comment)

Whoops; missed that. Please see reply above.

[codecov-commenter]

Codecov Report

Merging #3121 (bbf864a) into master (81730d2) will decrease coverage by 0.06%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master    #3121      +/-   ##
==========================================
- Coverage   84.38%   84.32%   -0.07%     
==========================================
  Files         362      363       +1     
  Lines       15900    15944      +44     
  Branches     2583     2580       -3     
==========================================
+ Hits        13417    13444      +27     
- Misses       1867     1883      +16     
- Partials      616      617       +1     

Impacted Files	Coverage Δ
src/GraphQL/DI/IConfigureExecution.cs	0.00% <0.00%> (ø)
src/GraphQL/GraphQLBuilderExtensions.cs	96.04% <0.00%> (-0.91%)	⬇️
src/GraphQL/Execution/DocumentExecuter.cs	89.23% <95.83%> (-0.77%)	⬇️
src/GraphQL/DI/IConfigureExecutionOptions.cs	86.66% <100.00%> (+23.03%)	⬆️
...L/Instrumentation/ApolloTracingDocumentExecuter.cs	76.92% <100.00%> (-23.08%)	⬇️
src/GraphQL/Types/Schema.cs	78.99% <0.00%> (-0.92%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81730d2...bbf864a. Read the comment docs.