fix(address-space): exact element-type check for array writes + array-capability shape edges (#610 review R4)

MAJOR-1: the SDK skips TypeInfo.IsInstanceOfDataType on any node with an OnWriteValue handler (every writable equipment node), and the node manager's fail-fast treated numeric<->numeric as compatible, so a UInt32[] to a Galaxy Int32 array matched MxValueEncoder's int[] arm by CLR covariance and was written bit-reinterpreted with a Good result. EvaluateEquipmentWriteStructure now matches an ARRAY write's element type exactly (Byte[]/ByteString the only equivalence; Variant[]/object[] refused) -> BadTypeMismatch, nothing dispatched. MxValueEncoder's int[]/long[] arms are exact-type guarded too, and the Galaxy parity test's false "the SDK refuses" premise is corrected (the UInt32/UInt64 exclusion is now the encoder's own answer). Scalar writes keep numeric widening by decision: consumers write CLR-typed values (Double to a Float node) and every driver's scalar encoder range-checks. MINOR: Modbus row honours maxRegistersPerWrite (smallest of DriverConfig / the tag's DeviceConfig, default 123) and FC15's 1968-coil ceiling, and requires region + numeric address; Modbus/S7/Galaxy require isArray + integer arrayLength >= 1 (Galaxy: an unbounded node would let a short write resize the attribute); an array value to a scalar node is refused inline. Docs: Uns.md array-write rules + upgrade note (ReadWrite array tags become client-writable on first boot), Modbus.md, Galaxy.md. Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe
2026-10-08 08:07:36 -04:00
parent b7a48e2b93
commit dabd8eb955
14 changed files with 532 additions and 85 deletions
@@ -668,7 +668,8 @@ public static class AddressSpaceComposer
            rawFolders, driverInstances, devices, tagGroups, tags, unsTagReferences,
            unsAreas, unsLines, equipment, topology);
        var unsReferenceVariables = BuildUnsReferenceVariables(
-            unsTagReferences, tags, unsAreas, unsLines, equipment, topology, driverInstances);
+            unsTagReferences, tags, unsAreas, unsLines, equipment, topology,
+            BuildArrayWriteCapability(driverInstances, devices));

        return new AddressSpaceComposition(areas, lines, nodes, plans, alarms)
        {
@@ -767,7 +768,7 @@ public static class AddressSpaceComposer
        // TagId → the set of referencing equipment UNS folder paths (Area/Line/Equipment), for the
        // native-alarm multi-notifier. Distinct + ordinal-sorted for determinism.
        var equipPathByTag = BuildReferencingEquipmentPathsByTag(unsTagReferences, unsAreas, unsLines, equipment);
-        var driverTypeById = BuildDriverTypeById(driverInstances);
+        var arrayWritableFor = BuildArrayWriteCapability(driverInstances, devices);

        var rawTags = new List<RawTagPlan>();
        foreach (var t in tags ?? Array.Empty<Tag>())
@@ -779,8 +780,7 @@ public static class AddressSpaceComposer
                : topology.DevicePaths.GetValueOrDefault(t.DeviceId);
            var driverInstanceId = topology.DriverIdByDevice.GetValueOrDefault(t.DeviceId, string.Empty);
            var intent = TagConfigIntent.Parse(t.TagConfig);
-            var arrayWritable = intent.IsArray && DriverArrayWriteSupport.Supports(
-                driverTypeById.GetValueOrDefault(driverInstanceId), t.DataType, t.TagConfig);
+            var arrayWritable = intent.IsArray && arrayWritableFor(t);
            var alarm = intent.Alarm is { } a
                ? new EquipmentTagAlarmInfo(a.AlarmType, a.Severity, a.HistorizeToAveva)
                : null;
@@ -808,12 +808,30 @@ public static class AddressSpaceComposer
        return (containers, rawTags);
    }

