From fa36d64e8f61632a36278d60ecf292e1926d7ff0 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 16 Aug 2026 04:03:49 -0400 Subject: [PATCH] perf(adminui): one guard round-trip per bulk reference add --- .../Uns/EffectiveNameGuard.cs | 114 ++++++++++++++---- .../Uns/IEffectiveNameGuard.cs | 34 ++++++ .../Uns/UnsTreeService.cs | 14 ++- .../Uns/EffectiveNameGuardTests.cs | 53 ++++++++ .../Uns/UnsTreeServiceReferenceTests.cs | 64 +++++++++- 5 files changed, 251 insertions(+), 28 deletions(-) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/EffectiveNameGuard.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/EffectiveNameGuard.cs index 97c8d124..2c1c11e8 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/EffectiveNameGuard.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/EffectiveNameGuard.cs @@ -12,8 +12,11 @@ namespace ZB.MOM.WW.OtOpcUa.AdminUI.Uns; /// DraftValidator UnsEffectiveNameCollision rule. /// /// -/// Each call creates and disposes its own pooled context — the same per-call pattern -/// uses — so the guard is safe to register Scoped. +/// Each call creates and disposes its own context — the same per-call pattern +/// uses — so the guard is safe to register Scoped. The factory +/// really is pooled: ServiceCollectionExtensions.AddOtOpcUaConfigDb registers it with +/// AddPooledDbContextFactory, so the per-call create rents from the pool and the dispose +/// returns the context to it. /// public sealed class EffectiveNameGuard(IDbContextFactory dbFactory) : IEffectiveNameGuard { @@ -27,6 +30,48 @@ public sealed class EffectiveNameGuard(IDbContextFactory { await using var db = await dbFactory.CreateDbContextAsync(ct); + var nameSpace = await LoadNameSpaceAsync(db, equipmentId, ct); + + return FirstCollision(nameSpace, proposedEffectiveName, kind, excludeSourceId, equipmentId); + } + + /// + /// + /// The whole point of the batch entry point: ONE context and ONE load of the equipment's name + /// space, then every proposed name tested in memory against it — the per-name rules and message + /// text come from the same helper uses, so + /// the two can never drift. A batch is a create of N new rows, so there is no self-row to exclude. + /// + public async Task CheckManyAsync( + string equipmentId, + IReadOnlyList proposedEffectiveNames, + EffectiveNameSourceKind kind, + CancellationToken ct = default) + { + ArgumentNullException.ThrowIfNull(proposedEffectiveNames); + if (proposedEffectiveNames.Count == 0) return null; + + await using var db = await dbFactory.CreateDbContextAsync(ct); + + var nameSpace = await LoadNameSpaceAsync(db, equipmentId, ct); + + foreach (var proposed in proposedEffectiveNames) + { + var collision = FirstCollision(nameSpace, proposed, kind, excludeSourceId: null, equipmentId); + if (collision is not null) return collision; + } + + return null; + } + + /// + /// Loads the equipment's current effective-name space as a flat, ordered row list: references + /// first (override else the backing raw tag's Name), then VirtualTags, then ScriptedAlarms + /// — the order has always reported collisions in. + /// + private static async Task> LoadNameSpaceAsync( + OtOpcUaConfigDbContext db, string equipmentId, CancellationToken ct) + { // References for this equipment. The effective name is the display-name override else the // backing raw tag's current Name, so the two sets (references + tags) are loaded and joined // in memory — the per-equipment set is small and this mirrors the deploy rule exactly @@ -46,47 +91,66 @@ public sealed class EffectiveNameGuard(IDbContextFactory .GroupBy(t => t.TagId, StringComparer.Ordinal) .ToDictionary(g => g.Key, g => g.First().Name, StringComparer.Ordinal); - foreach (var r in references) - { - if (IsSelf(EffectiveNameSourceKind.Reference, r.UnsTagReferenceId, kind, excludeSourceId)) - continue; - var effective = r.DisplayNameOverride - ?? (tagNameById.TryGetValue(r.TagId, out var n) ? n : null); - if (Collides(effective, proposedEffectiveName)) - return Message(proposedEffectiveName, "reference", r.UnsTagReferenceId, equipmentId); - } - var virtualTags = await db.VirtualTags .AsNoTracking() .Where(v => v.EquipmentId == equipmentId) .Select(v => new { v.VirtualTagId, v.Name }) .ToListAsync(ct); - foreach (var v in virtualTags) - { - if (IsSelf(EffectiveNameSourceKind.VirtualTag, v.VirtualTagId, kind, excludeSourceId)) - continue; - if (Collides(v.Name, proposedEffectiveName)) - return Message(proposedEffectiveName, "VirtualTag", v.VirtualTagId, equipmentId); - } - var scriptedAlarms = await db.ScriptedAlarms .AsNoTracking() .Where(a => a.EquipmentId == equipmentId) .Select(a => new { a.ScriptedAlarmId, a.Name }) .ToListAsync(ct); - foreach (var a in scriptedAlarms) + var rows = new List(references.Count + virtualTags.Count + scriptedAlarms.Count); + + foreach (var r in references) { - if (IsSelf(EffectiveNameSourceKind.ScriptedAlarm, a.ScriptedAlarmId, kind, excludeSourceId)) - continue; - if (Collides(a.Name, proposedEffectiveName)) - return Message(proposedEffectiveName, "ScriptedAlarm", a.ScriptedAlarmId, equipmentId); + var effective = r.DisplayNameOverride + ?? (tagNameById.TryGetValue(r.TagId, out var n) ? n : null); + rows.Add(new NameRow(EffectiveNameSourceKind.Reference, r.UnsTagReferenceId, effective, "reference")); + } + + foreach (var v in virtualTags) + rows.Add(new NameRow(EffectiveNameSourceKind.VirtualTag, v.VirtualTagId, v.Name, "VirtualTag")); + + foreach (var a in scriptedAlarms) + rows.Add(new NameRow(EffectiveNameSourceKind.ScriptedAlarm, a.ScriptedAlarmId, a.Name, "ScriptedAlarm")); + + return rows; + } + + /// + /// The single in-memory collision rule shared by and + /// — same self-row exclusion, same ordinal comparison, same message. + /// + private static string? FirstCollision( + List nameSpace, + string proposedEffectiveName, + EffectiveNameSourceKind kind, + string? excludeSourceId, + string equipmentId) + { + foreach (var row in nameSpace) + { + if (IsSelf(row.Kind, row.SourceId, kind, excludeSourceId)) continue; + if (Collides(row.EffectiveName, proposedEffectiveName)) + return Message(proposedEffectiveName, row.SourceLabel, row.SourceId, equipmentId); } return null; } + /// One existing effective-name-space row: what it is, its logical id, its effective name + /// (null when a reference's backing tag is missing — such a row contributes no name), and the label + /// the collision message uses. + private readonly record struct NameRow( + EffectiveNameSourceKind Kind, + string SourceId, + string? EffectiveName, + string SourceLabel); + /// True when a row is the self-row being edited — same kind AND same logical id — so it /// is excluded from the collision set (a rename / override-change of a row cannot collide with itself). private static bool IsSelf( diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/IEffectiveNameGuard.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/IEffectiveNameGuard.cs index 959d8acd..5461c59d 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/IEffectiveNameGuard.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/IEffectiveNameGuard.cs @@ -58,4 +58,38 @@ public interface IEffectiveNameGuard EffectiveNameSourceKind kind, string? excludeSourceId, CancellationToken ct = default); + + /// + /// Checks many proposed names against ONE load of the equipment's name space. Returns the first + /// collision message (in input order), or when all are free. + /// + /// + /// The bulk-authoring entry point (a select-all "add references" over N raw tags), where the + /// per-name would cost N contexts × 4 queries. There is no + /// excludeSourceId: a batch is a CREATE of N new rows, so no self-row exists to exclude. + /// The default implementation simply loops — correct but not fast — so + /// a stub / no-op guard needs no change; EffectiveNameGuard overrides it with the + /// single-load version. + /// + /// The owning equipment's logical id. + /// The effective names to be authored, in the order the caller wants them reported. + /// Which surface the proposed names are for (words the error; identifies the self-row kind). + /// Cancellation token. + /// when every name is free; otherwise the first name's readable error. + async Task CheckManyAsync( + string equipmentId, + IReadOnlyList proposedEffectiveNames, + EffectiveNameSourceKind kind, + CancellationToken ct = default) + { + ArgumentNullException.ThrowIfNull(proposedEffectiveNames); + + foreach (var proposed in proposedEffectiveNames) + { + var collision = await CheckAsync(equipmentId, proposed, kind, excludeSourceId: null, ct); + if (collision is not null) return collision; + } + + return null; + } } 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 03509caa..00b76d9e 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/UnsTreeService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/UnsTreeService.cs @@ -209,6 +209,11 @@ public sealed class UnsTreeService( // Track effective names proposed within THIS batch so two raw tags sharing a Name are caught (the // guard sees only persisted rows; a same-batch collision would otherwise slip through). var batchNames = new HashSet(StringComparer.Ordinal); + // The proposed effective names, in selection order, for the ONE post-loop guard round-trip. The + // per-name guard call used to sit inside this loop, costing a fresh context + 4 queries per + // selected tag (a select-all of 500 tags ≈ 2,000 queries); the batch check loads the equipment's + // name space once and reports the first collision in this same order. + var proposedNames = new List(distinctTagIds.Count); var pending = new List(); foreach (var tagId in distinctTagIds) @@ -228,8 +233,7 @@ public sealed class UnsTreeService( return new UnsMutationResult(false, $"Two selected raw tags share the effective name '{tag.Name}'; set a display-name override so they stay unique within the equipment."); - var collision = await _guard.CheckAsync(equipmentId, tag.Name, EffectiveNameSourceKind.Reference, null, ct); - if (collision is not null) return new UnsMutationResult(false, collision); + proposedNames.Add(tag.Name); pending.Add(new UnsTagReference { @@ -241,6 +245,12 @@ public sealed class UnsTreeService( }); } + // One guard round-trip for the whole batch (one context + one load of the equipment's existing + // effective-name space), regardless of how many tags were selected. + var collision = await _guard.CheckManyAsync( + equipmentId, proposedNames, EffectiveNameSourceKind.Reference, ct); + if (collision is not null) return new UnsMutationResult(false, collision); + db.UnsTagReferences.AddRange(pending); try { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/EffectiveNameGuardTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/EffectiveNameGuardTests.cs index fe1ec492..71a3332e 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/EffectiveNameGuardTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/EffectiveNameGuardTests.cs @@ -190,6 +190,59 @@ public sealed class EffectiveNameGuardTests result.ShouldBeNull(); } + /// + /// The batch check is a pure round-trip saving: for every proposed name it must return exactly + /// what returns for the FIRST colliding name in input + /// order — including the source-specific message text — across all three source kinds. + /// + [Theory] + // First collision is the no-override reference ("speed"), even though an alarm name follows it. + [InlineData(new[] { "free-a", "speed", "highpress", "temperature" }, "speed")] + // First collision is the OVERRIDE-derived reference effective name. + [InlineData(new[] { "free-a", "temperature", "speed" }, "temperature")] + // First collision is the VirtualTag. + [InlineData(new[] { "free-a", "computed", "speed" }, "computed")] + // First collision is the ScriptedAlarm. + [InlineData(new[] { "free-a", "highpress", "speed" }, "highpress")] + public async Task CheckMany_matches_CheckAsync_verdicts(string[] names, string expectedCollidingName) + { + var factory = SeedFactory(); + var guard = Guard(factory); + + var expected = await guard.CheckAsync( + EquipmentId, expectedCollidingName, EffectiveNameSourceKind.Reference, excludeSourceId: null); + expected.ShouldNotBeNull(); + + var actual = await guard.CheckManyAsync(EquipmentId, names, EffectiveNameSourceKind.Reference); + + actual.ShouldBe(expected); + } + + /// A batch where every proposed name is free reports no collision. + [Fact] + public async Task CheckMany_all_free_returns_null() + { + var guard = Guard(SeedFactory()); + + var result = await guard.CheckManyAsync( + EquipmentId, + ["free-a", "free-b", "Speed" /* ordinal: capital S does not collide */, "free-c"], + EffectiveNameSourceKind.Reference); + + result.ShouldBeNull(); + } + + /// An empty batch is trivially free (and must not throw). + [Fact] + public async Task CheckMany_empty_batch_returns_null() + { + var guard = Guard(SeedFactory()); + + var result = await guard.CheckManyAsync(EquipmentId, [], EffectiveNameSourceKind.Reference); + + result.ShouldBeNull(); + } + private sealed class NamedFactory(string name) : IDbContextFactory { public OtOpcUaConfigDbContext CreateDbContext() => diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/UnsTreeServiceReferenceTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/UnsTreeServiceReferenceTests.cs index 8eda6188..ad29ef89 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/UnsTreeServiceReferenceTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/UnsTreeServiceReferenceTests.cs @@ -23,13 +23,24 @@ public sealed class UnsTreeServiceReferenceTests { private const string EquipmentId = "EQ-1"; - /// A configurable : returns from every check. + /// + /// A configurable : returns from every check. + /// is implemented explicitly (rather than inheriting the interface's + /// looping default) so counts BATCH round-trips — that count is what + /// pins "one guard round-trip per bulk add, regardless of how many tags were selected". + /// private sealed class FakeGuard : IEffectiveNameGuard { public string? Result { get; set; } public string? LastProposedName { get; private set; } + /// How many times the BATCH entry point was called. + public int CheckManyCalls { get; private set; } + + /// The names passed to the last call, in input order. + public IReadOnlyList LastProposedNames { get; private set; } = []; + public Task CheckAsync( string equipmentId, string proposedEffectiveName, EffectiveNameSourceKind kind, string? excludeSourceId, CancellationToken ct = default) @@ -37,6 +48,16 @@ public sealed class UnsTreeServiceReferenceTests LastProposedName = proposedEffectiveName; return Task.FromResult(Result); } + + public Task CheckManyAsync( + string equipmentId, IReadOnlyList proposedEffectiveNames, + EffectiveNameSourceKind kind, CancellationToken ct = default) + { + CheckManyCalls++; + LastProposedNames = [.. proposedEffectiveNames]; + LastProposedName = proposedEffectiveNames.Count > 0 ? proposedEffectiveNames[^1] : null; + return Task.FromResult(proposedEffectiveNames.Count > 0 ? Result : null); + } } private static OtOpcUaConfigDbContext Db(string name) => @@ -178,6 +199,47 @@ public sealed class UnsTreeServiceReferenceTests db.UnsTagReferences.Any(r => r.EquipmentId == EquipmentId).ShouldBeFalse(); } + /// + /// A multi-tag add asks the guard ONCE — one batch call carrying every proposed effective name in + /// selection order — instead of one round-trip per selected tag. + /// + [Fact] + public async Task AddReferences_asks_the_guard_once_for_the_whole_batch() + { + var name = $"ref-batch-guard-{Guid.NewGuid():N}"; + Seed(name); + var guard = new FakeGuard(); + var service = new UnsTreeService(Factory(name), guard); + + var result = await service.AddReferencesAsync(EquipmentId, ["TAG-speed", "TAG-running", "TAG-grouped"]); + + result.Ok.ShouldBeTrue(); + guard.CheckManyCalls.ShouldBe(1); + guard.LastProposedNames.ShouldBe(new[] { "speed", "running", "nested" }); + + using var db = Db(name); + db.UnsTagReferences.Count(r => r.EquipmentId == EquipmentId).ShouldBe(3); + } + + /// A collision reported by the batch guard fails the whole add; nothing is persisted. + [Fact] + public async Task AddReferences_batch_guard_collision_rejected() + { + var name = $"ref-batch-guard-fail-{Guid.NewGuid():N}"; + Seed(name); + var guard = new FakeGuard { Result = "'running' already exists on equipment 'machine-1'." }; + var service = new UnsTreeService(Factory(name), guard); + + var result = await service.AddReferencesAsync(EquipmentId, ["TAG-speed", "TAG-running"]); + + result.Ok.ShouldBeFalse(); + result.Error.ShouldBe("'running' already exists on equipment 'machine-1'."); + guard.CheckManyCalls.ShouldBe(1); + + using var db = Db(name); + db.UnsTagReferences.Any(r => r.EquipmentId == EquipmentId).ShouldBeFalse(); + } + // ----- SetReferenceOverride ----- /// Setting an override changes the effective name; clearing it falls back to the raw name.