refactor(opcua): FB-7 review nits — model-event SourceNode=Server, dataType guard, shape tests

Code-review follow-ups on the FB-7 surgical shape-write commit: - GeneralModelChangeEvent now sets SourceNode=Server + SourceName (Part 3 §8.7.4) so clients filtering events by SourceNode match it (report still uses source:null). - UpdateTagAttributes adds an explicit dataType null/empty guard (widened surface). - Tighten the ArrayLengthDiffers doc comment. - Add array→scalar transition test + null-arrayLength zero-default test (coverage symmetry). 275/275 OpcUaServer.Tests green.
2026-06-19 03:30:29 -04:00
parent fb094fa566
commit 7a8ae9600b
2 changed files with 59 additions and 4 deletions
@@ -1417,6 +1417,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
    public bool UpdateTagAttributes(string variableNodeId, bool writable, string? historianTagname, string dataType, bool isArray, uint? arrayLength)
    {
        ArgumentException.ThrowIfNullOrEmpty(variableNodeId);
+        ArgumentException.ThrowIfNullOrEmpty(dataType); // widened surface ⇒ explicit contract (unknown names still map to BaseDataType)
        BaseDataVariableState? shapeChangedNode = null;
        lock (Lock)
        {
@@ -1457,13 +1458,14 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
        return true;
    }

-    /// <summary>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 <see cref="UpdateTagAttributes"/>, so this only fires for an array-to-array length edit).</summary>
+    /// <summary>True when an array target's requested 1-D length differs from the node's current one. This is
+    /// the array-to-array length-edit case: the caller's <c>ValueRank != newValueRank</c> check already catches
+    /// any scalar↔array transition, so this runs to decide whether an already-array node's dimension changed
+    /// (and treats absent/empty current dimensions as "differs", forcing the in-place update).</summary>
    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
+        if (v.ArrayDimensions is not { Count: > 0 }) return true; // no current dimension ⇒ shape differs
        return v.ArrayDimensions[0] != (arrayLength ?? 0u);
    }

@@ -1483,6 +1485,11 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
            source: null,
            severity: EventSeverity.Medium,
            message: new LocalizedText($"Node {variable.NodeId} definition changed (DataType/ValueRank)"));
+        // Part 3 §8.7.4: a GeneralModelChangeEvent is emitted by the Server object — set SourceNode/SourceName
+        // to Server explicitly (we report with source:null since this manager has no Server NodeState handle),
+        // so conformant clients that filter events by SourceNode still match this one.
+        e.SetChildValue(SystemContext, BrowseNames.SourceNode, ObjectIds.Server, false);
+        e.SetChildValue(SystemContext, BrowseNames.SourceName, "Server", false);
        var change = new ModelChangeStructureDataType
        {
            Affected = variable.NodeId,
@@ -112,6 +112,54 @@ public sealed class NodeManagerSurgicalShapeUpdateTests : IDisposable
        await host.DisposeAsync();
    }

+    /// <summary>The symmetric array → scalar flip: ValueRank drops back to Scalar, ArrayDimensions clears to
+    /// null, and the (now wrong-shaped) array value is reset.</summary>
+    [Fact]
+    public async Task Array_to_scalar_flip_clears_dimensions_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.ValueRank.ShouldBe(ValueRanks.OneDimension); // arrange guard
+
+        var applied = nm.UpdateTagAttributes("eq-1/buf", writable: false, historianTagname: null,
+            dataType: "Int16", isArray: false, arrayLength: null);
+
+        applied.ShouldBeTrue();
+        node.ValueRank.ShouldBe(ValueRanks.Scalar);
+        node.ArrayDimensions.ShouldBeNull();
+        node.Value.ShouldBeNull();
+        node.StatusCode.ShouldBe((StatusCode)StatusCodes.BadWaitingForInitialData);
+
+        await host.DisposeAsync();
+    }
+
+    /// <summary>A scalar → array flip with a null arrayLength defaults ArrayDimensions to [0] — mirroring
+    /// <see cref="OtOpcUaNodeManager.EnsureVariable"/>'s fresh-node default for an unspecified length.</summary>
+    [Fact]
+    public async Task Array_flip_with_null_length_defaults_dimension_to_zero()
+    {
+        var (host, server) = await BootAsync();
+        var nm = server.NodeManager!;
+
+        nm.EnsureVariable("eq-1/buf", parentFolderNodeId: null, displayName: "Buf", dataType: "Int16", writable: false);
+        var node = nm.TryGetVariable("eq-1/buf")!;
+
+        var applied = nm.UpdateTagAttributes("eq-1/buf", writable: false, historianTagname: null,
+            dataType: "Int16", isArray: true, arrayLength: null);
+
+        applied.ShouldBeTrue();
+        node.ValueRank.ShouldBe(ValueRanks.OneDimension);
+        node.ArrayDimensions.ShouldNotBeNull();
+        node.ArrayDimensions[0].ShouldBe(0u); // null length ⇒ [0], same as EnsureVariable
+
+        await host.DisposeAsync();
+    }
+
    // ───────────────────────────── Backward compatibility (shape unchanged) ─────────────────────────────

    /// <summary>A Writable-only change (DataType + array-ness identical to the live node) must NOT reset the