Shard notification hub #54
Conversation
Co-authored-by: Justin Baur <[email protected]>
Deleting user was extracted to a command in bitwarden#4803, this updates that work to use just the device ids as I did elsewhere in abd67e8
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
29 file(s) reviewed, 45 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -46,15 +46,15 @@ await _pushRegistrationService.CreateOrUpdateRegistrationAsync(model.PushToken, | |||
| public async Task PostDelete([FromBody] PushDeviceRequestModel model) | |||
| { | |||
| CheckUsage(); | |||
| await _pushRegistrationService.DeleteRegistrationAsync(Prefix(model.Id), model.Type); | |||
| await _pushRegistrationService.DeleteRegistrationAsync(Prefix(model.Id)); | |||
There was a problem hiding this comment.
logic: Removal of device type parameter may affect how different device types are handled during deletion
| await _pushRegistrationService.AddUserRegistrationOrganizationAsync( | ||
| model.Devices.Select(d => new KeyValuePair<string, Core.Enums.DeviceType>(Prefix(d.Id), d.Type)), | ||
| model.Devices.Select(d => Prefix(d.Id)), | ||
| Prefix(model.OrganizationId)); |
There was a problem hiding this comment.
logic: Device type information is no longer passed to AddUserRegistrationOrganizationAsync, which could impact device-specific organization management
| await _pushRegistrationService.DeleteUserRegistrationOrganizationAsync( | ||
| model.Devices.Select(d => new KeyValuePair<string, Core.Enums.DeviceType>(Prefix(d.Id), d.Type)), | ||
| model.Devices.Select(d => Prefix(d.Id)), | ||
| Prefix(model.OrganizationId)); |
There was a problem hiding this comment.
logic: Similar to the add-organization endpoint, device type information is removed from DeleteUserRegistrationOrganizationAsync
| .Where(d => !string.IsNullOrWhiteSpace(d.PushToken)) | ||
| .Select(d => new KeyValuePair<string, DeviceType>(d.Id.ToString(), d.Type)); | ||
| .Select(d => d.Id.ToString()); | ||
| } |
There was a problem hiding this comment.
logic: The Select statement now only returns the device ID as a string, omitting the DeviceType. Ensure this doesn't break any code that previously relied on the device type information.
| @@ -37,4 +37,25 @@ public static bool IsInstallationDeviceId(string deviceId) | |||
| { | |||
| return deviceId != null && deviceId.Length == 73 && deviceId[36] == '_'; | |||
| } | |||
| public static bool TryParse(string deviceId, out InstallationDeviceEntity installationDeviceEntity) | |||
There was a problem hiding this comment.
style: consider adding XML documentation for this public method
| // Act | ||
| var result = connection.RegistrationEnabled(CoreHelpers.GenerateComb(Guid.NewGuid(), DateTime.UtcNow.AddHours(1))); |
There was a problem hiding this comment.
style: Test might be sensitive to execution time. Consider using fixed timestamps
| RegistrationStartDate = null, | ||
| RegistrationEndDate = null, |
There was a problem hiding this comment.
logic: Test might not accurately represent a scenario with no valid hubs. Consider setting dates to the past
|
|
||
| [Theory] | ||
| [MemberData(nameof(ClientMethods))] | ||
| public async void CallsAllClients(Func<NotificationHubClientProxy, Task> proxyMethod, Func<INotificationHubClient, Task> clientMethod) |
There was a problem hiding this comment.
style: Use Task instead of async void for better error handling
| [MemberData(nameof(ClientMethods))] | ||
| public async void CallsAllClients(Func<NotificationHubClientProxy, Task> proxyMethod, Func<INotificationHubClient, Task> clientMethod) | ||
| { | ||
| var clients = _clients.ToArray(); |
There was a problem hiding this comment.
logic: Ensure _clients is not empty before converting to array
| private readonly ILogger<MultiServicePushNotificationService> _logger; | ||
| private readonly ILogger<RelayPushNotificationService> _relayLogger; | ||
| private readonly ILogger<NotificationsApiPushNotificationService> _hubLogger; | ||
| private readonly IEnumerable<IPushNotificationService> _services; |
There was a problem hiding this comment.
style: Consider using a more specific type like IList instead of IEnumerable for better control over the collection
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-9452
📔 Objective
Reworks notification hub sharding to be based on the date of GUID comb creation and a binning algorithm.
This change is needed so that we can keep track of the installation location of each device, which is needed for updates related to changes to a device's
⏰ 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 changesGreptile Summary
This pull request implements a significant rework of the notification hub sharding mechanism, focusing on GUID comb creation dates and a binning algorithm for improved device management.
NotificationHubPoolandNotificationHubConnectionclasses for managing multiple notification hub clientsDeviceTypeparameter from various methods, simplifying push notification and registration interfacesINotificationHubProxyandINotificationHubPoolfor better abstraction of notification hub operationsCoreHelpers.DateFromCombandCoreHelpers.BinForCombmethods to support the new sharding mechanismGlobalSettingsto includeNotificationHubPoolSettingswith registration start and end dates for each hub