This repository has been archived by the owner. It is now read-only.

/Mvc

Design extensibility for executors #6822

Closed
wants to merge 1 commit into
base: dev
from

Conversation

Reviewers

@pranavkm pranavkm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@rynowak @khellang @pranavkm @dnfclas
@rynowak
Member

rynowak commented Sep 14, 2017

We have all of these executors but they aren't really
documented/supported for extensibility today. This change introduces a
pattern for action result executors so we can make them extensible.

@dnfclas dnfclas added the cla-already-signed label Sep 14, 2017

@rynowak

This comment has been minimized.

Sign in to view
Member

rynowak commented Sep 14, 2017

/cc @pranavkm

@rynowak rynowak requested a review from pranavkm Sep 14, 2017

@rynowak

This comment has been minimized.

Sign in to view
Member

rynowak commented Sep 14, 2017

I did one as an example. If we like the pattern I will go ahead and port the rest.
@rynowak

rynowak reviewed Sep 14, 2017
View changes

src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionResultExecutor.cs Outdated
{
public interface IActionResultExecutor<TResult> where TResult : IActionResult
{
Task ExecuteAsync(ActionContext context, TResult result);

This comment has been minimized.

Sign in to view
Copy link
@rynowak

rynowak Sep 14, 2017

Member

Most of the executors we have already follow this pattern, I'm just adding an interface.

I have big plans for these..... (trails off in a sinister way)
@khellang

This comment has been minimized.

Sign in to view
Contributor

khellang commented Sep 14, 2017

I like it! 😍 I'm already using ObjectResultExecutor for conneg with WebSockets, so this might make it a bit less hacky to use.

The formatting pipeline infrastructure is still a bit too tightly coupled to MVC, IMO. Have you guys thought about decoupling it so it could be used in other MW as well? (I'm guessing aspnet/Diagnostics#346 (comment) is related to this?)
@rynowak

This comment has been minimized.

Sign in to view
Member

rynowak commented Sep 14, 2017

Have you guys thought about decoupling it so it could be used in other MW as well? (I'm guessing aspnet/Diagnostics#346 (comment) is related to this?)

Yes, but it's not planned for right now

The formatting pipeline infrastructure is still a bit too tightly coupled to MVC, IMO

conneg is one of a few things in MVC that has an opinion at all. There's no technical need for it to be coupled, but we'd have to re-examine those decisions.
@pranavkm

pranavkm reviewed Sep 14, 2017
View changes

src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IActionResultExecutor.cs Outdated
namespace Microsoft.AspNetCore.Mvc.Infrastructure
{
public interface IActionResultExecutor<TResult> where TResult : IActionResult

This comment has been minimized.

Sign in to view
Copy link
@pranavkm

pranavkm Sep 14, 2017

Member

Is there any value in making this contravariant? Maybe for scenarios like IActionResultExecutor<NotFoundResult> = new StatusCodeResultExecutor();

This comment has been minimized.

Sign in to view
Copy link
@rynowak

rynowak Sep 14, 2017

Member

Hmm, I don't think it can hurt.

@pranavkm pranavkm referenced this pull request Sep 14, 2017

Closed

Introduce opinionated API defaults. #6807

@rynowak rynowak force-pushed the rynowak/executor branch 3 times, most recently from 602dd8a to 2dcc293 Sep 20, 2017

@rynowak

This comment has been minimized.

Sign in to view
Member

rynowak commented Sep 21, 2017

@pranavkm ping

@rynowak rynowak force-pushed the rynowak/executor branch 4 times, most recently from 410e738 to 82c44cb Sep 21, 2017

@pranavkm

pranavkm reviewed Sep 21, 2017
View changes

Looks good

src/Microsoft.AspNetCore.Mvc.Core/RedirectResult.cs
public override void ExecuteResult(ActionContext context)
{
if (context == null)
{
throw new ArgumentNullException(nameof(context));
}
var executor = context.HttpContext.RequestServices.GetRequiredService<RedirectResultExecutor>();
executor.Execute(context, this);
var services = context.HttpContext.RequestServices;

This comment has been minimized.

Sign in to view
Copy link
@pranavkm

pranavkm Sep 21, 2017

Member

Could we have these call the executor and block on it? In the default case, it should be fine since the executor is entirely synchronous.

This comment has been minimized.

Sign in to view
Copy link
@rynowak

rynowak Sep 21, 2017

Member

I don't want to put a .Result in the code. You could implement these interfaces and do some arbitrary stuff
src/Microsoft.AspNetCore.Mvc.Core/breakingchanges.netcore.json Outdated
@@ -145,5 +145,25 @@
"TypeId": "public class Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.DefaultModelMetadata : Microsoft.AspNetCore.Mvc.ModelBinding.ModelMetadata",
"MemberId": "public override Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.IModelBindingMessageProvider get_ModelBindingMessageProvider()",
"Kind": "Removal"
},
{

This comment has been minimized.

Sign in to view
Copy link
@pranavkm

pranavkm Sep 21, 2017

Member

FYI @javiercn
src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewResult.cs
{
await executor.ExecuteAsync(context, view, this);
}
var executor = context.HttpContext.RequestServices.GetRequiredService<IActionResultExecutor<ViewResult>>();

This comment has been minimized.

Sign in to view
Copy link
@pranavkm

pranavkm Sep 21, 2017

Member

Nice
src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/ViewResultExecutor.cs Outdated
@@ -176,6 +178,29 @@ public virtual Task ExecuteAsync(ActionContext actionContext, IView view, ViewRe
viewResult.StatusCode);
}
/// <inheritdoc />
public async Task ExecuteAsync(ActionContext context, ViewResult result)

This comment has been minimized.

Sign in to view
Copy link
@pranavkm

pranavkm Sep 21, 2017

Member

Could we remove the now unused overload since the type was in .Internal?

This comment has been minimized.

Sign in to view
Copy link
@rynowak

rynowak Sep 21, 2017

Member

Sure
src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/ContentResultExecutor.cs
using Microsoft.Extensions.Logging;
namespace Microsoft.AspNetCore.Mvc.Internal
namespace Microsoft.AspNetCore.Mvc.Infrastructure

This comment has been minimized.

Sign in to view
Copy link
@pranavkm

pranavkm Sep 21, 2017

Member

Is the namespace change to facilitate subclassing?

This comment has been minimized.

Sign in to view
Copy link
@rynowak

rynowak Sep 21, 2017

Member

Yes
@rynowak
Design extensibility for executors 
We have all of these executors but they aren't really
documented/supported for extensibility today. This change introduces a
pattern for action result executors so we can make them extensible.
Loading status checks…
d640115

@rynowak rynowak force-pushed the rynowak/executor branch from d50ab0d to d640115 Sep 22, 2017

@rynowak

This comment has been minimized.

Sign in to view
Member

rynowak commented Sep 22, 2017

3871260

@rynowak rynowak closed this Sep 22, 2017

@rynowak rynowak deleted the rynowak/executor branch Sep 22, 2017

@khellang khellang referenced this pull request Dec 11, 2017

Closed

Move various parsers/formatters into HttpAbstractions #916

@aspnet-hello aspnet-hello referenced this pull request Jan 1, 2018

Open

Move various parsers/formatters into HttpAbstractions #2324

@khellang khellang referenced this pull request Jan 12, 2018

Closed

Provide a more in depth example of writing responses in middleware #7238

@Rick-Anderson Rick-Anderson referenced this pull request Apr 28, 2018

Open

doc IActionResultExecutor<T> #6115

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.