diff --git a/code-reviews/Commons/findings.md b/code-reviews/Commons/findings.md new file mode 100644 index 00000000..b8996779 --- /dev/null +++ b/code-reviews/Commons/findings.md @@ -0,0 +1,120 @@ +# Code Review — Commons + + + +| Field | Value | +|---|---| +| Module | `src/Core/ZB.MOM.WW.OtOpcUa.Commons` | +| Reviewer | Claude Code | +| Review date | 2026-06-19 | +| Commit reviewed | `7286d320` | +| Status | Reviewed | +| Open findings | 0 | + +## Checklist coverage + +A comprehensive review completes every category, recording "No issues found" where +a category produced nothing rather than leaving it blank. + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | No issues found | +| 2 | OtOpcUa conventions | No issues found | +| 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 | No issues found | +| 8 | Code organization & conventions | Commons-002 (deferred) | +| 9 | Testing coverage | Commons-003, Commons-004 | +| 10 | Documentation & comments | Commons-001 | + +## Findings + +### Commons-001 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs:79` | +| Status | Resolved | + +**Description:** The XML doc comment on `StartAddressSpaceRebuildSpan` read "Phase7 plan → +apply", referencing the old `Phase7*` naming that was renamed to `AddressSpace*` in commit +`40e8a23e` (2026-06-18). The method name itself was correct; only the summary text was stale. + +**Recommendation:** Replace "Phase7 plan → apply" with "AddressSpace plan → apply" in the summary. + +**Resolution:** Fixed 2026-06-19 — updated stale "Phase7 plan → apply" to "AddressSpace plan → apply" in the XML doc comment (src edit, no commit SHA yet). + +--- + +### Commons-002 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Code organization & conventions | +| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Commons/Messages/Admin/RestartDriver.cs:27,36`, `ReconnectDriver.cs:16,25`, `TestDriverConnect.cs:16,27` | +| Status | Deferred | + +**Description:** `RestartDriver`, `ReconnectDriver`, and `TestDriverConnect` (plus their result +records) carry `Guid CorrelationId` parameters using the raw `System.Guid` type, while every +other Commons message record uses the typed `CorrelationId` wrapper. This divergence requires +callers to manually extract `.Value` or wrap a raw `Guid` when correlating across these +boundaries. The inconsistency appears to be a historical accident — these three messages +predate the `CorrelationId` struct. + +**Recommendation:** Migrate `Guid CorrelationId` → `CorrelationId CorrelationId` across +`RestartDriver`, `ReconnectDriver`, `TestDriverConnect` and their result records. This is a +cross-cutting public-API change that touches callers in at least AdminUI, Runtime, and Cluster +projects; defer to a coordinated cleanup pass. + +**Resolution:** Deferred — cross-cutting public-API change requiring coordinated update across at least three other modules (AdminUI, Runtime, Cluster). Safe to defer. + +--- + +### Commons-003 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Testing coverage | +| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Commons/OpcUa/DeferredAddressSpaceSink.cs`, `DeferredServiceLevelPublisher.cs` | +| Status | Resolved | + +**Description:** `DeferredAddressSpaceSink` and `DeferredServiceLevelPublisher` had no unit +tests. The deferred-sink pattern is a production-critical seam (a previously recorded MEMORY.md +gotcha: "a new optional capability interface MUST be forwarded through +`DeferredAddressSpaceSink` or it's inert on every driver-role host"). Key invariants now +covered: (1) before `SetSink`, all write operations are safe no-ops; (2) after `SetSink`, +operations are forwarded; (3) `UpdateTagAttributes` returns `false` for non-surgical inners and +`true` for surgical inners; (4) `SetSink(null)` / `SetInner(null)` reverts to the null sink. + +**Recommendation:** Add `DeferredAddressSpaceSinkTests` and `DeferredServiceLevelPublisherTests` +under `tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/`. + +**Resolution:** Fixed 2026-06-19 — added `DeferredAddressSpaceSinkTests.cs` (8 tests) and `DeferredServiceLevelPublisherTests.cs` (3 tests) covering the five invariants; all pass. + +--- + +### Commons-004 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Testing coverage | +| Location | `tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/EquipmentNodeIdsTests.cs` | +| Status | Resolved | + +**Description:** `EquipmentNodeIds.Variable` uses `IsNullOrWhiteSpace` to detect empty +`folderPath`, so a whitespace-only value (e.g. `" "`) is treated identically to +null/empty — the parent falls back to `equipmentId` directly. This is the correct behaviour +but the whitespace-only branch had no test, leaving the intent implicit. + +**Recommendation:** Add an `[InlineData]` case covering whitespace-only `folderPath`. + +**Resolution:** Fixed 2026-06-19 — added `Variable_with_whitespace_folder_is_equipment_slash_name` test to `EquipmentNodeIdsTests.cs`; passes. diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs index 1a284860..33553180 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Commons/Observability/OtOpcUaTelemetry.cs @@ -76,7 +76,7 @@ public static class OtOpcUaTelemetry return activity; } - /// Span wrapping a full OPC UA address-space rebuild (Phase7 plan → apply). + /// Span wrapping a full OPC UA address-space rebuild (AddressSpace plan → apply). public static Activity? StartAddressSpaceRebuildSpan() => ActivitySource.StartActivity("otopcua.opcua.address_space_rebuild", ActivityKind.Internal); } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/DeferredAddressSpaceSinkTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/DeferredAddressSpaceSinkTests.cs new file mode 100644 index 00000000..885fb595 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/DeferredAddressSpaceSinkTests.cs @@ -0,0 +1,154 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; + +namespace ZB.MOM.WW.OtOpcUa.Commons.Tests.OpcUa; + +/// +/// Covers the deferred-sink forwarding invariants documented in Commons-003. +/// The DeferredAddressSpaceSink is a production-critical seam: a new optional capability +/// interface MUST be forwarded through it or the optimization is inert on every driver-role +/// host (DeferredAddressSpaceSink is what actors inject, not the inner sink directly). +/// +public class DeferredAddressSpaceSinkTests +{ + // ---------- before SetSink — all calls are safe no-ops ---------- + + [Fact] + public void Before_SetSink_WriteValue_is_a_noop() + { + var sink = new DeferredAddressSpaceSink(); + // Must not throw. + sink.WriteValue("ns=2;s=x", 42, OpcUaQuality.Good, DateTime.UtcNow); + } + + [Fact] + public void Before_SetSink_RebuildAddressSpace_is_a_noop() + { + var sink = new DeferredAddressSpaceSink(); + sink.RebuildAddressSpace(); // Must not throw. + } + + [Fact] + public void Before_SetSink_UpdateTagAttributes_returns_false() + { + var sink = new DeferredAddressSpaceSink(); + sink.UpdateTagAttributes("ns=2;s=x", writable: true, historianTagname: null, + dataType: "Boolean", isArray: false, arrayLength: null).ShouldBeFalse(); + } + + // ---------- after SetSink — operations are forwarded ---------- + + [Fact] + public void After_SetSink_WriteValue_is_forwarded() + { + var inner = new SpySink(); + var sink = new DeferredAddressSpaceSink(); + sink.SetSink(inner); + + sink.WriteValue("ns=2;s=x", 99, OpcUaQuality.Good, DateTime.UtcNow); + + inner.WriteValueCalled.ShouldBeTrue(); + } + + [Fact] + public void After_SetSink_RebuildAddressSpace_is_forwarded() + { + var inner = new SpySink(); + var sink = new DeferredAddressSpaceSink(); + sink.SetSink(inner); + + sink.RebuildAddressSpace(); + + inner.RebuildCalled.ShouldBeTrue(); + } + + // ---------- ISurgicalAddressSpaceSink forwarding ---------- + + [Fact] + public void UpdateTagAttributes_returns_false_for_non_surgical_inner() + { + // SpySink does NOT implement ISurgicalAddressSpaceSink. + var sink = new DeferredAddressSpaceSink(); + sink.SetSink(new SpySink()); + + sink.UpdateTagAttributes("ns=2;s=x", writable: true, historianTagname: null, + dataType: "Int32", isArray: false, arrayLength: null).ShouldBeFalse(); + } + + [Fact] + public void UpdateTagAttributes_returns_true_for_surgical_inner() + { + var surgical = new SpySurgicalSink(); + var sink = new DeferredAddressSpaceSink(); + sink.SetSink(surgical); + + var result = sink.UpdateTagAttributes("ns=2;s=x", writable: true, historianTagname: null, + dataType: "Float", isArray: false, arrayLength: null); + + result.ShouldBeTrue(); + surgical.UpdateCalled.ShouldBeTrue(); + } + + // ---------- SetSink(null) reverts to null sink ---------- + + [Fact] + public void SetSink_null_reverts_to_null_sink_and_UpdateTagAttributes_returns_false() + { + var surgical = new SpySurgicalSink(); + var sink = new DeferredAddressSpaceSink(); + sink.SetSink(surgical); + sink.SetSink(null); // revert + + sink.UpdateTagAttributes("ns=2;s=x", writable: false, historianTagname: null, + dataType: "Boolean", isArray: false, arrayLength: null).ShouldBeFalse(); + } + + [Fact] + public void SetSink_null_reverts_to_null_sink_and_WriteValue_is_noop() + { + var inner = new SpySink(); + var sink = new DeferredAddressSpaceSink(); + sink.SetSink(inner); + sink.SetSink(null); // revert + + sink.WriteValue("ns=2;s=x", 1, OpcUaQuality.Good, DateTime.UtcNow); + + inner.WriteValueCalled.ShouldBeFalse("write should be no-op after reverting to null sink"); + } + + // ---- test doubles ---- + + private sealed class SpySink : IOpcUaAddressSpaceSink + { + public bool WriteValueCalled { get; private set; } + public bool RebuildCalled { get; private set; } + + public void WriteValue(string nodeId, object? value, OpcUaQuality quality, DateTime sourceTimestampUtc) + => WriteValueCalled = true; + + public void WriteAlarmCondition(string alarmNodeId, AlarmConditionSnapshot state, DateTime sourceTimestampUtc) { } + public void MaterialiseAlarmCondition(string alarmNodeId, string equipmentNodeId, string displayName, string alarmType, int severity, bool isNative = false) { } + public void EnsureFolder(string folderNodeId, string? parentNodeId, string displayName) { } + public void EnsureVariable(string variableNodeId, string? parentFolderNodeId, string displayName, string dataType, bool writable, string? historianTagname = null, bool isArray = false, uint? arrayLength = null) { } + public void RebuildAddressSpace() => RebuildCalled = true; + } + + private sealed class SpySurgicalSink : IOpcUaAddressSpaceSink, ISurgicalAddressSpaceSink + { + public bool UpdateCalled { get; private set; } + + public void WriteValue(string nodeId, object? value, OpcUaQuality quality, DateTime sourceTimestampUtc) { } + public void WriteAlarmCondition(string alarmNodeId, AlarmConditionSnapshot state, DateTime sourceTimestampUtc) { } + public void MaterialiseAlarmCondition(string alarmNodeId, string equipmentNodeId, string displayName, string alarmType, int severity, bool isNative = false) { } + public void EnsureFolder(string folderNodeId, string? parentNodeId, string displayName) { } + public void EnsureVariable(string variableNodeId, string? parentFolderNodeId, string displayName, string dataType, bool writable, string? historianTagname = null, bool isArray = false, uint? arrayLength = null) { } + public void RebuildAddressSpace() { } + + public bool UpdateTagAttributes(string variableNodeId, bool writable, string? historianTagname, string dataType, bool isArray, uint? arrayLength) + { + UpdateCalled = true; + return true; + } + } +} diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/DeferredServiceLevelPublisherTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/DeferredServiceLevelPublisherTests.cs new file mode 100644 index 00000000..b5b5dfe2 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/DeferredServiceLevelPublisherTests.cs @@ -0,0 +1,53 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; + +namespace ZB.MOM.WW.OtOpcUa.Commons.Tests.OpcUa; + +/// +/// Covers DeferredServiceLevelPublisher forwarding invariants (Commons-003 companion). +/// +public class DeferredServiceLevelPublisherTests +{ + [Fact] + public void Before_SetInner_Publish_is_a_noop() + { + var publisher = new DeferredServiceLevelPublisher(); + // Must not throw; the NullServiceLevelPublisher absorbs it. + publisher.Publish(200); + } + + [Fact] + public void After_SetInner_Publish_is_forwarded() + { + var spy = new SpyPublisher(); + var publisher = new DeferredServiceLevelPublisher(); + publisher.SetInner(spy); + + publisher.Publish(240); + + spy.LastPublished.ShouldBe((byte)240); + } + + [Fact] + public void SetInner_null_reverts_to_null_publisher() + { + var spy = new SpyPublisher(); + var publisher = new DeferredServiceLevelPublisher(); + publisher.SetInner(spy); + publisher.SetInner(null); // revert + + publisher.Publish(100); + + // The spy should NOT have received the publish after revert. + spy.LastPublished.ShouldBe((byte)0, "spy should not have been updated after revert to null"); + } + + // ---- test double ---- + + private sealed class SpyPublisher : IServiceLevelPublisher + { + public byte LastPublished { get; private set; } + public void Publish(byte serviceLevel) => LastPublished = serviceLevel; + } +} diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/EquipmentNodeIdsTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/EquipmentNodeIdsTests.cs index 4601771c..906ac5d2 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/EquipmentNodeIdsTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Commons.Tests/OpcUa/EquipmentNodeIdsTests.cs @@ -18,6 +18,10 @@ public class EquipmentNodeIdsTests public void Variable_with_folder_is_equipment_slash_folder_slash_name() => EquipmentNodeIds.Variable("eq-1", "registers", "speed").ShouldBe("eq-1/registers/speed"); + [Fact] + public void Variable_with_whitespace_folder_is_equipment_slash_name() + => EquipmentNodeIds.Variable("eq-1", " ", "speed").ShouldBe("eq-1/speed"); + [Fact] public void SubFolder_is_equipment_slash_folder() => EquipmentNodeIds.SubFolder("eq-1", "registers").ShouldBe("eq-1/registers");