chore(analyzer): clear the warnings from the SonarAnalyzer 10.31 bump - #307
Open
rlorenzo wants to merge 6 commits into
Open
chore(analyzer): clear the warnings from the SonarAnalyzer 10.31 bump#307rlorenzo wants to merge 6 commits into
rlorenzo wants to merge 6 commits into
Conversation
- treat a timed-out match as an unsafe name and fall back to the default, matching the fail-safe path every other rejection in the method takes
- S2077: the interpolated SQL carries only the generated @jc parameter placeholders; every job code binds through AddWithValue - S5332: the CAS literal is an XML namespace identifier, not an endpoint - S4502: the error page is anonymous, binds one route int, mutates nothing - S4790: widen the existing CA5350 disable to Sonar's equivalent rule
Completes c5e5eef, which folded the First/FirstOrDefault shapes and left every Any shape behind. All five receivers are EF DbSets, so the generated SQL is unchanged.
- pass context.RequestAborted to the sitemap writes, the Vite proxy body copy, the static-file fallback and the 502 body, so work stops when the client disconnects instead of running to completion - opt the two SSE Task.Run calls out explicitly with CancellationToken.None: a token there only prevents the task starting, which would skip the lambda catch and leave the channel un-completed
Bundle ReportBundle size has no change ✅ |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
rlorenzo
force-pushed
the
chore/sonar-1031-warnings
branch
from
August 8, 2026 06:33
92fb31b to
5be47a8
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
📝 WalkthroughWalkthroughThe pull request updates nullable-flow handling across tests and web services, adds request-cancellation handling for streamed responses, clarifies background task cancellation, simplifies existence queries, and adds scoped analyzer suppressions. ChangesApplication cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SitemapMiddleware
participant ViteProxyHelpers
participant ResponseStream
Client->>SitemapMiddleware: disconnects request
SitemapMiddleware->>ResponseStream: write with RequestAborted
ResponseStream-->>SitemapMiddleware: cancellation
SitemapMiddleware-->>Client: stop processing
Client->>ViteProxyHelpers: disconnects proxy request
ViteProxyHelpers->>ResponseStream: copy with RequestAborted
ResponseStream-->>ViteProxyHelpers: cancellation
ViteProxyHelpers-->>Client: suppress disconnect exception
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/ViteProxyHelpers.cs (1)
405-416: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle request-abort cancellation in the fallback path.
SendFileAsyncandWriteAsyncpasscontext.RequestAbortedand can throwOperationCanceledExceptionwhen the request is canceled. AddOperationCanceledExceptionto the catch filter, or handle it whencontext.RequestAborted.IsCancellationRequested, then return without writing the502response.🤖 Prompt for 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. In `@web/ViteProxyHelpers.cs` around lines 405 - 416, Update the static-file fallback exception handling around SendFileAsync to recognize request-abort OperationCanceledException when context.RequestAborted is canceled, log or handle it as appropriate, and return before setting status 502 or calling WriteAsync. Preserve the existing handling for file-related exceptions and ensure canceled requests do not attempt a response write.
🤖 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/CTS/Controllers/BundleCompetencyController.cs`:
- Around line 25-30: Update the read-only existence checks in
BundleCompetencyController.cs (lines 25-30) within BundleExists and
CompetencyExists, and BundleCompetencyGroupController.cs (lines 25-33) within
BundleExists and SameNameExists, by adding AsNoTracking() before each Any(...)
call.
In `@web/Areas/Students/Services/StudentList.cs`:
- Around line 61-63: Update the classYear filter in the surrounding student-list
method to replace the correlated _context.StudentClassYears.Any call with a
preloaded, AsNoTracking person-ID set for the requested classYear, then filter
eligible students using EF.Parameter(...).Contains(...) when the set has at
least 10 IDs; preserve the null-student exclusion and existing behavior for
smaller or empty sets.
---
Outside diff comments:
In `@web/ViteProxyHelpers.cs`:
- Around line 405-416: Update the static-file fallback exception handling around
SendFileAsync to recognize request-abort OperationCanceledException when
context.RequestAborted is canceled, log or handle it as appropriate, and return
before setting status 502 or calling WriteAsync. Preserve the existing handling
for file-related exceptions and ensure canceled requests do not attempt a
response write.
🪄 Autofix
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 Plus
Run ID: 325ff915-9b9f-42ec-9d0d-a885ddffc2a4
📒 Files selected for processing (31)
test/Areas/Directory/VMACSServiceTest.cstest/CMS/CMSContentControllerTests.cstest/CMS/CMSFilesControllerTests.cstest/CMS/CMSLeftNavControllerTests.cstest/CMS/CMSOptionsControllerTests.cstest/ClinicalScheduler/PermissionsControllerTest.cstest/Effort/HarvestTimeParserTests.cstest/HealthChecks/HealthCheckCollectorTokenHandlerTests.cstest/Scheduler/ScheduledJobRunnerTests.cstest/Students/EmergencyContactControllerTests.cstest/Students/EmergencyContactServiceTests.csweb/Areas/CMS/Services/CmsFilePathSafety.csweb/Areas/CTS/Controllers/BundleCompetencyController.csweb/Areas/CTS/Controllers/BundleCompetencyGroupController.csweb/Areas/CTS/Models/CompetencyBundleAssociationDto.csweb/Areas/ClinicalScheduler/Controllers/CliniciansController.csweb/Areas/ClinicalScheduler/Controllers/InstructorScheduleController.csweb/Areas/Computing/Services/BiorenderStudentLookup.csweb/Areas/Effort/Controllers/ClinicalImportController.csweb/Areas/Effort/Controllers/PercentRolloverController.csweb/Areas/Effort/Services/ClinicalImportService.csweb/Areas/Effort/Services/ClinicalScheduleService.csweb/Areas/Effort/Services/Harvest/ClinicalHarvestPhase.csweb/Areas/Effort/Services/Harvest/CrestHarvestPhase.csweb/Areas/Effort/Services/InstructorService.csweb/Areas/RAPS/Services/OuGroupService.csweb/Areas/RAPS/Services/UinformService.csweb/Areas/Students/Services/StudentList.csweb/Classes/SitemapMiddleware.csweb/Controllers/HomeController.csweb/ViteProxyHelpers.cs
41 sites flagged by S8969 across 20 files, 21 production and 20 test. Most are a plain deletion; two spots needed the surrounding code to make the non-null state provable instead: - both clinical-import commits guard on ownsTransaction, which Sonar can follow but the compiler cannot; null-check the transaction itself, which is identical by construction - AddInstructor and SetPrimaryEvaluator capture their [Required] nullable fields into non-null locals behind an explicit guard, which also clears the CodeQL nullable-dereference findings on those lines
rlorenzo
force-pushed
the
chore/sonar-1031-warnings
branch
from
August 8, 2026 08:26
5be47a8 to
3c9f501
Compare
There was a problem hiding this comment.
Pull request overview
Reduces warning noise introduced by the SonarAnalyzer 10.31 upgrade by applying targeted refactors and narrowly scoped suppressions, while keeping behavior stable (with a few intentional cancellation-handling tweaks at middleware/proxy boundaries).
Changes:
- Pass
context.RequestAbortedthrough streaming/copy operations and swallow request-abort cancellations where a disconnect is not an error signal. - Remove redundant null-forgiving operators and simplify LINQ patterns (
Where(...).Any()→Any(...)), plus add.AsNoTracking()to existence checks. - Add narrowly scoped suppressions/justifications for security analyzers where the flagged pattern is intentional (parameterized SQL placeholder list, CAS XML namespace literal, anonymous error page CSRF suppression, legacy HMACSHA1 requirement) and harden CMS filename sanitization with a regex timeout.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| web/ViteProxyHelpers.cs | Propagate RequestAborted during proxy response streaming and avoid misclassifying client disconnects as Vite outages. |
| web/Controllers/HomeController.cs | Add scoped suppressions with justifications for CAS XML namespace literal and anonymous error handler antiforgery. |
| web/Classes/SitemapMiddleware.cs | Treat client disconnect during sitemap streaming as a normal cancellation and stop pipeline execution. |
| web/Areas/Students/Services/StudentList.cs | Simplify .Where(...).Any() into .Any(...) within a composed EF query. |
| web/Areas/RAPS/Services/UinformService.cs | Suppress Sonar weak-crypto rule alongside existing CA suppression for legacy HMACSHA1 requirement. |
| web/Areas/RAPS/Services/OuGroupService.cs | Remove a redundant null-forgiving operator in group member login ID projection. |
| web/Areas/Effort/Services/InstructorService.cs | Remove null-forgiving operators in AAUD ID → PKey dictionary creation and downstream usage. |
| web/Areas/Effort/Services/Harvest/CrestHarvestPhase.cs | Remove null-forgiving operators in AAUD ID/PKey extraction and lookups. |
| web/Areas/Effort/Services/Harvest/ClinicalHarvestPhase.cs | Remove redundant null-forgiving operator in importable MothraId filtering. |
| web/Areas/Effort/Services/ClinicalScheduleService.cs | Add justification suppression for interpolated SQL that only injects parameter names. |
| web/Areas/Effort/Services/ClinicalImportService.cs | Make transaction commit guard provable (transaction != null) and reduce null-forgiving usage in title-code filtering. |
| web/Areas/Effort/Controllers/PercentRolloverController.cs | Use CancellationToken.None for Task.Run to ensure channel completion semantics for SSE. |
| web/Areas/Effort/Controllers/ClinicalImportController.cs | Same SSE/channel completion fix as rollover stream. |
| web/Areas/CTS/Models/CompetencyBundleAssociationDto.cs | Remove redundant null-forgiving operator in DTO projection. |
| web/Areas/CTS/Controllers/BundleCompetencyGroupController.cs | Use .AsNoTracking() for existence/name checks and simplify .Where().Any() to .Any(). |
| web/Areas/CTS/Controllers/BundleCompetencyController.cs | Use .AsNoTracking() for existence checks and simplify .Where().Any() to .Any(). |
| web/Areas/Computing/Services/BiorenderStudentLookup.cs | Remove pointless null filtering after Task.WhenAll when tasks always yield constructed results. |
| web/Areas/CMS/Services/CmsFilePathSafety.cs | Add regex timeout + fail-safe handling to prevent request-thread pinning on pathological filenames. |
| web/Areas/ClinicalScheduler/Controllers/InstructorScheduleController.cs | Capture required nullable request fields into non-null locals after explicit guards to satisfy nullable analysis. |
| web/Areas/ClinicalScheduler/Controllers/CliniciansController.cs | Remove redundant null-forgiving operator in rotation ordering projection. |
| test/Students/EmergencyContactServiceTests.cs | Remove redundant null-forgiving in assertions. |
| test/Students/EmergencyContactControllerTests.cs | Remove redundant null-forgiving in result/method assertions. |
| test/Scheduler/ScheduledJobRunnerTests.cs | Remove redundant null-forgiving in assertion. |
| test/HealthChecks/HealthCheckCollectorTokenHandlerTests.cs | Remove redundant null-forgiving in header assertion. |
| test/Effort/HarvestTimeParserTests.cs | Simplify nullable assertion by asserting the concrete type. |
| test/CMS/CMSOptionsControllerTests.cs | Remove redundant null-forgiving in assertions. |
| test/CMS/CMSLeftNavControllerTests.cs | Remove redundant null-forgiving in assertions. |
| test/CMS/CMSFilesControllerTests.cs | Remove redundant null-forgiving in assertions. |
| test/CMS/CMSContentControllerTests.cs | Remove redundant null-forgiving in assertions. |
| test/ClinicalScheduler/PermissionsControllerTest.cs | Remove redundant null-forgiving in dynamic response assertions. |
| test/Areas/Directory/VMACSServiceTest.cs | Remove redundant null-forgiving in assertion. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The Any in GetStudents was correlated to every outer StudentClassYears row. Load the matching person ids once and match with EF.Parameter(...).Contains so the filter becomes a single IN list.
rlorenzo
force-pushed
the
chore/sonar-1031-warnings
branch
from
August 8, 2026 17:37
089a3dd to
f086392
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated no new comments.
Suppressed comments (1)
web/Areas/Students/Services/StudentList.cs:69
- This change materializes all PersonIds for the class year into memory via ToListAsync, which can be large and adds an extra round-trip. You can keep it as a single SQL query without a correlated subquery by using a queryable subquery/IN (SELECT ...) instead of pre-loading the IDs.
.ToListAsync();
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes 58 of the 59 warnings added by the SonarAnalyzer 10.31 bump. Build goes from 79 warnings to 21.
One commit per rule family; the file sets are disjoint, so any single commit can be dropped independently.
context.RequestAborted; explicitCancellationToken.Noneon the two SSETask.RuncallsWhere(...).Any()intoAny(...)AddWithValue, XML namespace literal, anonymous error page, legacy uInform HMACSHA1Behavioural changes, worth a closer look:
ViteProxyHelpersandSitemapMiddlewarenow catchOperationCanceledException when (context.RequestAborted.IsCancellationRequested). Without it a client disconnect read as a Vite outage or a sitemap failure, and fell through to further middleware on an already-started response.if (ownsTransaction)toif (transaction != null): equivalent by construction, but provable to the compiler.AddInstructorandSetPrimaryEvaluatorcapture their[Required]nullable fields into non-null locals behind an explicit guard, clearing the CodeQL nullable-dereference findings on those lines.Not fixed: S1313, the hardcoded F5 internal IP in
ForwardedHeadersExtensions. Moving it to config beside the Cloudflare CIDRs is proxy-trust surface and belongs with theProgram.cswork.Verified: clean-cache
npm run verify:build79 to 21 warnings, zero CS diagnostics;npm run test:backend2708 passed.