Add IResult implementation for FileResult #32835
Conversation
ghost
added
area-mvc
|
|
|
Hi @davidfowl, @halter73 this is a draft of the PR. |
|
I think we have a clash 😢 #32647 |
I noticed the implementation on other FileResult inherited classes 😅 on the PR #32647 . |
| logger.WritingRangeToBody(); | ||
| } | ||
|
|
||
| return FileResultExecutorBase.WriteFileAsync(httpContext, httpContext.Response.Body, range, rangeLenth); |
There was a problem hiding this comment.
I'm pretty sure that FileResultExecutorBase.WriteFileAsync expects a fileStream as the second argument. It can get the Response.Body from the HttpContext, but I don't think we have the actual fileStream in the FileResult base class.
Thanks for taking the time to help out and submit this PR! Unfortunately, this does conflict with #32647, and I think we're going to end up merging that first.
There was a problem hiding this comment.
No problem. It's too bad we can't take both contributions.
I think it might be best to look for other help-wanted issues because it sounds like @barahonajm also looking to do more of these.
| SetContentType(context.HttpContext, result); | ||
| } | ||
|
|
||
| internal static void SetContentType(HttpContext httpContext, FileResult result) |
There was a problem hiding this comment.
Do you need a separate method for a one-liner?
| response.ContentType = result.ContentType; | ||
| } | ||
|
|
||
| private static void SetContentDispositionHeader(ActionContext context, FileResult result) | ||
| { | ||
| SetContentDispositionHeader(context.HttpContext, result); |
There was a problem hiding this comment.
Can you change the calling code to pass the HttpContext instead? Seems like overkill to have a one-liner like this.
| bool enableRangeProcessing, | ||
| DateTimeOffset? lastModified = null, | ||
| EntityTagHeaderValue? etag = null, | ||
| ILogger? logger = null) |
| fileLength); | ||
|
|
||
| // Overwrite the Content-Length header for valid range requests with the range length. | ||
| var rangeLength = SetContentLength(response, range); |
There was a problem hiding this comment.
This should pass in the ILogger.
| RequestHeaders httpRequestHeaders, | ||
| DateTimeOffset? lastModified, | ||
| EntityTagHeaderValue? etag) | ||
| EntityTagHeaderValue? etag, | ||
| ILogger? logger = null) |
There was a problem hiding this comment.
This should be non-nullable / non-optional.
| @@ -363,6 +394,54 @@ private static PreconditionState GetMaxPreconditionState(params PreconditionStat | |||
| return (range, rangeLength, serveBody); | |||
| } | |||
|
|
|||
| internal static (RangeItemHeaderValue? range, long rangeLength, bool serveBody) SetRangeHeaders( | |||
There was a problem hiding this comment.
Are you unable to re-use the existing SetRangeHeaders method?
Summary
These changes add necessary implementation to FileResult
Details
Addresses #32565