-    /// <summary><c>DriverInstanceId → DriverType</c>, for the per-driver array-write capability (Gitea #610).
-    /// First row wins on a duplicate id, matching the composer's other id maps.</summary>
-    private static IReadOnlyDictionary<string, string> BuildDriverTypeById(IReadOnlyList<DriverInstance> driverInstances) =>
-        driverInstances
+    /// <summary>
+    ///     The per-driver array-write capability (Gitea #610) as a per-tag predicate over the owning driver's
+    ///     <c>DriverType</c>, the driver's <c>DriverConfig</c> and the tag's own device's <c>DeviceConfig</c>
+    ///     (either config can carry a write cap such as Modbus <c>maxRegistersPerWrite</c>; the table takes the
+    ///     smaller). First row wins on a duplicate id, matching the composer's other id maps. Built once per
+    ///     compose and shared by the raw and UNS passes, so a mirror is writable exactly when its raw node is.
+    /// </summary>
+    private static Func<Tag, bool> BuildArrayWriteCapability(
+        IReadOnlyList<DriverInstance> driverInstances, IReadOnlyList<Device>? devices)
+    {
+        var driverById = driverInstances
            .GroupBy(d => d.DriverInstanceId, StringComparer.Ordinal)
-            .ToDictionary(g => g.Key, g => g.First().DriverType, StringComparer.Ordinal);
+            .ToDictionary(g => g.Key, g => g.First(), StringComparer.Ordinal);
+        var deviceById = (devices ?? Array.Empty<Device>())
+            .GroupBy(d => d.DeviceId, StringComparer.Ordinal)
+            .ToDictionary(g => g.Key, g => g.First(), StringComparer.Ordinal);
+        return tag =>
+        {
+            var device = deviceById.GetValueOrDefault(tag.DeviceId);
+            var driver = device is null ? null : driverById.GetValueOrDefault(device.DriverInstanceId);
+            return DriverArrayWriteSupport.Supports(
+                driver?.DriverType, tag.DataType, tag.TagConfig, driver?.DriverConfig, device?.DeviceConfig);
+        };
+    }

    /// <summary>
    ///     Emit the UNS reference Variables: one per <see cref="UnsTagReference"/> whose backing raw tag +
@@ -829,11 +847,10 @@ public static class AddressSpaceComposer
        IReadOnlyList<UnsLine> unsLines,
        IReadOnlyList<Equipment> equipment,
        RawTopology topology,
