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.

Sign up for GitHub

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

Jump to bottom

Separate IResult based results from ActionResults #33843

Merged

pranavkm merged 7 commits into main from prkrishn/results

Jun 30, 2021

Contributor

pranavkm commented Jun 25, 2021 •

edited

Loading

Fixes #33729

Member

davidfowl commented Jun 25, 2021

Why a new assembly?

Contributor Author

pranavkm commented Jun 25, 2021

The results depend on M.A.Routing which adds a circular dependency if they are in M.A.H.Extensions.

davidfowl reviewed

View reviewed changes

src/Http/Http.Results/src/ObjectResult.cs

+                      public Task ExecuteAsync(HttpContext httpContext)
+                      {
+                          var loggerFactory = httpContext.RequestServices.GetRequiredService<ILoggerFactory>();

Member

davidfowl Jun 25, 2021

We're going to want to cache these calls to the logger, but that should be in another PR. My guess is that we haven't looked at this cost in MVC.

davidfowl reviewed

View reviewed changes

src/Middleware/StaticFiles/src/LoggerExtensions.cs

-                      private static Action<ILogger, StringValues, string, Exception?> _copyingFileRange;
-                      private static Action<ILogger, Exception?> _writeCancelled;
-                      private static Action<ILogger, Exception?> _endpointMatched;
+                      private static readonly Action<ILogger, string, Exception?> _methodNotSupported;

Member

davidfowl Jun 25, 2021

Source generator?

Contributor Author

pranavkm Jun 26, 2021

It's this analyzer - which is a suggestion, but shows up as warnings in VS: https://github.com/dotnet/aspnetcore/blob/main/.editorconfig#L100-L101. Source generator to happen separately.

DamianEdwards reviewed

View reviewed changes

src/Http/Http.Results/src/Results.cs Outdated

+                      /// <summary>
+                      /// Creates a <see cref="ChallengeResult"/>.
+                      /// </summary>
+                      /// <returns>The created <see cref="ChallengeResult"/> for the response.</returns>

Member

DamianEdwards Jun 25, 2021

Is it OK to refer to internal types in these public doc comments? What happens when API doc is generated?

Contributor Author

pranavkm Jun 26, 2021

The API docs will referer to the type full name, and completion colorizes it correctly, but you're not able to control click on the type to get there. Not super weird. I didn't change the doc comments much as I mas transferring them from MVC, but if you feel strongly about it, I can do a pass to clean it up.

src/Http/Http.Results/src/Results.cs Outdated Show resolved Hide resolved

src/Http/Http.Results/src/Results.cs Outdated

+                      /// Creates a <see cref="AcceptedResult"/> object that produces an <see cref="StatusCodes.Status202Accepted"/> response.
+                      /// </summary>
+                      /// <param name="uri">The optional URI with the location at which the status of requested content can be monitored.
+                      /// May be null.</param>

Member

DamianEdwards Jun 25, 2021

We're not consistent with how we document nullable ref types in params. Some are 'blah blah; may be null', others are like this one. Let's just pick one form and stick with it.

src/Http/Http.Results/src/Results.cs Outdated

+                      /// <summary>
+                      /// Creates a <see cref="AcceptedResult"/> object that produces an <see cref="StatusCodes.Status202Accepted"/> response.
+                      /// </summary>
+                      /// <param name="value">The optional content value to format in the entity body; may be null.</param>

Member

DamianEdwards Jun 25, 2021

Might 'response body' be clearer language than 'entity body'?

Contributor Author

pranavkm Jun 26, 2021

Sure. I think this comment might have been from WebAPI2 which tends to use the RFC names

src/Http/Http.Results/src/Results.cs

+                          string contentType,
+                          string? fileDownloadName,
+                          bool enableRangeProcessing)
+                          => new PhysicalFileResult(physicalPath, contentType)

Member

DamianEdwards Jun 25, 2021

Curious, why is this one expression bodied but the next one not?

Contributor Author

pranavkm Jun 26, 2021

Somebody forgot to update MVC: https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/ControllerBase.cs#L1647-L1677

Pilchie added the area-runtime label

pranavkm force-pushed the prkrishn/results branch 3 times, most recently from 803688f to 025962d Compare

June 26, 2021 19:23

pranavkm marked this pull request as ready for review

June 26, 2021 19:23

pranavkm requested review from BrennanConroy, javiercn, Pilchie, Tratcher and a team as code owners

June 26, 2021 19:23


          Separate IResult based results from ActionResults

7d22183

pranavkm force-pushed the prkrishn/results branch from 025962d to 7d22183 Compare

June 26, 2021 19:24

Member

davidfowl commented Jun 27, 2021

@pranavkm can you make one of the samples use these?

Member

dougbu commented Jun 27, 2021

Curious: I skimmed the projects but couldn't find a reference to the new project. Other than the samples @davidfowl suggested, where will the assembly be used❔

ilkayilknur mentioned this pull request

Add IResult implementations for more IActionResults #32565

Closed

12 tasks

martincostello reviewed

View reviewed changes

src/Http/Http.Results/src/PhyiscalFileResult.cs Outdated Show resolved Hide resolved

pranavkm added 3 commits

June 28, 2021 10:22


          Add some functional tests

9f27fc6


          Fixups

53204a0


          Fix typo

a5f07c0

Contributor Author

pranavkm commented Jun 28, 2021

@dougbu the plan is to use these in the minimal actions project template. I updated a functional test to use some of these result types.

martincostello reviewed

View reviewed changes

src/Http/Http.Results/src/ChallengeResult.cs Outdated Show resolved Hide resolved

src/Http/Http.Results/src/PhysicalFileResult.cs Outdated Show resolved Hide resolved

src/Http/Http.Results/src/AcceptedAtRouteResult.cs Outdated Show resolved Hide resolved

src/Http/Http.Results/src/AcceptedAtRouteResult.cs Outdated Show resolved Hide resolved

pranavkm and others added 2 commits

June 28, 2021 13:45


          Apply suggestions from code review

85fc7cb

Co-authored-by: Martin Costello <martin@martincostello.com>


          Clean up docs

b330b73

pranavkm force-pushed the prkrishn/results branch from 3244ce7 to b330b73 Compare

June 28, 2021 23:55


          Merge branch 'main' into prkrishn/results

67cf9ae

halter73 approved these changes

View reviewed changes

pranavkm enabled auto-merge (squash)

June 30, 2021 16:59

pranavkm merged commit b9efadc into main

pranavkm deleted the prkrishn/results branch

June 30, 2021 17:32

ghost added this to the 6.0-preview7 milestone

halter73 mentioned this pull request

Make IResult methods more testable #37502

Closed

amcasey added area-middleware and removed area-runtime labels

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

Reviewers

davidfowl davidfowl left review comments

DamianEdwards DamianEdwards left review comments

martincostello martincostello left review comments

halter73 halter73 approved these changes

BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

javiercn Awaiting requested review from javiercn

Pilchie Awaiting requested review from Pilchie

Tratcher Awaiting requested review from Tratcher

Labels

area-middleware