diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/ArtifactDiff.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/ArtifactDiff.cs index 190561ec..4e08dd27 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/ArtifactDiff.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/ArtifactDiff.cs @@ -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 diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs index b2b6fb1f..3be5b05b 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/BundleImporter.cs @@ -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; diff --git a/src/ZB.MOM.WW.ScadaBridge.Transport/Import/TemplateChildEquality.cs b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/TemplateChildEquality.cs new file mode 100644 index 00000000..66cfc46d --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.Transport/Import/TemplateChildEquality.cs @@ -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; + +/// +/// #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. +/// +/// BOTH the Preview diff () and the +/// Overwrite child-sync (BundleImporter.SyncTemplate*Async) 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 +/// ElementDataType, ExecutionTimeoutSeconds, the alarm on-trigger +/// script, and LockedInDerived — reporting Identical for artifacts an +/// Overwrite would in fact mutate. Adding a writable field to any template-child +/// entity means adding it here, once. +/// +/// +internal static class TemplateChildEquality +{ + /// + /// True when the persisted attribute equals its bundle DTO across every writable + /// field. The single source of truth for "attribute changed"; both ArtifactDiff + /// and SyncTemplateAttributesAsync call it. + /// + /// Callers that normalise the DTO Value before comparing (the importer + /// normalises List values) pass a DTO whose Value is already normalised. + /// + /// + 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; + + /// + /// 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 — a resolver over the alarm's own + /// template scripts — to map the persisted OnTriggerScriptId FK back to a + /// name for comparison. The single source of truth for "alarm changed"; both + /// ArtifactDiff and SyncTemplateAlarmsAsync call it. + /// + public static bool AlarmsEqual(TemplateAlarm e, TemplateAlarmDto i, Func 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; + + /// + /// True when the persisted script equals its bundle DTO across every writable + /// field. The single source of truth for "script changed"; both ArtifactDiff + /// (via DiffScriptChildren) and SyncTemplateScriptsAsync call it. + /// + 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; + + /// + /// 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 SyncTemplateNativeAlarmSourcesAsync call it. + /// + 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; + + /// + /// 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. + /// + public static Func ScriptNameResolver(IEnumerable scripts) + { + var byId = new Dictionary(); + foreach (var s in scripts) + { + byId[s.Id] = s.Name; + } + + return id => id is { } sid && byId.TryGetValue(sid, out var name) ? name : null; + } +} 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 46cffe46..5fa731a3 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/ArtifactDiffTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Transport.Tests/Import/ArtifactDiffTests.cs @@ -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? scripts = null) => + private static TemplateDto MakeTemplateDto( + string name, + IReadOnlyList? scripts = null, + IReadOnlyList? attributes = null, + IReadOnlyList? alarms = null) => new( Name: name, FolderName: null, BaseTemplateName: null, Description: null, - Attributes: Array.Empty(), - Alarms: Array.Empty(), + Attributes: attributes ?? Array.Empty(), + Alarms: alarms ?? Array.Empty(), Scripts: scripts ?? Array.Empty(), Compositions: Array.Empty());