diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/ArtifactDiff.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/ArtifactDiff.cs index fd5acb02..b035ce0b 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/ArtifactDiff.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/ArtifactDiff.cs @@ -170,6 +170,13 @@ public sealed class ArtifactDiff // previewed as "Identical". Mirrors CompareDatabaseConnection below. AddIfDifferent(changes, "MaxRetries", existing.MaxRetries, incoming.MaxRetries); AddIfDifferent(changes, "RetryDelay", existing.RetryDelay.ToString(), incoming.RetryDelay.ToString()); + // arch-review 2026-08-19 P7-1 (row-27 shape): TimeoutSeconds is operative at + // runtime and the importer has always APPLIED it, but the diff omitted it — same + // "silently changed and invisible to preview" defect as the MaxRetries fix above. + // A null incoming value (bundle written before this field existed) compares as + // "no change" — it mirrors BundleImporter's Overwrite fallback, which leaves the + // target's existing timeout untouched in that case. + AddIfDifferent(changes, "TimeoutSeconds", existing.TimeoutSeconds, incoming.TimeoutSeconds ?? existing.TimeoutSeconds); // Secrets: presence-only comparison (we never echo the value in the diff). var existingHasSecret = !string.IsNullOrEmpty(existing.AuthConfiguration); diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs index c11b727d..82b8ad93 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs @@ -3090,6 +3090,9 @@ public sealed class BundleImporter : IBundleImporter existing.AuthConfiguration = dto.Secrets?.Values.TryGetValue("AuthConfiguration", out var auth) == true ? auth : null; existing.MaxRetries = dto.MaxRetries; existing.RetryDelay = dto.RetryDelay; + // A bundle written before this field existed carries null — leave the + // target's existing timeout untouched rather than resetting it to 0/unset. + existing.TimeoutSeconds = dto.TimeoutSeconds ?? existing.TimeoutSeconds; await _externalRepo.UpdateExternalSystemAsync(existing, ct).ConfigureAwait(false); await _auditService.LogAsync(user, "Update", "ExternalSystem", existing.Id.ToString(), existing.Name, new { existing.Name, existing.EndpointUrl }, ct).ConfigureAwait(false); @@ -3123,6 +3126,9 @@ public sealed class BundleImporter : IBundleImporter AuthConfiguration = dto.Secrets?.Values.TryGetValue("AuthConfiguration", out var auth) == true ? auth : null, MaxRetries = dto.MaxRetries, RetryDelay = dto.RetryDelay, + // Null (a bundle written before this field existed) falls back to the + // entity's own "0 = unset" default. + TimeoutSeconds = dto.TimeoutSeconds ?? 0, }; return sys; } diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntityDtos.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntityDtos.cs index 2e5ea0bf..8f99d3e6 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntityDtos.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntityDtos.cs @@ -252,7 +252,14 @@ public sealed record ExternalSystemDto( int MaxRetries, TimeSpan RetryDelay, IReadOnlyList Methods, - SecretsBlock? Secrets); + SecretsBlock? Secrets, + // Per-system HTTP round-trip timeout (arch-review 2026-08-19 P7-1, row-27 shape). + // Additive trailing field; null distinguishes a bundle written before this field + // existed from a bundle that explicitly carries the entity's own "0 = unset" + // sentinel. BundleImporter treats a null incoming value as "not specified by this + // bundle": Add falls back to the entity default (0), Overwrite leaves the target's + // existing value untouched. + int? TimeoutSeconds = null); public sealed record ExternalSystemMethodDto( string Name, diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntitySerializer.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntitySerializer.cs index a39335ed..dc6ec230 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntitySerializer.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Serialization/EntitySerializer.cs @@ -139,7 +139,8 @@ public sealed class EntitySerializer MaxRetries: sys.MaxRetries, RetryDelay: sys.RetryDelay, Methods: methods, - Secrets: secrets); + Secrets: secrets, + TimeoutSeconds: sys.TimeoutSeconds); }).ToList(), DatabaseConnections: aggregate.DatabaseConnections.Select(dc => new DatabaseConnectionDto( Name: dc.Name, @@ -433,6 +434,9 @@ public sealed class EntitySerializer AuthConfiguration = dto.Secrets?.Values.TryGetValue("AuthConfiguration", out var auth) == true ? auth : null, MaxRetries = dto.MaxRetries, RetryDelay = dto.RetryDelay, + // Null (a bundle written before this field existed) falls back to the + // entity's own "0 = unset" default. + TimeoutSeconds = dto.TimeoutSeconds ?? 0, }; externalSystems.Add(sys); foreach (var m in dto.Methods) diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs index 66a0a35b..d8bdaa92 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/Import/BundleImporterApplyTests.cs @@ -789,6 +789,194 @@ public sealed class BundleImporterApplyTests : IDisposable Assert.Equal(1, result.Overwritten); } + [Fact] + public async Task ApplyAsync_adds_new_external_system_with_TimeoutSeconds() + { + // arch-review 2026-08-19 P7-1 (row-27 shape): TimeoutSeconds is the + // per-system HTTP timeout ExternalSystemClient reads at call time — it must + // survive Add, not just the retry policy (T-002 above covers the sibling + // Methods-drop regression on Overwrite; this is the Add-path fidelity gap). + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + ctx.ExternalSystemDefinitions.Add(new ExternalSystemDefinition("Erp", "https://erp.example", "ApiKey") + { + TimeoutSeconds = 120, + }); + await ctx.SaveChangesAsync(); + } + var sessionId = await ExportAndLoadAsync(); + + // Wipe so the import exercises the Add path. + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + ctx.ExternalSystemDefinitions.RemoveRange(ctx.ExternalSystemDefinitions); + await ctx.SaveChangesAsync(); + } + + await using (var scope = _provider.CreateAsyncScope()) + { + var importer = scope.ServiceProvider.GetRequiredService(); + await importer.ApplyAsync(sessionId, + new List { new("ExternalSystem", "Erp", ResolutionAction.Add, null) }, + user: "bob"); + } + + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + var sys = await ctx.ExternalSystemDefinitions.SingleAsync(e => e.Name == "Erp"); + Assert.Equal(120, sys.TimeoutSeconds); + } + } + + [Fact] + public async Task ApplyAsync_Overwrite_restores_TimeoutSeconds_to_bundle_value() + { + // Seed with TimeoutSeconds=120, export, drift the target to 45, then Overwrite + // and assert the bundle's value wins (mirrors the retry-policy Overwrite + // coverage — TimeoutSeconds was the one scalar field the Overwrite branch never + // touched at all, so it silently kept whatever the target already had). + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + ctx.ExternalSystemDefinitions.Add(new ExternalSystemDefinition("Erp", "https://erp.example", "ApiKey") + { + TimeoutSeconds = 120, + }); + await ctx.SaveChangesAsync(); + } + var sessionId = await ExportAndLoadAsync(); + + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + var sys = await ctx.ExternalSystemDefinitions.SingleAsync(e => e.Name == "Erp"); + sys.TimeoutSeconds = 45; // now the only field that differs from the bundle + await ctx.SaveChangesAsync(); + } + + ImportResult result; + await using (var scope = _provider.CreateAsyncScope()) + { + var importer = scope.ServiceProvider.GetRequiredService(); + result = await importer.ApplyAsync(sessionId, + new List { new("ExternalSystem", "Erp", ResolutionAction.Overwrite, null) }, + user: "bob"); + } + + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + var sys = await ctx.ExternalSystemDefinitions.SingleAsync(e => e.Name == "Erp"); + Assert.Equal(120, sys.TimeoutSeconds); + } + Assert.Equal(1, result.Overwritten); + } + + // ───────────────────────────────────────────────────────────────────── + // arch-review 2026-08-19 P7-1: old-bundle semantics for TimeoutSeconds. Hand-packs + // a BundleContentDto with an ExternalSystemDto whose TimeoutSeconds is left at its + // default (null) — the same in-memory shape a pre-P7-1 bundle deserializes to, + // since the field is simply absent from that JSON text. + // ───────────────────────────────────────────────────────────────────── + private async Task PackAndLoadExternalSystemBundleAsync(params ExternalSystemDto[] externalSystems) + { + var content = new BundleContentDto( + TemplateFolders: Array.Empty(), + Templates: Array.Empty(), + SharedScripts: Array.Empty(), + ExternalSystems: externalSystems, + DatabaseConnections: Array.Empty(), + NotificationLists: Array.Empty(), + SmtpConfigs: Array.Empty(), + ApiMethods: Array.Empty()); + + await using var scope = _provider.CreateAsyncScope(); + var serializer = scope.ServiceProvider.GetRequiredService(); + var manifestBuilder = scope.ServiceProvider.GetRequiredService(); + var importer = scope.ServiceProvider.GetRequiredService(); + + var contentBytes = serializer.SerializeContentBytes(content); + var manifest = manifestBuilder.Build( + sourceEnvironment: "dev", + exportedBy: "alice", + scadaBridgeVersion: "1.0.0", + encryption: null, + summary: new BundleSummary(0, 0, 0, externalSystems.Length, 0, 0, 0, 0), + contents: Array.Empty(), + contentBytes: contentBytes); + await using var packed = serializer.Pack(content, manifest, passphrase: null, encryptor: null); + + using var ms = new MemoryStream(); + await packed.CopyToAsync(ms); + ms.Position = 0; + var session = await importer.LoadAsync(ms, passphrase: null); + return session.SessionId; + } + + [Fact] + public async Task ApplyAsync_Add_with_old_bundle_missing_TimeoutSeconds_uses_entity_default() + { + var dto = new ExternalSystemDto("Erp", "https://erp.example", "ApiKey", + MaxRetries: 3, RetryDelay: TimeSpan.FromSeconds(5), Methods: [], Secrets: null, + TimeoutSeconds: null); // old-bundle shape — the field never existed on the wire. + var sessionId = await PackAndLoadExternalSystemBundleAsync(dto); + + await using (var scope = _provider.CreateAsyncScope()) + { + var importer = scope.ServiceProvider.GetRequiredService(); + await importer.ApplyAsync(sessionId, + new List { new("ExternalSystem", "Erp", ResolutionAction.Add, null) }, + user: "bob"); + } + + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + var sys = await ctx.ExternalSystemDefinitions.SingleAsync(e => e.Name == "Erp"); + Assert.Equal(0, sys.TimeoutSeconds); // entity default, not an exception or -1. + } + } + + [Fact] + public async Task ApplyAsync_Overwrite_with_old_bundle_missing_TimeoutSeconds_preserves_target_value() + { + // The target already has a tuned timeout; an old bundle that predates this field + // must not reset it to 0 on Overwrite — there is nothing on the wire to apply, so + // the existing value stands. + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + ctx.ExternalSystemDefinitions.Add(new ExternalSystemDefinition("Erp", "https://erp.example", "ApiKey") + { + TimeoutSeconds = 90, + }); + await ctx.SaveChangesAsync(); + } + + var dto = new ExternalSystemDto("Erp", "https://erp.example", "ApiKey", + MaxRetries: 3, RetryDelay: TimeSpan.FromSeconds(5), Methods: [], Secrets: null, + TimeoutSeconds: null); // old-bundle shape. + var sessionId = await PackAndLoadExternalSystemBundleAsync(dto); + + await using (var scope = _provider.CreateAsyncScope()) + { + var importer = scope.ServiceProvider.GetRequiredService(); + await importer.ApplyAsync(sessionId, + new List { new("ExternalSystem", "Erp", ResolutionAction.Overwrite, null) }, + user: "bob"); + } + + await using (var scope = _provider.CreateAsyncScope()) + { + var ctx = scope.ServiceProvider.GetRequiredService(); + var sys = await ctx.ExternalSystemDefinitions.SingleAsync(e => e.Name == "Erp"); + Assert.Equal(90, sys.TimeoutSeconds); // untouched — bundle carried no opinion. + } + } + // ───────────────────────────────────────────────────────────────────── // S10b: SMS provider config bundle apply. Mirrors the SMTP apply path — // create a new config (Add) and overwrite an existing one (matched by the diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/RoundTripEquivalenceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/RoundTripEquivalenceTests.cs index bbe6429a..b753e285 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/RoundTripEquivalenceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.IntegrationTests/RoundTripEquivalenceTests.cs @@ -447,6 +447,11 @@ public sealed class RoundTripEquivalenceTests : IDisposable AuthConfiguration = "{\"apiKey\":\"super-secret\"}", // rides SecretsBlock MaxRetries = 6, RetryDelay = TimeSpan.FromSeconds(17), + // arch-review 2026-08-19 P7-1: a default-0 fixture value made this + // reflection guard vacuous for TimeoutSeconds (comparing default against + // default survives even a serializer that drops the field entirely) — a + // non-default value makes the guard actually exercise the field. + TimeoutSeconds = 120, }; ctx.ExternalSystemDefinitions.Add(sys); await ctx.SaveChangesAsync(); diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/ArtifactDiffTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/ArtifactDiffTests.cs index 3adf39ea..c04874f5 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/ArtifactDiffTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/ArtifactDiffTests.cs @@ -156,6 +156,70 @@ public sealed class ArtifactDiffTests Assert.Equal(ConflictKind.Identical, item.Kind); } + // ============ arch-review 2026-08-19 P7-1: TimeoutSeconds in the diff (row-27 shape) ============ + + /// + /// The bundle has always CARRIED TimeoutSeconds and BundleImporter has always APPLIED it + /// (the per-system HTTP timeout used by ExternalSystemClient), but CompareExternalSystem + /// never compared it — so an import whose ONLY change was a tuned timeout previewed as + /// "Identical" and the operator had no diff evidence the value was about to change. + /// + [Fact] + public void ExternalSystem_ChangedTimeoutSeconds_IsReported() + { + var existing = new ExternalSystemDefinition("Mes", "https://mes", "ApiKey") + { + TimeoutSeconds = 30, + }; + var incoming = new ExternalSystemDto("Mes", "https://mes", "ApiKey", + MaxRetries: 0, RetryDelay: TimeSpan.Zero, Methods: [], Secrets: null, TimeoutSeconds: 120); + + var item = _diff.CompareExternalSystem(incoming, existing, []); + + Assert.Equal(ConflictKind.Modified, item.Kind); + + var timeout = ChangeFor(item, "TimeoutSeconds"); + Assert.Equal("30", timeout.GetProperty("oldValue").GetString()); + Assert.Equal("120", timeout.GetProperty("newValue").GetString()); + } + + /// An unchanged timeout must not manufacture a spurious diff row. + [Fact] + public void ExternalSystem_UnchangedTimeoutSeconds_IsIdentical() + { + var existing = new ExternalSystemDefinition("Mes", "https://mes", "ApiKey") + { + TimeoutSeconds = 30, + }; + var incoming = new ExternalSystemDto("Mes", "https://mes", "ApiKey", + MaxRetries: 0, RetryDelay: TimeSpan.Zero, Methods: [], Secrets: null, TimeoutSeconds: 30); + + var item = _diff.CompareExternalSystem(incoming, existing, []); + + Assert.Equal(ConflictKind.Identical, item.Kind); + } + + /// + /// Old-bundle semantics: a bundle written before this field existed carries + /// TimeoutSeconds: null. The diff must treat that as "not specified by this + /// bundle" — same as BundleImporter's Overwrite fallback — rather than reading it as + /// "reset to 0/unset" and manufacturing a spurious change. + /// + [Fact] + public void ExternalSystem_MissingTimeoutSeconds_ComparesAsUnchanged() + { + var existing = new ExternalSystemDefinition("Mes", "https://mes", "ApiKey") + { + TimeoutSeconds = 30, + }; + var incoming = new ExternalSystemDto("Mes", "https://mes", "ApiKey", + MaxRetries: 0, RetryDelay: TimeSpan.Zero, Methods: [], Secrets: null, TimeoutSeconds: null); + + var item = _diff.CompareExternalSystem(incoming, existing, []); + + Assert.Equal(ConflictKind.Identical, item.Kind); + } + [Fact] public void ApiMethod_ModifiedScript_EmitsStructuredLineDiff() { diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Serialization/BundleDtoSerializationTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Serialization/BundleDtoSerializationTests.cs index 560d8483..6c754063 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Serialization/BundleDtoSerializationTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Serialization/BundleDtoSerializationTests.cs @@ -201,6 +201,52 @@ public sealed class BundleDtoSerializationTests Assert.Empty(content.Instances); } + // --- arch-review 2026-08-19 P7-1: ExternalSystem TimeoutSeconds wire format --- + + /// + /// Pins the old-bundle semantics: a bundle written before TimeoutSeconds + /// existed carries no such property in the JSON text at all (not even a null). It + /// must deserialize to a null DTO value — never to 0, which is a distinct, meaningful + /// wire value (the entity's own "unset, gateway default applies" sentinel) — so + /// BundleImporter can tell "not specified by this bundle" apart from "explicitly + /// unset" and apply the documented fallback (Add: entity default; Overwrite: leave + /// the target's existing value untouched). + /// + [Fact] + public void LegacyExternalSystem_MissingTimeoutSeconds_DeserializesToNull() + { + const string legacyJson = """ + { + "TemplateFolders": [], + "Templates": [], + "SharedScripts": [], + "ExternalSystems": [ + { + "Name": "erp", + "BaseUrl": "https://erp/api", + "AuthType": "ApiKey", + "MaxRetries": 3, + "RetryDelay": "00:00:05", + "Methods": [], + "Secrets": null + } + ], + "DatabaseConnections": [], + "NotificationLists": [], + "SmtpConfigs": [], + "ApiMethods": [] + } + """; + + var content = JsonSerializer.Deserialize(legacyJson, Options); + + Assert.NotNull(content); + var sys = Assert.Single(content!.ExternalSystems); + Assert.Equal("erp", sys.Name); + Assert.Equal(3, sys.MaxRetries); + Assert.Null(sys.TimeoutSeconds); + } + // --- S10: SMS recipient PhoneNumber + SmsConfig wire format --------------- [Fact] diff --git a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Serialization/EntitySerializerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Serialization/EntitySerializerTests.cs index 17907ec3..7e79c92b 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Serialization/EntitySerializerTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Serialization/EntitySerializerTests.cs @@ -340,6 +340,60 @@ public sealed class EntitySerializerTests Assert.Equal(TimeSpan.FromSeconds(15), rtSys.RetryDelay); } + /// + /// arch-review 2026-08-19 P7-1 (row-27 shape): TimeoutSeconds is operative at + /// runtime (the per-system HTTP timeout) but was silently dropped by the export/import + /// DTO — same defect class as the earlier MaxRetries/RetryDelay holes this + /// test's sibling covers. + /// + [Fact] + public void Roundtrip_external_system_preserves_timeout_seconds() + { + var sys = new ExternalSystemDefinition("billing", "https://billing/api", "Basic") + { + Id = 1, + TimeoutSeconds = 120, + }; + var aggregate = MakeEmptyAggregate() with { ExternalSystems = new[] { sys } }; + + var sut = new EntitySerializer(); + var dto = sut.ToBundleContent(aggregate); + Assert.Equal(120, Assert.Single(dto.ExternalSystems).TimeoutSeconds); + + var roundTripped = sut.FromBundleContent(dto); + + var rtSys = Assert.Single(roundTripped.ExternalSystems); + Assert.Equal("billing", rtSys.Name); + Assert.Equal(120, rtSys.TimeoutSeconds); + } + + /// + /// Pins the old-bundle semantics: a DTO with no TimeoutSeconds at all (the + /// property default, as an old bundle deserializes it) falls back to the entity's own + /// "0 = unset" default on import — it must not throw or coerce to some other sentinel. + /// + [Fact] + public void FromDto_with_null_TimeoutSeconds_yields_entity_default_zero() + { + var dto = new BundleContentDto( + TemplateFolders: Array.Empty(), + Templates: Array.Empty(), + SharedScripts: Array.Empty(), + ExternalSystems: new[] + { + new ExternalSystemDto("erp", "https://x", "None", MaxRetries: 3, RetryDelay: TimeSpan.FromSeconds(5), Array.Empty(), Secrets: null), + }, + DatabaseConnections: Array.Empty(), + NotificationLists: Array.Empty(), + SmtpConfigs: Array.Empty(), + ApiMethods: Array.Empty()); + + var aggregate = new EntitySerializer().FromBundleContent(dto); + + var sys = Assert.Single(aggregate.ExternalSystems); + Assert.Equal(0, sys.TimeoutSeconds); + } + [Fact] public void FromDto_with_null_SecretsBlock_yields_entity_with_default_empty_secrets() {