From e6ec0ad8be1b19b79913d803548bbbfc124f3236 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sun, 14 Jun 2026 20:10:16 -0400 Subject: [PATCH] fix(historian): events arm sets results on bad paths + Variant.Null SourceName + test hardening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - HistoryReadEvents miss path + catch path now both set results[handle.Index] explicitly (new SdkHistoryReadResult { StatusCode = BadHistoryOperationUnsupported }) — don't rely on base pre-seeding results[i] so every path sets BOTH errors and results coherently (#1) - ProjectEventField: SourceName null now emits Variant.Null instead of a String-typed null variant (evt.SourceName is null ? Variant.Null : new Variant(evt.SourceName)) (#3) - Comment near the HistoryRead dispatcher block updated: all four arms (Raw/Processed/AtTime + Events/Task 4) are now overridden — "left to the base" wording was stale (#5) - Happy-path test adds ReceiveTime to select clauses and asserts it projects ReceivedTimeUtc as a DateTime Variant at the correct select-order position (#4) - Backend-throw test hardened: asserts errors[0] via ServiceResult.IsBad + explicit code, asserts results[0] is non-null with the Bad code (no longer relies on base seeding), and asserts EventsEntered to prove the override reached the bridge before the throw (#1) - RecordingHistorianDataSource gains EventsEntered flag (set before ThrowOnRead check) (#1) - Events_non_source_node test gains clarifying doc comment explaining the SDK base rejects variable nodes (EventNotifier=None) for event reads before our override runs; the override's source-guard is exercised by the promoted-without-source test instead (#2) --- .../OtOpcUaNodeManager.cs | 18 +++--- .../NodeManagerHistoryReadEventsTests.cs | 55 ++++++++++++++----- 2 files changed, 53 insertions(+), 20 deletions(-) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs index bee425d6..80d601a1 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer/OtOpcUaNodeManager.cs @@ -1055,11 +1055,12 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 // it validates handles under Lock, builds `nodesToProcess` (a NodeHandle list for nodes WE own // that carry the HistoryRead access bit), validates the timestamp args, handles // `releaseContinuationPoints`, and dispatches by `details` runtime type to the per-details - // protected virtuals below. We override the three variable-history virtuals; HistoryReadEvents is - // left to the base (Task 4 adds it). Each override receives the pre-filtered handles and fills - // results[handle.Index] / errors[handle.Index] — handle.Index is the original index into the - // service-level results/errors lists, seeded by the base. The base pre-seeds every handle's error - // to BadHistoryOperationUnsupported, so a handle we don't recognise stays "unsupported" by default. + // protected virtuals below. We override all four arms: the three variable-history virtuals + // (Raw/Processed/AtTime) and the event-history arm (HistoryReadEvents, Task 4). Each override + // receives the pre-filtered handles and fills results[handle.Index] / errors[handle.Index] — + // handle.Index is the original index into the service-level results/errors lists, seeded by the + // base. The base pre-seeds every handle's error to BadHistoryOperationUnsupported, so a handle + // we don't recognise stays "unsupported" by default. // // NOTE: unlike OnWriteValue, the SDK does NOT hold the node-manager Lock while invoking these, so // block-bridging the async data source (GetAwaiter().GetResult()) is safe — it can't freeze the @@ -1196,8 +1197,10 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 if (idString is null || !_eventNotifierSources.TryGetValue(idString, out var sourceName)) { // Not a registered event-history source (plain folder / Null-source promotion) ⇒ unsupported. - // (The base pre-seeds this same status; set it explicitly so the contract is local + obvious.) + // Set both errors and results explicitly on every bad path — don't rely on the SDK base + // pre-seeding results[i], so every path is self-contained and the contract is obvious. errors[handle.Index] = StatusCodes.BadHistoryOperationUnsupported; + results[handle.Index] = new SdkHistoryReadResult { StatusCode = StatusCodes.BadHistoryOperationUnsupported }; continue; } @@ -1231,6 +1234,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 Utils.LogError(ex, "OtOpcUaNodeManager: HistoryReadEvents failed for node {0}", handle.NodeId); #pragma warning restore CS0618 errors[handle.Index] = StatusCodes.BadHistoryOperationUnsupported; + results[handle.Index] = new SdkHistoryReadResult { StatusCode = StatusCodes.BadHistoryOperationUnsupported }; } } } @@ -1289,7 +1293,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2 { // BaseEventType/EventId is a ByteString — encode the driver-specific string id as UTF-8 bytes. "EventId" => new Variant(System.Text.Encoding.UTF8.GetBytes(evt.EventId ?? string.Empty)), - "SourceName" => new Variant(evt.SourceName), // string; null ⇒ Variant.Null + "SourceName" => evt.SourceName is null ? Variant.Null : new Variant(evt.SourceName), "Time" => new Variant(evt.EventTimeUtc), "ReceiveTime" => new Variant(evt.ReceivedTimeUtc), "Message" => new Variant(new LocalizedText(evt.Message ?? string.Empty)), diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadEventsTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadEventsTests.cs index 6c4f20d7..17c92570 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadEventsTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadEventsTests.cs @@ -30,7 +30,8 @@ public sealed class NodeManagerHistoryReadEventsTests : IDisposable /// Happy path: the fake receives (sourceName == the equipment-folder id, StartTime, EndTime, /// maxEvents), and each returned event decodes to a HistoryEventFieldList whose EventFields are in /// SelectClause ORDER with correctly-typed Variants (EventId ByteString, SourceName string, Time - /// DateTime, Message LocalizedText, Severity UInt16). StatusCode is Good when events are present. + /// DateTime, ReceiveTime DateTime, Message LocalizedText, Severity UInt16). StatusCode is Good when + /// events are present. [Fact] public async Task Events_dispatches_to_source_and_projects_fields_in_select_order() { @@ -57,7 +58,8 @@ public sealed class NodeManagerHistoryReadEventsTests : IDisposable StartTime = start, EndTime = end, NumValuesPerNode = 50, - Filter = SelectFilter("EventId", "SourceName", "Time", "Message", "Severity"), + // ReceiveTime is included to verify it projects evt.ReceivedTimeUtc at index 3. + Filter = SelectFilter("EventId", "SourceName", "Time", "ReceiveTime", "Message", "Severity"), }; var (results, errors) = InvokeHistoryRead(server, nm, details, notifierNodeId); @@ -74,18 +76,20 @@ public sealed class NodeManagerHistoryReadEventsTests : IDisposable var history = (HistoryEvent)ExtensionObject.ToEncodeable(results[0].HistoryData); history.Events.Count.ShouldBe(1); var fields = history.Events[0].EventFields; - fields.Count.ShouldBe(5); + fields.Count.ShouldBe(6); // EventId ⇒ ByteString (UTF-8 of "evt-42"). fields[0].Value.ShouldBeOfType().ShouldBe(System.Text.Encoding.UTF8.GetBytes("evt-42")); // SourceName ⇒ string. fields[1].Value.ShouldBe("Pump_001"); - // Time ⇒ DateTime. + // Time ⇒ DateTime (event occurrence time). fields[2].Value.ShouldBe(evtTime); + // ReceiveTime ⇒ DateTime (server receipt time = ReceivedTimeUtc). + fields[3].Value.ShouldBe(rcvTime); // Message ⇒ LocalizedText. - fields[3].Value.ShouldBeOfType().Text.ShouldBe("Pump tripped"); + fields[4].Value.ShouldBeOfType().Text.ShouldBe("Pump tripped"); // Severity ⇒ UInt16. - fields[4].Value.ShouldBe((ushort)700); + fields[5].Value.ShouldBe((ushort)700); await host.DisposeAsync(); } @@ -204,8 +208,20 @@ public sealed class NodeManagerHistoryReadEventsTests : IDisposable await host.DisposeAsync(); } - /// A node we own that is NOT a registered event-notifier source (a plain variable node) ⇒ - /// BadHistoryOperationUnsupported; the source is never invoked. + /// A variable node targeted by a HistoryReadEvents request ⇒ BadHistoryOperationUnsupported; + /// the source is never invoked. + /// + /// NOTE — what this test actually pins: the SDK base (CustomNodeManager2.HistoryRead) filters + /// event-history reads by the EventNotifier.HistoryRead bit, NOT by + /// AccessLevel.HistoryRead. A variable node carries AccessLevel.HistoryRead (for + /// variable-history reads) but EventNotifier = None (no event-notifier bits at all). The + /// SDK base therefore rejects it and does NOT pass it to our HistoryReadEvents override; + /// the Bad result comes from the base's pre-seeding, not from our source-guard. This test pins + /// that base-level rejection of variable nodes for event reads. + /// The override's own source-guard (miss in _eventNotifierSources) is exercised by the + /// Events_folder_promoted_without_source_yields_BadHistoryOperationUnsupported test instead. + /// + /// [Fact] public async Task Events_non_source_node_yields_BadHistoryOperationUnsupported() { @@ -214,8 +230,8 @@ public sealed class NodeManagerHistoryReadEventsTests : IDisposable var fake = new RecordingHistorianDataSource(); nm.HistorianDataSource = fake; - // A historized variable node — owns the HistoryRead bit (so the base hands it to us) but is NOT an - // event-notifier source, so the Events arm must reject it. + // A historized variable node — has AccessLevel.HistoryRead (variable-history reads) but + // EventNotifier=None (no event-notifier bit). The SDK base rejects it before our override runs. nm.EnsureVariable("eq-1/temp", parentFolderNodeId: null, displayName: "Temp", dataType: "Float", writable: false, historianTagname: "WW.Temp"); var nodeId = nm.TryGetVariable("eq-1/temp")!.NodeId; @@ -237,7 +253,9 @@ public sealed class NodeManagerHistoryReadEventsTests : IDisposable } /// A backend that throws ⇒ that node's error is Bad and no exception escapes the - /// HistoryRead call. + /// HistoryRead call. The fake source MUST be invoked (proving we reached the bridge) and threw; + /// the production catch now sets BOTH errors and results explicitly, so both are asserted here + /// rather than relying on the SDK base pre-seeding results[i]. [Fact] public async Task Events_backend_throw_yields_bad_status_and_does_not_escape() { @@ -262,9 +280,16 @@ public sealed class NodeManagerHistoryReadEventsTests : IDisposable // The call must not throw even though the backend does. var (results, errors) = InvokeHistoryRead(server, nm, details, notifierNodeId); - StatusCode.IsBad(errors[0].StatusCode).ShouldBeTrue(); + // Authoritative per-node signal: errors[0] must be Bad. + ServiceResult.IsBad(errors[0]).ShouldBeTrue(); errors[0].StatusCode.Code.ShouldBe(StatusCodes.BadHistoryOperationUnsupported); - (results[0].StatusCode.Code == StatusCodes.GoodNoData).ShouldBeFalse(); + // The production catch now sets results[0] explicitly — assert it directly (not relying on base seeding). + results[0].ShouldNotBeNull(); + results[0].StatusCode.Code.ShouldBe(StatusCodes.BadHistoryOperationUnsupported); + // The source WAS entered (proving the override reached the bridge and the throw was swallowed). + fake.EventsEntered.ShouldBeTrue(); + // ThrowOnRead fires before EventsCalled/LastSourceName are set — that's the throw path. + fake.EventsCalled.ShouldBeFalse(); await host.DisposeAsync(); } @@ -319,6 +344,9 @@ public sealed class NodeManagerHistoryReadEventsTests : IDisposable public HistoricalEventsResult EventsResult { get; set; } = new(Array.Empty(), null); + /// Set on every ReadEventsAsync entry, even when ThrowOnRead causes it to throw + /// before EventsCalled is set — proves the override reached the bridge. + public bool EventsEntered { get; private set; } public bool EventsCalled { get; private set; } public string? LastSourceName { get; private set; } public DateTime LastStart { get; private set; } @@ -343,6 +371,7 @@ public sealed class NodeManagerHistoryReadEventsTests : IDisposable string? sourceName, DateTime startUtc, DateTime endUtc, int maxEvents, CancellationToken cancellationToken) { + EventsEntered = true; if (ThrowOnRead) throw new InvalidOperationException("backend boom for events"); EventsCalled = true; LastSourceName = sourceName;