Skip to content

Adding IResult implementation for FileStreamResult and LocalRedirectResult #32956

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Mvc/Mvc.Core/src/FileContentResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ Task IResult.ExecuteAsync(HttpContext httpContext)
}

var loggerFactory = httpContext.RequestServices.GetRequiredService<ILoggerFactory>();
var logger = loggerFactory.CreateLogger<RedirectResult>();
var logger = loggerFactory.CreateLogger<FileContentResult>();

var (range, rangeLength, serveBody) = FileResultExecutorBase.SetHeadersAndLog(
httpContext,
Expand Down
36 changes: 35 additions & 1 deletion src/Mvc/Mvc.Core/src/FileStreamResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
using System.Diagnostics.CodeAnalysis;
using System.IO;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc.Infrastructure;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Net.Http.Headers;

namespace Microsoft.AspNetCore.Mvc
Expand All @@ -15,7 +17,7 @@ namespace Microsoft.AspNetCore.Mvc
/// Represents an <see cref="ActionResult"/> that when executed will
/// write a file from a stream to the response.
/// </summary>
public class FileStreamResult : FileResult
public class FileStreamResult : FileResult, IResult
{
private Stream _fileStream;

Expand Down Expand Up @@ -79,5 +81,37 @@ public override Task ExecuteResultAsync(ActionContext context)
var executor = context.HttpContext.RequestServices.GetRequiredService<IActionResultExecutor<FileStreamResult>>();
return executor.ExecuteAsync(context, this);
}

async Task IResult.ExecuteAsync(HttpContext httpContext)
{
var loggerFactory = httpContext.RequestServices.GetRequiredService<ILoggerFactory>();
var logger = loggerFactory.CreateLogger<FileStreamResult>();

Task writeFileAsync(HttpContext httpContext, FileStreamResult result, RangeItemHeaderValue? range, long rangeLength)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Task writeFileAsync(HttpContext httpContext, FileStreamResult result, RangeItemHeaderValue? range, long rangeLength)
Task WriteFileAsync(HttpContext httpContext, FileStreamResult result, RangeItemHeaderValue? range, long rangeLength)

=> FileStreamResultExecutor.WriteFileAsyncInternal(httpContext, this, range, rangeLength, logger!);

(RangeItemHeaderValue? range, long rangeLength, bool serveBody) setHeadersAndLog(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these be

Suggested change
(RangeItemHeaderValue? range, long rangeLength, bool serveBody) setHeadersAndLog(
static (RangeItemHeaderValue? range, long rangeLength, bool serveBody) setHeadersAndLog(

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(RangeItemHeaderValue? range, long rangeLength, bool serveBody) setHeadersAndLog(
(RangeItemHeaderValue? range, long rangeLength, bool serveBody) SetHeadersAndLog(

HttpContext httpContext,
FileResult result,
long? fileLength,
bool enableRangeProcessing,
DateTimeOffset? lastModified,
EntityTagHeaderValue? etag)
=> FileResultExecutorBase.SetHeadersAndLog(
httpContext,
this,
fileLength,
EnableRangeProcessing,
LastModified,
EntityTag,
logger!);

await FileStreamResultExecutor.ExecuteAsyncInternal(
httpContext,
this,
setHeadersAndLog,
writeFileAsync,
Comment on lines +112 to +113
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
setHeadersAndLog,
writeFileAsync,
SetHeadersAndLog,
WriteFileAsync,

logger);
}
}
}
55 changes: 38 additions & 17 deletions src/Mvc/Mvc.Core/src/Infrastructure/FileStreamResultExecutor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

using System;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Logging;
using Microsoft.Net.Http.Headers;

Expand All @@ -26,6 +27,37 @@ public FileStreamResultExecutor(ILoggerFactory loggerFactory)

/// <inheritdoc />
public virtual async Task ExecuteAsync(ActionContext context, FileStreamResult result)
{
await ExecuteAsyncInternal(
context,
result,
SetHeadersAndLog,
WriteFileAsync,
Logger);
}

/// <summary>
/// Write the contents of the FileStreamResult to the response body.
/// </summary>
/// <param name="context">The <see cref="ActionContext"/>.</param>
/// <param name="result">The FileStreamResult to write.</param>
/// <param name="range">The <see cref="RangeItemHeaderValue"/>.</param>
/// <param name="rangeLength">The range length.</param>
protected virtual Task WriteFileAsync(
ActionContext context,
FileStreamResult result,
RangeItemHeaderValue? range,
long rangeLength)
{
return WriteFileAsyncInternal(context.HttpContext, result, range, rangeLength, Logger);
}

internal static async Task ExecuteAsyncInternal<TContext>(
TContext context,
FileStreamResult result,
Func<TContext, FileStreamResult, long?, bool, DateTimeOffset?, EntityTagHeaderValue?, (RangeItemHeaderValue?, long, bool)> SetHeadersAndLog,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Func<TContext, FileStreamResult, long?, bool, DateTimeOffset?, EntityTagHeaderValue?, (RangeItemHeaderValue?, long, bool)> SetHeadersAndLog,
Func<TContext, FileStreamResult, long?, bool, DateTimeOffset?, EntityTagHeaderValue?, (RangeItemHeaderValue?, long, bool)> setHeadersAndLog,

Func<TContext, FileStreamResult, RangeItemHeaderValue?, long, Task> WriteFileAsync,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Func<TContext, FileStreamResult, RangeItemHeaderValue?, long, Task> WriteFileAsync,
Func<TContext, FileStreamResult, RangeItemHeaderValue?, long, Task> writeFileAsync,

ILogger logger)
{
if (context == null)
{
Expand All @@ -39,7 +71,7 @@ public virtual async Task ExecuteAsync(ActionContext context, FileStreamResult r

using (result.FileStream)
{
Logger.ExecutingFileResult(result);
logger.ExecutingFileResult(result);

long? fileLength = null;
if (result.FileStream.CanSeek)
Expand All @@ -64,22 +96,11 @@ public virtual async Task ExecuteAsync(ActionContext context, FileStreamResult r
}
}

/// <summary>
/// Write the contents of the FileStreamResult to the response body.
/// </summary>
/// <param name="context">The <see cref="ActionContext"/>.</param>
/// <param name="result">The FileStreamResult to write.</param>
/// <param name="range">The <see cref="RangeItemHeaderValue"/>.</param>
/// <param name="rangeLength">The range length.</param>
protected virtual Task WriteFileAsync(
ActionContext context,
FileStreamResult result,
RangeItemHeaderValue? range,
long rangeLength)
internal static Task WriteFileAsyncInternal(HttpContext httpContext, FileStreamResult result, RangeItemHeaderValue? range, long rangeLength, ILogger logger)
{
if (context == null)
if (httpContext == null)
{
throw new ArgumentNullException(nameof(context));
throw new ArgumentNullException(nameof(httpContext));
}

if (result == null)
Expand All @@ -94,10 +115,10 @@ protected virtual Task WriteFileAsync(

if (range != null)
{
Logger.WritingRangeToBody();
logger.WritingRangeToBody();
}

return WriteFileAsync(context.HttpContext, result.FileStream, range, rangeLength);
return WriteFileAsync(httpContext, result.FileStream, range, rangeLength);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
using Microsoft.AspNetCore.Mvc.Core;
using Microsoft.AspNetCore.Mvc.Routing;
using Microsoft.Extensions.Logging;
using Microsoft.Net.Http.Headers;

namespace Microsoft.AspNetCore.Mvc.Infrastructure
{
Expand Down Expand Up @@ -45,36 +44,52 @@ public LocalRedirectResultExecutor(ILoggerFactory loggerFactory, IUrlHelperFacto
/// <inheritdoc />
public virtual Task ExecuteAsync(ActionContext context, LocalRedirectResult result)
{
if (context == null)
var urlHelper = result.UrlHelper ?? _urlHelperFactory.GetUrlHelper(context);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var urlHelper = result.UrlHelper ?? _urlHelperFactory.GetUrlHelper(context);
if (context == null)
{
throw new ArgumentNullException(nameof(context));
}
var urlHelper = result.UrlHelper ?? _urlHelperFactory.GetUrlHelper(context);


return ExecuteAsyncInternal(
context.HttpContext,
result,
urlHelper.IsLocalUrl,
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel a little iffy about this. This is now going to allocate a delegate per invocation. Could we have it fall-back in the implementation instead e.g.

internal static Task ExecuteAsyncInternal(
            HttpContext httpContext,
            LocalRedirectResult result,
            IUrlHelper? urlHelper)
{
  ...
   var isLocalUrl = urlHelper?.IsLocalUrl(url) ?? UrlHelperBase.CheckIsLocalUrl(url);
}

urlHelper.Content,
_logger);
}

internal static Task ExecuteAsyncInternal(
HttpContext httpContext,
LocalRedirectResult result,
Func<string, bool> isLocalUrl,
Func<string, string> getContent,
ILogger logger
)
Comment on lines +62 to +63
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ILogger logger
)
ILogger logger)

{
if (httpContext == null)
{
throw new ArgumentNullException(nameof(context));
throw new ArgumentNullException(nameof(httpContext));
}

if (result == null)
{
throw new ArgumentNullException(nameof(result));
}

var urlHelper = result.UrlHelper ?? _urlHelperFactory.GetUrlHelper(context);

// IsLocalUrl is called to handle Urls starting with '~/'.
if (!urlHelper.IsLocalUrl(result.Url))
if (!isLocalUrl(result.Url))
{
throw new InvalidOperationException(Resources.UrlNotLocal);
}

var destinationUrl = urlHelper.Content(result.Url);
_logger.LocalRedirectResultExecuting(destinationUrl);
var destinationUrl = getContent(result.Url);
logger.LocalRedirectResultExecuting(destinationUrl);

if (result.PreserveMethod)
{
context.HttpContext.Response.StatusCode = result.Permanent ?
httpContext.Response.StatusCode = result.Permanent ?
StatusCodes.Status308PermanentRedirect : StatusCodes.Status307TemporaryRedirect;
context.HttpContext.Response.Headers.Location = destinationUrl;
httpContext.Response.Headers.Location = destinationUrl;
}
else
{
context.HttpContext.Response.Redirect(destinationUrl, result.Permanent);
httpContext.Response.Redirect(destinationUrl, result.Permanent);
}

return Task.CompletedTask;
Expand Down
18 changes: 17 additions & 1 deletion src/Mvc/Mvc.Core/src/LocalRedirectResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,20 @@
using System;
using System.Diagnostics.CodeAnalysis;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc.Core;
using Microsoft.AspNetCore.Mvc.Infrastructure;
using Microsoft.AspNetCore.Mvc.Routing;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;

namespace Microsoft.AspNetCore.Mvc
{
/// <summary>
/// An <see cref="ActionResult"/> that returns a Found (302), Moved Permanently (301), Temporary Redirect (307),
/// or Permanent Redirect (308) response with a Location header to the supplied local URL.
/// </summary>
public class LocalRedirectResult : ActionResult
public class LocalRedirectResult : ActionResult, IResult
{
private string _localUrl;

Expand Down Expand Up @@ -103,5 +106,18 @@ public override Task ExecuteResultAsync(ActionContext context)
var executor = context.HttpContext.RequestServices.GetRequiredService<IActionResultExecutor<LocalRedirectResult>>();
return executor.ExecuteAsync(context, this);
}

Task IResult.ExecuteAsync(HttpContext httpContext)
{
var loggerFactory = httpContext.RequestServices.GetRequiredService<ILoggerFactory>();
var logger = loggerFactory.CreateLogger<LocalRedirectResult>();

return LocalRedirectResultExecutor.ExecuteAsyncInternal(
httpContext,
this,
UrlHelperBase.CheckIsLocalUrl,
url => UrlHelperBase.Content(httpContext, Url),
logger);
}
}
}
2 changes: 1 addition & 1 deletion src/Mvc/Mvc.Core/src/PhysicalFileResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ internal static Task ExecuteAsyncInternal(
long fileLength)
{
var loggerFactory = httpContext.RequestServices.GetRequiredService<ILoggerFactory>();
var logger = loggerFactory.CreateLogger<RedirectResult>();
var logger = loggerFactory.CreateLogger<PhysicalFileResult>();

logger.ExecutingFileResult(result, result.FileName);

Expand Down
2 changes: 1 addition & 1 deletion src/Mvc/Mvc.Core/src/VirtualFileResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ Task IResult.ExecuteAsync(HttpContext httpContext)
}

var loggerFactory = httpContext.RequestServices.GetRequiredService<ILoggerFactory>();
var logger = loggerFactory.CreateLogger<RedirectResult>();
var logger = loggerFactory.CreateLogger<VirtualFileResult>();

var lastModified = LastModified ?? fileInfo.LastModified;
var (range, rangeLength, serveBody) = FileResultExecutorBase.SetHeadersAndLog(
Expand Down
Loading