From 4c6524872744df9372be157d06a43f15d9927ab0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Eggert=20Lundsteen=20Olsen?= Date: Fri, 2 Oct 2026 12:40:03 +0200 Subject: [PATCH] Review 1.3: an instance removed from the configuration file is marked removed, not deleted The agent deleted ConfigFile rows no longer in the file, and with them (cascade) the pause and the database choices: commenting an instance out for a restart lost them. It is now soft-deleted like a Web instance and brought back, choices and all, when it returns to the file. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/servere-og-databaser.md | 2 +- ...InsightRepository.InstanceConfiguration.cs | 34 ++++++++++++------- .../0016_instance_configuration.sql | 7 ++-- .../InstanceConfigurationRepositoryTests.cs | 13 +++++-- 4 files changed, 38 insertions(+), 18 deletions(-) diff --git a/docs/servere-og-databaser.md b/docs/servere-og-databaser.md index 6a807b1..9df5fe1 100644 --- a/docs/servere-og-databaser.md +++ b/docs/servere-og-databaser.md @@ -47,7 +47,7 @@ AX-fund sendes som standard **ikke** til SQLInsight Hub. Kolonnen *AX-fund til S ## Servere fra konfigurationsfilen -Servere under `SQLInsight:Instances` i agentens `appsettings.json` eller `sqlinsight.ini` (fx `MONITOR_SERVER` fra installeren) virker som før: ved hver start spejler agenten dem ind i repositoriet, hvor de vises som **fra konfigurationsfil**. Forbindelsen (og en eventuel adgangskode) bliver i filen; den kopieres aldrig til repositoriet. Her kan de sættes på pause, og databaserne kan vælges, men de rettes og fjernes i filen. Som standard overvåges alle deres databaser, også nye, præcis som i 1.2. Bruges samme navn både i filen og i webbrugerfladen, vinder filen. +Servere under `SQLInsight:Instances` i agentens `appsettings.json` eller `sqlinsight.ini` (fx `MONITOR_SERVER` fra installeren) virker som før: ved hver start spejler agenten dem ind i repositoriet, hvor de vises som **fra konfigurationsfil**. Forbindelsen (og en eventuel adgangskode) bliver i filen; den kopieres aldrig til repositoriet. Her kan de sættes på pause, og databaserne kan vælges, men de rettes og fjernes i filen. Fjernes en server fra filen, markeres den som fjernet ved agentens næste start (det indsamlede bevares); sættes den i filen igen, får den sin pause og sine databasevalg tilbage. Som standard overvåges alle deres databaser, også nye, præcis som i 1.2. Bruges samme navn både i filen og i webbrugerfladen, vinder filen. Ændringer i webbrugerfladen (nye servere, pause, fjernelse, valg af databaser) bruges af agenten inden for et minut, uden genstart. Ændringer i konfigurationsfilen kræver stadig en genstart af agenten. diff --git a/src/SQLInsight.Repository/InsightRepository.InstanceConfiguration.cs b/src/SQLInsight.Repository/InsightRepository.InstanceConfiguration.cs index 8c6a60d..1394e18 100644 --- a/src/SQLInsight.Repository/InsightRepository.InstanceConfiguration.cs +++ b/src/SQLInsight.Repository/InsightRepository.InstanceConfiguration.cs @@ -349,8 +349,9 @@ public sealed partial class InsightRepository /// /// Brings the ConfigFile rows in step with SQLInsight:Instances in the agent's configuration: new ones are added - /// (monitoring every database, as before 1.3), changed ones updated, and ones no longer in the file deleted. A Web - /// instance with the same name gives way to the file. Returns the ids by name. + /// (monitoring every database, as before 1.3), changed ones updated, and ones no longer in the file marked removed + /// (RemovedAt, like a removed Web instance), so their pause and database choices come back with them if they are put + /// back in the file. A Web instance with the same name gives way to the file. Returns the ids by name. /// public async Task> SyncConfigFileInstancesAsync(IReadOnlyList instances, DateTimeOffset now, CancellationToken cancellationToken = default) @@ -361,27 +362,36 @@ public sealed partial class InsightRepository var at = Utc(now); var names = instances.Select(i => i.Name).ToList(); await connection.ExecuteAsync(""" - DELETE FROM insight.ConfiguredInstances - WHERE Source = 'ConfigFile' AND Name NOT IN (SELECT value FROM OPENJSON(@names)); + UPDATE insight.ConfiguredInstances + SET RemovedAt = @at, RemovedBy = N'SQLInsight Agent: ikke længere i konfigurationsfilen', UpdatedAt = @at, UpdatedBy = N'SQLInsight Agent (konfigurationsfil)' + WHERE Source = 'ConfigFile' AND RemovedAt IS NULL AND Name NOT IN (SELECT value FROM OPENJSON(@names)); UPDATE insight.ConfiguredInstances SET RemovedAt = @at, RemovedBy = N'SQLInsight Agent: navnet bruges i konfigurationsfilen' WHERE Source = 'Web' AND RemovedAt IS NULL AND Name IN (SELECT value FROM OPENJSON(@names)); """, new { names = System.Text.Json.JsonSerializer.Serialize(names), at }, transaction).ConfigureAwait(false); var ids = new Dictionary(StringComparer.OrdinalIgnoreCase); foreach (var instance in instances) { + // At most one ConfigFile row per name: a removed one is brought back (with its choices) rather than a new one added. ids[instance.Name] = await connection.ExecuteScalarAsync(""" - UPDATE insight.ConfiguredInstances - SET Server = @Server, Port = NULL, Authentication = @Authentication, UserName = @UserName, Encryption = @Encryption, - TrustServerCertificate = @TrustServerCertificate, PasswordCipher = NULL, PasswordKeyId = NULL - WHERE Source = 'ConfigFile' AND Name = @Name - AND (Server <> @Server OR Authentication <> @Authentication OR ISNULL(UserName, N'') <> ISNULL(@UserName, N'') - OR Encryption <> @Encryption OR TrustServerCertificate <> @TrustServerCertificate OR Port IS NOT NULL); - IF NOT EXISTS (SELECT 1 FROM insight.ConfiguredInstances WHERE Source = 'ConfigFile' AND Name = @Name) + DECLARE @id int = (SELECT TOP (1) ConfiguredInstanceId FROM insight.ConfiguredInstances + WHERE Source = 'ConfigFile' AND Name = @Name ORDER BY CASE WHEN RemovedAt IS NULL THEN 0 ELSE 1 END, ConfiguredInstanceId DESC); + IF @id IS NULL + BEGIN INSERT insight.ConfiguredInstances (Name, Server, Authentication, UserName, Encryption, TrustServerCertificate, Source, Paused, MonitorNewDatabases, CreatedAt, CreatedBy, UpdatedAt, UpdatedBy) VALUES (@Name, @Server, @Authentication, @UserName, @Encryption, @TrustServerCertificate, 'ConfigFile', 0, 1, @at, N'SQLInsight Agent (konfigurationsfil)', @at, N'SQLInsight Agent (konfigurationsfil)'); - SELECT ConfiguredInstanceId FROM insight.ConfiguredInstances WHERE Source = 'ConfigFile' AND Name = @Name; + SET @id = SCOPE_IDENTITY(); + END + ELSE + UPDATE insight.ConfiguredInstances + SET Server = @Server, Port = NULL, Authentication = @Authentication, UserName = @UserName, Encryption = @Encryption, + TrustServerCertificate = @TrustServerCertificate, PasswordCipher = NULL, PasswordKeyId = NULL, RemovedAt = NULL, RemovedBy = NULL, + UpdatedAt = @at, UpdatedBy = N'SQLInsight Agent (konfigurationsfil)' + WHERE ConfiguredInstanceId = @id + AND (RemovedAt IS NOT NULL OR Server <> @Server OR Authentication <> @Authentication OR ISNULL(UserName, N'') <> ISNULL(@UserName, N'') + OR Encryption <> @Encryption OR TrustServerCertificate <> @TrustServerCertificate OR Port IS NOT NULL); + SELECT @id; """, new { instance.Name, instance.Server, instance.Authentication, instance.UserName, instance.Encryption, instance.TrustServerCertificate, at }, transaction).ConfigureAwait(false); } diff --git a/src/SQLInsight.Repository/Migrations/0016_instance_configuration.sql b/src/SQLInsight.Repository/Migrations/0016_instance_configuration.sql index 25a9074..689492f 100644 --- a/src/SQLInsight.Repository/Migrations/0016_instance_configuration.sql +++ b/src/SQLInsight.Repository/Migrations/0016_instance_configuration.sql @@ -37,9 +37,10 @@ CREATE TABLE insight.ConfiguredInstances RemovedBy nvarchar(256) NULL, LastDiscoveryAt datetime2(0) NULL, LastDiscoveryError nvarchar(1000) NULL, - CredentialError nvarchar(1000) NULL, - /* An instance from the configuration file is removed by removing it from the file. */ - CONSTRAINT CK_ConfiguredInstances_RemoveWeb CHECK (Source = 'Web' OR RemovedAt IS NULL) + CredentialError nvarchar(1000) NULL + /* An instance from the configuration file is removed by removing it from the file: the agent then sets RemovedAt + (like a removed Web instance, so its history, pause and database choices are kept), and clears it again if the + instance comes back in the file. */ ); CREATE UNIQUE INDEX UQ_ConfiguredInstances_Name ON insight.ConfiguredInstances (Name) WHERE RemovedAt IS NULL; diff --git a/tests/SQLInsight.IntegrationTests/InstanceConfigurationRepositoryTests.cs b/tests/SQLInsight.IntegrationTests/InstanceConfigurationRepositoryTests.cs index 09eb7d9..c0e0a6f 100644 --- a/tests/SQLInsight.IntegrationTests/InstanceConfigurationRepositoryTests.cs +++ b/tests/SQLInsight.IntegrationTests/InstanceConfigurationRepositoryTests.cs @@ -68,7 +68,6 @@ public class InstanceConfigurationRepositoryTests(SqlServerFixture sql) : IAsync await Assert.ThrowsAsync(() => web.CompleteAgentRequestAsync(request, true, "{}", null, now)); await Assert.ThrowsAsync(() => web.SaveDiscoveredDatabasesAsync(id, [Db("Sales")], now)); await Assert.ThrowsAsync(() => web.PublishAgentKeyAsync(new AgentKeyRow { KeyId = "x", PublicKey = [1], MachineName = "m", ServiceAccount = "a", CreatedAt = DateTime.UtcNow, LastSeenAt = DateTime.UtcNow })); - await Assert.ThrowsAsync(() => web.SyncConfigFileInstancesAsync([], now)); await Assert.ThrowsAsync(() => ExecuteAsWebAsync($"DELETE FROM insight.ConfiguredInstances WHERE ConfiguredInstanceId = {id}")); await Assert.ThrowsAsync(() => ExecuteAsWebAsync($"UPDATE insight.ConfiguredInstances SET Source = 'ConfigFile' WHERE ConfiguredInstanceId = {id}")); await Assert.ThrowsAsync(() => ExecuteAsWebAsync($"UPDATE insight.ConfiguredInstances SET Name = N'x' WHERE ConfiguredInstanceId = {id}")); @@ -142,11 +141,21 @@ public class InstanceConfigurationRepositoryTests(SqlServerFixture sql) : IAsync new ConfiguredInstanceChange("evil", null, "Windows", null, null, null, false, "Mandatory", false), null, now)); Assert.Equal(ConfigurationChangeOutcome.Changed, await _repository.SetInstancePausedAsync(ids["prod-sql01"], true, null, now)); - // Started again with one instance changed and one gone: the same row is kept (with its choices), the other deleted. + // Started again with one instance changed and one gone: the same row is kept (with its choices), the other marked removed. var again = await _repository.SyncConfigFileInstancesAsync([new ConfigFileInstance("prod-sql01", "sql01b", "Windows", null, "Mandatory", false)], now); Assert.Equal(ids["prod-sql01"], again["prod-sql01"]); var row = Assert.Single(await _repository.GetConfiguredInstancesAsync()); Assert.Equal(("sql01b", true), (row.Server, row.Paused)); + Assert.Null(await _repository.GetConfiguredInstanceAsync(ids["prod-sql02"])); + + // Put back in the file: the same row comes back, nothing is lost and no second row is made. + var back = await _repository.SyncConfigFileInstancesAsync( + [ + new ConfigFileInstance("prod-sql01", "sql01b", "Windows", null, "Mandatory", false), + new ConfigFileInstance("prod-sql02", "sql02", "SqlLogin", "mon", "Optional", true), + ], now); + Assert.Equal(ids["prod-sql02"], back["prod-sql02"]); + Assert.Equal(2, (await _repository.GetConfiguredInstancesAsync()).Count); } [Fact]