-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add EnableRangeProcessingSwitch for FileContentResult and Fil… #6839
Conversation
@@ -17,6 +17,8 @@ namespace Microsoft.AspNetCore.Mvc.Internal | |||
{ | |||
public class FileResultExecutorBase | |||
{ | |||
internal const string ProcessRangeRequestsSwitch = "Switch.Microsoft.AspNetCore.Mvc.TurnOnRangeProcessing"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this name confusing? The switch only applies to FileContentResult
and FileStreamResult
.
cc @Eilon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's fine actually. Or maybe instead of TurnOn
use Enable
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing it to EnableRangeProcessing
@@ -40,6 +42,7 @@ internal enum PreconditionState | |||
ActionContext context, | |||
FileResult result, | |||
long? fileLength, | |||
bool isRangeRequest = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enableRangeProcessing = true
Why not add it at the end? Then you don't have to change PhysicalFile or VirtualFile.
@@ -33,12 +33,20 @@ public virtual Task ExecuteAsync(ActionContext context, FileStreamResult result) | |||
fileLength = result.FileStream.Length; | |||
} | |||
|
|||
#if NET451 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this work? MVC.Core only targets netstandard2.0
Assert.Equal(contentLength, httpResponse.ContentLength); | ||
Assert.Equal(expectedString, body); | ||
|
||
#if NET451 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be NET461, but the fact that the else worked likely means you don't need it at all. I assume this was copied from something outdated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this might not be needed at all if the switch APIs are available in netstandard20. The sample it's copied from is designed for things back to .NET 4.5.1.
5b19a9b
to
a9a87a5
Compare
@Tratcher Updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'll merge when @Tratcher approves. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…eStreamResult
Addresses #6792
cc @Tratcher