Add a Job Analyzer to check if scheduled jobs have corresponding invocation urls #1477

siri-varma · 2025-02-28T22:22:50Z

Description

Added a Roslyn Analyzer that checks if every scheduled job has a Job invocation url.

The analyzer checks for patterns like

/job/123
/job/{jobName}

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #1426

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Extended the documentation

Thank you @ngruson for the initial PRs. It served as a great reference

siri-varma · 2025-02-28T22:32:01Z

In the Dapr Jobs client, we have MapDaprScheduledJobHandler. However, the analyzer still throws an error because it is unable to detect the MapPost call within the method. I think this is what you were mentioning about @WhitWaldo. If we need to address this scenario, I can add a separate analyzer that checks for the presence of MapDaprScheduledJobHandler.

siri-varma · 2025-02-28T22:32:58Z

Will sign the commits shortly

WhitWaldo

Couple of notes

WhitWaldo · 2025-02-28T22:26:04Z

all.sln

@@ -155,6 +155,10 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "JobsSample", "examples\Jobs
 EndProject
 Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Dapr.Workflow.Test", "test\Dapr.Workflow.Test\Dapr.Workflow.Test.csproj", "{E90114C6-86FC-43B8-AE5C-D9273CF21FE4}"
 EndProject
+Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Dapr.Jobs.Analyzer", "src\Dapr.Jobs.Analyzer\Dapr.Jobs.Analyzer.csproj", "{28B87C37-4B52-400F-B84D-64F134931BDC}"


Could you please rename the projects as Dapr.Jobs.Analyzers and Dapr.Jobs.Analyzers.Tests so they're consistent with other analyzer projects?

WhitWaldo · 2025-02-28T22:29:18Z

src/Dapr.Jobs.Analyzer/DaprJobsAnalyzerAnalyzer.cs

+        private static readonly DiagnosticDescriptor DaprJobHandlerRule = new DiagnosticDescriptor(
+            id: "DAPRJOBS0001",
+            title: "Ensure Post Mapper handler is present for all the Scheduled Jobs",
+            messageFormat: "There should be a mapping post endpoint for each scheduled job to make sure app receives notifications for all the scheduled jobs",


Could we reword this to be something like "Job invocations require the MapJobInvocationHandler be set and configured for each anticipated job on IEndpointRouteBuilder" with something similar for the title?

WhitWaldo · 2025-02-28T22:32:44Z

src/Dapr.Jobs.Analyzer/DaprJobsAnalyzerAnalyzer.cs

+            }
+        }
+
+        private static void CheckForEndpointRoute(SyntaxNodeAnalysisContext context, string jobName)


While this looks like it can determine if an invocation mapping is registered with IEndpointRouteBuilder or not, it's not actually necessarily if the application never registers a job. Especially as jobs are only invoked on the application that registers them, if it's never registering them, no mapping is necessary as it's never invoked.

Given that, might we look around the assembly and see if there's any use of ScheduleJobAsync on a DaprJobClient type first?

Ah, based on your tests, it looks like you're already doing this in line 52 - could you add a comment indicating that's what magic is happening there?

https://github.com/dapr/dotnet-sdk/pull/1477/files#diff-95a4bc519fc6b5c49452acf4c17caa549c2f54e1e114de4431bf24ef32d51c3bR52

Thats right. If you see the line 52 line there is an if check. We first determine if ScheduleJobAsync is present as you mentioned and then only call the CheckForEndpointRoute

test/Dapr.Jobs.Analyzer.Test/DaprJobAnalyzerTest.cs

src/Dapr.Jobs.Analyzer/DaprJobsAnalyzerAnalyzer.cs

WhitWaldo

Few more thoughts

WhitWaldo · 2025-02-28T22:36:20Z

test/Dapr.Jobs.Analyzer.Test/DaprJobAnalyzerTest.cs

+        }
+
+        [Fact]
+        public async Task AnalyzeJobSchedulerHandler_ShouldRaiseDiagnostic_WhenJobHasHasEndpointButDoesNotMapToJobName()


Seems like this sort of specificity wouldn't end well if the developer passes in a variable to register and subscribe without a constant string.

In addition, string interpolation and concatenation are other cases where this approach may not work. If we want to avoid this complexity, we could simplify the analyzer to check only for the usage of MapDaprScheduledJobHandler. However, this approach has a caveat—we would still generate warnings if developers use IEndpointRouteBuilder directly.

A better approach might be to check for both IEndpointRouteBuilder and MapDaprScheduledJobHandler. This ensures that a mapping exists for the job while also acknowledging that this approach will have some false positive warnings.

I will try to think if there are other approaches available.

The number of places from where the MapPost endpoint can be populated is not exhaustive. It can come from a Config File, other nugets, and string concatenation and interpolations. Going down the current route will lead to large number of False positive which as we were discussing is not a good experience.

I am going to resort back to just checking if MapDaprScheduledJobHandler is being called when "ScheduleJobAsync" is present. Will push the changes in a bit

Pushed new changes

WhitWaldo · 2025-02-28T22:38:14Z

