chore(warnings): resolve 25 analyzer quick wins and fix RAPS bugs#261
chore(warnings): resolve 25 analyzer quick wins and fix RAPS bugs#261rlorenzo wants to merge 6 commits into
Conversation
- suppress CA1505 for scaffolded EF contexts and S6964 for ApiPagination, which is never model-bound (editorconfig precedent) - create the Data.CMS logger via ILoggerFactory so the logger category matches its consumer (S6672) - move the Directory route prefix to the controller, URLs unchanged (S6931) - return 400 from RAPS view actions when model binding fails instead of proceeding with defaulted params (S6967) - suppress S6967/S6932 on the RAPS and CTS nav filter overrides with justification; mark RAPSController.Nav [NonAction] to remove an unintended endpoint - make GroupAddEdit.GroupId nullable to reflect create semantics (S6964)
Splits the two large NSubstitute fakes out of ServiceLayerIntegrationTest constructor (CA1502); behavior unchanged.
The BadRequest result was discarded, so a PUT whose body groupId did not match the route fell through and updated the group anyway. Add a regression test asserting the 400 and that the group is unchanged.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #261 +/- ##
==========================================
+ Coverage 45.29% 45.32% +0.02%
==========================================
Files 897 897
Lines 51928 51965 +37
Branches 4878 4886 +8
==========================================
+ Hits 23523 23551 +28
- Misses 27803 27810 +7
- Partials 602 604 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/Areas/Directory/Controllers/DirectoryController.cs`:
- Line 35: Update the Directory home action route in DirectoryController to
preserve the trailing slash, using an area-root route that resolves to
/Directory/; alternatively, add a redirect from /Directory to /Directory/.
Ensure relative "nav" requests continue resolving under the Directory path.
In `@web/Areas/RAPS/Controllers/RAPSController.cs`:
- Around line 94-99: Update the read-only queries in Nav, specifically the
TblRoles, TblPermissions, and VwAaudUser lookups, to use AsNoTracking() before
execution while preserving their existing filters and null-handling behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b12c4003-6bfe-4fe7-9c83-98cd8619e44c
📒 Files selected for processing (9)
.editorconfigtest/ClinicalScheduler/Integration/ServiceLayerIntegrationTest.cstest/RAPS/AdGroupsControllerTests.csweb/Areas/CMS/Controllers/CMSController.csweb/Areas/CTS/Controllers/CTSController.csweb/Areas/Directory/Controllers/DirectoryController.csweb/Areas/RAPS/Controllers/AdGroupsController.csweb/Areas/RAPS/Controllers/RAPSController.csweb/Areas/RAPS/Models/GroupAddEdit.cs
SingleAsync threw on every RAPS page when the memberId query param matched no user; the nav already handles the null case.
OnActionExecutionAsync awaited both the AreaController base, which already invokes next(), and next() again, so every RAPS action ran its pipeline twice.
0375aea to
bd4a19f
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR is the first tranche of the .NET analyzer-warning cleanup plan for VIPER’s ASP.NET backend, combining low-risk warning fixes/suppressions with a few behavior-correctness fixes discovered during the cleanup (notably in RAPS and one API bug).
Changes:
- Fixes several real behavior bugs (e.g., AdGroups PUT ID mismatch fallthrough; RAPS
SingleAsync->SingleOrDefaultAsync; RAPS action pipeline double-execution; request-scoped DbContext capture in background sync). - Adds ModelState guards to multiple RAPS actions and uses targeted analyzer suppressions/
[NonAction]where appropriate. - Refactors a test constructor to reduce complexity and adds a regression test for the AdGroups update bug.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| web/Areas/RAPS/Models/GroupAddEdit.cs | Makes GroupId nullable to better represent “new group” semantics and satisfy analyzer guidance. |
| web/Areas/RAPS/Controllers/RAPSController.cs | Fixes filter pipeline double-execution, hardens nav member lookup, and moves OU sync to a background scope. |
| web/Areas/RAPS/Controllers/AdGroupsController.cs | Fixes missing return on mismatched PUT ID to prevent unintended updates. |
| web/Areas/Directory/Controllers/DirectoryController.cs | Consolidates route prefix at controller level while keeping endpoints consistent. |
| web/Areas/CTS/Controllers/CTSController.cs | Adds targeted analyzer suppression for filter override. |
| web/Areas/CMS/Controllers/CMSController.cs | Uses ILoggerFactory to create a Data.CMS-category logger without triggering the analyzer. |
| test/RAPS/AdGroupsControllerTests.cs | Adds regression coverage for the mismatched group ID PUT bug. |
| test/ClinicalScheduler/Integration/ServiceLayerIntegrationTest.cs | Extracts mock factories to reduce constructor complexity. |
| .editorconfig | Adds/extends justified suppressions for generated contexts and non-bound model types. |
| using var scope = _scopeFactory.CreateScope(); | ||
| var context = scope.ServiceProvider.GetRequiredService<RAPSContext>(); | ||
| try | ||
| { | ||
| await new OuGroupService(context).Sync(groupId, groupName); | ||
| } | ||
| catch (Exception ex) when (ex is DbUpdateException or InvalidOperationException | ||
| or System.DirectoryServices.Protocols.LdapException | ||
| or System.DirectoryServices.Protocols.DirectoryOperationException | ||
| or HttpRequestException) | ||
| { | ||
| LogManager.GetCurrentClassLogger().Error(ex, "Group sync failed for group {GroupId}", groupId); | ||
| } |
There was a problem hiding this comment.
First point applied in 9c1af27: CreateScope()/GetRequiredService<RAPSContext>() now resolve inside the try, and ObjectDisposedException was added to the filter for the host-shutdown race, so nothing can escape the fire-and-forget task unobserved.
On log injection: groupId here is a model-bound int (guarded by the new ModelState.IsValid check), so it cannot carry CRLF or any injection payload. The codebase convention (LogSanitizer.SanitizeId) only accepts strings and is applied to user-supplied string identifiers; CLAUDE.md scopes the rule to user-supplied string input. Leaving the int as-is.
The fire-and-forget sync captured the request-scoped RAPSContext, which is disposed once the response returns, faulting the background task. Resolve a fresh context from IServiceScopeFactory and log failures instead of leaving them unobserved.
bd4a19f to
9c1af27
Compare
Summary
First branch of the .NET warnings cleanup plan: closes 25 of 44 code-analysis warnings via low-risk fixes and justified suppressions, plus four real bugs found along the way (three in RAPS, one latent test hazard). Six single-purpose commits.
Warning cleanup (25 closed)
[NonAction]onRAPSController.Navremoves an unintended endpoint.editorconfigsuppression forApiPagination(never model-bound, always constructed server-side);GroupAddEdit.GroupIdmade nullable.editorconfigsuppression for scaffolded EF*Context.cs, following the existing S3251 precedentILoggerFactoryforData.CMS; log category unchanged[Route]; all 5 URLs provably unchangedServiceLayerIntegrationTestBug fixes (behavior changes to note)
AdGroupsController.UpdateGroup: missingreturnmeant a PUT with a mismatched group ID fell through and updated the group anyway; now 400. Regression test added (the group-edit UI always sends a matching ID, so normal use is unaffected).SingleAsync500ed every RAPS page when?memberId=matched no user; nowSingleOrDefaultAsyncwith the existing null handling.OnActionExecutionAsyncawaited both theAreaControllerbase (which already invokesnext()) andnext()again.GroupSyncbackground sync: fire-and-forget task captured the request-scopedRAPSContext(disposed when the response returns); now resolves a fresh context fromIServiceScopeFactoryand logs failures.Verification
verify:build: all 25 targeted warnings gone, no new ones; the 19 remaining warnings match the plan's deferred list exactly?roleId=abc400, nonexistent?memberId=200), mismatched-ID group PUT 400, all 5 Directory endpoints, CTS indexNot merged to
Developmentyet; that follows after review.