-        IReadOnlyList<DriverInstance> driverInstances)
+        Func<Tag, bool> arrayWritableFor)
    {
        var refs = unsTagReferences ?? Array.Empty<UnsTagReference>();
        if (refs.Count == 0) return Array.Empty<UnsReferenceVariable>();
-        var driverTypeById = BuildDriverTypeById(driverInstances);

        var tagsById = (tags ?? Array.Empty<Tag>())
            .GroupBy(t => t.TagId, StringComparer.Ordinal)
@@ -866,9 +883,7 @@ public static class AddressSpaceComposer
            var intent = TagConfigIntent.Parse(tag.TagConfig);
            // Same derivation as BuildRawSubtree's ArrayWritable, off the same row, so a mirror is writable
            // exactly when its raw node is.
-            var backingArrayWritable = intent.IsArray && DriverArrayWriteSupport.Supports(
-                driverTypeById.GetValueOrDefault(topology.DriverIdByDevice.GetValueOrDefault(tag.DeviceId, string.Empty)),
-                tag.DataType, tag.TagConfig);
+            var backingArrayWritable = intent.IsArray && arrayWritableFor(tag);

            vars.Add(new UnsReferenceVariable(
                UnsTagReferenceId: r.UnsTagReferenceId,
@@ -2025,10 +2025,16 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
    ///         conservative — it only rejects when BOTH the expected and actual built-in types are confidently
    ///         resolved AND differ (with numeric widening + the BaseDataType "accept anything" wildcard
    ///         allowed); anything uncertain (a non-variable node, an unresolved/abstract DataType, a
-    ///         <see cref="Variant"/>/array payload whose element type isn't cheaply known) is allowed through so
-    ///         the SDK's own (authoritative) type coercion in <c>BaseVariableState.WriteValue</c> remains the
-    ///         final arbiter. We deliberately do NOT attempt deep array-dimension / structured-type validation
-    ///         here — that is left to the SDK.
+    ///         <see cref="Variant"/> payload whose type isn't cheaply known) is allowed through.
+    ///     </para>
+    ///     <para>
+    ///         <b>⚠️ This is the ONLY type check a writable equipment node gets</b> (Gitea #610 review). The SDK's
+    ///         <c>BaseVariableState.WriteValueAttribute</c> runs <c>TypeInfo.IsInstanceOfDataType</c> only when
+    ///         the node has NO <c>OnWriteValue</c> handler; with one (every writable equipment node) it checks the
+    ///         access level, calls the handler and stores the value as given. So: an ARRAY write is matched
+    ///         EXACTLY (shape, declared length and element type — the SDK's own rule, restored), and an array
+    ///         value to a scalar node is refused. A SCALAR write keeps the looser numeric rule above on purpose
+    ///         (see <see cref="IsBuiltInTypeCompatible"/>).
    ///     </para>
    ///     Pure (no SDK server / Lock needed): reads only <paramref name="value"/> and the node's static
    ///     <c>DataType</c>, so it is unit-testable in isolation.
@@ -2049,8 +2055,10 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
        // 1-D array of its declared length: every array-capable driver refuses a short or long block (Modbus
        // FC15/FC16, S7's contiguous block), so a wrong shape is rejected here instead of being optimistically
        // applied, dispatched, refused and reverted. ArrayDimensions [0] / absent means unbounded ⇒ any length.
+        var isArrayNode = false;
        if (node is BaseDataVariableState { ValueRank: ValueRanks.OneDimension } arrayNode)
        {
+            isArrayNode = true;
            if (value is not Array { Rank: 1 } array)
            {
                return new ServiceResult(StatusCodes.BadTypeMismatch, "a 1-D array node takes a 1-D array value");
@@ -2071,14 +2079,43 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
        var expected = TypeInfo.GetBuiltInType(variable.DataType); // NodeId ⇒ built-in; abstract/unknown ⇒ Null
        if (expected is BuiltInType.Null or BuiltInType.Variant or BuiltInType.DataValue) return null; // unresolved / wildcard

+        // Gitea #610 review — the mirror case: an array value to a SCALAR node of a resolved DataType is refused
+        // up front rather than optimistically stored, dispatched to a scalar encoder, refused and reverted. A
+        // byte[] is exempt: on the wire it is a ByteString, which is a scalar (the type check below still decides
+        // whether the node takes one). An unresolved-DataType node deferred above, as it always has.
+        if (variable.ValueRank == ValueRanks.Scalar && value is Array and not byte[])
+        {
+            return new ServiceResult(StatusCodes.BadTypeMismatch, "a scalar node takes a scalar value");
+        }
+
        // SAFETY BOUNDARY: only reject against the CLOSED set of built-in types a writable equipment node can
        // actually carry (per ResolveBuiltInDataType). Any other expected type (Enumeration, Guid, NodeId,
        // StatusCode, …) is DEFERRED to the SDK's authoritative coercion so this fail-fast can NEVER false-reject
        // a write the SDK would have accepted (e.g. an Int32 payload to an Enumeration node coerces fine).
        if (!IsCheckableExpectedType(expected)) return null;

-        // TypeInfo.Construct(object) classifies the runtime payload; an unclassifiable value ⇒ Unknown (Null).
+        // TypeInfo.Construct(object) classifies the runtime payload (an array by its ELEMENT type); an
+        // unclassifiable value ⇒ Unknown (Null).
        var actual = TypeInfo.Construct(value).BuiltInType;
+
+        // Gitea #610 review (MAJOR-1) — an ARRAY write is checked EXACTLY, element type against DataType. The SDK
+        // runs its own TypeInfo.IsInstanceOfDataType check only when a node has NO OnWriteValue handler, and every
+        // writable equipment node has one, so nothing else stands between a mistyped array and the driver: a
+        // UInt32[] to a Galaxy Int32 array matched MxValueEncoder's int[] arm by CLR array covariance and wrote
+        // uint.MaxValue as -1 with a Good result, and a Modbus/S7 node kept serving an int[] under an Int16
+        // DataType. This restores the SDK's own rule for arrays — the element type must match, with the single
+        // Byte-array/ByteString equivalence (a Byte[] decodes as byte[], which classifies as ByteString) — and
+        // an unclassifiable element type (a Variant[] / object[]) is refused rather than deferred. Arrays were
+        // never client-writable before #610, so no client relies on a looser rule.
+        if (isArrayNode)
+        {
+            return actual == expected || (expected, actual) is (BuiltInType.Byte, BuiltInType.ByteString)
+                ? null
+                : new ServiceResult(
+                    StatusCodes.BadTypeMismatch,
+                    $"array element type {actual} does not match node data type {expected}");
+        }
+
        if (actual == BuiltInType.Null) return null; // couldn't classify the payload ⇒ defer to the SDK

        if (!IsBuiltInTypeCompatible(expected, actual))
@@ -2106,9 +2143,13 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
            : new ServiceResult(StatusCodes.BadWriteNotSupported, "IndexRange writes are not supported; write the whole array");

    /// <summary>
-    ///     Conservative built-in-type compatibility test for <see cref="EvaluateEquipmentWriteStructure"/>.
-    ///     Returns true (compatible — allow through) unless there is a confident mismatch. Exact matches pass;
-    ///     numeric-to-numeric is treated as compatible (the SDK widens/narrows numerics); a
+    ///     Conservative built-in-type compatibility test for <see cref="EvaluateEquipmentWriteStructure"/>'s
+    ///     SCALAR path (array writes are matched exactly — see there). Returns true (compatible — allow through)
+    ///     unless there is a confident mismatch. Exact matches pass; numeric-to-numeric is treated as compatible —
+    ///     deliberately looser than the SDK, which would refuse it: consumers (ScadaBridge) write CLR-typed values
+    ///     such as a Double to a Float node, and every driver's SCALAR encoder converts with a range check
+    ///     (<c>Convert.ToInt16</c>, MxValueEncoder's <c>uint ≤ int.MaxValue</c> arm, …), so an out-of-range value
+    ///     fails the device write and is reverted rather than silently reinterpreted; a
    ///     <see cref="BuiltInType.ByteString"/>↔<see cref="BuiltInType.Byte"/> array nuance and any
    ///     <see cref="BuiltInType.Variant"/> on either side are treated as compatible. Only a clear cross-family
    ///     mismatch (e.g. writing a String to a Boolean node) returns false. Pure + static.