From 5d01271f038bc9027e9d09d94d760408920c5065 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 12:22:53 -0400 Subject: [PATCH] review(Driver.S7.Contracts): first review; gate stringLength check to String type First review at 7286d320. -002 (Low): stringLength range check now only fires for S7DataType.String (was rejecting valid non-String equipment-tag blobs). -001 ArrayCount=1 doc fix. -003 (driver-specs.md CpuType omissions) deferred. Consuming Driver.S7.Tests green. --- code-reviews/Driver.S7.Contracts/findings.md | 83 +++++++++++++++++++ .../S7DriverOptions.cs | 3 +- .../S7EquipmentTagParser.cs | 7 +- 3 files changed, 89 insertions(+), 4 deletions(-) create mode 100644 code-reviews/Driver.S7.Contracts/findings.md diff --git a/code-reviews/Driver.S7.Contracts/findings.md b/code-reviews/Driver.S7.Contracts/findings.md new file mode 100644 index 00000000..e84e3a0b --- /dev/null +++ b/code-reviews/Driver.S7.Contracts/findings.md @@ -0,0 +1,83 @@ +# Code Review — Driver.S7.Contracts + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts` | +| Reviewer | Claude Code | +| Review date | 2026-06-19 | +| Commit reviewed | `a19b0f86` | +| 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 | Driver.S7.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 | Driver.S7.Contracts-003 | +| 8 | Code organization & conventions | No issues found | +| 9 | Testing coverage | No issues found | +| 10 | Documentation & comments | Driver.S7.Contracts-001 | + +## Findings + + + +### Driver.S7.Contracts-001 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs:107` | +| Status | Resolved | + +**Description:** The `ArrayCount` XML doc on `S7TagDefinition` stated "null (or `<= 1`) for a scalar", which is inaccurate. The driver and the OPC UA materializer both use `ArrayCount is >= 1` as the array gate (in `RejectUnsupportedTagConfigs`, `ReadOneAsync`, `WriteOneAsync`, and `DiscoverAsync`), meaning `ArrayCount = 1` surfaces as a 1-element OPC UA array node, not a scalar. The `<= 1` phrasing would lead a caller to pass `ArrayCount = 1` expecting scalar behaviour and instead receive an array node. + +**Recommendation:** Change the doc to "null for a scalar; any value >= 1 (including 1) surfaces as a 1-D OPC UA array node." + +**Resolution:** Fixed in this review (2026-06-19, commit `a19b0f86`) — updated `S7TagDefinition.ArrayCount` XML doc to accurately state "null for a scalar; any value >= 1 (including 1) surfaces as a 1-D OPC UA array node." Verified by build (no test project). + +--- + +### Driver.S7.Contracts-002 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7EquipmentTagParser.cs:38` | +| Status | Resolved | + +**Description:** `TryParse` validated the `stringLength` range (`< 0 || > 254`) unconditionally for all `DataType` values. A non-String tag blob that carries an out-of-range `stringLength` field (e.g., from copy-paste or manual edit: `{"address":"DB1.DBD0","dataType":"Float32","stringLength":300}`) caused `TryParse` to return `false`, silently failing to resolve the equipment tag. The driver would then return `BadNodeIdUnknown` on every read/write for that tag, with no config error at init — exactly the hard-to-diagnose failure mode that init-time validation was meant to prevent. The `stringLength` field is only meaningful when `DataType == String`; for all other types it is stored in the record but never consumed by the driver. + +**Recommendation:** Gate the range check on `dataType == S7DataType.String`, leaving non-String blobs unaffected regardless of any `stringLength` value they carry. + +**Resolution:** Fixed in this review (2026-06-19, commit `a19b0f86`) — added `dataType == S7DataType.String &&` guard before the out-of-range check. Non-String blobs now parse correctly regardless of their `stringLength` value; String blobs with an out-of-range length are still correctly rejected. Verified by build (no test project). + +--- + +### Driver.S7.Contracts-003 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Design-document adherence | +| Location | `docs/v2/driver-specs.md:369` | +| Status | Deferred | + +**Description:** The `CpuType` table in `docs/v2/driver-specs.md` (§5, "Connection Settings") lists only four values: `S7300`, `S7400`, `S71200`, `S71500`. The `S7CpuType` enum in this module defines seven values — `S7200 = 0`, `Logo0BA8 = 1`, `S7200Smart = 2`, `S7300 = 10`, `S7400 = 20`, `S71200 = 30`, `S71500 = 40` — all mapped through `S7CpuTypeMap.ToS7Net` in the driver. `docs/drivers/S7.md` mentions S7-200 / S7-200 Smart / LOGO! 0BA8 in prose but gives no enum string names or rack/slot conventions. An operator targeting a LOGO! 0BA8 has no documentation reference for the `CpuType` JSON string to use. + +**Recommendation:** Add `S7200`, `S7200Smart`, `Logo0BA8` rows to the `driver-specs.md` CpuType table (or note limited S7comm support for these legacy families) and add their enum names to `docs/drivers/S7.md`'s rack/slot table. + +**Resolution:** Deferred — the fix is in `docs/v2/driver-specs.md` and `docs/drivers/S7.md`, outside this module's source files. No code impact. Tracked as a documentation-only follow-up. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs index 6b5005fc..1e374cd5 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7DriverOptions.cs @@ -105,7 +105,8 @@ public sealed class S7ProbeOptions /// coils that drive edge-triggered routines in the PLC program. /// /// -/// Element count when the tag is a 1-D array; null (or <= 1) for a scalar. +/// Element count when the tag is a 1-D array; null for a scalar. Any value >= 1 +/// (including 1) surfaces as a 1-D OPC UA array node. /// For an equipment tag this is threaded from the TagConfig JSON's arrayLength /// (honoured only when isArray is true) by . When /// set, the driver issues a single contiguous block read of diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7EquipmentTagParser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7EquipmentTagParser.cs index 8430335c..8c1844e0 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7EquipmentTagParser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7EquipmentTagParser.cs @@ -33,9 +33,10 @@ public static class S7EquipmentTagParser if (string.IsNullOrWhiteSpace(address)) return false; var dataType = ReadEnum(root, "dataType", S7DataType.Int16); var stringLength = ReadInt(root, "stringLength"); - // Range-guard rather than truncate: an S7 string can't exceed 254 chars, and a - // negative length is meaningless — reject so a malformed blob can't slip through. - if (stringLength < 0 || stringLength > MaxStringLength) return false; + // Range-guard applies only to String tags: an S7 string can't exceed 254 chars, and a + // negative length is meaningless. For non-String types stringLength is irrelevant and any + // authored value (including an out-of-range remnant from copy-paste) must not block parsing. + if (dataType == S7DataType.String && (stringLength < 0 || stringLength > MaxStringLength)) return false; // Array intent: the canonical sink-side parse (DeploymentArtifact.ExtractTagArray) // honours arrayLength ONLY when isArray is true AND the prop is a JSON number — mirror // that here so the driver's transient def agrees byte-for-byte with the materialised