Skip to content
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

Open
wants to merge 37 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
787996b
Allow for binning of comb IDs by date and value
MGibson1 Jul 2, 2024
aa83720
Introduce notification hub pool
MGibson1 Jul 2, 2024
abd67e8
Replace device type sharding with comb + range sharding
MGibson1 Jul 2, 2024
7098515
Fix proxy interface
MGibson1 Jul 2, 2024
5bc4945
Use enumerable services for multiServiceNotificationHub
MGibson1 Jul 2, 2024
c7fd6bf
Fix push interface usage
MGibson1 Jul 2, 2024
15c0490
Fix push notification service dependencies
MGibson1 Jul 2, 2024
a84b486
Fix push notification keys
MGibson1 Jul 2, 2024
b3785d3
Fixup documentation
MGibson1 Jul 2, 2024
2ce81b1
Remove deprecated settings
MGibson1 Jul 2, 2024
c53037e
Fix tests
MGibson1 Jul 2, 2024
6fbcb1a
PascalCase method names
MGibson1 Jul 3, 2024
78c6bd6
Remove unused request model properties
MGibson1 Jul 3, 2024
7c7f92b
Remove unused setting
MGibson1 Jul 3, 2024
14ff31c
Improve DateFromComb precision
MGibson1 Jul 8, 2024
1445d01
Prefer readonly service enumerable
MGibson1 Jul 9, 2024
640e9a0
Pascal case template holes
MGibson1 Jul 9, 2024
aa125d6
Name TryParse methods TryParse
MGibson1 Jul 9, 2024
3a6b128
Apply suggestions from code review
MGibson1 Jul 9, 2024
e9a10bc
AllClients is a set of clients and must be deduplicated
MGibson1 Jul 19, 2024
fb86bf5
Merge branch 'main' into shard-notification-hub
MGibson1 Aug 8, 2024
8f36b4a
Fix registration start time
MGibson1 Aug 8, 2024
76c79b4
Merge branch 'main' into shard-notification-hub
bnagawiecki Sep 18, 2024
3ae86e0
Add logging to initialization of a notification hub
MGibson1 Sep 18, 2024
d6c67f4
more logging
MGibson1 Sep 18, 2024
06b06bd
Merge branch 'main' into shard-notification-hub
MGibson1 Sep 19, 2024
2fc81ad
Add lower level logging for hub settings
MGibson1 Sep 19, 2024
c17fb1a
Merge branch 'main' into shard-notification-hub
MGibson1 Sep 19, 2024
5e58174
Log when connection is resolved
MGibson1 Sep 19, 2024
d81e412
Improve log message
MGibson1 Sep 19, 2024
ac0371d
Log pushes to notification hub
MGibson1 Sep 19, 2024
dd9339c
temporarily elevate log messages for visibility
MGibson1 Sep 19, 2024
ed5183b
Log in multi-service when relaying to another push service
MGibson1 Sep 19, 2024
c1d0182
Merge branch 'main' into shard-notification-hub
bnagawiecki Oct 10, 2024
1885e6b
Revert to more reasonable logging free of user information
MGibson1 Oct 16, 2024
f3581f4
Merge branch 'main' into shard-notification-hub
MGibson1 Oct 16, 2024
09dbb08
Fixup merge
MGibson1 Oct 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/Api/Controllers/PushController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link

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

}

[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
Copy link

Choose a reason for hiding this comment

The 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

}

Expand All @@ -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
Copy link

Choose a reason for hiding this comment

The 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

}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,12 +162,12 @@ private async Task RepositoryDeleteUserAsync(OrganizationUser orgUser, Guid? del
}
}

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());
}

private async Task DeleteAndPushUserRegistrationAsync(Guid organizationId, Guid userId)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


public async Task ReplaceAndUpdateCacheAsync(Organization org, EventType? orgEvent = null)
Expand Down
3 changes: 0 additions & 3 deletions src/Core/Models/Api/Request/PushDeviceRequestModel.cs
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; }
}
5 changes: 2 additions & 3 deletions src/Core/Models/Api/Request/PushUpdateRequestModel.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
๏ปฟusing System.ComponentModel.DataAnnotations;
using Bit.Core.Enums;

namespace Bit.Core.Models.Api;

Expand All @@ -8,9 +7,9 @@ public class PushUpdateRequestModel
public PushUpdateRequestModel()
{ }

public PushUpdateRequestModel(IEnumerable<KeyValuePair<string, DeviceType>> devices, string organizationId)
public PushUpdateRequestModel(IEnumerable<string> deviceIds, string organizationId)
{
Devices = devices.Select(d => new PushDeviceRequestModel { Id = d.Key, Type = d.Value });
Devices = deviceIds.Select(d => new PushDeviceRequestModel { Id = d });
OrganizationId = organizationId;
}

Expand Down
21 changes: 21 additions & 0 deletions src/Core/Models/Data/InstallationDeviceEntity.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

The 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("_");
Copy link

Choose a reason for hiding this comment

The 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);
Copy link

Choose a reason for hiding this comment

The 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;
}
}
8 changes: 8 additions & 0 deletions src/Core/NotificationHub/INotificationHubClientProxy.cs
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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: return type is complex, may need explanation

}
9 changes: 9 additions & 0 deletions src/Core/NotificationHub/INotificationHubPool.cs
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);
Copy link

Choose a reason for hiding this comment

The 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; }
Copy link

Choose a reason for hiding this comment

The 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

}
26 changes: 26 additions & 0 deletions src/Core/NotificationHub/NotificationHubClientProxy.cs
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
Copy link

Choose a reason for hiding this comment

The 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;
Copy link

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

The 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

}
128 changes: 128 additions & 0 deletions src/Core/NotificationHub/NotificationHubConnection.cs
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
Copy link

Choose a reason for hiding this comment

The 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)
Copy link

Choose a reason for hiding this comment

The 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);
Copy link

Choose a reason for hiding this comment

The 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));
}
}
62 changes: 62 additions & 0 deletions src/Core/NotificationHub/NotificationHubPool.cs
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; }
Copy link

Choose a reason for hiding this comment

The 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);
Copy link

Choose a reason for hiding this comment

The 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();
Copy link

Choose a reason for hiding this comment

The 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();
Copy link

Choose a reason for hiding this comment

The 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); } }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: consider lazy initialization of AllClients to improve performance

}
Loading
Loading