refactor(transport): single-source template child equality — diff and overwrite-sync can no longer drift

New TemplateChildEquality is the one place "has this template child changed?" is decided, comparing the COMPLETE writable field set per entity (attributes now incl. ElementDataType + LockedInDerived; alarms incl. the on-trigger script by name + LockedInDerived; scripts incl. MinTimeBetweenRuns/ExecutionTimeoutSeconds + LockedInDerived; native sources per T5). ArtifactDiff.CompareTemplate and all four BundleImporter.SyncTemplate*Async predicates now route through it — closing the class of bug where Preview reported Identical for an artifact an Overwrite would mutate. The attribute sync passes a value-normalised DTO so List-value re-imports stay idempotent; the alarm on-trigger change now surfaces in both diff and sync. Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
2026-07-09 17:17:18 -04:00
parent 33cf577718
commit 38edfefc42
4 changed files with 228 additions and 73 deletions
@@ -64,16 +64,23 @@ public sealed class ArtifactDiff
        AddIfDifferent(changes, "FolderName", FolderNameOf(existing), incoming.FolderName);
        AddIfDifferent(changes, "BaseTemplateName", BaseTemplateNameOf(existing), incoming.BaseTemplateName);

+        // Bundles reference the alarm on-trigger script by name; resolve the
+        // persisted FK back to a name over this template's own scripts so the
+        // shared equality can compare it. See TemplateChildEquality.
+        var scriptNameById = TemplateChildEquality.ScriptNameResolver(existing.Scripts);
+
        // Children: compare each child collection by name. We track which names
        // were added, which were removed, and which existed on both sides but
        // diverged in body. We use coarse value equality / line counts for
-        // scripts so the diff JSON stays under a few KB per item.
+        // scripts so the diff JSON stays under a few KB per item. Change
+        // detection is single-sourced through TemplateChildEquality so the diff
+        // can never disagree with what an Overwrite sync writes (#05-T6).
        DiffChildren(
            existing.Attributes,
            incoming.Attributes,
            e => e.Name,
            i => i.Name,
-            AttributesEqual,
+            TemplateChildEquality.AttributesEqual,
            "Attributes",
            changes);

@@ -82,7 +89,7 @@ public sealed class ArtifactDiff
            incoming.Alarms,
            e => e.Name,
            i => i.Name,
-            AlarmsEqual,
+            (e, i) => TemplateChildEquality.AlarmsEqual(e, i, scriptNameById),
            "Alarms",
            changes);

@@ -106,7 +113,7 @@ public sealed class ArtifactDiff
            incoming.NativeAlarmSources,
            e => e.Name,
            i => i.Name,
-            NativeAlarmSourcesEqual,
+            TemplateChildEquality.NativeAlarmSourcesEqual,
            "NativeAlarmSources",
            changes);

@@ -641,7 +648,7 @@ public sealed class ArtifactDiff
        foreach (var (name, ex) in existingByName)
        {
            if (!incomingByName.TryGetValue(name, out var inc)) continue;
-            if (!ScriptsEqual(ex, inc))
+            if (!TemplateChildEquality.ScriptsEqual(ex, inc))
            {
                // A script row can diverge in Code and/or its trigger/param metadata.
                // The structured line diff covers the Code body; when only the
@@ -652,37 +659,6 @@ public sealed class ArtifactDiff
        }
    }

-    private static bool AttributesEqual(TemplateAttribute e, TemplateAttributeDto i) =>
-        e.Value == i.Value
-        && e.DataType == i.DataType
-        && e.IsLocked == i.IsLocked
-        && e.Description == i.Description
-        && e.DataSourceReference == i.DataSourceReference;
-
-    private static bool AlarmsEqual(TemplateAlarm e, TemplateAlarmDto i) =>
-        e.Description == i.Description
-        && e.PriorityLevel == i.PriorityLevel
-        && e.TriggerType == i.TriggerType
-        && e.TriggerConfiguration == i.TriggerConfiguration
-        && e.IsLocked == i.IsLocked;
-
-    private static bool ScriptsEqual(TemplateScript e, TemplateScriptDto i) =>
-        string.Equals(e.Code, i.Code, StringComparison.Ordinal)
-        && e.TriggerType == i.TriggerType
-        && e.TriggerConfiguration == i.TriggerConfiguration
-        && e.ParameterDefinitions == i.ParameterDefinitions
-        && e.ReturnDefinition == i.ReturnDefinition
-        && e.IsLocked == i.IsLocked;
-
-    private static bool NativeAlarmSourcesEqual(TemplateNativeAlarmSource e, TemplateNativeAlarmSourceDto i) =>
-        e.Description == i.Description
-        && e.ConnectionName == i.ConnectionName
-        && e.SourceReference == i.SourceReference
-        && e.ConditionFilter == i.ConditionFilter
-        && e.IsLocked == i.IsLocked
-        && e.IsInherited == i.IsInherited
-        && e.LockedInDerived == i.LockedInDerived;
-
    private static bool ExternalSystemMethodsEqual(ExternalSystemMethod e, ExternalSystemMethodDto i) =>
        e.HttpMethod == i.HttpMethod
        && e.Path == i.Path
