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

review(Driver.AbCip.Contracts): first review; document IsArray/Writable contract

Browse Source 
First review at 7286d320. 6 findings (doc fixes -005/-006 resolved; -001 writable-hardcode,
-002 Dt-units doc, -003 dead [Display] attrs, -004 parser test gap left Open). Surfaced
cross-module: AbCipTagDto/AbCipMemberDto in the factory drop ElementCount/IsArray.
This commit is contained in:
Joseph Doherty
2026-06-19 12:22:53 -04:00
parent a19b0f8612
commit a4d24b5cf5
3 changed files with 234 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/Driver.AbCip.Contracts/findings.md
+226
View File
@@ -0,0 +1,226 @@
# Code Review — Driver.AbCip.Contracts
<!-- Template for a per-module findings file. Copy to code-reviews/<Module>/findings.md.
See ../../REVIEW-PROCESS.md for the full process. The base README.md is generated
from the per-module `findings.md` files by regen-readme.py — do not edit README.md by hand. -->
| Field | Value |
|---|---|
| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts` |
| Reviewer | Claude Code |
| Review date | 2026-06-19 |
| Commit reviewed | `a19b0f86` |
| Status | Reviewed |
| Open findings | 4 |
## 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 | Driver.AbCip.Contracts-001, Driver.AbCip.Contracts-002 |
| 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 | Driver.AbCip.Contracts-003 |
| 9 | Testing coverage | Driver.AbCip.Contracts-004 |
| 10 | Documentation & comments | Driver.AbCip.Contracts-005, Driver.AbCip.Contracts-006 |
## Findings
<!-- One ### entry per finding. IDs are <Module>-NNN, sequential within the module,
never reused. Findings are never deleted — close them by changing Status and
completing Resolution. -->
### Driver.AbCip.Contracts-001
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs:42` |
| Status | Open |
**Description:** `AbCipEquipmentTagParser.TryParse` hard-codes `Writable: true` on every
equipment-tag definition it produces, regardless of any `writable` field in the TagConfig JSON.
The consequence is that an operator who intends a read-only equipment tag — relying on the PLC's
ExternalAccess attribute to block writes — will still receive a writable OPC UA node in the
address space: the server advertises the node as writable, and the `OnWriteValue` path in the
driver dispatches writes to the PLC (which the PLC may then reject). The node is never declared
`BadNotWritable` proactively. This is inconsistent with the pre-declared tag path, where
`AbCipTagDefinition.Writable` is explicitly authored per tag and controls the OPC UA
`AccessLevel` bit at materialization.
Additionally, `TryParse` does not guard against `AbCipDataType.Structure` in the `"dataType"`
JSON field. An equipment tag authored with `"dataType":"Structure"` succeeds and returns a
Structure-typed definition with `Members: null`. The driver treats this as a black-box
Structure (readable via dotted-path child addressing), but the address space emits a placeholder
`String` variable for it rather than a UDT folder. No error or warning is surfaced. The AdminUI
tag editor does not expose UDT member declarations, so a Structure equipment tag is essentially
a misconfiguration.
**Recommendation:** (1) Read a `"writable"` boolean field from the TagConfig JSON (default `true`
when absent) and thread it into `AbCipTagDefinition.Writable`. This matches the contract on the
record's `Writable` parameter. (2) Either return `false` from `TryParse` when `dataType` parses
to `AbCipDataType.Structure` (equipment-tag flow cannot declare members), or add an explicit
comment documenting the black-box dotted-path behaviour so the next reader understands the intent.
**Resolution:** _(empty until closed)_
---
### Driver.AbCip.Contracts-002
| Field | Value |
|---|---|
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDataType.cs:28` |
| Status | Open |
**Description:** The inline comment on `AbCipDataType.Dt` reads:
```
Dt, // Date/Time — Logix DT == DINT representing seconds-since-epoch per Rockwell conventions
```
This description is inaccurate in two ways. First, `CipSymbolObjectDecoder` (in the driver
module) maps both CIP type code `0xCD` (Logix `DATE` — a 4-byte unsigned integer representing
days since 1 January 1984) and `0xCF` (Logix `DATE_AND_TIME` / `DT` — an 8-byte unsigned
integer representing microseconds since 1 January 1970) to `AbCipDataType.Dt`. The comment
fits only the `DT` / `DATE_AND_TIME` form, not `DATE`. Second, even for `DT` proper, the unit
is microseconds, not seconds.
In practice the driver reads `Dt` via `GetInt32(offset)` with a 4-byte stride: this is correct
for `DATE` but truncates `DATE_AND_TIME` to its low 4 bytes, losing the upper 32 bits. That
underlying decode issue lives in `LibplctagTagRuntime` (the Driver.AbCip module), but the
inaccurate comment here perpetuates the misconception for anyone authoring or reading configs
that reference the `Dt` member.
**Recommendation:** Update the inline comment to describe both mapped CIP types and the
4-byte stride constraint, e.g.:
```csharp
Dt, // Logix DATE (0xCD — 4-byte unsigned days since 1984-01-01) or DATE_AND_TIME / DT
// (0xCF — 8-byte unsigned microseconds since 1970-01-01). The driver reads 4 bytes
// via GetInt32; DATE decodes correctly, DATE_AND_TIME is truncated to the low 4 bytes
// (a known limitation tracked in LibplctagTagRuntime).
```
No behaviour change; documentation only.
**Resolution:** _(empty until closed)_
---
### Driver.AbCip.Contracts-003
| Field | Value |
|---|---|
| Severity | Low |
| Category | Code organization & conventions |
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs:84-85` |
| Status | Open |
**Description:** `AbCipDriverOptions.ProbeTimeoutSeconds` carries `[Display]` and `[Range(1, 60)]`
attributes from `System.ComponentModel.DataAnnotations`. No other driver contracts project
(Modbus, S7, AbLegacy, Galaxy, TwinCAT, FOCAS, OpcUaClient) annotates `ProbeTimeoutSeconds`
with data-annotation attributes. More importantly, `AbCipDriverOptions` is never bound directly
by an ASP.NET model binder: the AdminUI driver page (`AbCipDriverPage.razor`) deserializes it
via `JsonSerializer.Deserialize<AbCipDriverOptions>` and then transfers values into a separate
`FormModel`. Neither the `[Display]` grouping hint nor the `[Range]` validation is evaluated at
runtime — they are dead metadata. The `using System.ComponentModel.DataAnnotations;` directive
exists in this project solely to support these two unused attributes.
**Recommendation:** Remove `[Display]` and `[Range(1, 60)]` from `ProbeTimeoutSeconds` and
remove the `using System.ComponentModel.DataAnnotations;` directive. This brings the project in
line with every other driver contracts project and removes a superfluous framework dependency.
If the intent is to document the valid range, an `<remarks>` tag (e.g. "Valid range: 1–60
seconds; the AdminUI clamps to 60s server-side.") achieves the same goal without the attribute
dependency.
**Resolution:** _(empty until closed)_
---
### Driver.AbCip.Contracts-004
| Field | Value |
|---|---|
| Severity | Low |
| Category | Testing coverage |
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs` (entire file) |
| Status | Open |
**Description:** The contracts module has no dedicated test project, and `AbCipEquipmentTagParser.TryParse`
is the module's only non-trivial logic. Its `ReadArrayShape` helper has four distinct outcome
branches; the method also has multiple early-return paths and one JSON exception path. None of
these are covered by targeted unit tests. Coverage gaps include:
- `isArray: true` + `arrayLength: 0` — canonical rule says this is a scalar; not tested.
- `isArray: true` + `arrayLength` absent — same canonical rule; not tested.
- `"dataType": "Structure"` input — no test (see Driver.AbCip.Contracts-001).
- The `Writable: true` hardcode is not asserted anywhere.
- A `tagPath` that is present as a JSON string but contains only whitespace — returns `false`; not tested.
- Malformed JSON input (the `JsonException` catch) — not tested.
**Recommendation:** Add a dedicated test class for `AbCipEquipmentTagParser` in the existing
driver test project (`tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/`). No new project
is needed. Cover: valid scalar round-trip, 1-element array, N-element array, each degenerate
array-shape combination, non-JSON and non-object input, missing/blank `tagPath`, the Structure
DataType path, and the `Writable` default.
**Resolution:** _(empty until closed)_
---
### Driver.AbCip.Contracts-005
| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs:141-145` |
| Status | Resolved |
**Description:** The `<param name="IsArray">` doc for `AbCipTagDefinition` did not state that
the field is "Ignored for `AbCipDataType.Structure`", while the immediately preceding
`<param name="ElementCount">` (line 140) explicitly carries that note. The asymmetry left readers
unable to determine from the XML doc whether `IsArray = true` on a Structure-typed definition
is meaningful or silently ignored.
**Recommendation:** Add "Ignored for `<see cref="AbCipDataType.Structure"/>`." to the
`IsArray` param doc, matching `ElementCount`.
**Resolution:** Resolved 2026-06-19 — added "Ignored for `<see cref="AbCipDataType.Structure"/>`."
to the `IsArray` `<param>` doc on `AbCipTagDefinition`. Build verified green (0 errors, 0 warnings).
---
### Driver.AbCip.Contracts-006
| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs:11-14` |
| Status | Resolved |
**Description:** The `TryParse` method summary and parameter docs were silent about the fact
that `Writable` is always set to `true` in the produced `AbCipTagDefinition`. This is a
non-obvious behavioral contract: callers who inspect the returned definition expecting `Writable`
to reflect an authored intent from the JSON will find it unconditionally enabled. The omission
made the hardcode invisible to readers of the public-facing doc comment.
**Recommendation:** Add a `<remarks>` element to `TryParse` stating that `Writable` is always
`true` in the produced definition and that the PLC's ExternalAccess attribute is the effective
write gate.
**Resolution:** Resolved 2026-06-19 — added a `<remarks>` element to `TryParse` documenting
that `Writable` is always `true` and that PLC ExternalAccess is the effective write gate.
Build verified green (0 errors, 0 warnings).