Consider adding Text overloads to support UTF8 text

[davidfowl]

Background and Motivation

We've added the utf8 text literals to C# 11 and it would be nice to add some convenience helpers for producing results with utf8 string content.

Proposed API

namespace Microsoft.AspNetCore.Http;

public static class Results
{
+    public static IResult Text(ReadOnlySpan<byte> utf8Content, string? contentType = null, Encoding? contentEncoding = null, int? statusCode = null) 
}

public static class TypedResults
{
+    public static ContentHttpResult Text(ReadOnlySpan<byte> utf8Content, string? contentType = null, Encoding? contentEncoding = null, int? statusCode = null);
}

Usage Examples

// UTF8 literals
app.MapGet("/utf8-0", () => Results.Text("Hello World"u8));
app.MapGet("/utf8-1", () => TypedResults.Text("Hello World"u8));

// UTF8 Memory<byte>, not the most efficient by yah
var utf8FileBuffer = File.ReadAllBytes("/utf8Files/utf8file1.txt");

// Response with the first 30 bytes
app.MapGet("/u8-5", () => Results.Text(utf8FileBuffer[..30]));

Risks

None

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[gfoidl]

Does

ReadOnlyMemory<byte> foo = "foo"u8;

work without intermediate byte[]-allocation?
For ROS it works as expected, but I'm not aware that the same works for ROM (which doesn't mean that latest discussion around that topic includes support for ROM too, of course).

[davidfowl]

I believe that allocates but @stephentoub would know.

[stephentoub]

That won't compile as of the latest language design and compiler version. You'd need to write:

ReadOnlyMemory<byte> foo = "foo"u8.ToArray();

which does of course allocate. The code you wrote with the existing compiler allocates as well.

[davidfowl]

Hmm I need to get an updated compiler..

[halter73]

Do we expect people to use pooled ReadOnlyMemory<byte> with this API? Do we need to copy? Unfortunately, since we're returning the IResult, we cannot immediately write to the response body.

[halter73]

API review notes:

• @stephentoub Is there type for UTF8 literals like "foo"u8 other than ReadOnlyMemory<byte> that would allow us to directly take it as a parameter without .ToArray()?
• Let's wait on approval until we can test with new language version.

[stephentoub]

Is there type for UTF8 literals like "foo"u8 other than ReadOnlyMemory that would allow us to directly take it as a parameter with .ToArray()?

The natural type of "..."u8 is ReadOnlySpan<byte>. There are no other types it implicitly or explicitly target types.

[halter73]

If we're going to have to copy either way which it seems like we will because there's no way for the developer returning Results.Text(...) to know when they can reclaim pooled memory, maybe we should just take ReadOnlySpan<byte>.

[davidfowl]

@halter73 Sure.

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[halter73]

API Review:

• Do we need Memory overloads too?
  • Not for now. Span is easy enough and we want to copy anyway.
• Do we want to add an overload to Results.Stream() supporting leaveOpen:?
  • Maybe. Not yet though.

Api approved!

namespace Microsoft.AspNetCore.Http;

public static class Results
{
+    public static IResult Text(ReadOnlySpan<byte> utf8Content, string? contentType = null, int? statusCode = null) 
}

public static class TypedResults
{
+    public static ContentHttpResult Text(ReadOnlySpan<byte> utf8Content, string? contentType = null, int? statusCode = null);
}

[Tarun047]

Hi Guys,
I'd like to take this one up, if it's available,
As this my first time around here, I'm not sure if it's okay to create a PR directly.
I'm thinking this is the change to be done to
aspnetcore/src/Http/Http.Results/src/TypedResults.cs:

And for aspnetcore/src/Http/Http.Results/src/Results.cs:

public static IResult Text(ReadOnlySpan<byte> utf8Content, string? contentType = null, Encoding? contentEncoding = null, int? statusCode = null) => TypedResults.Text(utf8Content, contentType, contentEncoding, statusCode);

[davidfowl]

Almost. We don't want to convert to a String here.

@halter73 We missed an API (my bad) there should also be a new Utf8ContentHttpResult that we create as part of this:

public sealed partial class Utf8ContentHttpResult : IResult
{
    public Memory<byte> ResponseContent { get; internal init; }

    public string? ContentType { get; internal init; }

    public int? StatusCode { get; internal init; }

    public Task ExecuteAsync(HttpContext httpContext);
}

[davidfowl]

Made a few changes to the API:

• Removed the content encoding, these are UTF8 APIs
• Added Utf8ContentHttpResult

namespace Microsoft.AspNetCore.Http;

public static class Results
{
+    public static IResult Text(ReadOnlySpan<byte> utf8Content, string? contentType = null, int? statusCode = null) 
}

public static class TypedResults
{
+    public static Utf8ContentHttpResult Text(ReadOnlySpan<byte> utf8Content, string? contentType = null, int? statusCode = null);
}

+public sealed partial class Utf8ContentHttpResult : IResult, IStatusCodeHttpResult, IContentTypeHttpResult
+{
+   public ReadOnlyMemory<byte> ResponseContent { get; internal init; }
+
+    public string? ContentType { get; internal init; }
+
+    public int? StatusCode { get; internal init; }
+    
+    int IStatusCodeHttpResult.StatusCode => StatusCode ?? 200;
+
+    public Task ExecuteAsync(HttpContext httpContext);
+}

[davidfowl]

@Tarun047 my bad, I forgot you wanted to do this one 😞

[Tarun047]

No worries @davidfowl 😀

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[BrennanConroy]

Utf8ContentHttpResult should also implement IContentHttpResult

[davidfowl]

It could also implement IValueResult (both generic and non generic) but I don’t have a strong opinion here

[davidfowl]

OK now it matches what ContentHttpResult would have.

[halter73]

Is there a particular reason that the IStatusCodeHttpResult implementation is explicit?

[halter73]

I think I see why. Should we make IStatusCodeHttpResult.StatusCode nullable instead?

[brunolins16]

I think I see why. Should we make IStatusCodeHttpResult.StatusCode nullable instead?

In my opinion, I don't think we should because the only reason for IStatusCodeHttpResult is to describe a HttpResult that contains a StatusCode.

I did not want to change it in my PR but, in my opinion, ContentHttpResult, JsonHttpResult and Utf8ContentHttpResult should be changed to non-nullable StatusCode and explicitly set a default value (HTTP 200 OK) avoiding relying on the underlying behavior (similar issue happened in MVC #27165).

[halter73]

in my opinion, ContentHttpResult, JsonHttpResult and Utf8ContentHttpResult should be changed to non-nullable StatusCode and explicitly set a default value (HTTP 200 OK) avoiding relying on the underlying behavior

I think there's a big difference between a result that always sets a 200 OK result vs a result that just skips setting the status code. Certain result types like Utf8ContentHttpResult can do either based on its state. I think that's why it's important that IStatusCodeHttpResult.StatusCode can take distinct values (200 and null respectively in this case) for when a status code is set vs when it's skipped.

[davidfowl]

Did we close on this? Should it be nullable or not?

[halter73]

API Review Notes:

• Should IStatusCodeHttpResult.StatusCode be nullable?
  • Yes.
  • This means that any result type with non-nullable status codes should now implement the interface explicitly.

namespace Microsoft.AspNetCore.Http;

public static class Results
{
+    public static IResult Text(ReadOnlySpan<byte> utf8Content, string? contentType = null, int? statusCode = null) 
}

public static class TypedResults
{
+    public static Utf8ContentHttpResult Text(ReadOnlySpan<byte> utf8Content, string? contentType = null, int? statusCode = null);
}

+public sealed partial class Utf8ContentHttpResult : IResult, IStatusCodeHttpResult, IContentTypeHttpResult
+{
+   public ReadOnlyMemory<byte> ResponseContent { get; }
+
+    public string? ContentType { get; }
+
+    public int? StatusCode { get;  }
+
+    public Task ExecuteAsync(HttpContext httpContext);
+}

public interface IStatusCodeHttpResult
{
-    public int StatusCode { get; }
+    public int? StatusCode { get; }
}

// Plus a lot of changes to other types implementing IStatusCodeHttpResult

Approved!