fix(opcua): address code review on write-outcome surfacing
v2-ci / build (push) Failing after 35s
v2-ci / unit-tests (tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests) (push) Has been skipped
v2-ci / unit-tests (tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests) (push) Has been skipped
v2-ci / unit-tests (tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests) (push) Has been skipped
v2-ci / unit-tests (tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests) (push) Has been skipped
v2-ci / unit-tests (tests/Server/ZB.MOM.WW.OtOpcUa.Security.Tests) (push) Has been skipped
v2-ci / integration (tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests) (push) Has been skipped
v2-ci / integration (tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests) (push) Has been skipped
v2-ci / build (push) Failing after 35s
v2-ci / unit-tests (tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests) (push) Has been skipped
v2-ci / unit-tests (tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests) (push) Has been skipped
v2-ci / unit-tests (tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests) (push) Has been skipped
v2-ci / unit-tests (tests/Server/ZB.MOM.WW.OtOpcUa.Runtime.Tests) (push) Has been skipped
v2-ci / unit-tests (tests/Server/ZB.MOM.WW.OtOpcUa.Security.Tests) (push) Has been skipped
v2-ci / integration (tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests) (push) Has been skipped
v2-ci / integration (tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests) (push) Has been skipped
- A.1 (false-rejection safety): restrict the structural fail-fast's confident-mismatch check to the CLOSED set of built-in types ResolveBuiltInDataType emits (numeric families + Boolean/ String/DateTime/ByteString). Any other expected type (Enumeration, Guid, …) now defers to the SDK, so a coercible write (Int32→Enumeration) is never false-rejected. + A7/A8 regression tests. - C.1: guard BuildWriteFailureAuditEvent (under Lock) in try/catch like ReportAuditEvent, so a SetChildValue surprise is swallowed+logged, never thrown out of the fire-and-forget continuation.
This commit is contained in:
@@ -828,6 +828,12 @@ 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
|
||||
|
||||
// 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).
|
||||
var actual = TypeInfo.Construct(value).BuiltInType;
|
||||
if (actual == BuiltInType.Null) return null; // couldn't classify the payload ⇒ defer to the SDK
|
||||
@@ -869,6 +875,13 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
|
||||
BuiltInType.Int32 or BuiltInType.UInt32 or BuiltInType.Int64 or BuiltInType.UInt64 or
|
||||
BuiltInType.Float or BuiltInType.Double;
|
||||
|
||||
/// <summary>The CLOSED set of expected built-in DataTypes the structural fail-fast is allowed to reject
|
||||
/// against — exactly the types <see cref="ResolveBuiltInDataType"/> can emit for a writable equipment node
|
||||
/// (the numeric families + Boolean / String / DateTime / ByteString). Any expected type OUTSIDE this set is
|
||||
/// deferred to the SDK so a coercible write (Int32→Enumeration, etc.) is never false-rejected.</summary>
|
||||
private static bool IsCheckableExpectedType(BuiltInType t) =>
|
||||
IsNumeric(t) || t is BuiltInType.Boolean or BuiltInType.String or BuiltInType.DateTime or BuiltInType.ByteString;
|
||||
|
||||
/// <summary>
|
||||
/// Pure decision for the write-outcome self-correction: revert the node to its pre-write value ONLY on
|
||||
/// a FAILED outcome AND only while the node still holds the optimistic value. The
|
||||
@@ -951,7 +964,20 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
|
||||
variable.ClearChangeMasks(SystemContext, includeChildren: false); // notify — restore prior
|
||||
|
||||
// Item C: build the audit event-state while we hold the node reference + Lock, but DON'T report yet.
|
||||
auditEvent = BuildWriteFailureAuditEvent(variable, outcome, optimisticValue, priorValue, clientUserId);
|
||||
// Guarded like ReportAuditEvent: the revert above has ALREADY happened, so a surprise from the SDK
|
||||
// child-population path (e.g. a SetChildValue on a null OldValue/NewValue) must be swallowed + logged,
|
||||
// never thrown out of this fire-and-forget continuation.
|
||||
try
|
||||
{
|
||||
auditEvent = BuildWriteFailureAuditEvent(variable, outcome, optimisticValue, priorValue, clientUserId);
|
||||
}
|
||||
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 build AuditWriteUpdateEvent for {0}", nodeId);
|
||||
#pragma warning restore CS0618
|
||||
auditEvent = null;
|
||||
}
|
||||
}
|
||||
|
||||
// Report OUTSIDE Lock — keeps the hold short and sidesteps any re-entrancy through the server event path.
|
||||
|
||||
@@ -169,6 +169,26 @@ public sealed class EquipmentWriteGateTests
|
||||
OtOpcUaNodeManager.EvaluateEquipmentWriteStructure(value: 1, folder).ShouldBeNull();
|
||||
}
|
||||
|
||||
/// <summary>(A7) Closed-set safety boundary: an Enumeration-typed node with an Int32 payload DEFERS
|
||||
/// (returns null) rather than false-rejecting — an enum write normally carries an Int32 the SDK coerces.
|
||||
/// Before the closed-set gate this returned <c>BadTypeMismatch</c> — a false rejection of a valid write.</summary>
|
||||
[Fact]
|
||||
public void Structure_enumeration_datatype_defers_to_sdk()
|
||||
{
|
||||
var node = ValueNode(DataTypeIds.Enumeration);
|
||||
OtOpcUaNodeManager.EvaluateEquipmentWriteStructure(value: 2, node).ShouldBeNull();
|
||||
}
|
||||
|
||||
/// <summary>(A8) Closed-set safety boundary: any expected built-in type outside the materialiser's emitted
|
||||
/// set (here Guid) DEFERS — only the numeric families + Boolean/String/DateTime/ByteString are ever rejected,
|
||||
/// so the fail-fast can never reject a write the SDK would coerce.</summary>
|
||||
[Fact]
|
||||
public void Structure_noncheckable_expected_type_defers()
|
||||
{
|
||||
var node = ValueNode(DataTypeIds.Guid);
|
||||
OtOpcUaNodeManager.EvaluateEquipmentWriteStructure(value: "any", node).ShouldBeNull();
|
||||
}
|
||||
|
||||
private static BaseDataVariableState ValueNode(NodeId dataType) => new(null)
|
||||
{
|
||||
NodeId = new NodeId("eq/x", 2),
|
||||
|
||||
Reference in New Issue
Block a user