From f2bdd8bc1c312b1a3617b06fca479034f130abcb Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 19 Jun 2026 11:34:34 -0400 Subject: [PATCH] review(Driver.S7): reject writable array tags at init instead of silent write failure Re-review at 7286d320. S7-015 (Medium): a Writable array tag had no WriteArrayAsync path and silently returned BadCommunicationError on write; now rejected at init with a clear NotSupportedException (read-only arrays still accepted) + TDD. S7-016 (factory JSON can't produce array tags; needs AdminUI DTO) deferred. --- code-reviews/Driver.S7/findings.md | 49 +++++++++++++++- .../ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs | 24 ++++++++ .../S7DriverScaffoldTests.cs | 57 +++++++++++++++++++ 3 files changed, 128 insertions(+), 2 deletions(-) diff --git a/code-reviews/Driver.S7/findings.md b/code-reviews/Driver.S7/findings.md index 312b2e62..c10bcb88 100644 --- a/code-reviews/Driver.S7/findings.md +++ b/code-reviews/Driver.S7/findings.md @@ -4,8 +4,8 @@ |---|---| | Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7` | | Reviewer | Claude Code | -| Review date | 2026-05-22 | -| Commit reviewed | `76d35d1` | +| Review date | 2026-06-19 | +| Commit reviewed | `7286d320` | | Status | Reviewed | | Open findings | 0 | @@ -27,6 +27,21 @@ a category produced nothing rather than leaving it blank. | 9 | Testing coverage | Driver.S7-014 | | 10 | Documentation & comments | Driver.S7-012 (shared) | +## Re-review 2026-06-19 (commit 7286d320) + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | Driver.S7-015 | +| 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-016 | +| 8 | Code organization & conventions | No issues found | +| 9 | Testing coverage | No issues found | +| 10 | Documentation & comments | No issues found | + ## Findings ### Driver.S7-001 @@ -450,3 +465,33 @@ live/mock-server happy-path coverage as an explicit follow-up rather than an open-ended deferral. **Resolution:** Resolved 2026-05-22 — factored `ReadOneAsync` type-reinterpret into `internal static ReinterpretRawValue` and `WriteOneAsync` boxing into `internal static BoxValueForWrite`; added `S7TypeMappingTests.cs` (26 tests) covering every implemented type round-trip (Bool/Byte/UInt16/Int16/UInt32/Int32/Float32), unsupported-type `NotSupportedException` assertions, and write overflow paths. + +### Driver.S7-015 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Correctness & logic bugs | +| Location | `S7Driver.cs` — `RejectUnsupportedTagConfigs`, `WriteOneAsync` | +| Status | Resolved | + +**Description:** `WriteOneAsync` has no array write path. Array reads are fully implemented (`ReadArrayAsync` / `DecodeArrayBlock`), but `WriteOneAsync` has no symmetric `WriteArrayAsync` branch. When a writable array tag (e.g., `Int16[4]`) receives a write, `WriteOneAsync` falls through to `BoxValueForWrite(tag.DataType, value)` with the whole typed array as the value argument. `Convert.ToInt16(short[])` throws `InvalidCastException`, which the outer catch maps to `BadCommunicationError` — misleading to an operator who expects `BadNotSupported`. An init-time guard that rejects writable array tags existed only for wide-type arrays (guard-a in `RejectUnsupportedTagConfigs`); non-wide arrays (Int16[], UInt16[], Float32[], etc.) were accepted at init, created writable OPC UA nodes, and failed on every write with an uninformative status code. + +**Recommendation:** Add init-time guard-(e) in `RejectUnsupportedTagConfigs` rejecting any `Writable=true` array tag (non-wide types). Also add a defensive `NotSupportedException` at the top of `WriteOneAsync` for transient equipment-tag refs that bypass init validation. Both paths correctly map to `BadNotSupported` via the existing catch ladder. + +**Resolution:** Resolved 2026-06-19 (commit SHA blank) — added guard-(e) to `RejectUnsupportedTagConfigs` that throws `NotSupportedException` for any non-wide writable array tag; added a defensive `NotSupportedException` at the top of `WriteOneAsync` for transient equipment-tag refs. Regression tests: `Initialize_rejects_writable_array_tag_with_NotSupportedException` (Theory, 7 element types × non-wide path) and `Initialize_accepts_readonly_array_tag` (verifies read-only arrays still pass). + +### Driver.S7-016 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Design-document adherence | +| Location | `S7DriverFactoryExtensions.cs` — `S7TagDto`, `BuildTag` | +| Status | Deferred | + +**Description:** `S7TagDto` lacks `ArrayCount`/`IsArray` fields and `BuildTag` never passes `ArrayCount` to `S7TagDefinition`. As a result, tags authored via the standard driver JSON config (the factory path) can never be configured as arrays — `ArrayCount` is always null. Only equipment-tag refs resolved transiently by `S7EquipmentTagParser` can carry `ArrayCount`. The array infrastructure (read + init guards) is in place; the factory DTO simply doesn't surface the knob. A site that wants to configure a multi-element tag via the standard tag table has no way to do so. + +**Recommendation:** Add `int? ArrayCount` and `bool? IsArray` to `S7TagDto` and propagate them through `BuildTag`. Mirror the `S7EquipmentTagParser` semantics (honour `arrayLength` only when `isArray` is true and the value is positive). + +**Resolution:** Deferred — requires an AdminUI page/DTO change and a corresponding config-form field addition. Tracked as a follow-up to the array-read Phase 4d work. No code change in this review; the existing init guard-(e) correctly rejects any programmatically-constructed writable array reaching the driver until the DTO is wired. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index 96cf059c..a400afd0 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -378,6 +378,21 @@ public sealed class S7Driver $"S7 tag '{t.Name}' is a Timer/Counter ('{t.Address}') declared Writable — " + "Timer/Counter are read-only this phase; set Writable=false."); } + + // (e) Array tag declared Writable — WriteOneAsync has no WriteArrayAsync path yet + // (Driver.S7-015). Without this guard a writable array node is discovered and + // accepted, then every write returns BadCommunicationError (InvalidCastException from + // BoxValueForWrite receiving a typed array) instead of the informative BadNotSupported + // the caller could act on. Wide-type arrays are already rejected above (guard-a); this + // targets non-wide types (Int16[], Byte[], Float32[], etc.) that ARE supported for + // reading but not yet for writing. + if (t.ArrayCount is >= 1 && t.Writable) + { + throw new NotSupportedException( + $"S7 tag '{t.Name}' is a {t.DataType} array (ArrayCount={t.ArrayCount}) declared Writable — " + + "array writes are not yet supported by the S7 driver. Set Writable=false for read-only array tags, " + + "or split into individual scalar tags until array-write support lands."); + } } } @@ -1013,6 +1028,15 @@ public sealed class S7Driver private async Task WriteOneAsync(Plc plc, S7TagDefinition tag, object? value, CancellationToken ct) { + // Defence-in-depth guard for Driver.S7-015: authored array tags are rejected at init + // (RejectUnsupportedTagConfigs guard-e), but a transient equipment-tag ref resolved by + // _resolver bypasses that path. Both should fail with NotSupportedException → BadNotSupported, + // not InvalidCastException → BadCommunicationError from BoxValueForWrite receiving an array. + if (tag.ArrayCount is >= 1) + throw new NotSupportedException( + $"S7 array writes are not yet supported (tag '{tag.Name}'). " + + "Use individual scalar tags or set Writable=false for read-only arrays."); + // Parse the address the same way ReadOneAsync does: authored tags pre-parse at init // (_parsedByName); an equipment-tag ref (resolved transiently) parses on demand. Needed // here so the wide-type write can byte-address the block (the narrow path below addresses diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverScaffoldTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverScaffoldTests.cs index 005c9d5c..92a0f4ae 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverScaffoldTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7DriverScaffoldTests.cs @@ -225,4 +225,61 @@ public sealed class S7DriverScaffoldTests drv.GetHealth().State.ShouldBe(DriverState.Faulted); } + + // ── Driver.S7-015 — writable array tag must fail fast at init ──────────────────────── + // + // Array reads are implemented (ReadArrayAsync / DecodeArrayBlock), but WriteOneAsync has + // no WriteArrayAsync path. Without the init guard a writable array node is discovered and + // accepted, then every write returns BadCommunicationError (InvalidCastException from + // BoxValueForWrite receiving a typed array). Fail fast at init instead. + + /// Verifies that a writable non-wide array (e.g., Int16[4]) is rejected at init + /// with a clear "array writes not yet supported" message — Driver.S7-015. + [Theory] + [InlineData(S7DataType.Bool, "DB1.DBX0.0", 4)] + [InlineData(S7DataType.Byte, "DB1.DBB0", 8)] + [InlineData(S7DataType.Int16, "DB1.DBW0", 4)] + [InlineData(S7DataType.UInt16, "DB1.DBW0", 4)] + [InlineData(S7DataType.Int32, "DB1.DBD0", 2)] + [InlineData(S7DataType.UInt32, "DB1.DBD0", 2)] + [InlineData(S7DataType.Float32,"DB1.DBD0", 2)] + public async Task Initialize_rejects_writable_array_tag_with_NotSupportedException( + S7DataType dt, string addr, int count) + { + var opts = new S7DriverOptions + { + Host = "192.0.2.1", + Timeout = TimeSpan.FromMilliseconds(250), + Tags = [new S7TagDefinition("ArrTag", addr, dt, Writable: true, ArrayCount: count)], + }; + using var drv = new S7Driver(opts, $"s7-writable-arr-{dt}"); + + var ex = await Should.ThrowAsync(async () => + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); + ex.Message.ShouldContain("ArrTag"); + ex.Message.ShouldContain("array", Case.Insensitive); + ex.Message.ShouldContain("write", Case.Insensitive); + + drv.GetHealth().State.ShouldBe(DriverState.Faulted); + } + + /// Verifies that a read-only (Writable=false) array passes the init guard — + /// array reads are fully implemented; only writes are gated. + [Fact] + public async Task Initialize_accepts_readonly_array_tag() + { + var opts = new S7DriverOptions + { + Host = "192.0.2.1", + Timeout = TimeSpan.FromMilliseconds(250), + Tags = [new S7TagDefinition("ReadArr", "DB1.DBW0", S7DataType.Int16, Writable: false, ArrayCount: 4)], + }; + using var drv = new S7Driver(opts, "s7-readonly-arr"); + + // Must NOT throw NotSupportedException — the failure must be the TCP connect (unreachable host). + var ex = await Should.ThrowAsync(async () => + await drv.InitializeAsync("{}", TestContext.Current.CancellationToken)); + ex.ShouldNotBeOfType( + "read-only arrays are fully supported — the failure must be the TCP connect, not the array guard"); + } }