fix(historian): events arm sets results on bad paths + Variant.Null SourceName + test hardening

- 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)
This commit is contained in:
Joseph Doherty
2026-06-14 20:10:16 -04:00
parent e3c0ef7b41
commit e6ec0ad8be
2 changed files with 53 additions and 20 deletions
@@ -1055,11 +1055,12 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
// it validates handles under Lock, builds `nodesToProcess` (a NodeHandle list for nodes WE own // 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 // that carry the HistoryRead access bit), validates the timestamp args, handles
// `releaseContinuationPoints`, and dispatches by `details` runtime type to the per-details // `releaseContinuationPoints`, and dispatches by `details` runtime type to the per-details
// protected virtuals below. We override the three variable-history virtuals; HistoryReadEvents is // protected virtuals below. We override all four arms: the three variable-history virtuals
// left to the base (Task 4 adds it). Each override receives the pre-filtered handles and fills // (Raw/Processed/AtTime) and the event-history arm (HistoryReadEvents, Task 4). Each override
// results[handle.Index] / errors[handle.Index] — handle.Index is the original index into the // receives the pre-filtered handles and fills results[handle.Index] / errors[handle.Index] —
// service-level results/errors lists, seeded by the base. The base pre-seeds every handle's error // handle.Index is the original index into the service-level results/errors lists, seeded by the
// to BadHistoryOperationUnsupported, so a handle we don't recognise stays "unsupported" by default. // 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 // 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 // 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)) if (idString is null || !_eventNotifierSources.TryGetValue(idString, out var sourceName))
{ {
// Not a registered event-history source (plain folder / Null-source promotion) ⇒ unsupported. // 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; errors[handle.Index] = StatusCodes.BadHistoryOperationUnsupported;
results[handle.Index] = new SdkHistoryReadResult { StatusCode = StatusCodes.BadHistoryOperationUnsupported };
continue; continue;
} }
@@ -1231,6 +1234,7 @@ public sealed class OtOpcUaNodeManager : CustomNodeManager2
Utils.LogError(ex, "OtOpcUaNodeManager: HistoryReadEvents failed for node {0}", handle.NodeId); Utils.LogError(ex, "OtOpcUaNodeManager: HistoryReadEvents failed for node {0}", handle.NodeId);
#pragma warning restore CS0618 #pragma warning restore CS0618
errors[handle.Index] = StatusCodes.BadHistoryOperationUnsupported; 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. // 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)), "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), "Time" => new Variant(evt.EventTimeUtc),
"ReceiveTime" => new Variant(evt.ReceivedTimeUtc), "ReceiveTime" => new Variant(evt.ReceivedTimeUtc),
"Message" => new Variant(new LocalizedText(evt.Message ?? string.Empty)), "Message" => new Variant(new LocalizedText(evt.Message ?? string.Empty)),
@@ -30,7 +30,8 @@ public sealed class NodeManagerHistoryReadEventsTests : IDisposable
/// <summary>Happy path: the fake receives (sourceName == the equipment-folder id, StartTime, EndTime, /// <summary>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 /// maxEvents), and each returned event decodes to a HistoryEventFieldList whose EventFields are in
/// SelectClause ORDER with correctly-typed Variants (EventId ByteString, SourceName string, Time /// SelectClause ORDER with correctly-typed Variants (EventId ByteString, SourceName string, Time
/// DateTime, Message LocalizedText, Severity UInt16). StatusCode is Good when events are present.</summary> /// DateTime, ReceiveTime DateTime, Message LocalizedText, Severity UInt16). StatusCode is Good when
/// events are present.</summary>
[Fact] [Fact]
public async Task Events_dispatches_to_source_and_projects_fields_in_select_order() public async Task Events_dispatches_to_source_and_projects_fields_in_select_order()
{ {
@@ -57,7 +58,8 @@ public sealed class NodeManagerHistoryReadEventsTests : IDisposable
StartTime = start, StartTime = start,
EndTime = end, EndTime = end,
NumValuesPerNode = 50, 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); 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); var history = (HistoryEvent)ExtensionObject.ToEncodeable(results[0].HistoryData);
history.Events.Count.ShouldBe(1); history.Events.Count.ShouldBe(1);
var fields = history.Events[0].EventFields; var fields = history.Events[0].EventFields;
fields.Count.ShouldBe(5); fields.Count.ShouldBe(6);
// EventId ⇒ ByteString (UTF-8 of "evt-42"). // EventId ⇒ ByteString (UTF-8 of "evt-42").
fields[0].Value.ShouldBeOfType<byte[]>().ShouldBe(System.Text.Encoding.UTF8.GetBytes("evt-42")); fields[0].Value.ShouldBeOfType<byte[]>().ShouldBe(System.Text.Encoding.UTF8.GetBytes("evt-42"));
// SourceName ⇒ string. // SourceName ⇒ string.
fields[1].Value.ShouldBe("Pump_001"); fields[1].Value.ShouldBe("Pump_001");
// Time ⇒ DateTime. // Time ⇒ DateTime (event occurrence time).
fields[2].Value.ShouldBe(evtTime); fields[2].Value.ShouldBe(evtTime);
// ReceiveTime ⇒ DateTime (server receipt time = ReceivedTimeUtc).
fields[3].Value.ShouldBe(rcvTime);
// Message ⇒ LocalizedText. // Message ⇒ LocalizedText.
fields[3].Value.ShouldBeOfType<LocalizedText>().Text.ShouldBe("Pump tripped"); fields[4].Value.ShouldBeOfType<LocalizedText>().Text.ShouldBe("Pump tripped");
// Severity ⇒ UInt16. // Severity ⇒ UInt16.
fields[4].Value.ShouldBe((ushort)700); fields[5].Value.ShouldBe((ushort)700);
await host.DisposeAsync(); await host.DisposeAsync();
} }
@@ -204,8 +208,20 @@ public sealed class NodeManagerHistoryReadEventsTests : IDisposable
await host.DisposeAsync(); await host.DisposeAsync();
} }
/// <summary>A node we own that is NOT a registered event-notifier source (a plain variable node) ⇒ /// <summary>A variable node targeted by a HistoryReadEvents request ⇒ BadHistoryOperationUnsupported;
/// BadHistoryOperationUnsupported; the source is never invoked.</summary> /// the source is never invoked.
/// <para>
/// NOTE — what this test actually pins: the SDK base (CustomNodeManager2.HistoryRead) filters
/// event-history reads by the <c>EventNotifier.HistoryRead</c> bit, NOT by
/// <c>AccessLevel.HistoryRead</c>. A variable node carries <c>AccessLevel.HistoryRead</c> (for
/// variable-history reads) but <c>EventNotifier = None</c> (no event-notifier bits at all). The
/// SDK base therefore rejects it and does NOT pass it to our <c>HistoryReadEvents</c> 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 <c>_eventNotifierSources</c>) is exercised by the
/// <c>Events_folder_promoted_without_source_yields_BadHistoryOperationUnsupported</c> test instead.
/// </para>
/// </summary>
[Fact] [Fact]
public async Task Events_non_source_node_yields_BadHistoryOperationUnsupported() public async Task Events_non_source_node_yields_BadHistoryOperationUnsupported()
{ {
@@ -214,8 +230,8 @@ public sealed class NodeManagerHistoryReadEventsTests : IDisposable
var fake = new RecordingHistorianDataSource(); var fake = new RecordingHistorianDataSource();
nm.HistorianDataSource = fake; nm.HistorianDataSource = fake;
// A historized variable node — owns the HistoryRead bit (so the base hands it to us) but is NOT an // A historized variable node — has AccessLevel.HistoryRead (variable-history reads) but
// event-notifier source, so the Events arm must reject it. // 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", nm.EnsureVariable("eq-1/temp", parentFolderNodeId: null, displayName: "Temp", dataType: "Float",
writable: false, historianTagname: "WW.Temp"); writable: false, historianTagname: "WW.Temp");
var nodeId = nm.TryGetVariable("eq-1/temp")!.NodeId; var nodeId = nm.TryGetVariable("eq-1/temp")!.NodeId;
@@ -237,7 +253,9 @@ public sealed class NodeManagerHistoryReadEventsTests : IDisposable
} }
/// <summary>A backend that throws ⇒ that node's error is Bad and no exception escapes the /// <summary>A backend that throws ⇒ that node's error is Bad and no exception escapes the
/// HistoryRead call.</summary> /// 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].</summary>
[Fact] [Fact]
public async Task Events_backend_throw_yields_bad_status_and_does_not_escape() 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. // The call must not throw even though the backend does.
var (results, errors) = InvokeHistoryRead(server, nm, details, notifierNodeId); 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); 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(); await host.DisposeAsync();
} }
@@ -319,6 +344,9 @@ public sealed class NodeManagerHistoryReadEventsTests : IDisposable
public HistoricalEventsResult EventsResult { get; set; } = public HistoricalEventsResult EventsResult { get; set; } =
new(Array.Empty<HistoricalEvent>(), null); new(Array.Empty<HistoricalEvent>(), null);
/// <summary>Set on every ReadEventsAsync entry, even when ThrowOnRead causes it to throw
/// before EventsCalled is set — proves the override reached the bridge.</summary>
public bool EventsEntered { get; private set; }
public bool EventsCalled { get; private set; } public bool EventsCalled { get; private set; }
public string? LastSourceName { get; private set; } public string? LastSourceName { get; private set; }
public DateTime LastStart { 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, string? sourceName, DateTime startUtc, DateTime endUtc, int maxEvents,
CancellationToken cancellationToken) CancellationToken cancellationToken)
{ {
EventsEntered = true;
if (ThrowOnRead) throw new InvalidOperationException("backend boom for events"); if (ThrowOnRead) throw new InvalidOperationException("backend boom for events");
EventsCalled = true; EventsCalled = true;
LastSourceName = sourceName; LastSourceName = sourceName;