CodeQL 5: refactor(ClinicalScheduler): catch specific exceptions in services#193
CodeQL 5: refactor(ClinicalScheduler): catch specific exceptions in services#193rlorenzo wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughMultiple ClinicalScheduler services add Microsoft.Data.SqlClient imports (where needed) and replace broad catch-all exception handlers with filtered catches that only handle DbUpdateException, SqlException, and InvalidOperationException; ScheduleEditService preserves broad post-transaction catches annotated with CA1031. ChangesException handler narrowing across ClinicalScheduler services
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #193 +/- ##
=======================================
Coverage 42.96% 42.96%
=======================================
Files 877 877
Lines 51468 51468
Branches 4802 4802
=======================================
Hits 22113 22113
Misses 28831 28831
Partials 524 524
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/Areas/ClinicalScheduler/Services/InstructorScheduleService.cs (1)
108-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove
OperationCanceledExceptionfrom the catch filter.Wrapping
OperationCanceledExceptionintoInvalidOperationExceptionbreaks cancellation flow. Callers expecting cancellation signals will not receive them. LetOperationCanceledExceptionpropagate; catch and wrap only the true data-access failures.Proposed fix
- catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) + catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException) { _logger.LogError(ex, "Error retrieving instructor schedules"); throw new InvalidOperationException("Failed to retrieve instructor schedules", ex); }Note: This pattern appears across multiple ClinicalScheduler service files. Apply the same fix wherever this catch filter pattern exists.
🤖 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/Areas/ClinicalScheduler/Services/InstructorScheduleService.cs` around lines 108 - 112, The catch filter in InstructorScheduleService (the catch block handling exceptions around retrieving instructor schedules) incorrectly includes OperationCanceledException, which should be allowed to propagate; update the catch filter to remove OperationCanceledException so you only catch DbUpdateException, SqlException and InvalidOperationException, keep the _logger.LogError call and rethrow a wrapped InvalidOperationException as before, and apply the same removal of OperationCanceledException from identical catch filters across other ClinicalScheduler service classes.web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs (1)
244-261:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExclude
OperationCanceledExceptionfrom filtered catches to preserve cancellation semantics.These catch filters at lines 244, 362, 473, 498, 540, 568, 671, 704, and 729 include
OperationCanceledException, which causes cancellation signals to be logged and wrapped as ordinary failures. This breaks async/await cancellation propagation. Add an explicit passthrough catch forOperationCanceledExceptionbefore each filtered catch:catch (OperationCanceledException) { throw; } catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException) { // existing handler }🤖 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/Areas/ClinicalScheduler/Services/ScheduleEditService.cs` around lines 244 - 261, In ScheduleEditService.cs update the catch blocks that currently filter on OperationCanceledException (the try/catch around the DB save where variables like mothraId, rotationId, weekIds and LogSanitizer are used) so cancellation is not swallowed: add an explicit catch (OperationCanceledException) { throw; } immediately before the existing filtered catch and remove OperationCanceledException from the when(...) filter (so the filtered catch only checks DbUpdateException, SqlException, InvalidOperationException); keep the current logging and InvalidOperationException wrapping logic unchanged in the filtered catch.web/Areas/ClinicalScheduler/Services/SchedulePermissionService.cs (1)
91-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSeparate cancellation from failure handling—don't suppress
OperationCanceledException.All 8 methods catch
OperationCanceledExceptionalongside database/operational failures and return false/empty/default values. This masks cancellation as permission denial or missing data, breaking async semantics. Since all methods accept and passCancellationToken, cancellation is intentional and must propagate to callers.Add a dedicated catch for
OperationCanceledExceptionthat rethrows, before the catch block that handles actual database failures. Apply to all 8 methods: 91–96, 118–123, 154–159, 187–192, 216–220, 262–268, 308–313, 350–355.🤖 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/Areas/ClinicalScheduler/Services/SchedulePermissionService.cs` around lines 91 - 96, In SchedulePermissionService, do not swallow OperationCanceledException in the multi-exception catch; add an explicit catch (OperationCanceledException) { throw; } immediately before the existing catch (Exception ex) when (ex is DbUpdateException or SqlException or InvalidOperationException or OperationCanceledException) so cancellation is rethrown to callers; update all eight methods that currently catch OperationCanceledException (the blocks that call _userHelper.GetCurrentUser() and _logger.LogError(...)) to follow this pattern so only genuine DB/operational exceptions are handled while cancellations propagate.
🤖 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.
Outside diff comments:
In `@web/Areas/ClinicalScheduler/Services/InstructorScheduleService.cs`:
- Around line 108-112: The catch filter in InstructorScheduleService (the catch
block handling exceptions around retrieving instructor schedules) incorrectly
includes OperationCanceledException, which should be allowed to propagate;
update the catch filter to remove OperationCanceledException so you only catch
DbUpdateException, SqlException and InvalidOperationException, keep the
_logger.LogError call and rethrow a wrapped InvalidOperationException as before,
and apply the same removal of OperationCanceledException from identical catch
filters across other ClinicalScheduler service classes.
In `@web/Areas/ClinicalScheduler/Services/ScheduleEditService.cs`:
- Around line 244-261: In ScheduleEditService.cs update the catch blocks that
currently filter on OperationCanceledException (the try/catch around the DB save
where variables like mothraId, rotationId, weekIds and LogSanitizer are used) so
cancellation is not swallowed: add an explicit catch
(OperationCanceledException) { throw; } immediately before the existing filtered
catch and remove OperationCanceledException from the when(...) filter (so the
filtered catch only checks DbUpdateException, SqlException,
InvalidOperationException); keep the current logging and
InvalidOperationException wrapping logic unchanged in the filtered catch.
In `@web/Areas/ClinicalScheduler/Services/SchedulePermissionService.cs`:
- Around line 91-96: In SchedulePermissionService, do not swallow
OperationCanceledException in the multi-exception catch; add an explicit catch
(OperationCanceledException) { throw; } immediately before the existing catch
(Exception ex) when (ex is DbUpdateException or SqlException or
InvalidOperationException or OperationCanceledException) so cancellation is
rethrown to callers; update all eight methods that currently catch
OperationCanceledException (the blocks that call _userHelper.GetCurrentUser()
and _logger.LogError(...)) to follow this pattern so only genuine DB/operational
exceptions are handled while cancellations propagate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8e04794d-4749-4fa1-abaa-b6b0e8bb36fb
📒 Files selected for processing (8)
web/Areas/ClinicalScheduler/Services/InstructorScheduleService.csweb/Areas/ClinicalScheduler/Services/PersonService.csweb/Areas/ClinicalScheduler/Services/RotationService.csweb/Areas/ClinicalScheduler/Services/ScheduleAuditService.csweb/Areas/ClinicalScheduler/Services/ScheduleEditService.csweb/Areas/ClinicalScheduler/Services/SchedulePermissionService.csweb/Areas/ClinicalScheduler/Services/StudentScheduleService.csweb/Areas/ClinicalScheduler/Services/WeekService.cs
CodeQL cs/catch-of-all-exceptions: replace blanket `catch (Exception)` in 8 ClinicalScheduler service files with a `when` filter restricting to the exception families these methods actually need to wrap (DbUpdateException, SqlException, InvalidOperationException, OperationCanceledException). Anything outside that set now propagates unchanged. Four fire-and-forget catches around post-transaction work (email notifications and audit logging that must not roll back successful DB changes) are kept intentionally broad and marked with `#pragma warning disable CA1031` explaining why. Test EmailNotificationTest.RemoveInstructorScheduleAsync_EmailServiceFails_StillCompletesRemoval enforces this resilience contract.
ReSharper RedundantNameQualifier flagged all 45 occurrences of Microsoft.EntityFrameworkCore.DbUpdateException and Microsoft.Data.SqlClient.SqlException in the when-filtered catches introduced by codeql/5. Replaced with the unqualified type names; 'using Microsoft.Data.SqlClient;' added to the 7 files that didn't already have it.
rlorenzo
force-pushed
the
codeql/5-clinicalscheduler-exceptions
branch
from
aeaf341 to
72211c3
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ClinicalScheduler/Services/PersonService.cs`:
- Line 212: In PersonService, update the catch filter that currently reads catch
(Exception ex) when (ex is DbUpdateException or SqlException or
InvalidOperationException) to remove the redundant SqlException type (leave
DbUpdateException and InvalidOperationException) so the prior dedicated
SqlException handler remains reachable; modify the catch clause in the
PersonService class accordingly.
🪄 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: d5024205-c5f5-434f-b8b7-2dde1f732a9f
📒 Files selected for processing (8)
web/Areas/ClinicalScheduler/Services/InstructorScheduleService.csweb/Areas/ClinicalScheduler/Services/PersonService.csweb/Areas/ClinicalScheduler/Services/RotationService.csweb/Areas/ClinicalScheduler/Services/ScheduleAuditService.csweb/Areas/ClinicalScheduler/Services/ScheduleEditService.csweb/Areas/ClinicalScheduler/Services/SchedulePermissionService.csweb/Areas/ClinicalScheduler/Services/StudentScheduleService.csweb/Areas/ClinicalScheduler/Services/WeekService.cs
Two related fixes to the filtered catches added in c269ad7: 1. OperationCanceledException no longer swallowed. The filters caught cancellation alongside data-access failures, wrapping it as InvalidOperationException (or, in SchedulePermissionService, silently returning `false`/empty results). Dropping it from the `when` filters lets cooperative cancellation propagate naturally. 2. Unreachable types removed from three filters that listed exceptions already handled by a dedicated catch clause above them: - PersonService AddRangeInstructorScheduleAsync: SqlException is caught at the preceding `catch (SqlException ex)`. - ScheduleEditService AddInstructorScheduleAsync and RemoveInstructorScheduleAsync: InvalidOperationException is rethrown by the preceding `catch (InvalidOperationException)` passthrough.
rlorenzo
force-pushed
the
codeql/5-clinicalscheduler-exceptions
branch
from
72211c3 to
39e16db
Compare
2 participants
Summary
Closes ~39 of 43
cs/catch-of-all-exceptionsalerts inweb/Areas/ClinicalScheduler/Services/**. Replaces blanketcatch (Exception)with awhenfilter that restricts to the exception families these service methods actually need to wrap (DbUpdateException,SqlException,InvalidOperationException) per the CLAUDE.md convention.Why 4 broad catches are kept
Four catches wrap fire-and-forget post-transaction work - email notifications and audit logging that must run after a successful DB change and must not roll the change back. The
EmailNotificationTest.RemoveInstructorScheduleAsync_EmailServiceFails_StillCompletesRemovaltest enforces this resilience contract by throwing a rawExceptionfrom the mocked email service and asserting that the schedule removal still succeeds. These four sites are kept broad and explicitly annotated:CodeQL will still flag these four; they're known-intentional and can be dismissed in the dashboard.
Cancellation handling
The
whenfilters originally includedOperationCanceledException, which caused cancellation to be wrapped asInvalidOperationException(or, inSchedulePermissionService, silently returnedfalse/empty). Removed from all filters so cooperative cancellation now propagates naturally to callers — a behavior change addressing review feedback.Files changed
InstructorScheduleService.cs- 1 catch narrowedPersonService.cs- 6 catches narrowedRotationService.cs- 7 catches narrowedScheduleAuditService.cs- 3 catches narrowedScheduleEditService.cs- 9 narrowed, 4 kept broad with pragmaSchedulePermissionService.cs- 8 catches narrowedStudentScheduleService.cs- 1 catch narrowedWeekService.cs- 4 catches narrowedContext
Fifth in the
CodeQL N:cleanup series (after #189, #190, #191, #192).Test plan
npm run test:backend- 1946 tests passing (including the email-failure resilience test)npm run verify:build- clean (0 errors)