From 14003ab9b87b447cea5f96770f6fb58572831d29 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 12:09:14 -0400 Subject: [PATCH] feat(opcua): surgical RemoveVariableNode with NodeDeleted model-change (R2-07 T8) --- ...2-07-surgical-pure-adds-plan.md.tasks.json | 2 +- .../OtOpcUaNodeManager.cs | 79 ++++++- .../NodeManagerSurgicalRemoveTests.cs | 220 ++++++++++++++++++ 3 files changed, 295 insertions(+), 6 deletions(-) create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerSurgicalRemoveTests.cs diff --git a/archreview/plans/R2-07-surgical-pure-adds-plan.md.tasks.json b/archreview/plans/R2-07-surgical-pure-adds-plan.md.tasks.json index ee3c6b06..e77928e6 100644 --- a/archreview/plans/R2-07-surgical-pure-adds-plan.md.tasks.json +++ b/archreview/plans/R2-07-surgical-pure-adds-plan.md.tasks.json @@ -71,7 +71,7 @@ { "id": "T8", "subject": "Phase 2: OtOpcUaNodeManager.RemoveVariableNode + BuildNodesRemovedModelChange (NodeDeleted) (high-risk)", - "status": "pending", + "status": "completed", "blockedBy": [ "T7" ] diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index 91b4acb4..a09a7e58 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -1664,6 +1664,59 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 return e; } + /// Build (but do not report) the Part 3 GeneralModelChangeEvent announcing that the node + /// at was DELETED. Mirrors + /// exactly — the only differences are Verb = NodeDeleted and that the affected node has already + /// been dropped from the maps, so AffectedType resolves to (a valid Part 3 + /// "type not applicable"; clients re-browse the parent regardless). internal so a node-manager test + /// can assert the populated Changes structure at the nearest deterministic seam. + /// The node id of the deleted node. + /// A populated, unreported . + internal GeneralModelChangeEventState BuildNodesRemovedModelChange(string affectedNodeId) + { + var affected = new NodeId(affectedNodeId, NamespaceIndex); + var e = new GeneralModelChangeEventState(null); + e.Initialize( + SystemContext, + source: null, + severity: EventSeverity.Medium, + message: new LocalizedText($"Node deleted: {affected}")); + // Part 3 §8.7.4: emitted by the Server object — set SourceNode/SourceName to Server explicitly + // (mirrors BuildNodesAddedModelChange). + e.SetChildValue(SystemContext, BrowseNames.SourceNode, ObjectIds.Server, false); + e.SetChildValue(SystemContext, BrowseNames.SourceName, "Server", false); + var change = new ModelChangeStructureDataType + { + Affected = affected, + // The node is already gone from the maps, so its TypeDefinition is not applicable (Null). + AffectedType = ResolveAffectedTypeDefinition(affectedNodeId), + Verb = (byte)ModelChangeStructureVerbMask.NodeDeleted, + }; + e.SetChildValue(SystemContext, BrowseNames.Changes, new[] { change }, false); + return e; + } + + /// Report a pre-built OUTSIDE Lock — + /// Server.ReportEvent re-enters the server's own subscription/event path, so holding Lock + /// across it risks a lock-order inversion (mirrors / + /// ReportNodeShapeChangedEvent). Tolerant: swallow-and-log when eventing is disabled / there are no + /// monitored items / the server is shutting down — the node mutation already stands. + /// The pre-built event to report. + /// The affected node id (for the diagnostic only). + private void ReportModelChangeOutsideLock(GeneralModelChangeEventState e, string affectedNodeId) + { + try + { + Server.ReportEvent(SystemContext, e); + } + catch (Exception ex) + { +#pragma warning disable CS0618 // Utils.LogError is [Obsolete] in favour of an ITelemetryContext this manager doesn't carry. + Utils.LogError(ex, "OtOpcUaNodeManager: failed to report GeneralModelChangeEvent for {0}", affectedNodeId); +#pragma warning restore CS0618 + } + } + /// Resolve the TypeDefinition of a materialised node id from the live folder/variable maps for a /// model-change event's AffectedType; when the id is not registered. /// The folder-scoped node id whose TypeDefinition is wanted. @@ -1746,12 +1799,28 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 } } - // R2-07 Phase 2 (T7 placeholder) — surgical single-node / subtree removal. These return false (the - // rebuild-fallback contract) so the tree stays shippable at T7 while T8/T9/T10 implement the real - // in-place teardown (mirroring the per-node loops inside RebuildAddressSpace, scoped to the id/subtree, - // with a NodeDeleted model-change reported outside Lock). Implemented in T8/T9/T10. /// - public bool RemoveVariableNode(string variableNodeId) => false; + public bool RemoveVariableNode(string variableNodeId) + { + ArgumentException.ThrowIfNullOrEmpty(variableNodeId); + EnsureAddressSpaceCreated(); + + GeneralModelChangeEventState e; + lock (Lock) + { + // Unknown id ⇒ the node-manager maps drifted from what the planner believes; return false so the + // caller (AddressSpaceApplier) falls back to a full rebuild (resync). Mirrors the per-node + // teardown inside RebuildAddressSpace, scoped to this one id. + if (!_variables.TryRemove(variableNodeId, out var variable)) return false; + variable.Parent?.RemoveChild(variable); + PredefinedNodes?.Remove(variable.NodeId); + // Drop the historized-tagname registration alongside the variable it maps (Phase C parity). + _historizedTagnames.TryRemove(variableNodeId, out _); + e = BuildNodesRemovedModelChange(variableNodeId); + } + ReportModelChangeOutsideLock(e, variableNodeId); + return true; + } /// public bool RemoveAlarmConditionNode(string alarmNodeId) => false; diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerSurgicalRemoveTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerSurgicalRemoveTests.cs new file mode 100644 index 00000000..fd225779 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerSurgicalRemoveTests.cs @@ -0,0 +1,220 @@ +using Opc.Ua; +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests; + +/// +/// R2-07 Phase 2 (T8/T9/T10) — surgical IN-PLACE removal on the node manager: a single value variable +/// (), a single Part 9 alarm condition +/// (), and an equipment folder + its whole +/// descendant subtree with notifier demotion (). +/// Each removal detaches only the scoped node(s), cleans the matching maps, and raises a Part 3 +/// NodeDeleted model-change; an unknown id returns false (the caller falls back to a full rebuild). +/// +public sealed class NodeManagerSurgicalRemoveTests : IDisposable +{ + private static CancellationToken Ct => TestContext.Current.CancellationToken; + + private readonly string _pkiRoot = Path.Combine( + Path.GetTempPath(), + $"otopcua-surgical-remove-{Guid.NewGuid():N}"); + + // ---------- T8: RemoveVariableNode ---------- + + /// Ensure a variable then remove it: it disappears from the maps (TryGetVariable null, + /// VariableCount decremented), its historized-tagname registration is dropped, and the call returns + /// true. + [Trait("Category", "Unit")] + [Fact] + public async Task RemoveVariableNode_drops_variable_and_historian_registration() + { + var (host, server) = await BootAsync(); + var nm = server.NodeManager!; + nm.EnsureFolder("eq-1", parentNodeId: null, displayName: "Equipment 1"); + nm.EnsureVariable("eq-1/A", "eq-1", "A", "Float", writable: false, historianTagname: "Hist.A"); + nm.EnsureVariable("eq-1/B", "eq-1", "B", "Float", writable: false); + var countBefore = nm.VariableCount; + nm.TryGetHistorizedTagname("eq-1/A", out _).ShouldBeTrue(); + + nm.RemoveVariableNode("eq-1/A").ShouldBeTrue(); + + nm.TryGetVariable("eq-1/A").ShouldBeNull(); + nm.VariableCount.ShouldBe(countBefore - 1); + nm.TryGetHistorizedTagname("eq-1/A", out _).ShouldBeFalse(); + // The sibling variable is untouched. + nm.TryGetVariable("eq-1/B").ShouldNotBeNull(); + + await host.DisposeAsync(); + } + + /// Removing an unknown variable id returns false (map drift ⇒ caller rebuilds). + [Trait("Category", "Unit")] + [Fact] + public async Task RemoveVariableNode_unknown_id_returns_false() + { + var (host, server) = await BootAsync(); + var nm = server.NodeManager!; + + nm.RemoveVariableNode("eq-1/nope").ShouldBeFalse(); + + await host.DisposeAsync(); + } + + /// The built removed-node event announces the deleted node with verb NodeDeleted. + [Trait("Category", "Unit")] + [Fact] + public async Task Built_removed_event_announces_the_deleted_node_with_NodeDeleted_verb() + { + var (host, server) = await BootAsync(); + var nm = server.NodeManager!; + nm.EnsureFolder("eq-1", parentNodeId: null, displayName: "Equipment 1"); + nm.EnsureVariable("eq-1/A", "eq-1", "A", "Float", writable: false); + + var e = nm.BuildNodesRemovedModelChange("eq-1/A"); + + e.ShouldNotBeNull(); + e.Changes.ShouldNotBeNull(); + var changes = e.Changes.Value; + changes.Length.ShouldBe(1); + changes[0].Verb.ShouldBe((byte)ModelChangeStructureVerbMask.NodeDeleted); + + await host.DisposeAsync(); + } + + // ---------- T9: RemoveAlarmConditionNode ---------- + + /// Materialise a native alarm condition then remove it: it disappears from the condition map and + /// the native flag is cleared; unknown id returns false. + [Trait("Category", "Unit")] + [Fact] + public async Task RemoveAlarmConditionNode_drops_condition_and_native_flag() + { + var (host, server) = await BootAsync(); + var nm = server.NodeManager!; + nm.EnsureFolder("eq-1", parentNodeId: null, displayName: "Equipment 1"); + nm.MaterialiseAlarmCondition("eq-1/OverTemp", "eq-1", "OverTemp", "OffNormalAlarm", 700, isNative: true); + nm.TryGetAlarmCondition("eq-1/OverTemp").ShouldNotBeNull(); + nm.IsNativeAlarmNode("eq-1/OverTemp").ShouldBeTrue(); + + nm.RemoveAlarmConditionNode("eq-1/OverTemp").ShouldBeTrue(); + + nm.TryGetAlarmCondition("eq-1/OverTemp").ShouldBeNull(); + nm.IsNativeAlarmNode("eq-1/OverTemp").ShouldBeFalse(); + + nm.RemoveAlarmConditionNode("eq-1/OverTemp").ShouldBeFalse(); // already gone ⇒ false + + await host.DisposeAsync(); + } + + /// A scripted condition removes cleanly too (no native flag was set). + [Trait("Category", "Unit")] + [Fact] + public async Task RemoveAlarmConditionNode_removes_scripted_condition() + { + var (host, server) = await BootAsync(); + var nm = server.NodeManager!; + nm.EnsureFolder("eq-1", parentNodeId: null, displayName: "Equipment 1"); + nm.MaterialiseAlarmCondition("alm-1", "eq-1", "HighTemp", "OffNormalAlarm", 500, isNative: false); + + nm.RemoveAlarmConditionNode("alm-1").ShouldBeTrue(); + nm.TryGetAlarmCondition("alm-1").ShouldBeNull(); + + await host.DisposeAsync(); + } + + // ---------- T10: RemoveEquipmentSubtree ---------- + + /// Remove an equipment folder carrying a sub-folder, variables (one historized), and a condition: + /// every descendant disappears from every map, the notifier registration is demoted, and a SIBLING + /// equipment is fully intact. + [Trait("Category", "Unit")] + [Fact] + public async Task RemoveEquipmentSubtree_removes_all_descendants_and_leaves_siblings_intact() + { + var (host, server) = await BootAsync(); + var nm = server.NodeManager!; + + // Target equipment eq-1 with a sub-folder, two variables (one historized), and a native condition. + nm.EnsureFolder("eq-1", parentNodeId: null, displayName: "Equipment 1"); + nm.EnsureFolder("eq-1/Diag", parentNodeId: "eq-1", displayName: "Diag"); + nm.EnsureVariable("eq-1/A", "eq-1", "A", "Float", writable: false, historianTagname: "Hist.A"); + nm.EnsureVariable("eq-1/Diag/T", "eq-1/Diag", "T", "Float", writable: false); + nm.MaterialiseAlarmCondition("eq-1/OverTemp", "eq-1", "OverTemp", "OffNormalAlarm", 700, isNative: true); + + // Sibling equipment eq-2 that must survive untouched. + nm.EnsureFolder("eq-2", parentNodeId: null, displayName: "Equipment 2"); + nm.EnsureVariable("eq-2/S", "eq-2", "S", "Float", writable: false); + + nm.RemoveEquipmentSubtree("eq-1").ShouldBeTrue(); + + // Every eq-1 descendant is gone from every map. + nm.TryGetFolder("eq-1").ShouldBeNull(); + nm.TryGetFolder("eq-1/Diag").ShouldBeNull(); + nm.TryGetVariable("eq-1/A").ShouldBeNull(); + nm.TryGetVariable("eq-1/Diag/T").ShouldBeNull(); + nm.TryGetAlarmCondition("eq-1/OverTemp").ShouldBeNull(); + nm.TryGetHistorizedTagname("eq-1/A", out _).ShouldBeFalse(); + nm.IsNativeAlarmNode("eq-1/OverTemp").ShouldBeFalse(); + + // Sibling eq-2 is fully intact. + nm.TryGetFolder("eq-2").ShouldNotBeNull(); + nm.TryGetVariable("eq-2/S").ShouldNotBeNull(); + + // Re-materialising an alarm under eq-2 still works (the notifier machinery was not corrupted by the + // eq-1 demotion) — proves no orphaned root-notifier ref broke the event path. + Should.NotThrow(() => nm.MaterialiseAlarmCondition("eq-2/Alm", "eq-2", "Alm", "OffNormalAlarm", 300, isNative: false)); + + await host.DisposeAsync(); + } + + /// Removing an unknown equipment id returns false (map drift ⇒ caller rebuilds). + [Trait("Category", "Unit")] + [Fact] + public async Task RemoveEquipmentSubtree_unknown_id_returns_false() + { + var (host, server) = await BootAsync(); + var nm = server.NodeManager!; + + nm.RemoveEquipmentSubtree("eq-nope").ShouldBeFalse(); + + await host.DisposeAsync(); + } + + private async Task<(OpcUaApplicationHost Host, OtOpcUaSdkServer Server)> BootAsync() + { + var host = new OpcUaApplicationHost( + new OpcUaApplicationHostOptions + { + ApplicationName = "OtOpcUa.SurgicalRemoveTest", + ApplicationUri = $"urn:OtOpcUa.SurgicalRemoveTest:{Guid.NewGuid():N}", + OpcUaPort = AllocateFreePort(), + PublicHostname = "localhost", + PkiStoreRoot = _pkiRoot, + }, + Microsoft.Extensions.Logging.Abstractions.NullLogger.Instance); + + var server = new OtOpcUaSdkServer(); + await host.StartAsync(server, Ct); + return (host, server); + } + + private static int AllocateFreePort() + { + using var listener = new System.Net.Sockets.TcpListener(System.Net.IPAddress.Loopback, 0); + listener.Start(); + var port = ((System.Net.IPEndPoint)listener.LocalEndpoint).Port; + listener.Stop(); + return port; + } + + /// Cleans up the PKI root directory. + public void Dispose() + { + if (Directory.Exists(_pkiRoot)) + { + try { Directory.Delete(_pkiRoot, recursive: true); } + catch { /* best-effort cleanup */ } + } + } +}