Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IResult implementations for more IActionResults #32565

Closed
8 of 12 tasks
halter73 opened this issue May 11, 2021 · 17 comments
Closed
8 of 12 tasks

Add IResult implementations for more IActionResults #32565

halter73 opened this issue May 11, 2021 · 17 comments
Assignees
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-actions Controller-like actions for endpoint routing
Milestone

Comments

@halter73
Copy link
Member

halter73 commented May 11, 2021

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.

@halter73 halter73 added enhancement This issue represents an ask for new feature or an enhancement to an existing one area-runtime feature-minimal-actions Controller-like actions for endpoint routing labels May 11, 2021
@halter73 halter73 added help wanted Up for grabs. We would accept a PR to help resolve this issue and removed help wanted Up for grabs. We would accept a PR to help resolve this issue labels May 11, 2021
@davidfowl
Copy link
Member

@pranavkm should we bother with ActionResult<T>?

@halter73 halter73 added the help wanted Up for grabs. We would accept a PR to help resolve this issue label May 11, 2021
@halter73
Copy link
Member Author

halter73 commented May 11, 2021

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

@davidfowl
Copy link
Member

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

@halter73
Copy link
Member Author

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.

@BrennanConroy BrennanConroy added this to the Next sprint planning milestone May 12, 2021
@ghost
Copy link

ghost commented May 12, 2021

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.

@uabarahona
Copy link
Contributor

Sounds interesting, I will take the easiest ones 😄

@deepumi
Copy link

deepumi commented May 13, 2021

I would like to try ContentResult or something simple.

@uabarahona
Copy link
Contributor

@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?

@wcontayon
Copy link
Contributor

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

@davidfowl
Copy link
Member

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

@wcontayon
Copy link
Contributor

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

Why not 🙂. I'll submit PR on that.
Thanks @davidfowl

@davidfowl
Copy link
Member

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

@ilkayilknur
Copy link
Contributor

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

@halter73
Copy link
Member Author

I think that's a good idea @ilkayilknur

@pranavkm
Copy link
Contributor

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

@halter73 halter73 removed the help wanted Up for grabs. We would accept a PR to help resolve this issue label Jun 29, 2021
@ilkayilknur
Copy link
Contributor

Thanks for the update @pranavkm

@halter73
Copy link
Member Author

The remaining work was addressed by #33843

@ghost ghost locked as resolved and limited conversation to collaborators Jul 30, 2021
@amcasey amcasey added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-minimal-actions Controller-like actions for endpoint routing
Projects
None yet
Development

No branches or pull requests

9 participants