[PM-22380] Enable NRT for some Core project files #5912
Conversation
| [MemberNotNull(nameof(_hubClient))] | ||
| private NotificationHubConnection Init() | ||
| { | ||
| HubClient = NotificationHubClient.CreateClientFromConnectionString(ConnectionString, HubName, EnableSendTracing); | ||
| _hubClient = NotificationHubClient.CreateClientFromConnectionString(ConnectionString, HubName, EnableSendTracing); |
There was a problem hiding this comment.
This avoids needing to use a null-forgiving operator in the HubClient getter. The null analysis doesn't seem smart enough to work out that assigning to HubClient would just directly assign to _hubClient, so it has to be assigned to directly here. This makes the HubClient setter unused now, so that could be removed.
0c7492c to
0a47ef1
Compare
| @@ -16,7 +18,7 @@ public JobFactory(IServiceProvider container) | |||
| public IJob NewJob(TriggerFiredBundle bundle, IScheduler scheduler) | |||
| { | |||
| var scope = _container.CreateScope(); | |||
| return scope.ServiceProvider.GetService(bundle.JobDetail.JobType) as IJob; | |||
| return (scope.ServiceProvider.GetService(bundle.JobDetail.JobType) as IJob)!; | |||
There was a problem hiding this comment.
As I understand it this should only be null if a job type hasn't been registered in DI before being executed by Quartz.NET, which shouldn't happen and that'll just lead to Quartz throwing a bit after calling this anyway.
I haven't used Quartz before, but from looking into it a bit I think as of ~4 years ago this isn't necessary, as Quartz can handle all of this itself. It looks like there could be a pretty big cleanup of most of the use of Quartz, which would get rid of this, but that's a whole other thing.
There was a problem hiding this comment.
This could definitely use some cleanup it just never gets prioritized since it keeps working 🤷.
| @@ -142,7 +145,10 @@ public virtual async Task StartAsync(CancellationToken cancellationToken) | |||
|
|
|||
| public virtual async Task StopAsync(CancellationToken cancellationToken) | |||
| { | |||
| await _scheduler?.Shutdown(cancellationToken); | |||
There was a problem hiding this comment.
If _scheduler is null, this just throws a nullref exception as you can't await null.
There was a problem hiding this comment.
The changes here are mostly around Jobs potentially being null. It's a bit odd that there's some logic in StartAsync that only executes if Jobs is not null, but then right after that Jobs would be used assuming that it wasn't null. I just shuffled stuff around to make that not an issue.
|
Thank you for your contribution! We've added this to our internal Community PR board for review. Details on our contribution process can be found here: https://contributing.bitwarden.com/contributing/pull-requests/community-pr-process. |
|
@benbryant0 I synced up the branch to main, to run the CI pipeline, please watch out to pull the changes if making further changes. |
|
Great job, no security vulnerabilities found in this Pull Request |
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5912 +/- ##
==========================================
- Coverage 51.06% 47.73% -3.33%
==========================================
Files 1665 1665
Lines 75008 75013 +5
Branches 6755 6757 +2
==========================================
- Hits 38300 35811 -2489
- Misses 35188 37742 +2554
+ Partials 1520 1460 -60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -175,7 +175,7 @@ public async Task RotateUserKeyWrongData_Throws(SutProvider<AccountsKeyManagemen | |||
| } | |||
| catch (BadRequestException ex) | |||
| { | |||
| Assert.NotEmpty(ex.ModelState.Values); | |||
| Assert.NotEmpty(ex.ModelState!.Values); | |||
There was a problem hiding this comment.
In my opinion I would use an assertion that this thing is non null instead of the forgiving operator in tests.
| Assert.NotEmpty(ex.ModelState!.Values); | |
| Assert.NotNull(ex.ModelState); | |
| Assert.NotEmpty(ex.ModelState.Values); |
| @@ -210,7 +210,7 @@ public async Task PostSetKeyConnectorKeyAsync_SetKeyConnectorKeyFails_ThrowsBadR | |||
| var badRequestException = | |||
| await Assert.ThrowsAsync<BadRequestException>(() => sutProvider.Sut.PostSetKeyConnectorKeyAsync(data)); | |||
|
|
|||
| Assert.Equal(1, badRequestException.ModelState.ErrorCount); | |||
| Assert.Equal(1, badRequestException.ModelState!.ErrorCount); | |||
| @@ -284,7 +284,7 @@ public async Task PostConvertToKeyConnectorAsync_ConvertToKeyConnectorFails_Thro | |||
| var badRequestException = | |||
| await Assert.ThrowsAsync<BadRequestException>(() => sutProvider.Sut.PostConvertToKeyConnectorAsync()); | |||
|
|
|||
| Assert.Equal(1, badRequestException.ModelState.ErrorCount); | |||
| Assert.Equal(1, badRequestException.ModelState!.ErrorCount); | |||
| @@ -16,7 +18,7 @@ public JobFactory(IServiceProvider container) | |||
| public IJob NewJob(TriggerFiredBundle bundle, IScheduler scheduler) | |||
| { | |||
| var scope = _container.CreateScope(); | |||
| return scope.ServiceProvider.GetService(bundle.JobDetail.JobType) as IJob; | |||
| return (scope.ServiceProvider.GetService(bundle.JobDetail.JobType) as IJob)!; | |||
There was a problem hiding this comment.
This could definitely use some cleanup it just never gets prioritized since it keeps working 🤷.
|
Thanks! @quexten I'll leave the nits for now since you're fine with them and they're in test files with no change to existing behaviour. I do agree with them though, so I'll make a note to fix them when I next have a PR that touches KM ownership. I just rebased on main, so should be good to go. |
df26e36 to
818550b
Compare
|
|
Merged! Thank you for the contribution. |
🎟️ Tracking
Related to this ADR, but until that's accepted I'm working in the other direction and enabling per-file instead (as discussed here).
📔 Objective
This PR enables NRT (nullable reference types) for 25 files in the Core project and does some minimal work to fix the resulting errors.
I've intentionally targeted files that don't have any ownership according to the CODEOWNERS file, but a file in a test project that had to be changed does belong to the Key Management team.
Since the scope of these changes is pretty limited, it's not guaranteed that every field/variable/whatever that can be null has been marked as such. I've only focused on fixing errors, and been guided by callers of functions & constructors when those were also in nullable contexts and started erroring.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes