From 1ccd6cfe33396b53cb9dcdf817139869387cc254 Mon Sep 17 00:00:00 2001
From: Joseph Doherty <dohejw01@gmail.com>
Date: Tue, 10 Nov 2026 05:07:40 -0500
Subject: [PATCH] fix(templates): override rows on a derived template are not
 second definitions in the collision check (ScadaBridge#119)

CollisionDetector counted a derived template's own override row (same name as a
parent member, IsInherited=false) as a second definition alongside the
parent's row, so AddAttribute/AddAlarm/AddScript for a fresh name failed with a
false "Naming collision" once any override existed. The inheritance walk now
keeps a (type, name) shadow set: a member defined at a nearer level of the
chain shadows the same member further up (effective member set, by name),
including overrides at mid levels. Real collisions (e.g. against a composed
module) still report.

Regression tests in TemplateEngine.Tests (detector + service, attribute/alarm/
script, mid-level override, positive control); 6 fail without the fix.
The #118 importer's additions-before-overrides ordering is now unnecessary but
harmless and was left in place.

Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
---
 .../CollisionDetector.cs                      | 38 ++++++++---
 .../CollisionDetectorTests.cs                 | 56 ++++++++++++++++
 .../TemplateServiceTests.cs                   | 64 +++++++++++++++++++
 3 files changed, 149 insertions(+), 9 deletions(-)

diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/CollisionDetector.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/CollisionDetector.cs
index 6e0eb8df..20734f46 100644
--- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/CollisionDetector.cs
+++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/CollisionDetector.cs
@@ -36,7 +36,11 @@ public static class CollisionDetector
 
         // Collect direct (top-level) members. Inherited placeholder rows are skipped:
         // the inheritance walk below re-adds them under the parent's origin.
-        CollectDirectMembers(template, prefix: null, originPrefix: template.Name, allMembers, skipInherited: true);
+        // Names defined by a nearer level of the chain shadow the same (type, name) further
+        // up: an own row sharing a parent member's name is an OVERRIDE of it (the effective
+        // member set holds the name once), not a second definition (ScadaBridge#119).
+        var shadowed = new HashSet<(string Type, string Name)>();
+        CollectDirectMembers(template, prefix: null, originPrefix: template.Name, allMembers, skipInherited: true, shadowed);
 
         // Collect members from composed modules recursively
         foreach (var composition in template.Compositions)
@@ -53,7 +57,7 @@ public static class CollisionDetector
         }
 
         // Collect inherited members (walk parent chain)
-        CollectInheritedMembers(template, lookup, allMembers, moduleMemo, new HashSet<int> { template.Id });
+        CollectInheritedMembers(template, lookup, allMembers, moduleMemo, new HashSet<int> { template.Id }, shadowed);
 
         // Detect duplicates by canonical name
         var collisions = new List<string>();
@@ -93,28 +97,43 @@ public static class CollisionDetector
         string? prefix,
         string originPrefix,
         List<ResolvedMember> members,
-        bool skipInherited)
+        bool skipInherited,
+        HashSet<(string Type, string Name)>? shadowed = null)
     {
+        // Rows shadowed by a nearer level are skipped; this level's own names are only
+        // added to the shadow set afterwards so same-level duplicates stay visible.
+        var levelKeys = new List<(string, string)>();
+        bool Skip(string type, string name, bool inherited)
+        {
+            if (skipInherited && inherited) return true;
+            if (shadowed != null && shadowed.Contains((type, name))) return true;
+            levelKeys.Add((type, name));
+            return false;
+        }
+
         foreach (var attr in template.Attributes)
         {
-            if (skipInherited && attr.IsInherited) continue;
+            if (Skip("Attribute", attr.Name, attr.IsInherited)) continue;
             var canonicalName = prefix == null ? attr.Name : $"{prefix}.{attr.Name}";
             members.Add(new ResolvedMember(canonicalName, "Attribute", originPrefix));
         }
 
         foreach (var alarm in template.Alarms)
         {
-            if (skipInherited && alarm.IsInherited) continue;
+            if (Skip("Alarm", alarm.Name, alarm.IsInherited)) continue;
             var canonicalName = prefix == null ? alarm.Name : $"{prefix}.{alarm.Name}";
             members.Add(new ResolvedMember(canonicalName, "Alarm", originPrefix));
         }
 
         foreach (var script in template.Scripts)
         {
-            if (skipInherited && script.IsInherited) continue;
+            if (Skip("Script", script.Name, script.IsInherited)) continue;
             var canonicalName = prefix == null ? script.Name : $"{prefix}.{script.Name}";
             members.Add(new ResolvedMember(canonicalName, "Script", originPrefix));
         }
+
+        if (shadowed != null)
+            foreach (var k in levelKeys) shadowed.Add(k);
     }
 
     /// <summary>
@@ -234,7 +253,8 @@ public static class CollisionDetector
         Dictionary<int, Template> lookup,
         List<ResolvedMember> members,
         Dictionary<int, IReadOnlyList<RelativeMember>> moduleMemo,
