Skip serialization of http result when it is stored in an output binding #2896
Conversation
| @@ -85,6 +85,9 @@ private static async Task<bool> TryHandleHttpResult(object? result, FunctionCont | |||
| // processing is required. | |||
| context.GetInvocationResult().Value = null; | |||
| break; | |||
| case AspNetCoreHttpResponseData when !isInvocationResult: | |||
There was a problem hiding this comment.
I wonder if we should test for the base class - HttpResponseData instead? In theory, I could see a customer adding a middleware which will replace/wrap that input binding with their own. We would still want this to work in that case.
Now I am unsure if that scenario is possible with todays extension points. But unless we absolutely need the type to be exactly AspNetCoreHttpResponseData, it is typically better to work off the base class.
There was a problem hiding this comment.
The above case for the invocation result also uses the more specific type (AspNetCoreHttpResponseData). The reasoning is captured here in another code review. I also had the same thought originally.
| @@ -170,6 +170,24 @@ public async Task InvocationResultNull_WhenResultIsTypeAspNetCoreHttpResponseDat | |||
| test.MockCoordinator.Verify(p => p.CompleteFunctionInvocation(It.IsAny<string>()), Times.Once()); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public async Task HttpResultOutputBindingNull_WhenUsingAspNetCoreHttpResponseDataInMultiOutputBinding() | |||
There was a problem hiding this comment.
Seems like tests in this file have a blocker (see here)?
Issue describing the changes in this PR
resolves #2682, cannot be merged until Azure/azure-functions-host#10698 is released.
Pull request checklist
release_notes.mdAdditional information
Additional PR information