From d19271fff864f95db61b2d99c6a459370d99bda5 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 11 Jun 2026 21:44:19 -0400 Subject: [PATCH] fix(adminui): converter skips name-collisions + disabled relays (review) --- .../Uns/UnsTreeService.cs | 57 ++++++++++- .../Uns/UnsTreeServiceRelayConverterTests.cs | 99 ++++++++++++++++++- 2 files changed, 151 insertions(+), 5 deletions(-) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/UnsTreeService.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/UnsTreeService.cs index 162255fb..846a0bc0 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/UnsTreeService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/UnsTreeService.cs @@ -1050,6 +1050,17 @@ public sealed class UnsTreeService(IDbContextFactory 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(); var skipped = new List(); var aliasTagsToAdd = new List(); @@ -1071,6 +1082,13 @@ public sealed class UnsTreeService(IDbContextFactory 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 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 dbF } /// - /// Mints a unique alias TagId following the codebase's only id-minting convention — the - /// EQ- equipment-id scheme of "<prefix>-" + Guid.ToString("N")[..12] (decision #125). - /// No TAG- minting helper existed before this converter, so a fresh GUID-derived id is used: - /// it cannot collide with a removed relay's namespace and needs no read-back uniqueness check. + /// Mints a unique alias TagId following the codebase's id-minting convention — + /// "<prefix>-" + Guid.ToString("N")[..12] (decision #125). A collision with another + /// in-flight id is negligible given the 48-bit GUID prefix; the TagId unique index is the + /// backstop. /// private static string NewTagId() => $"TAG-{Guid.NewGuid().ToString("N")[..12]}"; @@ -1620,6 +1648,27 @@ public sealed class UnsTreeService(IDbContextFactory dbF TagConfig = BuildAliasTagConfig(input.FullName), }; + /// Lightweight key for the pre-loaded (EquipmentId, Name) tag-name collision set. + private readonly record struct EquipmentName(string EquipmentId, string Name); + + /// + /// Equality comparer for using ordinal string comparison on both fields, + /// matching the database unique index semantics for (EquipmentId, Name). + /// + private sealed class EquipmentNameComparer : IEqualityComparer + { + 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)); + } + /// /// Decision #122: an equipment may only bind to a driver in the same cluster as its line. /// Policy: diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/UnsTreeServiceRelayConverterTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/UnsTreeServiceRelayConverterTests.cs index f87893cf..f46911a7 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/UnsTreeServiceRelayConverterTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/UnsTreeServiceRelayConverterTests.cs @@ -116,7 +116,7 @@ public sealed class UnsTreeServiceRelayConverterTests /// Adds a virtual tag bound to a script on an equipment. 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(); + } + + /// Adds a plain (non-alias) Tag on an equipment, bound to the Modbus driver. + 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 ----- + + /// + /// 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. + /// + [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 ----- + + /// + /// A relay VirtualTag with Enabled == false 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. + /// + [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(); + } }