Skip to content
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

[BUG FIX] HTTP Trigger Functions with Multiple Output Bindings #2322

Merged
merged 25 commits into from
Apr 5, 2024
Merged
Show file tree
Hide file tree
Changes from 23 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
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,4 @@
- Improvements to context coordination/synchronization handling and observability
- Failure to receive any of the expected context synchronization calls will now result in a `TimeoutException` thrown with the appropriate exception information. Previously this would block indefinitely and failures here were difficult to diagnose.
- Debug logs are now emitted in the context coordination calls, improving observability.

- Introduces fix to properly handle multiple output binding scenarios (#2322).
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for license information.

using System;
using System.Collections.Concurrent;
using System.Linq;
using System.Threading.Tasks;
Expand All @@ -9,14 +10,17 @@
using Microsoft.AspNetCore.Mvc.Abstractions;
using Microsoft.AspNetCore.Routing;
using Microsoft.Azure.Functions.Worker.Extensions.Http.Converters;
using Microsoft.Azure.Functions.Worker.Extensions.Http.AspNetCore.Infrastructure;
using Microsoft.Azure.Functions.Worker.Http;
using Microsoft.Azure.Functions.Worker.Middleware;
using Microsoft.Extensions.DependencyInjection;

namespace Microsoft.Azure.Functions.Worker.Extensions.Http.AspNetCore
{
internal class FunctionsHttpProxyingMiddleware : IFunctionsWorkerMiddleware
{
private const string HttpTrigger = "httpTrigger";
private const string HttpBindingType = "http";

private readonly IHttpCoordinator _coordinator;
private readonly ConcurrentDictionary<string, bool> _isHttpTrigger = new();
Expand Down Expand Up @@ -47,40 +51,67 @@ public async Task Invoke(FunctionContext context, FunctionExecutionDelegate next

await next(context);

var invocationResult = context.GetInvocationResult();

if (invocationResult?.Value is IActionResult actionResult)
var responseHandled = await TryHandleHttpResult(context.GetInvocationResult().Value, context, httpContext, true)
|| await TryHandleOutputBindingsHttpResult(context, httpContext);

if (!responseHandled)
{
ActionContext actionContext = new ActionContext(httpContext, httpContext.GetRouteData(), new ActionDescriptor());

await actionResult.ExecuteResultAsync(actionContext);
var logger = context.InstanceServices.GetRequiredService<ExtensionTrace>();
fabiocav marked this conversation as resolved.
Show resolved Hide resolved
logger.NoHttpResponseReturned(context.FunctionDefinition.Name, context.InvocationId);
}
else if (invocationResult?.Value is AspNetCoreHttpResponseData)

// Allow ASP.NET Core middleware to continue
_coordinator.CompleteFunctionInvocation(invocationId);
}

private static async Task<bool> TryHandleHttpResult(object? result, FunctionContext context, HttpContext httpContext, bool isInvocationResult = false)
{
switch (result)
{
// The AspNetCoreHttpResponseData implementation is
// simply a wrapper over the underlying HttpResponse and
// all APIs manipulate the request.
// There's no need to return this result as no additional
// processing is required.
invocationResult.Value = null;
case IActionResult actionResult:
ActionContext actionContext = new ActionContext(httpContext, httpContext.GetRouteData(), new ActionDescriptor());
await actionResult.ExecuteResultAsync(actionContext);
break;
case AspNetCoreHttpRequestData when isInvocationResult:
// The AspNetCoreHttpResponseData implementation is
// simply a wrapper over the underlying HttpResponse and
// all APIs manipulate the request.
// There's no need to return this result as no additional
// processing is required.
context.GetInvocationResult().Value = null;
break;
case IResult iResult:
await iResult.ExecuteAsync(httpContext);
break;
default:
return false;
}

// allows asp.net middleware to continue
_coordinator.CompleteFunctionInvocation(invocationId);
return true;
}

private static Task<bool> TryHandleOutputBindingsHttpResult(FunctionContext context, HttpContext httpContext)
{
var httpOutputBinding = context.GetOutputBindings<object>()
.FirstOrDefault(a => string.Equals(a.BindingType, HttpBindingType, StringComparison.OrdinalIgnoreCase));

return httpOutputBinding is null
? Task.FromResult(false)
: TryHandleHttpResult(httpOutputBinding.Value, context, httpContext);
}

private static void AddHttpContextToFunctionContext(FunctionContext funcContext, HttpContext httpContext)
{
funcContext.Items.Add(Constants.HttpContextKey, httpContext);

// add asp net version of httprequestdata feature
// Add ASP.NET Core integration version of IHttpRequestDataFeature
funcContext.Features.Set<IHttpRequestDataFeature>(AspNetCoreHttpRequestDataFeature.Instance);
}

private static bool IsHttpTriggerFunction(FunctionContext funcContext)
{
return funcContext.FunctionDefinition.InputBindings
.Any(p => p.Value.Type.Equals(HttpTrigger, System.StringComparison.OrdinalIgnoreCase));
.Any(p => p.Value.Type.Equals(HttpTrigger, StringComparison.OrdinalIgnoreCase));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,21 @@ public void FunctionContextSet(string invocationId)
GeneralLog.FunctionContextSet(_defaultLogger, invocationId);
}

public void NoHttpResponseReturned(string functionName, string invocationId)
{
GeneralLog.NoHttpResponseReturned(_defaultLogger, functionName, invocationId);
}

private static partial class GeneralLog
{
[LoggerMessage(1, LogLevel.Debug, @"HttpContext set for invocation ""{InvocationId}"", Request id ""{RequestId}"".", EventName = nameof(HttpContextSet))]
public static partial void HttpContextSet(ILogger logger, string invocationId, string requestId);

[LoggerMessage(2, LogLevel.Debug, @"FunctionContext set for invocation ""{InvocationId}"".", EventName = nameof(FunctionContextSet))]
public static partial void FunctionContextSet(ILogger logger, string invocationId);

[LoggerMessage(3, LogLevel.Trace, @"No HTTP response returned from function '{FunctionName}', invocation {InvocationId}.", EventName = nameof(NoHttpResponseReturned))]
public static partial void NoHttpResponseReturned(ILogger logger, string functionName, string invocationId);
}
}
}
3 changes: 2 additions & 1 deletion extensions/Worker.Extensions.Http/release_notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
- My change description (#PR/#issue)
-->

### Microsoft.Azure.Functions.Worker.Extensions.Http <version>
### Microsoft.Azure.Functions.Worker.Extensions.Http 3.2.0

- Added ability to bind a POCO parameter to the request body using `FromBodyAttribute`
- Special thanks to @njqdev for the contributions and collaboration on this feature
- Introduces `HttpResultAttribute`, which should be used to label the parameter associated with the HTTP result in multiple output binding scenarios (#2322).
15 changes: 15 additions & 0 deletions extensions/Worker.Extensions.Http/src/HttpResultAttribute.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for license information.

using System;

namespace Microsoft.Azure.Functions.Worker
{
/// <summary>
/// Attribute used to mark an HTTP Response on an HTTP Trigger function with multiple output bindings.
/// </summary>
[AttributeUsage(AttributeTargets.Property, AllowMultiple = false)]
public sealed class HttpResultAttribute : Attribute
{
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
<Description>HTTP extensions for .NET isolated functions</Description>

<!--Version information-->
<VersionPrefix>3.1.0</VersionPrefix>
<VersionPrefix>3.2.0</VersionPrefix>
</PropertyGroup>

<Import Project="..\..\..\build\Extensions.props" />
Expand Down
3 changes: 2 additions & 1 deletion sdk/Sdk.Generators/Constants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ internal static class Types
internal const string BindingPropertyNameAttribute = "Microsoft.Azure.Functions.Worker.Extensions.Abstractions.BindingPropertyNameAttribute";
internal const string DefaultValue = "Microsoft.Azure.Functions.Worker.Extensions.Abstractions.DefaultValueAttribute";

internal const string HttpResponse = "Microsoft.Azure.Functions.Worker.Http.HttpResponseData";
internal const string HttpResponseData = "Microsoft.Azure.Functions.Worker.Http.HttpResponseData";
internal const string HttpResultAttribute = "Microsoft.Azure.Functions.Worker.HttpResultAttribute";
internal const string HttpTriggerBinding = "Microsoft.Azure.Functions.Worker.HttpTriggerAttribute";

internal const string BindingCapabilitiesAttribute = "Microsoft.Azure.Functions.Worker.Extensions.Abstractions.BindingCapabilitiesAttribute";
Expand Down
2 changes: 1 addition & 1 deletion sdk/Sdk.Generators/DiagnosticDescriptors.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ private static DiagnosticDescriptor Create(string id, string title, string messa
public static DiagnosticDescriptor MultipleHttpResponseTypes { get; }
= Create(id: "AZFW0007",
satvu marked this conversation as resolved.
Show resolved Hide resolved
title: "Symbol could not be found in user compilation.",
messageFormat: "Found multiple public properties of type HttpResponseData defined in return type '{0}'. Only one HTTP response binding type is supported in your return type definition.",
messageFormat: "Found multiple HTTP Response types (properties with HttpResultAttribute or properties of type HttpResponseData) defined in return type '{0}'. Only one HTTP response binding type is supported in your return type definition.",
category: "FunctionMetadataGeneration",
severity: DiagnosticSeverity.Error);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,15 +164,15 @@ private bool TryGetMethodOutputBinding(IMethodSymbol method, out bool hasMethodO

if (outputBindingAttribute != null)
{
if (!TryCreateBindingDict(outputBindingAttribute, Constants.FunctionMetadataBindingProps.ReturnBindingName, Location.None, out IDictionary<string, object>? bindingDict))
if (!TryCreateBindingDictionary(outputBindingAttribute, Constants.FunctionMetadataBindingProps.ReturnBindingName, Location.None, out IDictionary<string, object>? bindings))
{
bindingsList = null;
return false;
}

bindingsList = new List<IDictionary<string, object>>(capacity: 1)
{
bindingDict!
bindings!
};

return true;
Expand Down Expand Up @@ -288,22 +288,22 @@ private bool TryGetParameterInputAndTriggerBindings(IMethodSymbol method, out bo

string bindingName = parameter.Name;

if (!TryCreateBindingDict(attribute, bindingName, Location.None, out IDictionary<string, object>? bindingDict, supportsDeferredBinding))
if (!TryCreateBindingDictionary(attribute, bindingName, Location.None, out IDictionary<string, object>? bindings, supportsDeferredBinding))
{
bindingsList = null;
bindings = null;
return false;
}

// If cardinality is supported and validated, but was not found in named args, constructor args, or default value attributes
// default to Cardinality: One to stay in sync with legacy generator.
if (cardinalityValidated && !bindingDict!.Keys.Contains("cardinality"))
if (cardinalityValidated && !bindings!.Keys.Contains("cardinality"))
{
bindingDict!.Add("cardinality", "One");
bindings!.Add("cardinality", "One");
}

if (dataType is not DataType.Undefined)
{
bindingDict!.Add("dataType", Enum.GetName(typeof(DataType), dataType));
bindings!.Add("dataType", Enum.GetName(typeof(DataType), dataType));
}

// check for binding capabilities
Expand All @@ -318,7 +318,7 @@ private bool TryGetParameterInputAndTriggerBindings(IMethodSymbol method, out bo
}
}

bindingsList.Add(bindingDict!);
bindingsList.Add(bindings!);
}
}
}
Expand Down Expand Up @@ -434,7 +434,7 @@ private bool TryGetReturnTypeBindings(IMethodSymbol method, bool hasHttpTrigger,
}
}

if (SymbolEqualityComparer.Default.Equals(returnTypeSymbol, _knownFunctionMetadataTypes.HttpResponse)) // If return type is HttpResponseData
if (SymbolEqualityComparer.Default.Equals(returnTypeSymbol, _knownFunctionMetadataTypes.HttpResponseData))
{
bindingsList.Add(GetHttpReturnBinding(Constants.FunctionMetadataBindingProps.ReturnBindingName));
}
Expand Down Expand Up @@ -467,8 +467,9 @@ private bool TryGetReturnTypePropertyBindings(ITypeSymbol returnTypeSymbol, bool
return false;
}

// Check if this attribute is an HttpResponseData type attribute
if (prop is IPropertySymbol property && SymbolEqualityComparer.Default.Equals(property.Type, _knownFunctionMetadataTypes.HttpResponse))
satvu marked this conversation as resolved.
Show resolved Hide resolved
// Check for HttpResponseData type for legacy apps
if (prop is IPropertySymbol property
&& (SymbolEqualityComparer.Default.Equals(property.Type, _knownFunctionMetadataTypes.HttpResponseData)))
{
if (foundHttpOutput)
{
Expand All @@ -479,34 +480,46 @@ private bool TryGetReturnTypePropertyBindings(ITypeSymbol returnTypeSymbol, bool

foundHttpOutput = true;
bindingsList.Add(GetHttpReturnBinding(prop.Name));
continue;
}
else if (prop.GetAttributes().Length > 0) // check if this property has any attributes

var propAttributes = prop.GetAttributes();
satvu marked this conversation as resolved.
Show resolved Hide resolved

if (propAttributes.Length > 0)
{
var foundPropertyOutputAttr = false;
var bindingAttributes = propAttributes.Where(p => p.AttributeClass!.IsOrDerivedFrom(_knownFunctionMetadataTypes.BindingAttribute));

if (bindingAttributes.Count() > 1)
{
_context.ReportDiagnostic(Diagnostic.Create(DiagnosticDescriptors.MultipleBindingsGroupedTogether, Location.None, new object[] { "Property", prop.Name.ToString() }));
satvu marked this conversation as resolved.
Show resolved Hide resolved
bindingsList = null;
return false;
}

foreach (var attr in prop.GetAttributes()) // now loop through and check if any of the attributes are Binding attributes
// Check if this property has an HttpResultAttribute on it
if (HasHttpResultAttribute(prop))
{
if (SymbolEqualityComparer.Default.Equals(attr.AttributeClass?.BaseType, _knownFunctionMetadataTypes.OutputBindingAttribute))
if (foundHttpOutput)
{
// validate that there's only one binding attribute per property
if (foundPropertyOutputAttr)
{
_context.ReportDiagnostic(Diagnostic.Create(DiagnosticDescriptors.MultipleBindingsGroupedTogether, Location.None, new object[] { "Property", prop.Name.ToString() }));
bindingsList = null;
return false;
}
_context.ReportDiagnostic(Diagnostic.Create(DiagnosticDescriptors.MultipleHttpResponseTypes, Location.None, new object[] { returnTypeSymbol.Name }));
satvu marked this conversation as resolved.
Show resolved Hide resolved
bindingsList = null;
return false;
}

if (!TryCreateBindingDict(attr, prop.Name, prop.Locations.FirstOrDefault(), out IDictionary<string, object>? bindingDict))
{
bindingsList = null;
return false;
}
foundHttpOutput = true;
bindingsList.Add(GetHttpReturnBinding(prop.Name));
}
else
{
if (!TryCreateBindingDictionary(bindingAttributes.FirstOrDefault(), prop.Name, prop.Locations.FirstOrDefault(), out IDictionary<string, object>? bindings))
{
bindingsList = null;
return false;
}

bindingsList.Add(bindingDict!);
bindingsList.Add(bindings!);

returnTypeHasOutputBindings = true;
foundPropertyOutputAttr = true;
}
returnTypeHasOutputBindings = true;
}
}
}
Expand All @@ -519,6 +532,21 @@ private bool TryGetReturnTypePropertyBindings(ITypeSymbol returnTypeSymbol, bool
return true;
}

private bool HasHttpResultAttribute(ISymbol prop)
satvu marked this conversation as resolved.
Show resolved Hide resolved
{
var attributes = prop.GetAttributes();
foreach (var attribute in attributes)
{
if (attribute.AttributeClass is not null &&
attribute.AttributeClass.IsOrDerivedFrom(_knownFunctionMetadataTypes.HttpResultAttribute))
{
return true;
}
}

return false;
}

private IDictionary<string, object> GetHttpReturnBinding(string returnBindingName)
{
var httpBinding = new Dictionary<string, object>
Expand All @@ -531,7 +559,7 @@ private bool TryGetReturnTypePropertyBindings(ITypeSymbol returnTypeSymbol, bool
return httpBinding;
}

private bool TryCreateBindingDict(AttributeData bindingAttrData, string bindingName, Location? bindingLocation, out IDictionary<string, object>? bindings, bool supportsDeferredBinding = false)
private bool TryCreateBindingDictionary(AttributeData bindingAttrData, string bindingName, Location? bindingLocation, out IDictionary<string, object>? bindings, bool supportsDeferredBinding = false)
{
// Get binding info as a dictionary with keys as the property name and value as the property value
if (!TryGetAttributeProperties(bindingAttrData, bindingLocation, out IDictionary<string, object?>? attributeProperties))
Expand Down
Loading
Loading