From e8db0a683feb4dff782f063ce1750c299dbe5727 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Wed, 15 Jul 2026 11:28:34 -0400 Subject: [PATCH] fix(drivers): flip runtime tag parsers to strict enum parsing (#457) R2-11 Phase C. All six equipment-tag parsers (Modbus/S7/AbCip/AbLegacy/ TwinCAT/Focas) now read enum fields via the new TagConfigJson.TryReadEnumStrict: absent -> fallback, valid -> parsed, present-but-invalid (typo) -> TryParse returns false -> EquipmentTagRefResolver.TryResolve false -> driver surfaces BadNodeIdUnknown, instead of the old lenient path that silently defaulted a typo to a wrong-width Good. Modbus flips all three enum fields (region/dataType/byteOrder); the other five flip dataType. The deploy-time Deployment:TagConfigValidationMode=Error gate is unchanged and remains the operator pre-flight. Coverage: - Six *EquipmentTagParserStrictnessTests inverted from Freeze_typo_* (lenient) to Typo_*_rejects_the_tag + Valid_*_still_parses. - TagConfigJsonTests.TryReadEnumStrict_rejects_only_invalid matrix. - Driver-level end-to-end proof: AbCipEquipmentTagTests.Driver_read_of_a_typod_dataType_ref_surfaces_BadNodeIdUnknown drives the real AbCipDriver.ReadAsync through resolve->status. Golden parity corpus has no typo'd enums, so the flip is a no-op there. Full solution builds clean; all six driver suites + core + parity + gate green. --- archreview/plans/STATUS.md | 15 ++++++-- .../TagConfigJson.cs | 29 ++++++++++++++ .../AbCipEquipmentTagParser.cs | 4 +- .../AbLegacyEquipmentTagParser.cs | 4 +- .../FocasEquipmentTagParser.cs | 4 +- .../ModbusEquipmentTagParser.cs | 8 ++-- .../S7EquipmentTagParser.cs | 4 +- .../TwinCATEquipmentTagParser.cs | 4 +- .../TagConfigJsonTests.cs | 13 +++++++ .../AbCipEquipmentTagParserStrictnessTests.cs | 17 ++++++--- .../AbCipEquipmentTagTests.cs | 27 +++++++++++++ ...LegacyEquipmentTagParserStrictnessTests.cs | 16 ++++++-- .../FocasEquipmentTagParserStrictnessTests.cs | 18 ++++++--- ...ModbusEquipmentTagParserStrictnessTests.cs | 38 ++++++++++++++++--- .../S7EquipmentTagParserStrictnessTests.cs | 14 +++++-- ...winCATEquipmentTagParserStrictnessTests.cs | 16 ++++++-- 16 files changed, 193 insertions(+), 38 deletions(-) diff --git a/archreview/plans/STATUS.md b/archreview/plans/STATUS.md index 37563359..b85f81c9 100644 --- a/archreview/plans/STATUS.md +++ b/archreview/plans/STATUS.md @@ -177,7 +177,7 @@ what the unit leg cannot; (c) migrate the 3 xunit v2 holdouts once Akka ships an | **R2-10** | 03/U10=04/U-5 (Medium/Low, action #10) | `r2/10-resilience-observability` (off `46fedda3`) | `205d3980`…`` (11 task commits) | **Code-complete, offline-verified.** The resilience status tracker now has a production reader, mirroring the driver-health flow. Core truthfulness: `RecordBreakerClose` + derived `IsBreakerOpen` (breaker-open state derivable), `OnClosed` records the close, and `RecordSuccess` wired onto the invoker success paths (`ConsecutiveFailures` is a true consecutive counter, not stuck-forever). Transport: `DriverResilienceStatusChanged` Commons contract → periodic 5 s full-snapshot publisher `DriverResilienceStatusPublisherService` (driver nodes, registered in `AddOtOpcUaDriverFactories` with wiring guard `ResilienceStatusReaderRegistrationTests` — closes the built-but-never-wired class) → `driver-resilience-status` DPS topic → per-admin-node `DriverResilienceStatusBridge` → `IDriverResilienceStatusStore` → resilience chips (breaker-OPEN / consecutive-failures / in-flight / staleness) on `DriverStatusPanel.razor`, read in-process (self-HubConnection ban honored). Core.Tests resilience 31/31 (incl. deterministic breaker-close via a controllable TimeProvider), Host.IntegrationTests publisher+guard 6/6, AdminUI.Tests store 5/5; full-solution build clean. **Deferred (T10):** docker-dev live-`/run` — S7 dead-endpoint breaker-open observed on BOTH centrals + recovery-clear + staleness-dim (also the first live sighting of the #10 `circuit-breaker OPENED` log line, closing FOLLOWUP-10's behavioral gate). | | **R2-08** | 03/S2 (High, action #8), 03/S3 (High) | `r2/08-ldap-historyread-async` (off `1a698cbb`) | `ce6d9e21`…`` (14 task commits) | **Code-complete, offline-verified.** **S2 (LDAP async/timeout):** the SDK raises the impersonation callback synchronously under a `SessionManager`-wide `m_eventLock` (verified vs. 1.5.378 — a hung bind serializes ALL activations), so the fix is entirely at the `LdapOpcUaUserAuthenticator` boundary: five new `Security:Ldap` keys (`ConnectionTimeoutMs`=10000 projected into the shared lib, `AuthTimeoutMs`=15000 hard whole-flow `WaitAsync` bound → fail-closed "timed out", `MaxConcurrentAuthentications`=8 `SemaphoreSlim` with release-on-core-completion so orphaned binds still count, `OutageFailureThreshold`=3/`OutageCooldownSeconds`=15 directory-outage circuit → fast-deny "unavailable", half-open probe) + `LdapAuthResult.IsSystemFailure` seam (mapped from `ServiceAccountBindFailed`) distinguishing outage from credential-deny + `LdapOptionsValidator` rules + `HandleImpersonation` contract doc + `docs/security.md`. Stall-repro RED→GREEN; offline outage-sim through the real `HandleImpersonation` against an unroutable host. **S3 (HistoryRead fan-out):** all four HistoryRead arms async-ified (`ServeNodeAsync`/`ServeRawPagedAsync`/`ServeEventsAsync`, OCE→`BadTimeout`) then bounded per-node fan-out (`RunBounded` semaphore + single arm-boundary bridge) + per-request deadline CTS (`HistoryReadDeadline`=60s) + process-wide lazy batch limiter (`MaxConcurrentHistoryReadBatches`=16 → flood degrades to `BadTooManyOperations`); 16×4=64 in-flight cap; three new `ServerHistorian` keys + node-manager props + hosted-service wiring + `docs/Historian.md`/CLAUDE.md tables. Sequential-serving RED→GREEN. Host.IntegrationTests (LDAP resilience) 13/13, Security.Tests 11/11, Runtime.Tests options 11/11, OpcUaServer.Tests HistoryRead 30/30. **S2 GLAuth outage/recovery LIVE gate CLOSED 2026-07-15 (PR #455, master `5f12636c`)** — new env-gated `LdapGlauthOutageLiveTests` drives the circuit through the production path (`OtOpcUaLdapAuthService`+`LdapOpcUaUserAuthenticator`) against a REAL GLAuth on `10.100.0.35:3894` (paused/unpaused via `docker pause`); GREEN (12s): healthy alice/bob binds w/ mapped roles → pause → 3 bounded failures → circuit opens → sub-second "unavailable" → unpause → half-open close → recover. Also the FIRST end-to-end proof the PR #451 GLAuth swap binds correctly. Skips clean offline (1 skipped, 8ms). **S3 `Category=LiveIntegration` gateway soak — GREEN 2026-07-15 (6/6)** against the running local 0.2.0 gateway (`http://localhost:5221` h2c) + the real AVEVA historian over VPN; the Galaxy read round-trip exercises the `GatewayHistorianDataSource` read path S3 hardened. **R2-08 fully live-verified.** | -| **R2-11** | 01/C-1, 01/P-1 + 05/CONV-2, UNDER-1, UNDER-6 | `r2/11-tagconfig-consolidation` (off `46fedda3`) | first `2e14fe1f` … (24 task commits) | **Code-complete, offline-verified.** **Part 1 (C-1/P-1):** single `TagConfigIntent.Parse` + `DeviceConfigIntent` in Commons replace the four byte-parity TagConfig parse copies (composer/artifact/walker/DraftValidator) — composer now parses once per tag (P-1); a permanent `Runtime.Tests/TagConfigCorpusParityTests` (30-blob corpus) guards compose≡decode; `grep "MUST parse identically" src/` = 0. **Part 2 (CONV-2):** shared strict-capable `TagConfigJson` readers in Core.Abstractions replace the six copy-pasted `ReadEnum`s; all six driver parsers gain `Inspect()` warnings + honour the `writable` key (FOCAS **forced read-only**, 05/UNDER-1); FOCAS equipment tags run the capability-matrix pre-flight (05/UNDER-6); Modbus probe parses the factory DTO shape (`timeoutMs` no longer dropped). New `EquipmentTagConfigInspector` in ControlPlane + `AdminOperationsActor` deploy-gate wiring with `Deployment:TagConfigValidationMode` (**Warn default | Error opt-in**), proven through the actor (F10b consuming test). AdminUI: `writable` checkbox in the six typed editors. **Phase-C follow-up (deferred, recorded here):** flip the runtime parsers to strict (typo'd enum ⇒ `BadNodeIdUnknown` instead of wrong-width `Good`) once fleets have run `Error` mode clean — behavior change deliberately not shipped in this plan. **Deferred (T22/T24):** docker-dev `/run` of one typed editor's Writable checkbox + whole-solution serial sweep. | +| **R2-11** | 01/C-1, 01/P-1 + 05/CONV-2, UNDER-1, UNDER-6 | `r2/11-tagconfig-consolidation` (off `46fedda3`) | first `2e14fe1f` … (24 task commits) | **Code-complete, offline-verified.** **Part 1 (C-1/P-1):** single `TagConfigIntent.Parse` + `DeviceConfigIntent` in Commons replace the four byte-parity TagConfig parse copies (composer/artifact/walker/DraftValidator) — composer now parses once per tag (P-1); a permanent `Runtime.Tests/TagConfigCorpusParityTests` (30-blob corpus) guards compose≡decode; `grep "MUST parse identically" src/` = 0. **Part 2 (CONV-2):** shared strict-capable `TagConfigJson` readers in Core.Abstractions replace the six copy-pasted `ReadEnum`s; all six driver parsers gain `Inspect()` warnings + honour the `writable` key (FOCAS **forced read-only**, 05/UNDER-1); FOCAS equipment tags run the capability-matrix pre-flight (05/UNDER-6); Modbus probe parses the factory DTO shape (`timeoutMs` no longer dropped). New `EquipmentTagConfigInspector` in ControlPlane + `AdminOperationsActor` deploy-gate wiring with `Deployment:TagConfigValidationMode` (**Warn default | Error opt-in**), proven through the actor (F10b consuming test). AdminUI: `writable` checkbox in the six typed editors. **Phase-C follow-up — SHIPPED 2026-07-15 (Gitea #457):** the runtime parsers are now strict (typo'd enum ⇒ `BadNodeIdUnknown` instead of wrong-width `Good`) via `TagConfigJson.TryReadEnumStrict` across all six drivers; six strictness suites inverted + a driver-level `ReadAsync`→`BadNodeIdUnknown` end-to-end proof. See the "Deferred behavior-change follow-up" section below. **Deferred (T22/T24):** docker-dev `/run` of one typed editor's Writable checkbox + whole-solution serial sweep. | | **R2-07 (Phases 1+2+3)** | 03/P1 (action #7, surgical pure-adds/removes/mixed) | `r2/07-surgical-pure-adds` (off `1a698cbb`) | `bb226f45`…`e2b47638` (T1–T14) | **Phases 1+2 code-complete, offline-verified.** New pure `AddressSpaceChangeClassifier` (Empty/AttributeOnly/PureAdd/PureRemove/AddRemoveMix/Rebuild; first-match-wins, default-closed Rebuild; the F10b tag/vtag surgical-eligibility predicates moved here as `internal static`). `AddressSpaceApplier.Apply` routes by kind. **Phase 1 (PureAdd/AttributeOnly):** skip `RebuildAddressSpace` (the idempotent Materialise passes create exactly the added nodes; existing subscriptions survive); adds announced via `ComputeAddAnnouncements`+`AnnounceAddedNodes` (dedup affected-parent → existing guard-covered `RaiseNodesAddedModelChange`), fired by `OpcUaPublishActor` after the 4 passes gated on `!outcome.RebuildCalled`; `MaterialiseAlarmCondition` made idempotent (skip-if-present-and-same-kind keeps the condition NodeState alive). **NO new sink members in Phase 1.** **Phase 2 (PureRemove):** 3 new `ISurgicalAddressSpaceSink` members (`RemoveVariableNode`/`RemoveAlarmConditionNode`/`RemoveEquipmentSubtree`) — guard-first with the **observed negative control** (`DeferredSinkForwardingReflectionTests` fails member-named until `DeferredAddressSpaceSink` forwards) + `SdkAddressSpaceSink` forwards + `OtOpcUaNodeManager` in-place teardown (scoped map cleanup, notifier demotion via `RemoveRootNotifier`, `BuildNodesRemovedModelChange` verb=NodeDeleted reported outside `Lock`). Applier `PureRemove` arm: terminal Bad / `RemovedConditionState` writes (closing the removed-alarm-tag today-gap) → subsumption partition (removed-equipment children folded into the subtree remove) → in-place removes → any false/throw ⇒ full-rebuild ratchet; non-surgical sink ⇒ rebuild. **Phase 3 (AddRemoveMix):** removes-then-adds within one apply — the remove pass runs in `Apply` (no rebuild), then the publish actor's Materialise passes create the adds + `AnnounceAddedNodes` announces them; an id reused across the remove+add sets is remove-then-recreated as a fresh NodeState (correct — a different tag). **Guard stays green (Commons.Tests 17/17), OpcUaServer.Tests 336/336, Runtime.Tests 398/398.** **03/P1 marked REMEDIATED in `archreview/03-server-runtime.md`.** **Deferred-live (T5, T6, T12, T14):** `SubscriptionSurvivalTests` add + remove + mixed cases (authored + compile-green; NOT run — `*.IntegrationTests` memory constraint) and the docker-dev live-`/run` add/remove/mixed gates (`kind=PureAdd/PureRemove/AddRemoveMix, rebuild=False`; survivor subscription uninterrupted; removed node → `BadNodeIdUnknown`) — MUST run in the serial heavy pass (F10b inertness risk). | @@ -226,9 +226,18 @@ shared docker-dev rig / VPN infra isn't contended. To run one-at-a-time: - **2-node redundancy rig:** ~~R2-04 T13/T15~~ **BOTH ✅ PASSED 2026-07-14** — T15 live docker-dev gate + T13 in-process delivery-path test (see below). - **Genuinely external / needs-user:** ~~ALL CLOSED as of 2026-07-15.~~ ~~R2-06 T12~~ (6/6 on the VPN, 2026-07-13). ~~R2-08 S3 `Category=LiveIntegration` gateway soak~~ **CLOSED 2026-07-15** (6/6 vs. the running local 0.2.0 gateway + real historian over VPN). ~~R2-01 T11~~ **CLOSED 2026-07-15** (S7 blackhole via `docker pause`; found + fixed the read-leg wall-clock gap, PR #453). ~~R2-08 GLAuth outage cycle~~ **CLOSED 2026-07-15** (`LdapGlauthOutageLiveTests` vs. the harness GLAuth paused via `docker pause`, PR #455). **Every Round-2 live gate is now GREEN.** -### Deferred behavior-change follow-up (deliberately not shipped) +### Deferred behavior-change follow-up — **SHIPPED 2026-07-15 (Gitea #457)** **R2-11 Phase C** — flip the runtime driver parsers to strict (typo'd enum ⇒ `BadNodeIdUnknown` instead of a -wrong-width `Good`). Land only after fleets have run `Deployment:TagConfigValidationMode=Error` clean. +wrong-width `Good`). **DONE.** All six equipment-tag parsers (`{Modbus,S7,AbCip,AbLegacy,TwinCAT,Focas}EquipmentTagParser.TryParse`) +now read enums via the new `TagConfigJson.TryReadEnumStrict` (absent ⇒ fallback; valid ⇒ parsed; **present-but-invalid ⇒ +`TryParse` returns false ⇒ `EquipmentTagRefResolver.TryResolve` false ⇒ driver surfaces `BadNodeIdUnknown`**). Modbus flips +all three enum fields (`region`/`dataType`/`byteOrder`); the other five flip `dataType`. Regression coverage: the six +`*EquipmentTagParserStrictnessTests` inverted from `Freeze_typo_*` (lenient) to `Typo_*_rejects_the_tag` + `Valid_*_still_parses`; +a `TagConfigJsonTests.TryReadEnumStrict_rejects_only_invalid` matrix; and a **driver-level end-to-end proof** +(`AbCipEquipmentTagTests.Driver_read_of_a_typod_dataType_ref_surfaces_BadNodeIdUnknown`) driving the real `AbCipDriver.ReadAsync` +through resolve→status. The deploy-time `Deployment:TagConfigValidationMode=Error` gate (unit-proven in +`AdminOperationsActorTagConfigGateTests`) remains the operator's pre-flight — a fleet that deploys clean under `Error` has, by +construction, no typo'd enums and is safe on the flip. The golden parity corpus carries no typo'd enums, so the flip is a no-op there. ## docker-dev live-`/run` pass (2026-07-13) — CONCLUDED; 5 gates LIVE-confirmed on the built-from-source `6aeb53a9` rig diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/TagConfigJson.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/TagConfigJson.cs index 950b08a9..9833df5d 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/TagConfigJson.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/TagConfigJson.cs @@ -68,6 +68,35 @@ public static class TagConfigJson where TEnum : struct, Enum => TryReadEnum(o, name, out var v) == JsonEnumRead.Valid ? v : fallback; + /// + /// Strict enum read (R2-11 Phase C — the runtime flip). Absent ⇒ ; + /// a present, case-insensitively-parseable string ⇒ the parsed value; a present-but-invalid + /// string (a typo) ⇒ the read FAILS (returns ) so the runtime parser can + /// reject the whole tag and the driver surfaces BadNodeIdUnknown — instead of silently + /// substituting a wrong-width default and publishing a misleading Good. This is the runtime + /// counterpart of : identical absent/valid handling, but + /// is fatal rather than defaulted. The deploy-time gate + /// (TagConfigValidationMode.Error) is expected to have already surfaced the same typo via + /// ; a typo reaching this fatal path means it was authored + /// after the gate, or the gate ran in Warn mode. + /// + /// The enum type to parse. + /// The TagConfig root object. + /// The property name to read. + /// The value assigned when the field is absent. + /// The parsed enum (valid), the fallback (absent), or default (invalid). + /// only when the field is a present-but-invalid string; otherwise . + public static bool TryReadEnumStrict(JsonElement o, string name, TEnum fallback, out TEnum value) + where TEnum : struct, Enum + { + switch (TryReadEnum(o, name, out value)) + { + case JsonEnumRead.Valid: return true; + case JsonEnumRead.Absent: value = fallback; return true; + default: value = default; return false; // Invalid (typo) ⇒ reject the tag + } + } + /// /// Reads the optional "writable" flag with AbCip semantics (the model): an explicit JSON /// false ⇒ ; anything else (absent, true, or a non-bool token) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs index a474d0ff..82e77ee4 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs @@ -43,7 +43,9 @@ public static class AbCipEquipmentTagParser // resolves the full tag path (e.g. "Motor.Speed") without enumerating UDT members. // The address space emits a placeholder String variable; UDT member declarations are // not supported in the equipment-tag flow. - var dataType = TagConfigJson.ReadEnumOrDefault(root, "dataType", AbCipDataType.DInt); + // Strict enum read (R2-11 Phase C): a typo'd dataType rejects the tag (→ BadNodeIdUnknown) + // instead of silently defaulting to a wrong-width Good. + if (!TagConfigJson.TryReadEnumStrict(root, "dataType", AbCipDataType.DInt, out var dataType)) return false; // Review I-1 — an equipment tag is an ARRAY ⟺ isArray:true AND arrayLength >= 1. A // 1-element array (isArray:true, arrayLength:1) is a VALID 1-element array — the // foundation materialises a [1] OPC UA array node — so it must read as an array, not a diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyEquipmentTagParser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyEquipmentTagParser.cs index 12469c9a..59d20d7a 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyEquipmentTagParser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyEquipmentTagParser.cs @@ -39,7 +39,9 @@ public static class AbLegacyEquipmentTagParser return false; var address = addr.GetString(); if (string.IsNullOrWhiteSpace(address)) return false; - var dataType = TagConfigJson.ReadEnumOrDefault(root, "dataType", AbLegacyDataType.Int); + // Strict enum read (R2-11 Phase C): a typo'd dataType rejects the tag (→ BadNodeIdUnknown) + // instead of silently defaulting to a wrong-width Good. + if (!TagConfigJson.TryReadEnumStrict(root, "dataType", AbLegacyDataType.Int, out var dataType)) return false; var deviceHostAddress = ReadString(root, "deviceHostAddress"); int? arrayLength = null; if (IsArrayFlag(root)) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasEquipmentTagParser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasEquipmentTagParser.cs index 0e47e16e..1fe27347 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasEquipmentTagParser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasEquipmentTagParser.cs @@ -29,7 +29,9 @@ public static class FocasEquipmentTagParser return false; var address = addr.GetString(); if (string.IsNullOrWhiteSpace(address)) return false; - var dataType = TagConfigJson.ReadEnumOrDefault(root, "dataType", FocasDataType.Int32); + // Strict enum read (R2-11 Phase C): a typo'd dataType rejects the tag (→ BadNodeIdUnknown) + // instead of silently defaulting to a wrong-width Good. + if (!TagConfigJson.TryReadEnumStrict(root, "dataType", FocasDataType.Int32, out var dataType)) return false; var deviceHostAddress = ReadString(root, "deviceHostAddress") ?? ""; def = new FocasTagDefinition( Name: reference, DeviceHostAddress: deviceHostAddress, Address: address, diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusEquipmentTagParser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusEquipmentTagParser.cs index 65b16e29..7ebc789a 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusEquipmentTagParser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusEquipmentTagParser.cs @@ -30,9 +30,11 @@ public static class ModbusEquipmentTagParser || !addr.TryGetInt32(out var address) || address < 0 || address > ushort.MaxValue) return false; - var region = TagConfigJson.ReadEnumOrDefault(root, "region", ModbusRegion.HoldingRegisters); - var dataType = TagConfigJson.ReadEnumOrDefault(root, "dataType", ModbusDataType.Int16); - var byteOrder = TagConfigJson.ReadEnumOrDefault(root, "byteOrder", ModbusByteOrder.BigEndian); + // Strict enum reads (R2-11 Phase C): a present-but-invalid (typo'd) enum rejects the tag + // (→ BadNodeIdUnknown) instead of silently defaulting to a wrong-width Good. + if (!TagConfigJson.TryReadEnumStrict(root, "region", ModbusRegion.HoldingRegisters, out var region)) return false; + if (!TagConfigJson.TryReadEnumStrict(root, "dataType", ModbusDataType.Int16, out var dataType)) return false; + if (!TagConfigJson.TryReadEnumStrict(root, "byteOrder", ModbusByteOrder.BigEndian, out var byteOrder)) return false; var bitIndex = (byte)ReadInt(root, "bitIndex"); var stringLength = (ushort)ReadInt(root, "stringLength"); // Guard: String tags require StringLength >= 1. RegisterCount = (StringLength+1)/2, 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 320db73b..8f0cdf1c 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 @@ -32,7 +32,9 @@ public static class S7EquipmentTagParser return false; var address = addrEl.GetString(); if (string.IsNullOrWhiteSpace(address)) return false; - var dataType = TagConfigJson.ReadEnumOrDefault(root, "dataType", S7DataType.Int16); + // Strict enum read (R2-11 Phase C): a typo'd dataType rejects the tag (→ BadNodeIdUnknown) + // instead of silently defaulting to a wrong-width Good. + if (!TagConfigJson.TryReadEnumStrict(root, "dataType", S7DataType.Int16, out var dataType)) return false; var stringLength = ReadInt(root, "stringLength"); // 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 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 f4d620a4..af378af3 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 @@ -29,7 +29,9 @@ public static class TwinCATEquipmentTagParser var symbolPath = symbol.GetString(); if (string.IsNullOrWhiteSpace(symbolPath)) return false; var deviceHostAddress = ReadString(root, "deviceHostAddress"); - var dataType = TagConfigJson.ReadEnumOrDefault(root, "dataType", TwinCATDataType.DInt); + // Strict enum read (R2-11 Phase C): a typo'd dataType rejects the tag (→ BadNodeIdUnknown) + // instead of silently defaulting to a wrong-width Good. + if (!TagConfigJson.TryReadEnumStrict(root, "dataType", TwinCATDataType.DInt, out var dataType)) return false; // Array intent — same shape the runtime/OPC-UA foundation parses (camelCase // `isArray` bool + `arrayLength` uint). arrayLength is honoured ONLY when isArray // is true AND it is a positive JSON number, so a stale length behind a cleared diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/TagConfigJsonTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/TagConfigJsonTests.cs index b748551b..5a574b4a 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/TagConfigJsonTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/TagConfigJsonTests.cs @@ -9,6 +9,7 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests; /// Pins the shared strict-capable TagConfig field readers (R2-11, 05/CONV-2): the /// Absent/Valid/Invalid matrix (case-insensitive), /// lenient equivalence to the retired copies, +/// reject-only-invalid semantics (R2-11 Phase C), /// explicit-false-only semantics, and /// messages. /// @@ -44,6 +45,18 @@ public sealed class TagConfigJsonTests TagConfigJson.ReadEnumOrDefault(Root(json), "k", Sample.Alpha).ShouldBe(expected); } + [Theory] + [InlineData("{\"k\":\"Gamma\"}", true, Sample.Gamma)] // valid → parsed, ok + [InlineData("{}", true, Sample.Alpha)] // absent → fallback, ok + [InlineData("{\"k\":42}", true, Sample.Alpha)] // non-string → Absent → fallback, ok + [InlineData("{\"k\":\"typo\"}", false, default(Sample))] // invalid (typo) → REJECT (false) + public void TryReadEnumStrict_rejects_only_invalid(string json, bool expectedOk, Sample expectedValue) + { + var ok = TagConfigJson.TryReadEnumStrict(Root(json), "k", Sample.Alpha, out var v); + ok.ShouldBe(expectedOk); + v.ShouldBe(expectedValue); + } + [Theory] [InlineData("{\"writable\":false}", true, false)] // explicit false → false [InlineData("{\"writable\":true}", true, true)] // explicit true → default diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipEquipmentTagParserStrictnessTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipEquipmentTagParserStrictnessTests.cs index c5a84482..0441f976 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipEquipmentTagParserStrictnessTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipEquipmentTagParserStrictnessTests.cs @@ -4,15 +4,22 @@ using ZB.MOM.WW.OtOpcUa.Driver.AbCip; namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; -/// R2-11 (05/CONV-2) strictness surface for the AbCip equipment-tag parser: lenient runtime freeze, -/// Inspect reports the typo, and the already-honoured writable key stays bit-identical through -/// the shared reader swap. +/// R2-11 Phase C strictness surface for the AbCip equipment-tag parser: the runtime is now STRICT — a +/// typo'd dataType rejects the tag (TryParse ⇒ ⇒ BadNodeIdUnknown), +/// Inspect reports the typo, and the writable key stays bit-identical through the shared reader. public sealed class AbCipEquipmentTagParserStrictnessTests { [Fact] - public void Freeze_typo_dataType_still_defaults_to_DInt() + public void Typo_dataType_rejects_the_tag() { - AbCipEquipmentTagParser.TryParse("{\"tagPath\":\"Motor.Speed\",\"dataType\":\"DIntt\"}", out var def) + AbCipEquipmentTagParser.TryParse("{\"tagPath\":\"Motor.Speed\",\"dataType\":\"DIntt\"}", out _) + .ShouldBeFalse(); + } + + [Fact] + public void Valid_dataType_still_parses() + { + AbCipEquipmentTagParser.TryParse("{\"tagPath\":\"Motor.Speed\",\"dataType\":\"DInt\"}", out var def) .ShouldBeTrue(); def.DataType.ShouldBe(AbCipDataType.DInt); } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipEquipmentTagTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipEquipmentTagTests.cs index b1086ff3..af351073 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipEquipmentTagTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipEquipmentTagTests.cs @@ -76,4 +76,31 @@ public class AbCipEquipmentTagTests snapshots.Single().StatusCode.ShouldNotBe(AbCipStatusMapper.BadNodeIdUnknown); snapshots.Single().Value.ShouldBe(4242); } + + /// + /// End-to-end driver-level proof of the R2-11 Phase C flip: an equipment-tag ref carrying a + /// TYPO'd dataType enum now REJECTS at the resolver (strict parse ⇒ TryParse false) + /// so the read surfaces — instead of the old + /// lenient behaviour that silently defaulted to DInt and published a wrong-width + /// Good. Same driver + device wiring as the clean-read proof above; only the enum is + /// mistyped. + /// + [Fact] + public async Task Driver_read_of_a_typod_dataType_ref_surfaces_BadNodeIdUnknown() + { + var typoJson = """{"deviceHostAddress":"ab://10.0.0.5/1,0","tagPath":"Motor1.Speed","dataType":"DIntt"}"""; + var factory = new FakeAbCipTagFactory(); + var opts = new AbCipDriverOptions + { + Devices = [new AbCipDeviceOptions("ab://10.0.0.5/1,0")], + Tags = [], + }; + var drv = new AbCipDriver(opts, "abcip-eq", factory); + await drv.InitializeAsync("{}", CancellationToken.None); + factory.Customise = p => new FakeAbCipTag(p) { Value = 4242 }; + + var snapshots = await drv.ReadAsync([typoJson], CancellationToken.None); + + snapshots.Single().StatusCode.ShouldBe(AbCipStatusMapper.BadNodeIdUnknown); + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyEquipmentTagParserStrictnessTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyEquipmentTagParserStrictnessTests.cs index d54019b7..67f4dfbd 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyEquipmentTagParserStrictnessTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyEquipmentTagParserStrictnessTests.cs @@ -4,14 +4,22 @@ using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy; namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests; -/// R2-11 (05/CONV-2) strictness surface for the AbLegacy equipment-tag parser: lenient runtime -/// freeze, Inspect reports the typo, and the writable key is honoured (default true). +/// R2-11 Phase C strictness surface for the AbLegacy equipment-tag parser: the runtime is now STRICT — +/// a typo'd dataType rejects the tag (TryParse ⇒ ⇒ BadNodeIdUnknown), +/// Inspect reports the typo, and the writable key is honoured (default true). public sealed class AbLegacyEquipmentTagParserStrictnessTests { [Fact] - public void Freeze_typo_dataType_still_defaults_to_Int() + public void Typo_dataType_rejects_the_tag() { - AbLegacyEquipmentTagParser.TryParse("{\"address\":\"N7:0\",\"dataType\":\"Intt\"}", out var def) + AbLegacyEquipmentTagParser.TryParse("{\"address\":\"N7:0\",\"dataType\":\"Intt\"}", out _) + .ShouldBeFalse(); + } + + [Fact] + public void Valid_dataType_still_parses() + { + AbLegacyEquipmentTagParser.TryParse("{\"address\":\"N7:0\",\"dataType\":\"Int\"}", out var def) .ShouldBeTrue(); def.DataType.ShouldBe(AbLegacyDataType.Int); } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasEquipmentTagParserStrictnessTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasEquipmentTagParserStrictnessTests.cs index 1b487531..846b0b52 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasEquipmentTagParserStrictnessTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasEquipmentTagParserStrictnessTests.cs @@ -5,16 +5,24 @@ using ZB.MOM.WW.OtOpcUa.Driver.FOCAS; namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests; /// -/// R2-11 (05/CONV-2 + 05/UNDER-1) strictness surface for the FOCAS equipment-tag parser: lenient -/// runtime freeze on dataType, forced read-only (Writable == false regardless of any -/// authored writable:true), and Inspect reporting the typo + the write-request. +/// R2-11 Phase C (+ 05/UNDER-1) strictness surface for the FOCAS equipment-tag parser: the runtime is now +/// STRICT — a typo'd dataType rejects the tag (TryParse ⇒ ⇒ +/// BadNodeIdUnknown); FOCAS tags remain forced read-only (Writable == false regardless of any +/// authored writable:true), and Inspect reports the typo + the write-request. /// public sealed class FocasEquipmentTagParserStrictnessTests { [Fact] - public void Freeze_typo_dataType_still_defaults_to_Int32() + public void Typo_dataType_rejects_the_tag() { - FocasEquipmentTagParser.TryParse("{\"address\":\"D0100\",\"dataType\":\"Int322\"}", out var def) + FocasEquipmentTagParser.TryParse("{\"address\":\"D0100\",\"dataType\":\"Int322\"}", out _) + .ShouldBeFalse(); + } + + [Fact] + public void Valid_dataType_still_parses() + { + FocasEquipmentTagParser.TryParse("{\"address\":\"D0100\",\"dataType\":\"Int32\"}", out var def) .ShouldBeTrue(); def.DataType.ShouldBe(FocasDataType.Int32); } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusEquipmentTagParserStrictnessTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusEquipmentTagParserStrictnessTests.cs index 56690441..bcd3f2ac 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusEquipmentTagParserStrictnessTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusEquipmentTagParserStrictnessTests.cs @@ -5,19 +5,45 @@ using ZB.MOM.WW.OtOpcUa.Driver.Modbus; namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests; /// -/// R2-11 (05/CONV-2) strictness surface for the Modbus equipment-tag parser: the runtime stays lenient -/// (freeze), the new Inspect reports the silently-defaulted typo, and the writable key is -/// now honoured (default true). +/// R2-11 Phase C strictness surface for the Modbus equipment-tag parser: the runtime is now STRICT — a +/// present-but-invalid (typo'd) enum on any of the three enum fields (region / dataType / +/// byteOrder) rejects the whole tag (TryParse ⇒ ⇒ +/// BadNodeIdUnknown) instead of silently defaulting to a wrong-width Good. Inspect +/// still reports the same typo at deploy time, and the writable key is honoured (default true). /// public sealed class ModbusEquipmentTagParserStrictnessTests { - // ---- Phase-A behaviour freeze: a typo'd enum still silently defaults ---- + // ---- Phase C: a typo'd enum now rejects the tag (BadNodeIdUnknown), not a silent default ---- [Fact] - public void Freeze_typo_dataType_still_defaults_to_Int16() + public void Typo_dataType_rejects_the_tag() { ModbusEquipmentTagParser.TryParse( - "{\"region\":\"HoldingRegisters\",\"address\":1,\"dataType\":\"Intt16\"}", out var def) + "{\"region\":\"HoldingRegisters\",\"address\":1,\"dataType\":\"Intt16\"}", out _) + .ShouldBeFalse(); + } + + [Fact] + public void Typo_region_rejects_the_tag() + { + ModbusEquipmentTagParser.TryParse( + "{\"region\":\"HoldingRegisterz\",\"address\":1,\"dataType\":\"Int16\"}", out _) + .ShouldBeFalse(); + } + + [Fact] + public void Typo_byteOrder_rejects_the_tag() + { + ModbusEquipmentTagParser.TryParse( + "{\"region\":\"HoldingRegisters\",\"address\":1,\"dataType\":\"Int16\",\"byteOrder\":\"BigEndan\"}", out _) + .ShouldBeFalse(); + } + + [Fact] + public void Valid_enums_still_parse() + { + ModbusEquipmentTagParser.TryParse( + "{\"region\":\"HoldingRegisters\",\"address\":1,\"dataType\":\"Int16\",\"byteOrder\":\"BigEndian\"}", out var def) .ShouldBeTrue(); def.DataType.ShouldBe(ModbusDataType.Int16); } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7EquipmentTagParserStrictnessTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7EquipmentTagParserStrictnessTests.cs index 78a46964..83f1d7ff 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7EquipmentTagParserStrictnessTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7EquipmentTagParserStrictnessTests.cs @@ -4,14 +4,22 @@ using ZB.MOM.WW.OtOpcUa.Driver.S7; namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; -/// R2-11 (05/CONV-2) strictness surface for the S7 equipment-tag parser: lenient runtime freeze, +/// R2-11 Phase C strictness surface for the S7 equipment-tag parser: the runtime is now STRICT — a +/// typo'd dataType rejects the tag (TryParse ⇒ ⇒ BadNodeIdUnknown), /// Inspect reports the typo, and the writable key is honoured (default true). public sealed class S7EquipmentTagParserStrictnessTests { [Fact] - public void Freeze_typo_dataType_still_defaults_to_Int16() + public void Typo_dataType_rejects_the_tag() { - S7EquipmentTagParser.TryParse("{\"address\":\"DB1.DBW0\",\"dataType\":\"Intt16\"}", out var def) + S7EquipmentTagParser.TryParse("{\"address\":\"DB1.DBW0\",\"dataType\":\"Intt16\"}", out _) + .ShouldBeFalse(); + } + + [Fact] + public void Valid_dataType_still_parses() + { + S7EquipmentTagParser.TryParse("{\"address\":\"DB1.DBW0\",\"dataType\":\"Int16\"}", out var def) .ShouldBeTrue(); def.DataType.ShouldBe(S7DataType.Int16); } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATEquipmentTagParserStrictnessTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATEquipmentTagParserStrictnessTests.cs index 4e298ea2..4ce8dd8c 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATEquipmentTagParserStrictnessTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATEquipmentTagParserStrictnessTests.cs @@ -4,14 +4,22 @@ using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests; -/// R2-11 (05/CONV-2) strictness surface for the TwinCAT equipment-tag parser: lenient runtime -/// freeze, Inspect reports the typo, and the writable key is honoured (default true). +/// R2-11 Phase C strictness surface for the TwinCAT equipment-tag parser: the runtime is now STRICT — +/// a typo'd dataType rejects the tag (TryParse ⇒ ⇒ BadNodeIdUnknown), +/// Inspect reports the typo, and the writable key is honoured (default true). public sealed class TwinCATEquipmentTagParserStrictnessTests { [Fact] - public void Freeze_typo_dataType_still_defaults_to_DInt() + public void Typo_dataType_rejects_the_tag() { - TwinCATEquipmentTagParser.TryParse("{\"symbolPath\":\"MAIN.x\",\"dataType\":\"DIntt\"}", out var def) + TwinCATEquipmentTagParser.TryParse("{\"symbolPath\":\"MAIN.x\",\"dataType\":\"DIntt\"}", out _) + .ShouldBeFalse(); + } + + [Fact] + public void Valid_dataType_still_parses() + { + TwinCATEquipmentTagParser.TryParse("{\"symbolPath\":\"MAIN.x\",\"dataType\":\"DInt\"}", out var def) .ShouldBeTrue(); def.DataType.ShouldBe(TwinCATDataType.DInt); }