diff --git a/docs/specs/audit-instance-servicecontrol-queue-address.md b/docs/specs/audit-instance-servicecontrol-queue-address.md new file mode 100644 index 0000000000..78b680d0b0 --- /dev/null +++ b/docs/specs/audit-instance-servicecontrol-queue-address.md @@ -0,0 +1,70 @@ +# Feature: Audit Instance ServiceControl Queue Address + +**As an operator adding instances through ServiceControl Management Utility (SCMU), I want every audit instance to be connected to a ServiceControl (error) instance so that messages the audit instance sends to the error instance always have a valid destination.** + +> Bug: [#4753 — SCMU does not set ServiceControlQueueAddress when only adding audit instances](https://github.com/Particular/ServiceControl/issues/4753). +> When the `ServiceControl.Audit/ServiceControlQueueAddress` setting is missing from +> `ServiceControl.Audit.exe.config`, the audit instance fails at runtime with +> ["no destination specified for message"](https://docs.particular.net/servicecontrol/troubleshooting#no-destination-specified-for-message). + +## Rules and Examples + +### Rule 1: Must address the audit instance to the error instance installed in the same session + +When the user installs an error instance and an audit instance together, the audit +instance's queue address is the name of the error instance being installed — never an +already-installed one. + +- **Example:** The one where both instances are installed together and the audit + instance's queue address is the new error instance's name. +- **Counter-example:** The one where other error instances already exist on the + machine, yet no choice is offered — the error instance being installed always wins. + +--- + +### Rule 2: Should auto-detect the existing error instance when adding an audit instance alone + +- **Example:** The one where exactly one error instance exists on the machine and its + name is used as the queue address without any user input (no dropdown shown). + +--- + +### Rule 3: Must require an explicit choice when multiple existing error instances are found + +Auto-detection cannot guess between several error instances — picking one silently +risks routing messages to the wrong instance. The choice dropdown is shown **only** in +this case. + +- **Example:** The one where two error instances exist and the dropdown offers both. +- **Example:** The one where Save is blocked until the user picks one of the detected + instances, and unblocked once a choice is made. + +--- + +### Rule 4: Must block installation when no error instance exists to connect to + +An audit instance without a reachable error instance is misconfigured by definition; +SCMU must not produce it. + +- **Example:** The one where no error instance exists, the user adds an audit instance + alone, and a validation error prevents the installation from proceeding. +- **Counter-example:** The one where only an error instance is being installed — the + queue address does not apply and no validation error is raised. + +## Resolved decisions (for implementation) + +- **Auto-detect source:** installed Windows error instances, discovered via + `InstanceFinder.ServiceControlInstances()`; exposed on the view model through a + `GetInstalledErrorInstanceNames` function seam (mirrors the existing + `GetWindowsServiceNames` pattern) so tests can substitute it. +- **Multiple instances found:** user must choose from a dropdown that is visible only + when adding an audit instance alone **and** more than one error instance is detected. +- **No instance found:** Save is blocked by a validation error; deploying with + PowerShell remains the path for advanced scenarios. +- **Acceptance tier:** view model + validator observed through `INotifyDataErrorInfo` + — the same mechanism the UI uses to block Save. A full SCMU end-to-end test (install + a Windows service, inspect the written config file) is not automatable in this + repository's test suites. +- **Out of scope:** registering the new audit instance as a remote of the existing + error instance (`AddRemoteInstance` is only called when both instances are installed + together) — candidate for a follow-up issue. diff --git a/src/ServiceControl.Config.Tests/.editorconfig b/src/ServiceControl.Config.Tests/.editorconfig index 5f68a610b3..c5410d8c02 100644 --- a/src/ServiceControl.Config.Tests/.editorconfig +++ b/src/ServiceControl.Config.Tests/.editorconfig @@ -2,3 +2,7 @@ # Justification: Test project dotnet_diagnostic.CA2007.severity = none + +# Justification: Executable specifications intentionally assign properties after the +# object initializer to mirror user interaction order (e.g. typing a name after load) +dotnet_diagnostic.IDE0017.severity = none diff --git a/src/ServiceControl.Config.Tests/AddInstance/AuditInstanceServiceControlQueueAddress.cs b/src/ServiceControl.Config.Tests/AddInstance/AuditInstanceServiceControlQueueAddress.cs new file mode 100644 index 0000000000..cc2105f8ab --- /dev/null +++ b/src/ServiceControl.Config.Tests/AddInstance/AuditInstanceServiceControlQueueAddress.cs @@ -0,0 +1,196 @@ +namespace ServiceControl.Config.Tests.AddInstance +{ + using System.ComponentModel; + using NUnit.Framework; + using ServiceControl.Config.UI.InstanceAdd; + + /// + /// Executable specification for docs/specs/audit-instance-servicecontrol-queue-address.md + /// (bug https://github.com/Particular/ServiceControl/issues/4753). + /// + /// Organized as feature > rule > examples: + /// - this outer class is the feature, + /// - each nested fixture is one rule from the spec, + /// - each test is one example, named with the spec's "The one where ..." language. + /// + /// These tests are the OUTER loop of a double-loop TDD process. They observe the view + /// model and its validator through INotifyDataErrorInfo - the same mechanism the UI + /// uses to block Save - and reference members that do not exist yet: + /// GetInstalledErrorInstanceNames, ServiceControlQueueAddress, + /// ServiceControlQueueAddressOptions, ShowServiceControlQueueAddressSelection. + /// + public class AuditInstanceServiceControlQueueAddress + { + [TestFixture] + public class Rule_1_Must_address_the_audit_instance_to_the_error_instance_installed_in_the_same_session + { + [Test] + public void The_one_where_both_instances_are_installed_together_and_the_new_error_instance_name_is_used() + { + var viewModel = new ServiceControlAddViewModel + { + InstallErrorInstance = true, + InstallAuditInstance = true, + SubmitAttempted = true, + // No pre-existing error instances on the machine + GetInstalledErrorInstanceNames = () => new string[0] + }; + + viewModel.ErrorInstanceName = "My.Error.Instance"; + + viewModel.NotifyOfPropertyChange(nameof(viewModel.ServiceControlQueueAddress)); + + var notifyErrorInfo = GetNotifyErrorInfo(viewModel); + + using (Assert.EnterMultipleScope()) + { + Assert.That(viewModel.ServiceControlQueueAddress, Is.EqualTo("My.Error.Instance")); + Assert.That(viewModel.ShowServiceControlQueueAddressSelection, Is.False); + Assert.That(notifyErrorInfo.GetErrors(nameof(viewModel.ServiceControlQueueAddress)), Is.Empty); + } + } + + [Test] + public void The_one_where_other_error_instances_already_exist_yet_no_choice_is_offered_because_the_instance_being_installed_wins() + { + var viewModel = new ServiceControlAddViewModel + { + InstallErrorInstance = true, + InstallAuditInstance = true, + GetInstalledErrorInstanceNames = () => new string[] { "Particular.ServiceControl", "Particular.ServiceControl.2" } + }; + + Assert.That(viewModel.ShowServiceControlQueueAddressSelection, Is.False); + } + } + + [TestFixture] + public class Rule_2_Should_auto_detect_the_existing_error_instance_when_adding_an_audit_instance_alone + { + [Test] + public void The_one_where_a_single_error_instance_exists_and_its_name_is_used_without_any_user_input() + { + var viewModel = new ServiceControlAddViewModel + { + InstallErrorInstance = false, + InstallAuditInstance = true, + SubmitAttempted = true, + GetInstalledErrorInstanceNames = () => new string[] { "Particular.ServiceControl" } + }; + + viewModel.NotifyOfPropertyChange(nameof(viewModel.ServiceControlQueueAddress)); + + var notifyErrorInfo = GetNotifyErrorInfo(viewModel); + + using (Assert.EnterMultipleScope()) + { + Assert.That(viewModel.ServiceControlQueueAddress, Is.EqualTo("Particular.ServiceControl")); + Assert.That(viewModel.ShowServiceControlQueueAddressSelection, Is.False, "Dropdown must not show when there is only one existing error instance"); + Assert.That(notifyErrorInfo.GetErrors(nameof(viewModel.ServiceControlQueueAddress)), Is.Empty); + } + } + } + + [TestFixture] + public class Rule_3_Must_require_an_explicit_choice_when_multiple_existing_error_instances_are_found + { + [Test] + public void The_one_where_two_error_instances_exist_and_the_dropdown_offers_both() + { + var viewModel = new ServiceControlAddViewModel + { + InstallErrorInstance = false, + InstallAuditInstance = true, + GetInstalledErrorInstanceNames = () => new string[] { "Particular.ServiceControl", "Particular.ServiceControl.2" } + }; + + using (Assert.EnterMultipleScope()) + { + Assert.That(viewModel.ShowServiceControlQueueAddressSelection, Is.True); + Assert.That(viewModel.ServiceControlQueueAddressOptions, Is.EquivalentTo(new[] + { + "Particular.ServiceControl", + "Particular.ServiceControl.2" + })); + } + } + + [Test] + public void The_one_where_save_is_blocked_until_the_user_picks_one_of_the_detected_instances() + { + var viewModel = new ServiceControlAddViewModel + { + InstallErrorInstance = false, + InstallAuditInstance = true, + SubmitAttempted = true, + GetInstalledErrorInstanceNames = () => new string[] { "Particular.ServiceControl", "Particular.ServiceControl.2" } + }; + + viewModel.NotifyOfPropertyChange(nameof(viewModel.ServiceControlQueueAddress)); + + var notifyErrorInfo = GetNotifyErrorInfo(viewModel); + + // No selection made yet: must be blocked + Assert.That(notifyErrorInfo.GetErrors(nameof(viewModel.ServiceControlQueueAddress)), Is.Not.Empty, + "A validation error is expected until the user picks one of the existing error instances"); + + // User picks an instance from the dropdown + viewModel.ServiceControlQueueAddress = "Particular.ServiceControl.2"; + + using (Assert.EnterMultipleScope()) + { + Assert.That(viewModel.ServiceControlQueueAddress, Is.EqualTo("Particular.ServiceControl.2")); + Assert.That(notifyErrorInfo.GetErrors(nameof(viewModel.ServiceControlQueueAddress)), Is.Empty); + } + } + } + + [TestFixture] + public class Rule_4_Must_block_installation_when_no_error_instance_exists_to_connect_to + { + [Test] + public void The_one_where_no_error_instance_exists_and_a_validation_error_prevents_the_installation_from_proceeding() + { + var viewModel = new ServiceControlAddViewModel + { + InstallErrorInstance = false, + InstallAuditInstance = true, + SubmitAttempted = true, + GetInstalledErrorInstanceNames = () => new string[0] + }; + + viewModel.NotifyOfPropertyChange(nameof(viewModel.ServiceControlQueueAddress)); + + var notifyErrorInfo = GetNotifyErrorInfo(viewModel); + + using (Assert.EnterMultipleScope()) + { + Assert.That(viewModel.ServiceControlQueueAddress, Is.Null.Or.Empty); + Assert.That(viewModel.ShowServiceControlQueueAddressSelection, Is.False); + Assert.That(notifyErrorInfo.GetErrors(nameof(viewModel.ServiceControlQueueAddress)), Is.Not.Empty, + "A validation error is expected so the user cannot proceed without an existing error instance to connect to"); + } + } + + [Test] + public void The_one_where_only_an_error_instance_is_installed_and_the_queue_address_does_not_apply() + { + var viewModel = new ServiceControlAddViewModel + { + InstallErrorInstance = true, + InstallAuditInstance = false, + SubmitAttempted = true, + GetInstalledErrorInstanceNames = () => new string[0] + }; + + viewModel.NotifyOfPropertyChange(nameof(viewModel.ServiceControlQueueAddress)); + + var notifyErrorInfo = GetNotifyErrorInfo(viewModel); + + Assert.That(notifyErrorInfo.GetErrors(nameof(viewModel.ServiceControlQueueAddress)), Is.Empty); + } + } + + static INotifyDataErrorInfo GetNotifyErrorInfo(object vm) => vm as INotifyDataErrorInfo; + } +} diff --git a/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs b/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs index 118d0bc687..3c97a982ec 100644 --- a/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs +++ b/src/ServiceControl.Config.Tests/ServiceControlAddScreenLoadedTests.cs @@ -153,7 +153,7 @@ public void Database_maintenance_port_number_are_set_to_defaults_with_no_validat [Test] public void Destination_path_is_null() { - var viewModel = new ServiceControlAddViewModel(); + var viewModel = new ServiceControlAddViewModel(() => []); var errorInfo = (INotifyDataErrorInfo)viewModel; @@ -169,7 +169,7 @@ public void Destination_path_is_null() [Test] public void Log_path_is_null() { - var viewModel = new ServiceControlAddViewModel(); + var viewModel = new ServiceControlAddViewModel(() => []); var errorInfo = (INotifyDataErrorInfo)viewModel; @@ -186,7 +186,7 @@ public void Log_path_is_null() [Test] public void Database_path_is_null() { - var viewModel = new ServiceControlAddViewModel(); + var viewModel = new ServiceControlAddViewModel(() => []); var errorInfo = (INotifyDataErrorInfo)viewModel; diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs index 1d3c08009d..5c9863dd03 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddAttachment.cs @@ -100,7 +100,7 @@ async Task Add() auditNewInstance.AuditRetentionPeriod = viewModel.ServiceControlAudit.AuditRetentionPeriod; auditNewInstance.ServiceAccount = viewModel.ServiceControlAudit.ServiceAccount; auditNewInstance.ServiceAccountPwd = viewModel.ServiceControlAudit.Password; - auditNewInstance.ServiceControlQueueAddress = serviceControlNewInstance == null ? string.Empty : serviceControlNewInstance.InstanceName; + auditNewInstance.ServiceControlQueueAddress = viewModel.ServiceControlQueueAddress; auditNewInstance.EnableFullTextSearchOnBodies = viewModel.ServiceControlAudit.EnableFullTextSearchOnBodies.Value; } diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddView.xaml b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddView.xaml index 9ace3fbd7a..a1d1739098 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddView.xaml +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddView.xaml @@ -299,6 +299,21 @@ + + + + diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModel.cs b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModel.cs index cc8bdb3d2a..ce26263a1b 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModel.cs +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModel.cs @@ -1,4 +1,4 @@ -namespace ServiceControl.Config.UI.InstanceAdd +namespace ServiceControl.Config.UI.InstanceAdd { using System; using System.Collections.Generic; @@ -8,16 +8,21 @@ using System.Windows.Input; using PropertyChanged; using ServiceControl.Config.Extensions; + using ServiceControlInstaller.Engine.Instances; using Validar; using Xaml.Controls; [InjectValidation] public class ServiceControlAddViewModel : ServiceControlEditorViewModel { - public ServiceControlAddViewModel() + // The constructor runs the unique-name convention against the machine's Windows + // services, so tests inject a fake here to stay environment-independent; the + // GetWindowsServiceNames property seam is set too late for construction-time logic + public ServiceControlAddViewModel(Func getWindowsServiceNames = null) { DisplayName = "ADD SERVICECONTROL"; - GetWindowsServiceNames = () => ServiceController.GetServices().Select(windowsService => windowsService.ServiceName).ToArray(); + GetWindowsServiceNames = getWindowsServiceNames ?? (() => ServiceController.GetServices().Select(windowsService => windowsService.ServiceName).ToArray()); + GetInstalledErrorInstanceNames = () => InstanceFinder.ServiceControlInstances().Select(instance => instance.Name).ToArray(); ConventionName = "Particular.ServiceControl"; OnConventionNameChanged(); @@ -30,6 +35,18 @@ public ServiceControlAddViewModel() ServiceControl.PropertyChanged += ServiceControl_PropertyChanged; ServiceControlAudit.PropertyChanged += ServiceControl_PropertyChanged; + PropertyChanged += (_, e) => + { + // InstallErrorInstance/InstallAuditInstance live on the base class, so Fody + // cannot infer that these derived computed properties depend on them + if (e.PropertyName is nameof(InstallErrorInstance) or nameof(InstallAuditInstance)) + { + NotifyOfPropertyChange(nameof(ServiceControlQueueAddress)); + NotifyOfPropertyChange(nameof(ServiceControlQueueAddressOptions)); + NotifyOfPropertyChange(nameof(ShowServiceControlQueueAddressSelection)); + NotifyOfPropertyChange(nameof(ShowNoErrorInstanceFoundWarning)); + } + }; } void ServiceControl_PropertyChanged(object sender, System.ComponentModel.PropertyChangedEventArgs e) @@ -47,6 +64,40 @@ void ServiceControl_PropertyChanged(object sender, System.ComponentModel.Propert public Func GetWindowsServiceNames { get; set; } + public Func GetInstalledErrorInstanceNames { get; set; } + + public string[] ServiceControlQueueAddressOptions => GetInstalledErrorInstanceNames(); + + public string ServiceControlQueueAddress + { + get + { + if (InstallErrorInstance) + { + return ErrorInstanceName; + } + + var installedErrorInstanceNames = GetInstalledErrorInstanceNames(); + + return installedErrorInstanceNames.Length == 1 + ? installedErrorInstanceNames[0] + : serviceControlQueueAddress; + } + set => serviceControlQueueAddress = value; + } + + string serviceControlQueueAddress; + + public bool ShowServiceControlQueueAddressSelection => + InstallAuditInstance + && !InstallErrorInstance + && GetInstalledErrorInstanceNames().Length > 1; + + public bool ShowNoErrorInstanceFoundWarning => + InstallAuditInstance + && !InstallErrorInstance + && GetInstalledErrorInstanceNames().Length == 0; + public string ConventionName { get; set; } public void OnConventionNameChanged() diff --git a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModelValidator.cs b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModelValidator.cs index 7a627d954a..41aac63ee0 100644 --- a/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModelValidator.cs +++ b/src/ServiceControl.Config/UI/InstanceAdd/ServiceControlAddViewModelValidator.cs @@ -150,6 +150,11 @@ public ServiceControlAddViewModelValidator() .WithMessage("Audit instance name is already in use") .When(viewModel => viewModel.InstallAuditInstance); + RuleFor(viewModel => viewModel.ServiceControlQueueAddress) + .NotEmpty() + .WithMessage("An existing error instance must be selected to receive audit data") + .When(viewModel => viewModel.InstallAuditInstance && !viewModel.InstallErrorInstance); + RuleFor(x => x.AuditServiceAccount) .NotEmpty() .When(x => x.InstallAuditInstance && x.SubmitAttempted);