merge: browse-committed MTConnect tags carry the browsed inferred type (Gitea #560 closed; Driver.MTConnect-003 Resolved)

This commit is contained in:
Joseph Doherty
2026-08-10 07:37:23 -04:00
8 changed files with 207 additions and 44 deletions
@@ -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
+12 -5
View File
@@ -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
+17 -9
View File
@@ -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
@@ -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>
@@ -57,7 +57,7 @@ public static class RawBrowseCommitMapper
IReadOnlyDictionary<string, string>? addressFields = null)
{
var dataType = MapDataType(driverDataType) ?? defaultDataType;
var tagConfig = BuildTagConfig(driverType, fullName, addressFields);
var tagConfig = BuildTagConfig(driverType, fullName, addressFields, driverDataType);
var name = ToRawSegment(browseName);
var mirrored = createGroups && folderPath is { Count: > 0 }
? string.Join(RawPaths.Separator, folderPath)
@@ -212,16 +212,30 @@ public static class RawBrowseCommitMapper
/// <c>(group, edgeNode, device?, metric)</c> tuple — so it is built from the browse session's stated
/// <paramref name="addressFields"/> instead. See <see cref="BuildMqttTagConfig"/>.
/// </para>
/// <para>
/// <b>MTConnect binds by the driver-AGNOSTIC type, so it is forwarded from
/// <paramref name="driverDataType"/> rather than from a stated native field</b> (Gitea #560). Its
/// coercion type IS a <see cref="DriverDataType"/> — the very enum the browse leaf carries, produced
/// by the driver's own <c>/probe</c> inference — so there is no separate native spelling for the
/// browser to state and nothing to re-derive. Left unforwarded, the model's <c>String</c> default was
/// written as a non-blank authored type, which OUT-RANKS that same inference at the driver: every
/// browsed SAMPLE published text into the numeric node <see cref="MapLeaf"/> had just typed from the
/// identical inference. Same defect class as #536, one door along.
/// </para>
/// </summary>
/// <param name="driverType">The owning device's driver type.</param>
/// <param name="fullName">The leaf's driver-side full reference to write into the address field.</param>
/// <param name="addressFields">The leaf's structured address, when the driver binds by a tuple —
/// see <see cref="BuildMqttTagConfig"/>. Ignored by every single-reference driver.</param>
/// <param name="driverDataType">The leaf's <see cref="DriverDataType"/> NAME, for a driver whose tag
/// config carries that agnostic enum rather than one of its own (MTConnect); null when the browser
/// reported no type, which keeps the driver model's default.</param>
/// <returns>The serialised driver-typed <c>TagConfig</c> JSON.</returns>
public static string BuildTagConfig(
string driverType,
string fullName,
IReadOnlyDictionary<string, string>? addressFields = null)
IReadOnlyDictionary<string, string>? addressFields = null,
string? driverDataType = null)
{
var address = fullName ?? "";
if (Is(driverType, DriverTypeNames.Mqtt))
@@ -253,8 +267,17 @@ public static class RawBrowseCommitMapper
// MTConnect: the DataItem id. The driver also accepts "address"/"dataItemId", but the typed
// editor's canonical spelling is fullName — committing under the generic key below would open
// in that editor with an EMPTY id field and blank it on save.
//
// The coercion type is the leaf's own inferred DriverDataType (#560) — see the remarks above.
// A stated native "dataType" address field still wins where a browser supplies one, so this
// branch reads exactly like its AB CIP / TwinCAT / FOCAS siblings; the difference is only that
// MTConnect's fallback is the leaf's inference rather than a fixed model default.
if (Is(driverType, DriverTypeNames.MTConnect))
return new MTConnectTagConfigModel { FullName = address }.ToJson();
return new MTConnectTagConfigModel
{
FullName = address,
DataType = NativeType(addressFields, ParseNativeType(driverDataType, DriverDataType.String)),
}.ToJson();
if (Is(driverType, DriverTypeNames.Galaxy))
return WriteSingleKey("attributeRef", address);
// Sql: a browsed leaf is a COLUMN, and a column name alone cannot address a tag — SqlTagConfigModel
@@ -451,9 +474,27 @@ public static class RawBrowseCommitMapper
/// </remarks>
private static TEnum NativeType<TEnum>(IReadOnlyDictionary<string, string>? addressFields, TEnum fallback)
where TEnum : struct, Enum
=> TryGetField(addressFields, "dataType") is { Length: > 0 } raw
&& !char.IsAsciiDigit(raw[0]) && raw[0] is not ('-' or '+')
&& Enum.TryParse<TEnum>(raw, ignoreCase: true, out var parsed)
=> ParseNativeType(TryGetField(addressFields, "dataType"), fallback);
/// <summary>
/// Parses one stated type NAME into a driver's data-type enum, keeping <paramref name="fallback"/>
/// for anything unusable — the rule <see cref="NativeType{TEnum}"/> applies to a stated address
/// field, split out so a type that arrives by another route (MTConnect's leaf-level
/// <see cref="DriverDataType"/>, Gitea #560) is judged by exactly the same one.
/// </summary>
/// <typeparam name="TEnum">The data-type enum the driver's tag config carries.</typeparam>
/// <param name="raw">The stated type name, or null/blank.</param>
/// <param name="fallback">The default to keep when the name is absent or unusable.</param>
/// <returns>The parsed type, or <paramref name="fallback"/>.</returns>
/// <remarks>
/// See <see cref="NativeType{TEnum}"/> for why an unparseable name keeps the default rather than
/// failing the commit, and why a numeric string is refused before parsing at all.
/// </remarks>
private static TEnum ParseNativeType<TEnum>(string? raw, TEnum fallback)
where TEnum : struct, Enum
=> raw?.Trim() is { Length: > 0 } name
&& !char.IsAsciiDigit(name[0]) && name[0] is not ('-' or '+')
&& Enum.TryParse<TEnum>(name, ignoreCase: true, out var parsed)
&& Enum.IsDefined(parsed)
? parsed
: fallback;
@@ -248,6 +248,109 @@ public sealed class RawBrowseCommitMapperTests
JsonNode.Parse(json)!.AsObject().ContainsKey("dataType").ShouldBeFalse();
}
// ---- MTConnect: the commit must not retype the node it just typed (Gitea #560) ----------------
/// <summary>
/// A browse-committed MTConnect tag's OPC UA node type and its <c>TagConfig</c> coercion type
/// must BOTH come from the leaf's probe-inferred <see cref="DriverDataType"/> — the same defect
/// class as #536 arriving through a different door.
/// <para>
/// The pair is asserted together on purpose. The blob alone would pass against a mapper that
/// stopped typing the row, and the row alone was already right while the defect was live:
/// <c>MapLeaf</c> typed the node <c>Double</c> from the inference while <c>BuildTagConfig</c>
/// stamped the model's <c>String</c> default into the blob, and a non-blank authored type wins
/// the driver's coercion precedence — so every browsed SAMPLE published text into a numeric
/// node. Only the DISAGREEMENT is the bug, so only the agreement can pin the fix.
/// </para>
/// </summary>
/// <param name="inferred">The <see cref="DriverDataType"/> the Agent's <c>/probe</c> inference reported.</param>
/// <param name="expectedNodeType">The OPC UA built-in name the raw tag row must carry.</param>
[Theory]
[InlineData("Float64", "Double")]
[InlineData("Int64", "Int64")]
[InlineData("Boolean", "Boolean")]
[InlineData("String", "String")] // an EVENT item — String is a real inference here, not a default
public void MapLeaf_types_an_MTConnect_node_and_its_coercion_alike(string inferred, string expectedNodeType)
{
var row = RawBrowseCommitMapper.MapLeaf(
driverType: DriverTypeNames.MTConnect,
fullName: "dev1_pos",
browseName: "position",
driverDataType: inferred,
defaultDataType: "Double",
groupPrefix: null,
folderPath: null,
createGroups: false);
row.Tag.DataType.ShouldBe(expectedNodeType);
JsonNode.Parse(row.Tag.TagConfig)!.AsObject()["dataType"]!.GetValue<string>().ShouldBe(inferred);
// …and the blob stays one the typed editor and the driver both accept, so the commit is not
// merely self-consistent but authorable.
var model = MTConnectTagConfigModel.FromJson(row.Tag.TagConfig);
model.FullName.ShouldBe("dev1_pos");
model.DataType.ShouldBe(Enum.Parse<DriverDataType>(inferred));
model.Validate().ShouldBeNull();
}
/// <summary>
/// The inferred type is forwarded through <c>BuildTagConfig</c> itself, not patched on by the
/// caller — the same seam AB CIP / TwinCAT / FOCAS forward their browsed type through.
/// </summary>
[Fact]
public void BuildTagConfig_stamps_the_browsed_MTConnect_type()
{
var json = RawBrowseCommitMapper.BuildTagConfig(
DriverTypeNames.MTConnect, "dev1_pos", addressFields: null, driverDataType: "Float64");
var o = JsonNode.Parse(json)!.AsObject();
o["fullName"]!.GetValue<string>().ShouldBe("dev1_pos");
o["dataType"]!.GetValue<string>().ShouldBe("Float64");
}
/// <summary>
/// The parity control for the theory above: a leaf the browser reported NO type for keeps the
/// model's own <c>String</c> default — the pre-fix behaviour, and the right answer for a
/// weakly-typed Agent. A "fix" that emitted a blank or absent <c>dataType</c> would satisfy
/// "not String by default" while leaving the blob unauthorable, so this is what stops the
/// forward being implemented as a deletion.
/// </summary>
[Fact]
public void MapLeaf_keeps_the_MTConnect_String_default_when_the_browser_states_no_type()
{
var row = RawBrowseCommitMapper.MapLeaf(
driverType: DriverTypeNames.MTConnect,
fullName: "dev1_exec",
browseName: "execution",
driverDataType: null,
defaultDataType: "String",
groupPrefix: null,
folderPath: null,
createGroups: false);
JsonNode.Parse(row.Tag.TagConfig)!.AsObject()["dataType"]!.GetValue<string>().ShouldBe("String");
MTConnectTagConfigModel.FromJson(row.Tag.TagConfig).Validate().ShouldBeNull();
}
/// <summary>
/// A type name <see cref="DriverDataType"/> does not carry — an older browser, or a driver that
/// stamped something else — keeps the model default rather than failing the commit or writing a
/// name the runtime would reject. Mirrors the sibling drivers' fallback exactly, including the
/// numeric-string guard: <c>Enum.TryParse</c> accepts <c>"8"</c> for any enum and hands back
/// whatever member sits at that ordinal.
/// </summary>
/// <param name="stated">The unusable type name the browse leaf stated.</param>
[Theory]
[InlineData("NotATypeThisDriverKnows")]
[InlineData("8")]
public void BuildTagConfig_falls_back_to_String_for_an_unusable_MTConnect_type(string stated)
{
var json = RawBrowseCommitMapper.BuildTagConfig(
DriverTypeNames.MTConnect, "dev1_pos", addressFields: null, driverDataType: stated);
JsonNode.Parse(json)!.AsObject()["dataType"]!.GetValue<string>().ShouldBe("String");
}
// ---- BuildTagConfig: MQTT (the address is a DESCRIPTOR, not a single reference string) --------
/// <summary>The Sparkplug address a browse session states for a metric under a device.</summary>