From fb094fa5662b3a42bedce0a73f2af52c6fc86530 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 03:21:03 -0400 Subject: [PATCH] feat(opcua): FB-7 surgical DataType/array-shape in-place tag writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Widen the F10b surgical address-space path so a changed equipment tag whose only differences are DataType / IsArray / ArrayLength (on top of the existing Writable / Historizing) is applied IN PLACE on the live node instead of forcing a full RebuildAddressSpace that drops every client's subscriptions server-wide. ISurgicalAddressSpaceSink.UpdateTagAttributes gains (dataType, isArray, arrayLength); the DeferredAddressSpaceSink wrapper forwards all six args (the prod-inertness seam). OtOpcUaNodeManager swaps DataType + ValueRank + ArrayDimensions in place, and on a real shape change (a) resets the node to BadWaitingForInitialData so no stale wrong-typed value is exposed (closes the prior brief-value-type-mismatch objection) and (b) raises a Part 3 GeneralModelChangeEvent (verb=DataTypeChanged) so model-aware clients re-read the definition. A Writable/Historizing-only change leaves the shape untouched (no reset, no model event) — original behaviour preserved byte-for-byte. AddressSpaceApplier.TagDeltaIsSurgicalEligible adds the three shape fields to its whitelist; FullName/Name/DriverInstanceId/alarm differences still rebuild. Tests: new NodeManagerSurgicalShapeUpdateTests boots a real server to prove the in-place swap + value reset + the no-reset backward-compat path + the model-event builder; AddressSpaceApplierTests invert the two former DataType/IsArray-rebuild cases to surgical and assert the shape args land; DeferredAddressSpaceSinkTests assert the shape args forward. 273/273 OpcUaServer.Tests green; full solution builds. --- .../OpcUa/DeferredAddressSpaceSink.cs | 11 +- .../OpcUa/ISurgicalAddressSpaceSink.cs | 25 +- .../AddressSpaceApplier.cs | 40 ++-- .../OtOpcUaNodeManager.cs | 117 ++++++++- .../SdkAddressSpaceSink.cs | 12 +- .../AddressSpaceApplierTests.cs | 76 +++--- .../DeferredAddressSpaceSinkTests.cs | 25 +- .../NodeManagerSurgicalShapeUpdateTests.cs | 222 ++++++++++++++++++ 8 files changed, 458 insertions(+), 70 deletions(-) create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerSurgicalShapeUpdateTests.cs diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/DeferredAddressSpaceSink.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/DeferredAddressSpaceSink.cs index 8f714a31..9ba6f436 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/DeferredAddressSpaceSink.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/DeferredAddressSpaceSink.cs @@ -74,12 +74,17 @@ public sealed class DeferredAddressSpaceSink : IOpcUaAddressSpaceSink, ISurgical /// surgical capability. Returns false otherwise — before the real SdkAddressSpaceSink is /// swapped in (inner is still the null sink), or any inner sink that isn't surgical — so the caller /// (AddressSpaceApplier) falls back to a full rebuild. Without this forward the surgical optimization is - /// inert on every driver-role host, because actors inject THIS wrapper, not the inner sink. + /// inert on every driver-role host, because actors inject THIS wrapper, not the inner sink. ALL six args + /// (including the FB-7 DataType/array-shape ones) MUST be forwarded — a partial forward silently drops the + /// shape update on every driver-role host. /// The node ID of the variable to update in place. /// Whether the node should be read/write. /// null ⇒ not historized; non-null ⇒ Historizing + historian binding. + /// The OPC UA built-in data type name to apply in place. + /// When true the node becomes a 1-D array; when false scalar. + /// The declared length of the 1-D array when is true. /// True when the inner sink applied the update; false when it lacks the capability or the node is missing. - public bool UpdateTagAttributes(string variableNodeId, bool writable, string? historianTagname) + public bool UpdateTagAttributes(string variableNodeId, bool writable, string? historianTagname, string dataType, bool isArray, uint? arrayLength) => _inner is ISurgicalAddressSpaceSink surgical - && surgical.UpdateTagAttributes(variableNodeId, writable, historianTagname); + && surgical.UpdateTagAttributes(variableNodeId, writable, historianTagname, dataType, isArray, arrayLength); } diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/ISurgicalAddressSpaceSink.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/ISurgicalAddressSpaceSink.cs index 6a6f5592..1de37aed 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/ISurgicalAddressSpaceSink.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/ISurgicalAddressSpaceSink.cs @@ -2,12 +2,25 @@ namespace ZB.MOM.WW.OtOpcUa.Commons.OpcUa; /// Optional capability on an address-space sink: surgical in-place attribute updates on an /// EXISTING variable node, used by AddressSpaceApplier to avoid a full RebuildAddressSpace for pure-property -/// tag changes (Writable / Historizing). A sink that does not implement it ⇒ caller falls back to a -/// full rebuild (safe default). +/// tag changes (Writable / Historizing / DataType / array-shape). A sink that does not implement it ⇒ caller +/// falls back to a full rebuild (safe default). public interface ISurgicalAddressSpaceSink { - /// Update an existing variable node's Writable (AccessLevel + inbound-write handler) and - /// Historizing (+ historian-tagname binding) IN PLACE, notifying subscribers (ClearChangeMasks) - /// without a rebuild. Returns false if the node does not exist (caller should rebuild instead). - bool UpdateTagAttributes(string variableNodeId, bool writable, string? historianTagname); + /// Update an existing variable node's surgically-updatable attributes IN PLACE, notifying + /// subscribers (ClearChangeMasks) without a rebuild — so client MonitoredItems on the node survive. + /// Covers Writable (AccessLevel + inbound-write handler), Historizing (+ historian-tagname binding), + /// and the node's presentation shape: (the OPC UA DataType) and + /// / (ValueRank + ArrayDimensions). When the + /// shape actually changes, the implementation resets the node's value to BadWaitingForInitialData + /// (no stale wrong-typed value is exposed until the driver republishes) and raises a Part 3 + /// GeneralModelChangeEvent (verb=DataTypeChanged) so model-aware clients re-read the node definition. + /// Returns false if the node does not exist (caller should rebuild instead). + /// The folder-scoped node id of the variable to update in place. + /// When true the node becomes read/write (with the inbound-write handler); otherwise read-only. + /// null ⇒ not historized; non-null ⇒ Historizing with the HistoryRead bit and tagname binding. + /// The OPC UA built-in data type name (e.g. "Boolean", "Int32", "Float"). + /// When true the node is a 1-D array (ValueRank=OneDimension); when false scalar. + /// The declared length of the 1-D array when is true; ignored for scalars. + /// True when the in-place update was applied; false when the node is missing (caller rebuilds). + bool UpdateTagAttributes(string variableNodeId, bool writable, string? historianTagname, string dataType, bool isArray, uint? arrayLength); } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs index 17afaedf..cdcd9f74 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/AddressSpaceApplier.cs @@ -95,13 +95,15 @@ public sealed class AddressSpaceApplier // respawn (DriverHostActor → ApplyVirtualTags), so it skips the rebuild and PRESERVES every // client's server-wide subscriptions. Any structural / node-affecting vtag change (Name/ // FolderPath/DataType) — or any non-vtag change anywhere — still forces a full rebuild. - // F10b (surgical tag write): a CHANGED equipment tag whose ONLY differences are Writable / - // IsHistorized / HistorianTagname (a plain value variable — no alarm condition node) can be - // updated IN PLACE on the existing node via ISurgicalAddressSpaceSink.UpdateTagAttributes - // (see TagDeltaIsSurgicalEligible), again avoiding the full rebuild and preserving subscriptions. - // Any other tag difference (DataType/IsArray/ArrayLength/FullName/identity/alarm) — or a sink - // that lacks the surgical capability, or a node that turns out missing — falls back to a full - // rebuild (safe default). + // F10b + FB-7 (surgical tag write): a CHANGED equipment tag whose ONLY differences are Writable / + // IsHistorized / HistorianTagname / DataType / IsArray / ArrayLength (a plain value variable — no + // alarm condition node) can be updated IN PLACE on the existing node via + // ISurgicalAddressSpaceSink.UpdateTagAttributes (see TagDeltaIsSurgicalEligible), avoiding the full + // rebuild and preserving subscriptions. The shape fields (DataType/IsArray/ArrayLength) are now + // surgical because the sink swaps DataType + ValueRank + ArrayDimensions in place and raises a + // GeneralModelChangeEvent. Any other tag difference (FullName/Name/DriverInstanceId/identity/alarm) — + // or a sink that lacks the surgical capability, or a node that turns out missing — falls back to a + // full rebuild (safe default). var structuralRebuild = plan.AddedEquipment.Count > 0 || plan.RemovedEquipment.Count > 0 || plan.ChangedEquipment.Count > 0 || plan.AddedAlarms.Count > 0 || plan.RemovedAlarms.Count > 0 || plan.ChangedAlarms.Count > 0 || @@ -125,15 +127,16 @@ public sealed class AddressSpaceApplier var allApplied = true; foreach (var d in surgicalTagDeltas) { - // Compute the node id + writable + historian EXACTLY as MaterialiseEquipmentTags would - // so the in-place update matches what a rebuild would have produced. + // Compute the node id + writable + historian + shape EXACTLY as MaterialiseEquipmentTags + // would so the in-place update matches what a rebuild would have produced. Array tags are + // forced read-only (same as EnsureVariable: the driver write path doesn't handle arrays). var nodeId = EquipmentNodeIds.Variable(d.Current.EquipmentId, d.Current.FolderPath, d.Current.Name); var writable = d.Current.Writable && !d.Current.IsArray; var historian = d.Current.IsHistorized ? (string.IsNullOrWhiteSpace(d.Current.HistorianTagname) ? d.Current.FullName : d.Current.HistorianTagname) : null; bool ok; - try { ok = surgical.UpdateTagAttributes(nodeId, writable, historian); } + try { ok = surgical.UpdateTagAttributes(nodeId, writable, historian, d.Current.DataType, d.Current.IsArray, d.Current.ArrayLength); } catch (Exception ex) { _logger.LogError(ex, "AddressSpaceApplier: surgical UpdateTagAttributes threw for {Node}", nodeId); @@ -389,11 +392,15 @@ public sealed class AddressSpaceApplier Historize = d.Current.Historize, }).Equals(d.Current); - // F10b: a CHANGED equipment tag whose ONLY differences are Writable / IsHistorized / HistorianTagname - // (a plain value variable — no alarm condition node) can be updated IN PLACE on the existing node via - // ISurgicalAddressSpaceSink.UpdateTagAttributes, avoiding a full rebuild (preserving subscriptions). - // DataType / IsArray / ArrayLength / FullName / DriverInstanceId / identity / alarm differences fall - // through to a rebuild — the override-unequal default also covers any future field. + // F10b + FB-7: a CHANGED equipment tag whose ONLY differences are Writable / IsHistorized / + // HistorianTagname / DataType / IsArray / ArrayLength (a plain value variable — no alarm condition node) + // can be updated IN PLACE on the existing node via ISurgicalAddressSpaceSink.UpdateTagAttributes, + // avoiding a full rebuild (preserving subscriptions). The presentation-shape fields (DataType / IsArray / + // ArrayLength) join the whitelist now that the surgical sink swaps DataType + ValueRank + ArrayDimensions + // in place (and raises a GeneralModelChangeEvent). FullName / DriverInstanceId / Name / identity / alarm + // differences still fall through to a rebuild — FullName/DriverInstanceId re-route the node to a different + // driver point, Name re-derives the NodeId, and an alarm flip turns the node into a Part 9 condition. The + // override-unequal default also covers any future field. private static bool TagDeltaIsSurgicalEligible(AddressSpacePlan.EquipmentTagDelta d) => d.Previous.Alarm is null && d.Current.Alarm is null && (d.Previous with @@ -401,6 +408,9 @@ public sealed class AddressSpaceApplier Writable = d.Current.Writable, IsHistorized = d.Current.IsHistorized, HistorianTagname = d.Current.HistorianTagname, + DataType = d.Current.DataType, + IsArray = d.Current.IsArray, + ArrayLength = d.Current.ArrayLength, }).Equals(d.Current); /// The "no-event" condition state written to a removed equipment / alarm node before the diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index cdd51cd9..01995fd9 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -1386,12 +1386,38 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 } /// F10b surgical counterpart of : update an EXISTING tag variable's - /// Writable (AccessLevel + handler) and Historizing (+ historian-tagname - /// binding) in place and notify subscribers, WITHOUT a rebuild — so client MonitoredItems on the node - /// survive. Returns false when the node id is unknown (caller falls back to a full rebuild). - public bool UpdateTagAttributes(string variableNodeId, bool writable, string? historianTagname) + /// Writable (AccessLevel + handler), Historizing (+ historian-tagname + /// binding), and presentation shape ( → DataType; + /// / → ValueRank + ArrayDimensions) + /// in place and notify subscribers, WITHOUT a rebuild — so client MonitoredItems on the node survive. + /// Returns false when the node id is unknown (caller falls back to a full rebuild). + /// + /// FB-7: when the shape (DataType / ValueRank / ArrayDimensions) ACTUALLY changes, two extra things + /// happen. (1) The node's value is reset to BadWaitingForInitialData (Value=null) so no value + /// still typed to the OLD DataType is exposed between this update and the driver's next publish — the + /// same fresh-node state creates, which closes the "brief value-type + /// mismatch" window. (2) A Part 3 GeneralModelChangeEvent (verb=DataTypeChanged) is + /// raised from the Server object so model-aware clients re-read the node definition. A Writable / + /// Historizing-only change (DataType + array-ness unchanged) skips both — its value stays valid and + /// there is no model change to report (preserves the original surgical behaviour exactly). + /// + /// + /// The model-change event is built under Lock but reported AFTER the lock is released — + /// mirroring : Server.ReportEvent re-enters the + /// server's own subscription/event path, so holding the node Lock across it risks a + /// lock-order inversion. + /// + /// The folder-scoped node id of the variable to update in place. + /// When true the node becomes read/write with the inbound-write handler; otherwise read-only. + /// null ⇒ not historized; non-null ⇒ Historizing with the HistoryRead bit and tagname binding. + /// The OPC UA built-in data type name to apply (e.g. "Boolean", "Int32", "Float"). + /// When true the node becomes a 1-D array (ValueRank=OneDimension); when false scalar. + /// The declared length of the 1-D array when is true; ignored for scalars. + /// True when the in-place update was applied; false when the node id is unknown. + public bool UpdateTagAttributes(string variableNodeId, bool writable, string? historianTagname, string dataType, bool isArray, uint? arrayLength) { ArgumentException.ThrowIfNullOrEmpty(variableNodeId); + BaseDataVariableState? shapeChangedNode = null; lock (Lock) { if (!_variables.TryGetValue(variableNodeId, out var v)) return false; @@ -1403,8 +1429,89 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 v.OnWriteValue = writable ? OnEquipmentTagWrite : null; if (historized) _historizedTagnames[variableNodeId] = historianTagname!; else _historizedTagnames.TryRemove(variableNodeId, out _); + + // FB-7: swap DataType + ValueRank + ArrayDimensions in place, but only when they actually differ + // from the live node — a Writable/Historizing-only change leaves the shape untouched (no value + // reset, no model-change event), preserving the original surgical behaviour byte-for-byte. + var newDataType = ResolveBuiltInDataType(dataType); + var newValueRank = isArray ? ValueRanks.OneDimension : ValueRanks.Scalar; + if (v.DataType != newDataType || v.ValueRank != newValueRank || ArrayLengthDiffers(v, isArray, arrayLength)) + { + v.DataType = newDataType; + v.ValueRank = newValueRank; + v.ArrayDimensions = isArray + ? new ReadOnlyList(new UInt32Collection(new[] { arrayLength ?? 0u })) + : null; + // Drop the stale (old-typed) value — mirrors EnsureVariable's fresh-node state so a client + // never reads a value whose runtime type contradicts the just-changed DataType. + v.Value = null; + v.StatusCode = StatusCodes.BadWaitingForInitialData; + v.Timestamp = DateTime.MinValue; + shapeChangedNode = v; + } + v.ClearChangeMasks(SystemContext, includeChildren: false); - return true; + } + // Report OUTSIDE Lock (see the method remarks) — only when the shape actually changed. + if (shapeChangedNode is not null) ReportNodeShapeChangedEvent(shapeChangedNode); + return true; + } + + /// True when the node's current 1-D array length differs from the requested one (both must be + /// arrays for the comparison to matter — a scalar↔array transition is already caught by the ValueRank + /// check in , so this only fires for an array-to-array length edit). + private static bool ArrayLengthDiffers(BaseDataVariableState v, bool isArray, uint? arrayLength) + { + if (!isArray) return false; // scalar target ⇒ ValueRank check owns the diff + if (v.ArrayDimensions is not { Count: > 0 }) return true; // was scalar/empty ⇒ shape differs + return v.ArrayDimensions[0] != (arrayLength ?? 0u); + } + + /// Build (but do not report) the Part 3 GeneralModelChangeEvent announcing that + /// 's definition changed (DataType / ValueRank / ArrayDimensions). Verb = + /// DataTypeChanged — the model-change verb the SDK exposes for an attribute-shape change (there is + /// no separate ValueRank verb). internal (not private) so a node-manager test can assert the + /// populated Changes structure at the nearest deterministic seam (the end-to-end Server.ReportEvent + /// dispatch would need a subscribed event monitored-item to observe). + /// The variable whose shape changed. + /// A populated, unreported . + internal GeneralModelChangeEventState BuildNodeShapeChangedEvent(BaseDataVariableState variable) + { + var e = new GeneralModelChangeEventState(null); + e.Initialize( + SystemContext, + source: null, + severity: EventSeverity.Medium, + message: new LocalizedText($"Node {variable.NodeId} definition changed (DataType/ValueRank)")); + var change = new ModelChangeStructureDataType + { + Affected = variable.NodeId, + AffectedType = variable.TypeDefinitionId, + Verb = (byte)ModelChangeStructureVerbMask.DataTypeChanged, + }; + // SetChildValue lazily creates + sets the Changes property (same pattern the audit-event builder + // relies on for its child PropertyStates). + e.SetChildValue(SystemContext, BrowseNames.Changes, new[] { change }, false); + return e; + } + + /// Report the built GeneralModelChangeEvent through the SDK server, guarding against the + /// event path being disabled / having no subscribers / a transient failure — a surprise here MUST NOT + /// break the in-place update that already happened (mirrors ). + /// The variable whose shape changed. + private void ReportNodeShapeChangedEvent(BaseDataVariableState variable) + { + try + { + Server.ReportEvent(SystemContext, BuildNodeShapeChangedEvent(variable)); + } + catch (Exception ex) + { + // Model-change reporting disabled / no monitored items / server shutting down ⇒ ReportEvent may + // no-op or throw; either way the in-place update already stands. Log to the SDK trace, don't rethrow. +#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}", variable.NodeId); +#pragma warning restore CS0618 } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/SdkAddressSpaceSink.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/SdkAddressSpaceSink.cs index 709b1828..85f7238a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/SdkAddressSpaceSink.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/SdkAddressSpaceSink.cs @@ -65,13 +65,17 @@ public sealed class SdkAddressSpaceSink : IOpcUaAddressSpaceSink, ISurgicalAddre public void EnsureVariable(string variableNodeId, string? parentFolderNodeId, string displayName, string dataType, bool writable, string? historianTagname = null, bool isArray = false, uint? arrayLength = null) => _nodeManager.EnsureVariable(variableNodeId, parentFolderNodeId, displayName, dataType, writable, historianTagname, isArray, arrayLength); - /// F10b: surgically update an existing variable node's Writable + Historizing in place - /// (no rebuild). Returns false when the node does not exist (caller falls back to a full rebuild). + /// F10b: surgically update an existing variable node's Writable + Historizing + presentation + /// shape (DataType / array-ness) in place (no rebuild). Returns false when the node does not exist + /// (caller falls back to a full rebuild). /// The variable node identifier. /// When true the node becomes read/write with the inbound-write handler; otherwise read-only. /// null ⇒ not historized; non-null ⇒ Historizing with the HistoryRead bit and tagname binding. - public bool UpdateTagAttributes(string variableNodeId, bool writable, string? historianTagname) - => _nodeManager.UpdateTagAttributes(variableNodeId, writable, historianTagname); + /// The OPC UA built-in data type name to apply in place. + /// When true the node becomes a 1-D array; when false scalar. + /// The declared length of the 1-D array when is true. + public bool UpdateTagAttributes(string variableNodeId, bool writable, string? historianTagname, string dataType, bool isArray, uint? arrayLength) + => _nodeManager.UpdateTagAttributes(variableNodeId, writable, historianTagname, dataType, isArray, arrayLength); /// Rebuilds the entire OPC UA address space. public void RebuildAddressSpace() => _nodeManager.RebuildAddressSpace(); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs index b11d80a8..5650d42e 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/AddressSpaceApplierTests.cs @@ -616,10 +616,12 @@ public sealed class AddressSpaceApplierTests sink.RebuildCalls.ShouldBe(1); } - /// H1a — a deploy that ONLY changes an existing equipment tag (e.g. flips its dataType or - /// Writable bit) must rebuild the address space. The planner diffs the tag into - /// ChangedEquipmentTags with no Added/Removed of anything else; the applier must still drive - /// exactly one rebuild so the running server drops the stale node and re-materialises it. + /// H1a — a deploy that ONLY changes an existing equipment tag in a NON-surgical way (here the + /// driver-side FullName re-routes to a different point) must rebuild the address space. The planner + /// diffs the tag into ChangedEquipmentTags with no Added/Removed of anything else; the applier must + /// still drive exactly one rebuild so the running server drops the stale node and re-materialises it. + /// (Surgically-applicable tag changes — Writable/Historizing/DataType/array-shape — take the in-place path + /// instead; those are covered by the F10b + FB-7 surgical tests.) [Fact] public void Changed_equipment_tags_only_trigger_rebuild() { @@ -628,9 +630,9 @@ public sealed class AddressSpaceApplierTests var previous = CompositionWithTags( new EquipmentTagPlan("tag-1", "eq-1", "drv", FolderPath: "", Name: "Speed", DataType: "Float", FullName: "40001", Writable: false, Alarm: null)); - // Same tag id, but DataType + Writable flipped — the planner classifies this as a change. + // Same tag id, but the driver-side FullName flipped — a non-surgical change, so the applier rebuilds. var next = CompositionWithTags( - new EquipmentTagPlan("tag-1", "eq-1", "drv", FolderPath: "", Name: "Speed", DataType: "Int32", FullName: "40001", Writable: true, Alarm: null)); + new EquipmentTagPlan("tag-1", "eq-1", "drv", FolderPath: "", Name: "Speed", DataType: "Float", FullName: "40002", Writable: false, Alarm: null)); var plan = AddressSpacePlanner.Compute(previous, next); @@ -815,9 +817,9 @@ public sealed class AddressSpaceApplierTests } /// F10b — the skip is ONLY for a node-irrelevant vtag edit that is the SOLE change. A - /// node-irrelevant Expression-only vtag edit MIXED with any other change (here a changed equipment - /// tag) must still rebuild — the rebuild is forced by the OTHER change, and the running server gets - /// its single rebuild as before. + /// node-irrelevant Expression-only vtag edit MIXED with another NON-surgical change (here a changed + /// equipment tag whose driver-side FullName re-routes) must still rebuild — the rebuild is forced + /// by the OTHER change, and the running server gets its single rebuild as before. [Fact] public void Node_irrelevant_vtag_edit_mixed_with_another_change_still_rebuilds() { @@ -837,13 +839,13 @@ public sealed class AddressSpaceApplierTests Expression: "ctx.GetTag(\"a\") * 60", DependencyRefs: new[] { "a" }), }, }; - // Expression-only vtag edit (node-irrelevant) AND a node-affecting tag DataType flip. + // Expression-only vtag edit (node-irrelevant) AND a non-surgical tag change (FullName re-route). var next = new AddressSpaceComposition( Array.Empty(), Array.Empty(), Array.Empty()) { EquipmentTags = new[] { - new EquipmentTagPlan("tag-1", "eq-1", "drv", FolderPath: "", Name: "Speed", DataType: "Int32", FullName: "40001", Writable: false, Alarm: null), + new EquipmentTagPlan("tag-1", "eq-1", "drv", FolderPath: "", Name: "Speed", DataType: "Float", FullName: "40002", Writable: false, Alarm: null), }, EquipmentVirtualTags = new[] { @@ -1157,17 +1159,19 @@ public sealed class AddressSpaceApplierTests sink.SurgicalCalls.ShouldHaveSingleItem().Historian.ShouldBe("WW.New"); // override verbatim } - /// F10b safe-default — a tag delta whose DataType changed is NOT surgical-eligible (the - /// node's value type would differ), so the applier must rebuild and make NO surgical call. + /// FB-7 — a tag delta whose DataType changed is now surgical-eligible: the sink swaps the + /// node's DataType in place (and raises a GeneralModelChangeEvent), so the applier SKIPS the rebuild and + /// makes exactly one surgical call carrying the NEW DataType. Here Writable also flips, which the same + /// in-place update applies. Subscriptions are preserved. [Fact] - public void Changed_tag_data_type_change_rebuilds_and_no_surgical_call() + public void Changed_tag_data_type_change_skips_rebuild_and_updates_in_place() { var sink = new RecordingSink(); var applier = new AddressSpaceApplier(sink, NullLogger.Instance); var previous = CompositionWithTags( new EquipmentTagPlan("tag-1", "eq-1", "drv", FolderPath: "", Name: "Speed", DataType: "Float", FullName: "40001", Writable: false, Alarm: null)); - // DataType flips AND Writable flips — DataType is node-affecting, so this must rebuild. + // DataType flips Float → Int32 AND Writable flips false → true — both are now surgically applied. var next = CompositionWithTags( new EquipmentTagPlan("tag-1", "eq-1", "drv", FolderPath: "", Name: "Speed", DataType: "Int32", FullName: "40001", Writable: true, Alarm: null)); @@ -1176,15 +1180,22 @@ public sealed class AddressSpaceApplierTests var outcome = applier.Apply(plan); - outcome.RebuildCalled.ShouldBeTrue(); - sink.RebuildCalls.ShouldBe(1); - sink.SurgicalCalls.ShouldBeEmpty(); + outcome.RebuildCalled.ShouldBeFalse(); + sink.RebuildCalls.ShouldBe(0); // NO rebuild — subscriptions preserved + var call = sink.SurgicalCalls.ShouldHaveSingleItem(); + call.NodeId.ShouldBe(EquipmentNodeIds.Variable("eq-1", "", "Speed")); + call.DataType.ShouldBe("Int32"); // the NEW DataType + call.Writable.ShouldBeTrue(); // the NEW Writable, applied in the same call + call.IsArray.ShouldBeFalse(); + outcome.ChangedNodes.ShouldBe(1); } - /// F10b safe-default — a tag delta whose IsArray flag changed is NOT surgical-eligible - /// (array-ness drives ValueRank/ArrayDimensions on the node), so the applier rebuilds. + /// FB-7 — a tag delta whose IsArray flag flips scalar → array is now surgical-eligible: + /// the sink swaps ValueRank + ArrayDimensions in place, so the applier skips the rebuild and the surgical + /// call carries the new array shape. An array tag is forced read-only (matching EnsureVariable), so the + /// surgical Writable is false even though the tag stays non-writable here. [Fact] - public void Changed_tag_is_array_change_rebuilds() + public void Changed_tag_is_array_change_skips_rebuild_and_updates_in_place() { var sink = new RecordingSink(); var applier = new AddressSpaceApplier(sink, NullLogger.Instance); @@ -1201,9 +1212,13 @@ public sealed class AddressSpaceApplierTests var outcome = applier.Apply(plan); - outcome.RebuildCalled.ShouldBeTrue(); - sink.RebuildCalls.ShouldBe(1); - sink.SurgicalCalls.ShouldBeEmpty(); + outcome.RebuildCalled.ShouldBeFalse(); + sink.RebuildCalls.ShouldBe(0); + var call = sink.SurgicalCalls.ShouldHaveSingleItem(); + call.IsArray.ShouldBeTrue(); // the NEW array shape + call.ArrayLength.ShouldBe(16u); + call.DataType.ShouldBe("Int16"); // element type unchanged + call.Writable.ShouldBeFalse(); // array tag forced read-only } /// F10b safe-default — a tag delta whose driver-side FullName changed is NOT @@ -1499,10 +1514,10 @@ public sealed class AddressSpaceApplierTests private sealed class RecordingSink : IOpcUaAddressSpaceSink, ISurgicalAddressSpaceSink { - /// Gets the queue of surgical in-place tag-attribute update calls (F10b). - public ConcurrentQueue<(string NodeId, bool Writable, string? Historian)> SurgicalQueue { get; } = new(); + /// Gets the queue of surgical in-place tag-attribute update calls (F10b + FB-7). + public ConcurrentQueue<(string NodeId, bool Writable, string? Historian, string DataType, bool IsArray, uint? ArrayLength)> SurgicalQueue { get; } = new(); /// Gets the list of recorded surgical in-place tag-attribute update calls. - public List<(string NodeId, bool Writable, string? Historian)> SurgicalCalls => SurgicalQueue.ToList(); + public List<(string NodeId, bool Writable, string? Historian, string DataType, bool IsArray, uint? ArrayLength)> SurgicalCalls => SurgicalQueue.ToList(); /// When false, reports the node missing (returns false), /// driving the applier's rebuild fallback. Defaults to true (node present, update succeeds). public bool SurgicalReturns { get; init; } = true; @@ -1511,9 +1526,12 @@ public sealed class AddressSpaceApplierTests /// The variable node ID to update in place. /// The new Writable (AccessLevel) for the node. /// The resolved historian tagname (null ⇒ not historized). - public bool UpdateTagAttributes(string variableNodeId, bool writable, string? historianTagname) + /// The new OPC UA data type name to apply in place. + /// The new array-ness of the node. + /// The new 1-D array length when is true. + public bool UpdateTagAttributes(string variableNodeId, bool writable, string? historianTagname, string dataType, bool isArray, uint? arrayLength) { - SurgicalQueue.Enqueue((variableNodeId, writable, historianTagname)); + SurgicalQueue.Enqueue((variableNodeId, writable, historianTagname, dataType, isArray, arrayLength)); return SurgicalReturns; } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/DeferredAddressSpaceSinkTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/DeferredAddressSpaceSinkTests.cs index ecd9a2ef..f7727d5d 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/DeferredAddressSpaceSinkTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/DeferredAddressSpaceSinkTests.cs @@ -94,13 +94,19 @@ public sealed class DeferredAddressSpaceSinkTests var inner = new SurgicalRecordingSink { Result = true }; deferred.SetSink(inner); - ((ISurgicalAddressSpaceSink)deferred).UpdateTagAttributes("v-1", writable: true, historianTagname: "MyTag.PV") + ((ISurgicalAddressSpaceSink)deferred).UpdateTagAttributes("v-1", writable: true, historianTagname: "MyTag.PV", + dataType: "Int32", isArray: true, arrayLength: 8u) .ShouldBeTrue(); var call = inner.SurgicalCalls.ShouldHaveSingleItem(); call.NodeId.ShouldBe("v-1"); call.Writable.ShouldBeTrue(); call.Historian.ShouldBe("MyTag.PV"); + // FB-7: the DataType/array-shape args must forward verbatim too — a partial forward would silently + // drop the shape update on every driver-role host. + call.DataType.ShouldBe("Int32"); + call.IsArray.ShouldBeTrue(); + call.ArrayLength.ShouldBe(8u); } /// The surgical forward returns the inner's own result (false ⇒ node missing) so the caller @@ -111,7 +117,8 @@ public sealed class DeferredAddressSpaceSinkTests var deferred = new DeferredAddressSpaceSink(); deferred.SetSink(new SurgicalRecordingSink { Result = false }); - ((ISurgicalAddressSpaceSink)deferred).UpdateTagAttributes("v-1", writable: false, historianTagname: null) + ((ISurgicalAddressSpaceSink)deferred).UpdateTagAttributes("v-1", writable: false, historianTagname: null, + dataType: "Float", isArray: false, arrayLength: null) .ShouldBeFalse(); } @@ -122,11 +129,13 @@ public sealed class DeferredAddressSpaceSinkTests public void UpdateTagAttributes_returns_false_when_inner_is_not_surgical() { var deferred = new DeferredAddressSpaceSink(); // default inner = null sink (not surgical) - ((ISurgicalAddressSpaceSink)deferred).UpdateTagAttributes("v-1", writable: true, historianTagname: null) + ((ISurgicalAddressSpaceSink)deferred).UpdateTagAttributes("v-1", writable: true, historianTagname: null, + dataType: "Float", isArray: false, arrayLength: null) .ShouldBeFalse(); deferred.SetSink(new RecordingSink()); // a non-surgical inner - ((ISurgicalAddressSpaceSink)deferred).UpdateTagAttributes("v-1", writable: true, historianTagname: null) + ((ISurgicalAddressSpaceSink)deferred).UpdateTagAttributes("v-1", writable: true, historianTagname: null, + dataType: "Float", isArray: false, arrayLength: null) .ShouldBeFalse(); } @@ -175,13 +184,13 @@ public sealed class DeferredAddressSpaceSinkTests { /// Gets or sets the value returns. public bool Result { get; set; } = true; - /// Gets the recorded surgical calls. - public List<(string NodeId, bool Writable, string? Historian)> SurgicalCalls { get; } = new(); + /// Gets the recorded surgical calls (incl. the FB-7 DataType/array-shape args). + public List<(string NodeId, bool Writable, string? Historian, string DataType, bool IsArray, uint? ArrayLength)> SurgicalCalls { get; } = new(); /// - public bool UpdateTagAttributes(string variableNodeId, bool writable, string? historianTagname) + public bool UpdateTagAttributes(string variableNodeId, bool writable, string? historianTagname, string dataType, bool isArray, uint? arrayLength) { - SurgicalCalls.Add((variableNodeId, writable, historianTagname)); + SurgicalCalls.Add((variableNodeId, writable, historianTagname, dataType, isArray, arrayLength)); return Result; } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerSurgicalShapeUpdateTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerSurgicalShapeUpdateTests.cs new file mode 100644 index 00000000..1ec2e708 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerSurgicalShapeUpdateTests.cs @@ -0,0 +1,222 @@ +using Opc.Ua; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; + +namespace ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests; + +/// +/// FB-7 — the BEHAVIOURAL half of the surgical DataType/array-shape in-place write (the applier-level +/// eligibility + dispatch decisions live in ). Boots a real +/// through (the same harness +/// uses) so +/// runs against a live node manager (real Lock + SystemContext + Server), and asserts: +/// +/// a DataType / ValueRank / array-length change is applied IN PLACE on the existing node and +/// RESETS its value to BadWaitingForInitialData (no stale wrong-typed value), while the node identity +/// (and therefore client subscriptions) survive; +/// a Writable / Historizing-only change (shape unchanged) does NOT reset the value — the +/// original surgical behaviour is preserved byte-for-byte; +/// the built GeneralModelChangeEvent carries Changes=[{Affected=node, Verb=DataTypeChanged}]. +/// +/// +/// Coverage boundary (deliberate, mirrors ): the model-change +/// event is asserted via its builder () +/// in isolation, not its end-to-end Server.ReportEvent dispatch — observing that would require a +/// subscribed event monitored-item. The single in-lock report call-site is covered by inspection. +/// +/// +public sealed class NodeManagerSurgicalShapeUpdateTests : IDisposable +{ + private static CancellationToken Ct => TestContext.Current.CancellationToken; + + private readonly string _pkiRoot = Path.Combine( + Path.GetTempPath(), + $"otopcua-surgical-shape-{Guid.NewGuid():N}"); + + // ───────────────────────────── Shape swap (DataType / ValueRank / array length) ───────────────────────────── + + /// A DataType change swaps the node's DataType in place and resets the value (the old Float value + /// must not survive on a now-Int32 node); the node id is unchanged, so client subscriptions are preserved. + [Fact] + public async Task DataType_change_swaps_in_place_and_resets_value() + { + var (host, server) = await BootAsync(); + var nm = server.NodeManager!; + + nm.EnsureVariable("eq-1/sp", parentFolderNodeId: null, displayName: "Sp", dataType: "Float", writable: false); + nm.WriteValue("eq-1/sp", 3.5f, OpcUaQuality.Good, DateTime.UtcNow); + var node = nm.TryGetVariable("eq-1/sp")!; + node.DataType.ShouldBe(DataTypeIds.Float); // arrange guard + + var applied = nm.UpdateTagAttributes("eq-1/sp", writable: false, historianTagname: null, + dataType: "Int32", isArray: false, arrayLength: null); + + applied.ShouldBeTrue(); + node.DataType.ShouldBe(DataTypeIds.Int32); // swapped in place + node.ValueRank.ShouldBe(ValueRanks.Scalar); + node.Value.ShouldBeNull(); // stale Float value dropped + node.StatusCode.ShouldBe((StatusCode)StatusCodes.BadWaitingForInitialData); + + await host.DisposeAsync(); + } + + /// A scalar → array flip swaps ValueRank to OneDimension + ArrayDimensions=[len] in place and + /// resets the value. + [Fact] + public async Task Scalar_to_array_flip_swaps_value_rank_and_resets_value() + { + var (host, server) = await BootAsync(); + var nm = server.NodeManager!; + + nm.EnsureVariable("eq-1/buf", parentFolderNodeId: null, displayName: "Buf", dataType: "Int16", writable: false); + nm.WriteValue("eq-1/buf", (short)42, OpcUaQuality.Good, DateTime.UtcNow); + var node = nm.TryGetVariable("eq-1/buf")!; + node.ValueRank.ShouldBe(ValueRanks.Scalar); // arrange guard + + var applied = nm.UpdateTagAttributes("eq-1/buf", writable: false, historianTagname: null, + dataType: "Int16", isArray: true, arrayLength: 8u); + + applied.ShouldBeTrue(); + node.ValueRank.ShouldBe(ValueRanks.OneDimension); + node.ArrayDimensions.ShouldNotBeNull(); + node.ArrayDimensions[0].ShouldBe(8u); + node.Value.ShouldBeNull(); // stale scalar value dropped + node.StatusCode.ShouldBe((StatusCode)StatusCodes.BadWaitingForInitialData); + + await host.DisposeAsync(); + } + + /// An array-to-array LENGTH change (rank stays OneDimension, only ArrayDimensions[0] differs) is + /// still treated as a shape change — the dimension is updated and the (now wrong-length) value reset. + [Fact] + public async Task Array_length_change_swaps_dimension_and_resets_value() + { + var (host, server) = await BootAsync(); + var nm = server.NodeManager!; + + nm.EnsureVariable("eq-1/buf", parentFolderNodeId: null, displayName: "Buf", dataType: "Int16", + writable: false, historianTagname: null, isArray: true, arrayLength: 4u); + nm.WriteValue("eq-1/buf", new short[] { 1, 2, 3, 4 }, OpcUaQuality.Good, DateTime.UtcNow); + var node = nm.TryGetVariable("eq-1/buf")!; + node.ArrayDimensions![0].ShouldBe(4u); // arrange guard + + var applied = nm.UpdateTagAttributes("eq-1/buf", writable: false, historianTagname: null, + dataType: "Int16", isArray: true, arrayLength: 8u); + + applied.ShouldBeTrue(); + node.ArrayDimensions[0].ShouldBe(8u); + node.Value.ShouldBeNull(); + node.StatusCode.ShouldBe((StatusCode)StatusCodes.BadWaitingForInitialData); + + await host.DisposeAsync(); + } + + // ───────────────────────────── Backward compatibility (shape unchanged) ───────────────────────────── + + /// A Writable-only change (DataType + array-ness identical to the live node) must NOT reset the + /// value — the original surgical behaviour stands. AccessLevel flips to ReadWrite; the prior Good value + /// survives untouched. + [Fact] + public async Task Writable_only_change_keeps_value_and_does_not_reset() + { + var (host, server) = await BootAsync(); + var nm = server.NodeManager!; + + nm.EnsureVariable("eq-1/sp", parentFolderNodeId: null, displayName: "Sp", dataType: "Float", writable: false); + nm.WriteValue("eq-1/sp", 7.0f, OpcUaQuality.Good, DateTime.UtcNow); + var node = nm.TryGetVariable("eq-1/sp")!; + + // Same DataType ("Float") + same scalar shape — only Writable flips false → true. + var applied = nm.UpdateTagAttributes("eq-1/sp", writable: true, historianTagname: null, + dataType: "Float", isArray: false, arrayLength: null); + + applied.ShouldBeTrue(); + node.Value.ShouldBe(7.0f); // value preserved (NOT reset) + node.StatusCode.ShouldBe((StatusCode)StatusCodes.Good); // status preserved + node.DataType.ShouldBe(DataTypeIds.Float); + // ReadWrite ⇒ CurrentRead | CurrentWrite. + node.AccessLevel.ShouldBe((byte)(AccessLevels.CurrentRead | AccessLevels.CurrentWrite)); + + await host.DisposeAsync(); + } + + /// An unknown node id (rebuilt/removed in the interim) returns false so the caller falls back to a + /// full rebuild; it must not throw. + [Fact] + public async Task Missing_node_returns_false() + { + var (host, server) = await BootAsync(); + var nm = server.NodeManager!; + + bool result = true; + Should.NotThrow(() => result = nm.UpdateTagAttributes("eq-1/gone", writable: false, historianTagname: null, + dataType: "Int32", isArray: false, arrayLength: null)); + result.ShouldBeFalse(); + + await host.DisposeAsync(); + } + + // ───────────────────────────── GeneralModelChangeEvent builder ───────────────────────────── + + /// The built model-change event announces the affected node with verb DataTypeChanged and the + /// node's TypeDefinition as AffectedType — what model-aware clients consume to re-read the definition. + [Fact] + public async Task Built_model_change_event_reflects_the_affected_node() + { + var (host, server) = await BootAsync(); + var nm = server.NodeManager!; + + nm.EnsureVariable("eq-1/sp", parentFolderNodeId: null, displayName: "Sp", dataType: "Float", writable: false); + var node = nm.TryGetVariable("eq-1/sp")!; + + var e = nm.BuildNodeShapeChangedEvent(node); + + e.ShouldNotBeNull(); + e.Changes.ShouldNotBeNull(); + var changes = e.Changes.Value; + changes.Length.ShouldBe(1); + changes[0].Affected.ShouldBe(node.NodeId); + changes[0].AffectedType.ShouldBe(VariableTypeIds.BaseDataVariableType); + changes[0].Verb.ShouldBe((byte)ModelChangeStructureVerbMask.DataTypeChanged); + + await host.DisposeAsync(); + } + + private async Task<(OpcUaApplicationHost Host, OtOpcUaSdkServer Server)> BootAsync() + { + var host = new OpcUaApplicationHost( + new OpcUaApplicationHostOptions + { + ApplicationName = "OtOpcUa.SurgicalShapeTest", + ApplicationUri = $"urn:OtOpcUa.SurgicalShapeTest:{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 */ } + } + } +}