docs(mtconnect): describe the fixed browse-commit coercion type (#560)
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.
This commit is contained in:
Joseph Doherty
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
+9
-7
@@ -37,13 +37,15 @@ public readonly record struct MTConnectInferredType(DriverDataType DataType, boo
|
||||
/// blob declares no type).
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// ⚠️ <b>Two call sites this remark used to name do not exist.</b> The universal browser's
|
||||
/// commit path (<c>RawBrowseCommitMapper</c>) never calls this — it re-maps the browsed leaf's
|
||||
/// already-inferred <c>DriverDataType</c> onto the raw tag's OPC UA type but writes a
|
||||
/// <c>TagConfig</c> whose <c>dataType</c> is the model default — and the AdminUI typed editor
|
||||
/// (<c>MTConnectTagConfigModel</c>) deliberately does not recompute the inference at all. The
|
||||
/// first of those is a live defect, not merely a stale comment: see
|
||||
/// <c>code-reviews/Driver.MTConnect/findings.md</c> (Driver.MTConnect-003).
|
||||
/// <b>Two further call sites this remark used to name do not exist, and should not.</b> The
|
||||
/// universal browser's commit path (<c>RawBrowseCommitMapper</c>) never calls this: it
|
||||
/// FORWARDS the browsed leaf's already-inferred <c>DriverDataType</c> — onto the raw tag's OPC
|
||||
/// UA type, and (since Gitea #560) into the committed <c>TagConfig</c>'s <c>dataType</c> — so
|
||||
/// both come from this rule without re-running it. Before #560 the blob carried the model's
|
||||
/// <c>String</c> default instead and disagreed with the node beside it
|
||||
/// (<c>Driver.MTConnect-003</c>). The AdminUI typed editor (<c>MTConnectTagConfigModel</c>)
|
||||
/// deliberately does not recompute the inference at all — it re-serialises whatever the
|
||||
/// operator picked or the commit stamped.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>MTConnect is weakly typed on the wire</b> — every observation is text, and the standard
|
||||
|
||||
@@ -87,10 +87,13 @@ public sealed class MTConnectDriverOptions
|
||||
/// <see cref="Tags"/> entry naming the same DataItem id; failing that, from the Agent's
|
||||
/// own <c>/probe</c> declaration (<see cref="MTConnectDataTypeInference"/>); failing
|
||||
/// that, <see cref="DriverDataType.String"/> — the coercion that cannot fail.
|
||||
/// ⚠️ The <c>/probe</c> step is <b>not</b> reached by a browse-committed tag at HEAD:
|
||||
/// the AdminUI's commit mapper writes a full typed blob whose <c>dataType</c> defaults
|
||||
/// to <c>String</c>, which the first step then honours. See
|
||||
/// <c>code-reviews/Driver.MTConnect/findings.md</c> (Driver.MTConnect-003).</item>
|
||||
/// The <c>/probe</c> 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 <c>String</c> default and silently
|
||||
/// mistyped every browsed tag (<c>code-reviews/Driver.MTConnect/findings.md</c>,
|
||||
/// Driver.MTConnect-003). The step still serves a hand-authored / CSV / API blob that
|
||||
/// states no type.</item>
|
||||
/// <item>A tag in <see cref="Tags"/> only is reachable by its DataItem <c>id</c>
|
||||
/// (the driver CLI's authoring surface, and every pre-v3 test) and still contributes
|
||||
/// its coercion type to the index.</item>
|
||||
|
||||
@@ -3227,19 +3227,18 @@ public sealed class MTConnectDriver
|
||||
/// data item, else <see cref="DriverDataType.String"/>.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// ⚠️ <b>The third step does NOT currently rescue a browse-committed tag, and the
|
||||
/// AdminUI is where that has to be fixed.</b> It was written on the premise that
|
||||
/// <c>RawBrowseCommitMapper</c> writes only the address into the blob — true when this
|
||||
/// was authored, false at HEAD: the MTConnect branch of
|
||||
/// <c>RawBrowseCommitMapper.BuildTagConfig</c> now emits a whole
|
||||
/// <b>A browse-committed tag never reaches the third step, and no longer needs to.</b>
|
||||
/// The MTConnect branch of <c>RawBrowseCommitMapper.BuildTagConfig</c> emits a whole
|
||||
/// <c>MTConnectTagConfigModel</c>, whose <c>ToJson</c> unconditionally writes
|
||||
/// <c>"dataType"</c> from a field defaulting to <see cref="DriverDataType.String"/>. 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 <c>Double</c> /
|
||||
/// <c>Int64</c> from the SAME probe inference (<c>RawBrowseCommitMapper.MapLeaf</c>).
|
||||
/// 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
|
||||
/// <c>code-reviews/Driver.MTConnect/findings.md</c> (Driver.MTConnect-003).
|
||||
/// <c>"dataType"</c> — 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
|
||||
/// <see cref="DriverDataType"/>, 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
|
||||
/// <c>RawBrowseCommitMapper.MapLeaf</c>). Historically it wrote the model's
|
||||
/// <see cref="DriverDataType.String"/> default instead, and every browse-committed tag
|
||||
/// coerced to text inside a <c>Double</c>/<c>Int64</c> node — <c>Driver.MTConnect-003</c>.
|
||||
/// The step remains the rescue for a blob authored by hand, by CSV or by API that states
|
||||
/// no type at all.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
/// <param name="options">The options document to derive from.</param>
|
||||
|
||||
Reference in New Issue
Block a user