-        HashSet<int> visited)
+        HashSet<int> visited,
+        HashSet<(string Type, string Name)> shadowed)
     {
         if (!template.ParentTemplateId.HasValue)
             return;
@@ -248,7 +268,7 @@ public static class CollisionDetector
         // Inherited direct members (no prefix). Skip the parent's OWN inherited
         // placeholders too: the next recursion adds the grandparent's real rows, so
         // counting the parent's copies would re-introduce the collision one level up.
-        CollectDirectMembers(parent, prefix: null, $"parent '{parent.Name}'", members, skipInherited: true);
+        CollectDirectMembers(parent, prefix: null, $"parent '{parent.Name}'", members, skipInherited: true, shadowed);
 
         // Inherited composed modules
         foreach (var composition in parent.Compositions)
@@ -265,6 +285,6 @@ public static class CollisionDetector
         }
 
         // Continue up the inheritance chain
-        CollectInheritedMembers(parent, lookup, members, moduleMemo, visited);
+        CollectInheritedMembers(parent, lookup, members, moduleMemo, visited, shadowed);
     }
 }
diff --git a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/CollisionDetectorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/CollisionDetectorTests.cs
index 2a6d92fa..4a1c1958 100644
--- a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/CollisionDetectorTests.cs
+++ b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/CollisionDetectorTests.cs
@@ -197,6 +197,62 @@ public class CollisionDetectorTests
         Assert.Contains(collisions, c => c.Contains("dup.Shared"));
     }
 
+    // ========================================================================
+    // ScadaBridge#119: an OVERRIDE row on a derived template (own, IsInherited=false,
+    // same name as a parent member) is not a second definition of the member.
+    // ========================================================================
+
+    private static (Template parent, Template derived) OverrideFixture()
+    {
+        var parent = new Template("Base") { Id = 1 };
+        parent.Attributes.Add(new TemplateAttribute("A") { Id = 10, TemplateId = 1, DataType = DataType.Float });
+        parent.Alarms.Add(new TemplateAlarm("Al") { Id = 11, TemplateId = 1, TriggerType = AlarmTriggerType.ValueMatch });
+        parent.Scripts.Add(new TemplateScript("Sc", "return 1;") { Id = 12, TemplateId = 1 });
+
+        var derived = new Template("Slot") { Id = 2, ParentTemplateId = 1 };
+        derived.Attributes.Add(new TemplateAttribute("A") { Id = 20, TemplateId = 2, DataType = DataType.Float, Value = "9" }); // override
+        derived.Alarms.Add(new TemplateAlarm("Al") { Id = 21, TemplateId = 2, TriggerType = AlarmTriggerType.ValueMatch });
+        derived.Scripts.Add(new TemplateScript("Sc", "return 2;") { Id = 22, TemplateId = 2 });
+        return (parent, derived);
+    }
+
+    /// <summary>Verifies that override rows of every member kind are not collisions.</summary>
+    [Fact]
+    public void DetectCollisions_OverrideRowsOnDerivedTemplate_NoFalseCollision()
+    {
+        var (parent, derived) = OverrideFixture();
+        derived.Attributes.Add(new TemplateAttribute("New") { TemplateId = 2, DataType = DataType.String });
+        Assert.Empty(CollisionDetector.DetectCollisions(derived, new List<Template> { parent, derived }));
+    }
+
+    /// <summary>Verifies that an override on a mid-level template does not collide with the grandparent.</summary>
+    [Fact]
+    public void DetectCollisions_OverrideAtMidLevel_NoFalseCollision()
+    {
+        var gp = new Template("Root") { Id = 1 };
+        gp.Attributes.Add(new TemplateAttribute("A") { Id = 10, TemplateId = 1, DataType = DataType.Float });
+        var mid = new Template("Mid") { Id = 2, ParentTemplateId = 1 };
+        mid.Attributes.Add(new TemplateAttribute("A") { Id = 20, TemplateId = 2, DataType = DataType.Float });
+        var leaf = new Template("Leaf") { Id = 3, ParentTemplateId = 2 };
+        leaf.Attributes.Add(new TemplateAttribute("A") { Id = 30, TemplateId = 3, DataType = DataType.Float, IsInherited = true });
+        Assert.Empty(CollisionDetector.DetectCollisions(leaf, new List<Template> { gp, mid, leaf }));
+    }
+
+    /// <summary>Positive control: a real collision on a template that also has overrides is still reported.</summary>
+    [Fact]
+    public void DetectCollisions_OverrideRowPresent_RealCollisionStillDetected()
+    {
+        var (parent, derived) = OverrideFixture();
+        var module = new Template("Module") { Id = 3 };
+        module.Attributes.Add(new TemplateAttribute("Shared") { Id = 30, TemplateId = 3, DataType = DataType.Float });
+        derived.Compositions.Add(new TemplateComposition("dup") { Id = 1, TemplateId = 2, ComposedTemplateId = 3 });
+        derived.Attributes.Add(new TemplateAttribute("dup.Shared") { TemplateId = 2, DataType = DataType.Float });
+
+        var c = CollisionDetector.DetectCollisions(derived, new List<Template> { parent, derived, module });
+        Assert.Single(c);
+        Assert.Contains("dup.Shared", c[0]);
+    }
+
     // ========================================================================
     // Repeated composition blindness. A template composed into TWO
     // different slots was added to the global visited-set on the first slot, so
