Browse-committed MTConnect tags coerce to String at runtime while their nodes are numerically typed (RawBrowseCommitMapper drops the browsed native type — #536 class) #560

New Issue

2026-08-10T07:22:14-04:00

dohertj2 commented

2026-08-10 07:22:14 -04:00

Found by the 2026-08-10 review-coverage round — recorded as Driver.MTConnect-003 (High, Correctness) in code-reviews/Driver.MTConnect/findings.md, where the full analysis lives. Filed here because the fix belongs in ZB.MOM.WW.OtOpcUa.AdminUI, outside the reviewed driver modules.

Defect

Every browse-committed MTConnect tag is coerced to String at runtime while its OPC UA node is typed from the same probe inference — a browsed SAMPLE publishes text into a Double/Int64 node.

RawBrowseCommitMapper.MapLeaf (src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs:256) does two things with one browsed leaf:

sets the raw tag row's OPC UA DataType from the leaf's inferred DriverDataType (MapDataType: Float64 → Double, Int64 → Int64); and
calls BuildTagConfig, whose MTConnect branch is new MTConnectTagConfigModel { FullName = address }.ToJson().

MTConnectTagConfigModel.DataType defaults to DriverDataType.String and ToJson() unconditionally writes dataType, so the committed blob is {"fullName":"dev1_pos","dataType":"String"}. That is a non-blank authored type, so it wins the driver's documented coercion precedence and step 3 — the Agent's own /probe declaration via MTConnectDataTypeInference — is never reached for a browse-committed tag.

This is the #536 defect class: the AbCip, TwinCAT and FOCAS branches of BuildTagConfig forward the browsed native type via NativeType(addressFields, …) for exactly this reason. MTConnect got the typed-model branch without that forwarding.

Why High: the address picker is the primary authoring path for this driver; the mistyping is silent (the blob is perfectly valid, so no authoring warning and no deploy-gate rejection); and the resulting node advertises a numeric type it will never carry.

Fix direction

In RawBrowseCommitMapper, forward the browsed leaf's DriverDataType into the MTConnect branch as the other three typed-model branches already do — MapLeaf already holds driverDataType; either thread it into BuildTagConfig alongside addressFields, or supply it through addressFields under the dataType key NativeType already reads.

Test guidance from the review: add the MTConnect case to whatever pins the #536 behaviour for AbCip/TwinCAT/FOCAS, and assert the pair — the tag row's OPC UA type and the blob's coercion type agree — rather than either alone: asserting only the blob would pass against a mapper that had stopped setting the row's type.

Already corrected alongside (merged to master)

The driver-side comments that concealed this — MTConnectDriver.BuildBinding's claim that the /probe step "is what makes a browse-committed tag work", MTConnectDataTypeInference's remark naming call sites that don't exist, and docs/drivers/MTConnect.md — were rewritten at HEAD to state the live defect and point at the finding (Driver.MTConnect-008 / Driver.MTConnect.Contracts-001). On close, also update the coercion-precedence warning block in docs/drivers/MTConnect.md and the finding's Status/Resolution in code-reviews/Driver.MTConnect/findings.md (then python code-reviews/regen-readme.py).

**Found by the 2026-08-10 review-coverage round** — recorded as `Driver.MTConnect-003` (High, Correctness) in `code-reviews/Driver.MTConnect/findings.md`, where the full analysis lives. Filed here because the fix belongs in `ZB.MOM.WW.OtOpcUa.AdminUI`, outside the reviewed driver modules. ## Defect **Every browse-committed MTConnect tag is coerced to `String` at runtime while its OPC UA node is typed from the same probe inference** — a browsed `SAMPLE` publishes text into a `Double`/`Int64` node. `RawBrowseCommitMapper.MapLeaf` (`src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs:256`) does two things with one browsed leaf: - sets the raw tag row's OPC UA `DataType` from the leaf's inferred `DriverDataType` (`MapDataType`: `Float64` → `Double`, `Int64` → `Int64`); and - calls `BuildTagConfig`, whose MTConnect branch is `new MTConnectTagConfigModel { FullName = address }.ToJson()`. `MTConnectTagConfigModel.DataType` defaults to `DriverDataType.String` and `ToJson()` **unconditionally** writes `dataType`, so the committed blob is `{"fullName":"dev1_pos","dataType":"String"}`. That is a non-blank *authored* type, so it wins the driver's documented coercion precedence and step 3 — the Agent's own `/probe` declaration via `MTConnectDataTypeInference` — is **never reached for a browse-committed tag**. This is the #536 defect class: the AbCip, TwinCAT and FOCAS branches of `BuildTagConfig` forward the browsed native type via `NativeType(addressFields, …)` for exactly this reason. MTConnect got the typed-model branch **without** that forwarding. **Why High:** the address picker is the *primary* authoring path for this driver; the mistyping is silent (the blob is perfectly valid, so no authoring warning and no deploy-gate rejection); and the resulting node advertises a numeric type it will never carry. ## Fix direction In `RawBrowseCommitMapper`, forward the browsed leaf's `DriverDataType` into the MTConnect branch as the other three typed-model branches already do — `MapLeaf` already holds `driverDataType`; either thread it into `BuildTagConfig` alongside `addressFields`, or supply it through `addressFields` under the `dataType` key `NativeType` already reads. **Test guidance from the review:** add the MTConnect case to whatever pins the #536 behaviour for AbCip/TwinCAT/FOCAS, and assert the **pair** — the tag row's OPC UA type and the blob's coercion type agree — rather than either alone: asserting only the blob would pass against a mapper that had stopped setting the row's type. ## Already corrected alongside (merged to master) The driver-side comments that concealed this — `MTConnectDriver.BuildBinding`'s claim that the `/probe` step "is what makes a browse-committed tag work", `MTConnectDataTypeInference`'s remark naming call sites that don't exist, and `docs/drivers/MTConnect.md` — were rewritten at HEAD to state the live defect and point at the finding (`Driver.MTConnect-008` / `Driver.MTConnect.Contracts-001`). On close, also update the coercion-precedence warning block in `docs/drivers/MTConnect.md` and the finding's Status/Resolution in `code-reviews/Driver.MTConnect/findings.md` (then `python code-reviews/regen-readme.py`).

dohertj2 commented

2026-08-10 07:38:45 -04:00

Fixed — merged to master as 5226f7fd (fix commits 64be41d8 + 1c67d019).

The fix, in RawBrowseCommitMapper as this issue specified: MapLeaf threads its driverDataType into BuildTagConfig, and the MTConnect branch now emits DataType = NativeType(addressFields, ParseNativeType(driverDataType, DriverDataType.String)). A threaded parameter rather than a fabricated addressFields entry, deliberately: MTConnect's coercion type IS the driver-agnostic DriverDataType the browse leaf already carries (the driver never stamps NativeDataType, so NativeTypeFields emits nothing for it), and inventing an address field would contradict NativeType's documented "the type is READ, never derived" contract. A stated native dataType field still wins where a browser supplies one, so the branch reads exactly like its AbCip/TwinCAT/FOCAS siblings — only the fallback differs (the leaf's inference instead of the fixed model default). The numeric-string guard and unparseable-name fallback were extracted into a shared ParseNativeType so both routes are judged by one rule.

TDD, red first: the new pair theory failed against unfixed code with dataType should be "Float64" but was "String" (same for Int64/Boolean), while the String-leaf case and the no-type-stated control passed — proving the fix changes exactly the mistyped cases. Per the finding's test guidance the assertions cover the pair (the committed row's OPC UA type AND the blob's coercion type agree with the browsed inference), not either alone.

Verified on the merged tree: AdminUI.Tests 1,466 → 1,474 green (+8: 4-case pair theory, BuildTagConfig forwarding test, String-default parity control, 2-case unusable-name fallback theory); Driver.MTConnect.Tests 538 green; full solution build clean.

Docs followed the fix (they had been rewritten during the review round to document the defect as live at HEAD): docs/drivers/MTConnect.md's coercion-precedence warning block, MTConnectDriver.BuildBinding's remark, and the MTConnectDataTypeInference/MTConnectDriverOptions remarks now describe the fixed behaviour; code-reviews/Driver.MTConnect/findings.md finding Driver.MTConnect-003 is Resolved (module open count 5 → 4, README regenerated).

**Fixed** — merged to master as `5226f7fd` (fix commits `64be41d8` + `1c67d019`). **The fix, in `RawBrowseCommitMapper` as this issue specified:** `MapLeaf` threads its `driverDataType` into `BuildTagConfig`, and the MTConnect branch now emits `DataType = NativeType(addressFields, ParseNativeType(driverDataType, DriverDataType.String))`. A **threaded parameter** rather than a fabricated `addressFields` entry, deliberately: MTConnect's coercion type IS the driver-agnostic `DriverDataType` the browse leaf already carries (the driver never stamps `NativeDataType`, so `NativeTypeFields` emits nothing for it), and inventing an address field would contradict `NativeType`'s documented "the type is READ, never derived" contract. A stated native `dataType` field still wins where a browser supplies one, so the branch reads exactly like its AbCip/TwinCAT/FOCAS siblings — only the *fallback* differs (the leaf's inference instead of the fixed model default). The numeric-string guard and unparseable-name fallback were extracted into a shared `ParseNativeType` so both routes are judged by one rule. **TDD, red first:** the new pair theory failed against unfixed code with `dataType should be "Float64" but was "String"` (same for Int64/Boolean), while the String-leaf case and the no-type-stated control passed — proving the fix changes exactly the mistyped cases. Per the finding's test guidance the assertions cover the **pair** (the committed row's OPC UA type AND the blob's coercion type agree with the browsed inference), not either alone. **Verified on the merged tree:** AdminUI.Tests 1,466 → **1,474** green (+8: 4-case pair theory, `BuildTagConfig` forwarding test, String-default parity control, 2-case unusable-name fallback theory); Driver.MTConnect.Tests **538** green; full solution build clean. **Docs followed the fix** (they had been rewritten during the review round to document the defect as live at HEAD): `docs/drivers/MTConnect.md`'s coercion-precedence warning block, `MTConnectDriver.BuildBinding`'s remark, and the `MTConnectDataTypeInference`/`MTConnectDriverOptions` remarks now describe the fixed behaviour; `code-reviews/Driver.MTConnect/findings.md` finding `Driver.MTConnect-003` is **Resolved** (module open count 5 → 4, README regenerated).

dohertj2 closed this issue

2026-08-10 07:38:45 -04:00

dohertj2 referenced this issue from a commit

2026-08-10 08:50:49 -04:00

docs(reviews): cross-reference Gitea #560 in Driver.MTConnect-003

dohertj2 referenced this issue from a commit

2026-08-10 08:50:49 -04:00

fix(adminui): forward the browsed MTConnect leaf's inferred type into the committed TagConfig (#560)

dohertj2 referenced this issue from a commit

2026-08-10 08:50:49 -04:00

docs(mtconnect): describe the fixed browse-commit coercion type (#560)

dohertj2 referenced this issue from a commit