Adding `IResult` implementation for FileStreamResult and LocalRedirectResult #32956

uabarahona · 2021-05-24T03:10:06Z

Summary

These changes add IResult implementation on ActionResults that does not have it

Details

The ActionResults that are going to be worked on this PR are:

FileStreamResult
LocalRedirectResult

Task that needs to be done for this PR are:

Basic implementation for IResults
Add test suite to test the new IResults

Addresses part of #32565

pranavkm · 2021-05-25T22:23:04Z

src/Mvc/Mvc.Core/src/FileStreamResult.cs

+                throw new ArgumentNullException(nameof(httpContext));
+            }
+
+            using (FileStream)


Out of curiosity, it is possible to share this implementation with the one in FileStreamExecutor (using an internal static method)? We could make the WriteFileAsync an optional parameter to the function to help with the overload.

static ExecuteAsync(HttpContext context, ILogger logger, Func<..., Task>)

Same q with the redirect result.

That's a good idea!, I will make the change on night :)

@pranavkm I take me more than I thought to get back into this, I have applied your suggestions, would you take a look?

nil4 · 2021-05-31T08:22:32Z

src/Mvc/Mvc.Core/src/FileStreamResult.cs

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


Is RedirectResult the relevant type parameter for this logger?

No, it should be FileStreamResult, thanks! already fixed

uabarahona · 2021-05-31T19:57:28Z

@pranavkm @nil4 I have fixed the logger types on all my other implementations, also if they way I implemented the approach suggested by @pranavkm is correct, I will migrate the other implementations either on this ticket or in a new PR later in this week.

Thanks!

pranavkm

Sorry for the delay in getting back

pranavkm · 2021-06-04T23:10:27Z

src/Mvc/Mvc.Core/src/Infrastructure/FileStreamResultExecutor.cs

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


Suggested change

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

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

pranavkm · 2021-06-04T23:10:33Z

src/Mvc/Mvc.Core/src/Infrastructure/FileStreamResultExecutor.cs

+            TContext context,
+            FileStreamResult result,
+            Func<TContext, FileStreamResult, long?, bool, DateTimeOffset?, EntityTagHeaderValue?, (RangeItemHeaderValue?, long, bool)> SetHeadersAndLog,
+            Func<TContext, FileStreamResult, RangeItemHeaderValue?, long, Task> WriteFileAsync,


Suggested change

Func<TContext, FileStreamResult, RangeItemHeaderValue?, long, Task> WriteFileAsync,

Func<TContext, FileStreamResult, RangeItemHeaderValue?, long, Task> writeFileAsync,

pranavkm · 2021-06-04T23:11:12Z

src/Mvc/Mvc.Core/src/FileStreamResult.cs

+            Task writeFileAsync(HttpContext httpContext, FileStreamResult result, RangeItemHeaderValue? range, long rangeLength)
+                => FileStreamResultExecutor.WriteFileAsyncInternal(httpContext, this, range, rangeLength, logger!);
+
+            (RangeItemHeaderValue? range, long rangeLength, bool serveBody) setHeadersAndLog(


Can these be

Suggested change

(RangeItemHeaderValue? range, long rangeLength, bool serveBody) setHeadersAndLog(

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

Suggested change

(RangeItemHeaderValue? range, long rangeLength, bool serveBody) setHeadersAndLog(

(RangeItemHeaderValue? range, long rangeLength, bool serveBody) SetHeadersAndLog(

pranavkm · 2021-06-04T23:11:36Z

src/Mvc/Mvc.Core/src/FileStreamResult.cs

+            var loggerFactory = httpContext.RequestServices.GetRequiredService<ILoggerFactory>();
+            var logger = loggerFactory.CreateLogger<FileStreamResult>();
+
+            Task writeFileAsync(HttpContext httpContext, FileStreamResult result, RangeItemHeaderValue? range, long rangeLength)


Suggested change

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

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

pranavkm · 2021-06-04T23:12:02Z

src/Mvc/Mvc.Core/src/FileStreamResult.cs

+                setHeadersAndLog,
+                writeFileAsync,


Suggested change

setHeadersAndLog,

writeFileAsync,

SetHeadersAndLog,

WriteFileAsync,

pranavkm · 2021-06-04T23:12:25Z

src/Mvc/Mvc.Core/src/Infrastructure/LocalRedirectResultExecutor.cs

+            ILogger logger
+            )


Suggested change

ILogger logger

)

ILogger logger)

pranavkm · 2021-06-04T23:14:00Z

src/Mvc/Mvc.Core/src/Infrastructure/LocalRedirectResultExecutor.cs

@@ -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);