diff --git a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/TemplateServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/TemplateServiceTests.cs
index dce8d198..9b4b5c46 100644
--- a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/TemplateServiceTests.cs
+++ b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/TemplateServiceTests.cs
@@ -307,6 +307,70 @@ public class TemplateServiceTests
         Assert.Equal(8, result.Value.TemplateId);
     }
 
+    private (Template parent, Template derived) SetupDerivedWithOverrides()
+    {
+        var parent = new Template("Base") { Id = 7 };
+        parent.Attributes.Add(new TemplateAttribute("A") { Id = 70, TemplateId = 7, DataType = DataType.Float });
+        parent.Alarms.Add(new TemplateAlarm("Al") { Id = 71, TemplateId = 7, TriggerType = AlarmTriggerType.ValueMatch });
+        parent.Scripts.Add(new TemplateScript("Sc", "return 1;") { Id = 72, TemplateId = 7 });
+        var derived = new Template("Slot") { Id = 8, ParentTemplateId = 7 };
+        derived.Attributes.Add(new TemplateAttribute("A") { Id = 80, TemplateId = 8, DataType = DataType.Float, Value = "9" });
+        derived.Alarms.Add(new TemplateAlarm("Al") { Id = 81, TemplateId = 8, TriggerType = AlarmTriggerType.ValueMatch });
+        derived.Scripts.Add(new TemplateScript("Sc", "return 2;") { Id = 82, TemplateId = 8 });
+        _repoMock.Setup(r => r.GetTemplateByIdAsync(8, It.IsAny<CancellationToken>())).ReturnsAsync(derived);
+        _repoMock.Setup(r => r.GetAllTemplatesAsync(It.IsAny<CancellationToken>()))
+            .ReturnsAsync(new List<Template> { parent, derived });
+        _repoMock.Setup(r => r.GetAllTemplatesForAnalysisAsync(It.IsAny<CancellationToken>()))
+            .ReturnsAsync(new List<Template> { parent, derived });
+        return (parent, derived);
+    }
+
+    /// <summary>ScadaBridge#119: a new attribute can be added once the derived template has an override.</summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task AddAttribute_ToDerivedTemplateWithOverrides_Succeeds()
+    {
+        SetupDerivedWithOverrides();
+        var result = await _service.AddAttributeAsync(8, new TemplateAttribute("Fresh") { DataType = DataType.String, Value = "" }, "admin");
+        Assert.True(result.IsSuccess, result.IsFailure ? result.Error : null);
+    }
+
+    /// <summary>ScadaBridge#119: a new alarm can be added once the derived template has an override.</summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task AddAlarm_ToDerivedTemplateWithOverrides_Succeeds()
+    {
+        SetupDerivedWithOverrides();
+        var alarm = new TemplateAlarm("Fresh") { PriorityLevel = 500, TriggerType = AlarmTriggerType.RangeViolation, TriggerConfiguration = """{"Max": 100}""" };
+        var result = await _service.AddAlarmAsync(8, alarm, "admin");
+        Assert.True(result.IsSuccess, result.IsFailure ? result.Error : null);
+    }
+
+    /// <summary>ScadaBridge#119: a new script can be added once the derived template has an override.</summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task AddScript_ToDerivedTemplateWithOverrides_Succeeds()
+    {
+        SetupDerivedWithOverrides();
+        var result = await _service.AddScriptAsync(8, new TemplateScript("Fresh", "return true;") { TriggerType = "Startup" }, "admin");
+        Assert.True(result.IsSuccess, result.IsFailure ? result.Error : null);
+    }
+
+    /// <summary>ScadaBridge#119 positive control: a genuine collision is still rejected on a template with overrides.</summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task AddAttribute_ToDerivedTemplateWithOverrides_RealCollisionStillRejected()
+    {
+        var (_, derived) = SetupDerivedWithOverrides();
+        var module = new Template("Module") { Id = 3 };
+        module.Attributes.Add(new TemplateAttribute("Shared") { Id = 30, TemplateId = 3, DataType = DataType.Float });
+        derived.Compositions.Add(new TemplateComposition("dup") { Id = 1, TemplateId = 8, ComposedTemplateId = 3 });
+        _repoMock.Setup(r => r.GetAllTemplatesForAnalysisAsync(It.IsAny<CancellationToken>()))
+            .ReturnsAsync(new List<Template> { derived, module });
+        var result = await _service.AddAttributeAsync(8, new TemplateAttribute("dup.Shared") { DataType = DataType.Float, Value = "" }, "admin");
+        Assert.True(result.IsFailure);
+    }
+
     /// <summary>Verifies that add attribute, duplicate name, fails.</summary>
     /// <returns>A task that represents the asynchronous operation.</returns>
     [Fact]