From 1c67d019e5d1f217dcf6b1c3296e6343a93223d4 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 07:36:45 -0400 Subject: [PATCH] docs(mtconnect): describe the fixed browse-commit coercion type (#560) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comments and docs that described Driver.MTConnect-003 as live at HEAD would have been wrong the moment 64be41d8 landed: - code-reviews/Driver.MTConnect/findings.md — Driver.MTConnect-003 Resolved, with the fixing commit; module open-finding count 5 -> 4. - code-reviews/Driver.MTConnect.Contracts/findings.md — the one clause inside Contracts-001's prose that asserted 003 was Open. - docs/drivers/MTConnect.md — the coercion-precedence warning now states that a browse-committed tag resolves at step 1 CARRYING the step-3 answer, and that step 3 remains the rescue for a hand-authored / CSV / API blob with no type. - MTConnectDriver.BuildBinding, MTConnectDataTypeInference and MTConnectDriverOptions.RawTags remarks — same correction, each keeping a short historical note of what the defect was so a reader meeting the old blobs still understands them. Comment-only in src; the driver suite (538) and AdminUI suite (1474) re-run green. --- .../Driver.MTConnect.Contracts/findings.md | 2 +- code-reviews/Driver.MTConnect/findings.md | 17 ++++++++---- docs/drivers/MTConnect.md | 26 ++++++++++++------- .../MTConnectDataTypeInference.cs | 16 +++++++----- .../MTConnectDriverOptions.cs | 11 +++++--- .../MTConnectDriver.cs | 23 ++++++++-------- 6 files changed, 57 insertions(+), 38 deletions(-) diff --git a/code-reviews/Driver.MTConnect.Contracts/findings.md b/code-reviews/Driver.MTConnect.Contracts/findings.md index 3fbf237c..aa19bc02 100644 --- a/code-reviews/Driver.MTConnect.Contracts/findings.md +++ b/code-reviews/Driver.MTConnect.Contracts/findings.md @@ -61,7 +61,7 @@ false premise concealed a live defect rather than merely misinforming a reader. The second is what makes this worth recording rather than a tidy-up: a reader auditing the browse path would have taken that remark as evidence the case was handled. The underlying defect is -recorded as `Driver.MTConnect-003` (Open, fix belongs in `RawBrowseCommitMapper`). +recorded as `Driver.MTConnect-003` (Resolved `64be41d8`, 2026-08-10, in `RawBrowseCommitMapper`). **Recommendation:** Correct both remarks to describe the callers that exist, and — rather than deleting the precedence rule, which is still correct for hand-authored, CSV-imported and API-authored diff --git a/code-reviews/Driver.MTConnect/findings.md b/code-reviews/Driver.MTConnect/findings.md index 3847ca8f..2877ed1a 100644 --- a/code-reviews/Driver.MTConnect/findings.md +++ b/code-reviews/Driver.MTConnect/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-08-10 | | Commit reviewed | `b990adae` | | Status | Complete | -| Open findings | 5 | +| Open findings | 4 | ## Checklist coverage @@ -110,7 +110,7 @@ requested `/probe`, expected `/VMC-1/probe`) and the control | Severity | High | | Category | Correctness & logic bugs | | Location | `src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs:256` (cause) · `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs:3286` (`BuildBinding` precedence) | -| Status | Open | +| Status | Resolved | **Description:** **Every browse-committed MTConnect tag is coerced to `String` at runtime while its OPC UA node is typed from the probe inference**, so a browsed `SAMPLE` publishes text into a @@ -150,9 +150,16 @@ behaviour for AbCip/TwinCAT/FOCAS, and assert the **pair** — that the tag row' 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. -**Resolution:** _(open — the fix belongs in `ZB.MOM.WW.OtOpcUa.AdminUI`, outside this review's -modules; tracked as Gitea **#560**. The stale premises this defect invalidated inside these two -modules are corrected under `Driver.MTConnect-008` and `Driver.MTConnect.Contracts-001`.)_ +**Resolution:** Fixed `64be41d8`, 2026-08-10 (Gitea **#560**) — cross-module, in +`ZB.MOM.WW.OtOpcUa.AdminUI`: `RawBrowseCommitMapper.MapLeaf` now threads the browsed leaf's +`driverDataType` into `BuildTagConfig`, whose MTConnect branch uses it as the `NativeType` fallback, +so the committed blob's coercion type and the raw tag row's OPC UA type both come from the one probe +inference — the same forwarding the AbCip / TwinCAT / FOCAS branches already did for #536. A leaf +inferred (or reported) as `String` still commits `"dataType":"String"`, and an unusable name still +falls back to it through the shared numeric-string / unparseable guards. Pinned by +`RawBrowseCommitMapperTests` asserting the PAIR plus that `String` control; the stale premises this +defect left inside these two modules are corrected under `Driver.MTConnect-008` and +`Driver.MTConnect.Contracts-001`. ### Driver.MTConnect-004 diff --git a/docs/drivers/MTConnect.md b/docs/drivers/MTConnect.md index 489448ed..a93825bb 100644 --- a/docs/drivers/MTConnect.md +++ b/docs/drivers/MTConnect.md @@ -194,15 +194,23 @@ into) is resolved in this order, first non-null wins: (category/type/units/representation → `DriverDataType`). 4. `DriverDataType.String` — the coercion that cannot fail. -> ⚠️ **Step 3 does not currently rescue a browse-committed tag.** `RawBrowseCommitMapper`'s -> MTConnect branch emits a whole `MTConnectTagConfigModel`, whose `ToJson()` always writes -> `dataType` from a field defaulting to `String` — a non-blank authored type, so step 1 wins -> and step 3 is never consulted. The raw tag's OPC UA `DataType` meanwhile *is* taken from the -> browsed leaf's inferred type (`RawBrowseCommitMapper.MapLeaf` → `MapDataType`), so a browsed -> `SAMPLE` lands as a `Double` node whose driver coerces the observation to text. Fix belongs in -> the commit mapper (forward the leaf's `DriverDataType` into the model, as the AbCip / TwinCAT / -> FOCAS branches already do). Recorded as `Driver.MTConnect-003` in -> [`code-reviews/Driver.MTConnect/findings.md`](../../code-reviews/Driver.MTConnect/findings.md). +> **A browse-committed tag resolves at step 1, carrying the step-3 answer.** `RawBrowseCommitMapper`'s +> MTConnect branch emits a whole `MTConnectTagConfigModel`, whose `ToJson()` always writes `dataType` +> — so a committed blob always states a non-blank type and step 1 always wins. Since Gitea **#560** +> that stated type is the **browsed leaf's own inferred `DriverDataType`**, the very value step 3 +> would compute, forwarded by `MapLeaf` into `BuildTagConfig` exactly as the AbCip / TwinCAT / FOCAS +> branches forward their browsed native type (Gitea #536). It therefore agrees by construction with +> the raw tag row's OPC UA `DataType`, which `MapLeaf` → `MapDataType` takes from the same inference. +> +> Before #560 the branch stamped the model's `String` default instead, so a browsed `SAMPLE` landed +> as a `Double` node whose driver coerced the observation to text — recorded as `Driver.MTConnect-003` +> in [`code-reviews/Driver.MTConnect/findings.md`](../../code-reviews/Driver.MTConnect/findings.md). +> A leaf the browser reports **no** type for still commits `String`, and a leaf legitimately inferred +> as `String` (an `EVENT` item) commits `String` too — in both cases the type the driver would have +> reached anyway. +> +> **Step 3 remains the rescue for a blob that states no type at all** — hand-authored, CSV-imported, +> or API-posted `RawTags[]` entries carrying only an id, which is the shape it was written for. ### Quality mapping diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDataTypeInference.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDataTypeInference.cs index d8e16053..c932693e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDataTypeInference.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDataTypeInference.cs @@ -37,13 +37,15 @@ public readonly record struct MTConnectInferredType(DriverDataType DataType, boo /// blob declares no type). /// /// -/// ⚠️ Two call sites this remark used to name do not exist. The universal browser's -/// commit path (RawBrowseCommitMapper) never calls this — it re-maps the browsed leaf's -/// already-inferred DriverDataType onto the raw tag's OPC UA type but writes a -/// TagConfig whose dataType is the model default — and the AdminUI typed editor -/// (MTConnectTagConfigModel) deliberately does not recompute the inference at all. The -/// first of those is a live defect, not merely a stale comment: see -/// code-reviews/Driver.MTConnect/findings.md (Driver.MTConnect-003). +/// Two further call sites this remark used to name do not exist, and should not. The +/// universal browser's commit path (RawBrowseCommitMapper) never calls this: it +/// FORWARDS the browsed leaf's already-inferred DriverDataType — onto the raw tag's OPC +/// UA type, and (since Gitea #560) into the committed TagConfig's dataType — so +/// both come from this rule without re-running it. Before #560 the blob carried the model's +/// String default instead and disagreed with the node beside it +/// (Driver.MTConnect-003). The AdminUI typed editor (MTConnectTagConfigModel) +/// deliberately does not recompute the inference at all — it re-serialises whatever the +/// operator picked or the commit stamped. /// /// /// MTConnect is weakly typed on the wire — every observation is text, and the standard diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDriverOptions.cs index ef6a8edb..7657e2a0 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDriverOptions.cs @@ -87,10 +87,13 @@ public sealed class MTConnectDriverOptions /// entry naming the same DataItem id; failing that, from the Agent's /// own /probe declaration (); failing /// that, — the coercion that cannot fail. - /// ⚠️ The /probe step is not reached by a browse-committed tag at HEAD: - /// the AdminUI's commit mapper writes a full typed blob whose dataType defaults - /// to String, which the first step then honours. See - /// code-reviews/Driver.MTConnect/findings.md (Driver.MTConnect-003). + /// The /probe step is not reached by a browse-committed tag — the AdminUI's + /// commit mapper writes a full typed blob, so the first step always answers — but + /// since Gitea #560 the type it writes IS the browsed leaf's inference, so the answer + /// is the same one. It previously wrote the model's String default and silently + /// mistyped every browsed tag (code-reviews/Driver.MTConnect/findings.md, + /// Driver.MTConnect-003). The step still serves a hand-authored / CSV / API blob that + /// states no type. /// A tag in only is reachable by its DataItem id /// (the driver CLI's authoring surface, and every pre-v3 test) and still contributes /// its coercion type to the index. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs index 7734a187..ced2d4da 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect/MTConnectDriver.cs @@ -3227,19 +3227,18 @@ public sealed class MTConnectDriver /// data item, else . /// /// - /// ⚠️ The third step does NOT currently rescue a browse-committed tag, and the - /// AdminUI is where that has to be fixed. It was written on the premise that - /// RawBrowseCommitMapper writes only the address into the blob — true when this - /// was authored, false at HEAD: the MTConnect branch of - /// RawBrowseCommitMapper.BuildTagConfig now emits a whole + /// A browse-committed tag never reaches the third step, and no longer needs to. + /// The MTConnect branch of RawBrowseCommitMapper.BuildTagConfig emits a whole /// MTConnectTagConfigModel, whose ToJson unconditionally writes - /// "dataType" from a field defaulting to . That - /// is a non-blank authored type, so step 1 wins and this step is never reached — every - /// browse-committed tag coerces to text while its raw-tag node was typed Double / - /// Int64 from the SAME probe inference (RawBrowseCommitMapper.MapLeaf). - /// The step is kept because it is still the correct rule and still serves a blob authored - /// by hand, by CSV or by API with no type; see - /// code-reviews/Driver.MTConnect/findings.md (Driver.MTConnect-003). + /// "dataType" — always a non-blank authored type, so step 1 always wins. Since + /// Gitea #560 the value it writes is the browsed leaf's own inferred + /// , i.e. what this step would have computed, so it agrees + /// with the raw-tag node's OPC UA type (taken from the same inference by + /// RawBrowseCommitMapper.MapLeaf). Historically it wrote the model's + /// default instead, and every browse-committed tag + /// coerced to text inside a Double/Int64 node — Driver.MTConnect-003. + /// The step remains the rescue for a blob authored by hand, by CSV or by API that states + /// no type at all. /// /// /// The options document to derive from.