src/Dapr.Jobs.Analyzer/DaprJobsAnalyzerAnalyzer.cs

+            }
+        }
+
+        private static void CheckForEndpointRoute(SyntaxNodeAnalysisContext context, string jobName)


Ah, based on your tests, it looks like you're already doing this in line 52 - could you add a comment indicating that's what magic is happening there?

siri-varma · 2025-03-12T05:32:34Z

@WhitWaldo I have addressed all the comments. Please let me know if any additional changes are required.

WhitWaldo · 2025-03-17T01:28:05Z

Changing the base to merge into release-1.16 instead of master.

WhitWaldo

Change to the analyzer ID, otherwise looks good.

src/Dapr.Jobs.Analyzer/AnalyzerReleases.Shipped.md

src/Dapr.Jobs.Analyzer/MapDaprScheduledJobHandlerAnalyzer.cs

test/Dapr.Jobs.Analyzer.Test/MapDaprScheduledJobHandlerTest.cs

Signed-off-by: Siri Varma Vegiraju <[email protected]>

siri-varma requested review from a team as code owners February 28, 2025 22:22

siri-varma changed the title ~~Users/svegiraju/job analyzer~~ Add a Job Analyzer to check if scheduled jobs have corresponding invocation urls Feb 28, 2025

WhitWaldo assigned siri-varma Feb 28, 2025

WhitWaldo added area/client/jobs kind/enhancement New feature or request labels Feb 28, 2025

WhitWaldo added this to the v1.16 milestone Feb 28, 2025

siri-varma force-pushed the users/svegiraju/job-analyzer branch from 8505bf5 to 6d55fe8 Compare February 28, 2025 22:34

WhitWaldo requested changes Feb 28, 2025

View reviewed changes

siri-varma force-pushed the users/svegiraju/job-analyzer branch from 33ea75e to 2f377f5 Compare March 3, 2025 20:52

siri-varma requested a review from WhitWaldo March 12, 2025 05:30

WhitWaldo changed the base branch from master to release-1.16 March 17, 2025 01:27

WhitWaldo requested changes Mar 17, 2025

View reviewed changes

svegiraju-microsoft and others added 10 commits March 17, 2025 07:47

Add Analyzer

3c2b5b6

Signed-off-by: Siri Varma Vegiraju <[email protected]>

Remvoe files

488cf0c

Signed-off-by: Siri Varma Vegiraju <[email protected]>

Fix deps

674cda4

Signed-off-by: Siri Varma Vegiraju <[email protected]>

Add tests

2a8a025

Signed-off-by: Siri Varma Vegiraju <[email protected]>

Add tests

2cc9b3a

Signed-off-by: Siri Varma Vegiraju <[email protected]>

remove unnecessary

17d24fb

Signed-off-by: Siri Varma Vegiraju <[email protected]>

Fix things

cc1fe0b

Signed-off-by: Siri Varma Vegiraju <[email protected]>

Address comments

30803e9

Signed-off-by: Siri Varma Vegiraju <[email protected]>

Change job id

6b1d45e

Signed-off-by: Siri Varma Vegiraju <[email protected]>

Remove .net6,7

d3e5334

Signed-off-by: Siri Varma Vegiraju <[email protected]>

siri-varma force-pushed the users/svegiraju/job-analyzer branch from 6d2c212 to d3e5334 Compare March 17, 2025 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a Job Analyzer to check if scheduled jobs have corresponding invocation urls #1477

Add a Job Analyzer to check if scheduled jobs have corresponding invocation urls #1477

siri-varma commented Feb 28, 2025 •

edited

Loading

siri-varma commented Feb 28, 2025

siri-varma commented Feb 28, 2025

WhitWaldo left a comment

WhitWaldo Feb 28, 2025

WhitWaldo Feb 28, 2025

WhitWaldo Feb 28, 2025

WhitWaldo Feb 28, 2025

siri-varma Feb 28, 2025

WhitWaldo left a comment

WhitWaldo Feb 28, 2025

siri-varma Mar 1, 2025

siri-varma Mar 3, 2025 •

edited

Loading

siri-varma Mar 3, 2025

WhitWaldo Feb 28, 2025

siri-varma commented Mar 12, 2025

WhitWaldo commented Mar 17, 2025

WhitWaldo left a comment

Add a Job Analyzer to check if scheduled jobs have corresponding invocation urls #1477

Are you sure you want to change the base?

Add a Job Analyzer to check if scheduled jobs have corresponding invocation urls #1477

Conversation

siri-varma commented Feb 28, 2025 • edited Loading

Description

Issue reference

Checklist

siri-varma commented Feb 28, 2025

siri-varma commented Feb 28, 2025

WhitWaldo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WhitWaldo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siri-varma Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siri-varma commented Mar 12, 2025

WhitWaldo commented Mar 17, 2025

WhitWaldo left a comment

Choose a reason for hiding this comment

siri-varma commented Feb 28, 2025 •

edited

Loading

siri-varma Mar 3, 2025 •

edited

Loading