From a4d24b5cf52f9d21d5c3a710e5a8c426bbaec4ad Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 12:22:53 -0400 Subject: [PATCH] review(Driver.AbCip.Contracts): first review; document IsArray/Writable contract 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. --- .../Driver.AbCip.Contracts/findings.md | 226 ++++++++++++++++++ .../AbCipDriverOptions.cs | 3 +- .../AbCipEquipmentTagParser.cs | 6 + 3 files changed, 234 insertions(+), 1 deletion(-) create mode 100644 code-reviews/Driver.AbCip.Contracts/findings.md diff --git a/code-reviews/Driver.AbCip.Contracts/findings.md b/code-reviews/Driver.AbCip.Contracts/findings.md new file mode 100644 index 00000000..bc197ed5 --- /dev/null +++ b/code-reviews/Driver.AbCip.Contracts/findings.md @@ -0,0 +1,226 @@ +# Code Review — Driver.AbCip.Contracts + + + +| 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 + + + +### 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` 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 `` 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 `` doc for `AbCipTagDefinition` did not state that +the field is "Ignored for `AbCipDataType.Structure`", while the immediately preceding +`` (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 ``." to the +`IsArray` param doc, matching `ElementCount`. + +**Resolution:** Resolved 2026-06-19 — added "Ignored for ``." +to the `IsArray` `` 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 `` 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 `` 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). diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs index 573268a2..6c4d081e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs @@ -142,7 +142,8 @@ public sealed record AbCipDeviceOptions( /// had isArray:true (with arrayLength >= 1); the tag discovers as an OPC UA /// array node (IsArray + ArrayDim) and reads as a typed CLR array — even when /// is 1 (a valid 1-element array). ElementCount alone -/// cannot carry this because a scalar and a 1-element array both have a count of 1. +/// cannot carry this because a scalar and a 1-element array both have a count of 1. +/// Ignored for . public sealed record AbCipTagDefinition( string Name, string DeviceHostAddress, diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs index 2a799fcb..062da496 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs @@ -12,6 +12,12 @@ public static class AbCipEquipmentTagParser /// The equipment tag's TagConfig JSON (also used as the def identity). /// The transient definition when parsing succeeds. /// when is an AbCip TagConfig object. + /// + /// The produced is always true: the + /// TagConfig JSON format for equipment tags does not carry a writability field, so the + /// PLC's ExternalAccess attribute is the effective write gate. Operators who need a + /// read-only OPC UA surface must rely on the PLC's ExternalAccess rejecting the write. + /// public static bool TryParse(string reference, out AbCipTagDefinition def) { def = null!;