dotnet/aspnetcore

Add IResult implementations for more IActionResults

halter73 opened this issue ยท 17 comments

Currently, only JsonResult and StatusCodeResult implement both an IActionResult and IResult. This means there are a bunch of remaining built-in IActionResults implementations that have no IResult equivalent.

  • ContentResult
  • VirtualFileResult
  • PhysicalFileResult
  • FileStreamResult
  • FileContentResult
  • RedirectResult
  • LocalRedirectResult
  • ChallengeResult
  • ForbidResult
  • SignIn/OutResult
  • RedirectToAction/PageResult?
  • ObjectResult?

ObjectResult is the big IActionResult that probably doesn't have an easy IResult implementation. I feel this requires some support for content-negotiation, but maybe we can just assume JSON.

@pranavkm should we bother with ActionResult<T>?

We should think how ActionResult<T> plays with the OpenAPI definition. #30662 @jkotalik

We'll strike out the action results that don't make sense once we investigate their feasibility

For anyone looking at this because of the help-wanted label, feel free to take a subset of these types. I have a feeling something like RedirectResult will be easier to chew off than ObjectResult.

Thanks for contacting us.

We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

Sounds interesting, I will take the easiest ones ๐Ÿ˜„

I would like to try ContentResult or something simple.

@halter73 @davidfowl I would like to confirm the approach I am taking is how you were envisioning the implementation of these new IActionResults, could you take a look at #32647?

If the approach its fine I will continue with the rest but there is a question, similar to MVC we will need to register singleton of each ActionResult like this

services.TryAddSingleton<OutputFormatterSelector, DefaultOutputFormatterSelector>();
services.TryAddSingleton<IActionResultExecutor<ObjectResult>, ObjectResultExecutor>();
services.TryAddSingleton<IActionResultExecutor<PhysicalFileResult>, PhysicalFileResultExecutor>();
services.TryAddSingleton<IActionResultExecutor<VirtualFileResult>, VirtualFileResultExecutor>();
services.TryAddSingleton<IActionResultExecutor<FileStreamResult>, FileStreamResultExecutor>();
services.TryAddSingleton<IActionResultExecutor<FileContentResult>, FileContentResultExecutor>();
services.TryAddSingleton<IActionResultExecutor<RedirectResult>, RedirectResultExecutor>();
services.TryAddSingleton<IActionResultExecutor<LocalRedirectResult>, LocalRedirectResultExecutor>();
services.TryAddSingleton<IActionResultExecutor<RedirectToActionResult>, RedirectToActionResultExecutor>();
services.TryAddSingleton<IActionResultExecutor<RedirectToRouteResult>, RedirectToRouteResultExecutor>();
services.TryAddSingleton<IActionResultExecutor<RedirectToPageResult>, RedirectToPageResultExecutor>();
services.TryAddSingleton<IActionResultExecutor<ContentResult>, ContentResultExecutor>();
services.TryAddSingleton<IActionResultExecutor<JsonResult>, SystemTextJsonResultExecutor>();
services.TryAddSingleton<IClientErrorFactory, ProblemDetailsClientErrorFactory>();
services.TryAddSingleton<ProblemDetailsFactory, DefaultProblemDetailsFactory>();

Where do you think is the best place to add these new addings in this particular case?

@halter73 @davidfowl can I try the IResult implementation for ObjectResult ?

That's the hard one. Maybe the file results?

That's the hard one. Maybe the file results?

Why not ๐Ÿ™‚. I'll submit PR on that.
Thanks @davidfowl

@pranavkm suggested that we should support ObjectResult by doing JSON and if another content type is specified then fail.

I'm implementing the IResult on the RedirectToActionResult. I think it's best to wait for #33843 to be merged to avoid conflicts, right?

I think that's a good idea @ilkayilknur

@ilkayilknur I think the plan is to not add any more IResult to MVC's action result types.

Thanks for the update @pranavkm

The remaining work was addressed by #33843