From b8c213673249d84f1ef45bd1885a43957c2a89de Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 8 Oct 2026 19:29:47 -0400 Subject: [PATCH] fix(opcuaclient): #649 review minors - CodeBits match, Bad-only exception ctor, BadNoData + Raw-resume tests, latent-impact docs Claude-Session: https://claude.ai/code/session_01Ef2ozxS7QPanGT8uoATbTe --- docs/drivers/OpcUaClient.md | 18 ++++-- .../IHistoryProvider.cs | 7 ++- .../OpcUaClientDriver.cs | 5 +- .../HistoryReadStatusExceptionTests.cs | 21 +++++++ .../OpcUaClientDriverHistoryWireTests.cs | 19 ++++++ .../NodeManagerHistoryReadPagingTests.cs | 60 +++++++++++++++++++ 6 files changed, 122 insertions(+), 8 deletions(-) create mode 100644 tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/Historian/HistoryReadStatusExceptionTests.cs diff --git a/docs/drivers/OpcUaClient.md b/docs/drivers/OpcUaClient.md index 16cbe891..f50120c4 100644 --- a/docs/drivers/OpcUaClient.md +++ b/docs/drivers/OpcUaClient.md @@ -123,13 +123,21 @@ DurationGood/Bad, PercentGood/Bad, WorstQuality, NumberOfTransitions, Annotation population StandardDeviation/Variance — each mapped to its OPC UA Part 13 standard aggregate NodeId; #285), and at-time reads are supported; each returned `DataValue` keeps its upstream `StatusCode` and timestamps verbatim. -**The per-node `HistoryReadResult.StatusCode` is honoured (#649).** A Bad node status is never read as +**The per-node `HistoryReadResult.StatusCode` is honoured (#649).** The driver never reads a Bad node status as empty data: `BadAggregateNotSupported` on a Processed read throws `HistoryAggregateNotSupportedException`, and any other Bad status (`BadUserAccessDenied`, `BadNodeIdUnknown`, `BadHistoryOperationUnsupported`, ...) throws -`HistoryReadStatusException` carrying the code. The server's HistoryRead path relays both verbatim as that node's -result status (and only that node's). `Good`, `GoodNoData`, `GoodMoreData` (continuation point) and `Uncertain` -are unchanged, and a node-level `BadNoData` is still an empty result. Callers that previously saw an empty list for -a refused read now see the Bad status, so an unhistorized upstream node reads `BadHistoryOperationUnsupported`. +`HistoryReadStatusException` carrying the code (the constructor rejects non-Bad codes). `Good`, `GoodNoData`, +`GoodMoreData` (continuation point) and `Uncertain` are unchanged, and a node-level `BadNoData` is still an empty +result. The status is matched on its code bits, so an info/overflow sub-status does not dodge the match. + +⚠️ **This is currently latent.** The OtOpcUa server does not dispatch HistoryRead to a driver's `IHistoryProvider` +(it serves only the server-wide `IHistorianDataSource`, see `docs/Historian.md`), so no OPC UA client of OtOpcUa +sees this driver's history results or its refusals today. The server's HistoryRead arms (`ServeNodeAsync`, +`ServeRawPagedAsync`) do map a `HistoryReadStatusException` thrown by an `IHistorianDataSource` to that node's +status verbatim, and the driver/wire tests exercise that path; if driver history is ever routed to the server, +refusals will reach clients correctly instead of as empty Good data. A routed deterministic refusal would also +need excluding from the HistoryRead pipeline's retry/breaker handling. + Event-history (`ReadEventsAsync`) is left at the interface default — the interface doesn't yet carry the EventFilter surface needed to forward it. diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IHistoryProvider.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IHistoryProvider.cs index aecc688e..310ac626 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IHistoryProvider.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IHistoryProvider.cs @@ -200,9 +200,14 @@ public sealed class HistoryReadStatusException : Exception /// Creates the exception for the upstream . /// The Bad OPC UA status code to surface for the node. + /// is not a Bad code (severity bits 10/11). public HistoryReadStatusException(uint statusCode) - : base($"The upstream server refused the history read with status 0x{statusCode:X8}.") + : base(IsBad(statusCode) + ? $"The upstream server refused the history read with status 0x{statusCode:X8}." + : throw new ArgumentException($"0x{statusCode:X8} is not a Bad status code.", nameof(statusCode))) => StatusCode = statusCode; + + private static bool IsBad(uint statusCode) => (statusCode & 0x80000000u) != 0; } /// diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs index 976cc852..a18e0341 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient/OpcUaClientDriver.cs @@ -2382,9 +2382,10 @@ public sealed class OpcUaClientDriver : IDriver, ITagDiscovery, IReadable, IWrit // The per-node status decides whether the payload means anything. BadNoData is the one Bad code // that legitimately means "nothing in the window", so it stays an empty result. - if (StatusCode.IsBad(r.StatusCode) && r.StatusCode.Code != StatusCodes.BadNoData) + // CodeBits (not Code) so an info/overflow sub-status bit on the upstream code does not dodge the match. + if (StatusCode.IsBad(r.StatusCode) && r.StatusCode.CodeBits != StatusCodes.BadNoData) { - if (r.StatusCode.Code == StatusCodes.BadAggregateNotSupported && aggregate is { } agg) + if (r.StatusCode.CodeBits == StatusCodes.BadAggregateNotSupported && aggregate is { } agg) throw new HistoryAggregateNotSupportedException(agg); throw new HistoryReadStatusException(r.StatusCode.Code); } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/Historian/HistoryReadStatusExceptionTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/Historian/HistoryReadStatusExceptionTests.cs new file mode 100644 index 00000000..c943a8c1 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/Historian/HistoryReadStatusExceptionTests.cs @@ -0,0 +1,21 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; + +namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests.Historian; + +public sealed class HistoryReadStatusExceptionTests +{ + [Theory] + [InlineData(0x801F0000u)] // BadUserAccessDenied + [InlineData(0xC0000000u)] // severity 11 (reserved Bad) + public void A_Bad_code_is_carried_verbatim(uint code) => + new HistoryReadStatusException(code).StatusCode.ShouldBe(code); + + [Theory] + [InlineData(0x00000000u)] // Good + [InlineData(0x00A70000u)] // GoodNoData + [InlineData(0x40000000u)] // Uncertain + public void A_non_Bad_code_is_rejected(uint code) => + Should.Throw(() => new HistoryReadStatusException(code)); +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/OpcUaClientDriverHistoryWireTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/OpcUaClientDriverHistoryWireTests.cs index eb443fd6..b1afc571 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/OpcUaClientDriverHistoryWireTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.IntegrationTests/OpcUaClientDriverHistoryWireTests.cs @@ -17,6 +17,7 @@ public sealed class OpcUaClientDriverHistoryWireTests private const string HistRef = "Sim/dev1/Hist"; private const string HistTag = "Plant.Hist.Series"; private const uint Uncertain = 0x40000000u; + private const uint BadNoData = 0x809B0000u; private const uint BadUserAccessDenied = 0x801F0000u; private const uint BadHistoryOperationUnsupported = 0x80720000u; @@ -55,6 +56,9 @@ public sealed class OpcUaClientDriverHistoryWireTests // Minimum stands in for "the backend refuses with some OTHER Bad status" (#649). if (aggregate == HistoryAggregateType.Minimum) throw new HistoryReadStatusException(BadUserAccessDenied); + // Maximum stands in for a node-level BadNoData ("nothing in the window"), which must stay empty. + if (aggregate == HistoryAggregateType.Maximum) + throw new HistoryReadStatusException(BadNoData); if (aggregate != HistoryAggregateType.Average) throw new HistoryAggregateNotSupportedException(aggregate); @@ -204,6 +208,21 @@ public sealed class OpcUaClientDriverHistoryWireTests lock (fake.Calls) fake.Calls.ShouldContain($"processed:{HistTag}:Minimum"); } + [Fact] + public async Task A_node_level_BadNoData_stays_an_empty_result_not_an_error() + { + var ct = TestContext.Current.CancellationToken; + var (rig, fake, drv) = await StartAsync("hist-badnodata", ct); + await using var _r = rig; + await using var _d = drv; + + var result = await drv.ReadProcessedAsync( + HistRef, At(0), At(6), TimeSpan.FromSeconds(2), HistoryAggregateType.Maximum, ct); + + result.Samples.ShouldBeEmpty(); + lock (fake.Calls) fake.Calls.ShouldContain($"processed:{HistTag}:Maximum", "the call must have reached the backend"); + } + [Fact] public async Task A_Good_read_with_no_samples_in_the_window_is_still_empty_data_not_an_error() { diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadPagingTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadPagingTests.cs index 16d8bf29..7e9bb01e 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadPagingTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.OpcUaServer.Tests/NodeManagerHistoryReadPagingTests.cs @@ -312,6 +312,61 @@ public sealed class NodeManagerHistoryReadPagingTests : IDisposable await host.DisposeAsync(); } + /// A passthrough backend's Bad status on a RESUME read surfaces verbatim for that node only (#649): + /// no outbound continuation point, the cursor is consumed (no stray cursor), and a sibling node in the same + /// batch is unaffected. + [Fact] + public async Task Raw_resume_read_that_throws_HistoryReadStatusException_surfaces_the_code_and_isolates_the_sibling() + { + var (host, server) = await BootAsync(); + var nm = server.NodeManager!; + nm.HistoryContinuationStore = new InMemoryHistoryContinuationStore(); + var fake = new SeriesHistorianDataSource(MakeSeries(count: 250, stepSeconds: 1)); + nm.HistorianDataSource = fake; + + nm.EnsureVariable("eq-1/bad", parentFolderNodeId: null, displayName: "Bad", dataType: "Double", + writable: false, historianTagname: "WW.Bad", realm: AddressSpaceRealm.Uns); + nm.EnsureVariable("eq-1/ok", parentFolderNodeId: null, displayName: "Ok", dataType: "Double", + writable: false, historianTagname: "WW.Ok", realm: AddressSpaceRealm.Uns); + var badId = nm.TryGetVariable("eq-1/bad")!.NodeId; + var okId = nm.TryGetVariable("eq-1/ok")!.NodeId; + + // Page 1 succeeds and emits a continuation point. + var (_, _, cp1) = ReadRaw(nm, badId, Epoch, Epoch.AddHours(1), max: 100, inboundCp: null); + cp1.ShouldNotBeNull(); + + // The backend now refuses the bad node; one batch resumes it and freshly reads the sibling. + fake.FailTagname = "WW.Bad"; + var context = new OperationContext( + new RequestHeader(), secureChannelContext: null, RequestType.HistoryRead, identity: null); + var details = new ReadRawModifiedDetails + { + StartTime = Epoch, EndTime = Epoch.AddHours(1), NumValuesPerNode = 100, IsReadModified = false, + }; + var nodesToRead = new List + { + new() { NodeId = badId, ContinuationPoint = cp1 }, + new() { NodeId = okId }, + }; + var results = new List { null!, null! }; + var errors = new List { null!, null! }; + nm.HistoryRead(context, details, TimestampsToReturn.Both, + releaseContinuationPoints: false, nodesToRead, results, errors); + + errors[0].StatusCode.Code.ShouldBe(StatusCodes.BadUserAccessDenied); + results[0].ContinuationPoint.ShouldBeNull(); + errors[1].StatusCode.Code.ShouldBe(StatusCodes.Good); + ((HistoryData)ExtensionObject.ToEncodeable(results[1].HistoryData)).DataValues.Count.ShouldBe(100); + results[1].ContinuationPoint.ShouldNotBeNull("the sibling is full-page and pages normally"); + + // The consumed cursor is gone: nothing stray to resume. + fake.FailTagname = null; + var (_, e, _) = ReadRaw(nm, badId, Epoch, Epoch.AddHours(1), max: 100, inboundCp: cp1); + e.StatusCode.Code.ShouldBe(StatusCodes.BadContinuationPointInvalid); + + await host.DisposeAsync(); + } + // --- helpers -------------------------------------------------------------------------------- /// Issue a single-node Raw HistoryRead and return that node's decoded sample values, error, @@ -412,6 +467,9 @@ public sealed class NodeManagerHistoryReadPagingTests : IDisposable { private int _readCount; public int ReadCount => _readCount; + + /// When set, a Raw read of this tagname throws (#649). + public volatile string? FailTagname; public void ResetReadCount() => _readCount = 0; public Task ReadRawAsync( @@ -419,6 +477,8 @@ public sealed class NodeManagerHistoryReadPagingTests : IDisposable CancellationToken cancellationToken) { Interlocked.Increment(ref _readCount); + if (FailTagname is { } bad && bad == fullReference) + throw new HistoryReadStatusException(StatusCodes.BadUserAccessDenied); var window = series .Where(s => (s.SourceTimestampUtc ?? DateTime.MinValue) >= startUtc && (s.SourceTimestampUtc ?? DateTime.MinValue) <= endUtc)