From d5b8c802ce5d954844b28e693853e5d651e41122 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 22 May 2026 09:20:22 -0400 Subject: [PATCH] fix(driver-abcip): resolve Medium code-review finding (Driver.AbCip-005) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Structure tags with declared Members no longer register the bare parent name in `_tagsByName` — reading it would return Good/null, which is misleading. Clients read individual member paths. Both the member fan-out and the scalar-tag paths now perform a duplicate-key check that throws `InvalidOperationException` naming both colliding entries (fail- fast, consistent with the AbCipHostAddress validation pattern). Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Driver.AbCip/findings.md | 4 +-- .../AbCipDriver.cs | 25 ++++++++++++++++++- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/code-reviews/Driver.AbCip/findings.md b/code-reviews/Driver.AbCip/findings.md index f4e158a..cf664bc 100644 --- a/code-reviews/Driver.AbCip/findings.md +++ b/code-reviews/Driver.AbCip/findings.md @@ -93,13 +93,13 @@ | Severity | Medium | | Category | Correctness & logic bugs | | Location | `AbCipDriver.cs:124-141` | -| Status | Open | +| Status | Resolved | **Description:** In `InitializeAsync`, when a `Structure` tag declares `Members`, the loop registers each fanned-out member into `_tagsByName` but the parent Structure tag itself is also left in `_tagsByName` (added at line 125 before the member check). A subsequent `ReadAsync` of the parent name routes through `ReadSingleAsync` then `DecodeValue(AbCipDataType.Structure, ...)` which returns `null` with `Good` status. A client reading the parent UDT node thus gets a Good/null snapshot rather than a fault or a structured value. Also, member registration does not check for name collisions: if two configured tags produce the same parent-dot-member key (or a member name collides with an independently-declared tag), the later silently overwrites the earlier with no diagnostic. **Recommendation:** Decide the parent-Structure read contract explicitly: either do not register the bare parent name as a readable tag, or have the Structure read return a proper status. Add a duplicate-key check during `_tagsByName` population that throws an `InvalidOperationException` naming both colliding tags, consistent with the fail-fast validation `AbCipHostAddress` parsing already does. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — Structure tags with declared members are no longer added to `_tagsByName` under the bare parent name; clients must address individual member paths. Both member fan-out and scalar tag registration now perform a duplicate-key check that throws `InvalidOperationException` naming both colliding tags (fail-fast, consistent with `AbCipHostAddress` parsing). ### Driver.AbCip-006 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs index 70f46fd..7f88fb2 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipDriver.cs @@ -144,7 +144,10 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, } foreach (var tag in _options.Tags) { - _tagsByName[tag.Name] = tag; + // Driver.AbCip-005: Structure tags whose Members are declared fan out to per-member + // entries only. The bare parent name is deliberately NOT registered as a readable + // tag — reading it would return Good/null (DecodeValue for Structure returns null), + // which is misleading. Clients read the individual member paths instead. if (tag.DataType == AbCipDataType.Structure && tag.Members is { Count: > 0 }) { foreach (var member in tag.Members) @@ -156,9 +159,29 @@ public sealed class AbCipDriver : IDriver, IReadable, IWritable, ITagDiscovery, DataType: member.DataType, Writable: member.Writable, WriteIdempotent: member.WriteIdempotent); + // Duplicate-key check: a collision means two configured tags produce the + // same readable path (e.g. two structure parents with the same member name, + // or a member name that matches an independently-declared tag). Fail fast + // at init time with a diagnostic rather than silently clobbering. + if (_tagsByName.TryGetValue(memberTag.Name, out var existing)) + throw new InvalidOperationException( + $"AbCip tag name collision: '{memberTag.Name}' is produced by both " + + $"'{tag.Name}.{member.Name}' (member fan-out) and an existing tag " + + $"'{existing.Name}'. Rename one of the configured tags to resolve."); _tagsByName[memberTag.Name] = memberTag; } } + else + { + // Non-structure tag (or Structure with no declared members — kept readable + // so the whole-UDT path or declaration-only grouping can serve it). + if (_tagsByName.TryGetValue(tag.Name, out var existing)) + throw new InvalidOperationException( + $"AbCip tag name collision: '{tag.Name}' is declared more than once. " + + $"Existing entry DeviceHostAddress='{existing.DeviceHostAddress}', " + + $"TagPath='{existing.TagPath}'. Rename or remove the duplicate."); + _tagsByName[tag.Name] = tag; + } } // Probe loops — one per device when enabled + a ProbeTagPath is configured.