CodeQL 3: fix: close CodeQL correctness alerts#191
Open
rlorenzo wants to merge 3 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughTightens null-handling, adds year validation and tests, converts StudentClassYear to sealed, refactors RoleTemplates list/preview logic, and refactors some controller index/nullable checks. ChangesNull-safety and code cleanup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Bundle ReportBundle size has no change ✅ |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
4 tasks
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #191 +/- ##
==========================================
+ Coverage 43.02% 43.08% +0.05%
==========================================
Files 881 881
Lines 51437 51442 +5
Branches 4812 4808 -4
==========================================
+ Hits 22131 22164 +33
+ Misses 28780 28751 -29
- Partials 526 527 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
rlorenzo
force-pushed
the
codeql/3-correctness
branch
2 times, most recently
from
7b95293 to
53b6eef
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 3
🤖 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/RAPS/Controllers/RoleTemplatesController.cs`:
- Around line 45-49: Replace the explicit loop and mutable list plumbing by
projecting dbRoleTemplates into the simplified DTO: use a LINQ Select that calls
RoleTemplateSimplified.FromRoleTemplate for each rt on dbRoleTemplates and
materialize to a List (assign to roleTemplates) instead of creating
roleTemplates and using foreach + Add.
In `@web/Areas/RAPS/Models/RoleTemplateSimplified.cs`:
- Around line 22-31: Replace the manual mapping inside
RoleTemplateSimplified.FromRoleTemplate by delegating to a Mapperly-generated
mapper: change the implementation to call
RoleTemplateSimplifiedMapper.ToSimplified(rt), and add a new static partial
mapper type named RoleTemplateSimplifiedMapper decorated with
[Mapper(RequiredMappingStrategy = RequiredMappingStrategy.None)] that declares a
partial method ToSimplified(RoleTemplate source) returning
RoleTemplateSimplified so Mapperly can generate the mapping.
In `@web/Program.cs`:
- Around line 558-565: The lookup can return null for unknown names; replace the
reflection-based lookup with RegionEndpoint.GetBySystemName(regionValue) (or try
both regionValue and a PascalCase variant) and if that returns null either set
profile.Region = RegionEndpoint.USWest1 as a safe default or throw a
FormatException; update the code around regionValue/profile.Region to use
RegionEndpoint.GetBySystemName(regionValue) and handle a null result explicitly
(throwing FormatException or assigning the default).
🪄 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: 501d2df3-d7b5-44d5-a105-53874a3f7b1b
📒 Files selected for processing (15)
test/CTS/AssessmentControllerTest.csweb/Areas/CMS/Models/CMSFile.csweb/Areas/CTS/Controllers/AssessmentController.csweb/Areas/CTS/Controllers/CompetencyController.csweb/Areas/CTS/Models/AuditRow.csweb/Areas/Effort/Services/PercentRolloverService.csweb/Areas/Effort/Services/PercentageService.csweb/Areas/RAPS/Controllers/RAPSController.csweb/Areas/RAPS/Controllers/RoleTemplatesController.csweb/Areas/RAPS/Models/RoleTemplateSimplified.csweb/Areas/RAPS/Services/RAPSAuditService.csweb/Classes/Utilities/AcademicYearHelper.csweb/Controllers/HomeController.csweb/Models/Students/StudentClassYear.csweb/Program.cs
💤 Files with no reviewable changes (1)
- web/Areas/CTS/Controllers/CompetencyController.cs
rlorenzo
force-pushed
the
codeql/3-correctness
branch
7 times, most recently
from
d915095 to
9743d93
Compare
rlorenzo
force-pushed
the
codeql/3-correctness
branch
from
9743d93 to
df971c6
Compare
Three rule families consolidated into one PR because each fix needs per-site judgment rather than mechanical replacement. cs/virtual-call-in-constructor (5): seal CMSFile and StudentClassYear - neither is inherited from in C#, neither is mocked, and the project has no UseLazyLoadingProxies so EF does not need them non-sealed. Drops `virtual` on properties newly declared inside the now-sealed classes. For RoleTemplateSimplified the same alert is closed by dropping `virtual` from the Roles property, which is a smaller change than sealing and keeps the public constructor surface intact. cs/unsafe-year-construction (4): in PercentRolloverService, bound the HTTP-derived year parameter with ArgumentOutOfRangeException and use .AddYears(1) instead of `new DateTime(year+1, ...)` for the target end date. In AcademicYearHelper.GetAcademicYearStart, build the July date first then .AddYears(-1) instead of subtracting from .Year before constructing. cs/constant-condition (21): remove redundant null-propagation that the analyzer can already prove dead - `?.` chains after `!= null` guards, an unreachable CompetencyId range check after the FindAsync record-existence check, and a duplicated `RegionEndpoint` element lookup in SetAwsCredentials. RAPSController null-flow was restructured to lift the path null check up. The SetAwsCredentials region lookup also gets hardened to truly close the alert: trim the value, look it up with BindingFlags.IgnoreCase, and fall back to USWest1 when the name is non-empty but does not match any known region (previously left profile.Region as null).
rlorenzo
force-pushed
the
codeql/3-correctness
branch
from
df971c6 to
5a244d0
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
This PR closes remaining CodeQL correctness alerts by tightening nullability assumptions, avoiding unsafe year construction patterns, and adding coverage around RAPS role template DTO mapping.
Changes:
- Seals
StudentClassYearand removes unnecessary virtual navigation properties. - Reworks several CodeQL constant-condition sites to use proven non-null values.
- Adds safer date construction and RAPS role-template constructor tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
web/Models/Students/StudentClassYear.cs |
Seals the EF entity and removes virtual navigation properties. |
web/Controllers/HomeController.cs |
Replaces a redundant null-propagating access after username validation. |
web/Classes/Utilities/AcademicYearHelper.cs |
Avoids constructing an invalid prior year directly. |
web/Areas/RAPS/Services/RAPSAuditService.cs |
Removes redundant nullable access on non-null audit entries. |
web/Areas/RAPS/Controllers/RoleTemplatesController.cs |
Simplifies DTO mapping and removes unreachable null fallbacks. |
web/Areas/RAPS/Controllers/RAPSController.cs |
Consolidates path/index null checks. |
web/Areas/Effort/Services/PercentRolloverService.cs |
Adds year bounds and avoids year + 1 DateTime construction. |
web/Areas/Effort/Services/PercentageService.cs |
Removes a redundant null check after validation short-circuiting. |
web/Areas/CTS/Models/AuditRow.cs |
Removes redundant nullable access after an existing navigation guard. |
test/RAPS/RoleTemplateSimplifiedTests.cs |
Adds unit tests for role template DTO projection behavior. |
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/Areas/RAPS/Controllers/RoleTemplatesController.cs (1)
172-181: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRefactor to LINQ projection.
The
foreach+Addpattern should be replaced with.Select(...).ToList()for consistency with the guideline and the pattern already applied at lines 44-46.♻️ Proposed refactor
- foreach (var role in roleTemplate.RoleTemplateRoles) - { - rolesToApply.Add(new RoleApplyPreview - { - RoleId = role.Role.RoleId, - RoleName = role.Role.FriendlyName, - Description = role.Role.Description, - UserHasRole = userRoles.Find(r => r.RoleId == role.RoleTemplateRoleRoleId) != null - }); - } + List<RoleApplyPreview> rolesToApply = roleTemplate.RoleTemplateRoles + .Select(role => new RoleApplyPreview + { + RoleId = role.Role.RoleId, + RoleName = role.Role.FriendlyName, + Description = role.Role.Description, + UserHasRole = userRoles.Find(r => r.RoleId == role.RoleTemplateRoleRoleId) != null + }) + .ToList();As per coding guidelines: "Use
.Where()for filtering (notifinsideforeach) and.Select()for mapping (notforeach + Add) in LINQ expressions."🤖 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/RAPS/Controllers/RoleTemplatesController.cs` around lines 172 - 181, Replace the foreach that builds rolesToApply from roleTemplate.RoleTemplateRoles with a LINQ projection: call roleTemplate.RoleTemplateRoles.Select(...) to project each role into a new RoleApplyPreview and then .ToList() to assign to rolesToApply; inside the Select use role.Role.RoleId, role.Role.FriendlyName, role.Role.Description and compute UserHasRole by checking userRoles.Any(r => r.RoleId == role.RoleTemplateRoleRoleId) (or .Find(...) != null converted to Any) so the code is a single Select(...).ToList() expression producing rolesToApply.
🤖 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/RAPS/Controllers/RAPSController.cs`:
- Around line 53-60: The null-forgiving operator is used inconsistently on
`path` leading to compiler warnings; ensure all accesses to `path` are uniformly
null-suppressed or guarded: either perform a single null-check (e.g., if (path
== null) return/throw) before the block or add the null-forgiving operator to
each access (replace `path.Count` and `path[idx + N]` with `path!.Count` and
`path![idx + N]`) where `instance` and `page` are assigned so the compiler stops
warning about possible null references.
In `@web/Classes/Utilities/AcademicYearHelper.cs`:
- Around line 55-56: The current logic computes julyOfYear and does
julyOfYear.AddYears(-1) when date.Month < 7, which throws if date.Year == 1;
modify the method (AcademicYearHelper) to guard before calling AddYears: if
date.Year == 1 && date.Month < 7 then return julyOfYear (or the equivalent new
DateTime(1,7,1,0,0,0,DateTimeKind.Local)) instead of calling AddYears(-1);
otherwise keep the existing AddYears(-1) path — ensure you check the condition
where julyOfYear is created and avoid producing year zero.
---
Outside diff comments:
In `@web/Areas/RAPS/Controllers/RoleTemplatesController.cs`:
- Around line 172-181: Replace the foreach that builds rolesToApply from
roleTemplate.RoleTemplateRoles with a LINQ projection: call
roleTemplate.RoleTemplateRoles.Select(...) to project each role into a new
RoleApplyPreview and then .ToList() to assign to rolesToApply; inside the Select
use role.Role.RoleId, role.Role.FriendlyName, role.Role.Description and compute
UserHasRole by checking userRoles.Any(r => r.RoleId ==
role.RoleTemplateRoleRoleId) (or .Find(...) != null converted to Any) so the
code is a single Select(...).ToList() expression producing rolesToApply.
🪄 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: bccee1ce-376f-4d51-8f83-73b123f31518
📒 Files selected for processing (8)
test/RAPS/RoleTemplateSimplifiedTests.csweb/Areas/CTS/Models/AuditRow.csweb/Areas/Effort/Services/PercentRolloverService.csweb/Areas/Effort/Services/PercentageService.csweb/Areas/RAPS/Controllers/RAPSController.csweb/Areas/RAPS/Controllers/RoleTemplatesController.csweb/Areas/RAPS/Services/RAPSAuditService.csweb/Classes/Utilities/AcademicYearHelper.cs
rlorenzo
force-pushed
the
codeql/3-correctness
branch
from
5a244d0 to
6f4722e
Compare
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
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/RAPS/Controllers/RoleTemplatesController.cs`:
- Line 159: Remove the redundant comment that restates the code in
RoleTemplatesController (the "//if user is not found, return null" above the if
(user == null) return null; line); simply delete that comment so only the code
remains, keeping the method and the if-check unchanged.
🪄 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: ed6914c3-c9b5-436d-ac19-7bf3b4ae2e28
📒 Files selected for processing (11)
test/RAPS/RoleTemplateSimplifiedTests.cstest/Utilities/AcademicYearHelperTests.csweb/Areas/CTS/Models/AuditRow.csweb/Areas/Effort/Services/PercentRolloverService.csweb/Areas/Effort/Services/PercentageService.csweb/Areas/RAPS/Controllers/RAPSController.csweb/Areas/RAPS/Controllers/RoleTemplatesController.csweb/Areas/RAPS/Services/RAPSAuditService.csweb/Classes/Utilities/AcademicYearHelper.csweb/Controllers/HomeController.csweb/Models/Students/StudentClassYear.cs
ReSharper PR-scoped gate fixes on lines the CodeQL 3 change touched:
- RAPSController: 'is int idx' on int? -> 'is { } idx'
- RoleTemplatesController: drop '??' fallbacks on non-null properties
CodeRabbit review cleanups on the same change:
- RoleTemplatesController: replace both foreach + Add blocks with LINQ
projections; pre-build a HashSet so the apply-preview no longer runs an
O(n^2) userRoles.Find inside the loop
- RAPSController: hoist a single 'path != null' guard, dropping the
inconsistent 'path!' null-forgiving operators
- AcademicYearHelper.GetAcademicYearStart: guard year-1 dates before July
instead of underflowing to year 0 in AddYears(-1); add covering test
Two unit tests for the constructor that drives the GetRoleTemplates projection: - Constructor_MapsScalarsAndFlattensRoles: verifies the scalar properties and the nested RoleTemplateRoles -> Roles flattening via Role.RoleId / Role.FriendlyName. - Constructor_EmptyRoles_ReturnsEmptyCollection: ensures the Roles initializer does not depend on a non-empty source collection. Pins the constructor's mapping behavior so the cs/virtual-call-in- constructor fix (dropping `virtual` from Roles) does not silently regress the projection.
rlorenzo
force-pushed
the
codeql/3-correctness
branch
from
6f4722e to
f98d884
Compare
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.
Summary
Closes ~15 CodeQL alerts across three correctness rule families. Each fix needs per-site judgment, so they're consolidated here rather than split — the reviewer mental model is the same throughout.
cs/virtual-call-in-constructor
StudentClassYear(web/Models/Students/StudentClassYear.cs) —class→sealed class, dropsvirtualon the four navigation properties declared on the now-sealed class.Safety check: not inherited from in C#, not
Substitute.For<...>in tests, and there is noUseLazyLoadingProxies()call anywhere in the project so EF doesn't need it non-sealed.A
RoleTemplateSimplifiedalert in the same family is closed in commit 2dd08d8 by droppingvirtualfrom theRolesproperty — a smaller change than sealing, which keeps the public constructor surface intact. New unit tests in test/RAPS/RoleTemplateSimplifiedTests.cs lock in the constructor mapping (RoleTemplateId,TemplateName,Description, andRolesprojection) so future regressions surface.cs/unsafe-year-construction
ArgumentOutOfRangeException.ThrowIfLessThan(year, 1)/ThrowIfGreaterThan(year, 9998)at the top ofGetRolloverPreviewAsync(year comes from HTTP). Replacednew DateTime(year + 1, 6, 30, ...)withjune30Start.AddYears(1).GetAcademicYearStartnow constructs July ofdate.Yearfirst, then.AddYears(-1)if needed, instead ofnew DateTime(date.Year - 1, ...).cs/constant-condition
Removed redundant null-propagation that the analyzer can already prove dead:
type != null &&after theIsValidguard already eliminated the null case.successNode?.Element(...)→successNode!.Element(...)inside the guarded block (validatedUserNameis derived fromsuccessNode, so it can only be non-null whensuccessNodeis non-null).auditLog?.chains; theforeachvariable is non-null by construction.user?.after theif (user == null) return null;guard. Same controller also gets a small cleanup at L41-45:foreach+.Addrewritten as.Select().ToList().if (rapsIdx != null && rapsIdx > -1 && path?.Count > ...)blocks into a single outerrapsIdx is { } idx && idx > -1check that lifts thepathnull check up and uses pattern matching for the cast.dbAudit?.Encounter?./dbAudit.Encounter?.Student?.chains collapsed inside the surrounding non-null guard.Context
Third in the
CodeQL N:cleanup series (after #189, #190).Test plan
npm run test— backend + frontend passingnpm run verify:build— clean (0 errors)npm run lint— passing on changed files