Better validation - #25
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #25 +/- ##
==========================================
+ Coverage 77.95% 78.42% +0.46%
==========================================
Files 2 2
Lines 186 190 +4
Branches 28 30 +2
==========================================
+ Hits 145 149 +4
Misses 21 21
Partials 20 20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR tightens input validation for MinimalWorker’s periodic and cron worker registration APIs and improves source-generator diagnostics/reporting behavior, with accompanying test updates.
Changes:
- Add fail-fast argument validation for
RunPeriodicBackgroundWorker(interval must be > 0) andRunCronBackgroundWorker(cron expression must be non-empty/whitespace). - Update/add unit tests to assert the new exception behavior for invalid intervals/cron expressions.
- Enhance the source generator to collect/report diagnostics and skip generation for models flagged as errored; refactor generated worker init code to cache some lookups and reduce per-error allocations.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/MinimalWorker.Test/PeriodicWorkerTests.cs | Updates the zero-interval test to expect an exception; adds negative-interval coverage. |
| test/MinimalWorker.Test/CronWorkerTests.cs | Adds theory coverage for null/empty/whitespace cron expressions throwing ArgumentException. |
| src/MinimalWorker/BackgroundWorkerExtensions.cs | Adds argument validation for periodic interval and cron expression. |
| src/MinimalWorker.Generators/WorkerGenerator.cs | Adds diagnostic collection/reporting and filters errored worker models from code generation. |
| src/MinimalWorker.Generators/WorkerEmitter.cs | Refactors emitted code to cache some lookups and adjust tag handling in error paths. |
| src/MinimalWorker.Generators/InvocationModel.cs | Extends the invocation model to track diagnostics and generation-blocking errors. |
| .claude/settings.local.json | Expands allowed Claude Bash commands (ls/publish) in local settings. |
Comments suppressed due to low confidence (1)
src/MinimalWorker.Generators/WorkerGenerator.cs:161
AnalyzeDelegatereturnsfalsewhendelegateType/invokeMethodcan't be resolved, but it doesn't setmodel.HasErrorsor add a diagnostic in those paths. SinceExecute()now only filters outHasErrors, these unresolved delegates can still be treated as valid and code generation may proceed with an incomplete/incorrect model. Mark the model as errored and report a diagnostic when the delegate cannot be analyzed (not just for MW0001/MW0002).
if (delegateType == null)
return false;
var invokeMethod = delegateType.DelegateInvokeMethod;
if (invokeMethod == null)
return false;
| public static IWorkerBuilder RunCronBackgroundWorker(this IHost host, string cronExpression, Delegate action) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(cronExpression)) | ||
| throw new ArgumentException("Cron expression cannot be null or empty.", nameof(cronExpression)); |
There was a problem hiding this comment.
The validation uses string.IsNullOrWhiteSpace(cronExpression) but the thrown message says "null or empty". This is inaccurate for whitespace-only inputs; update the message to mention whitespace (or change the guard to IsNullOrEmpty to match the message).
| throw new ArgumentException("Cron expression cannot be null or empty.", nameof(cronExpression)); | |
| throw new ArgumentException("Cron expression cannot be null, empty, or whitespace.", nameof(cronExpression)); |
| "Bash(dotnet build:*)", | ||
| "Bash(ls:*)", | ||
| "Bash(dotnet publish:*)" |
There was a problem hiding this comment.
This PR is titled "Better validation", but this change expands Claude tool permissions (ls/publish). If this is intentional, it should be called out in the PR description; otherwise consider moving it to a separate PR to keep the scope focused.
| foreach (var worker in workers) | ||
| { | ||
| if (worker?.Diagnostics != null) | ||
| { | ||
| foreach (var diag in worker.Diagnostics) | ||
| { | ||
| context.ReportDiagnostic(Diagnostic.Create( | ||
| diag.Descriptor, | ||
| diag.Location, | ||
| diag.MessageArgs ?? System.Array.Empty<object>())); | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var worker in workers) | |
| { | |
| if (worker?.Diagnostics != null) | |
| { | |
| foreach (var diag in worker.Diagnostics) | |
| { | |
| context.ReportDiagnostic(Diagnostic.Create( | |
| diag.Descriptor, | |
| diag.Location, | |
| diag.MessageArgs ?? System.Array.Empty<object>())); | |
| } | |
| foreach (var worker in workers.Where(w => w?.Diagnostics != null)) | |
| { | |
| foreach (var diag in worker.Diagnostics!) | |
| { | |
| context.ReportDiagnostic(Diagnostic.Create( | |
| diag.Descriptor, | |
| diag.Location, | |
| diag.MessageArgs ?? System.Array.Empty<object>())); |
No description provided.