diff --git a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_enabled.cs b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_enabled.cs index 20d1e6b30d..e797fe8edd 100644 --- a/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_enabled.cs +++ b/src/ServiceControl.AcceptanceTests/Monitoring/CustomChecks/When_email_notifications_are_enabled.cs @@ -64,16 +64,12 @@ class SetupNotificationSettings(INotificationsDataStore notificationsDataStore) { public async Task StartAsync(CancellationToken cancellationToken) { - using var notificationsManager = await notificationsDataStore.CreateNotificationsManager(); + await using var notificationsManager = await notificationsDataStore.CreateNotificationsManager(); var settings = await notificationsManager.LoadSettings(); - settings.Email = new EmailNotifications - { - Enabled = true, - From = "YouServiceControl@particular.net", - To = "WhoeverMightBeConcerned@particular.net", - }; - + settings.Email.Enabled = true; + settings.Email.From = "YouServiceControl@particular.net"; + settings.Email.To = "WhoeverMightBeConcerned@particular.net"; await notificationsManager.SaveChanges(); } diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/EditFailedMessagesManager.cs b/src/ServiceControl.Persistence.EFCore/Implementation/EditFailedMessagesManager.cs index 5a80ec2287..cb795e9b89 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/EditFailedMessagesManager.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/EditFailedMessagesManager.cs @@ -20,9 +20,5 @@ public Task SetFailedMessageAsResolved() => public Task SaveChanges() => throw new NotImplementedException(); - public void Dispose() - { - // Nothing to dispose yet - GC.SuppressFinalize(this); - } + public ValueTask DisposeAsync() => default; } diff --git a/src/ServiceControl.Persistence.EFCore/Implementation/NotificationsManager.cs b/src/ServiceControl.Persistence.EFCore/Implementation/NotificationsManager.cs index 74dabd8071..b601155a89 100644 --- a/src/ServiceControl.Persistence.EFCore/Implementation/NotificationsManager.cs +++ b/src/ServiceControl.Persistence.EFCore/Implementation/NotificationsManager.cs @@ -4,15 +4,11 @@ namespace ServiceControl.Persistence.EFCore.Implementation; public class NotificationsManager : INotificationsManager { - public Task LoadSettings(TimeSpan? cacheTimeout = null) => + public Task LoadSettings() => throw new NotImplementedException(); public Task SaveChanges() => throw new NotImplementedException(); - public void Dispose() - { - // Nothing to dispose yet - GC.SuppressFinalize(this); - } + public ValueTask DisposeAsync() => default; } diff --git a/src/ServiceControl.Persistence.RavenDB/Editing/NotificationSettingsDocument.cs b/src/ServiceControl.Persistence.RavenDB/Editing/NotificationSettingsDocument.cs new file mode 100644 index 0000000000..7ef8ee17b5 --- /dev/null +++ b/src/ServiceControl.Persistence.RavenDB/Editing/NotificationSettingsDocument.cs @@ -0,0 +1,9 @@ +namespace ServiceControl.Persistence.RavenDB.Editing; + +using Notifications; + +class NotificationsSettingsDocument +{ + public string Id { get; set; } + public EmailNotifications Email { get; set; } = new(); +} \ No newline at end of file diff --git a/src/ServiceControl.Persistence.RavenDB/Editing/NotificationsManager.cs b/src/ServiceControl.Persistence.RavenDB/Editing/NotificationsManager.cs index 52e41d6a44..57e8d2b454 100644 --- a/src/ServiceControl.Persistence.RavenDB/Editing/NotificationsManager.cs +++ b/src/ServiceControl.Persistence.RavenDB/Editing/NotificationsManager.cs @@ -8,27 +8,24 @@ class NotificationsManager(IAsyncDocumentSession session) : AbstractSessionManager(session), INotificationsManager { const string SingleDocumentId = "NotificationsSettings/All"; - static readonly TimeSpan CacheTimeoutDefault = TimeSpan.FromMinutes(5); // Raven requires this to be at least 1 second + static readonly TimeSpan CacheTimeout = TimeSpan.FromMinutes(5); // Raven requires this to be at least 1 second - public async Task LoadSettings(TimeSpan? cacheTimeout = null) + public async Task LoadSettings() { - using var aggressivelyCacheFor = await Session.Advanced.DocumentStore.AggressivelyCacheForAsync(cacheTimeout ?? CacheTimeoutDefault); + using var aggressivelyCacheFor = await Session.Advanced.DocumentStore.AggressivelyCacheForAsync(CacheTimeout); var settings = await Session - .LoadAsync(SingleDocumentId); + .LoadAsync(SingleDocumentId); - if (settings != null) + if (settings == null) { - return settings; + settings = new NotificationsSettingsDocument { Id = SingleDocumentId }; + await Session.StoreAsync(settings); } - settings = new NotificationsSettings + return new NotificationsSettings() { - Id = SingleDocumentId + Email = settings.Email }; - - await Session.StoreAsync(settings); - - return settings; } } } \ No newline at end of file diff --git a/src/ServiceControl.Persistence.RavenDB/Transactions/RavenTransactionalDataStore.cs b/src/ServiceControl.Persistence.RavenDB/Transactions/RavenTransactionalDataStore.cs index 6f04ae065c..86138440e8 100644 --- a/src/ServiceControl.Persistence.RavenDB/Transactions/RavenTransactionalDataStore.cs +++ b/src/ServiceControl.Persistence.RavenDB/Transactions/RavenTransactionalDataStore.cs @@ -8,6 +8,10 @@ abstract class AbstractSessionManager(IAsyncDocumentSession session) : IDataSess protected IAsyncDocumentSession Session { get; } = session; public Task SaveChanges() => Session.SaveChangesAsync(); - public void Dispose() => Session.Dispose(); + public ValueTask DisposeAsync() + { + Session.Dispose(); + return ValueTask.CompletedTask; + } } } \ No newline at end of file diff --git a/src/ServiceControl.Persistence.Tests.PostgreSql/ServiceControl.Persistence.Tests.PostgreSql.csproj b/src/ServiceControl.Persistence.Tests.PostgreSql/ServiceControl.Persistence.Tests.PostgreSql.csproj index b7f3f20807..492db2ec1c 100644 --- a/src/ServiceControl.Persistence.Tests.PostgreSql/ServiceControl.Persistence.Tests.PostgreSql.csproj +++ b/src/ServiceControl.Persistence.Tests.PostgreSql/ServiceControl.Persistence.Tests.PostgreSql.csproj @@ -39,6 +39,7 @@ + diff --git a/src/ServiceControl.Persistence.Tests.SqlServer/ServiceControl.Persistence.Tests.SqlServer.csproj b/src/ServiceControl.Persistence.Tests.SqlServer/ServiceControl.Persistence.Tests.SqlServer.csproj index 05b839198a..2e762eb089 100644 --- a/src/ServiceControl.Persistence.Tests.SqlServer/ServiceControl.Persistence.Tests.SqlServer.csproj +++ b/src/ServiceControl.Persistence.Tests.SqlServer/ServiceControl.Persistence.Tests.SqlServer.csproj @@ -39,6 +39,7 @@ + diff --git a/src/ServiceControl.Persistence.Tests/NotificationsDataStoreTests.cs b/src/ServiceControl.Persistence.Tests/NotificationsDataStoreTests.cs new file mode 100644 index 0000000000..0c31cdc66e --- /dev/null +++ b/src/ServiceControl.Persistence.Tests/NotificationsDataStoreTests.cs @@ -0,0 +1,167 @@ +namespace ServiceControl.Persistence.Tests; + +using System.Threading.Tasks; +using NUnit.Framework; + +class NotificationsDataStoreTests : PersistenceTestBase +{ + [Test, CancelAfter(30_000)] + public async Task LoadSettings_returns_defaults_when_no_settings_exist() + { + await using var manager = await NotificationsStore.CreateNotificationsManager(); + + var settings = await manager.LoadSettings(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(settings, Is.Not.Null); + Assert.That(settings.Email, Is.Not.Null); + Assert.That(settings.Email.Enabled, Is.False); + Assert.That(settings.Email.SmtpServer, Is.Null); + Assert.That(settings.Email.SmtpPort, Is.Null); + Assert.That(settings.Email.EnableTLS, Is.False); + Assert.That(settings.Email.From, Is.Null); + Assert.That(settings.Email.To, Is.Null); + Assert.That(settings.Email.AuthenticationAccount, Is.Null); + Assert.That(settings.Email.AuthenticationPassword, Is.Null); + } + } + + [Test, CancelAfter(30_000)] + public async Task SaveChanges_persists_email_settings_round_trip() + { + await using (var manager = await NotificationsStore.CreateNotificationsManager()) + { + var settings = await manager.LoadSettings(); + + settings.Email.Enabled = true; + settings.Email.SmtpServer = "smtp.example.com"; + settings.Email.SmtpPort = 587; + settings.Email.EnableTLS = true; + settings.Email.From = "sc@example.com"; + settings.Email.To = "ops@example.com"; + settings.Email.AuthenticationAccount = "user"; + settings.Email.AuthenticationPassword = "p@ssw0rd"; + + await manager.SaveChanges(); + } + + await CompleteDatabaseOperation(); + + await using var verifyManager = await NotificationsStore.CreateNotificationsManager(); + var loaded = await verifyManager.LoadSettings(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(loaded.Email.Enabled, Is.True); + Assert.That(loaded.Email.SmtpServer, Is.EqualTo("smtp.example.com")); + Assert.That(loaded.Email.SmtpPort, Is.EqualTo(587)); + Assert.That(loaded.Email.EnableTLS, Is.True); + Assert.That(loaded.Email.From, Is.EqualTo("sc@example.com")); + Assert.That(loaded.Email.To, Is.EqualTo("ops@example.com")); + Assert.That(loaded.Email.AuthenticationAccount, Is.EqualTo("user")); + Assert.That(loaded.Email.AuthenticationPassword, Is.EqualTo("p@ssw0rd")); + } + } + + [Test, CancelAfter(30_000)] + public async Task Toggling_enabled_is_persisted() + { + await using (var manager = await NotificationsStore.CreateNotificationsManager()) + { + var settings = await manager.LoadSettings(); + settings.Email.Enabled = true; + await manager.SaveChanges(); + } + + await CompleteDatabaseOperation(); + + await using (var manager = await NotificationsStore.CreateNotificationsManager()) + { + var settings = await manager.LoadSettings(); + Assert.That(settings.Email.Enabled, Is.True); + + settings.Email.Enabled = false; + await manager.SaveChanges(); + } + + await CompleteDatabaseOperation(); + + await using var verifyManager = await NotificationsStore.CreateNotificationsManager(); + var final = await verifyManager.LoadSettings(); + Assert.That(final.Email.Enabled, Is.False); + } + + [Test, CancelAfter(30_000)] + public async Task LoadSettings_returns_previously_saved_settings() + { + await using (var manager = await NotificationsStore.CreateNotificationsManager()) + { + var settings = await manager.LoadSettings(); + settings.Email.SmtpServer = "configured.server"; + settings.Email.SmtpPort = 2525; + await manager.SaveChanges(); + } + + await CompleteDatabaseOperation(); + + await using var manager2 = await NotificationsStore.CreateNotificationsManager(); + var loaded = await manager2.LoadSettings(); + + using (Assert.EnterMultipleScope()) + { + Assert.That(loaded.Email.SmtpServer, Is.EqualTo("configured.server")); + Assert.That(loaded.Email.SmtpPort, Is.EqualTo(2525)); + // Untouched fields keep their defaults + Assert.That(loaded.Email.Enabled, Is.False); + Assert.That(loaded.Email.EnableTLS, Is.False); + } + } + + [Test, CancelAfter(30_000)] + public async Task Updating_individual_fields_preserves_others() + { + await using (var manager = await NotificationsStore.CreateNotificationsManager()) + { + var settings = await manager.LoadSettings(); + settings.Email.Enabled = true; + settings.Email.SmtpServer = "original.smtp"; + settings.Email.SmtpPort = 25; + settings.Email.EnableTLS = false; + settings.Email.From = "from@orig"; + settings.Email.To = "to@orig"; + settings.Email.AuthenticationAccount = "acct"; + settings.Email.AuthenticationPassword = "secret"; + await manager.SaveChanges(); + } + + await CompleteDatabaseOperation(); + + await using (var manager = await NotificationsStore.CreateNotificationsManager()) + { + var settings = await manager.LoadSettings(); + settings.Email.SmtpServer = "updated.smtp"; + settings.Email.EnableTLS = true; + await manager.SaveChanges(); + } + + await CompleteDatabaseOperation(); + + await using var verifyManager = await NotificationsStore.CreateNotificationsManager(); + var loaded = await verifyManager.LoadSettings(); + + using (Assert.EnterMultipleScope()) + { + // Updated fields + Assert.That(loaded.Email.SmtpServer, Is.EqualTo("updated.smtp")); + Assert.That(loaded.Email.EnableTLS, Is.True); + // Preserved fields + Assert.That(loaded.Email.Enabled, Is.True); + Assert.That(loaded.Email.SmtpPort, Is.EqualTo(25)); + Assert.That(loaded.Email.From, Is.EqualTo("from@orig")); + Assert.That(loaded.Email.To, Is.EqualTo("to@orig")); + Assert.That(loaded.Email.AuthenticationAccount, Is.EqualTo("acct")); + Assert.That(loaded.Email.AuthenticationPassword, Is.EqualTo("secret")); + } + } +} \ No newline at end of file diff --git a/src/ServiceControl.Persistence.Tests/Recoverability/EditMessageTests.cs b/src/ServiceControl.Persistence.Tests/Recoverability/EditMessageTests.cs index 1c571b413b..94eef1fa3d 100644 --- a/src/ServiceControl.Persistence.Tests/Recoverability/EditMessageTests.cs +++ b/src/ServiceControl.Persistence.Tests/Recoverability/EditMessageTests.cs @@ -76,7 +76,7 @@ public async Task Should_discard_edit_when_different_edit_already_exists() _ = await CreateAndStoreFailedMessage(failedMessageId); - using (var editFailedMessagesManager = await EditFailedMessagesStore.CreateEditFailedMessageManager()) + await using (var editFailedMessagesManager = await EditFailedMessagesStore.CreateEditFailedMessageManager()) { _ = await editFailedMessagesManager.GetFailedMessage(failedMessageId); await editFailedMessagesManager.SetCurrentEditingRequestId(previousEdit); @@ -88,7 +88,7 @@ public async Task Should_discard_edit_when_different_edit_already_exists() // Act await handler.Handle(message, new TestableMessageHandlerContext()); - using (var editFailedMessagesManagerAssert = await EditFailedMessagesStore.CreateEditFailedMessageManager()) + await using (var editFailedMessagesManagerAssert = await EditFailedMessagesStore.CreateEditFailedMessageManager()) { var failedMessage = await editFailedMessagesManagerAssert.GetFailedMessage(failedMessageId); var editId = await editFailedMessagesManagerAssert.GetCurrentEditingRequestId(failedMessageId); @@ -125,18 +125,16 @@ public async Task Should_dispatch_edited_message_when_first_edit() Assert.That(dispatchedMessage.Item1.Message.Headers["someKey"], Is.EqualTo("someValue")); } - using (var x = await EditFailedMessagesStore.CreateEditFailedMessageManager()) - { - var failedMessage2 = await x.GetFailedMessage(failedMessage.UniqueMessageId); - Assert.That(failedMessage2, Is.Not.Null, "Edited failed message"); + await using var x = await EditFailedMessagesStore.CreateEditFailedMessageManager(); + var failedMessage2 = await x.GetFailedMessage(failedMessage.UniqueMessageId); + Assert.That(failedMessage2, Is.Not.Null, "Edited failed message"); - var editId = await x.GetCurrentEditingRequestId(failedMessage2.UniqueMessageId); + var editId = await x.GetCurrentEditingRequestId(failedMessage2.UniqueMessageId); - using (Assert.EnterMultipleScope()) - { - Assert.That(failedMessage2.Status, Is.EqualTo(FailedMessageStatus.Resolved), "Failed message status"); - Assert.That(editId, Is.EqualTo(handlerContent.MessageId), "MessageId"); - } + using (Assert.EnterMultipleScope()) + { + Assert.That(failedMessage2.Status, Is.EqualTo(FailedMessageStatus.Resolved), "Failed message status"); + Assert.That(editId, Is.EqualTo(handlerContent.MessageId), "MessageId"); } } diff --git a/src/ServiceControl.Persistence/IDataSessionManager.cs b/src/ServiceControl.Persistence/IDataSessionManager.cs index fa6973ce03..b87bd91802 100644 --- a/src/ServiceControl.Persistence/IDataSessionManager.cs +++ b/src/ServiceControl.Persistence/IDataSessionManager.cs @@ -3,7 +3,7 @@ using System; using System.Threading.Tasks; - public interface IDataSessionManager : IDisposable + public interface IDataSessionManager : IAsyncDisposable { Task SaveChanges(); } diff --git a/src/ServiceControl.Persistence/INotificationsManager.cs b/src/ServiceControl.Persistence/INotificationsManager.cs index 6bdb21dd5b..673932d5ec 100644 --- a/src/ServiceControl.Persistence/INotificationsManager.cs +++ b/src/ServiceControl.Persistence/INotificationsManager.cs @@ -1,11 +1,10 @@ namespace ServiceControl.Persistence { - using System; using System.Threading.Tasks; using Notifications; public interface INotificationsManager : IDataSessionManager { - Task LoadSettings(TimeSpan? cacheTimeout = null); + Task LoadSettings(); } } \ No newline at end of file diff --git a/src/ServiceControl.Persistence/NotificationsSettings.cs b/src/ServiceControl.Persistence/NotificationsSettings.cs index 97bc8c4f55..159052a419 100644 --- a/src/ServiceControl.Persistence/NotificationsSettings.cs +++ b/src/ServiceControl.Persistence/NotificationsSettings.cs @@ -2,8 +2,6 @@ { public class NotificationsSettings { - public string Id { get; set; } - - public EmailNotifications Email { get; set; } = new EmailNotifications(); + public EmailNotifications Email { get; init; } = new EmailNotifications(); } } \ No newline at end of file diff --git a/src/ServiceControl.UnitTests/MessageFailures/EditFailedMessagesControllerAuditTests.cs b/src/ServiceControl.UnitTests/MessageFailures/EditFailedMessagesControllerAuditTests.cs index 76bd5f3056..bab51af05a 100644 --- a/src/ServiceControl.UnitTests/MessageFailures/EditFailedMessagesControllerAuditTests.cs +++ b/src/ServiceControl.UnitTests/MessageFailures/EditFailedMessagesControllerAuditTests.cs @@ -48,15 +48,13 @@ sealed class FakeEditFailedMessagesManager : IEditFailedMessagesManager { public string? CurrentEditingRequestId { get; set; } - public void Dispose() - { - } - public Task SaveChanges() => Task.CompletedTask; public Task GetFailedMessage(string failedMessageId) => Task.FromResult(null); public Task GetCurrentEditingRequestId(string failedMessageId) => Task.FromResult(CurrentEditingRequestId); public Task SetCurrentEditingRequestId(string editingMessageId) => Task.CompletedTask; public Task SetFailedMessageAsResolved() => Task.CompletedTask; + + public ValueTask DisposeAsync() => ValueTask.CompletedTask; } sealed class StubErrorMessageDataStore : IFailedMessageQueryDataStore, IEditFailedMessagesDataStore diff --git a/src/ServiceControl/Notifications/Api/NotificationsController.cs b/src/ServiceControl/Notifications/Api/NotificationsController.cs index f25c3433da..c12ae41447 100644 --- a/src/ServiceControl/Notifications/Api/NotificationsController.cs +++ b/src/ServiceControl/Notifications/Api/NotificationsController.cs @@ -19,7 +19,7 @@ public class NotificationsController(INotificationsDataStore store, Settings set [HttpGet] public async Task GetEmailNotificationsSettings() { - using var manager = await store.CreateNotificationsManager(); + await using var manager = await store.CreateNotificationsManager(); var notificationsSettings = await manager.LoadSettings(); return notificationsSettings.Email; @@ -30,7 +30,7 @@ public async Task GetEmailNotificationsSettings() [HttpPost] public async Task ToggleEmailNotifications(ToggleEmailNotifications request) { - using var manager = await store.CreateNotificationsManager(); + await using var manager = await store.CreateNotificationsManager(); var notificationsSettings = await manager.LoadSettings(); notificationsSettings.Email.Enabled = request.Enabled; @@ -45,7 +45,7 @@ public async Task ToggleEmailNotifications(ToggleEmailNotificatio [HttpPost] public async Task UpdateSettings(UpdateEmailNotificationsSettingsRequest request) { - using var manager = await store.CreateNotificationsManager(); + await using var manager = await store.CreateNotificationsManager(); var notificationsSettings = await manager.LoadSettings(); var emailSettings = notificationsSettings.Email; @@ -70,7 +70,7 @@ public async Task UpdateSettings(UpdateEmailNotificationsSettings [HttpPost] public async Task SendTestEmail() { - using var manager = await store.CreateNotificationsManager(); + await using var manager = await store.CreateNotificationsManager(); var notificationsSettings = await manager.LoadSettings(); try diff --git a/src/ServiceControl/Notifications/Email/SendEmailNotificationHandler.cs b/src/ServiceControl/Notifications/Email/SendEmailNotificationHandler.cs index 6c9c2c9550..671491ed07 100644 --- a/src/ServiceControl/Notifications/Email/SendEmailNotificationHandler.cs +++ b/src/ServiceControl/Notifications/Email/SendEmailNotificationHandler.cs @@ -16,9 +16,9 @@ public async Task Handle(SendEmailNotification message, IMessageHandlerContext c { NotificationsSettings notifications; - using (var manager = await store.CreateNotificationsManager()) + await using (var manager = await store.CreateNotificationsManager()) { - notifications = await manager.LoadSettings(cacheTimeout); + notifications = await manager.LoadSettings(); } logger.LogInformation("Processing email notification. Subject: {Subject}, Body: {Body}", message.Subject, message.Body); @@ -85,7 +85,6 @@ public async Task Handle(SendEmailNotification message, IMessageHandlerContext c static readonly TimeSpan spinDelay = TimeSpan.FromSeconds(1); static readonly TimeSpan throttlingDelay = TimeSpan.FromSeconds(30); - static readonly TimeSpan cacheTimeout = TimeSpan.FromMinutes(5); public static RecoverabilityAction RecoverabilityPolicy(RecoverabilityConfig config, ErrorContext context) { diff --git a/src/ServiceControl/Recoverability/Editing/EditHandler.cs b/src/ServiceControl/Recoverability/Editing/EditHandler.cs index f0d3adbbcc..82a90b1fd7 100644 --- a/src/ServiceControl/Recoverability/Editing/EditHandler.cs +++ b/src/ServiceControl/Recoverability/Editing/EditHandler.cs @@ -24,7 +24,7 @@ public async Task Handle(EditAndSend message, IMessageHandlerContext context) { FailedMessage failedMessage; string editId; - using (var session = await store.CreateEditFailedMessageManager()) + await using (var session = await store.CreateEditFailedMessageManager()) { failedMessage = await session.GetFailedMessage(message.FailedMessageId);