Cleanup Results helper #34296

pranavkm · 2021-07-12T23:38:42Z

Add Results.Bytes, Results.Stream, and Results.Text overloads.
Consolidate Physical and Virtual files helpers.
Use default arguments to consolidate overloads.
Remove Accepted(Uri?) / Accepted(Uri?, object? value) overload to remove ambiguity with AcceptedAt(string?, object? value);

* Add Results.Bytes, Results.Stream, and Results.Text overloads. * Consolidate Physical and Virtual files helpers. * Use default arguments to consolidate overloads. * Remove Accepted(Uri?) / Accepted(Uri?, object? value) overload to remove ambiguity with AcceptedAt(string?, object? value);

JunTaoLuo · 2021-07-12T23:50:54Z

src/Http/Http.Results/src/PublicAPI.Unshipped.txt

-static Microsoft.AspNetCore.Http.Results.AcceptedAtRoute(string? routeName) -> Microsoft.AspNetCore.Http.IResult!
-static Microsoft.AspNetCore.Http.Results.AcceptedAtRoute(string? routeName, object? routeValues) -> Microsoft.AspNetCore.Http.IResult!
-static Microsoft.AspNetCore.Http.Results.AcceptedAtRoute(string? routeName, object? routeValues, object? value) -> Microsoft.AspNetCore.Http.IResult!
+static Microsoft.AspNetCore.Http.Results.Accepted(string? uri = null, object? value = null) -> Microsoft.AspNetCore.Http.IResult!


What about Accepted(System.Uri! uri)? Are we saying just use Uri.ToString()?

I personally like less overloads for now. If people really want this, we can add the overload later.

They can call ToString 😄

Uri.AbsoulteUri, never use ToString

The current code does AbsoluteUri and other strange things for non-absolute uris: https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/AcceptedResult.cs#L56-L63. But yeah, people could just write that code.

halter73 · 2021-07-13T00:11:10Z

src/Http/Http.Results/src/PublicAPI.Unshipped.txt

-static Microsoft.AspNetCore.Http.Results.Content(string! content, string! contentType) -> Microsoft.AspNetCore.Http.IResult!
-static Microsoft.AspNetCore.Http.Results.Content(string! content, string! contentType, System.Text.Encoding! contentEncoding) -> Microsoft.AspNetCore.Http.IResult!
+static Microsoft.AspNetCore.Http.Results.Content(string! content, Microsoft.Net.Http.Headers.MediaTypeHeaderValue! contentType) -> Microsoft.AspNetCore.Http.IResult!
+static Microsoft.AspNetCore.Http.Results.Content(string! content, string? contentType = null, System.Text.Encoding? contentEncoding = null) -> Microsoft.AspNetCore.Http.IResult!


How do we feel about just keeping Text and removing the Content overloads?

Honestly, I don't have a strong sense for how commonly this result type is used. That said, the reasons to keep it would be compat with MVC (portability / muscle memory) which might be enough of a tipping point.

JunTaoLuo · 2021-07-13T00:22:03Z

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

        /// <summary>
        /// Writes the <paramref name="content"/> string to the HTTP response.
+        /// <para>
+        /// This is an alias for <see cref="Text(string, string?, Encoding?)"/>.


I thought we were going to ignore precedence and only have Text/Stream?

JunTaoLuo

When tests pass.

davidfowl · 2021-07-13T01:51:17Z

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

+            return new ContentResult
+            {
+                Content = content,
+                ContentType = mediaTypeHeaderValue?.ToString()


Why do we ToString after Parsing? Validation?

We kinda only want the string representation to set the response header. The parsing allows us to overwrite any existing encoding with the value specified.

davidfowl

Should UnprocessableEntity/Ok/NotFound etc should haven one overload? Seems like you still have 2 method each here.
Are Forbid/SignIn/SignOut usable with optional parameters and param arrays?
Do we need a status code option for Text/Bytes/Stream?
Should Bytes be Memory<byte> instead of byte[]?
Should content type be optional and assume application/octet-stream for Bytes/Stream/File?
I think we need an overload of Stream that gives you access to the underlying response body. This is the old PushStreamContent from WebAPI. The scenario that comes up alot in MVC controllers is proxying a download from a blob backend. In these scenarios you want to use one of [these APIs](https://docs.microsoft.com/en-us/dotnet/api/azure.storage.blobs.specialized.blobbaseclient.downloadtoasync?view=azure-dotnet#azure-storage-blobs-specialized-blobbaseclient-downloadtoasync(system-io-stream-system-threading-can) that lets you download the blob to a Stream. The scenario is that you have a thing that's not a Stream itself but it can has a method to copy to a Stream.

app.MapGet("/blob", (BlobServiceClient client) => 
{
    BlobClient blob = client.GetBlobContainerClient("container").GetBlobClient("blob");
    return Results.Stream((stream, cancellationToken) => blob.DownloadToAsync(stream,  cancellationToken), fileDownloadName: fileName));
});

pranavkm · 2021-07-13T13:26:03Z

/azp run

azure-pipelines · 2021-07-13T13:26:25Z

Azure Pipelines successfully started running 2 pipeline(s).

davidfowl · 2021-07-13T17:42:00Z

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

            }

            OnFormatting(httpContext);
+            if (Value is null)


Shouldn't this go earlier?

