fix(driver-historian-wonderware-client): resolve High code-review finding (Driver.Historian.Wonderware.Client-001)
WonderwareHistorianClient.ReadAtTimeAsync passed the sidecar's reply.Samples straight through ToSnapshots, which violated the IHistorianDataSource contract: the result MUST be the same length and order as the requested timestampsUtc, with gaps returned as Bad-quality snapshots. If the sidecar dropped or reordered samples, OPC UA HistoryReadAtTime would silently misalign values with timestamps. Add an AlignAtTimeSnapshots helper that indexes the returned samples by timestamp ticks, builds the result array at timestampsUtc.Count in request order, and emits a Bad-quality (0x80000000) snapshot for any requested timestamp the sidecar did not return. Add the ReadAtTimeAsync_PartialAndReorderedReply_AlignsByTimestamp_AndFillsGapsAsBad regression test where the fake returns a partial, reordered sample set. Update code-reviews/Driver.Historian.Wonderware.Client/findings.md: -001 Resolved, open-finding count 10 -> 9. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
@@ -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
|
||||
|
||||
|
||||
@@ -109,7 +109,51 @@ public sealed class WonderwareHistorianClient : IHistorianDataSource, IAlarmHist
|
||||
};
|
||||
var reply = await Invoke<ReadAtTimeRequest, ReadAtTimeReply>(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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Reconciles a <c>ReadAtTime</c> sidecar reply against the requested timestamps to
|
||||
/// honour the <see cref="IHistorianDataSource.ReadAtTimeAsync"/> 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 (<c>0x80000000</c>) snapshot rather than positionally misaligning values.
|
||||
/// </summary>
|
||||
private static IReadOnlyList<DataValueSnapshot> AlignAtTimeSnapshots(
|
||||
IReadOnlyList<DateTime> timestampsUtc, HistorianSampleDto[] samples)
|
||||
{
|
||||
// Index returned samples by timestamp ticks. Duplicate timestamps keep the first.
|
||||
var byTicks = new Dictionary<long, HistorianSampleDto>(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<object>(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<HistoricalEventsResult> ReadEventsAsync(
|
||||
|
||||
@@ -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<object>(3.0),
|
||||
Quality = 192, TimestampUtcTicks = t3.Ticks,
|
||||
},
|
||||
new HistorianSampleDto
|
||||
{
|
||||
ValueBytes = MessagePackSerializer.Serialize<object>(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()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user