Skip to content

Commit

Permalink
[Source Gen] Fix cardinality validation to include all scenarios (#1402)
Browse files Browse the repository at this point in the history 
* edit diag descriptors for cardinality

* refactor cardinality validation
  • Loading branch information
@satvu
satvu committed Mar 16, 2023
1 parent 2635d7a commit d7baa52
Show file tree
Hide file tree
Showing 5 changed files with 129 additions and 52 deletions.
12 changes: 7 additions & 5 deletions docs/analyzer-rules/AZFW0008.md
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# AZFW0008: Invalid EventHubs Trigger
# AZFW0008: Invalid Cardinality

| | Value |
|-|-|
Expand All @@ -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<string>`. 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

Expand Down
6 changes: 3 additions & 3 deletions sdk/Sdk.Generators/DiagnosticDescriptors.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
97 changes: 57 additions & 40 deletions sdk/Sdk.Generators/FunctionMetadataProviderGenerator.Parser.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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<String> 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;
Expand All @@ -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!);
}
}
Expand Down Expand Up @@ -576,67 +570,90 @@ 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;
}

/// <summary>
/// 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.
/// </summary>
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)
{
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)))
{
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;
Expand Down
15 changes: 13 additions & 2 deletions test/Sdk.Generator.Tests/FunctionMetadataProviderGeneratorTests/EventHubsBindingsTests.cs
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -790,11 +792,20 @@ public static void InvalidEventHubsTrigger([EventHubTrigger(""test"", Connection
string? expectedGeneratedFileName = null;
string? expectedOutput = null;

await TestHelpers.RunTestAsync<ExtensionStartupRunnerGenerator>(
var expectedDiagnosticResults = new List<DiagnosticResult>
{
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<FunctionMetadataProviderGenerator>(
_referencedExtensionAssemblies,
inputCode,
expectedGeneratedFileName,
expectedOutput);
expectedOutput,
expectedDiagnosticResults);
}
}
}
Expand Down
Loading

0 comments on commit d7baa52

Please sign in to comment.