davidfowl · 2021-07-13T21:24:26Z

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

        {
            var logger = httpContext.RequestServices.GetRequiredService<ILogger<FileStreamResult>>();
-            using (FileStream)
+            await using (FileStream)


Ah hah! That was the bug!

halter73 · 2021-07-13T21:38:39Z

Do we need a status code option for Text/Bytes/Stream?

I don't think so. I think 200 responses are the 99% case, and you can manually set the status on the HttpResponse if you want to

davidfowl · 2021-07-14T01:33:10Z

I don't get why this PR is making the blazor wasm tests fail:

/datadisks/disk1/work/A9DC0913/w/C4540A6A/e/Templates/BaseFolder/AspNet.qmmkzui22qe/Server/Data/ApplicationDbContext.cs(4,7): error CS0246: The type or namespace name 'IdentityServer4' could not be found (are you missing a using directive or an assembly reference?) [/datadisks/disk1/work/A9DC0913/w/C4540A6A/e/Templates/BaseFolder/AspNet.qmmkzui22qe/Server/AspNet.qmmkzui22qe.Server.csproj]
/datadisks/disk1/work/A9DC0913/w/C4540A6A/e/Templates/BaseFolder/AspNet.qmmkzui22qe/Server/Data/ApplicationDbContext.cs(13,22): error CS0246: The type or namespace name 'OperationalStoreOptions' could not be found (are you missing a using directive or an assembly reference?) [/datadisks/disk1/work/A9DC0913/w/C4540A6A/e/Templates/BaseFolder/AspNet.qmmkzui22qe/Server/AspNet.qmmkzui22qe.Server.csproj]/datadisks/disk1/work/A9DC0913/w/C4540A6A/e/Templates/BaseFolder/AspNet.qmmkzui22qe/Server/Data/ApplicationDbContext.cs(4,7): error CS0246: The type or namespace name 'IdentityServer4' could not be found (are you missing a using directive or an assembly reference?) [/datadisks/disk1/work/A9DC0913/w/C4540A6A/e/Templates/BaseFolder/AspNet.qmmkzui22qe/Server/AspNet.qmmkzui22qe.Server.csproj]
/datadisks/disk1/work/A9DC0913/w/C4540A6A/e/Templates/BaseFolder/AspNet.qmmkzui22qe/Server/Data/ApplicationDbContext.cs(13,22): error CS0246: The type or namespace name 'OperationalStoreOptions' could not be found (are you missing a using directive or an assembly reference?) [/datadisks/disk1/work/A9DC0913/w/C4540A6A/e/Templates/BaseFolder/AspNet.qmmkzui22qe/Server/AspNet.qmmkzui22qe.Server.csproj]

davidfowl · 2021-07-14T01:40:23Z

/azp run

azure-pipelines · 2021-07-14T01:40:42Z

Azure Pipelines successfully started running 2 pipeline(s).

davidfowl · 2021-07-14T01:42:25Z

Seems like it was this #34317

dougbu · 2021-07-14T04:53:32Z

/azp run

azure-pipelines · 2021-07-14T04:53:50Z

Azure Pipelines successfully started running 2 pipeline(s).

pranavkm requested review from BrennanConroy, Pilchie and Tratcher as code owners July 12, 2021 23:38

pranavkm requested review from JunTaoLuo and halter73 July 12, 2021 23:41

Tratcher approved these changes Jul 12, 2021

View reviewed changes

JunTaoLuo reviewed Jul 12, 2021

View reviewed changes

halter73 reviewed Jul 13, 2021

View reviewed changes

halter73 approved these changes Jul 13, 2021

View reviewed changes

JunTaoLuo reviewed Jul 13, 2021

View reviewed changes

JunTaoLuo approved these changes Jul 13, 2021

View reviewed changes

davidfowl reviewed Jul 13, 2021

View reviewed changes

davidfowl requested changes Jul 13, 2021

View reviewed changes

Fixup

bbf2648

pranavkm requested a review from javiercn as a code owner July 13, 2021 05:18

PR feedback

c8dde28

davidfowl reviewed Jul 13, 2021

View reviewed changes

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

}

OnFormatting(httpContext);

if (Value is null)

Copy link

Member

davidfowl Jul 13, 2021

Choose a reason for hiding this comment

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

Shouldn't this go earlier?

Add a new overload

d13991d

pranavkm requested a review from davidfowl July 13, 2021 20:24

davidfowl reviewed Jul 13, 2021

View reviewed changes

pranavkm enabled auto-merge (squash) July 13, 2021 22:49

davidfowl approved these changes Jul 14, 2021

View reviewed changes

pranavkm merged commit 837b178 into main Jul 14, 2021

pranavkm deleted the prkrishn/results-cleanup branch July 14, 2021 06:23

ghost added this to the 6.0-preview7 milestone Jul 14, 2021

pranavkm mentioned this pull request Sep 17, 2021

Scrub File prefix from internal File*Result types #36666

Closed

Cleanup Results helper #34296

Cleanup Results helper #34296

Uh oh!

Conversation

pranavkm commented Jul 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JunTaoLuo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavkm commented Jul 13, 2021

Uh oh!

azure-pipelines bot commented Jul 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halter73 commented Jul 13, 2021

Uh oh!

davidfowl commented Jul 14, 2021

Uh oh!

davidfowl commented Jul 14, 2021

Uh oh!

azure-pipelines bot commented Jul 14, 2021

Uh oh!

davidfowl commented Jul 14, 2021

Uh oh!

dougbu commented Jul 14, 2021

Uh oh!

azure-pipelines bot commented Jul 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

davidfowl left a comment •

edited

Loading