fix(adminui): converter skips name-collisions + disabled relays (review)

src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/UnsTreeService.cs

@@ -1050,6 +1050,17 @@ public sealed class UnsTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF
         var scripts = (await db.Scripts.Where(s => scriptIds.Contains(s.ScriptId)).ToListAsync(ct))
             .ToDictionary(s => s.ScriptId, s => s, StringComparer.Ordinal);
 
+        // Pre-load all existing (EquipmentId, Name) tag pairs so per-candidate name-collision checks
+        // are O(1) HashSet lookups rather than per-row DB queries. The set reflects the state BEFORE
+        // this batch; intra-batch alias-vs-alias collisions per equipment cannot occur because vtag
+        // names are unique within an equipment's VirtualTags table.
+        var existingTagNames = (await db.Tags
+                .Where(t => t.EquipmentId != null)
+                .Select(t => new { t.EquipmentId, t.Name })
+                .ToListAsync(ct))
+            .Select(t => new EquipmentName(t.EquipmentId!, t.Name))
+            .ToHashSet(EquipmentNameComparer.Instance);
+
         var converted = new List<RelayConversionItem>();
         var skipped = new List<RelayConversionSkip>();
         var aliasTagsToAdd = new List<Tag>();
@@ -1071,6 +1082,13 @@ public sealed class UnsTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF
                 continue;
             }
 
+            if (!vtag.Enabled)
+            {
+                skipped.Add(new RelayConversionSkip(vtag.EquipmentId, vtag.Name,
+                    "Virtual tag is disabled — convert manually if intended."));
+                continue;
+            }
+
             if (vtag.Historize)
             {
                 skipped.Add(new RelayConversionSkip(vtag.EquipmentId, vtag.Name,
@@ -1142,6 +1160,16 @@ public sealed class UnsTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF
                 fullName = rawRef;
             }
 
+            // Check for a name collision with an already-existing Tag on this equipment. If a Tag with
+            // the same (EquipmentId, Name) exists, converting would violate the unique index and abort
+            // the whole SaveChangesAsync — skip this vtag and report it instead.
+            if (existingTagNames.Contains(new EquipmentName(vtag.EquipmentId, vtag.Name)))
+            {
+                skipped.Add(new RelayConversionSkip(vtag.EquipmentId, vtag.Name,
+                    $"A tag named '{vtag.Name}' already exists on this equipment — convert manually if intended."));
+                continue;
+            }
+
             converted.Add(new RelayConversionItem(vtag.EquipmentId, vtag.Name, fullName, vtag.DataType));
 
             if (!dryRun)
@@ -1186,10 +1214,10 @@ public sealed class UnsTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF
     }
 
     /// <summary>
-    /// Mints a unique alias <c>TagId</c> following the codebase's only id-minting convention — the
+    /// Mints a unique alias <c>TagId</c> following the codebase's id-minting convention —
-    /// <c>EQ-</c> equipment-id scheme of <c>"&lt;prefix&gt;-" + Guid.ToString("N")[..12]</c> (decision #125).
+    /// <c>"&lt;prefix&gt;-" + Guid.ToString("N")[..12]</c> (decision #125). A collision with another
-    /// No <c>TAG-</c> minting helper existed before this converter, so a fresh GUID-derived id is used:
+    /// in-flight id is negligible given the 48-bit GUID prefix; the <c>TagId</c> unique index is the
-    /// it cannot collide with a removed relay's namespace and needs no read-back uniqueness check.
+    /// backstop.
     /// </summary>
     private static string NewTagId() => $"TAG-{Guid.NewGuid().ToString("N")[..12]}";
 
@@ -1620,6 +1648,27 @@ public sealed class UnsTreeService(IDbContextFactory<OtOpcUaConfigDbContext> dbF
         TagConfig = BuildAliasTagConfig(input.FullName),
     };
 
+    /// <summary>Lightweight key for the pre-loaded (EquipmentId, Name) tag-name collision set.</summary>
+    private readonly record struct EquipmentName(string EquipmentId, string Name);
+
+    /// <summary>
+    /// Equality comparer for <see cref="EquipmentName"/> using ordinal string comparison on both fields,
+    /// matching the database unique index semantics for <c>(EquipmentId, Name)</c>.
+    /// </summary>
+    private sealed class EquipmentNameComparer : IEqualityComparer<EquipmentName>
+    {
+        public static readonly EquipmentNameComparer Instance = new();
+
+        public bool Equals(EquipmentName x, EquipmentName y) =>
+            StringComparer.Ordinal.Equals(x.EquipmentId, y.EquipmentId) &&
+            StringComparer.Ordinal.Equals(x.Name, y.Name);
+
+        public int GetHashCode(EquipmentName obj) =>
+            HashCode.Combine(
+                StringComparer.Ordinal.GetHashCode(obj.EquipmentId),
+                StringComparer.Ordinal.GetHashCode(obj.Name));
+    }
+
     /// <summary>
     /// Decision #122: an equipment may only bind to a driver in the same cluster as its line.
     /// Policy:

tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/UnsTreeServiceRelayConverterTests.cs

@@ -116,7 +116,7 @@ public sealed class UnsTreeServiceRelayConverterTests
     /// <summary>Adds a virtual tag bound to a script on an equipment.</summary>
     private static void AddVirtualTag(
         string dbName, string virtualTagId, string equipmentId, string name, string scriptId,
-        string dataType = "Int32", bool historize = false)
+        string dataType = "Int32", bool historize = false, bool enabled = true)
     {
         using var db = UnsTreeTestDb.CreateNamed(dbName);
         db.VirtualTags.Add(new VirtualTag
@@ -127,6 +127,24 @@ public sealed class UnsTreeServiceRelayConverterTests
             DataType = dataType,
             ScriptId = scriptId,
             Historize = historize,
+            Enabled = enabled,
+        });
+        db.SaveChanges();
+    }
+
+    /// <summary>Adds a plain (non-alias) Tag on an equipment, bound to the Modbus driver.</summary>
+    private static void AddPlainTag(string dbName, string tagId, string equipmentId, string name)
+    {
+        using var db = UnsTreeTestDb.CreateNamed(dbName);
+        db.Tags.Add(new Tag
+        {
+            TagId = tagId,
+            DriverInstanceId = ModbusDriverId,
+            EquipmentId = equipmentId,
+            Name = name,
+            DataType = "Int32",
+            AccessLevel = TagAccessLevel.Read,
+            TagConfig = "{}",
         });
         db.SaveChanges();
     }
