From 0c1d5f7f8837d83993bfbf61ba7563e7bad7bd28 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 12:22:53 -0400 Subject: [PATCH] review(Driver.TwinCAT.Contracts): first review; fix ReadArrayLength doc First review at 7286d320. -001 (Low): ReadArrayLength doc corrected (positive int32 via TryGetInt32, not uint). -002 (Structure datatype doc) Open. Consistent with the sibling Driver.TwinCAT-017 ArrayLength flow. --- .../Driver.TwinCAT.Contracts/findings.md | 111 ++++++++++++++++++ .../TwinCATEquipmentTagParser.cs | 8 +- 2 files changed, 116 insertions(+), 3 deletions(-) create mode 100644 code-reviews/Driver.TwinCAT.Contracts/findings.md diff --git a/code-reviews/Driver.TwinCAT.Contracts/findings.md b/code-reviews/Driver.TwinCAT.Contracts/findings.md new file mode 100644 index 00000000..2e3ea405 --- /dev/null +++ b/code-reviews/Driver.TwinCAT.Contracts/findings.md @@ -0,0 +1,111 @@ +# Code Review — Driver.TwinCAT.Contracts + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Contracts` | +| Reviewer | Claude Code | +| Review date | 2026-06-19 | +| Commit reviewed | `a19b0f86` | +| Status | Reviewed | +| Open findings | 1 | + +## 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.TwinCAT.Contracts-001 (Resolved) | +| 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 | No issues found | +| 9 | Testing coverage | No issues found (no test project; thin contracts module) | +| 10 | Documentation & comments | Driver.TwinCAT.Contracts-002 | + +## Cross-module consistency (Driver.TwinCAT-017 context) + +Driver.TwinCAT-017 added `int? ArrayLength` to `TwinCATTagDto` in the sibling driver project, +fixing the pre-declared-tag authoring gap. The Contracts module's two consumers of +`TwinCATTagDefinition.ArrayLength` are both consistent: + +- `TwinCATEquipmentTagParser.ReadArrayLength` reads `isArray`+`arrayLength` from the + equipment-tag TagConfig JSON (the AdminUI TagModal's `TagArrayConfig` layer). Field names, + guard logic (isArray must be the JSON literal `true`; length must be a positive number), and + the return type (`int?`) all match the driver's consumption in `DiscoverAsync` / + `ReadValueAsync`. No gap. +- `TwinCATDriverOptions.TwinCATTagDefinition` (the record) receives `ArrayLength` from both + `BuildTag` (pre-declared JSON path, fixed by -017) and `TwinCATEquipmentTagParser.TryParse` + (equipment-tag JSON path). Both paths are consistent. No gap introduced by -017. + +One minor comment inaccuracy found and fixed below (Driver.TwinCAT.Contracts-001). + +## Findings + +### Driver.TwinCAT.Contracts-001 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `TwinCATEquipmentTagParser.cs:56-66` (`ReadArrayLength`) | +| Status | Resolved | + +**Description:** The XML doc comment for `ReadArrayLength` describes `arrayLength` as "a +positive uint", but the implementation calls `lEl.TryGetInt32(out var len)`. If `arrayLength` +in the TagConfig JSON holds a value greater than `int.MaxValue` (2 147 483 647), `TryGetInt32` +returns `false` and the method silently returns `null` — treating the tag as scalar instead of +returning an error. The comment implies uint semantics but the code applies int semantics, +creating a mismatch that would mislead a maintainer trying to understand the accepted range. + +In practice the `TagArrayConfig` layer (AdminUI) writes `arrayLength` as a `uint`, so values +above `int.MaxValue` are theoretically possible in JSON (though no real PLC array would have +two billion elements). The discrepancy is documentation-level rather than a real data-loss risk. + +**Recommendation:** Correct the comment to say "a positive int32 (values above `int.MaxValue` +are treated as absent/scalar)" to match the actual `TryGetInt32` call. The simplest safe fix +is the comment correction; the behaviour is acceptable in practice. + +**Resolution:** Resolved 2026-06-19 — corrected the `ReadArrayLength` XML doc comment to say +"a positive int32 (values above `int.MaxValue` are treated as absent/scalar)" so it matches +the actual `TryGetInt32` call; no behaviour change. Verified by build: 0 errors, 0 warnings. + +--- + +### Driver.TwinCAT.Contracts-002 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `TwinCATDataType.cs:33` (`Structure` member) | +| Status | Open | + +**Description:** The `Structure` enum member's XML summary says "UDT / FB instance. Resolved +per member at discovery time." This is accurate for the discovery path, but the comment omits +that `Structure` is **rejected at pre-declaration time** by +`TwinCATDriverFactoryExtensions.BuildTag` (with a clear `InvalidOperationException`), and that +it is also **not parseable as a valid data type by `TwinCATEquipmentTagParser`** (the parser +falls through to the `DInt` fallback because `Structure` is never written by the AdminUI typed +editor). A maintainer reading only the Contracts project will infer that `Structure` is a +usable user-facing data type, when in fact it is an internal-discovery-only sentinel that must +not appear in operator-authored config. + +**Recommendation:** Expand the `Structure` XML doc to reflect its actual contract: + +```csharp +/// +/// UDT / FB instance. Used internally by DiscoverAsync when browsing controller +/// symbols — members are resolved to atomic types and emitted individually. Must not be +/// used in pre-declared tags (rejected at initialisation by the factory with +/// ) or in AdminUI equipment-tag configs (the typed +/// editor does not offer it; the parser falls back to DInt). +/// +Structure, +``` + +**Resolution:** _(empty until closed)_ diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Contracts/TwinCATEquipmentTagParser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Contracts/TwinCATEquipmentTagParser.cs index 4c6131b7..7fd03292 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Contracts/TwinCATEquipmentTagParser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Contracts/TwinCATEquipmentTagParser.cs @@ -53,9 +53,11 @@ public static class TwinCATEquipmentTagParser ? e.GetString() ?? "" : ""; /// - /// Reads the optional 1-D array length: arrayLength (a positive uint) honoured ONLY - /// when isArray is the JSON literal true. Returns null (scalar) when - /// isArray is absent/false, when arrayLength is absent / non-numeric / zero / negative. + /// Reads the optional 1-D array length: arrayLength (a positive int32; values above + /// are treated as absent/scalar) honoured ONLY when isArray + /// is the JSON literal true. Returns null (scalar) when isArray is absent/false, + /// when arrayLength is absent / non-numeric / zero / negative / greater than + /// . /// private static int? ReadArrayLength(JsonElement o) {