From 928e06dd011f891d77f53a14135d6b774f2434e2 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 16 Jul 2026 02:45:06 -0400 Subject: [PATCH] review(b2-waveA): /raw auto-expand enterprise roots; friendly create-race + delete failures; auth-guard classifies /raw + dev demo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wave-A reviewer findings (no Critical/High): - M-2: GlobalRaw auto-expands Enterprise roots so the Cluster level is visible on load (mirrors GlobalUns.ExpandStructural); clusters stay lazily collapsed. - L-1: create mutations route SaveChanges through SaveCreateAsync — a lost sibling-name race (filtered-unique-index clash) becomes a friendly failure instead of an uncaught DbUpdateException. - L-2: SaveDeleteAsync no longer leaks raw EF/SQL text; operator-friendly guidance. - L-5: RawNode.ChildCount doc clarified (direct children only). - Regression fix: PageAuthorizationGuardTests classifies the two new routable pages (/raw → ConfigEditor, /dev/context-menu-demo → AuthenticatedRead). M-1 (ordinal vs SQL-collation sibling uniqueness) assessed benign: name indexes use the DB default collation and the server-side pre-check runs under that same collation, so pre-check and index agree — no case-variant rows can coexist, so runtime ordinal RawPath keying never collides. Claude-Session: https://claude.ai/code/session_01LVneM3eh1UtJxEisFXgmox --- .../Components/Pages/Raw/GlobalRaw.razor | 6 ++++ .../ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawNode.cs | 2 +- .../Uns/RawTreeService.cs | 36 +++++++++++++------ .../PageAuthorizationGuardTests.cs | 6 ++-- 4 files changed, 37 insertions(+), 13 deletions(-) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Raw/GlobalRaw.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Raw/GlobalRaw.razor index 57af836f..845b9e06 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Raw/GlobalRaw.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Raw/GlobalRaw.razor @@ -55,6 +55,12 @@ try { _roots = await Svc.LoadRootsAsync(); + // Auto-expand the Enterprise roots so the Cluster level (which owns the lazy plumbing) is + // visible on load, mirroring GlobalUns.ExpandStructural. Enterprise children (clusters) are + // eagerly materialised by LoadRootsAsync, so this needs no lazy fetch; clusters stay + // collapsed and load their folders/drivers on first expand. + foreach (var enterprise in _roots) + enterprise.Expanded = true; } catch (Exception ex) { diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawNode.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawNode.cs index 973a989c..ad54d8c2 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawNode.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawNode.cs @@ -69,7 +69,7 @@ public sealed class RawNode /// Whether a Driver/Device node is enabled (drives the enable/disable menu label + styling); true otherwise. public bool Enabled { get; init; } = true; - /// Direct child count (for the badge). For a device this is its tag-group + tag count. + /// Direct child count (for the badge) — for a device, its root tag-groups + root tags (direct children only, not the whole subtree). public int ChildCount { get; set; } /// diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs index d5370f59..0965d71c 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs @@ -370,8 +370,7 @@ public sealed class RawTreeService(IDbContextFactory dbF ParentRawFolderId = parentFolderId, Name = name, }); - await db.SaveChangesAsync(ct); - return new UnsMutationResult(true, null, id); + return await SaveCreateAsync(db, "folder", id, ct); } /// @@ -510,8 +509,7 @@ public sealed class RawTreeService(IDbContextFactory dbF DeviceConfig = "{}", }); - await db.SaveChangesAsync(ct); - return new UnsMutationResult(true, null, driverId); + return await SaveCreateAsync(db, "driver", driverId, ct); } /// @@ -618,8 +616,7 @@ public sealed class RawTreeService(IDbContextFactory dbF Name = name, DeviceConfig = string.IsNullOrWhiteSpace(deviceConfigJson) ? "{}" : deviceConfigJson, }); - await db.SaveChangesAsync(ct); - return new UnsMutationResult(true, null, id); + return await SaveCreateAsync(db, "device", id, ct); } /// @@ -703,8 +700,7 @@ public sealed class RawTreeService(IDbContextFactory dbF ParentTagGroupId = parentGroupId, Name = name, }); - await db.SaveChangesAsync(ct); - return new UnsMutationResult(true, null, id); + return await SaveCreateAsync(db, "tag group", id, ct); } /// @@ -840,10 +836,30 @@ public sealed class RawTreeService(IDbContextFactory dbF { return new UnsMutationResult(false, $"Another user changed this {noun} while you were viewing it."); } - catch (Exception ex) + catch (Exception) { // Defensive net for a real SQL Server where a child slipped past the pre-check (race). - return new UnsMutationResult(false, $"Delete failed: {ex.Message}."); + // Don't surface the raw EF/SQL text (leaks schema detail); give operator-friendly guidance. + return new UnsMutationResult(false, + $"Delete failed — a related record still references this {noun}. Reload and retry."); + } + } + + /// Commits a create, translating a lost sibling-name race (filtered-unique-index clash) into a friendly failure. + private static async Task SaveCreateAsync( + OtOpcUaConfigDbContext db, string noun, string createdId, CancellationToken ct) + { + try + { + await db.SaveChangesAsync(ct); + return new UnsMutationResult(true, null, createdId); + } + catch (DbUpdateException) + { + // The name-uniqueness pre-check narrows but doesn't close the race against a concurrent + // create of the same-name sibling; the DB's filtered-unique index is the backstop. + return new UnsMutationResult(false, + $"A {noun} with that name already exists here (created concurrently). Reload and retry."); } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Authorization/PageAuthorizationGuardTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Authorization/PageAuthorizationGuardTests.cs index 4e12e47b..03297bd5 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Authorization/PageAuthorizationGuardTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Authorization/PageAuthorizationGuardTests.cs @@ -29,8 +29,9 @@ public class PageAuthorizationGuardTests /// private static readonly IReadOnlyDictionary ExpectedPolicy = new Dictionary { - // ── ConfigEditor (20): the config-authoring surface (incl. live ResilienceConfig) ── + // ── ConfigEditor (21): the config-authoring surface (incl. live ResilienceConfig) ── [typeof(GlobalUns)] = AdminUiPolicies.ConfigEditor, + [typeof(global::ZB.MOM.WW.OtOpcUa.AdminUI.Components.Pages.Raw.GlobalRaw)] = AdminUiPolicies.ConfigEditor, [typeof(EquipmentPage)] = AdminUiPolicies.ConfigEditor, [typeof(NewCluster)] = AdminUiPolicies.ConfigEditor, [typeof(ClusterEdit)] = AdminUiPolicies.ConfigEditor, @@ -54,8 +55,9 @@ public class PageAuthorizationGuardTests // ── FleetAdmin (1) ── [typeof(RoleGrants)] = AdminUiPolicies.FleetAdmin, - // ── AuthenticatedRead (16): dashboards, lists, live tails (read-only) ── + // ── AuthenticatedRead (17): dashboards, lists, live tails (read-only) + the dev ContextMenu demo ── [typeof(Home)] = AdminUiPolicies.AuthenticatedRead, + [typeof(global::ZB.MOM.WW.OtOpcUa.AdminUI.Components.Pages.Dev.ContextMenuDemo)] = AdminUiPolicies.AuthenticatedRead, [typeof(Fleet)] = AdminUiPolicies.AuthenticatedRead, [typeof(global::ZB.MOM.WW.OtOpcUa.AdminUI.Components.Pages.Hosts)] = AdminUiPolicies.AuthenticatedRead, [typeof(Alerts)] = AdminUiPolicies.AuthenticatedRead,