Skip to content

Add IResult implementations for more IActionResults #32647

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

Merged

Conversation

uabarahona
Copy link
Contributor

@uabarahona uabarahona commented May 13, 2021

Summary

These changes add necessary implementation to IActionResults that does not have it

Details

The IActionResults that are going to be worked on this PR are:

  • RedirectResult
  • VirtualFileResult
  • FileContentResult
  • PhysicalFileResult

Task that needs to be done for this PR are:

  • Basic implementatio for IActionResults
  • Add test suite to test the new IActionResults

Addresses #32565

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label May 13, 2021
@uabarahona
Copy link
Contributor Author

uabarahona commented May 13, 2021

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

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?

@jkotalik jkotalik requested review from halter73 and davidfowl May 13, 2021 16:24
@Pilchie Pilchie added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 13, 2021
@halter73 halter73 added area-runtime and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels May 13, 2021
@uabarahona uabarahona force-pushed the feature/add-implementation-of-various-iResult branch from f64ba95 to 07b6825 Compare May 18, 2021 03:36
@uabarahona uabarahona changed the title WIP: Add IResult implementations for more IActionResults Add IResult implementations for more IActionResults May 18, 2021
@uabarahona uabarahona marked this pull request as ready for review May 18, 2021 03:36
@uabarahona
Copy link
Contributor Author

uabarahona commented May 18, 2021

@halter73 I have added implementation for:

  • RedirectResult
  • VirtualFileResult
  • FileContentResult

I will work on others tomorrow either on this PR or in a new one if this get accepted, thanks for reviewing :)

@uabarahona uabarahona force-pushed the feature/add-implementation-of-various-iResult branch from 07b6825 to ef58304 Compare May 18, 2021 14:04
@uabarahona uabarahona force-pushed the feature/add-implementation-of-various-iResult branch from ef58304 to dc5c76d Compare May 18, 2021 14:20
@uabarahona uabarahona requested a review from halter73 May 19, 2021 02:39
Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good!

@pranavkm pranavkm merged commit 72444f9 into dotnet:main May 21, 2021
@ghost ghost added this to the 6.0-preview6 milestone May 21, 2021
@pranavkm
Copy link
Contributor

Thanks for the PR!

@uabarahona uabarahona deleted the feature/add-implementation-of-various-iResult branch May 21, 2021 13:02
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants