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. diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs index 97b50e60..dfdaec5f 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawBrowseCommitMapper.cs @@ -57,7 +57,7 @@ public static class RawBrowseCommitMapper IReadOnlyDictionary? 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 /// (group, edgeNode, device?, metric) tuple — so it is built from the browse session's stated /// instead. See . /// + /// + /// MTConnect binds by the driver-AGNOSTIC type, so it is forwarded from + /// rather than from a stated native field (Gitea #560). Its + /// coercion type IS a — the very enum the browse leaf carries, produced + /// by the driver's own /probe inference — so there is no separate native spelling for the + /// browser to state and nothing to re-derive. Left unforwarded, the model's String 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 had just typed from the + /// identical inference. Same defect class as #536, one door along. + /// /// /// The owning device's driver type. /// The leaf's driver-side full reference to write into the address field. /// The leaf's structured address, when the driver binds by a tuple — /// see . Ignored by every single-reference driver. + /// The leaf's 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. /// The serialised driver-typed TagConfig JSON. public static string BuildTagConfig( string driverType, string fullName, - IReadOnlyDictionary? addressFields = null) + IReadOnlyDictionary? 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 /// private static TEnum NativeType(IReadOnlyDictionary? 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(raw, ignoreCase: true, out var parsed) + => ParseNativeType(TryGetField(addressFields, "dataType"), fallback); + + /// + /// Parses one stated type NAME into a driver's data-type enum, keeping + /// for anything unusable — the rule applies to a stated address + /// field, split out so a type that arrives by another route (MTConnect's leaf-level + /// , Gitea #560) is judged by exactly the same one. + /// + /// The data-type enum the driver's tag config carries. + /// The stated type name, or null/blank. + /// The default to keep when the name is absent or unusable. + /// The parsed type, or . + /// + /// See for why an unparseable name keeps the default rather than + /// failing the commit, and why a numeric string is refused before parsing at all. + /// + private static TEnum ParseNativeType(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(name, ignoreCase: true, out var parsed) && Enum.IsDefined(parsed) ? parsed : fallback; diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawBrowseCommitMapperTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawBrowseCommitMapperTests.cs index 7d858302..bdc5af13 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawBrowseCommitMapperTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawBrowseCommitMapperTests.cs @@ -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) ---------------- + + /// + /// A browse-committed MTConnect tag's OPC UA node type and its TagConfig coercion type + /// must BOTH come from the leaf's probe-inferred — the same defect + /// class as #536 arriving through a different door. + /// + /// 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: + /// MapLeaf typed the node Double from the inference while BuildTagConfig + /// stamped the model's String 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. + /// + /// + /// The the Agent's /probe inference reported. + /// The OPC UA built-in name the raw tag row must carry. + [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().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(inferred)); + model.Validate().ShouldBeNull(); + } + + /// + /// The inferred type is forwarded through BuildTagConfig itself, not patched on by the + /// caller — the same seam AB CIP / TwinCAT / FOCAS forward their browsed type through. + /// + [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().ShouldBe("dev1_pos"); + o["dataType"]!.GetValue().ShouldBe("Float64"); + } + + /// + /// The parity control for the theory above: a leaf the browser reported NO type for keeps the + /// model's own String default — the pre-fix behaviour, and the right answer for a + /// weakly-typed Agent. A "fix" that emitted a blank or absent dataType would satisfy + /// "not String by default" while leaving the blob unauthorable, so this is what stops the + /// forward being implemented as a deletion. + /// + [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().ShouldBe("String"); + MTConnectTagConfigModel.FromJson(row.Tag.TagConfig).Validate().ShouldBeNull(); + } + + /// + /// A type name 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: Enum.TryParse accepts "8" for any enum and hands back + /// whatever member sits at that ordinal. + /// + /// The unusable type name the browse leaf stated. + [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().ShouldBe("String"); + } + // ---- BuildTagConfig: MQTT (the address is a DESCRIPTOR, not a single reference string) -------- /// The Sparkplug address a browse session states for a metric under a device.