Suggested change

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

if (context == null)

{

throw new ArgumentNullException(nameof(context));

}

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

pranavkm · 2021-06-04T23:20:11Z

src/Mvc/Mvc.Core/src/Infrastructure/LocalRedirectResultExecutor.cs

+            return ExecuteAsyncInternal(
+                context.HttpContext,
+                result,
+                urlHelper.IsLocalUrl,


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); }

pranavkm · 2021-07-01T23:37:55Z

Thanks for your effort @barahonajm. We ended up deciding that it was best to avoid referencing MVC types from minimal actions especially for a common scenario such as the result types. We ended up duplicating the results and as such we no longer want to take this PR. Once again, thanks for your all your time!

uabarahona · 2021-07-02T01:54:46Z

@pranavkm that sound like a right decision, thanks for your guidance during this time. See you in another PR :)

ghost · 2021-07-02T01:54:48Z

Hi @barahonajm. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

uabarahona requested review from javiercn and pranavkm as code owners May 24, 2021 03:10

ghost added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates community-contribution Indicates that the PR has been added by a community member labels May 24, 2021

uabarahona changed the title ~~Adding implementation for FileStreamResult and LocalRedirectResult~~ Adding IResult implementation for FileStreamResult and LocalRedirectResult May 24, 2021

pranavkm reviewed May 25, 2021

View reviewed changes

mkArtakMSFT assigned pranavkm May 26, 2021

uabarahona requested a review from pranavkm May 30, 2021 22:39

nil4 reviewed May 31, 2021

View reviewed changes

uabarahona added 4 commits May 31, 2021 13:20

Adding implementation for FileStreamResult and LocalRedirectResult

bdc812c

Share implementation between executor and IResult

1dcfb6f

code clean up

e128fa3

Fixing and error with the loogger

1bf722e

uabarahona force-pushed the barahonajm/adding_IResult_Implementations branch from d49cb15 to 1bf722e Compare May 31, 2021 17:33

uabarahona added 2 commits May 31, 2021 13:34

Fixing a error with the logging

e2c05d2

Fixing the loggers

991067f

pranavkm reviewed Jun 4, 2021

View reviewed changes

pranavkm closed this Jul 1, 2021

uabarahona deleted the barahonajm/adding_IResult_Implementations branch July 2, 2021 01:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding `IResult` implementation for FileStreamResult and LocalRedirectResult #32956

Adding `IResult` implementation for FileStreamResult and LocalRedirectResult #32956

uabarahona commented May 24, 2021

pranavkm May 25, 2021

pranavkm May 25, 2021

uabarahona May 26, 2021

uabarahona May 30, 2021

nil4 May 31, 2021

uabarahona May 31, 2021

uabarahona commented May 31, 2021

pranavkm left a comment

pranavkm Jun 4, 2021

pranavkm Jun 4, 2021

pranavkm Jun 4, 2021

pranavkm Jun 4, 2021

pranavkm Jun 4, 2021

pranavkm Jun 4, 2021

pranavkm Jun 4, 2021

pranavkm Jun 4, 2021

pranavkm Jun 4, 2021

pranavkm commented Jul 1, 2021

uabarahona commented Jul 2, 2021

ghost commented Jul 2, 2021

	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,
	Func<TContext, FileStreamResult, RangeItemHeaderValue?, long, Task> writeFileAsync,

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

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

Adding IResult implementation for FileStreamResult and LocalRedirectResult #32956

Adding IResult implementation for FileStreamResult and LocalRedirectResult #32956

Conversation

uabarahona commented May 24, 2021

Summary

Details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uabarahona commented May 31, 2021

pranavkm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm commented Jul 1, 2021

uabarahona commented Jul 2, 2021

ghost commented Jul 2, 2021

Adding `IResult` implementation for FileStreamResult and LocalRedirectResult #32956

Adding `IResult` implementation for FileStreamResult and LocalRedirectResult #32956