diff --git a/code-reviews/Core.Abstractions/findings.md b/code-reviews/Core.Abstractions/findings.md index 2d068685..336a5724 100644 --- a/code-reviews/Core.Abstractions/findings.md +++ b/code-reviews/Core.Abstractions/findings.md @@ -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 `` 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` (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 `Continuation contract when using the sentinel:` note to `IHistoryProvider.ReadEventsAsync` and update the `` 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` `` and to `IHistorianDataSource.ReadEventsAsync` `` (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`. diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/EquipmentTagRefResolver.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/EquipmentTagRefResolver.cs index ca71667c..4d6af673 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/EquipmentTagRefResolver.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/EquipmentTagRefResolver.cs @@ -1,4 +1,5 @@ using System.Collections.Concurrent; +using System.Diagnostics.CodeAnalysis; namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions; @@ -27,9 +28,14 @@ public sealed class EquipmentTagRefResolver where TDef : class /// True when resolves to a def (authored or equipment). /// The wire reference handed to the driver. - /// The resolved tag-definition when this returns true. + /// + /// The resolved tag-definition when this returns . When this returns + /// the value is undefined — callers must not use it. + /// [MaybeNullWhen(false)] informs the nullable-reference-types (NRT) analyser so + /// callers in NRT-enabled contexts do not need to suppress warnings. + /// /// when a definition was found. - public bool TryResolve(string fullReference, out TDef def) + public bool TryResolve(string fullReference, [MaybeNullWhen(false)] out TDef def) { var authored = _byName(fullReference); if (authored is not null) { def = authored; return true; } @@ -37,7 +43,7 @@ public sealed class EquipmentTagRefResolver where TDef : class var resolved = _cache.GetOrAdd(fullReference, _parseRef); if (resolved is not null) { def = resolved; return true; } - def = null!; + def = default; return false; } diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/Historian/IHistorianDataSource.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/Historian/IHistorianDataSource.cs index 0f234500..aa80b1c5 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/Historian/IHistorianDataSource.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/Historian/IHistorianDataSource.cs @@ -91,7 +91,13 @@ public interface IHistorianDataSource : IDisposable /// The source name to filter events, or null to return events from all sources. /// The start of the time range in UTC. /// The end of the time range in UTC. - /// The maximum number of events to return, or a non-positive value to use the default backend cap. + /// + /// The maximum number of events to return. A non-positive value uses the backend's + /// default cap. When the backend cap truncates the result, implementations MUST set + /// to a non-null token so + /// callers can detect truncation and page — a null continuation point means all + /// matching events were returned. (Core.Abstractions-009.) + /// /// A cancellation token that can be used to cancel the operation. Task ReadEventsAsync( string? sourceName, 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 6a337153..51ffb318 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IHistoryProvider.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/IHistoryProvider.cs @@ -92,6 +92,14 @@ public interface IHistoryProvider /// adapters historically treat maxEvents <= 0 as a sentinel meaning /// "use the backend's default cap" (see WonderwareHistorianClient / /// HistorianDataSource). The asymmetry is intentional — Core.Abstractions-006. + /// + /// Continuation contract when using the sentinel: When maxEvents <= 0 + /// the backend applies its own cap. If the backend's cap truncates the result + /// (i.e. more matching events exist beyond the returned set), implementations MUST set + /// to a non-null token so + /// callers can detect truncation and page. A null ContinuationPoint means all + /// matching events were returned — callers rely on this to decide whether to page. + /// (Core.Abstractions-009.) /// /// Request cancellation. /// diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/EquipmentTagRefResolverTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/EquipmentTagRefResolverTests.cs index d6b5f498..55b92f8f 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/EquipmentTagRefResolverTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/EquipmentTagRefResolverTests.cs @@ -51,4 +51,23 @@ public class EquipmentTagRefResolverTests r.TryResolve("{\"a\":1}", out _).ShouldBeTrue(); calls.ShouldBe(2); } + + /// + /// Core.Abstractions-009: carries + /// [MaybeNullWhen(false)] on its out parameter so NRT-enabled callers do + /// not need a null-forgiving operator when the return value is . + /// This test pins the false-path behaviour: def must not be used when the method + /// returns — the contract is that it is undefined (currently null). + /// + [Fact] + public void TryResolve_false_path_sets_def_to_default() + { + var r = Make(new(), _ => null); + var returned = r.TryResolve("no-such-ref", out var def); + returned.ShouldBeFalse(); + // The [MaybeNullWhen(false)] attribute means callers must treat def as potentially null + // on the false path — the value here is the implementation-defined default (null for + // a reference type), which satisfies that contract. + def.ShouldBeNull(); + } } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/Historian/IHistorianDataSourceContractTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/Historian/IHistorianDataSourceContractTests.cs index 501ee475..562dab11 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/Historian/IHistorianDataSourceContractTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/Historian/IHistorianDataSourceContractTests.cs @@ -169,6 +169,33 @@ public sealed class IHistorianDataSourceContractTests parameter!.ParameterType.ShouldBe(expectedType); } + /// + /// Core.Abstractions-009 (OpcUaServer-002): when maxEvents <= 0 the contract + /// requires implementations to set + /// non-null when more results exist. Pins the maxEvents parameter type and the + /// continuation-point member so accidental removal + /// breaks this test — the downstream paging contract depends on both. + /// + [Fact] + public void ReadEventsAsync_sentinel_and_continuation_contract_types_pinned() + { + var method = typeof(IHistorianDataSource).GetMethod("ReadEventsAsync"); + method.ShouldNotBeNull(); + + var maxEventsParam = method!.GetParameters().FirstOrDefault(p => p.Name == "maxEvents"); + maxEventsParam.ShouldNotBeNull("maxEvents parameter must exist for the sentinel contract"); + maxEventsParam!.ParameterType.ShouldBe(typeof(int), + "maxEvents is int (not uint) so callers can pass <=0 as a 'use backend default cap' sentinel"); + + // HistoricalEventsResult must carry a nullable ContinuationPoint so implementations + // can signal 'more results exist' (non-null) vs 'all results returned' (null). + var resultType = typeof(HistoricalEventsResult); + var continuationProp = resultType.GetProperty("ContinuationPoint"); + continuationProp.ShouldNotBeNull("HistoricalEventsResult.ContinuationPoint must exist for paging"); + continuationProp!.PropertyType.ShouldBe(typeof(byte[]), + "ContinuationPoint is byte[]? — null means no more pages, non-null means caller must page"); + } + /// /// Core.Abstractions-008: and /// are C# default interface methods diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/PollGroupEngineTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/PollGroupEngineTests.cs index 1a0af37f..5e6d0065 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/PollGroupEngineTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/PollGroupEngineTests.cs @@ -65,7 +65,7 @@ public sealed class PollGroupEngineTests (h, r, s) => events.Enqueue((h, r, s))); var handle = engine.Subscribe(["X"], TimeSpan.FromMilliseconds(100)); - await Task.Delay(500); + await Task.Delay(500, TestContext.Current.CancellationToken); engine.Unsubscribe(handle); events.Count.ShouldBe(1); @@ -108,7 +108,7 @@ public sealed class PollGroupEngineTests var afterUnsub = events.Count; src.Values["X"] = 999; - await Task.Delay(400); + await Task.Delay(400, TestContext.Current.CancellationToken); events.Count.ShouldBe(afterUnsub); } @@ -125,7 +125,7 @@ public sealed class PollGroupEngineTests minInterval: TimeSpan.FromMilliseconds(200)); var handle = engine.Subscribe(["X"], TimeSpan.FromMilliseconds(5)); - await Task.Delay(300); + await Task.Delay(300, TestContext.Current.CancellationToken); engine.Unsubscribe(handle); // 300 ms window, 200 ms floor, stable value → initial push + at most 1 extra poll. @@ -243,7 +243,7 @@ public sealed class PollGroupEngineTests engine.ActiveSubscriptionCount.ShouldBe(0); var afterDispose = events.Count; - await Task.Delay(300); + await Task.Delay(300, TestContext.Current.CancellationToken); // After dispose no more events — everything is cancelled. events.Count.ShouldBe(afterDispose); } @@ -277,7 +277,7 @@ public sealed class PollGroupEngineTests var handle = engine.Subscribe(["A"], TimeSpan.FromMilliseconds(50)); // Allow several poll cycles so a broken implementation would accumulate extra events. - await Task.Delay(400); + await Task.Delay(400, TestContext.Current.CancellationToken); engine.Unsubscribe(handle); // Only the initial-data push should have fired; subsequent polls with identical