-
Notifications
You must be signed in to change notification settings - Fork 374
Skip DAPR1301 analyzer when AddDaprWorkflowVersioning is present #1778
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
Changes from 2 commits
5d766de
6bcf97a
a80f353
08e73fb
104a502
e62aaa3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,9 @@ namespace Dapr.Workflow.Analyzers; | |
| [DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
| public class WorkflowRegistrationAnalyzer : DiagnosticAnalyzer | ||
| { | ||
| private const string WorkflowVersioningExtensionsMetadataName = | ||
| "Dapr.Workflow.Versioning.WorkflowVersioningServiceCollectionExtensions"; | ||
|
|
||
| internal static readonly DiagnosticDescriptor WorkflowDiagnosticDescriptor = new( | ||
| id: "DAPR1301", | ||
| title: new LocalizableResourceString(nameof(Resources.DAPR1301Title), Resources.ResourceManager, typeof(Resources)), | ||
|
|
@@ -33,7 +36,13 @@ public override void Initialize(AnalysisContext context) | |
| { | ||
| context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); | ||
| context.EnableConcurrentExecution(); | ||
| context.RegisterSyntaxNodeAction(AnalyzeWorkflowRegistration, SyntaxKind.InvocationExpression); | ||
| context.RegisterCompilationStartAction(compilationContext => | ||
| { | ||
| if (CheckIfWorkflowVersioningIsRegistered(compilationContext.Compilation)) | ||
| return; | ||
|
|
||
| compilationContext.RegisterSyntaxNodeAction(AnalyzeWorkflowRegistration, SyntaxKind.InvocationExpression); | ||
| }); | ||
| } | ||
|
|
||
| private static void AnalyzeWorkflowRegistration(SyntaxNodeAnalysisContext context) | ||
|
|
@@ -70,6 +79,28 @@ private static void AnalyzeWorkflowRegistration(SyntaxNodeAnalysisContext contex | |
| context.ReportDiagnostic(diagnostic); | ||
| } | ||
|
|
||
| private static bool CheckIfWorkflowVersioningIsRegistered(Compilation compilation) | ||
| { | ||
| var versioningExtensionsType = compilation.GetTypeByMetadataName(WorkflowVersioningExtensionsMetadataName); | ||
| if (versioningExtensionsType is null) | ||
| return false; | ||
|
|
||
| foreach (var syntaxTree in compilation.SyntaxTrees) | ||
| { | ||
| var root = syntaxTree.GetRoot(); | ||
| foreach (var invocation in root.DescendantNodes().OfType<InvocationExpressionSyntax>()) | ||
| { | ||
| if (invocation.Expression is MemberAccessExpressionSyntax memberAccess && | ||
| memberAccess.Name.Identifier.Text == "AddDaprWorkflowVersioning") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While a minor point, I would like for this to look for the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in a80f353. The check now uses the deferred-diagnostics pattern to get a proper semantic model: when the versioning type is present in the compilation, a |
||
| { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| private static bool CheckIfWorkflowIsRegistered(INamedTypeSymbol workflowType, SemanticModel semanticModel, CancellationToken cancellationToken) | ||
| { | ||
| var methodInvocations = new List<InvocationExpressionSyntax>(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While workflow versioning will automatically pick up all the workflows, I would also like for the analyzer to still run in case the user happens to define their own Workflow DI registrations, so can you ensure this scenario is covered as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in a80f353. The deferred-diagnostics pattern keeps the per-workflow
CheckIfWorkflowIsRegisteredlogic fully active even when versioning is present. For everyScheduleNewWorkflowAsync(nameof(X))call, a node action checks explicitRegisterWorkflow<X>registrations first — if found, no diagnostic is added. If not found, a pending diagnostic is collected and later suppressed only becauseAddDaprWorkflowVersioningwas confirmed. I also added a testVerifyWorkflowRegisteredWithVersioningPresentcovering the combination of versioning + explicit registration.