diff --git a/docs/analyzer-rules/AZFW0008.md b/docs/analyzer-rules/AZFW0008.md index d07e433e8..7201f5a3b 100644 --- a/docs/analyzer-rules/AZFW0008.md +++ b/docs/analyzer-rules/AZFW0008.md @@ -1,4 +1,4 @@ -# AZFW0008: Invalid EventHubs Trigger +# AZFW0008: Invalid Cardinality | | Value | |-|-| @@ -8,17 +8,19 @@ ## Cause -This rule is triggered when the EventHubs trigger of an Azure Function is invalid. +This rule is triggered when an Input or Trigger Binding of an Azure Function has an invalid Cardinality. ## Rule description -EventHubs triggers must correctly declare a compatible "IsBatched" value and parameter input type. For example, for EventHubs triggers where `IsBatched = true`, the input parameter type must be an iterable collection. +"Cardinality" dictates whether or not an input is batched together or processed individually. It is defined by using the argument "IsBatched" in an Azure Functions Input or Trigger Binding attribute. When IsBatched is true, Cardinality is set to `Many`. When IsBatched is false, Cardinality is set to `One`. -_**Note:**_ The default value of IsBatched is true. If you are not explicitly providing a value, the parameter type should be a collection type. +All Input and Trigger Bindings must correctly declare a compatible "IsBatched" value and parameter input type. For example, for bindings where `IsBatched = true`, the input parameter type must be an iterable collection like `string[]` or `List`. Combining `IsBatched = true` with a parameter of `string[]` is valid, but combining `IsBatched = true` with a parameter of `string` is invalid. + +_**Note:**_ The default value of `IsBatched` changes depending on the Input or Trigger Binding type. The default value of `IsBatched` for each type can be found in the dotnet-isolated [extensions libraries](https://github.com/Azure/azure-functions-dotnet-worker/tree/main/extensions). If a value is not explicitly provided in the named arguments of the attribute, the default value will be used. ## How to fix violations -If you have an EventHubs trigger where `IsBatched = true` (it is true by default), make sure your input parameter type is an iterable collection. +If an Input or Trigger Bindings has `IsBatched = true` (explicitly or by default), the input parameter type must be changed to an iterable collection. Otherwise, `IsBatched` needs to be set to false. ## When to suppress warnings diff --git a/sdk/Sdk.Generators/DiagnosticDescriptors.cs b/sdk/Sdk.Generators/DiagnosticDescriptors.cs index 592660795..1d305fc23 100644 --- a/sdk/Sdk.Generators/DiagnosticDescriptors.cs +++ b/sdk/Sdk.Generators/DiagnosticDescriptors.cs @@ -49,10 +49,10 @@ private static DiagnosticDescriptor Create(string id, string title, string messa category: "FunctionMetadataGeneration", severity: DiagnosticSeverity.Error); - public static DiagnosticDescriptor InvalidEventHubsTrigger { get; } + public static DiagnosticDescriptor InvalidCardinality { get; } = Create(id: "AZFW0008", - title: "EventHub Trigger invalid.", - messageFormat: "The EventHub trigger on parameter '{0}' is invalid. IsBatched may be used incorrectly.", + title: "Input or Trigger Binding Cardinality is invalid.", + messageFormat: "The Cardinality of the Input or Trigger Binding on parameter '{0}' is invalid. IsBatched may be used incorrectly.", category: "FunctionMetadataGeneration", severity: DiagnosticSeverity.Error); } diff --git a/sdk/Sdk.Generators/FunctionMetadataProviderGenerator.Parser.cs b/sdk/Sdk.Generators/FunctionMetadataProviderGenerator.Parser.cs index f36067d1f..801946666 100644 --- a/sdk/Sdk.Generators/FunctionMetadataProviderGenerator.Parser.cs +++ b/sdk/Sdk.Generators/FunctionMetadataProviderGenerator.Parser.cs @@ -7,7 +7,6 @@ using System.IO; using System.Linq; using System.Threading; -using Microsoft.Azure.Functions.Worker.Sdk.Generators.Enums; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp.Syntax; @@ -210,25 +209,29 @@ private bool TryGetParameterInputAndTriggerBindings(MethodDeclarationSyntax meth { if (SymbolEqualityComparer.Default.Equals(attribute.AttributeClass?.BaseType?.BaseType, Compilation.GetTypeByMetadataName(Constants.Types.BindingAttribute))) { - var validEventHubs = false; - var cardinality = Cardinality.Undefined; - var dataType = GetDataType(parameterSymbol.Type); - // There are two special cases we need to handle: HttpTrigger and EventHubsTrigger. if (SymbolEqualityComparer.Default.Equals(attribute.AttributeClass, Compilation.GetTypeByMetadataName(Constants.Types.HttpTriggerBinding))) { hasHttpTrigger = true; } - else if (SymbolEqualityComparer.Default.Equals(attribute.AttributeClass, Compilation.GetTypeByMetadataName(Constants.Types.EventHubsTrigger))) + + DataType dataType = GetDataType(parameterSymbol.Type); + + if (IsCardinalitySupported(attribute)) { - // there are special rules for EventHubsTriggers that we will have to validate - validEventHubs = IsEventHubsTriggerValid(parameterSymbol, parameter.Type, model, attribute, out dataType, out cardinality); - if (!validEventHubs) + DataType updatedDataType = DataType.Undefined; + + if (!IsCardinalityValid(parameterSymbol, parameter.Type, model, attribute, out updatedDataType)) { - _context.ReportDiagnostic(Diagnostic.Create(DiagnosticDescriptors.InvalidEventHubsTrigger, parameter.Identifier.GetLocation(), nameof(parameterSymbol))); + _context.ReportDiagnostic(Diagnostic.Create(DiagnosticDescriptors.InvalidCardinality, parameter.Identifier.GetLocation(), parameterSymbol.Name)); bindingsList = null; return false; } + + // update the DataType of this binding with the updated type found during call to IsCardinalityValid + // ex. IList would be evaluated as "Undefined" by the call to GetDataType + // but it would be correctly evaluated as "String" during the call to IsCardinalityValid which parses iterable collections + dataType = updatedDataType; } string bindingName = parameter.Identifier.ValueText; @@ -238,21 +241,12 @@ private bool TryGetParameterInputAndTriggerBindings(MethodDeclarationSyntax meth bindingsList = null; return false; } - + if (dataType is not DataType.Undefined) { bindingDict!.Add("dataType", Enum.GetName(typeof(DataType), dataType)); } - // special handling for EventHubsTrigger - we need to define a property called "cardinality" - if (validEventHubs) - { - if (!bindingDict!.ContainsKey("cardinality")) - { - bindingDict["cardinality"] = cardinality is Cardinality.Many ? "Many" : "One"; - } - } - bindingsList.Add(bindingDict!); } } @@ -576,14 +570,29 @@ private void OverrideBindingName(INamedTypeSymbol attributeClass, ref string arg } } + private bool IsCardinalitySupported(AttributeData attribute) + { + return TryGetIsBatchedProp(attribute, out var isBatchedProp); + } + + private bool TryGetIsBatchedProp(AttributeData attribute, out ISymbol? isBatchedProp) + { + var attrClass = attribute.AttributeClass; + isBatchedProp = attrClass! + .GetMembers() + .SingleOrDefault(m => string.Equals(m.Name, Constants.FunctionMetadataBindingProps.IsBatchedKey, StringComparison.OrdinalIgnoreCase)); + + return isBatchedProp != null; + } + /// - /// This method verifies that an EventHubsTrigger matches our expectations on cardinality (isBatched property). If isBatched is set to true, the parameter with the + /// This method verifies that a binding that has Cardinality (isBatched property) is valid. If isBatched is set to true, the parameter with the /// attribute must be an enumerable type. /// - private bool IsEventHubsTriggerValid(IParameterSymbol parameterSymbol, TypeSyntax? parameterTypeSyntax, SemanticModel model, AttributeData attribute, out DataType dataType, out Cardinality cardinality) + private bool IsCardinalityValid(IParameterSymbol parameterSymbol, TypeSyntax? parameterTypeSyntax, SemanticModel model, AttributeData attribute, out DataType dataType) { dataType = DataType.Undefined; - cardinality = Cardinality.Undefined; + var cardinalityIsNamedArg = false; // check if IsBatched is defined in the NamedArguments foreach (var arg in attribute.NamedArguments) @@ -591,42 +600,48 @@ private bool IsEventHubsTriggerValid(IParameterSymbol parameterSymbol, TypeSynta if (String.Equals(arg.Key, Constants.FunctionMetadataBindingProps.IsBatchedKey) && arg.Value.Value != null) { + cardinalityIsNamedArg = true; var isBatched = (bool)arg.Value.Value; // isBatched takes in booleans so we can just type cast it here to use if (!isBatched) { dataType = GetDataType(parameterSymbol.Type); - cardinality = Cardinality.One; return true; } } } - // Check the default value of IsBatched - var eventHubsAttr = attribute.AttributeClass; - var isBatchedProp = eventHubsAttr!.GetMembers().Where(m => string.Equals(m.Name, Constants.FunctionMetadataBindingProps.IsBatchedKey, StringComparison.OrdinalIgnoreCase)).SingleOrDefault(); - AttributeData defaultValAttr = isBatchedProp.GetAttributes().Where(attr => SymbolEqualityComparer.Default.Equals(attr.AttributeClass, Compilation.GetTypeByMetadataName(Constants.Types.DefaultValue))).SingleOrDefault(); - var defaultVal = defaultValAttr.ConstructorArguments.SingleOrDefault().Value!.ToString(); // there is only one constructor arg, the default value - if (!bool.TryParse(defaultVal, out bool b) || !b) + // When "IsBatched" is not a named arg, we have to check the default value + if (!cardinalityIsNamedArg) { - dataType = GetDataType(parameterSymbol.Type); - cardinality = Cardinality.One; - return true; + if (!TryGetIsBatchedProp(attribute, out var isBatchedProp)) + { + dataType = DataType.Undefined; + return false; + } + + var defaultValAttr = isBatchedProp! + .GetAttributes() + .SingleOrDefault(attr => SymbolEqualityComparer.Default.Equals(attr.AttributeClass, Compilation.GetTypeByMetadataName(Constants.Types.DefaultValue))); + + var defaultVal = defaultValAttr!.ConstructorArguments.SingleOrDefault().Value!.ToString(); // there is only one constructor arg for the DefaultValue attribute (the default value) + + if (!bool.TryParse(defaultVal, out bool b) || !b) + { + dataType = GetDataType(parameterSymbol.Type); + return true; + } } // we check if the param is an array type - // we exclude byte arrays (byte[]) b/c we handle that as cardinality one (we handle this similar to how a char[] is basically a string) + // we exclude byte arrays (byte[]) b/c we handle that as Cardinality.One (we handle this similar to how a char[] is basically a string) if (parameterSymbol.Type is IArrayTypeSymbol && !SymbolEqualityComparer.Default.Equals(parameterSymbol.Type, Compilation.GetTypeByMetadataName(Constants.Types.ByteArray))) { dataType = GetDataType(parameterSymbol.Type); - cardinality = Cardinality.Many; return true; } - var isGenericEnumerable = parameterSymbol.Type.IsOrImplementsOrDerivesFrom(Compilation.GetTypeByMetadataName(Constants.Types.IEnumerableGeneric)); - var isEnumerable = parameterSymbol.Type.IsOrImplementsOrDerivesFrom(Compilation.GetTypeByMetadataName(Constants.Types.IEnumerable)); - - // Check if mapping type - mapping enumerables are not valid types for EventHubParameters + // Check if mapping type - mapping enumerables are not valid types for Cardinality.Many if (parameterSymbol.Type.IsOrImplementsOrDerivesFrom(Compilation.GetTypeByMetadataName(Constants.Types.IEnumerableOfKeyValuePair)) || parameterSymbol.Type.IsOrImplementsOrDerivesFrom(Compilation.GetTypeByMetadataName(Constants.Types.LookupGeneric)) || parameterSymbol.Type.IsOrImplementsOrDerivesFrom(Compilation.GetTypeByMetadataName(Constants.Types.DictionaryGeneric))) @@ -634,9 +649,11 @@ private bool IsEventHubsTriggerValid(IParameterSymbol parameterSymbol, TypeSynta return false; } + var isGenericEnumerable = parameterSymbol.Type.IsOrImplementsOrDerivesFrom(Compilation.GetTypeByMetadataName(Constants.Types.IEnumerableGeneric)); + var isEnumerable = parameterSymbol.Type.IsOrImplementsOrDerivesFrom(Compilation.GetTypeByMetadataName(Constants.Types.IEnumerable)); + if (!IsStringType(parameterSymbol.Type) && (isGenericEnumerable || isEnumerable)) { - cardinality = Cardinality.Many; if (IsStringType(parameterSymbol.Type)) { dataType = DataType.String; diff --git a/test/Sdk.Generator.Tests/FunctionMetadataProviderGeneratorTests/EventHubsBindingsTests.cs b/test/Sdk.Generator.Tests/FunctionMetadataProviderGeneratorTests/EventHubsBindingsTests.cs index f964544c1..63c5791d9 100644 --- a/test/Sdk.Generator.Tests/FunctionMetadataProviderGeneratorTests/EventHubsBindingsTests.cs +++ b/test/Sdk.Generator.Tests/FunctionMetadataProviderGeneratorTests/EventHubsBindingsTests.cs @@ -1,9 +1,11 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the MIT License. See License.txt in the project root for license information. +using System.Collections.Generic; using System.Reflection; using System.Text; using Microsoft.Azure.Functions.Worker.Sdk.Generators; +using Microsoft.CodeAnalysis.Testing; using Xunit; namespace Microsoft.Azure.Functions.SdkGeneratorTests @@ -790,11 +792,20 @@ public static void InvalidEventHubsTrigger([EventHubTrigger(""test"", Connection string? expectedGeneratedFileName = null; string? expectedOutput = null; - await TestHelpers.RunTestAsync( + var expectedDiagnosticResults = new List + { + new DiagnosticResult(DiagnosticDescriptors.InvalidCardinality) + .WithSpan(15, 146, 15, 151) + // these arguments are the values we pass as the message format parameters when creating the DiagnosticDescriptor instance. + .WithArguments("input") + }; + + await TestHelpers.RunTestAsync( _referencedExtensionAssemblies, inputCode, expectedGeneratedFileName, - expectedOutput); + expectedOutput, + expectedDiagnosticResults); } } } diff --git a/test/Sdk.Generator.Tests/FunctionMetadataProviderGeneratorTests/StorageBindingTests.cs b/test/Sdk.Generator.Tests/FunctionMetadataProviderGeneratorTests/StorageBindingTests.cs index a0747389c..2195ce7f8 100644 --- a/test/Sdk.Generator.Tests/FunctionMetadataProviderGeneratorTests/StorageBindingTests.cs +++ b/test/Sdk.Generator.Tests/FunctionMetadataProviderGeneratorTests/StorageBindingTests.cs @@ -1,8 +1,10 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the MIT License. See License.txt in the project root for license information. +using System.Collections.Generic; using System.Reflection; using Microsoft.Azure.Functions.Worker.Sdk.Generators; +using Microsoft.CodeAnalysis.Testing; using Xunit; namespace Microsoft.Azure.Functions.SdkGeneratorTests @@ -18,7 +20,7 @@ public StorageBindingTests() // load all extensions used in tests (match extensions tested on E2E app? Or include ALL extensions?) var abstractionsExtension = Assembly.LoadFrom("Microsoft.Azure.Functions.Worker.Extensions.Abstractions.dll"); var httpExtension = Assembly.LoadFrom("Microsoft.Azure.Functions.Worker.Extensions.Http.dll"); - var storageExtension = Assembly.LoadFrom("Microsoft.Azure.Functions.Worker.Extensions.Storage.dll"); + var storageExtension = Assembly.LoadFrom("Microsoft.Azure.Functions.Worker.Extensions.Storage.dll"); var queueExtension = Assembly.LoadFrom("Microsoft.Azure.Functions.Worker.Extensions.Storage.Queues.dll"); var hostingExtension = Assembly.LoadFrom("Microsoft.Extensions.Hosting.dll"); var diExtension = Assembly.LoadFrom("Microsoft.Extensions.DependencyInjection.dll"); @@ -217,7 +219,7 @@ public Task> GetFunctionMetadataAsync(string d metadataList.Add(Function1); var Function2RawBindings = new List(); Function2RawBindings.Add(@"{""name"":""$return"",""type"":""Queue"",""direction"":""Out"",""queueName"":""queue2""}"); - Function2RawBindings.Add(@"{""name"":""blobs"",""type"":""Blob"",""direction"":""In"",""blobPath"":""container2"",""cardinality"":""Many""}"); + Function2RawBindings.Add(@"{""name"":""blobs"",""type"":""Blob"",""direction"":""In"",""blobPath"":""container2"",""cardinality"":""Many"",""dataType"":""String""}"); var Function2 = new DefaultFunctionMetadata { @@ -257,6 +259,51 @@ await TestHelpers.RunTestAsync( expectedGeneratedFileName, expectedOutput); } + + [Fact] + public async void TestInvalidBlobCardinalityMany() + { + string inputCode = """ + using System; + using System.Collections.Generic; + using System.Linq; + using System.Net; + using System.Text.Json.Serialization; + using Microsoft.Azure.Functions.Worker; + using Microsoft.Azure.Functions.Worker.Http; + + namespace FunctionApp + { + public class BlobTest + { + [Function("Function1")] + public HttpResponseData Run([HttpTrigger(AuthorizationLevel.Anonymous, "get", "post")] HttpRequestData req, + [BlobInput("input-container", Connection = "AzureWebJobsStorage", IsBatched = true)] string blobs) + { + throw new NotImplementedException(); + } + } + } + """; + + string? expectedGeneratedFileName = null; + string? expectedOutput = null; + + var expectedDiagnosticResults = new List + { + new DiagnosticResult(DiagnosticDescriptors.InvalidCardinality) + .WithSpan(15, 105, 15, 110) + // these arguments are the values we pass as the message format parameters when creating the DiagnosticDescriptor instance. + .WithArguments("blobs") + }; + + await TestHelpers.RunTestAsync( + _referencedExtensionAssemblies, + inputCode, + expectedGeneratedFileName, + expectedOutput, + expectedDiagnosticResults); + } } } }