diff --git a/code-reviews/Driver.OpcUaClient.Contracts/findings.md b/code-reviews/Driver.OpcUaClient.Contracts/findings.md new file mode 100644 index 00000000..114e47cc --- /dev/null +++ b/code-reviews/Driver.OpcUaClient.Contracts/findings.md @@ -0,0 +1,80 @@ +# Code Review — Driver.OpcUaClient.Contracts + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts` | +| Reviewer | Claude Code | +| Review date | 2026-06-19 | +| Commit reviewed | `a19b0f86` | +| Status | Reviewed | +| Open findings | 1 | + +## Checklist coverage + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | Driver.OpcUaClient.Contracts-001 | +| 2 | OtOpcUa conventions | Driver.OpcUaClient.Contracts-002 | +| 3 | Concurrency & thread safety | No issues found | +| 4 | Error handling & resilience | No issues found | +| 5 | Security | No issues found | +| 6 | Performance & resource management | No issues found | +| 7 | Design-document adherence | Driver.OpcUaClient.Contracts-002 | +| 8 | Code organization & conventions | Driver.OpcUaClient.Contracts-003 | +| 9 | Testing coverage | No test project — no issues to record | +| 10 | Documentation & comments | No issues found | + +## Findings + +### Driver.OpcUaClient.Contracts-001 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/NamespaceMap.cs:118-134` | +| Status | Resolved | + +**Description:** `TryResolve` accepts `currentSession` as a non-nullable `ISession` parameter but contains a runtime null guard (`if (currentSession is null || …) return false`). The non-nullable annotation tells callers that null is invalid, yet the implementation silently returns false on null — masking the bug that prompted the guard. More practically: on the driver's reconnect path `_namespaceMap` is rebuilt under `_probeLock`, but `TryResolve` is called outside that lock (at the read/write call-site). A session handed to `TryResolve` concurrently with a session swap could be non-null at the call site but torn-down inside the call; the silent false return gives the caller no indication that resolution failed due to a racing session swap versus a genuinely unknown NodeId. The `out NodeId nodeId` parameter contract also does not document that callers must use `NodeId.IsNull(nodeId)` (OPC UA SDK idiom) rather than `nodeId == null` (C# null) to test the out-value — no current caller makes this mistake, but the distinction is non-obvious. + +**Recommendation:** Change the parameter to `ISession? currentSession` (nullable) so the annotation matches reality and the compiler guides callers. Add XML-doc on the `out nodeId` parameter specifying `NodeId.IsNull(nodeId)` is the correct test. + +**Resolution:** Resolved 2026-06-19, verified by build (no test project). Changed `ISession currentSession` to `ISession? currentSession` so the nullable annotation matches the runtime guard. Expanded XML doc on `currentSession` to explain the defensive-null contract and on `nodeId` to specify `NodeId.IsNull(nodeId)` as the correct test. Browser and runtime driver both build clean (0 errors, 0 warnings) with `TreatWarningsAsErrors=true`. + +--- + +### Driver.OpcUaClient.Contracts-002 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | OtOpcUa conventions / Design-document adherence | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts.csproj:9` | +| Status | Open | + +**Description:** The Contracts project carries a direct `PackageReference` to `OPCFoundation.NetStandard.Opc.Ua.Client` solely to support `NamespaceMap`. This makes the Contracts project not a lightweight DTO/enum assembly: every consumer (AdminUI serializers, future test helpers, config-only tooling) that references Contracts for `OpcUaClientDriverOptions` and its enums also transitively receives the full OPC UA client SDK. Both `OpcUaClientDriver` and `OpcUaClientDriverBrowser` already reference the SDK directly and could host `NamespaceMap` without any new dependency. Moving `NamespaceMap` to the runtime driver (or to a dedicated shared helper project referenced by both driver and browser) would restore the Contracts project to its intended SDK-free role. + +**Recommendation:** Move `NamespaceMap` out of Contracts into `ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient` (or an internal shared helper). Remove the `OPCFoundation.NetStandard.Opc.Ua.Client` reference from the Contracts `.csproj`. `NamespaceMap` is not part of the public wire contract; it is an implementation detail of the driver's namespace-stability mechanism. + +**Resolution:** _(empty until closed — deferred: refactor affects driver and browser project references; no wire or public-contract change, but requires updating ProjectReference entries in two .csproj files and adjusting the Browser's using directive)_ + +--- + +### Driver.OpcUaClient.Contracts-003 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Code organization & conventions | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/OpcUaClientDriverOptions.cs:131-138`, `:174-176` | +| Status | Resolved | + +**Description:** Two inconsistency gaps in the options class: + +1. `[Range(1, 60)]` is applied only to `ProbeTimeoutSeconds`. `MaxDiscoveredNodes` and `MaxBrowseDepth` have meaningful minimum bounds (zero or negative values produce silent misbehaviour: `MaxDiscoveredNodes = 0` halts all discovery, `MaxBrowseDepth = 0` skips all browsing). No `[Range]` guards exist for these knobs. + +2. The `[Display(Name = …, Description = …, GroupName = "Diagnostics")]` attribute on `ProbeTimeoutSeconds` is unused dead metadata: the AdminUI page binds the field directly via `InputNumber @bind-Value` and does not read `[Display]` attributes. No UI scaffold, reflection-based form generator, or API documentation tool in the codebase consumes it. Having a `[Display]` on one property of 18 properties creates a false impression that more of the class is attributed. + +**Recommendation:** Add `[Range(1, int.MaxValue)]` to `MaxDiscoveredNodes` and `MaxBrowseDepth`. Remove the `[Display]` from `ProbeTimeoutSeconds` (retaining `[Range(1, 60)]` which is enforced by the AdminUI's `DataAnnotationsValidator`). + +**Resolution:** Resolved 2026-06-19, verified by build (no test project). Added `[Range(1, int.MaxValue)]` to `MaxDiscoveredNodes` and `MaxBrowseDepth`. Removed the dead `[Display(…)]` attribute from `ProbeTimeoutSeconds`; `[Range(1, 60)]` retained. All three projects (Contracts, Driver, Browser) build clean with `TreatWarningsAsErrors=true`. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/NamespaceMap.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/NamespaceMap.cs index 82ab003f..ed4cd3ee 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/NamespaceMap.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/NamespaceMap.cs @@ -110,12 +110,16 @@ public sealed class NamespaceMap /// or a plain ns=N;… NodeId — against the /// 's namespace table. A nsu=… reference is /// re-bound through the current session's URI table so a remote reorder since connect - /// time is corrected. Returns false for empty/malformed input or an unknown URI. + /// time is corrected. Returns false for empty/malformed input, an unknown URI, or a + /// null session (defensive — callers on the reconnect path may pass a session that has + /// been torn down concurrently). /// - /// The OPC UA session whose namespace table to resolve against. + /// The live OPC UA session to resolve against. Passing null + /// returns false rather than throwing, matching the TryXxx contract. /// The reference string to resolve (either nsu=… or ns=N;… format). - /// On success, the resolved NodeId; otherwise NodeId.Null. - public static bool TryResolve(ISession currentSession, string reference, out NodeId nodeId) + /// On success, the resolved NodeId; on failure, NodeId.Null. + /// Use — not == null — to test this value. + public static bool TryResolve(ISession? currentSession, string reference, out NodeId nodeId) { nodeId = NodeId.Null; if (currentSession is null || string.IsNullOrWhiteSpace(reference)) return false; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/OpcUaClientDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/OpcUaClientDriverOptions.cs index d979cd6d..022a45ef 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/OpcUaClientDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/OpcUaClientDriverOptions.cs @@ -128,6 +128,7 @@ public sealed class OpcUaClientDriverOptions /// written to the driver health surface; the partially-discovered tree is still /// projected into the local address space. /// + [Range(1, int.MaxValue)] public int MaxDiscoveredNodes { get; init; } = 10_000; /// @@ -135,6 +136,7 @@ public sealed class OpcUaClientDriverOptions /// OPC UA information models, shallow enough that cyclic graphs can't spin the /// browse forever. /// + [Range(1, int.MaxValue)] public int MaxBrowseDepth { get; init; } = 10; /// @@ -171,7 +173,6 @@ public sealed class OpcUaClientDriverOptions /// Timeout for the AdminUI Test Connect probe, in seconds. The AdminUI clamps to a /// 60s server-side maximum; this default is what the form pre-fills for new instances. /// - [Display(Name = "Probe timeout (seconds)", Description = "Connection test timeout. Default 15s.", GroupName = "Diagnostics")] [Range(1, 60)] public int ProbeTimeoutSeconds { get; init; } = 15; }