Joseph Doherty
26662aa2ec
First review at 7286d320. -001 (Low) fixed: ArrayLength=1 doc corrected (it is a 1-element
array node, not scalar). -002/-003/-004 (parser edge doc, dead MaxTagBytes, ForFamily
fallback) Open.
6.5 KiB
Code Review — Driver.AbLegacy.Contracts
| Field | Value |
|---|---|
| Module | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts |
| Reviewer | Claude Code |
| Review date | 2026-06-19 |
| Commit reviewed | a19b0f86 |
| Status | Reviewed |
| Open findings | 3 |
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 | 1 finding (Driver.AbLegacy.Contracts-003) |
| 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 | 1 finding (Driver.AbLegacy.Contracts-004) |
| 9 | Testing coverage | No issues found (contracts exercised by Driver.AbLegacy.Tests and AdminUI.Tests) |
| 10 | Documentation & comments | 2 findings (Driver.AbLegacy.Contracts-001 fixed, Driver.AbLegacy.Contracts-002 open) |
Findings
Driver.AbLegacy.Contracts-001
| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyDriverOptions.cs:48 |
| Status | Resolved |
Description: The <param name="ArrayLength"> XML doc on AbLegacyTagDefinition stated
<c>1</c> reads as a scalar. This was incorrect. Both the parser (AbLegacyEquipmentTagParser,
line 44: if (rawLength >= 1)) and the driver (AbLegacyDriver.IsArrayTag, checks len >= 1)
treat ArrayLength: 1 as a valid one-element array, exposing a [1] OPC UA array node -- not a
scalar. The in-source comment at AbLegacyEquipmentTagParser.cs:36 correctly stated
"A 1-element array (isArray:true, arrayLength:1) is a valid [1] array." The XML doc contradicted this.
Recommendation: Change the offending sentence from <c>1</c> reads as a scalar. to
<c>1</c> is a valid one-element array exposed as a <c>[1]</c> OPC UA array node.
Resolution: Fixed in same review session, 2026-06-19. Changed the misleading sentence in
AbLegacyDriverOptions.cs:48 to correctly describe ArrayLength 1 as a one-element array;
verified by build (no test project in this module).
Driver.AbLegacy.Contracts-002
| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyEquipmentTagParser.cs:15 |
| Status | Open |
Description: AbLegacyEquipmentTagParser.TryParse has an undocumented edge case: when
isArray is the JSON literal true but arrayLength is absent, zero, or negative,
ReadInt returns 0, the rawLength >= 1 guard fails, and arrayLength stays
null -- producing a scalar tag despite isArray:true. This is intentional (the in-source
comment at lines 33-39 references "review C-2"), but it is invisible from the public
<summary> and <returns> XML docs, and there is no unit test covering this case.
An operator who hand-edits the raw TagConfig JSON with {"isArray":true} and no
arrayLength will silently get a scalar OPC UA node instead of the expected array shape.
Recommendation: Add a <remarks> element to the XML doc noting that isArray:true
without a valid positive arrayLength silently produces a scalar (null ArrayLength).
Optionally add a unit test to AbLegacyEquipmentTagTests covering
isArray:true, arrayLength:0/absent -> null for regression protection.
Resolution: (empty until closed)
Driver.AbLegacy.Contracts-003
| Field | Value |
|---|---|
| Severity | Low |
| Category | OtOpcUa conventions |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyPlcFamilyProfile.cs:10 |
| Status | Open |
Description: AbLegacyPlcFamilyProfile.MaxTagBytes is a record constructor parameter
populated with distinct values per family (240/232/240/240), but a global search finds zero
call sites that read this property anywhere in the codebase -- not in AbLegacyDriver.cs,
the probe, the CLI, or any test. The field is dead weight in the public contract surface.
Keeping it may mislead future implementors into assuming it is enforced: a caller that adds
array-size clamping logic on MaxTagBytes without knowing the values are approximate
(e.g. SLC 5/05 240 is the PCCC packet payload cap, not a libplctag fragment limit) could
produce off-by-one sizing. If reserved for future clamping, the intent is undocumented.
Recommendation: Either (a) add XML doc noting it is reserved for future array-length clamping and is not currently enforced, or (b) remove the field and its four initialisers in a dedicated cleanup PR (signature change is out of scope for this review).
Resolution: (empty until closed)
Driver.AbLegacy.Contracts-004
| Field | Value |
|---|---|
| Severity | Low |
| Category | Code organization & conventions |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyPlcFamilyProfile.cs:22 |
| Status | Open |
Description: AbLegacyPlcFamilyProfile.ForFamily has a catch-all arm _ => Slc500
that silently returns the SLC 500 profile for any unrecognised AbLegacyPlcFamily value
(e.g. an integer cast). The XML doc on ForFamily does not mention the fallback. A
misconfigured device (or a future enum member added without updating the switch) will
silently receive SLC 500 libplctag attributes (plc="slc500", path="1,0") and a
MaxTagBytes of 240, which may produce libplctag session errors or wrong data only at
runtime, far from where the enum value was set. The parallel FOCAS driver throws on
unexpected values.
Recommendation: Either (a) document the silent fallback in the XML doc and note it is
intentional (e.g. for forward-compatibility with configs authored before a new family was
added), or (b) replace _ => Slc500 with
_ => throw new ArgumentOutOfRangeException(nameof(family), family, null) and apply it
in a cleanup PR. Semantic change -- defer.
Resolution: (empty until closed)