-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Shard notification hub #54
base: main
Are you sure you want to change the base?
Changes from all commits
787996b
aa83720
abd67e8
7098515
5bc4945
c7fd6bf
15c0490
a84b486
b3785d3
2ce81b1
c53037e
6fbcb1a
78c6bd6
7c7f92b
14ff31c
1445d01
640e9a0
aa125d6
3a6b128
e9a10bc
fb86bf5
8f36b4a
76c79b4
3ae86e0
d6c67f4
06b06bd
2fc81ad
c17fb1a
5e58174
d81e412
ac0371d
dd9339c
ed5183b
c1d0182
1885e6b
f3581f4
09dbb08
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)); | ||
} | ||
|
||
[HttpPut("add-organization")] | ||
public async Task PutAddOrganization([FromBody] PushUpdateRequestModel model) | ||
{ | ||
CheckUsage(); | ||
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)); | ||
Comment on lines
56
to
58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Device type information is no longer passed to AddUserRegistrationOrganizationAsync, which could impact device-specific organization management |
||
} | ||
|
||
|
@@ -63,7 +63,7 @@ public async Task PutDeleteOrganization([FromBody] PushUpdateRequestModel model) | |
{ | ||
CheckUsage(); | ||
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)); | ||
Comment on lines
65
to
67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Similar to the add-organization endpoint, device type information is removed from DeleteUserRegistrationOrganizationAsync |
||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1832,12 +1832,12 @@ await _pushRegistrationService.DeleteUserRegistrationOrganizationAsync(devices, | |
} | ||
|
||
|
||
private async Task<IEnumerable<KeyValuePair<string, DeviceType>>> GetUserDeviceIdsAsync(Guid userId) | ||
private async Task<IEnumerable<string>> GetUserDeviceIdsAsync(Guid userId) | ||
{ | ||
var devices = await _deviceRepository.GetManyByUserIdAsync(userId); | ||
return devices | ||
.Where(d => !string.IsNullOrWhiteSpace(d.PushToken)) | ||
.Select(d => new KeyValuePair<string, DeviceType>(d.Id.ToString(), d.Type)); | ||
.Select(d => d.Id.ToString()); | ||
} | ||
Comment on lines
1839
to
1841
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: The |
||
|
||
public async Task ReplaceAndUpdateCacheAsync(Organization org, EventType? orgEvent = null) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,9 @@ | ||
๏ปฟusing System.ComponentModel.DataAnnotations; | ||
using Bit.Core.Enums; | ||
|
||
namespace Bit.Core.Models.Api; | ||
|
||
public class PushDeviceRequestModel | ||
{ | ||
[Required] | ||
public string Id { get; set; } | ||
[Required] | ||
public DeviceType Type { get; set; } | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: consider adding XML documentation for this public method |
||
{ | ||
installationDeviceEntity = null; | ||
var installationId = Guid.Empty; | ||
var deviceIdGuid = Guid.Empty; | ||
if (!IsInstallationDeviceId(deviceId)) | ||
{ | ||
return false; | ||
} | ||
Comment on lines
+45
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: this check is redundant with the length check on line 50 |
||
var parts = deviceId.Split("_"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: use string.Split('_', 2) to limit the number of splits |
||
if (parts.Length < 2) | ||
{ | ||
return false; | ||
} | ||
if (!Guid.TryParse(parts[0], out installationId) || !Guid.TryParse(parts[1], out deviceIdGuid)) | ||
{ | ||
return false; | ||
} | ||
installationDeviceEntity = new InstallationDeviceEntity(installationId, deviceIdGuid); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: use named arguments for clarity: new InstallationDeviceEntity(installationId: installationId, deviceId: deviceIdGuid) |
||
return true; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
๏ปฟusing Microsoft.Azure.NotificationHubs; | ||
|
||
namespace Bit.Core.NotificationHub; | ||
|
||
public interface INotificationHubProxy | ||
{ | ||
Task<(INotificationHubClient Client, NotificationOutcome Outcome)[]> SendTemplateNotificationAsync(IDictionary<string, string> properties, string tagExpression); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: return type is complex, may need explanation |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
๏ปฟusing Microsoft.Azure.NotificationHubs; | ||
|
||
namespace Bit.Core.NotificationHub; | ||
|
||
public interface INotificationHubPool | ||
{ | ||
NotificationHubClient ClientFor(Guid comb); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider adding XML documentation for this method to explain the purpose of the 'comb' parameter and what it returns |
||
INotificationHubProxy AllClients { get; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider adding XML documentation for this property to explain its purpose and usage |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
๏ปฟusing Microsoft.Azure.NotificationHubs; | ||
|
||
namespace Bit.Core.NotificationHub; | ||
|
||
public class NotificationHubClientProxy : INotificationHubProxy | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider adding XML documentation for the class and its public members |
||
{ | ||
private readonly IEnumerable<INotificationHubClient> _clients; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider using IReadOnlyList instead of IEnumerable for better performance |
||
|
||
public NotificationHubClientProxy(IEnumerable<INotificationHubClient> clients) | ||
{ | ||
_clients = clients; | ||
} | ||
|
||
private async Task<(INotificationHubClient, T)[]> ApplyToAllClientsAsync<T>(Func<INotificationHubClient, Task<T>> action) | ||
{ | ||
var tasks = _clients.Select(async c => (c, await action(c))); | ||
return await Task.WhenAll(tasks); | ||
} | ||
Comment on lines
+14
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: This method could potentially throw exceptions if any of the client operations fail. Consider adding try-catch blocks or implementing a more robust error handling strategy |
||
|
||
// partial proxy of INotificationHubClient implementation | ||
// Note: Any other methods that are needed can simply be delegated as done here. | ||
public async Task<(INotificationHubClient Client, NotificationOutcome Outcome)[]> SendTemplateNotificationAsync(IDictionary<string, string> properties, string tagExpression) | ||
{ | ||
return await ApplyToAllClientsAsync(async c => await c.SendTemplateNotificationAsync(properties, tagExpression)); | ||
} | ||
Comment on lines
+22
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: This method returns an array of tuples. Consider creating a custom return type for better readability and maintainability |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,128 @@ | ||
๏ปฟusing Bit.Core.Settings; | ||
using Bit.Core.Utilities; | ||
using Microsoft.Azure.NotificationHubs; | ||
|
||
class NotificationHubConnection | ||
{ | ||
public string HubName { get; init; } | ||
public string ConnectionString { get; init; } | ||
public bool EnableSendTracing { get; init; } | ||
private NotificationHubClient _hubClient; | ||
/// <summary> | ||
/// Gets the NotificationHubClient for this connection. | ||
/// | ||
/// If the client is null, it will be initialized. | ||
/// | ||
/// <throws>Exception</throws> if the connection is invalid. | ||
/// </summary> | ||
public NotificationHubClient HubClient | ||
{ | ||
get | ||
{ | ||
if (_hubClient == null) | ||
{ | ||
if (!IsValid) | ||
{ | ||
throw new Exception("Invalid notification hub settings"); | ||
} | ||
Init(); | ||
} | ||
return _hubClient; | ||
} | ||
private set | ||
{ | ||
_hubClient = value; | ||
} | ||
} | ||
/// <summary> | ||
/// Gets the start date for registration. | ||
/// | ||
/// If null, registration is always disabled. | ||
/// </summary> | ||
public DateTime? RegistrationStartDate { get; init; } | ||
/// <summary> | ||
/// Gets the end date for registration. | ||
/// | ||
/// If null, registration has no end date. | ||
/// </summary> | ||
public DateTime? RegistrationEndDate { get; init; } | ||
/// <summary> | ||
/// Gets whether all data needed to generate a connection to Notification Hub is present. | ||
/// </summary> | ||
public bool IsValid | ||
{ | ||
get | ||
{ | ||
{ | ||
var invalid = string.IsNullOrWhiteSpace(HubName) || string.IsNullOrWhiteSpace(ConnectionString); | ||
return !invalid; | ||
} | ||
} | ||
Comment on lines
+52
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Simplify IsValid property |
||
} | ||
|
||
public string LogString | ||
{ | ||
get | ||
{ | ||
return $"HubName: {HubName}, EnableSendTracing: {EnableSendTracing}, RegistrationStartDate: {RegistrationStartDate}, RegistrationEndDate: {RegistrationEndDate}"; | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Gets whether registration is enabled for the given comb ID. | ||
/// This is based off of the generation time encoded in the comb ID. | ||
/// </summary> | ||
/// <param name="comb"></param> | ||
/// <returns></returns> | ||
public bool RegistrationEnabled(Guid comb) | ||
{ | ||
var combTime = CoreHelpers.DateFromComb(comb); | ||
return RegistrationEnabled(combTime); | ||
} | ||
|
||
/// <summary> | ||
/// Gets whether registration is enabled for the given time. | ||
/// </summary> | ||
/// <param name="queryTime">The time to check</param> | ||
/// <returns></returns> | ||
public bool RegistrationEnabled(DateTime queryTime) | ||
{ | ||
if (queryTime >= RegistrationEndDate || RegistrationStartDate == null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Check for null RegistrationEndDate |
||
{ | ||
return false; | ||
} | ||
|
||
return RegistrationStartDate < queryTime; | ||
} | ||
|
||
private NotificationHubConnection() { } | ||
|
||
/// <summary> | ||
/// Creates a new NotificationHubConnection from the given settings. | ||
/// </summary> | ||
/// <param name="settings"></param> | ||
/// <returns></returns> | ||
public static NotificationHubConnection From(GlobalSettings.NotificationHubSettings settings) | ||
{ | ||
return new() | ||
{ | ||
HubName = settings.HubName, | ||
ConnectionString = settings.ConnectionString, | ||
EnableSendTracing = settings.EnableSendTracing, | ||
// Comb time is not precise enough for millisecond accuracy | ||
RegistrationStartDate = settings.RegistrationStartDate.HasValue ? Truncate(settings.RegistrationStartDate.Value, TimeSpan.FromMilliseconds(10)) : null, | ||
RegistrationEndDate = settings.RegistrationEndDate | ||
}; | ||
} | ||
|
||
private NotificationHubConnection Init() | ||
{ | ||
HubClient = NotificationHubClient.CreateClientFromConnectionString(ConnectionString, HubName, EnableSendTracing); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Ensure exception handling for CreateClientFromConnectionString |
||
return this; | ||
} | ||
|
||
private static DateTime Truncate(DateTime dateTime, TimeSpan resolution) | ||
{ | ||
return dateTime.AddTicks(-(dateTime.Ticks % resolution.Ticks)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
๏ปฟusing Bit.Core.Settings; | ||
using Bit.Core.Utilities; | ||
using Microsoft.Azure.NotificationHubs; | ||
using Microsoft.Extensions.Logging; | ||
|
||
namespace Bit.Core.NotificationHub; | ||
|
||
public class NotificationHubPool : INotificationHubPool | ||
{ | ||
private List<NotificationHubConnection> _connections { get; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: consider making _connections readonly |
||
private readonly IEnumerable<INotificationHubClient> _clients; | ||
private readonly ILogger<NotificationHubPool> _logger; | ||
public NotificationHubPool(ILogger<NotificationHubPool> logger, GlobalSettings globalSettings) | ||
{ | ||
_logger = logger; | ||
_connections = FilterInvalidHubs(globalSettings.NotificationHubPool.NotificationHubs); | ||
_clients = _connections.GroupBy(c => c.ConnectionString).Select(g => g.First().HubClient); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: potential performance issue if many connections share the same ConnectionString |
||
} | ||
|
||
private List<NotificationHubConnection> FilterInvalidHubs(IEnumerable<GlobalSettings.NotificationHubSettings> hubs) | ||
{ | ||
List<NotificationHubConnection> result = new(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: use List result = new(hubs.Count()) for better performance |
||
_logger.LogDebug("Filtering {HubCount} notification hubs", hubs.Count()); | ||
foreach (var hub in hubs) | ||
{ | ||
var connection = NotificationHubConnection.From(hub); | ||
if (!connection.IsValid) | ||
{ | ||
_logger.LogWarning("Invalid notification hub settings: {HubName}", hub.HubName ?? "hub name missing"); | ||
continue; | ||
} | ||
_logger.LogDebug("Adding notification hub: {ConnectionLogString}", connection.LogString); | ||
result.Add(connection); | ||
} | ||
|
||
return result; | ||
} | ||
|
||
|
||
/// <summary> | ||
/// Gets the NotificationHubClient for the given comb ID. | ||
/// </summary> | ||
/// <param name="comb"></param> | ||
/// <returns></returns> | ||
/// <exception cref="InvalidOperationException">Thrown when no notification hub is found for a given comb.</exception> | ||
public NotificationHubClient ClientFor(Guid comb) | ||
{ | ||
var possibleConnections = _connections.Where(c => c.RegistrationEnabled(comb)).ToArray(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: consider caching the result of this operation if it's called frequently |
||
if (possibleConnections.Length == 0) | ||
{ | ||
throw new InvalidOperationException($"No valid notification hubs are available for the given comb ({comb}).\n" + | ||
$"The comb's datetime is {CoreHelpers.DateFromComb(comb)}." + | ||
$"Hub start and end times are configured as follows:\n" + | ||
string.Join("\n", _connections.Select(c => $"Hub {c.HubName} - Start: {c.RegistrationStartDate}, End: {c.RegistrationEndDate}"))); | ||
} | ||
var resolvedConnection = possibleConnections[CoreHelpers.BinForComb(comb, possibleConnections.Length)]; | ||
_logger.LogTrace("Resolved notification hub for comb {Comb} out of {HubCount} hubs.\n{ConnectionInfo}", comb, possibleConnections.Length, resolvedConnection.LogString); | ||
return resolvedConnection.HubClient; | ||
} | ||
|
||
public INotificationHubProxy AllClients { get { return new NotificationHubClientProxy(_clients); } } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: consider lazy initialization of AllClients to improve performance |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Removal of device type parameter may affect how different device types are handled during deletion