diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/TemplateCommands.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/TemplateCommands.cs index d5f18681..6d282d97 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/TemplateCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/TemplateCommands.cs @@ -140,7 +140,7 @@ public static class TemplateCommands var templateIdOption = new Option("--template-id") { Description = "Template ID", Required = true }; var nameOption = new Option("--name") { Description = "Attribute name", Required = true }; var dataTypeOption = new Option("--data-type") { Description = "Data type", Required = true }; - var valueOption = new Option("--value") { Description = "Default value. For a List attribute, supply a JSON array (e.g. '[\"WO-1\",\"WO-2\"]')." }; + var valueOption = new Option("--value") { Description = "Default value. For a List attribute, supply a JSON array in native form: numeric/boolean elements unquoted (e.g. an Int32 list '[10,20,30]'), string elements quoted (e.g. '[\"WO-1\",\"WO-2\"]')." }; var descOption = new Option("--description") { Description = "Description" }; var sourceOption = new Option("--data-source") { Description = "Data source reference" }; var elementTypeOption = new Option("--element-type") { Description = ElementTypeOptionDescription }; @@ -183,7 +183,7 @@ public static class TemplateCommands var updateIdOption = new Option("--id") { Description = "Attribute ID", Required = true }; var updateNameOption = new Option("--name") { Description = "Attribute name", Required = true }; var updateDataTypeOption = new Option("--data-type") { Description = "Data type", Required = true }; - var updateValueOption = new Option("--value") { Description = "Default value. For a List attribute, supply a JSON array (e.g. '[\"WO-1\",\"WO-2\"]')." }; + var updateValueOption = new Option("--value") { Description = "Default value. For a List attribute, supply a JSON array in native form: numeric/boolean elements unquoted (e.g. an Int32 list '[10,20,30]'), string elements quoted (e.g. '[\"WO-1\",\"WO-2\"]')." }; var updateDescOption = new Option("--description") { Description = "Description" }; var updateSourceOption = new Option("--data-source") { Description = "Data source reference" }; var updateElementTypeOption = new Option("--element-type") { Description = ElementTypeOptionDescription }; diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs index 67958423..32bf9731 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs @@ -42,6 +42,7 @@ public class DeploymentService private readonly OperationLockManager _lockManager; private readonly IAuditService _auditService; private readonly DiffService _diffService; + private readonly RevisionHashService _revisionHashService; private readonly IDeploymentStatusNotifier _statusNotifier; private readonly DeploymentManagerOptions _options; private readonly ILogger _logger; @@ -64,6 +65,11 @@ public class DeploymentService /// Manager for per-instance operation locks. /// Service for recording audit log entries. /// Service for computing configuration diffs. + /// + /// Service for recomputing a flattened configuration's revision hash. Used by + /// to derive the deployed-side + /// staleness hash from the (List-normalized) deserialized snapshot — see I-1. + /// /// Notifier for pushing deployment status changes to the UI. /// Deployment manager configuration options. /// Logger instance. @@ -75,6 +81,7 @@ public class DeploymentService OperationLockManager lockManager, IAuditService auditService, DiffService diffService, + RevisionHashService revisionHashService, IDeploymentStatusNotifier statusNotifier, IOptions options, ILogger logger) @@ -86,6 +93,7 @@ public class DeploymentService _lockManager = lockManager; _auditService = auditService; _diffService = diffService; + _revisionHashService = revisionHashService; _statusNotifier = statusNotifier; _options = options.Value; _logger = logger; @@ -588,21 +596,41 @@ public class DeploymentService var currentConfig = currentResult.Value.Configuration; var currentHash = currentResult.Value.RevisionHash; - var isStale = snapshot.RevisionHash != currentHash; - // DeploymentManager-007: deserialize the deployed snapshot and run the - // TemplateEngine DiffService so the result carries real - // added/removed/changed detail, not just a hash comparison. A snapshot - // that cannot be deserialized (corrupt / older schema) still yields the - // hash-based staleness result, with a null diff. + // I-1 (latent): the snapshot's ConfigurationJson + RevisionHash froze the + // FLATTENED config at deploy time. The current config is a FRESH flatten, + // now always in native List form (#93 consolidated element-type/coercion + // into AttributeValueCodec, which emits native-form JSON arrays). A List + // attribute deployed in the OLD quoted form (e.g. ["10","20"]) therefore + // both (a) hashes differently from the native re-flatten — a spurious + // stale flag — and (b) shows a spurious Changed attribute in the diff + // (DiffService.AttributesEqual is an ordinal Value comparison). Normalize + // the deserialized snapshot's List values through AttributeValueCodec + // Decode→Encode so an old-form value becomes native form and compares + // equal to the native re-flatten, then drive BOTH the staleness hash and + // the diff off that normalized snapshot. Scalars are left untouched. + // + // DeploymentManager-007: a snapshot that cannot be deserialized (corrupt / + // older schema) still yields the frozen-hash staleness result, with a + // null diff. + var deployedRevisionHash = snapshot.RevisionHash; ConfigurationDiff? diff = null; try { var deployedConfig = JsonSerializer.Deserialize(snapshot.ConfigurationJson); if (deployedConfig != null) { + deployedConfig = NormalizeListAttributeValues(deployedConfig); + + // Recompute the deployed-side hash from the normalized snapshot so + // an old-form List value is not flagged stale against the native + // re-flatten. For a faithfully-stored scalar-only snapshot this + // reproduces the frozen RevisionHash exactly, so behaviour is + // unchanged outside the List-normalization case. + deployedRevisionHash = _revisionHashService.ComputeHash(deployedConfig); + diff = _diffService.ComputeDiff( - deployedConfig, currentConfig, snapshot.RevisionHash, currentHash); + deployedConfig, currentConfig, deployedRevisionHash, currentHash); } else { @@ -620,9 +648,11 @@ public class DeploymentService instanceId); } + var isStale = deployedRevisionHash != currentHash; + var result = new DeploymentComparisonResult( instanceId, - snapshot.RevisionHash, + deployedRevisionHash, currentHash, isStale, snapshot.DeployedAt, @@ -631,6 +661,66 @@ public class DeploymentService return Result.Success(result); } + /// + /// I-1 (latent): returns a copy of whose + /// attribute values have been round-tripped through + /// → + /// (native JSON-array form). This normalizes a value deployed in the OLD quoted + /// form (e.g. ["10","20"]) to the native form ([10,20]) the current + /// flattener now produces, so the staleness hash and the structured diff do not + /// report a spurious change. Scalar / string attributes are returned unchanged + /// (only is normalized). A value that cannot be + /// decoded (malformed JSON, bad element, or an unparseable element type) is left + /// as-is — a normalization failure must never break the read-only comparison. + /// + private ResolvedAttribute NormalizeListAttribute(ResolvedAttribute attr) + { + if (!string.Equals(attr.DataType, nameof(DataType.List), StringComparison.OrdinalIgnoreCase) + || string.IsNullOrEmpty(attr.Value)) + { + return attr; + } + + if (!Enum.TryParse(attr.ElementDataType, ignoreCase: true, out var elementType) + || !AttributeValueCodec.IsValidElementType(elementType)) + { + return attr; + } + + try + { + var normalized = AttributeValueCodec.Encode( + AttributeValueCodec.Decode(attr.Value, DataType.List, elementType)); + return normalized == attr.Value ? attr : attr with { Value = normalized }; + } + catch (FormatException ex) + { + // Best-effort: a snapshot value that no longer round-trips is left + // untouched rather than aborting the comparison. Logged so an operator + // can investigate the stored value. + _logger.LogWarning(ex, + "Could not normalize List attribute '{Attribute}' in deployed snapshot; " + + "comparing its stored value verbatim", + attr.CanonicalName); + return attr; + } + } + + /// + /// I-1 (latent): applies to every attribute + /// in , returning the original instance unchanged when + /// no List value needed normalizing (the common scalar-only case). + /// + private FlattenedConfiguration NormalizeListAttributeValues(FlattenedConfiguration config) + { + if (config.Attributes.Count == 0) + return config; + + var normalized = config.Attributes.Select(NormalizeListAttribute).ToList(); + var changed = normalized.Where((a, i) => !ReferenceEquals(a, config.Attributes[i])).Any(); + return changed ? config with { Attributes = normalized } : config; + } + /// /// WP-2: Returns the current persisted for /// the given deployment ID from the configuration database. This is a pure diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/TopologyPageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/TopologyPageTests.cs index f4176b4f..756fb69e 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/TopologyPageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/TopologyPageTests.cs @@ -62,6 +62,10 @@ public class TopologyPageTests : BunitContext // DeploymentService gained a DiffService dependency (DeploymentManager // contract change); register it so the page's DI graph resolves. Services.AddScoped(); + // I-1: DeploymentService also recomputes the deployed-side staleness hash + // from the (List-normalized) snapshot via RevisionHashService; register it + // so the page's DI graph resolves. + Services.AddScoped(); // CentralUI-006: DeploymentService now also depends on the // deployment-status notifier (a process singleton in production). Services.AddSingleton( diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs index f3902651..c6331a8b 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs @@ -51,6 +51,7 @@ public class DeploymentServiceTests : TestKit _service = new DeploymentService( _repo, siteRepo, _pipeline, _comms, _lockManager, _audit, new DiffService(), + new RevisionHashService(), new DeploymentStatusNotifier(NullLogger.Instance), options, NullLogger.Instance); @@ -135,6 +136,7 @@ public class DeploymentServiceTests : TestKit var service = new DeploymentService( _repo, siteRepo, _pipeline, _comms, _lockManager, _audit, new DiffService(), + new RevisionHashService(), new DeploymentStatusNotifier(NullLogger.Instance), Options.Create(new DeploymentManagerOptions { OperationLockTimeout = TimeSpan.FromSeconds(5) }), NullLogger.Instance); @@ -413,17 +415,30 @@ public class DeploymentServiceTests : TestKit [Fact] public async Task GetDeploymentComparisonAsync_SameHash_NotStale() { - var snapshot = new DeployedConfigSnapshot("dep1", "sha256:abc", "{}") + // I-1: the deployed-side staleness hash is now recomputed from the + // (List-normalized) deserialized snapshot, so the snapshot's stored + // ConfigurationJson and the current re-flatten must describe the SAME + // config to be not-stale. Use a faithfully-serialized config on both + // sides; recompute the current hash with the real RevisionHashService so + // the recomputed deployed hash equals it. + var deployedConfig = new FlattenedConfiguration + { + InstanceUniqueName = "TestInst", + Attributes = [new ResolvedAttribute { CanonicalName = "Setpoint", Value = "5", DataType = "Int32" }] + }; + var sameHash = new RevisionHashService().ComputeHash(deployedConfig); + var snapshot = new DeployedConfigSnapshot( + "dep1", sameHash, System.Text.Json.JsonSerializer.Serialize(deployedConfig)) { InstanceId = 1, DeployedAt = DateTimeOffset.UtcNow }; _repo.GetDeployedSnapshotByInstanceIdAsync(1).Returns(snapshot); - var config = new FlattenedConfiguration { InstanceUniqueName = "TestInst" }; + var currentConfig = deployedConfig with { }; _pipeline.FlattenAndValidateAsync(1, Arg.Any()) .Returns(Result.Success( - new FlatteningPipelineResult(config, "sha256:abc", ValidationResult.Success()))); + new FlatteningPipelineResult(currentConfig, sameHash, ValidationResult.Success()))); var result = await _service.GetDeploymentComparisonAsync(1); @@ -434,17 +449,33 @@ public class DeploymentServiceTests : TestKit [Fact] public async Task GetDeploymentComparisonAsync_DifferentHash_IsStale() { - var snapshot = new DeployedConfigSnapshot("dep1", "sha256:abc", "{}") + // I-1: a genuinely different current config still reports stale. The + // deployed-side hash is recomputed from the snapshot; the current hash is + // the (different) re-flatten hash, so the two differ. + var deployedConfig = new FlattenedConfiguration + { + InstanceUniqueName = "TestInst", + Attributes = [new ResolvedAttribute { CanonicalName = "Setpoint", Value = "5", DataType = "Int32" }] + }; + var deployedHash = new RevisionHashService().ComputeHash(deployedConfig); + var snapshot = new DeployedConfigSnapshot( + "dep1", deployedHash, System.Text.Json.JsonSerializer.Serialize(deployedConfig)) { InstanceId = 1, DeployedAt = DateTimeOffset.UtcNow }; _repo.GetDeployedSnapshotByInstanceIdAsync(1).Returns(snapshot); - var config = new FlattenedConfiguration { InstanceUniqueName = "TestInst" }; + // The current re-flatten changed the attribute value. + var currentConfig = new FlattenedConfiguration + { + InstanceUniqueName = "TestInst", + Attributes = [new ResolvedAttribute { CanonicalName = "Setpoint", Value = "9", DataType = "Int32" }] + }; + var currentHash = new RevisionHashService().ComputeHash(currentConfig); _pipeline.FlattenAndValidateAsync(1, Arg.Any()) .Returns(Result.Success( - new FlatteningPipelineResult(config, "sha256:xyz", ValidationResult.Success()))); + new FlatteningPipelineResult(currentConfig, currentHash, ValidationResult.Success()))); var result = await _service.GetDeploymentComparisonAsync(1); @@ -497,6 +528,143 @@ public class DeploymentServiceTests : TestKit c => c.CanonicalName == "OldAttr" && c.ChangeType == DiffChangeType.Removed); } + // ── I-1 (latent): old-form List snapshot must not show a spurious stale/diff ── + + [Fact] + public async Task GetDeploymentComparisonAsync_OldFormListValue_NormalizedToNative_NotStaleNoDiff() + { + // I-1 regression: a List attribute deployed in the OLD quoted form + // (e.g. ["10","20","30"]) was frozen into the snapshot with a RevisionHash + // computed over that old form. The current flatten emits the SAME list in + // native form ([10,20,30]) with a different hash — so a naive comparison + // would report a spurious stale flag AND a spurious Changed attribute. + // After the fix the deserialized snapshot's List value is normalized via + // AttributeValueCodec Decode→Encode before BOTH the staleness hash and the + // diff, so the comparison reports equal. + var hasher = new RevisionHashService(); + + var oldFormConfig = new FlattenedConfiguration + { + InstanceUniqueName = "ListInst", + Attributes = + [ + new ResolvedAttribute + { + CanonicalName = "Thresholds", + DataType = "List", + ElementDataType = "Int32", + Value = "[\"10\",\"20\",\"30\"]" // OLD quoted form + } + ] + }; + // The frozen hash reflects the OLD form — it differs from the native hash, + // which is exactly what would have produced the spurious stale flag. + var frozenOldFormHash = hasher.ComputeHash(oldFormConfig); + var snapshot = new DeployedConfigSnapshot( + "dep1", frozenOldFormHash, System.Text.Json.JsonSerializer.Serialize(oldFormConfig)) + { + InstanceId = 50, + DeployedAt = DateTimeOffset.UtcNow + }; + _repo.GetDeployedSnapshotByInstanceIdAsync(50, Arg.Any()).Returns(snapshot); + + // The current re-flatten carries the SAME list in native form. + var nativeConfig = new FlattenedConfiguration + { + InstanceUniqueName = "ListInst", + Attributes = + [ + new ResolvedAttribute + { + CanonicalName = "Thresholds", + DataType = "List", + ElementDataType = "Int32", + Value = "[10,20,30]" // native form + } + ] + }; + var nativeHash = hasher.ComputeHash(nativeConfig); + _pipeline.FlattenAndValidateAsync(50, Arg.Any()) + .Returns(Result.Success( + new FlatteningPipelineResult(nativeConfig, nativeHash, ValidationResult.Success()))); + + // Sanity: the OLD-form frozen hash genuinely differs from the native hash — + // without normalization the comparison WOULD be spuriously stale. + Assert.NotEqual(frozenOldFormHash, nativeHash); + + var result = await _service.GetDeploymentComparisonAsync(50); + + Assert.True(result.IsSuccess); + // Normalization collapses old-form → native: not stale, no attribute diff. + Assert.False(result.Value.IsStale); + Assert.NotNull(result.Value.Diff); + Assert.DoesNotContain(result.Value.Diff!.AttributeChanges, + c => c.CanonicalName == "Thresholds"); + Assert.False(result.Value.Diff.HasChanges); + // The recomputed deployed hash equals the native current hash. + Assert.Equal(nativeHash, result.Value.DeployedRevisionHash); + } + + [Fact] + public async Task GetDeploymentComparisonAsync_GenuinelyChangedListValue_StillStaleAndDiffs() + { + // I-1 negative: a List value that genuinely changed (not just a form + // difference) must still report stale and a Changed attribute. The + // deployed snapshot is native [10,20,30]; the current re-flatten is + // [10,20,40] — normalization does not mask a real change. + var hasher = new RevisionHashService(); + + var deployedConfig = new FlattenedConfiguration + { + InstanceUniqueName = "ListInst", + Attributes = + [ + new ResolvedAttribute + { + CanonicalName = "Thresholds", + DataType = "List", + ElementDataType = "Int32", + Value = "[10,20,30]" + } + ] + }; + var deployedHash = hasher.ComputeHash(deployedConfig); + var snapshot = new DeployedConfigSnapshot( + "dep1", deployedHash, System.Text.Json.JsonSerializer.Serialize(deployedConfig)) + { + InstanceId = 51, + DeployedAt = DateTimeOffset.UtcNow + }; + _repo.GetDeployedSnapshotByInstanceIdAsync(51, Arg.Any()).Returns(snapshot); + + var currentConfig = new FlattenedConfiguration + { + InstanceUniqueName = "ListInst", + Attributes = + [ + new ResolvedAttribute + { + CanonicalName = "Thresholds", + DataType = "List", + ElementDataType = "Int32", + Value = "[10,20,40]" // genuinely different element + } + ] + }; + var currentHash = hasher.ComputeHash(currentConfig); + _pipeline.FlattenAndValidateAsync(51, Arg.Any()) + .Returns(Result.Success( + new FlatteningPipelineResult(currentConfig, currentHash, ValidationResult.Success()))); + + var result = await _service.GetDeploymentComparisonAsync(51); + + Assert.True(result.IsSuccess); + Assert.True(result.Value.IsStale); + Assert.NotNull(result.Value.Diff); + Assert.Contains(result.Value.Diff!.AttributeChanges, + c => c.CanonicalName == "Thresholds" && c.ChangeType == DiffChangeType.Changed); + } + // ── WP-2: GetDeploymentStatusAsync ── [Fact] @@ -773,6 +941,7 @@ public class DeploymentServiceTests : TestKit return new DeploymentService( _repo, siteRepo, _pipeline, comms, _lockManager, _audit, new DiffService(), + new RevisionHashService(), new DeploymentStatusNotifier(NullLogger.Instance), Options.Create(new DeploymentManagerOptions { OperationLockTimeout = TimeSpan.FromSeconds(5) }), NullLogger.Instance); @@ -1210,6 +1379,7 @@ public class DeploymentServiceTests : TestKit var service = new DeploymentService( _repo, siteRepo, _pipeline, comms, _lockManager, _audit, new DiffService(), + new RevisionHashService(), new DeploymentStatusNotifier(NullLogger.Instance), Options.Create(new DeploymentManagerOptions { @@ -1266,6 +1436,7 @@ public class DeploymentServiceTests : TestKit var service = new DeploymentService( _repo, siteRepo, _pipeline, comms, _lockManager, _audit, new DiffService(), + new RevisionHashService(), new DeploymentStatusNotifier(NullLogger.Instance), Options.Create(new DeploymentManagerOptions { @@ -1314,6 +1485,7 @@ public class DeploymentServiceTests : TestKit var service = new DeploymentService( _repo, siteRepo, _pipeline, comms, _lockManager, _audit, new DiffService(), + new RevisionHashService(), new DeploymentStatusNotifier(NullLogger.Instance), Options.Create(new DeploymentManagerOptions { @@ -1359,6 +1531,7 @@ public class DeploymentServiceTests : TestKit var service = new DeploymentService( _repo, siteRepo, _pipeline, comms, _lockManager, _audit, new DiffService(), + new RevisionHashService(), new DeploymentStatusNotifier(NullLogger.Instance), Options.Create(new DeploymentManagerOptions { diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentStatusNotifierTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentStatusNotifierTests.cs index 15edc7d4..52d82f9a 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentStatusNotifierTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentStatusNotifierTests.cs @@ -60,7 +60,7 @@ public class DeploymentStatusNotifierTests : TestKit _service = new DeploymentService( _repo, siteRepo, _pipeline, _comms, _lockManager, _audit, - new DiffService(), _notifier, options, + new DiffService(), new RevisionHashService(), _notifier, options, NullLogger.Instance); }