From 6910409b8b3090215c4807f392820aa8c4396258 Mon Sep 17 00:00:00 2001 From: sarah <35204912+satvu@users.noreply.github.com> Date: Tue, 27 Jun 2023 16:38:55 -0700 Subject: [PATCH] [Source Gen] Add retry options diagnostic descriptors (#1683) --- docs/analyzer-rules/AZFW0012.md | 30 +++++++++++++++ sdk/Sdk.Generators/DiagnosticDescriptors.cs | 11 +++++- ...nctionMetadataProviderGenerator.Emitter.cs | 12 +++--- ...unctionMetadataProviderGenerator.Parser.cs | 16 ++++---- sdk/release_notes.md | 1 + .../DiagnosticResultTests.cs | 38 +++++++++++++++++++ .../RetryOptionsTests.cs | 5 ++- 7 files changed, 96 insertions(+), 17 deletions(-) create mode 100644 docs/analyzer-rules/AZFW0012.md diff --git a/docs/analyzer-rules/AZFW0012.md b/docs/analyzer-rules/AZFW0012.md new file mode 100644 index 000000000..23ecbc326 --- /dev/null +++ b/docs/analyzer-rules/AZFW0012.md @@ -0,0 +1,30 @@ +# AZFW0012: Invalid Retry Options + +| | Value | +|-|-| +| **Rule ID** |AZFW0012| +| **Category** |[AzureFunctionsSyntax]| +| **Severity** |Error| + +## Cause + +This rule is triggered when a retry attribute is incorrectly used on an Azure Function. + +## Rule description + +This rule is triggered in two scenarios: + +- A retry strategy is not recognized (only Fixed Delay and Exponential Backoff are supported) +- A retry attribute is used on a trigger type that does not support function-level retry. + +A list of triggers that support function-level retry can be found [here](https://learn.microsoft.com/en-us/azure/azure-functions/functions-bindings-error-pages?tabs=fixed-delay%2Cin-process&pivots=programming-language-csharp#retries). + +## How to fix violations + +Use the supported retry attributes on the correct trigger types. This information can be found [here](https://learn.microsoft.com/en-us/azure/azure-functions/functions-bindings-error-pages?tabs=fixed-delay%2Cin-process&pivots=programming-language-csharp#retries). + +Example functions using retry attributes can be found on the [extensions sample app](https://github.com/Azure/azure-functions-dotnet-worker/tree/main/samples/Extensions) in the source repository. There are examples on the CosmosDB function, the timer function, and more. + +## When to suppress warnings + +This rule should not be suppressed because this error will lead to unexpected retry behavior. \ No newline at end of file diff --git a/sdk/Sdk.Generators/DiagnosticDescriptors.cs b/sdk/Sdk.Generators/DiagnosticDescriptors.cs index 1d305fc23..2ebf9408a 100644 --- a/sdk/Sdk.Generators/DiagnosticDescriptors.cs +++ b/sdk/Sdk.Generators/DiagnosticDescriptors.cs @@ -51,8 +51,15 @@ private static DiagnosticDescriptor Create(string id, string title, string messa public static DiagnosticDescriptor InvalidCardinality { get; } = Create(id: "AZFW0008", - 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.", + 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); + + public static DiagnosticDescriptor InvalidRetryOptions { get; } + = Create(id: "AZFW0012", + title: "Invalid operation with a retry attribute.", + messageFormat: "Invalid use of a retry attribute. Check that the attribute is used on a trigger that supports function-level retry.", category: "FunctionMetadataGeneration", severity: DiagnosticSeverity.Error); } diff --git a/sdk/Sdk.Generators/FunctionMetadataProviderGenerator/FunctionMetadataProviderGenerator.Emitter.cs b/sdk/Sdk.Generators/FunctionMetadataProviderGenerator/FunctionMetadataProviderGenerator.Emitter.cs index 06f3e3844..539ab1bd9 100644 --- a/sdk/Sdk.Generators/FunctionMetadataProviderGenerator/FunctionMetadataProviderGenerator.Emitter.cs +++ b/sdk/Sdk.Generators/FunctionMetadataProviderGenerator/FunctionMetadataProviderGenerator.Emitter.cs @@ -134,12 +134,12 @@ private StringBuilder BuildRetryOptions(GeneratorRetryOptions? retry) else if (retry?.Strategy is RetryStrategy.ExponentialBackoff) { builder.AppendLine($$""" - Retry = new DefaultRetryOptions - { - MaxRetryCount = {{retry!.MaxRetryCount}}, - MinimumInterval = TimeSpan.Parse("{{retry.MinimumInterval}}"), - MaximumInterval = TimeSpan.Parse("{{retry.MaximumInterval}}") - }, + Retry = new DefaultRetryOptions + { + MaxRetryCount = {{retry!.MaxRetryCount}}, + MinimumInterval = TimeSpan.Parse("{{retry.MinimumInterval}}"), + MaximumInterval = TimeSpan.Parse("{{retry.MaximumInterval}}") + }, """); } diff --git a/sdk/Sdk.Generators/FunctionMetadataProviderGenerator/FunctionMetadataProviderGenerator.Parser.cs b/sdk/Sdk.Generators/FunctionMetadataProviderGenerator/FunctionMetadataProviderGenerator.Parser.cs index f28d57004..fa2ba0e1f 100644 --- a/sdk/Sdk.Generators/FunctionMetadataProviderGenerator/FunctionMetadataProviderGenerator.Parser.cs +++ b/sdk/Sdk.Generators/FunctionMetadataProviderGenerator/FunctionMetadataProviderGenerator.Parser.cs @@ -6,6 +6,7 @@ using System.Collections.Immutable; using System.IO; using System.Linq; +using System.Reflection; using System.Threading; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp.Syntax; @@ -114,13 +115,14 @@ private bool TryGetBindings(MethodDeclarationSyntax method, SemanticModel model, if (retryOptions is not null) { - if (supportsRetryOptions && retryOptions is not null) + if (supportsRetryOptions) { validatedRetryOptions = retryOptions; } - else if (!supportsRetryOptions && retryOptions is not null) + else if (!supportsRetryOptions) { - // TODO: Log retry options related diagnostic error here + _context.ReportDiagnostic(Diagnostic.Create(DiagnosticDescriptors.InvalidRetryOptions, method.GetLocation())); + return false; } } @@ -145,7 +147,7 @@ private bool TryGetMethodOutputBinding(MethodDeclarationSyntax method, SemanticM { if (SymbolEqualityComparer.Default.Equals(attribute.AttributeClass?.BaseType, _knownFunctionMetadataTypes.RetryAttribute)) { - if (TryGetRetryOptionsFromAtttribute(attribute, out GeneratorRetryOptions? retryOptionsFromAttr)) + if (TryGetRetryOptionsFromAttribute(attribute, method.GetLocation(), out GeneratorRetryOptions? retryOptionsFromAttr)) { retryOptions = retryOptionsFromAttr; } @@ -188,7 +190,7 @@ private bool TryGetMethodOutputBinding(MethodDeclarationSyntax method, SemanticM return true; } - private bool TryGetRetryOptionsFromAtttribute(AttributeData attribute, out GeneratorRetryOptions? retryOptions) + private bool TryGetRetryOptionsFromAttribute(AttributeData attribute, Location location, out GeneratorRetryOptions? retryOptions) { retryOptions = null; @@ -224,12 +226,12 @@ private bool TryGetRetryOptionsFromAtttribute(AttributeData attribute, out Gener if (attrProperties.TryGetValue(Constants.RetryConstants.MaximumIntervalKey, out object? maximumInterval)) // nonnullable constructor arg of attribute, wouldn't expect this to fail { - retryOptions.MinimumInterval = maximumInterval!.ToString(); + retryOptions.MaximumInterval = maximumInterval!.ToString(); } } else { - // TODO: Diagnostic error for retry options parsing failure + _context.ReportDiagnostic(Diagnostic.Create(DiagnosticDescriptors.InvalidRetryOptions, location)); return false; } diff --git a/sdk/release_notes.md b/sdk/release_notes.md index fc642e19a..233d3c0ad 100644 --- a/sdk/release_notes.md +++ b/sdk/release_notes.md @@ -17,3 +17,4 @@ - Add support for retry options (#1548) - Bug fix for when DefaultValue is not present on an IsBatched prop (#1602). - SDK changes for .NET 8.0 support (#1643) +- Add diagnostic descriptors for retry options (#1683) diff --git a/test/Sdk.Generator.Tests/FunctionMetadataProviderGeneratorTests/DiagnosticResultTests.cs b/test/Sdk.Generator.Tests/FunctionMetadataProviderGeneratorTests/DiagnosticResultTests.cs index c5cb90402..8478337b2 100644 --- a/test/Sdk.Generator.Tests/FunctionMetadataProviderGeneratorTests/DiagnosticResultTests.cs +++ b/test/Sdk.Generator.Tests/FunctionMetadataProviderGeneratorTests/DiagnosticResultTests.cs @@ -193,6 +193,44 @@ await TestHelpers.RunTestAsync( expectedOutput, expectedDiagnosticResults); } + + [Fact] + public async void InvalidRetryOptionsFailure() + { + var inputCode = @"using System; + using System.Threading.Tasks; + using Microsoft.Azure.Functions.Worker; + using Microsoft.Azure.Functions.Worker.Http; + + namespace FunctionApp + { + public class HttpFunction + { + [Function(""HttpFunction"")] + [FixedDelayRetry(5, ""00:00:10"")] + public string Run([HttpTrigger(""get"")] string req) + { + throw new NotImplementedException(); + } + } + }"; + + string? expectedGeneratedFileName = null; + string? expectedOutput = null; + + var expectedDiagnosticResults = new List + { + new DiagnosticResult(DiagnosticDescriptors.InvalidRetryOptions) + .WithSpan(10, 25, 15, 26) + }; + + await TestHelpers.RunTestAsync( + referencedExtensionAssemblies, + inputCode, + expectedGeneratedFileName, + expectedOutput, + expectedDiagnosticResults); + } } } } diff --git a/test/Sdk.Generator.Tests/FunctionMetadataProviderGeneratorTests/RetryOptionsTests.cs b/test/Sdk.Generator.Tests/FunctionMetadataProviderGeneratorTests/RetryOptionsTests.cs index c3625351b..e105f64f8 100644 --- a/test/Sdk.Generator.Tests/FunctionMetadataProviderGeneratorTests/RetryOptionsTests.cs +++ b/test/Sdk.Generator.Tests/FunctionMetadataProviderGeneratorTests/RetryOptionsTests.cs @@ -143,7 +143,7 @@ namespace SampleApp public static class TimerFunction { [Function("TimerFunction")] - [FixedDelayRetry(5, "00:00:10")] + [ExponentialBackoffRetry(5, "00:00:04", "00:15:00")] public static void Run([TimerTrigger("0 */5 * * * *")] TimerInfo timerInfo, FunctionContext context) { @@ -184,7 +184,8 @@ public Task> GetFunctionMetadataAsync(string d Retry = new DefaultRetryOptions { MaxRetryCount = 5, - DelayInterval = TimeSpan.Parse("00:00:10") + MinimumInterval = TimeSpan.Parse("00:00:04"), + MaximumInterval = TimeSpan.Parse("00:15:00") }, ScriptFile = "TestProject.dll" };