dohertj2/lmxopcua
1
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

review(Core.Abstractions): document ReadEventsAsync continuation contract (OpcUaServer-002 root)

Browse Source 
Re-review at 7286d320. Core.Abstractions-009: ReadEventsAsync maxEvents<=0 sentinel now
documents the implementer's continuation-point obligation when a backend cap truncates
(the root of OpcUaServer-002). -010: PollGroupEngineTests pass CancellationToken. Plus
EquipmentTagRefResolver.TryResolve [MaybeNullWhen(false)] NRT cleanup + test.
This commit is contained in:
Joseph Doherty
2026-06-19 11:06:56 -04:00
parent 354b0e7613
commit 65e6af6001
7 changed files with 135 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/Core.Abstractions/findings.md
+60 -3
View File
@@ -4,12 +4,12 @@
|---|---|
| Module | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions` |
| Reviewer | Claude Code |
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Review date | 2026-06-19 (re-review; original 2026-05-22) |
| Commit reviewed | `7286d320` |
| Status | Reviewed |
| Open findings | 0 |
## Checklist coverage
## Checklist coverage — original review (76d35d1, 2026-05-22)
A comprehensive review completes every category, recording "No issues found" where
a category produced nothing rather than leaving it blank.
@@ -27,6 +27,21 @@ a category produced nothing rather than leaving it blank.
| 9 | Testing coverage | Core.Abstractions-007 |
| 10 | Documentation & comments | Core.Abstractions-008 |
## Checklist coverage — re-review (7286d320, 2026-06-19)
| # | Category | Result |
|---|---|---|
| 1 | Correctness & logic bugs | No new issues found |
| 2 | OtOpcUa conventions | No issues found |
| 3 | Concurrency & thread safety | No new issues found |
| 4 | Error handling & resilience | No new issues found |
| 5 | Security | No issues found |
| 6 | Performance & resource management | No issues found |
| 7 | Design-document adherence | No issues found |
| 8 | Code organization & conventions | No new issues found |
| 9 | Testing coverage | Core.Abstractions-010 |
| 10 | Documentation & comments | Core.Abstractions-009 |
## Findings
### Core.Abstractions-001
@@ -154,3 +169,45 @@ a category produced nothing rather than leaving it blank.
**Recommendation:** Reword `DriverHealth.LastError` to "Most recent error message; may be null when no error has been recorded" without tying nullness to a specific state. Add a one-line note on `IHistoryProvider`/`IHistorianDataSource` explaining why one surface uses default methods and the other does not.
**Resolution:** Resolved 2026-05-23 — reworded `DriverHealth.LastError` to "null when no error has been recorded" and called out that the field is independent of `State`. Added an asymmetry `<remarks>` to `IHistoryProvider` (default-impl-throws so legacy drivers compile) and to `IHistorianDataSource.ReadEventsAsync` (required because server-side historians own the full surface) cross-referencing the finding. Added `LastError_IsIndependent_OfState` + `DriverState_EnumContainsExpectedMembers` and `HistoryProvider_OptionalMethods_HaveDefaultImplementation` contract tests so the asymmetry pins.
---
## Re-review 2026-06-19 (commit 7286d320)
Changes since `76d35d1`: all eight original findings resolved. New code added: `EquipmentTagRefResolver<TDef>` (generic helper bridging authored tag-table lookup and equipment-tag JSON ref parsing, with `ConcurrentDictionary` negative-result cache), `NullHistorianDataSource` singleton, `AlarmAcknowledgeRequest.OperatorUser` optional param, `AlarmEventArgs.Kind` (`AlarmTransitionKind`), `NamespaceKindCompatibility.SystemPlatform` removed, `AlarmConditionInfo` sub-attribute refs, numerous XML-doc backfill. Test count rose to 88 at the prior commit. Re-review scope: verify all prior findings are properly closed, evaluate all new code against the 10 categories, and assess the OpcUaServer-002 cross-module item.
### Core.Abstractions-009
| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IHistoryProvider.cs:89-107`, `src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/Historian/IHistorianDataSource.cs:91-101` |
| Status | Resolved |
**Description (OpcUaServer-002 verdict):** The `ReadEventsAsync` `maxEvents <= 0` sentinel contract is present (Core.Abstractions-006 fix), but neither `IHistoryProvider.ReadEventsAsync` nor `IHistorianDataSource.ReadEventsAsync` specifies what a compliant implementation MUST do with `ContinuationPoint` when the backend caps the result. Without this, callers cannot distinguish "all matching events were returned" from "the backend hit its cap and silently truncated." The OpcUaServer review noted that when `NumValuesPerNode == 0` ("all events") the server dispatches with `maxEvents = 0` and gets back a result whose `ContinuationPoint` is null even when the backend capped it — causing the server to report `GoodNoData` / empty with no `GoodMoreData` response. This is a contract-under-specification issue in the interface definition: it needs to state that when the backend cap truncates, `ContinuationPoint` MUST be non-null.
Changing the signature is forbidden (public-contract break); the fix is a documentation clarification.
**Recommendation:** Add a `<b>Continuation contract when using the sentinel:</b>` note to `IHistoryProvider.ReadEventsAsync` and update the `<param name="maxEvents">` doc in `IHistorianDataSource.ReadEventsAsync` to state: implementations MUST set `ContinuationPoint` non-null when the backend cap truncates the result; null means all matching events were returned.
**Resolution:** Resolved 2026-06-19 — added explicit continuation-contract note to `IHistoryProvider.ReadEventsAsync` `<param name="maxEvents">` and to `IHistorianDataSource.ReadEventsAsync` `<param name="maxEvents">` (both: "when backend cap truncates, MUST set ContinuationPoint non-null; null means all events returned"). Added `ReadEventsAsync_sentinel_and_continuation_contract_types_pinned` contract test in `IHistorianDataSourceContractTests` pinning `maxEvents` type and `ContinuationPoint` member presence. SHA: (2026-06-19, blank — not committed).
### Core.Abstractions-010
| Field | Value |
|---|---|
| Severity | Low |
| Category | Testing coverage |
| Location | `tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/PollGroupEngineTests.cs:68,111,128,246,280` |
| Status | Resolved |
**Description:** Five `Task.Delay(ms)` calls in `PollGroupEngineTests` omit a `CancellationToken`, triggering xUnit v3 analyser warning xUnit1051 ("Calls to methods which accept CancellationToken should use `TestContext.Current.CancellationToken`"). These delays cannot be cancelled when a test is cancelled or times out, making those test methods unresponsive to test-runner abort signals.
**Recommendation:** Pass `TestContext.Current.CancellationToken` as the second argument to every `Task.Delay` call inside test methods.
**Resolution:** Resolved 2026-06-19 — updated all five bare `Task.Delay(ms)` calls in `PollGroupEngineTests` (lines 68, 111, 128, 246, 280 at review commit) to `Task.Delay(ms, TestContext.Current.CancellationToken)`. Build warnings eliminated; test count unchanged at 88 (+ 2 new from -009 fix = 90 total). SHA: (2026-06-19, blank — not committed).
#### Note: EquipmentTagRefResolver null-safety improvement (not a separately-tracked finding)
The `TryResolve` out-parameter `def` was typed as `out TDef def` (non-nullable) but set to `null!` on the false path — idiomatic but silently unsound in NRT-enabled callers. Added `[MaybeNullWhen(false)]` attribute and changed `null!` to `default`. Pinned by new `TryResolve_false_path_sets_def_to_default` test in `EquipmentTagRefResolverTests`.