Open davidfowl opened 2 years ago
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:
- Func<Stream, Task> streamFactory
+Func<Stream, Task> writeHttpResponseStreamAsync,
or something like that. I think we'd spoken about this API when reviewing the Results
type but left it alone at that time to get feedback. Also I think we should try and keep some parity between this and the API in controllers to allow users to migrate from one to another without too much effort (in particular since it's low cost for us to add support)
Thanks for contacting us.
We're moving this issue to the .NET 7 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.
API review:
We decided to add a few more overloads during review.
public class Results
{
+ public IResult Stream(
+ Func<Stream, Task> streamWriterCallback,
+ string? contentType = null,
+ string? fileDownloadName = null,
+ DateTimeOffset? lastModified = null,
+ EntityTagHeaderValue? entityTag = null,
+ bool enableRangeProcessing = false);
+ public IResult Stream(
+ PipeReader pipeReader,
+ string? contentType = null,
+ string? fileDownloadName = null,
+ DateTimeOffset? lastModified = null,
+ EntityTagHeaderValue? entityTag = null,
+ bool enableRangeProcessing = false);
+ public IResult Stream(
+ Func<PipeWriter, Task> pipeWriterCallback,
+ string? contentType = null,
+ string? fileDownloadName = null,
+ DateTimeOffset? lastModified = null,
+ EntityTagHeaderValue? entityTag = null,
+ bool enableRangeProcessing = false);
+ public IResult Bytes(
+ ReadOnlyMemory<byte> byteBuffer,
+ string? contentType = null,
+ string? fileDownloadName = null,
+ bool enableRangeProcessing = false,
+ DateTimeOffset? lastModified = null,
+ EntityTagHeaderValue? entityTag = null);
}
public class ControllerBase
{
+ public PushFileStreamResult File(
+ Func<Stream, Task> streamWriterCallback,
+ string? contentType = null,
+ string? fileDownloadName = null,
+ DateTimeOffset? lastModified = null,
+ EntityTagHeaderValue? entityTag = null,
+ bool enableRangeProcessing = false);
// This needs to add a remark that the byte buffer
// should not use a pooled source since it's lifetime
// isn't tied to the action result execution.
// Accessing FileContentResult.FileContents should copy the results from the memory
+ public FileContentResult File(
+ ReadOnlyMemory<byte> fileContents,
+ string? contentType = null,
+ string? fileDownloadName = null,
+ bool enableRangeProcessing = false,
+ DateTimeOffset? lastModified = null,
+ EntityTagHeaderValue? entityTag = null);
+ public class PushFileStreamResult : FileResult
+ {
+ public Func<Stream, Task> StreamWriterCallback { get; set; }
+ }
}
We should also add PipeReader / PipeWriter overloads for controllers. @rafikiassumani-msft could we have someone propose the API for this and bring it to review?
I just tried to implement this and the overloads conflicts because of how we use optional parameters... We might need to rename these or find a clever workaround.
Actually this is fine, it's just the API analyzer complaining.
OK having implemented, this I think we need to support an overload of the callbacks that support range processing:
public class Results
{
// Range processing overloads
+ public IResult Stream(
+ Func<Stream, long?, long, Task> streamWriterCallback,
+ string? contentType = null,
+ string? fileDownloadName = null,
+ DateTimeOffset? lastModified = null,
+ EntityTagHeaderValue? entityTag = null);
+ public IResult Stream(
+ Func<PipeWriter, long?, long, Task> pipeWriterCallback,
+ string? contentType = null,
+ string? fileDownloadName = null,
+ DateTimeOffset? lastModified = null,
+ EntityTagHeaderValue? entityTag = null);
// Non-Range processing overloads
+ public IResult Stream(
+ Func<Stream, Task> streamWriterCallback,
+ string? contentType = null,
+ string? fileDownloadName = null,
+ DateTimeOffset? lastModified = null,
+ EntityTagHeaderValue? entityTag = null);
+ public IResult Stream(
+ Func<PipeWriter, Task> pipeWriterCallback,
+ string? contentType = null,
+ string? fileDownloadName = null,
+ DateTimeOffset? lastModified = null,
+ EntityTagHeaderValue? entityTag = null);
}
@pranavkm brought up the fact that we might want a custom delegate type so we can document the arguments of the range processing overloads. The downside is that we need 2 types (one for pipelines and one for streams): Here's a strawman:
public delegate Task WriteRangeStreamCallback(Stream stream, long? start, long length);
public delegate Task WriteRangePipeWriterCallback(PipeWriter pipeWriter, long? start, long length);
I'm not yet convinced...
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:
We made a couple of decisions:
Func<PipeWriter, Task>
overload to avoid APIs ambiguity with the Stream APIs.Triage: We will keep this issue open to give us time to add these results to Web APIs in MVC.
It looks like the new File
method that returns a PushFileStreamResult
proposed above by @pranavkm has not yet been implemented. https://github.com/dotnet/aspnetcore/issues/39383#issuecomment-1009261849
Can we expect it for .NET 7 ? If not, can we expect it for .NET 8 ?
Triage: We will keep this issue open to give us time to add these results to Web APIs in MVC.
Results.Stream
is great. Glad to see this is on y'all's radar for MVC, as I looked for it today and didn't find the equivalent.
Since it has both api-approved
and help wanted
tags I'd like to implement the MVC part. But before I dive into the implementation I'd like to be sure about the exact API.
@davidfowl you said
Actually this is fine, it's just the API analyzer complaining.
So is it OK to ignore RS0027?
Symbol 'File' violates the backcompat requirement: Public API with optional parameter(s) should have the most parameters amongst its public overloads.
Wouldn't it be wiser to go the same route as all the other File
methods? I.e.
File(Func<Stream, Task> streamWriterCallback, string contentType);
File(Func<Stream, Task> streamWriterCallback, string contentType, bool enableRangeProcessing);
File(Func<Stream, Task> streamWriterCallback, string contentType, string? fileDownloadName);
File(Func<Stream, Task> streamWriterCallback, string contentType, string? fileDownloadName, bool enableRangeProcessing);
File(Func<Stream, Task> streamWriterCallback, string contentType, string? fileDownloadName, DateTimeOffset? lastModified, EntityTagHeaderValue entityTag);
File(Func<Stream, Task> streamWriterCallback, string contentType, string? fileDownloadName, DateTimeOffset? lastModified, EntityTagHeaderValue entityTag, bool enableRangeProcessing);
File(Func<Stream, Task> streamWriterCallback, string contentType, DateTimeOffset? lastModified, EntityTagHeaderValue entityTag);
File(Func<Stream, Task> streamWriterCallback, string contentType, DateTimeOffset? lastModified, EntityTagHeaderValue entityTag, bool enableRangeProcessing);
Also, I think the StreamWriterCallback
property should probably be Func<Stream, CancellationToken, Task>
instead of Func<Stream, Task>
(i.e. with a CancellationToken
parameter).
Edit: I guess one could capture the CancellationToken
provided by the controller method in the callback so that's probably not necessary.
Well, .NET 8 RC1 was released yesterday so I guess this will be postponed to at least .NET 9 or is there still a chance to get it merged for the .NET 8 general availability (GA) release?
I see that the help wanted
tag has been replaced by the help candidate
tag. What does that mean for an external (non-Microsoft) contributor like me?
I'd be willing to help but I'd prefer to get answers to my questions first before diving into the implementation.
Details about the new process can be found here.
Thanks @martincostello for pointing out the right document. So if I understand correctly, it's up to @davidfowl (the engineer assigned to this issue) to give directions on how to move forward, right?
Background and Motivation
There are scenarios where APIs want to work directly on the response stream without buffering. Today the Stream overloads assume you have a stream that you have produced that is then copied to the underlying response stream, instead, we want to provide a result type that gives you access to the underlying response stream.
Example:
https://github.com/khalidabuhakmeh/dotnet-dramameter/blob/main/DramaMeter/Program.cs#L37-L43
Another is writing a blob storage results to the http response.
Proposed API
Usage Examples
Risks
None