@@ -514,4 +532,83 @@ public sealed class UnsTreeServiceRelayConverterTests
         db.VirtualTags.Any(v => v.VirtualTagId == "VT-1").ShouldBeTrue();
         db.Tags.Any(t => t.Name == "speed").ShouldBeFalse();
     }
+
+    // ----- Fix A: existing Tag name collision -> skip + batch continues -----
+
+    /// <summary>
+    /// When a relay VirtualTag has the same name as an existing Tag on the same equipment the
+    /// converter must skip it (not throw DbUpdateException from a unique-index violation) and continue
+    /// converting any other relay on the same equipment in the same batch. The colliding VirtualTag
+    /// and its Script must remain unchanged; exactly one Tag with that name must exist.
+    /// </summary>
+    [Fact]
+    public async Task Name_collision_with_existing_tag_is_skipped_and_batch_continues()
+    {
+        var dbName = SeedCluster();
+        AddEquipment(dbName, "EQ-1", "m_001");
+
+        // Existing plain Tag named "speed-rpm" (e.g. a manually-created Modbus tag).
+        AddPlainTag(dbName, "TAG-EXISTING", "EQ-1", "speed-rpm");
+
+        // Relay VirtualTag also named "speed-rpm" — would collide on the unique index.
+        AddScript(dbName, "SCRIPT-COLLIDE", Relay("TestMachine_020.SpeedRpm"));
+        AddVirtualTag(dbName, "VT-COLLIDE", "EQ-1", "speed-rpm", "SCRIPT-COLLIDE");
+
+        // A second, non-colliding relay on the same equipment in the same batch.
+        AddScript(dbName, "SCRIPT-OK", Relay("TestMachine_020.Torque"));
+        AddVirtualTag(dbName, "VT-OK", "EQ-1", "torque", "SCRIPT-OK");
+
+        var service = new UnsTreeService(UnsTreeTestDb.Factory(dbName));
+        var result = await service.ConvertRelayVirtualTagsToAliasesAsync("EQ-1", dryRun: false);
+
+        // The colliding vtag must appear in Skipped with a reason mentioning "already exists".
+        var skip = result.Skipped.ShouldHaveSingleItem(s => s.VirtualTagName == "speed-rpm");
+        skip.Reason.ShouldContain("already exists", Case.Insensitive);
+
+        // The non-colliding relay must still have been converted (batch was NOT aborted).
+        result.Converted.ShouldHaveSingleItem(i => i.VirtualTagName == "torque");
+
+        using var db = UnsTreeTestDb.CreateNamed(dbName);
+
+        // Exactly one Tag named "speed-rpm" (the original plain tag) — no duplicate was added.
+        db.Tags.Count(t => t.EquipmentId == "EQ-1" && t.Name == "speed-rpm").ShouldBe(1);
+        db.Tags.Single(t => t.EquipmentId == "EQ-1" && t.Name == "speed-rpm").TagId.ShouldBe("TAG-EXISTING");
+
+        // The colliding VirtualTag (and its script) must still be present.
+        db.VirtualTags.Any(v => v.VirtualTagId == "VT-COLLIDE").ShouldBeTrue();
+        db.Scripts.Any(s => s.ScriptId == "SCRIPT-COLLIDE").ShouldBeTrue();
+
+        // The non-colliding relay's Tag exists and its vtag + script are gone.
+        db.Tags.Any(t => t.EquipmentId == "EQ-1" && t.Name == "torque").ShouldBeTrue();
+        db.VirtualTags.Any(v => v.VirtualTagId == "VT-OK").ShouldBeFalse();
+        db.Scripts.Any(s => s.ScriptId == "SCRIPT-OK").ShouldBeFalse();
+    }
+
+    // ----- Fix B: disabled relay -> skip + report -----
+
+    /// <summary>
+    /// A relay VirtualTag with <c>Enabled == false</c> must be skipped rather than silently promoted
+    /// into an always-active alias Tag. The reason must mention "disabled". The VirtualTag and Script
+    /// remain unchanged and no Tag is added.
+    /// </summary>
+    [Fact]
+    public async Task Disabled_relay_is_skipped()
+    {
+        var dbName = SeedCluster();
+        AddEquipment(dbName, "EQ-1", "m_001");
+        AddScript(dbName, "SCRIPT-1", Relay("TestMachine_020.DisabledVal"));
+        AddVirtualTag(dbName, "VT-1", "EQ-1", "disabled-relay", "SCRIPT-1", enabled: false);
+
+        var service = new UnsTreeService(UnsTreeTestDb.Factory(dbName));
+        var result = await service.ConvertRelayVirtualTagsToAliasesAsync("EQ-1", dryRun: false);
+
+        result.Converted.ShouldBeEmpty();
+        var skip = result.Skipped.ShouldHaveSingleItem(s => s.VirtualTagName == "disabled-relay");
+        skip.Reason.ShouldContain("disabled", Case.Insensitive);
+
+        using var db = UnsTreeTestDb.CreateNamed(dbName);
+        db.VirtualTags.Any(v => v.VirtualTagId == "VT-1").ShouldBeTrue();
+        db.Scripts.Any(s => s.ScriptId == "SCRIPT-1").ShouldBeTrue();
+        db.Tags.Any(t => t.Name == "disabled-relay").ShouldBeFalse();
+    }
 }