fix(server): resolve Medium code-review finding (Server-003)
Fix ReadRawAsync: correct XML doc from newest-first to oldest-first (ascending source timestamp per OPC UA Part 11); move maxValuesPerNode cap inside the time-window filter loop so paging limits apply to in-window results only, not the whole buffer snapshot. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -60,13 +60,13 @@
|
|||||||
| Severity | Medium |
|
| Severity | Medium |
|
||||||
| Category | Correctness & logic bugs |
|
| Category | Correctness & logic bugs |
|
||||||
| Location | `src/Server/ZB.MOM.WW.OtOpcUa.Server/Phase7/RingBufferHistoryWriter.cs:96-119` |
|
| Location | `src/Server/ZB.MOM.WW.OtOpcUa.Server/Phase7/RingBufferHistoryWriter.cs:96-119` |
|
||||||
| Status | Open |
|
| Status | Resolved |
|
||||||
|
|
||||||
**Description:** `ReadRawAsync`'s XML doc claims "newest-first," but `TagRingBuffer.Snapshot()` returns oldest-to-newest and the loop preserves that order — so results are oldest-first. Also `maxValuesPerNode` is capped against total buffer size *before* the `[startUtc, endUtc)` filter, so a paged read returns the oldest in-window samples, contradicting the doc and usual HistoryRead expectations.
|
**Description:** `ReadRawAsync`'s XML doc claims "newest-first," but `TagRingBuffer.Snapshot()` returns oldest-to-newest and the loop preserves that order — so results are oldest-first. Also `maxValuesPerNode` is capped against total buffer size *before* the `[startUtc, endUtc)` filter, so a paged read returns the oldest in-window samples, contradicting the doc and usual HistoryRead expectations.
|
||||||
|
|
||||||
**Recommendation:** Make code and doc agree on ordering (raw HistoryRead is normally ascending source-timestamp). Apply `maxValuesPerNode` to the in-window count, not the whole buffer.
|
**Recommendation:** Make code and doc agree on ordering (raw HistoryRead is normally ascending source-timestamp). Apply `maxValuesPerNode` to the in-window count, not the whole buffer.
|
||||||
|
|
||||||
**Resolution:** _(open)_
|
**Resolution:** Resolved 2026-05-22 — corrected XML doc from "newest-first" to "oldest-first (ascending source timestamp, matching OPC UA Part 11 §6.4 raw-values default)"; moved `maxValuesPerNode` cap inside the time-window loop so the limit applies only to in-window results, not the whole buffer snapshot.
|
||||||
|
|
||||||
### Server-004
|
### Server-004
|
||||||
| Field | Value |
|
| Field | Value |
|
||||||
|
|||||||
@@ -90,8 +90,12 @@ public sealed class RingBufferHistoryWriter : IHistoryWriter, IHistorianDataSour
|
|||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Returns samples in the ring buffer whose source timestamp falls within
|
/// Returns samples in the ring buffer whose source timestamp falls within
|
||||||
/// [<paramref name="startUtc"/>, <paramref name="endUtc"/>), newest-first.
|
/// [<paramref name="startUtc"/>, <paramref name="endUtc"/>), oldest-first
|
||||||
/// <paramref name="maxValuesPerNode"/> caps the result count.
|
/// (ascending source timestamp, matching OPC UA Part 11 §6.4 raw-values default).
|
||||||
|
/// <paramref name="maxValuesPerNode"/> caps the count of <em>in-window</em> results,
|
||||||
|
/// not the whole buffer snapshot — a paged read therefore returns the oldest
|
||||||
|
/// <paramref name="maxValuesPerNode"/> samples within the time window, not the oldest
|
||||||
|
/// samples in the buffer irrespective of the window (Server-003).
|
||||||
/// </summary>
|
/// </summary>
|
||||||
public Task<HistoryReadResult> ReadRawAsync(
|
public Task<HistoryReadResult> ReadRawAsync(
|
||||||
string fullReference,
|
string fullReference,
|
||||||
@@ -104,15 +108,19 @@ public sealed class RingBufferHistoryWriter : IHistoryWriter, IHistorianDataSour
|
|||||||
return Task.FromResult(new HistoryReadResult([], null));
|
return Task.FromResult(new HistoryReadResult([], null));
|
||||||
|
|
||||||
var all = buffer.Snapshot();
|
var all = buffer.Snapshot();
|
||||||
var limit = (int)Math.Min(maxValuesPerNode, (uint)all.Length);
|
|
||||||
var result = new List<DataValueSnapshot>(limit);
|
|
||||||
|
|
||||||
|
// Iterate the full snapshot (oldest → newest) and apply the time-window filter first;
|
||||||
|
// then cap with maxValuesPerNode so the limit applies to in-window results only.
|
||||||
|
// Snapshot() returns an array bounded by _capacity so this is O(capacity) at most.
|
||||||
|
var result = new List<DataValueSnapshot>();
|
||||||
foreach (var snap in all)
|
foreach (var snap in all)
|
||||||
{
|
{
|
||||||
if (result.Count >= limit) break;
|
|
||||||
var ts = snap.SourceTimestampUtc ?? snap.ServerTimestampUtc;
|
var ts = snap.SourceTimestampUtc ?? snap.ServerTimestampUtc;
|
||||||
if (ts >= startUtc && ts < endUtc)
|
if (ts >= startUtc && ts < endUtc)
|
||||||
|
{
|
||||||
result.Add(snap);
|
result.Add(snap);
|
||||||
|
if (maxValuesPerNode > 0 && result.Count >= (int)maxValuesPerNode) break;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
return Task.FromResult(new HistoryReadResult(result, null));
|
return Task.FromResult(new HistoryReadResult(result, null));
|
||||||
|
|||||||
Reference in New Issue
Block a user