@@ -1645,15 +1645,12 @@ public sealed class BundleImporter : IBundleImporter
                attrDto.Value, attrDto.DataType, attrDto.ElementDataType, _logger, attrDto.Name);
            if (existingByName.TryGetValue(attrDto.Name, out var current))
            {
-                // Update only if any field actually changed.
-                bool changed =
-                    !string.Equals(current.Value, normalizedValue, StringComparison.Ordinal) ||
-                    current.DataType != attrDto.DataType ||
-                    current.IsLocked != attrDto.IsLocked ||
-                    !string.Equals(current.Description, attrDto.Description, StringComparison.Ordinal) ||
-                    !string.Equals(current.DataSourceReference, attrDto.DataSourceReference, StringComparison.Ordinal) ||
-                    current.ElementDataType != attrDto.ElementDataType ||
-                    current.LockedInDerived != attrDto.LockedInDerived;
+                // Update only if any field actually changed — single-sourced through
+                // TemplateChildEquality (#05-T6). Compare against the DTO with its
+                // List value already normalised so an idempotent re-import of an
+                // old-form bundle doesn't spuriously report a Value change.
+                bool changed = !TemplateChildEquality.AttributesEqual(
+                    current, attrDto with { Value = normalizedValue });
                if (!changed) continue;

                current.Value = normalizedValue;
@@ -1738,6 +1735,13 @@ public sealed class BundleImporter : IBundleImporter
        var existingByName = ex.Alarms.ToDictionary(a => a.Name, a => a, StringComparer.Ordinal);
        var dtoByName = dto.Alarms.ToDictionary(a => a.Name, a => a, StringComparer.Ordinal);

+        // On-trigger script is referenced by name in the bundle; resolve the
+        // persisted FK back to a name over this template's scripts so the shared
+        // equality can compare it (#05-T6). Scripts are synced after alarms, so
+        // this reflects the pre-sync (current) script set — which is exactly the
+        // set current.OnTriggerScriptId points into.
+        var scriptNameById = TemplateChildEquality.ScriptNameResolver(ex.Scripts);
+
        foreach (var existing in existingByName.Values.ToList())
        {
            if (dtoByName.ContainsKey(existing.Name)) continue;
@@ -1757,13 +1761,9 @@ public sealed class BundleImporter : IBundleImporter
        {
            if (existingByName.TryGetValue(alarmDto.Name, out var current))
            {
-                bool changed =
-                    !string.Equals(current.Description, alarmDto.Description, StringComparison.Ordinal) ||
-                    current.PriorityLevel != alarmDto.PriorityLevel ||
-                    current.TriggerType != alarmDto.TriggerType ||
-                    !string.Equals(current.TriggerConfiguration, alarmDto.TriggerConfiguration, StringComparison.Ordinal) ||
-                    current.IsLocked != alarmDto.IsLocked ||
-                    current.LockedInDerived != alarmDto.LockedInDerived;
+                // Single-sourced through TemplateChildEquality (#05-T6) — includes
+                // the on-trigger script binding, which the diff also compares.
+                bool changed = !TemplateChildEquality.AlarmsEqual(current, alarmDto, scriptNameById);
                if (!changed)
                {
                    // Always reset the script FK on Overwrite so the post-flush
@@ -1877,16 +1877,8 @@ public sealed class BundleImporter : IBundleImporter
        {
            if (existingByName.TryGetValue(scriptDto.Name, out var current))
            {
-                bool changed =
-                    !string.Equals(current.Code, scriptDto.Code, StringComparison.Ordinal) ||
-                    !string.Equals(current.TriggerType, scriptDto.TriggerType, StringComparison.Ordinal) ||
-                    !string.Equals(current.TriggerConfiguration, scriptDto.TriggerConfiguration, StringComparison.Ordinal) ||
-                    !string.Equals(current.ParameterDefinitions, scriptDto.ParameterDefinitions, StringComparison.Ordinal) ||
-                    !string.Equals(current.ReturnDefinition, scriptDto.ReturnDefinition, StringComparison.Ordinal) ||
-                    current.IsLocked != scriptDto.IsLocked ||
-                    current.MinTimeBetweenRuns != scriptDto.MinTimeBetweenRuns ||
-                    current.ExecutionTimeoutSeconds != scriptDto.ExecutionTimeoutSeconds ||
-                    current.LockedInDerived != scriptDto.LockedInDerived;
+                // Single-sourced through TemplateChildEquality (#05-T6).
+                bool changed = !TemplateChildEquality.ScriptsEqual(current, scriptDto);
                if (!changed) continue;

                current.Code = scriptDto.Code;
@@ -1993,14 +1985,8 @@ public sealed class BundleImporter : IBundleImporter
        {
            if (existingByName.TryGetValue(srcDto.Name, out var current))
            {
-                bool changed =
-                    !string.Equals(current.Description, srcDto.Description, StringComparison.Ordinal) ||
-                    !string.Equals(current.ConnectionName, srcDto.ConnectionName, StringComparison.Ordinal) ||
-                    !string.Equals(current.SourceReference, srcDto.SourceReference, StringComparison.Ordinal) ||
-                    !string.Equals(current.ConditionFilter, srcDto.ConditionFilter, StringComparison.Ordinal) ||
-                    current.IsLocked != srcDto.IsLocked ||
-                    current.IsInherited != srcDto.IsInherited ||
-                    current.LockedInDerived != srcDto.LockedInDerived;
+                // Single-sourced through TemplateChildEquality (#05-T6).
+                bool changed = !TemplateChildEquality.NativeAlarmSourcesEqual(current, srcDto);
                if (!changed) continue;

                current.Description = srcDto.Description;
@@ -0,0 +1,104 @@
+using ZB.MOM.WW.ScadaBridge.Commons.Entities.Templates;
+using ZB.MOM.WW.ScadaBridge.Transport.Serialization;
+
+namespace ZB.MOM.WW.ScadaBridge.Transport.Import;
+
+/// <summary>
+/// #05-T6 — the single source of truth for "has this template child changed?": the
+/// field-for-field equality between a persisted template-child entity and its bundle
+/// DTO. Every writable field is compared.
+/// <para>
+/// BOTH the Preview diff (<see cref="ArtifactDiff.CompareTemplate"/>) and the
+/// Overwrite child-sync (<c>BundleImporter.SyncTemplate*Async</c>) MUST route their
+/// change detection through these methods so a Preview can never disagree with what
+/// an Overwrite actually writes. Historically the two drifted — the diff omitted
+/// <c>ElementDataType</c>, <c>ExecutionTimeoutSeconds</c>, the alarm on-trigger
+/// script, and <c>LockedInDerived</c> — reporting <c>Identical</c> for artifacts an
+/// Overwrite would in fact mutate. Adding a writable field to any template-child
+/// entity means adding it here, once.
+/// </para>
+/// </summary>
+internal static class TemplateChildEquality
+{
+    /// <summary>
+    /// True when the persisted attribute equals its bundle DTO across every writable
+    /// field. The single source of truth for "attribute changed"; both ArtifactDiff
+    /// and <c>SyncTemplateAttributesAsync</c> call it.
+    /// <para>
+    /// Callers that normalise the DTO <c>Value</c> before comparing (the importer
+    /// normalises List values) pass a DTO whose <c>Value</c> is already normalised.
+    /// </para>
+    /// </summary>
+    public static bool AttributesEqual(TemplateAttribute e, TemplateAttributeDto i) =>
+        e.Value == i.Value
+        && e.DataType == i.DataType
+        && e.ElementDataType == i.ElementDataType
+        && e.IsLocked == i.IsLocked
+        && e.LockedInDerived == i.LockedInDerived
+        && e.Description == i.Description
+        && e.DataSourceReference == i.DataSourceReference;
+
+    /// <summary>
+    /// True when the persisted alarm equals its bundle DTO across every writable
+    /// field, including the on-trigger script binding. Bundles reference the
+    /// on-trigger script by NAME (ids are environment-specific), so the caller
+    /// supplies <paramref name="scriptNameById"/> — a resolver over the alarm's own
+    /// template scripts — to map the persisted <c>OnTriggerScriptId</c> FK back to a
+    /// name for comparison. The single source of truth for "alarm changed"; both
+    /// ArtifactDiff and <c>SyncTemplateAlarmsAsync</c> call it.
+    /// </summary>
+    public static bool AlarmsEqual(TemplateAlarm e, TemplateAlarmDto i, Func<int?, string?> scriptNameById) =>
+        e.Description == i.Description
+        && e.PriorityLevel == i.PriorityLevel
+        && e.TriggerType == i.TriggerType
+        && e.TriggerConfiguration == i.TriggerConfiguration
+        && e.IsLocked == i.IsLocked
+        && e.LockedInDerived == i.LockedInDerived
+        && scriptNameById(e.OnTriggerScriptId) == i.OnTriggerScriptName;
+
+    /// <summary>
+    /// True when the persisted script equals its bundle DTO across every writable
+    /// field. The single source of truth for "script changed"; both ArtifactDiff
+    /// (via <c>DiffScriptChildren</c>) and <c>SyncTemplateScriptsAsync</c> call it.
+    /// </summary>
+    public static bool ScriptsEqual(TemplateScript e, TemplateScriptDto i) =>
+        string.Equals(e.Code, i.Code, StringComparison.Ordinal)
+        && e.TriggerType == i.TriggerType
+        && e.TriggerConfiguration == i.TriggerConfiguration
+        && e.ParameterDefinitions == i.ParameterDefinitions
+        && e.ReturnDefinition == i.ReturnDefinition
+        && e.IsLocked == i.IsLocked
+        && e.LockedInDerived == i.LockedInDerived
+        && e.MinTimeBetweenRuns == i.MinTimeBetweenRuns
+        && e.ExecutionTimeoutSeconds == i.ExecutionTimeoutSeconds;
+
+    /// <summary>
+    /// True when the persisted native alarm source equals its bundle DTO across every
+    /// writable field. The single source of truth for "native alarm source changed";
+    /// both ArtifactDiff and <c>SyncTemplateNativeAlarmSourcesAsync</c> call it.
+    /// </summary>
+    public static bool NativeAlarmSourcesEqual(TemplateNativeAlarmSource e, TemplateNativeAlarmSourceDto i) =>
+        e.Description == i.Description
+        && e.ConnectionName == i.ConnectionName
+        && e.SourceReference == i.SourceReference
+        && e.ConditionFilter == i.ConditionFilter
+        && e.IsLocked == i.IsLocked
+        && e.IsInherited == i.IsInherited
+        && e.LockedInDerived == i.LockedInDerived;
+
+    /// <summary>
+    /// Builds a resolver mapping a template script id → its name over the given
+    /// scripts, for the alarm on-trigger comparison. Returns null for a null id or an
+    /// id absent from the set.
+    /// </summary>
+    public static Func<int?, string?> ScriptNameResolver(IEnumerable<TemplateScript> scripts)
+    {
+        var byId = new Dictionary<int, string>();
+        foreach (var s in scripts)
+        {
+            byId[s.Id] = s.Name;
+        }
+
+        return id => id is { } sid && byId.TryGetValue(sid, out var name) ? name : null;
+    }
+}
@@ -150,6 +150,91 @@ public sealed class ArtifactDiffTests
        Assert.Contains(("add", "new2"), hunks);
    }

+    // ============ #05-T6: single-source child equality — diff sees the full field set ============
+
+    [Fact]
+    public void CompareTemplate_AttributeElementDataTypeChange_IsModified()
+    {
+        var existing = MakeTemplate("T1");
+        existing.Attributes.Add(new TemplateAttribute("L")
+        {
+            DataType = DataType.List,
+            ElementDataType = DataType.Int32,
+            Value = "[1,2]",
+        });
+        var incoming = MakeTemplateDto("T1", attributes:
+        [
+            new TemplateAttributeDto("L", "[1,2]", DataType.List, false, null, null, DataType.String),
+        ]);
+
+        var item = _diff.CompareTemplate(incoming, existing);
+
+        Assert.Equal(ConflictKind.Modified, item.Kind);
+        ChangeFor(item, "Attributes.L"); // ElementDataType diverged — previously missed.
+    }
+
+    [Fact]
+    public void CompareTemplate_LockedInDerivedChange_IsModified()
+    {
+        var existing = MakeTemplate("T1");
+        existing.Attributes.Add(new TemplateAttribute("L")
+        {
+            DataType = DataType.Int32,
+            Value = "1",
+            LockedInDerived = false,
+        });
+        var incoming = MakeTemplateDto("T1", attributes:
+        [
+            new TemplateAttributeDto("L", "1", DataType.Int32, false, null, null, null, LockedInDerived: true),
+        ]);
+
+        var item = _diff.CompareTemplate(incoming, existing);
+
+        Assert.Equal(ConflictKind.Modified, item.Kind);
+        ChangeFor(item, "Attributes.L"); // LockedInDerived diverged — previously missed.
+    }
+
+    [Fact]
+    public void CompareTemplate_ScriptExecutionTimeoutChange_IsModified()
+    {
+        var existing = MakeTemplate("T1");
+        existing.Scripts.Add(new TemplateScript("S", "return 1;") { ExecutionTimeoutSeconds = 30 });
+        var incoming = MakeTemplateDto("T1", scripts:
+        [
+            // identical code; only ExecutionTimeoutSeconds differs.
+            new TemplateScriptDto("S", "return 1;", null, null, null, null, false, null, ExecutionTimeoutSeconds: 60),
+        ]);
+
+        var item = _diff.CompareTemplate(incoming, existing);
+
+        Assert.Equal(ConflictKind.Modified, item.Kind);
+        ChangeFor(item, "Scripts.S"); // ExecutionTimeoutSeconds diverged — previously missed.
+    }
+
+    [Fact]
+    public void CompareTemplate_AlarmOnTriggerScriptChange_IsModified()
+    {
+        var existing = MakeTemplate("T1");
+        existing.Scripts.Add(new TemplateScript("Run", "return 1;") { Id = 1 });
+        existing.Alarms.Add(new TemplateAlarm("A")
+        {
+            TriggerType = AlarmTriggerType.RangeViolation,
+            OnTriggerScriptId = 1, // resolves to "Run"
+        });
+        var incoming = MakeTemplateDto("T1",
+            scripts: [new TemplateScriptDto("Run", "return 1;", null, null, null, null, false, null)],
+            alarms:
+            [
+                // same alarm fields but no on-trigger script (was "Run").
+                new TemplateAlarmDto("A", null, 0, AlarmTriggerType.RangeViolation, null, false, OnTriggerScriptName: null),
+            ]);
+
+        var item = _diff.CompareTemplate(incoming, existing);
+
+        Assert.Equal(ConflictKind.Modified, item.Kind);
+        ChangeFor(item, "Alarms.A"); // OnTriggerScriptName diverged — previously missed.
+    }
+
    // ============ M8: CompareSite ============

    [Fact]
@@ -433,14 +518,18 @@ public sealed class ArtifactDiffTests
    private static Commons.Entities.Templates.Template MakeTemplate(string name) =>
        new(name);

-    private static TemplateDto MakeTemplateDto(string name, IReadOnlyList<TemplateScriptDto>? scripts = null) =>
+    private static TemplateDto MakeTemplateDto(
+        string name,
+        IReadOnlyList<TemplateScriptDto>? scripts = null,
+        IReadOnlyList<TemplateAttributeDto>? attributes = null,
+        IReadOnlyList<TemplateAlarmDto>? alarms = null) =>
        new(
            Name: name,
            FolderName: null,
            BaseTemplateName: null,
            Description: null,
-            Attributes: Array.Empty<TemplateAttributeDto>(),
-            Alarms: Array.Empty<TemplateAlarmDto>(),
+            Attributes: attributes ?? Array.Empty<TemplateAttributeDto>(),
+            Alarms: alarms ?? Array.Empty<TemplateAlarmDto>(),
            Scripts: scripts ?? Array.Empty<TemplateScriptDto>(),
            Compositions: Array.Empty<TemplateCompositionDto>());