CodeQL 3: fix: close CodeQL correctness alerts#191
Open
rlorenzo wants to merge 3 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughPR hardens null-safety by consolidating nullable-conditional patterns into explicit non-null assertions where control flow guarantees non-nullability. Also seals model classes, removes EF virtual modifiers, adopts factory-method pattern for RoleTemplateSimplified, changes CompetencyController duplicate-detection behavior to include the current competency, and improves validation logic in services and utilities. ChangesNull-safety assertions and code quality improvements
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 51439 +2
Branches 4812 4808 -4
==========================================
+ Hits 22131 22160 +29
+ Misses 28780 28752 -28
- 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).
These were flagged by the ReSharper PR-scoped gate as new warnings on
lines my CodeQL 3 fix touched:
- CMSFile.cs: drop redundant 'partial' modifier (PartialTypeWithSinglePart)
- RAPSController.cs: 'is int idx' on int? -> 'is { } idx'
(ConvertTypeCheckPatternToNullCheck)
- RoleTemplatesController.cs: drop '??' fallbacks; both properties are
declared non-null (NullCoalescingConditionIsAlwaysNotNullAccordingToAPIContract)
Also folds in the controller cleanup from CodeRabbit on the same lines:
replace foreach + Add with .Select(...).ToList() to match the project's
LINQ conventions.
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
df971c6 to
5a244d0
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
3 participants
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