diff --git a/code-reviews/Driver.Historian.Wonderware.Client/findings.md b/code-reviews/Driver.Historian.Wonderware.Client/findings.md index 5455859..068d3f8 100644 --- a/code-reviews/Driver.Historian.Wonderware.Client/findings.md +++ b/code-reviews/Driver.Historian.Wonderware.Client/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 10 | +| Open findings | 9 | ## Checklist coverage @@ -36,7 +36,7 @@ a category produced nothing rather than leaving it blank. | Severity | High | | Category | Correctness & logic bugs | | Location | `WonderwareHistorianClient.cs:98-113` | -| Status | Open | +| Status | Resolved | **Description:** `ReadAtTimeAsync` violates the explicit `IHistorianDataSource.ReadAtTimeAsync` contract. The interface XML doc states: the returned list MUST be the same length and @@ -55,7 +55,7 @@ entries, and emit a Bad-quality (`0x80000000`) snapshot for any requested timest sidecar did not return. Alternatively assert `reply.Samples.Length == timestampsUtc.Count` and fail loudly. Add a test where the fake returns a partial/reordered sample set. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `ReadAtTimeAsync` now reconciles the sidecar reply against the requested timestamps via a new `AlignAtTimeSnapshots` helper: it indexes returned samples by timestamp ticks, builds the result at `timestampsUtc.Count` in request order, and emits a Bad-quality (`0x80000000`) snapshot for any requested timestamp the sidecar did not return; added the `ReadAtTimeAsync_PartialAndReorderedReply_AlignsByTimestamp_AndFillsGapsAsBad` regression test. ### Driver.Historian.Wonderware.Client-002 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client/WonderwareHistorianClient.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client/WonderwareHistorianClient.cs index 916ec64..49dc4a2 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client/WonderwareHistorianClient.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client/WonderwareHistorianClient.cs @@ -109,7 +109,51 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist }; var reply = await Invoke(MessageKind.ReadAtTimeRequest, MessageKind.ReadAtTimeReply, req, cancellationToken).ConfigureAwait(false); ThrowIfFailed(reply.Success, reply.Error, "ReadAtTime"); - return new HistoryReadResult(ToSnapshots(reply.Samples), ContinuationPoint: null); + return new HistoryReadResult(AlignAtTimeSnapshots(timestampsUtc, reply.Samples), ContinuationPoint: null); + } + + /// + /// Reconciles a ReadAtTime sidecar reply against the requested timestamps to + /// honour the contract: the result + /// MUST have exactly one snapshot per requested timestamp, in request order. The sidecar + /// is not required to return a sample for every timestamp (e.g. it may drop + /// boundary-less timestamps) nor to preserve order, so each requested timestamp is + /// matched by ticks; any timestamp the sidecar did not return is filled with a + /// Bad-quality (0x80000000) snapshot rather than positionally misaligning values. + /// + private static IReadOnlyList AlignAtTimeSnapshots( + IReadOnlyList timestampsUtc, HistorianSampleDto[] samples) + { + // Index returned samples by timestamp ticks. Duplicate timestamps keep the first. + var byTicks = new Dictionary(samples.Length); + foreach (var sample in samples) + byTicks.TryAdd(sample.TimestampUtcTicks, sample); + + var result = new DataValueSnapshot[timestampsUtc.Count]; + for (var i = 0; i < timestampsUtc.Count; i++) + { + var requested = DateTime.SpecifyKind(timestampsUtc[i], DateTimeKind.Utc); + if (byTicks.TryGetValue(requested.Ticks, out var dto)) + { + var value = dto.ValueBytes is null ? null : MessagePackSerializer.Deserialize(dto.ValueBytes); + result[i] = new DataValueSnapshot( + Value: value, + StatusCode: QualityMapper.Map(dto.Quality), + SourceTimestampUtc: requested, + ServerTimestampUtc: DateTime.UtcNow); + } + else + { + // Gap — sidecar returned no sample for this timestamp. Per the contract this + // is a Bad-quality snapshot stamped at the requested time, not a dropped row. + result[i] = new DataValueSnapshot( + Value: null, + StatusCode: 0x80000000u, // Bad + SourceTimestampUtc: requested, + ServerTimestampUtc: DateTime.UtcNow); + } + } + return result; } public async Task ReadEventsAsync( diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/WonderwareHistorianClientTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/WonderwareHistorianClientTests.cs index ac8c2cb..381d631 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/WonderwareHistorianClientTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Historian.Wonderware.Client.Tests/WonderwareHistorianClientTests.cs @@ -127,6 +127,59 @@ public sealed class WonderwareHistorianClientTests result.Samples[1].SourceTimestampUtc.ShouldBe(t2); } + [Fact] + public async Task ReadAtTimeAsync_PartialAndReorderedReply_AlignsByTimestamp_AndFillsGapsAsBad() + { + var pipe = UniquePipeName(); + var t1 = new DateTime(2026, 4, 29, 1, 0, 0, DateTimeKind.Utc); + var t2 = new DateTime(2026, 4, 29, 2, 0, 0, DateTimeKind.Utc); + var t3 = new DateTime(2026, 4, 29, 3, 0, 0, DateTimeKind.Utc); + + await using var server = new FakeSidecarServer(pipe, Secret) + { + // Sidecar returns only t3 and t1 (out of order), drops t2 entirely. A + // contract-compliant client must realign by timestamp and synthesize a + // Bad-quality snapshot for the missing t2. + OnReadAtTime = _ => new ReadAtTimeReply + { + Success = true, + Samples = + [ + new HistorianSampleDto + { + ValueBytes = MessagePackSerializer.Serialize(3.0), + Quality = 192, TimestampUtcTicks = t3.Ticks, + }, + new HistorianSampleDto + { + ValueBytes = MessagePackSerializer.Serialize(1.0), + Quality = 192, TimestampUtcTicks = t1.Ticks, + }, + ], + }, + }; + await server.StartAsync(); + + await using var client = new WonderwareHistorianClient(OptsFor(pipe)); + var result = await client.ReadAtTimeAsync("Tank.Level", new[] { t1, t2, t3 }, CancellationToken.None); + + // Result MUST be the same length and order as the request. + result.Samples.Count.ShouldBe(3); + + result.Samples[0].SourceTimestampUtc.ShouldBe(t1); + result.Samples[0].StatusCode.ShouldBe(0x00000000u); // Good + result.Samples[0].Value.ShouldBe(1.0); + + // t2 was not returned by the sidecar → Bad-quality gap snapshot at the requested time. + result.Samples[1].SourceTimestampUtc.ShouldBe(t2); + result.Samples[1].StatusCode.ShouldBe(0x80000000u); // Bad + result.Samples[1].Value.ShouldBeNull(); + + result.Samples[2].SourceTimestampUtc.ShouldBe(t3); + result.Samples[2].StatusCode.ShouldBe(0x00000000u); // Good + result.Samples[2].Value.ShouldBe(3.0); + } + [Fact] public async Task ReadEventsAsync_PreservesEventFields() {