dohertj2/lmxopcua
1
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

review(Driver.TwinCAT): thread ArrayLength through factory DTO (Medium)

Browse Source 
Re-review at 7286d320. -017 (Medium): TwinCATTagDto lacked ArrayLength, so JSON-authored
pre-declared array tags were silently scalar (Phase-4c array path dead for them). Fix:
add ArrayLength to the DTO + thread through BuildTag with positive-value guard + TDD.
This commit is contained in:
Joseph Doherty
2026-06-19 11:47:11 -04:00
parent 91e2609560
commit 22f7d92b72
3 changed files with 130 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/Driver.TwinCAT/findings.md
+54 -2
View File
@@ -4,8 +4,8 @@
|---|---|
| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT` |
| Reviewer | Claude Code |
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Review date | 2026-06-19 |
| Commit reviewed | `04e0877b` |
| Status | Reviewed |
| Open findings | 0 |
@@ -27,6 +27,25 @@ a category produced nothing rather than leaving it blank.
| 9 | Testing coverage | Driver.TwinCAT-016 |
| 10 | Documentation & comments | Driver.TwinCAT-004 (data-type comment), Driver.TwinCAT-014 |
## Re-review 2026-06-19 (commit 04e0877b)
All 16 prior findings remain Resolved. One new finding (Driver.TwinCAT-017) identified and fixed in this session.
#### Re-review checklist
| # | Category | Result |
|---|---|---|
| 1 | Correctness & logic bugs | Driver.TwinCAT-017 |
| 2 | OtOpcUa conventions | No new issues |
| 3 | Concurrency & thread safety | No new issues |
| 4 | Error handling & resilience | No new issues |
| 5 | Security | No issues found |
| 6 | Performance & resource management | No new issues |
| 7 | Design-document adherence | No new issues |
| 8 | Code organization & conventions | No new issues |
| 9 | Testing coverage | No new issues (2 regression tests added for -017) |
| 10 | Documentation & comments | No new issues |
## Findings
### Driver.TwinCAT-001
@@ -424,3 +443,36 @@ addressed, especially a concurrency stress test for `EnsureConnectedAsync` and a
`ReinitializeAsync`-applies-new-config test.
**Resolution:** Resolved 2026-05-23 — the previously-closed High findings each grew their regression coverage as they were resolved (see `TwinCATHighFindingsRegressionTests`: `ReinitializeAsync_applies_changed_device_config` for -001, `LInt_read_round_trips_value_above_int_MaxValue` + `DataType_mapping_preserves_width_and_signedness` for -002, `Concurrent_reads_on_one_device_create_a_single_client` + `Concurrent_reads_and_writes_share_one_client` for -007, `Symbol_version_changed_raises_OnRediscoveryNeeded` + `TwinCATDriver_implements_IRediscoverable` for -013). This pass added the two remaining gaps: `Structure_typed_pre_declared_tag_is_rejected_at_config_parse` (-003) and `Probe_loop_and_read_share_one_client_per_device` (-009 disposal-race coverage races 64 readers against the probe loop for 500ms and asserts a single client / single connect). All coverage lives in the test files `TwinCATHighFindingsRegressionTests.cs` and the new `TwinCATLowFindingsRegressionTests.cs`.
### Driver.TwinCAT-017
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | `TwinCATDriverFactoryExtensions.cs:168-188` (`TwinCATTagDto`) |
| Status | Resolved |
**Description:** `TwinCATTagDto` (the JSON-deserialization DTO for pre-declared tags in the
driver config) lacked an `ArrayLength` field. `BuildTag` therefore always constructed
`TwinCATTagDefinition` with `ArrayLength: null`, making it impossible to declare a 1-D array
pre-declared tag via the JSON driver config. The Phase 4c array-read path (`ReadValueAsync`
with `arrayCount != null`) and the array-discovery path (`IsArray` / `ArrayDim` in
`DiscoverAsync`) were both silently unavailable to operators who author pre-declared tags in
the driver config JSON — only equipment-tag JSON blobs (`TwinCATEquipmentTagParser`) and the
in-memory `TwinCATTagDefinition` constructor (used in tests) supported arrays.
This is a correctness gap introduced alongside Phase 4c (the two authoring surfaces were
inconsistent) but not caught because the Phase 4c tests use the in-memory constructor directly
rather than round-tripping through the JSON parser.
**Recommendation:** Add `int? ArrayLength` to `TwinCATTagDto` and thread it through `BuildTag`
(only positive values are honoured; null / non-positive keeps the scalar default, mirroring the
equipment-tag parser's `ReadArrayLength` guard).
**Resolution:** Resolved 2026-06-19 — added `int? ArrayLength` to `TwinCATTagDto`; `BuildTag`
now passes `ArrayLength: t.ArrayLength is > 0 ? t.ArrayLength : null` to the
`TwinCATTagDefinition` constructor. Two regression tests in `TwinCATArraySupportTests`:
`Predeclared_array_tag_arrayLength_parses_from_driver_config_json` (parse) and
`Predeclared_array_tag_from_json_reports_IsArray_in_discovery` (end-to-end discovery). All 170
unit tests pass. (SHA blank — fix committed in this review session.)