fix(deploy): normalize snapshot List values (Decode→Encode) before staleness/diff (#102); CLI --value native-List help

2026-06-19 02:24:26 -04:00
parent 454e47ea38
commit fb18253f32
5 changed files with 284 additions and 17 deletions
@@ -140,7 +140,7 @@ public static class TemplateCommands
        var templateIdOption = new Option<int>("--template-id") { Description = "Template ID", Required = true };
        var nameOption = new Option<string>("--name") { Description = "Attribute name", Required = true };
        var dataTypeOption = new Option<string>("--data-type") { Description = "Data type", Required = true };
-        var valueOption = new Option<string?>("--value") { Description = "Default value. For a List attribute, supply a JSON array (e.g. '[\"WO-1\",\"WO-2\"]')." };
+        var valueOption = new Option<string?>("--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<string?>("--description") { Description = "Description" };
        var sourceOption = new Option<string?>("--data-source") { Description = "Data source reference" };
        var elementTypeOption = new Option<string?>("--element-type") { Description = ElementTypeOptionDescription };
@@ -183,7 +183,7 @@ public static class TemplateCommands
        var updateIdOption = new Option<int>("--id") { Description = "Attribute ID", Required = true };
        var updateNameOption = new Option<string>("--name") { Description = "Attribute name", Required = true };
        var updateDataTypeOption = new Option<string>("--data-type") { Description = "Data type", Required = true };
-        var updateValueOption = new Option<string?>("--value") { Description = "Default value. For a List attribute, supply a JSON array (e.g. '[\"WO-1\",\"WO-2\"]')." };
+        var updateValueOption = new Option<string?>("--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<string?>("--description") { Description = "Description" };
        var updateSourceOption = new Option<string?>("--data-source") { Description = "Data source reference" };
        var updateElementTypeOption = new Option<string?>("--element-type") { Description = ElementTypeOptionDescription };
@@ -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<DeploymentService> _logger;
@@ -64,6 +65,11 @@ public class DeploymentService
    /// <param name="lockManager">Manager for per-instance operation locks.</param>
    /// <param name="auditService">Service for recording audit log entries.</param>
    /// <param name="diffService">Service for computing configuration diffs.</param>
+    /// <param name="revisionHashService">
+    /// Service for recomputing a flattened configuration's revision hash. Used by
+    /// <see cref="GetDeploymentComparisonAsync"/> to derive the deployed-side
+    /// staleness hash from the (List-normalized) deserialized snapshot — see I-1.
+    /// </param>
    /// <param name="statusNotifier">Notifier for pushing deployment status changes to the UI.</param>
    /// <param name="options">Deployment manager configuration options.</param>
    /// <param name="logger">Logger instance.</param>
@@ -75,6 +81,7 @@ public class DeploymentService
        OperationLockManager lockManager,
        IAuditService auditService,
        DiffService diffService,
+        RevisionHashService revisionHashService,
        IDeploymentStatusNotifier statusNotifier,
        IOptions<DeploymentManagerOptions> options,
        ILogger<DeploymentService> 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<FlattenedConfiguration>(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<DeploymentComparisonResult>.Success(result);
    }

+    /// <summary>
+    /// I-1 (latent): returns a copy of <paramref name="config"/> whose
+    /// <see cref="DataType.List"/> attribute values have been round-tripped through
+    /// <see cref="AttributeValueCodec.Decode"/> → <see cref="AttributeValueCodec.Encode"/>
+    /// (native JSON-array form). This normalizes a value deployed in the OLD quoted
+    /// form (e.g. <c>["10","20"]</c>) to the native form (<c>[10,20]</c>) 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 <see cref="DataType.List"/> 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.
+    /// </summary>
+    private ResolvedAttribute NormalizeListAttribute(ResolvedAttribute attr)
+    {
+        if (!string.Equals(attr.DataType, nameof(DataType.List), StringComparison.OrdinalIgnoreCase)
+            || string.IsNullOrEmpty(attr.Value))
+        {
+            return attr;
+        }
+
+        if (!Enum.TryParse<DataType>(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;
+        }
+    }
+
+    /// <summary>
+    /// I-1 (latent): applies <see cref="NormalizeListAttribute"/> to every attribute
+    /// in <paramref name="config"/>, returning the original instance unchanged when
+    /// no List value needed normalizing (the common scalar-only case).
+    /// </summary>
+    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;
+    }
+
    /// <summary>
    /// WP-2: Returns the current persisted <see cref="DeploymentRecord"/> for
    /// the given deployment ID from the configuration database. This is a pure
@@ -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<ZB.MOM.WW.ScadaBridge.TemplateEngine.Flattening.DiffService>();
+        // 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<ZB.MOM.WW.ScadaBridge.TemplateEngine.Flattening.RevisionHashService>();
        // CentralUI-006: DeploymentService now also depends on the
        // deployment-status notifier (a process singleton in production).
        Services.AddSingleton<ZB.MOM.WW.ScadaBridge.DeploymentManager.IDeploymentStatusNotifier>(
@@ -51,6 +51,7 @@ public class DeploymentServiceTests : TestKit
        _service = new DeploymentService(
            _repo, siteRepo, _pipeline, _comms, _lockManager, _audit,
            new DiffService(),
+            new RevisionHashService(),
            new DeploymentStatusNotifier(NullLogger<DeploymentStatusNotifier>.Instance),
            options,
            NullLogger<DeploymentService>.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<DeploymentStatusNotifier>.Instance),
            Options.Create(new DeploymentManagerOptions { OperationLockTimeout = TimeSpan.FromSeconds(5) }),
            NullLogger<DeploymentService>.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<CancellationToken>())
            .Returns(Result<FlatteningPipelineResult>.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<CancellationToken>())
            .Returns(Result<FlatteningPipelineResult>.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<CancellationToken>()).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<CancellationToken>())
+            .Returns(Result<FlatteningPipelineResult>.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<CancellationToken>()).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<CancellationToken>())
+            .Returns(Result<FlatteningPipelineResult>.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<DeploymentStatusNotifier>.Instance),
            Options.Create(new DeploymentManagerOptions { OperationLockTimeout = TimeSpan.FromSeconds(5) }),
            NullLogger<DeploymentService>.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<DeploymentStatusNotifier>.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<DeploymentStatusNotifier>.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<DeploymentStatusNotifier>.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<DeploymentStatusNotifier>.Instance),
            Options.Create(new DeploymentManagerOptions
            {
@@ -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<DeploymentService>.Instance);
    }