diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipTagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipTagDefinitionFactory.cs index eaeaa4fb..77770c8c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipTagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipTagDefinitionFactory.cs @@ -189,16 +189,34 @@ public static class AbCipTagDefinitionFactory } /// - /// Deploy-time inspection (05/CONV-2): warns on a present-but-invalid dataType (silently - /// defaulted by the lenient runtime) and on a structurally unparseable TagConfig. Empty when clean - /// or not an equipment-tag object. Never throws. + /// The RawPath handed to during inspection — the mapper only copies it + /// onto and never validates it. /// + private const string InspectionRawPath = "(inspect)"; + + /// + /// Deploy-time inspection (05/CONV-2): warns for a blob would + /// reject — a structurally unparseable or non-object document, an absent / non-string / + /// blank tagPath, a present-but-invalid dataType (the runtime enum read is STRICT + /// since R2-11 Phase C, so a typo rejects the tag), or a malformed declared UDT member (see + /// ). Such a tag is skipped by the driver and reads + /// BadNodeIdUnknown forever. Empty when the mapper accepts the blob, when it is blank, or + /// when it is not an equipment-tag object. Never throws. + /// + /// + /// The verdict comes from , not from a mirror (Gitea #551) — see + /// ModbusTagDefinitionFactory.Inspect's remarks for the full argument. + /// /// The equipment tag's TagConfig JSON. /// The warnings; empty when clean. public static IReadOnlyList Inspect(string reference) { var warnings = new List(); if (string.IsNullOrWhiteSpace(reference) || reference[0] != '{') return warnings; + + // The accept gate: whatever the driver reads draws nothing. + if (FromTagConfig(reference, InspectionRawPath, out _)) return warnings; + try { using var doc = JsonDocument.Parse(reference); @@ -208,16 +226,129 @@ public static class AbCipTagDefinitionFactory warnings.Add("AbCip TagConfig root is not a JSON object — the tag will not resolve (BadNodeIdUnknown)."); return warnings; } - var w = TagConfigJson.DescribeInvalidEnum(root, "dataType"); - if (w is not null) warnings.Add(w); + + DescribeRejection(root, warnings); } catch (JsonException) { warnings.Add("AbCip TagConfig is not valid JSON — the tag will not resolve (BadNodeIdUnknown)."); + return warnings; } + + if (warnings.Count == 0) + { + warnings.Add( + "AbCip TagConfig was rejected by the driver's tag reader — the driver will skip this tag " + + "and it will read BadNodeIdUnknown."); + } + return warnings; } + /// + /// Explains why refused an object-rooted blob, walking its checks in the + /// same order. Runs only after the mapper has already refused, so it can never invent strictness. + /// + /// The TagConfig root object. + /// The list to append warnings to. + private static void DescribeRejection(JsonElement root, List warnings) + { + if (!root.TryGetProperty("tagPath", out var tagPathEl) || tagPathEl.ValueKind != JsonValueKind.String) + { + warnings.Add( + "AbCip TagConfig has no string 'tagPath' — AbCip is a symbolic driver and addresses by " + + "Logix tag path, so the driver will skip this tag and it will read BadNodeIdUnknown."); + } + else if (string.IsNullOrWhiteSpace(tagPathEl.GetString())) + { + warnings.Add("AbCip TagConfig has a blank 'tagPath' — the tag will not resolve (BadNodeIdUnknown)."); + } + + if (TagConfigJson.DescribeInvalidEnum(root, "dataType") is { } w) warnings.Add(w); + + if (DescribeInvalidMember(root) is { } memberWarning) warnings.Add(memberWarning); + } + + /// + /// The single authority on the declared-UDT-member rules, consulted by + /// and by the AdminUI's AbCipTagConfigModel.Validate alike. Describes the FIRST malformed + /// entry of the optional members array — a non-object entry, one with no usable name, + /// or one whose dataType is a present-but-invalid — or + /// when every member reads. + /// + /// The member rules are as strict as the top-level ones ( refuses the + /// WHOLE tag for one bad member), but no authoring surface checked them: the typed editor emits + /// only tagPath/dataType/writable and members rides the model's + /// preserved key bag untouched, so a hand-authored / CSV / API blob with a typo'd member + /// deployed clean and died at the driver — Gitea #551 item 3. + /// + /// + /// A members value that is absent or not a JSON array draws nothing: the mapper reads it + /// only when it is an array, and an array of zero usable members simply means "no member + /// fan-out". Saying otherwise here would make the gate stricter than the reader. + /// + /// + /// The TagConfig root object. + /// The warning text, or when the members read. + public static string? DescribeInvalidMember(JsonElement root) + { + if (!root.TryGetProperty("members", out var membersEl) || membersEl.ValueKind != JsonValueKind.Array) + return null; + + var index = 0; + foreach (var member in membersEl.EnumerateArray()) + { + var label = $"members[{index}]"; + index++; + + if (member.ValueKind != JsonValueKind.Object) + { + return $"AbCip {label} is not a JSON object — a declared UDT member needs a 'name' and may " + + "carry a 'dataType'; the WHOLE tag is rejected (BadNodeIdUnknown)."; + } + + if (!member.TryGetProperty("name", out var nameEl) + || nameEl.ValueKind != JsonValueKind.String + || string.IsNullOrWhiteSpace(nameEl.GetString())) + { + return $"AbCip {label} has no usable 'name' — a declared UDT member must name itself; the " + + "WHOLE tag is rejected (BadNodeIdUnknown)."; + } + + if (TagConfigJson.DescribeInvalidEnum(member, "dataType") is { } dataTypeWarning) + { + return $"AbCip {label}: {dataTypeWarning} — the WHOLE tag is rejected (BadNodeIdUnknown)."; + } + } + + return null; + } + + /// + /// String-input overload of , for the AdminUI + /// authoring model — which holds a mutable JsonObject key bag rather than a + /// and so reaches the same rule through a different door. Never throws: a + /// blank / unparseable / non-object blob draws nothing here, because those shapes are the OTHER + /// gates' business, not the member rule's. + /// + /// The authored equipment-tag TagConfig JSON. + /// The warning text, or when the members read. + public static string? DescribeInvalidMember(string? tagConfig) + { + if (string.IsNullOrWhiteSpace(tagConfig)) return null; + try + { + using var doc = JsonDocument.Parse(tagConfig); + return doc.RootElement.ValueKind == JsonValueKind.Object + ? DescribeInvalidMember(doc.RootElement) + : null; + } + catch (JsonException) + { + return null; + } + } + private static bool ReadBool(JsonElement o, string name) => o.TryGetProperty(name, out var e) && e.ValueKind == JsonValueKind.True; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyTagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyTagDefinitionFactory.cs index 011be526..7cc458a2 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyTagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyTagDefinitionFactory.cs @@ -119,16 +119,40 @@ public static class AbLegacyTagDefinitionFactory } /// - /// Deploy-time inspection (05/CONV-2): warns on a present-but-invalid dataType (silently - /// defaulted by the lenient runtime) and on a structurally unparseable TagConfig. Empty when clean - /// or not an equipment-tag object. Never throws. + /// The RawPath handed to during inspection — the mapper only copies it + /// onto and never validates it. /// + private const string InspectionRawPath = "(inspect)"; + + /// + /// Deploy-time inspection (05/CONV-2): warns for a blob would + /// reject — a structurally unparseable or non-object document, an absent / non-string / + /// blank address, or a present-but-invalid dataType (the runtime enum read is STRICT + /// since R2-11 Phase C, so a typo rejects the tag). Such a tag is skipped by the driver and reads + /// BadNodeIdUnknown forever. Empty when the mapper accepts the blob, when it is blank, or + /// when it is not an equipment-tag object. Never throws. + /// + /// + /// The verdict comes from , not from a mirror (Gitea #551) — see + /// ModbusTagDefinitionFactory.Inspect's remarks for the full argument. + /// + /// An arrayLength above is deliberately NOT + /// warned about: the mapper CLAMPS it rather than rejecting, so the driver serves the tag. The + /// AdminUI model refuses it at authoring because the un-clamped length also sets the OPC UA + /// ArrayDimensions; a warning here could be promoted to a deploy ERROR and would then + /// block a tag the runtime happily reads. + /// + /// /// The equipment tag's TagConfig JSON. /// The warnings; empty when clean. public static IReadOnlyList Inspect(string reference) { var warnings = new List(); if (string.IsNullOrWhiteSpace(reference) || reference[0] != '{') return warnings; + + // The accept gate: whatever the driver reads draws nothing. + if (FromTagConfig(reference, InspectionRawPath, out _)) return warnings; + try { using var doc = JsonDocument.Parse(reference); @@ -138,16 +162,47 @@ public static class AbLegacyTagDefinitionFactory warnings.Add("AbLegacy TagConfig root is not a JSON object — the tag will not resolve (BadNodeIdUnknown)."); return warnings; } - var w = TagConfigJson.DescribeInvalidEnum(root, "dataType"); - if (w is not null) warnings.Add(w); + + DescribeRejection(root, warnings); } catch (JsonException) { warnings.Add("AbLegacy TagConfig is not valid JSON — the tag will not resolve (BadNodeIdUnknown)."); + return warnings; } + + if (warnings.Count == 0) + { + warnings.Add( + "AbLegacy TagConfig was rejected by the driver's tag reader — the driver will skip this tag " + + "and it will read BadNodeIdUnknown."); + } + return warnings; } + /// + /// Explains why refused an object-rooted blob, walking its checks in the + /// same order. Runs only after the mapper has already refused, so it can never invent strictness. + /// + /// The TagConfig root object. + /// The list to append warnings to. + private static void DescribeRejection(JsonElement root, List warnings) + { + if (!root.TryGetProperty("address", out var addr) || addr.ValueKind != JsonValueKind.String) + { + warnings.Add( + "AbLegacy TagConfig has no string 'address' (e.g. N7:0, B3:1/2, F8:5) — the driver will " + + "skip this tag and it will read BadNodeIdUnknown."); + } + else if (string.IsNullOrWhiteSpace(addr.GetString())) + { + warnings.Add("AbLegacy TagConfig has a blank 'address' — the tag will not resolve (BadNodeIdUnknown)."); + } + + if (TagConfigJson.DescribeInvalidEnum(root, "dataType") is { } w) warnings.Add(w); + } + private static int ReadInt(JsonElement o, string name) => o.TryGetProperty(name, out var e) && e.ValueKind == JsonValueKind.Number && e.TryGetInt32(out var v) ? v : 0; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasTagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasTagDefinitionFactory.cs index 3ece4da2..7b20ca1c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasTagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Contracts/FocasTagDefinitionFactory.cs @@ -95,10 +95,33 @@ public static class FocasTagDefinitionFactory } /// - /// Deploy-time inspection (05/CONV-2, 05/UNDER-1): warns on a present-but-invalid dataType - /// (silently defaulted by the lenient runtime), on an authored writable:true (the FOCAS wire - /// is read-only — WriteAsync always returns BadNotWritable), and on a structurally - /// unparseable TagConfig. Empty when clean or not an equipment-tag object. Never throws. + /// The RawPath handed to during inspection — the mapper only copies it + /// onto and never validates it. + /// + private const string InspectionRawPath = "(inspect)"; + + /// + /// Deploy-time inspection (05/CONV-2, 05/UNDER-1). Two kinds of warning, and the distinction is + /// load-bearing: + /// + /// + /// Rejections — a blob would refuse: an unparseable or + /// non-object document, an absent / non-string / blank address, or a + /// present-but-invalid dataType (the runtime enum read is STRICT since R2-11 Phase C, + /// so a typo rejects the tag). The driver skips such a tag and it reads + /// BadNodeIdUnknown forever. These are diagnosed only when the mapper has actually + /// refused, so this gate can never be stricter than the runtime (Gitea #551). + /// + /// + /// Advisories — an accepted blob that still will not work: an unparseable + /// FOCAS address (#540; the mapper only requires a non-blank string, the driver's own init + /// then drops it) and an authored writable:true (the FOCAS wire is read-only, so + /// WriteAsync always returns BadNotWritable). These run whether or not the + /// blob parses, which is why FOCAS does not simply return empty on accept. + /// + /// + /// Empty for a clean, accepted blob, for a blank one, or for one that is not an equipment-tag + /// object. Never throws. /// /// The equipment tag's TagConfig JSON. /// The warnings; empty when clean. @@ -106,6 +129,11 @@ public static class FocasTagDefinitionFactory { var warnings = new List(); if (string.IsNullOrWhiteSpace(reference) || reference[0] != '{') return warnings; + + // The accept gate: only a blob the driver REFUSES gets a rejection diagnosis. The advisories below + // apply either way — see the summary. + var accepted = FromTagConfig(reference, InspectionRawPath, out _); + try { using var doc = JsonDocument.Parse(reference); @@ -115,30 +143,72 @@ public static class FocasTagDefinitionFactory warnings.Add("FOCAS TagConfig root is not a JSON object — the tag will not resolve (BadNodeIdUnknown)."); return warnings; } - var w = TagConfigJson.DescribeInvalidEnum(root, "dataType"); - if (w is not null) warnings.Add(w); - // Gitea #540: an address the driver cannot parse is skipped at init and reads - // BadNodeIdUnknown forever, while driver health stays Healthy with zero errors. The - // AdminUI modal rejects it outright; this covers the paths that never touch the modal - // (CSV import, the API) by at least surfacing it on the deployment. - if (root.TryGetProperty("address", out var addr) - && addr.ValueKind == JsonValueKind.String - && addr.GetString() is { } address - && !string.IsNullOrWhiteSpace(address) - && FocasAddress.TryParse(address) is null) - { - warnings.Add( - $"FOCAS address '{address.Trim()}' is not parseable (expected X0.0 / R100, " + - "PARAM:1020, MACRO:500 or FIXED:Identity/SeriesNumber) — the driver will skip " + - "this tag at startup and it will read BadNodeIdUnknown."); - } - if (root.TryGetProperty("writable", out var wr) && wr.ValueKind == JsonValueKind.True) - warnings.Add("FOCAS wire is read-only — an authored 'writable:true' still fails at the wire with BadNotWritable."); + + if (!accepted) DescribeRejection(root, warnings); + DescribeAdvisories(root, warnings); } catch (JsonException) { warnings.Add("FOCAS TagConfig is not valid JSON — the tag will not resolve (BadNodeIdUnknown)."); + return warnings; } + + if (!accepted && warnings.Count == 0) + { + warnings.Add( + "FOCAS TagConfig was rejected by the driver's tag reader — the driver will skip this tag " + + "and it will read BadNodeIdUnknown."); + } + return warnings; } + + /// + /// Explains why refused an object-rooted blob, walking its checks in the + /// same order. Runs only after the mapper has already refused, so it can never invent strictness. + /// + /// The TagConfig root object. + /// The list to append warnings to. + private static void DescribeRejection(JsonElement root, List warnings) + { + if (!root.TryGetProperty("address", out var addr) || addr.ValueKind != JsonValueKind.String) + { + warnings.Add( + "FOCAS TagConfig has no string 'address' — the driver will skip this tag and it will read " + + "BadNodeIdUnknown."); + } + else if (string.IsNullOrWhiteSpace(addr.GetString())) + { + warnings.Add("FOCAS TagConfig has a blank 'address' — the tag will not resolve (BadNodeIdUnknown)."); + } + + if (TagConfigJson.DescribeInvalidEnum(root, "dataType") is { } w) warnings.Add(w); + } + + /// + /// Warnings about a blob the mapper ACCEPTS but the driver still cannot serve as authored — see the + /// "advisories" bullet on . + /// + /// The TagConfig root object. + /// The list to append warnings to. + private static void DescribeAdvisories(JsonElement root, List warnings) + { + // Gitea #540: an address the driver cannot parse is skipped at init and reads BadNodeIdUnknown + // forever, while driver health stays Healthy with zero errors. The AdminUI modal rejects it + // outright; this covers the paths that never touch the modal (CSV import, the API). + if (root.TryGetProperty("address", out var addr) + && addr.ValueKind == JsonValueKind.String + && addr.GetString() is { } address + && !string.IsNullOrWhiteSpace(address) + && FocasAddress.TryParse(address) is null) + { + warnings.Add( + $"FOCAS address '{address.Trim()}' is not parseable (expected X0.0 / R100, " + + "PARAM:1020, MACRO:500 or FIXED:Identity/SeriesNumber) — the driver will skip " + + "this tag at startup and it will read BadNodeIdUnknown."); + } + + if (root.TryGetProperty("writable", out var wr) && wr.ValueKind == JsonValueKind.True) + warnings.Add("FOCAS wire is read-only — an authored 'writable:true' still fails at the wire with BadNotWritable."); + } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusTagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusTagDefinitionFactory.cs index 7d24df85..6c10215f 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusTagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Contracts/ModbusTagDefinitionFactory.cs @@ -130,18 +130,41 @@ public static class ModbusTagDefinitionFactory } /// - /// Deploy-time inspection of an equipment-tag TagConfig blob (05/CONV-2). Returns - /// human-readable warnings for present-but-invalid enum values (region / dataType / - /// byteOrder — which the lenient runtime silently defaults) and for a structurally - /// unparseable TagConfig (which the runtime turns into a silent BadNodeIdUnknown). Empty when - /// the blob is clean or is not an equipment-tag TagConfig object. Never throws. + /// The RawPath handed to during inspection. The mapper only copies it + /// onto and never validates it, so inspection is free to pass + /// a placeholder — the deploy gate is inspecting a blob, not a placed tag. /// + private const string InspectionRawPath = "(inspect)"; + + /// + /// Deploy-time inspection of an equipment-tag TagConfig blob (05/CONV-2). Returns + /// human-readable warnings for a blob would reject — a + /// structurally unparseable or non-object document, an absent region, an absent or + /// out-of-range address, a present-but-invalid region / dataType / + /// byteOrder / stringByteOrder (each rejects the tag: the runtime enum reads are + /// STRICT since R2-11 Phase C), or a type-dependent guard (String with no + /// stringLength, BitInRegister with a bitIndex outside 0–15). Such a tag is + /// skipped by the driver and reads BadNodeIdUnknown forever. Empty when the mapper accepts + /// the blob, when it is blank, or when it is not an equipment-tag TagConfig object. Never throws. + /// + /// + /// The verdict comes from , not from a mirror (Gitea #551). The + /// rule this gate must never break is being stricter than the runtime — a blob the driver serves + /// must not draw a warning calling it invalid — and running the mapper first makes that structural + /// rather than a promise. It also closes the opposite gap: the previous hand-list of enum checks + /// was silent for every rejection cause that was not on it (an absent region, a bad + /// address, stringByteOrder), so a tag the driver refuses deployed clean. + /// /// The equipment tag's TagConfig JSON. /// The warnings; empty when clean. public static IReadOnlyList Inspect(string reference) { var warnings = new List(); if (string.IsNullOrWhiteSpace(reference) || reference[0] != '{') return warnings; + + // The accept gate: whatever the driver reads draws nothing. See the remarks. + if (FromTagConfig(reference, InspectionRawPath, out _)) return warnings; + try { using var doc = JsonDocument.Parse(reference); @@ -151,23 +174,90 @@ public static class ModbusTagDefinitionFactory warnings.Add("Modbus TagConfig root is not a JSON object — the tag will not resolve (BadNodeIdUnknown)."); return warnings; } - foreach (var w in new[] - { - TagConfigJson.DescribeInvalidEnum(root, "region"), - TagConfigJson.DescribeInvalidEnum(root, "dataType"), - TagConfigJson.DescribeInvalidEnum(root, "byteOrder"), - }) - { - if (w is not null) warnings.Add(w); - } + + DescribeRejection(root, warnings); } catch (JsonException) { warnings.Add("Modbus TagConfig is not valid JSON — the tag will not resolve (BadNodeIdUnknown)."); + return warnings; } + + // Backstop: the mapper refused for a reason nothing above names (it also swallows FormatException + // and InvalidOperationException). Saying so beats reporting a clean blob the driver will skip. + if (warnings.Count == 0) + { + warnings.Add( + "Modbus TagConfig was rejected by the driver's tag reader — the driver will skip this tag " + + "and it will read BadNodeIdUnknown."); + } + return warnings; } + /// + /// Explains why refused an object-rooted blob, walking its checks in the + /// same order. Every branch mirrors a return false there — and it runs only after the mapper + /// has already refused, so it can never invent strictness of its own. + /// + /// The TagConfig root object. + /// The list to append warnings to. + private static void DescribeRejection(JsonElement root, List warnings) + { + if (!root.TryGetProperty("region", out _)) + { + warnings.Add( + "Modbus TagConfig has no 'region' — every Modbus tag must name one (valid: " + + string.Join(", ", Enum.GetNames()) + + "); the driver will skip this tag and it will read BadNodeIdUnknown."); + } + + if (!root.TryGetProperty("address", out var addr)) + { + warnings.Add("Modbus TagConfig has no 'address' — the tag will not resolve (BadNodeIdUnknown)."); + } + else if (addr.ValueKind != JsonValueKind.Number + || !addr.TryGetInt32(out var address) + || address < 0 || address > ushort.MaxValue) + { + warnings.Add( + $"value '{addr.GetRawText()}' for 'address' is not a Modbus register address (an integer " + + $"0–{ushort.MaxValue}); the tag will not resolve (BadNodeIdUnknown)."); + } + + // Strict enums, in the order FromTagConfig reads them. stringByteOrder is read as strictly as its + // three siblings, and was the one the previous hand-list forgot (Gitea #551). + foreach (var w in new[] + { + TagConfigJson.DescribeInvalidEnum(root, "region"), + TagConfigJson.DescribeInvalidEnum(root, "dataType"), + TagConfigJson.DescribeInvalidEnum(root, "byteOrder"), + TagConfigJson.DescribeInvalidEnum(root, "stringByteOrder"), + }) + { + if (w is not null) warnings.Add(w); + } + + // Type-dependent guards. The byte/ushort NARROWING is part of the rule, not an accident of the + // mapper: it reads `(byte)bitIndex` / `(ushort)stringLength`, so 256 is bit 0 and is ACCEPTED. + // Describing the un-narrowed value here would call a blob invalid that the driver serves. + if (!TagConfigJson.TryReadEnumStrict(root, "dataType", ModbusDataType.Int16, out var dataType)) return; + + if (dataType == ModbusDataType.String && (ushort)ReadInt(root, "stringLength") < 1) + { + warnings.Add( + "Modbus String tag has no 'stringLength' of at least 1 — a zero-length string reads zero " + + "registers, which is an illegal Modbus request, so the tag is rejected (BadNodeIdUnknown)."); + } + + if (dataType == ModbusDataType.BitInRegister && (byte)ReadInt(root, "bitIndex") > 15) + { + warnings.Add( + "Modbus BitInRegister tag has a 'bitIndex' outside 0–15 — the bit mask would overflow a " + + "16-bit register, so the tag is rejected (BadNodeIdUnknown)."); + } + } + private static int ReadInt(JsonElement o, string name) => o.TryGetProperty(name, out var e) && e.ValueKind == JsonValueKind.Number && e.TryGetInt32(out var v) ? v : 0; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttTagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttTagDefinitionFactory.cs index abceea13..c8f08782 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttTagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttTagDefinitionFactory.cs @@ -205,19 +205,46 @@ public static class MqttTagDefinitionFactory } /// - /// Deploy-time inspection of an equipment-tag TagConfig blob. Returns human-readable - /// warnings for a structurally unparseable blob (which the runtime turns into a silent - /// BadNodeIdUnknown), for present-but-invalid payloadFormat / dataType / - /// qos values, and for a wildcard tag topic (a tag bound to + / # - /// would receive values from many topics — ambiguous, and almost never what the operator meant). - /// Empty when the blob is clean or is not an equipment-tag TagConfig object. Never throws. + /// The RawPath handed to the mappers during inspection — they only copy it onto + /// and never validate it. /// + private const string InspectionRawPath = "(inspect)"; + + /// + /// Deploy-time inspection of an equipment-tag TagConfig blob. Two kinds of warning: + /// + /// + /// Rejections — a blob NEITHER runtime mapper would read: an unparseable or + /// non-object document, present-but-invalid payloadFormat / dataType / + /// qos values (all read strictly), or an address that names neither a Plain + /// topic nor a complete Sparkplug groupId/edgeNodeId/metricName + /// tuple. Such a tag is skipped by the driver and reads BadNodeIdUnknown forever. + /// + /// + /// Advisory — a wildcard tag topic (+ / #). The mapper + /// deliberately ACCEPTS it (ambiguous, not unparseable), so this runs either way. + /// + /// + /// Empty for a clean, readable blob, for a blank one, or for one that is not an equipment-tag + /// TagConfig object. Never throws. + /// + /// + /// The accept gate is the UNION of the two mappers (Gitea #551), because Inspect is + /// handed a blob with no mode: the driver's decides which of + /// / reads it, and nothing about a + /// tag row says which. Gating on the Plain mapper alone would warn that every correctly-authored + /// Sparkplug tag has no topic — a gate stricter than the runtime, the one thing it must never be. + /// /// The equipment tag's TagConfig JSON. /// The warnings; empty when clean. public static IReadOnlyList Inspect(string reference) { var warnings = new List(); if (string.IsNullOrWhiteSpace(reference) || reference[0] != '{') return warnings; + + var accepted = FromTagConfig(reference, InspectionRawPath, out _) + || FromSparkplugTagConfig(reference, InspectionRawPath, out _); + try { using var doc = JsonDocument.Parse(reference); @@ -227,16 +254,11 @@ public static class MqttTagDefinitionFactory warnings.Add("Mqtt TagConfig root is not a JSON object — the tag will not resolve (BadNodeIdUnknown)."); return warnings; } - foreach (var w in new[] - { - TagConfigJson.DescribeInvalidEnum(root, "payloadFormat"), - TagConfigJson.DescribeInvalidEnum(root, "dataType"), - DescribeInvalidQos(root), - }) - { - if (w is not null) warnings.Add(w); - } - var topic = ReadString(root, "topic"); + + if (!accepted) DescribeRejection(root, warnings); + + // Advisory — the mappers accept a wildcard topic on purpose; see the summary. + var topic = ReadString(root, MqttTagConfigKeys.Topic); if (topic.IndexOfAny(TopicWildcards) >= 0) { warnings.Add( @@ -247,10 +269,53 @@ public static class MqttTagDefinitionFactory catch (JsonException) { warnings.Add("Mqtt TagConfig is not valid JSON — the tag will not resolve (BadNodeIdUnknown)."); + return warnings; } + + if (!accepted && warnings.Count == 0) + { + warnings.Add( + "Mqtt TagConfig was rejected by the driver's tag reader — the driver will skip this tag " + + "and it will read BadNodeIdUnknown."); + } + return warnings; } + /// + /// Explains why BOTH mappers refused an object-rooted blob. The strict-field checks are shared by + /// the two shapes; the address check names what each shape needed. Runs only after both mappers have + /// already refused, so it can never invent strictness. + /// + /// The TagConfig root object. + /// The list to append warnings to. + private static void DescribeRejection(JsonElement root, List warnings) + { + foreach (var w in new[] + { + TagConfigJson.DescribeInvalidEnum(root, "payloadFormat"), + TagConfigJson.DescribeInvalidEnum(root, "dataType"), + DescribeInvalidQos(root), + }) + { + if (w is not null) warnings.Add(w); + } + + // Address: Plain needs a topic, Sparkplug needs the whole binding tuple. Neither is satisfied, or + // one of the strict fields above already refused both — only say it when the address is the cause. + var hasTopic = !string.IsNullOrWhiteSpace(ReadString(root, MqttTagConfigKeys.Topic)); + var hasSparkplugTuple = !string.IsNullOrWhiteSpace(ReadString(root, MqttTagConfigKeys.GroupId)) + && !string.IsNullOrWhiteSpace(ReadString(root, MqttTagConfigKeys.EdgeNodeId)) + && !string.IsNullOrWhiteSpace(ReadString(root, MqttTagConfigKeys.MetricName)); + if (!hasTopic && !hasSparkplugTuple) + { + warnings.Add( + "Mqtt TagConfig names no address — a Plain-mode tag needs a 'topic', a Sparkplug B tag " + + "needs 'groupId' + 'edgeNodeId' + 'metricName'. The driver will skip this tag and it " + + "will read BadNodeIdUnknown."); + } + } + /// /// Strict qos read, mirroring 's /// absent / valid / present-but-invalid split: absent ⇒ (the driver-level diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagDefinitionFactory.cs index 48044406..6d1839b4 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Contracts/S7TagDefinitionFactory.cs @@ -111,16 +111,61 @@ public static class S7TagDefinitionFactory } /// - /// Deploy-time inspection (05/CONV-2): warns on a present-but-invalid dataType (silently - /// defaulted by the lenient runtime) and on a structurally unparseable TagConfig (a silent - /// BadNodeIdUnknown at runtime). Empty when clean or not an equipment-tag object. Never throws. + /// The TagConfig key carrying the S7 data type. Shared with the AdminUI authoring model so + /// "does the blob declare a type?" is asked of one spelling, not two (Gitea #551). /// + public const string DataTypeKey = "dataType"; + + /// + /// The RawPath handed to during inspection — the mapper only copies it + /// onto and never validates it. + /// + private const string InspectionRawPath = "(inspect)"; + + /// + /// The single authority on whether an S7 blob DECLARES a data type, consulted by + /// and by the AdminUI's S7TagConfigModel.Validate alike. + /// + /// S7 is the one driver in the fleet whose reader requires the key to be present + /// ( refuses a blob without it, where every sibling falls back to a + /// default), so a hand-authored / CSV / API blob that omits it used to pass both gates and die + /// at the driver with BadNodeIdUnknown — Gitea #551 item 2. + /// + /// + /// Presence is judged on the KEY, not on the value: a non-string token + /// ("dataType": 8) satisfies the presence check and is then treated as absent by + /// , so the blob is ACCEPTED as + /// . Judging the value here would make this gate stricter than + /// the reader. + /// + /// + /// The TagConfig root object. + /// when the blob carries a dataType key of any kind. + public static bool DeclaresDataType(JsonElement root) => root.TryGetProperty(DataTypeKey, out _); + + /// + /// Deploy-time inspection (05/CONV-2): warns for a blob would + /// reject — a structurally unparseable or non-object document, an absent / non-string / + /// blank address, an absent dataType (see ), a + /// present-but-invalid dataType (the runtime enum read is STRICT since R2-11 Phase C, so a + /// typo rejects the tag), or an out-of-range stringLength on a String tag. Such a tag + /// is skipped by the driver and reads BadNodeIdUnknown forever. Empty when the mapper accepts + /// the blob, when it is blank, or when it is not an equipment-tag object. Never throws. + /// + /// + /// The verdict comes from , not from a mirror (Gitea #551) — see + /// ModbusTagDefinitionFactory.Inspect's remarks for the full argument. + /// /// The equipment tag's TagConfig JSON. /// The warnings; empty when clean. public static IReadOnlyList Inspect(string reference) { var warnings = new List(); if (string.IsNullOrWhiteSpace(reference) || reference[0] != '{') return warnings; + + // The accept gate: whatever the driver reads draws nothing. + if (FromTagConfig(reference, InspectionRawPath, out _)) return warnings; + try { using var doc = JsonDocument.Parse(reference); @@ -130,16 +175,67 @@ public static class S7TagDefinitionFactory warnings.Add("S7 TagConfig root is not a JSON object — the tag will not resolve (BadNodeIdUnknown)."); return warnings; } - var w = TagConfigJson.DescribeInvalidEnum(root, "dataType"); - if (w is not null) warnings.Add(w); + + DescribeRejection(root, warnings); } catch (JsonException) { warnings.Add("S7 TagConfig is not valid JSON — the tag will not resolve (BadNodeIdUnknown)."); + return warnings; } + + if (warnings.Count == 0) + { + warnings.Add( + "S7 TagConfig was rejected by the driver's tag reader — the driver will skip this tag and " + + "it will read BadNodeIdUnknown."); + } + return warnings; } + /// + /// Explains why refused an object-rooted blob, walking its checks in the + /// same order. Runs only after the mapper has already refused, so it can never invent strictness. + /// + /// The TagConfig root object. + /// The list to append warnings to. + private static void DescribeRejection(JsonElement root, List warnings) + { + if (!root.TryGetProperty("address", out var addrEl) || addrEl.ValueKind != JsonValueKind.String) + { + warnings.Add( + "S7 TagConfig has no string 'address' (e.g. DB1.DBW0, M0.0, I0.0, QD4) — the driver will " + + "skip this tag and it will read BadNodeIdUnknown."); + } + else if (string.IsNullOrWhiteSpace(addrEl.GetString())) + { + warnings.Add("S7 TagConfig has a blank 'address' — the tag will not resolve (BadNodeIdUnknown)."); + } + + if (!DeclaresDataType(root)) + { + warnings.Add( + "S7 TagConfig has no 'dataType' — the S7 reader REQUIRES the key to be present (unlike its " + + "sibling drivers, which default it), so the driver will skip this tag and it will read " + + "BadNodeIdUnknown. Valid: " + string.Join(", ", Enum.GetNames()) + "."); + } + + if (TagConfigJson.DescribeInvalidEnum(root, DataTypeKey) is { } w) warnings.Add(w); + + if (TagConfigJson.TryReadEnumStrict(root, DataTypeKey, S7DataType.Int16, out var dataType) + && dataType == S7DataType.String) + { + var stringLength = ReadInt(root, "stringLength"); + if (stringLength < 0 || stringLength > MaxStringLength) + { + warnings.Add( + $"S7 String tag has a 'stringLength' of {stringLength} — an S7 string is 0–" + + $"{MaxStringLength} characters, so the tag is rejected (BadNodeIdUnknown)."); + } + } + } + private static int ReadInt(JsonElement o, string name) => o.TryGetProperty(name, out var e) && e.ValueKind == JsonValueKind.Number && e.TryGetInt32(out var v) ? v : 0; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Contracts/TwinCATTagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Contracts/TwinCATTagDefinitionFactory.cs index 92aca8c7..b7ed7357 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Contracts/TwinCATTagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Contracts/TwinCATTagDefinitionFactory.cs @@ -109,16 +109,34 @@ public static class TwinCATTagDefinitionFactory } /// - /// Deploy-time inspection (05/CONV-2): warns on a present-but-invalid dataType (silently - /// defaulted by the lenient runtime) and on a structurally unparseable TagConfig. Empty when clean - /// or not an equipment-tag object. Never throws. + /// The RawPath handed to during inspection — the mapper only copies it + /// onto and never validates it. /// + private const string InspectionRawPath = "(inspect)"; + + /// + /// Deploy-time inspection (05/CONV-2): warns for a blob would + /// reject — a structurally unparseable or non-object document, an absent / non-string / + /// blank symbolPath, a present-but-invalid dataType (the runtime enum read is STRICT + /// since R2-11 Phase C, so a typo rejects the tag), or the unsupported + /// type. Such a tag is skipped by the driver and reads + /// BadNodeIdUnknown forever. Empty when the mapper accepts the blob, when it is blank, or + /// when it is not an equipment-tag object. Never throws. + /// + /// + /// The verdict comes from , not from a mirror (Gitea #551) — see + /// ModbusTagDefinitionFactory.Inspect's remarks for the full argument. + /// /// The equipment tag's TagConfig JSON. /// The warnings; empty when clean. public static IReadOnlyList Inspect(string reference) { var warnings = new List(); if (string.IsNullOrWhiteSpace(reference) || reference[0] != '{') return warnings; + + // The accept gate: whatever the driver reads draws nothing. + if (FromTagConfig(reference, InspectionRawPath, out _)) return warnings; + try { using var doc = JsonDocument.Parse(reference); @@ -128,16 +146,56 @@ public static class TwinCATTagDefinitionFactory warnings.Add("TwinCAT TagConfig root is not a JSON object — the tag will not resolve (BadNodeIdUnknown)."); return warnings; } - var w = TagConfigJson.DescribeInvalidEnum(root, "dataType"); - if (w is not null) warnings.Add(w); + + DescribeRejection(root, warnings); } catch (JsonException) { warnings.Add("TwinCAT TagConfig is not valid JSON — the tag will not resolve (BadNodeIdUnknown)."); + return warnings; } + + if (warnings.Count == 0) + { + warnings.Add( + "TwinCAT TagConfig was rejected by the driver's tag reader — the driver will skip this tag " + + "and it will read BadNodeIdUnknown."); + } + return warnings; } + /// + /// Explains why refused an object-rooted blob, walking its checks in the + /// same order. Runs only after the mapper has already refused, so it can never invent strictness. + /// + /// The TagConfig root object. + /// The list to append warnings to. + private static void DescribeRejection(JsonElement root, List warnings) + { + if (!root.TryGetProperty("symbolPath", out var symbol) || symbol.ValueKind != JsonValueKind.String) + { + warnings.Add( + "TwinCAT TagConfig has no string 'symbolPath' (e.g. MAIN.bStart) — the driver will skip " + + "this tag and it will read BadNodeIdUnknown."); + } + else if (string.IsNullOrWhiteSpace(symbol.GetString())) + { + warnings.Add("TwinCAT TagConfig has a blank 'symbolPath' — the tag will not resolve (BadNodeIdUnknown)."); + } + + if (TagConfigJson.DescribeInvalidEnum(root, "dataType") is { } w) warnings.Add(w); + + if (TagConfigJson.TryReadEnumStrict(root, "dataType", TwinCATDataType.DInt, out var dataType) + && dataType == TwinCATDataType.Structure) + { + warnings.Add( + "TwinCAT 'dataType' is Structure — the driver does not support UDT / FB-instance tags and " + + "rejects them (BadNodeIdUnknown). Declare the atomic-typed members instead, or use " + + "EnableControllerBrowse to discover them individually."); + } + } + /// /// Reads the optional 1-D array length: arrayLength (a positive int32; values above /// are treated as absent/scalar) honoured ONLY when isArray diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTagCsvMapper.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTagCsvMapper.cs index c6390206..72b7e9d7 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTagCsvMapper.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTagCsvMapper.cs @@ -19,6 +19,14 @@ namespace ZB.MOM.WW.OtOpcUa.AdminUI.Uns; /// columns. A typed column therefore wins over a conflicting key carried in the TagConfigJson /// fallback; the review model flags when that override happened so the operator sees it. /// +/// +/// The assembled blob is then put to the driver's own (Gitea #551): the +/// typed columns were always strictly parsed, but the free-form TagConfigJson column reached the +/// database unread, so a CSV could author a tag the driver rejects — one that deploys clean and reads +/// BadNodeIdUnknown forever. A failing row is an error, blocking the all-or-nothing commit; +/// it is never silently skipped. This check is for the operator's benefit (it names the row before +/// commit); the guarantee lives in , which re-runs it. +/// /// public static class RawTagCsvMapper { @@ -84,7 +92,7 @@ public static class RawTagCsvMapper continue; } - reviewed.Add(BuildRow(r, headers, headerIndex, cells, map, groupPathPrefix)); + reviewed.Add(BuildRow(r, headers, headerIndex, cells, driverType, map, groupPathPrefix)); } FlagDuplicates(reviewed); @@ -96,6 +104,7 @@ public static class RawTagCsvMapper string[] headers, IReadOnlyDictionary headerIndex, string[] cells, + string driverType, CsvDriverTagMap? map, string? groupPathPrefix) { @@ -193,6 +202,19 @@ public static class RawTagCsvMapper // Alarm flags — an alarm is authored iff Alarm.AlarmType is present. json = ApplyAlarm(json, Cell("Alarm.AlarmType"), Cell("Alarm.Severity"), Cell("Alarm.HistorizeToAveva")); + // The driver-typed config gate, on the FINAL assembled blob (Gitea #551 item 1). Until this, + // the typed columns above were strictly parsed but the free-form TagConfigJson column was not + // checked at all, so a CSV could import a blob the driver rejects — a tag that deploys clean + // and then reads BadNodeIdUnknown forever. Surfacing it here, on the staged review row, is what + // lets the operator SEE which row is wrong before committing; IRawTreeService.ImportTagsAsync + // re-checks it as the actual guarantee (a form/grid cannot cover the API or a future caller). + // A rejected row is an ERROR, never a skip: the commit is all-or-nothing, and "skipped" would + // claim the tag is already present. + if (TagConfigValidator.Validate(driverType, json) is { } configError) + { + return Invalid(rowNumber, cells, $"TagConfig: {configError}"); + } + var importRow = new RawTagImportRow(groupPath, new RawTagInput(name, dataType, access, writeIdempotent, pollGroupId, json)); return new RawCsvReviewRow diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs index 1039b03c..a4e92493 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/RawTreeService.cs @@ -1,5 +1,6 @@ using System.Text.Json; using Microsoft.EntityFrameworkCore; +using ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors; using ZB.MOM.WW.OtOpcUa.Commons.Types; using ZB.MOM.WW.OtOpcUa.Configuration; using ZB.MOM.WW.OtOpcUa.Configuration.Entities; @@ -834,6 +835,9 @@ public sealed class RawTreeService(IDbContextFactory dbF t => t.DeviceId == deviceId && t.TagGroupId == tagGroupId && t.Name == input.Name, ct)) return new UnsMutationResult(false, $"A tag named '{input.Name}' already exists here."); + if (await ValidateTagConfigAsync(db, deviceId, tagConfig, ct) is { } configError) + return new UnsMutationResult(false, configError); + var id = NewId("TAG"); db.Tags.Add(new Tag { @@ -872,6 +876,9 @@ public sealed class RawTreeService(IDbContextFactory dbF && t.Name == input.Name && t.TagId != tagId, ct)) return new UnsMutationResult(false, $"A tag named '{input.Name}' already exists here."); + if (await ValidateTagConfigAsync(db, entity.DeviceId, tagConfig, ct) is { } configError) + return new UnsMutationResult(false, configError); + db.Entry(entity).Property(e => e.RowVersion).OriginalValue = rowVersion; entity.Name = input.Name; entity.DataType = input.DataType; @@ -902,6 +909,10 @@ public sealed class RawTreeService(IDbContextFactory dbF if (!await db.Devices.AnyAsync(dev => dev.DeviceId == deviceId, ct)) return new RawTagImportResult(0, new[] { $"Device '{deviceId}' not found." }); + // The device's driver type, resolved ONCE: an import is per-device, and a device hangs off exactly + // one driver, so every row in the batch is judged by the same driver's rules (Gitea #551 item 1). + var driverType = await ResolveDriverTypeAsync(db, deviceId, ct); + // Existing groups on the device: a (parent-group-id | null, name) → group-id lookup we extend with // pending (auto-created) groups so a repeated path within the batch resolves to one group. var groupLookup = (await db.TagGroups.AsNoTracking() @@ -951,6 +962,15 @@ public sealed class RawTreeService(IDbContextFactory dbF continue; } + // The driver-typed config gate. A rejected row is an ERROR — the import is all-or-nothing, so + // the batch inserts nothing and the row is NAMED; it is never skipped ("skipped" would claim + // the tag is already present). See ValidateTagConfigAsync for why this lives in the service. + if (TagConfigValidator.Validate(driverType, tagConfig) is { } configError) + { + errors.Add($"{rowLabel}: TagConfig: {configError}"); + continue; + } + if (!takenNames.Add((groupId, row.Tag.Name))) { errors.Add($"{rowLabel}: a tag named '{row.Tag.Name}' already exists at that path."); @@ -1196,6 +1216,55 @@ public sealed class RawTreeService(IDbContextFactory dbF } } + /// + /// The driver-type string of the driver a device hangs off, or when the + /// device (or its driver) is gone. Joined in exactly as LoadDeviceForEditAsync does — a + /// device carries no driver type of its own. + /// + /// The open context. + /// The device whose driver type to resolve. + /// Cancellation token. + /// The canonical driver-type string, or null. + private static Task ResolveDriverTypeAsync( + OtOpcUaConfigDbContext db, string deviceId, CancellationToken ct) + => db.Devices.AsNoTracking() + .Where(dev => dev.DeviceId == deviceId) + .Join(db.DriverInstances.AsNoTracking(), + dev => dev.DriverInstanceId, + drv => drv.DriverInstanceId, + (dev, drv) => drv.DriverType) + .FirstOrDefaultAsync(ct); + + /// + /// The driver-typed TagConfig gate, applied on the SERVER for every tag-writing path — + /// create, update and bulk import alike (Gitea #551 item 1). + /// + /// It lives here rather than in the modals because a form cannot be the guarantee. The + /// tag modal and the manual-entry grid already run for fast + /// feedback, but the CSV import did not, and the API and any future caller reach none of them — + /// so a blob the driver rejects could be persisted, deploy clean, and read + /// BadNodeIdUnknown forever. This repo also has no bUnit, so a rule only a page enforces + /// is a rule no test can reach. + /// + /// + /// The rule itself is not stated here: dispatches to the + /// driver's typed model, which in turn asks the driver's own reader. An unmapped driver type + /// (Galaxy) validates to null, exactly as it does in the modals. + /// + /// + /// The open context. + /// The device the tag belongs to (supplies the driver type). + /// The normalised TagConfig JSON about to be persisted. + /// Cancellation token. + /// An operator-facing error, or when the config is acceptable. + private static async Task ValidateTagConfigAsync( + OtOpcUaConfigDbContext db, string deviceId, string tagConfig, CancellationToken ct) + { + var driverType = await ResolveDriverTypeAsync(db, deviceId, ct); + var error = TagConfigValidator.Validate(driverType, tagConfig); + return error is null ? null : $"TagConfig: {error}"; + } + /// Ordinal (case-sensitive) equality for a nullable-parent + name tag-group key. private sealed class GroupKeyComparer : IEqualityComparer<(string? Parent, string Name)> { diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbCipTagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbCipTagConfigModel.cs index ace7cbc6..b4cd3f43 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbCipTagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbCipTagConfigModel.cs @@ -59,11 +59,23 @@ public sealed class AbCipTagConfigModel /// (BadNodeIdUnknown). Without this the blob would be accepted here, silently rewritten to /// DInt on save, and — on the CSV-import and API paths, which save it verbatim — deployed /// into a tag the driver skips (Gitea #549). + /// + /// Declared UDT members are checked too (Gitea #551), and the rule is not restated here. + /// The member rules are as strict as the top-level ones — one member with no name or a + /// typo'd dataType rejects the WHOLE tag — but nothing on the authoring side saw them: + /// this editor emits only tagPath/dataType/writable, so members + /// rides untouched from whatever hand-authored / CSV / API blob produced it. + /// The verdict AND the wording come from + /// , the same authority the + /// driver's reader and the deploy gate consult, so this gate cannot drift from the runtime in + /// either direction. + /// /// /// An error message describing the validation failure, or null when valid. public string? Validate() { if (TagConfigJson.DescribeInvalidEnum(_bag, "dataType") is { } dtError) { return dtError; } - return string.IsNullOrWhiteSpace(TagPath) ? "Tag path is required." : null; + if (string.IsNullOrWhiteSpace(TagPath)) { return "Tag path is required."; } + return AbCipTagDefinitionFactory.DescribeInvalidMember(TagConfigJson.Serialize(_bag)); } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/ModbusTagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/ModbusTagConfigModel.cs index 9396b38c..dad040e5 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/ModbusTagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/ModbusTagConfigModel.cs @@ -69,17 +69,26 @@ public sealed class ModbusTagConfigModel /// Validation hook; returns an error message or null when the model is valid. /// - /// All three enums are read from the ORIGINAL key bag, not from the leniently-defaulted typed + /// All FOUR enums are read from the ORIGINAL key bag, not from the leniently-defaulted typed /// fields above: ModbusTagDefinitionFactory reads each with TryReadEnumStrict, so a /// present-but-invalid string rejects the whole tag at the driver (BadNodeIdUnknown) where - /// the load silently substitutes HoldingRegisters/Int16/BigEndian. Accepting - /// one here would let the CSV-import and API paths — which save the blob verbatim — deploy a tag - /// the driver skips (Gitea #549). Also note a wrong-but-valid region is not this gate's - /// business: only a spelling the runtime cannot read is rejected. + /// the load silently substitutes HoldingRegisters/Int16/BigEndian/ + /// HighByteFirst. Accepting one here would let the CSV-import and API paths — which save the + /// blob verbatim — deploy a tag the driver skips (Gitea #549). Also note a wrong-but-valid + /// region is not this gate's business: only a spelling the runtime cannot read is rejected. + /// + /// stringByteOrder is the fourth (Gitea #551): the runtime has always read it as + /// strictly as its three siblings, but this model — and the deploy gate's Inspect — + /// checked only the other three, so a typo'd stringByteOrder passed BOTH gates and died + /// at the driver. It is not in 's exposed-field list either, so it can only + /// ever reach the bag from a hand-authored blob, a CSV import or the API — exactly the paths + /// this gate exists for. + /// /// /// An error message describing the validation failure, or null when the model is valid. public string? Validate() => TagConfigJson.DescribeInvalidEnum(_bag, "region") ?? TagConfigJson.DescribeInvalidEnum(_bag, "dataType") - ?? TagConfigJson.DescribeInvalidEnum(_bag, "byteOrder"); + ?? TagConfigJson.DescribeInvalidEnum(_bag, "byteOrder") + ?? TagConfigJson.DescribeInvalidEnum(_bag, "stringByteOrder"); } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs index 5617bb3c..f6a4f748 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs @@ -22,6 +22,19 @@ public sealed class S7TagConfigModel private JsonObject _bag = new(); + // Whether the blob this model was LOADED FROM was missing the "dataType" key entirely — captured + // because the lenient load above cannot tell "absent" from "present": both land on Int16. S7 is the one + // driver whose reader REQUIRES the key (S7TagDefinitionFactory.DeclaresDataType), so a blob without one + // is rejected outright and reads BadNodeIdUnknown; without this capture that blob validated clean + // (Gitea #551). Same capture-the-as-loaded-shape pattern as MTConnectTagConfigModel's _rawDataType. + // + // It records a property of the SOURCE, so the default (false) is the honest answer for a model built by + // object initialiser rather than loaded — RawBrowseCommitMapper does exactly that, and ToJson() then + // writes the key from the typed DataType field. ToJson() likewise clears it: a blob that has been + // re-serialised through this model always carries the key, so it can only ever be true on the FIRST + // load of a blob authored elsewhere (hand-edited, CSV import, API). + private bool _dataTypeMissingInSource; + /// Loads a model from a TagConfig JSON string, defaulting any absent field and retaining /// every original key (so fields this editor doesn't expose survive a load→save). /// The raw TagConfig JSON string, or null/empty for a fresh model. @@ -32,9 +45,10 @@ public sealed class S7TagConfigModel return new S7TagConfigModel { Address = TagConfigJson.GetString(o, "address") ?? "", - DataType = TagConfigJson.GetEnum(o, "dataType", S7DataType.Int16), + DataType = TagConfigJson.GetEnum(o, S7TagDefinitionFactory.DataTypeKey, S7DataType.Int16), StringLength = TagConfigJson.GetInt(o, "stringLength"), Writable = TagConfigJson.GetBoolNullable(o, "writable"), + _dataTypeMissingInSource = !o.ContainsKey(S7TagDefinitionFactory.DataTypeKey), _bag = o, }; } @@ -45,7 +59,8 @@ public sealed class S7TagConfigModel public string ToJson() { TagConfigJson.Set(_bag, "address", Address.Trim()); - TagConfigJson.Set(_bag, "dataType", DataType); + TagConfigJson.Set(_bag, S7TagDefinitionFactory.DataTypeKey, DataType); + _dataTypeMissingInSource = false; TagConfigJson.Set(_bag, "stringLength", StringLength); TagConfigJson.Set(_bag, "writable", Writable); return TagConfigJson.Serialize(_bag); @@ -59,10 +74,36 @@ public sealed class S7TagConfigModel /// so a present-but-invalid string rejects the whole tag at the driver (BadNodeIdUnknown) /// where the load silently substitutes Int16. Accepting it here would let the CSV-import /// and API paths — which save the blob verbatim — deploy an unreadable tag (Gitea #549). + /// + /// An ABSENT dataType is rejected too (Gitea #551), and S7 is the only driver where + /// that is right. S7TagDefinitionFactory.FromTagConfig refuses a blob that does not + /// carry the key at all — unique in the fleet, where every sibling falls back to a default — so + /// such a tag deployed clean through both gates and then read BadNodeIdUnknown forever. + /// Presence is judged on the key, not the value, matching + /// : a non-string token is present-enough + /// for the reader, which then treats it as absent and accepts the tag as Int16. + /// + /// + /// The one blob this can newly block in the modal is one that was already dead. The + /// typed editor pushes — which always writes dataType — only when a + /// field CHANGES, so opening a legacy key-less blob and saving it untouched hands this method + /// the as-loaded string. That save is refused, and correctly: the tag it would persist is one + /// the driver already skips. Picking any data type in the editor writes the key and clears it. + /// /// public string? Validate() { - if (TagConfigJson.DescribeInvalidEnum(_bag, "dataType") is { } dtError) { return dtError; } - return string.IsNullOrWhiteSpace(Address) ? "Address is required." : null; + if (TagConfigJson.DescribeInvalidEnum(_bag, S7TagDefinitionFactory.DataTypeKey) is { } dtError) + { + return dtError; + } + + if (string.IsNullOrWhiteSpace(Address)) { return "Address is required."; } + + return _dataTypeMissingInSource + ? "A data type is required — the S7 driver rejects a tag whose TagConfig carries no " + + $"'{S7TagDefinitionFactory.DataTypeKey}' key (it would read BadNodeIdUnknown). Valid: " + + string.Join(", ", Enum.GetNames()) + "." + : null; } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs index 6f21f0e3..7c5112a6 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/AdminOperationsActor.cs @@ -246,8 +246,11 @@ public sealed class AdminOperationsActor : ReceiveActor } // R2-11 (05/CONV-2): deploy-time TagConfig strictness inspection over equipment tags. The - // driver-typed EquipmentTagConfigInspector surfaces present-but-invalid enum values (which the - // lenient runtime silently defaults) + structurally-unparseable TagConfig. In Warn mode + // driver-typed EquipmentTagConfigInspector surfaces every blob the driver's own reader would + // REJECT — a present-but-invalid enum value, an absent or malformed address field, a + // structurally-unparseable TagConfig — each of which makes the driver skip the tag so it reads + // BadNodeIdUnknown forever. (Pre-R2-11 Phase C the runtime silently defaulted a typo'd enum; + // it has rejected the tag since, so these warnings describe rejections.) In Warn mode // (default) these are non-blocking (logged + appended to the accepted result Message); in Error // mode they fold into the reject list so a typo'd config cannot be re-deployed until fixed. // Running servers are untouched either way — the gate only sees re-deploys. diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/EquipmentTagConfigInspector.cs b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/EquipmentTagConfigInspector.cs index b17ad450..57163a36 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/EquipmentTagConfigInspector.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/AdminOperations/EquipmentTagConfigInspector.cs @@ -17,8 +17,17 @@ namespace ZB.MOM.WW.OtOpcUa.ControlPlane.AdminOperations; /// Deploy-time TagConfig inspection dispatched by DriverType (R2-11, 05/CONV-2). Mirrors the /// AdminUI TagConfigValidator shape: a case-insensitive map from the canonical driver-type string /// to that driver's lightweight Contracts parser Inspect(), returning human-readable -/// warnings (present-but-invalid enum values that the lenient runtime silently defaults, plus -/// structurally-unparseable TagConfig). Never throws. +/// warnings. Never throws. +/// The warnings describe REJECTIONS, not silent defaults. Every driver's runtime read has +/// been strict since R2-11 Phase C (TagConfigJson.TryReadEnumStrict): a present-but-invalid enum +/// value — like an absent or malformed address field, or an unparseable document — makes the reader +/// refuse the whole tag, so the driver skips it and it reads BadNodeIdUnknown forever with the +/// deployment otherwise green. That is what these warnings are for. (Before Phase C the runtime did +/// silently substitute a default for a typo'd enum; the doc-comments here and on the per-driver +/// Inspect methods still said so until Gitea #551.) Because a warning can be PROMOTED to a +/// blocking error by TagConfigValidationMode.Error, each Inspect asks its driver's own +/// reader for the verdict and only explains a refusal — never restating the rule, and so never able to +/// be stricter than the runtime it reports on. /// Coverage is every driver that ships an Inspect, and that invariant is guarded by /// EquipmentTagConfigInspectorCoverageTests, which reflects over the referenced /// *.Contracts assemblies — so a driver that gains an inspector cannot be silently left out. This diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipInspectAcceptGateTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipInspectAcceptGateTests.cs new file mode 100644 index 00000000..725eb52c --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipInspectAcceptGateTests.cs @@ -0,0 +1,100 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Gitea #551 item 3: is accept-gate-first — it +/// asks for the verdict and only explains a +/// refusal. See ModbusInspectAcceptGateTests's remarks for why both directions (never stricter, +/// never silent) are pinned. +/// +/// AbCip's uncovered surface was the declared-UDT members array: the mapper rejects the WHOLE +/// tag for one member with no name or a typo'd dataType, but neither this gate nor the +/// AdminUI editor (which emits only tagPath/dataType/writable and carries +/// members through its preserved key bag) ever looked. +/// +/// +public sealed class AbCipInspectAcceptGateTests +{ + private const string RawPath = "cell/abcip/dev1/T1"; + + /// Blobs the mapper ACCEPTS — the gate must say nothing about any of them. + public static TheoryData Accepted() => + [ + """{"tagPath":"Program:Main.Speed","dataType":"DInt"}""", + """{"tagPath":"Speed"}""", + """{"tagPath":"Motor","dataType":"Structure","members":[{"name":"Run","dataType":"Bool"}]}""", + // members present but not an ARRAY is ignored by the mapper, so it must be ignored here too. + """{"tagPath":"Speed","members":{"name":"Run"}}""", + """{"tagPath":"Speed","members":[]}""", + // A member's writable / writeIdempotent / array keys are all optional. + """{"tagPath":"Motor","members":[{"name":"Run"}]}""", + ]; + + /// Blobs the mapper REJECTS — the gate must produce at least one warning for each. + public static TheoryData Rejected() => + [ + """{"dataType":"DInt"}""", // no tagPath + """{"tagPath":"","dataType":"DInt"}""", // blank tagPath + """{"tagPath":123}""", // tagPath not a string + """{"tagPath":"Speed","dataType":"DIntt"}""", // typo'd top-level dataType + """{"tagPath":"Motor","members":[{"dataType":"Bool"}]}""", // member with no name + """{"tagPath":"Motor","members":[{"name":" ","dataType":"Bool"}]}""", // member with a blank name + """{"tagPath":"Motor","members":[{"name":"Run","dataType":"Booll"}]}""", // typo'd member dataType + """{"tagPath":"Motor","members":["Run"]}""", // member not an object + ]; + + [Theory] + [MemberData(nameof(Accepted))] + public void A_config_the_mapper_accepts_draws_no_warning(string tagConfig) + { + AbCipTagDefinitionFactory.FromTagConfig(tagConfig, RawPath, out _).ShouldBeTrue("fixture must be accepted"); + AbCipTagDefinitionFactory.Inspect(tagConfig).ShouldBeEmpty(); + } + + [Theory] + [MemberData(nameof(Rejected))] + public void A_config_the_mapper_rejects_is_never_passed_over_in_silence(string tagConfig) + { + AbCipTagDefinitionFactory.FromTagConfig(tagConfig, RawPath, out _).ShouldBeFalse("fixture must be rejected"); + AbCipTagDefinitionFactory.Inspect(tagConfig).ShouldNotBeEmpty(); + } + + [Fact] + public void An_absent_tagPath_is_named() + => AbCipTagDefinitionFactory.Inspect("""{"dataType":"DInt"}""") + .ShouldContain(w => w.Contains("'tagPath'")); + + [Fact] + public void A_typo_in_a_member_dataType_is_named_with_its_index() + => AbCipTagDefinitionFactory.Inspect( + """{"tagPath":"Motor","members":[{"name":"Run","dataType":"Bool"},{"name":"Speed","dataType":"Reall"}]}""") + .ShouldContain(w => w.Contains("members[1]") && w.Contains("Reall")); + + [Fact] + public void A_member_with_no_name_is_named_with_its_index() + => AbCipTagDefinitionFactory.Inspect("""{"tagPath":"Motor","members":[{"dataType":"Bool"}]}""") + .ShouldContain(w => w.Contains("members[0]") && w.Contains("name")); + + // ---- DescribeInvalidMember is the shared authority the AdminUI model asks (one rule, two doors) ---- + + [Theory] + [InlineData("""{"tagPath":"Motor","members":[{"name":"Run","dataType":"Bool"}]}""", false)] + [InlineData("""{"tagPath":"Motor"}""", false)] + [InlineData("""{"tagPath":"Motor","members":[]}""", false)] + [InlineData("""{"tagPath":"Motor","members":{"name":"Run"}}""", false)] + [InlineData("not json at all", false)] + [InlineData("""{"tagPath":"Motor","members":[{"name":"Run","dataType":"Booll"}]}""", true)] + [InlineData("""{"tagPath":"Motor","members":[{"dataType":"Bool"}]}""", true)] + public void DescribeInvalidMember_over_a_string_matches_the_mapper(string tagConfig, bool expectWarning) + => (AbCipTagDefinitionFactory.DescribeInvalidMember(tagConfig) is not null).ShouldBe(expectWarning); + + [Fact] + public void A_blank_reference_draws_nothing() + => AbCipTagDefinitionFactory.Inspect(" ").ShouldBeEmpty(); + + [Fact] + public void Unparseable_json_names_the_json() + => AbCipTagDefinitionFactory.Inspect("{oops").ShouldContain(w => w.Contains("not valid JSON")); +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyInspectAcceptGateTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyInspectAcceptGateTests.cs new file mode 100644 index 00000000..ae656596 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyInspectAcceptGateTests.cs @@ -0,0 +1,64 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests; + +/// +/// Gitea #551: is accept-gate-first — it asks +/// for the verdict and only explains a refusal. +/// See ModbusInspectAcceptGateTests's remarks for why both directions (never stricter, never +/// silent) are pinned. +/// +public sealed class AbLegacyInspectAcceptGateTests +{ + private const string RawPath = "cell/ablegacy/dev1/T1"; + + /// Blobs the mapper ACCEPTS — the gate must say nothing about any of them. + public static TheoryData Accepted() => + [ + """{"address":"N7:0","dataType":"Int"}""", + """{"address":"B3:1/2"}""", + """{"address":"F8:5","dataType":"Float","isArray":true,"arrayLength":10}""", + // An over-long array is CLAMPED by the mapper, not rejected — so the gate must stay silent, even + // though the AdminUI model refuses it (the un-clamped length also sets OPC UA ArrayDimensions). + """{"address":"F8:5","dataType":"Float","isArray":true,"arrayLength":9999}""", + ]; + + /// Blobs the mapper REJECTS — the gate must produce at least one warning for each. + public static TheoryData Rejected() => + [ + """{"dataType":"Int"}""", // no address + """{"address":"","dataType":"Int"}""", // blank address + """{"address":7,"dataType":"Int"}""", // address not a string + """{"address":"N7:0","dataType":"Intt"}""", // typo'd dataType + ]; + + [Theory] + [MemberData(nameof(Accepted))] + public void A_config_the_mapper_accepts_draws_no_warning(string tagConfig) + { + AbLegacyTagDefinitionFactory.FromTagConfig(tagConfig, RawPath, out _).ShouldBeTrue("fixture must be accepted"); + AbLegacyTagDefinitionFactory.Inspect(tagConfig).ShouldBeEmpty(); + } + + [Theory] + [MemberData(nameof(Rejected))] + public void A_config_the_mapper_rejects_is_never_passed_over_in_silence(string tagConfig) + { + AbLegacyTagDefinitionFactory.FromTagConfig(tagConfig, RawPath, out _).ShouldBeFalse("fixture must be rejected"); + AbLegacyTagDefinitionFactory.Inspect(tagConfig).ShouldNotBeEmpty(); + } + + [Fact] + public void An_absent_address_is_named() + => AbLegacyTagDefinitionFactory.Inspect("""{"dataType":"Int"}""") + .ShouldContain(w => w.Contains("'address'")); + + [Fact] + public void A_blank_reference_draws_nothing() + => AbLegacyTagDefinitionFactory.Inspect(" ").ShouldBeEmpty(); + + [Fact] + public void Unparseable_json_names_the_json() + => AbLegacyTagDefinitionFactory.Inspect("{oops").ShouldContain(w => w.Contains("not valid JSON")); +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasInspectAcceptGateTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasInspectAcceptGateTests.cs new file mode 100644 index 00000000..8a0da241 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasInspectAcceptGateTests.cs @@ -0,0 +1,98 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests; + +/// +/// Gitea #551: gained an accept gate for its +/// rejection diagnosis — see ModbusInspectAcceptGateTests's remarks for why both +/// directions are pinned. +/// +/// FOCAS is the one factory that deliberately does NOT return empty on accept: two of its warnings +/// are advisories about blobs the mapper ACCEPTS but the driver still cannot serve as +/// authored — an unparseable FOCAS address (#540, dropped at the driver's own init) and an authored +/// writable:true (the wire is read-only). Folding FOCAS into the plain "accept ⇒ empty" +/// shape would have silently deleted both, so they are pinned here alongside the new rejections. +/// +/// +public sealed class FocasInspectAcceptGateTests +{ + private const string RawPath = "cell/focas/dev1/T1"; + + /// Blobs the mapper ACCEPTS and that carry no advisory — the gate must be silent. + public static TheoryData CleanAndAccepted() => + [ + """{"address":"R100","dataType":"Int32"}""", + """{"address":"X0.0","dataType":"Bit"}""", + """{"address":"PARAM:1020"}""", + """{"address":"FIXED:Identity/SeriesNumber","dataType":"String"}""", + ]; + + /// Blobs the mapper REJECTS — the gate must produce at least one warning for each. + public static TheoryData Rejected() => + [ + """{"dataType":"Int32"}""", // no address + """{"address":"","dataType":"Int32"}""", // blank address + """{"address":100}""", // address not a string + """{"address":"R100","dataType":"Intt32"}""", // typo'd dataType + ]; + + [Theory] + [MemberData(nameof(CleanAndAccepted))] + public void A_clean_config_the_mapper_accepts_draws_no_warning(string tagConfig) + { + FocasTagDefinitionFactory.FromTagConfig(tagConfig, RawPath, out _).ShouldBeTrue("fixture must be accepted"); + FocasTagDefinitionFactory.Inspect(tagConfig).ShouldBeEmpty(); + } + + [Theory] + [MemberData(nameof(Rejected))] + public void A_config_the_mapper_rejects_is_never_passed_over_in_silence(string tagConfig) + { + FocasTagDefinitionFactory.FromTagConfig(tagConfig, RawPath, out _).ShouldBeFalse("fixture must be rejected"); + FocasTagDefinitionFactory.Inspect(tagConfig).ShouldNotBeEmpty(); + } + + [Fact] + public void An_absent_address_is_named() + => FocasTagDefinitionFactory.Inspect("""{"dataType":"Int32"}""") + .ShouldContain(w => w.Contains("'address'")); + + // ---- The two advisories the accept gate must NOT swallow ---- + + [Fact] + public void An_unparseable_address_still_warns_even_though_the_mapper_accepts_it() + { + const string Blob = """{"address":"not-an-address","dataType":"Int32"}"""; + FocasTagDefinitionFactory.FromTagConfig(Blob, RawPath, out _).ShouldBeTrue("the mapper accepts any non-blank address"); + FocasTagDefinitionFactory.Inspect(Blob).ShouldContain(w => w.Contains("not parseable")); + } + + [Fact] + public void An_authored_writable_still_warns_even_though_the_mapper_accepts_it() + { + const string Blob = """{"address":"R100","dataType":"Int32","writable":true}"""; + FocasTagDefinitionFactory.FromTagConfig(Blob, RawPath, out _).ShouldBeTrue("the mapper accepts writable:true"); + FocasTagDefinitionFactory.Inspect(Blob).ShouldContain(w => w.Contains("read-only")); + } + + /// A rejected blob still gets its advisories — the two diagnoses are independent. + [Fact] + public void A_rejected_config_reports_the_rejection_and_the_advisory_together() + { + var warnings = FocasTagDefinitionFactory.Inspect( + """{"address":"not-an-address","dataType":"Intt32","writable":true}"""); + + warnings.ShouldContain(w => w.Contains("dataType")); + warnings.ShouldContain(w => w.Contains("not parseable")); + warnings.ShouldContain(w => w.Contains("read-only")); + } + + [Fact] + public void A_blank_reference_draws_nothing() + => FocasTagDefinitionFactory.Inspect(" ").ShouldBeEmpty(); + + [Fact] + public void Unparseable_json_names_the_json() + => FocasTagDefinitionFactory.Inspect("{oops").ShouldContain(w => w.Contains("not valid JSON")); +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusInspectAcceptGateTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusInspectAcceptGateTests.cs new file mode 100644 index 00000000..7ecfd2bb --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusInspectAcceptGateTests.cs @@ -0,0 +1,128 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests; + +/// +/// Gitea #551 items 2/3: is accept-gate-first — +/// it asks for the verdict and only explains a +/// refusal. +/// +/// Two properties, and they pull in opposite directions, which is why both are pinned here. The +/// gate must never be stricter than the mapper (a warning can be promoted to a blocking +/// deploy error by TagConfigValidationMode.Error, so a warning on a blob the driver serves +/// would refuse a working config); and it must never be silent on a blob the mapper refuses +/// (that tag deploys green and then reads BadNodeIdUnknown forever). The previous hand-list +/// of enum checks satisfied the first and failed the second for every cause not on the list — +/// an absent region, a bad address, stringByteOrder, and the two +/// type-dependent guards. +/// +/// +public sealed class ModbusInspectAcceptGateTests +{ + private const string RawPath = "cell/modbus/dev1/T1"; + private const string Clean = """{"region":"HoldingRegisters","address":1,"dataType":"Int16"}"""; + + /// Every blob below is one the mapper ACCEPTS, so the gate must say nothing about it. + public static TheoryData Accepted() => + [ + Clean, + """{"region":"Coils","address":0,"dataType":"Bool"}""", + """{"region":"HoldingRegisters","address":65535,"dataType":"String","stringLength":10,"stringByteOrder":"LowByteFirst"}""", + """{"region":"HoldingRegisters","address":1,"dataType":"BitInRegister","bitIndex":15}""", + // The byte/ushort NARROWING is part of the mapper's rule: bitIndex 256 truncates to bit 0 and is + // ACCEPTED. A gate that described the un-narrowed value would refuse a tag the driver serves. + """{"region":"HoldingRegisters","address":1,"dataType":"BitInRegister","bitIndex":256}""", + // A non-string enum token reads as ABSENT, not invalid — the mapper accepts it as the default. + """{"region":"HoldingRegisters","address":1,"dataType":3}""", + ]; + + /// Every blob below is one the mapper REJECTS, so the gate must produce at least one warning. + public static TheoryData Rejected() => + [ + """{"address":1,"dataType":"Int16"}""", // no region + """{"region":"HoldingRegisters","dataType":"Int16"}""", // no address + """{"region":"HoldingRegisters","address":-1,"dataType":"Int16"}""", // address < 0 + """{"region":"HoldingRegisters","address":65536,"dataType":"Int16"}""", // address > ushort + """{"region":"HoldingRegisters","address":"1","dataType":"Int16"}""", // address not a number + """{"region":"HoldingRegisterz","address":1,"dataType":"Int16"}""", // typo'd region + """{"region":"HoldingRegisters","address":1,"dataType":"Intt16"}""", // typo'd dataType + """{"region":"HoldingRegisters","address":1,"dataType":"Int16","byteOrder":"Bigg"}""", // typo'd byteOrder + """{"region":"HoldingRegisters","address":1,"dataType":"Int16","stringByteOrder":"Nope"}""", // typo'd stringByteOrder + """{"region":"HoldingRegisters","address":1,"dataType":"String"}""", // String with no length + """{"region":"HoldingRegisters","address":1,"dataType":"BitInRegister","bitIndex":16}""", // bit out of range + ]; + + [Theory] + [MemberData(nameof(Accepted))] + public void A_config_the_mapper_accepts_draws_no_warning(string tagConfig) + { + ModbusTagDefinitionFactory.FromTagConfig(tagConfig, RawPath, out _).ShouldBeTrue("fixture must be accepted"); + ModbusTagDefinitionFactory.Inspect(tagConfig).ShouldBeEmpty(); + } + + [Theory] + [MemberData(nameof(Rejected))] + public void A_config_the_mapper_rejects_is_never_passed_over_in_silence(string tagConfig) + { + ModbusTagDefinitionFactory.FromTagConfig(tagConfig, RawPath, out _).ShouldBeFalse("fixture must be rejected"); + ModbusTagDefinitionFactory.Inspect(tagConfig).ShouldNotBeEmpty(); + } + + // ---- The specific causes the issue named, each with a message that points at the right key ---- + + [Fact] + public void An_absent_region_is_named() + => ModbusTagDefinitionFactory.Inspect("""{"address":1,"dataType":"Int16"}""") + .ShouldContain(w => w.Contains("'region'")); + + [Fact] + public void An_out_of_range_address_is_named() + => ModbusTagDefinitionFactory.Inspect("""{"region":"Coils","address":70000,"dataType":"Int16"}""") + .ShouldContain(w => w.Contains("'address'")); + + /// Gitea #551 item 3: read strictly by the mapper since R2-11, checked by nothing until now. + [Fact] + public void A_typo_in_stringByteOrder_is_named() + => ModbusTagDefinitionFactory.Inspect( + """{"region":"HoldingRegisters","address":1,"dataType":"Int16","stringByteOrder":"HighByteFrist"}""") + .ShouldContain(w => w.Contains("stringByteOrder") && w.Contains("HighByteFrist")); + + [Fact] + public void A_String_tag_with_no_stringLength_is_named() + => ModbusTagDefinitionFactory.Inspect("""{"region":"HoldingRegisters","address":1,"dataType":"String"}""") + .ShouldContain(w => w.Contains("stringLength")); + + [Fact] + public void A_BitInRegister_tag_with_a_bit_over_15_is_named() + => ModbusTagDefinitionFactory.Inspect( + """{"region":"HoldingRegisters","address":1,"dataType":"BitInRegister","bitIndex":16}""") + .ShouldContain(w => w.Contains("bitIndex")); + + // ---- Framing rules preserved from the pre-#551 shape ---- + + [Fact] + public void A_blank_reference_draws_nothing() + => ModbusTagDefinitionFactory.Inspect(" ").ShouldBeEmpty(); + + /// + /// The pre-existing framing convention, deliberately left alone by #551 and shared by all eleven + /// inspectors: a reference that does not START with { is not an equipment-tag TagConfig at + /// all, so it draws nothing — even though the mapper would refuse it. Widening this is a separate + /// decision about what a TagConfig IS, not about how a refusal is explained. + /// + /// The non-object reference. + [Theory] + [InlineData("just a string")] + [InlineData("[1,2,3]")] + [InlineData("42")] + public void A_non_object_reference_draws_nothing_because_it_is_not_a_TagConfig(string reference) + { + ModbusTagDefinitionFactory.FromTagConfig(reference, RawPath, out _).ShouldBeFalse(); + ModbusTagDefinitionFactory.Inspect(reference).ShouldBeEmpty(); + } + + [Fact] + public void Unparseable_json_names_the_json() + => ModbusTagDefinitionFactory.Inspect("{oops").ShouldContain(w => w.Contains("not valid JSON")); +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttInspectAcceptGateTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttInspectAcceptGateTests.cs new file mode 100644 index 00000000..2750f36d --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttInspectAcceptGateTests.cs @@ -0,0 +1,88 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests; + +/// +/// Gitea #551: gained an accept gate — see +/// ModbusInspectAcceptGateTests's remarks for why both directions (never stricter, never silent) +/// are pinned. +/// +/// MQTT's gate is the UNION of its two mappers, because Inspect is handed a blob with +/// no mode: the driver's MqttMode decides whether +/// or +/// reads it, and nothing on a tag row +/// says which. Gating on the Plain mapper alone would warn that every correctly-authored Sparkplug +/// tag has no topic — the falsification control below is exactly that case. +/// +/// +public sealed class MqttInspectAcceptGateTests +{ + private const string RawPath = "cell/mqtt/dev1/T1"; + + private static bool EitherModeAccepts(string tagConfig) + => MqttTagDefinitionFactory.FromTagConfig(tagConfig, RawPath, out _) + || MqttTagDefinitionFactory.FromSparkplugTagConfig(tagConfig, RawPath, out _); + + /// Blobs at least one mapper ACCEPTS, with no wildcard advisory — the gate must be silent. + public static TheoryData Accepted() => + [ + """{"topic":"plant/line1/temp"}""", + """{"topic":"plant/line1/temp","payloadFormat":"Json","dataType":"Float64","qos":1}""", + // Sparkplug: no topic at all, and that is CORRECT — the control this whole union exists for. + """{"groupId":"OtOpcUaSim","edgeNodeId":"EdgeA","metricName":"Temp"}""", + """{"groupId":"OtOpcUaSim","edgeNodeId":"EdgeA","deviceId":"Filler1","metricName":"Node Control/Rebirth"}""", + ]; + + /// Blobs NEITHER mapper accepts — the gate must produce at least one warning for each. + public static TheoryData Rejected() => + [ + "{}", // no address of either shape + """{"payloadFormat":"Json","dataType":"String"}""", // still no address + """{"groupId":"G","edgeNodeId":"E"}""", // incomplete Sparkplug tuple + """{"topic":"a/b","payloadFormat":"Jsonn"}""", // typo'd payloadFormat + """{"topic":"a/b","dataType":"Flooat64"}""", // typo'd dataType + """{"topic":"a/b","qos":5}""", // out-of-range qos + """{"groupId":"G","edgeNodeId":"E","metricName":"M","qos":"high"}""", // non-numeric qos, both modes + ]; + + [Theory] + [MemberData(nameof(Accepted))] + public void A_config_a_mapper_accepts_draws_no_warning(string tagConfig) + { + EitherModeAccepts(tagConfig).ShouldBeTrue("fixture must be accepted by at least one mode"); + MqttTagDefinitionFactory.Inspect(tagConfig).ShouldBeEmpty(); + } + + [Theory] + [MemberData(nameof(Rejected))] + public void A_config_neither_mapper_accepts_is_never_passed_over_in_silence(string tagConfig) + { + EitherModeAccepts(tagConfig).ShouldBeFalse("fixture must be rejected by both modes"); + MqttTagDefinitionFactory.Inspect(tagConfig).ShouldNotBeEmpty(); + } + + [Fact] + public void A_blob_with_no_address_of_either_shape_names_both() + { + var warnings = MqttTagDefinitionFactory.Inspect("{}"); + warnings.ShouldContain(w => w.Contains("topic") && w.Contains("metricName")); + } + + /// The wildcard advisory is about an ACCEPTED blob, so the accept gate must not swallow it. + [Fact] + public void A_wildcard_topic_still_warns_even_though_the_mapper_accepts_it() + { + const string Blob = """{"topic":"plant/+/temp"}"""; + MqttTagDefinitionFactory.FromTagConfig(Blob, RawPath, out _).ShouldBeTrue("a wildcard topic is accepted on purpose"); + MqttTagDefinitionFactory.Inspect(Blob).ShouldContain(w => w.Contains("wildcard")); + } + + [Fact] + public void A_blank_reference_draws_nothing() + => MqttTagDefinitionFactory.Inspect(" ").ShouldBeEmpty(); + + [Fact] + public void Unparseable_json_names_the_json() + => MqttTagDefinitionFactory.Inspect("{oops").ShouldContain(w => w.Contains("not valid JSON")); +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7InspectAcceptGateTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7InspectAcceptGateTests.cs new file mode 100644 index 00000000..af0913f4 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7.Tests/S7InspectAcceptGateTests.cs @@ -0,0 +1,98 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.S7.Tests; + +/// +/// Gitea #551 item 2: is accept-gate-first — it asks +/// for the verdict and only explains a refusal. See +/// ModbusInspectAcceptGateTests's remarks for why both directions (never stricter, never silent) +/// are pinned. +/// +/// S7 carries the fleet's one unique rule: the reader REQUIRES the dataType key to be +/// present. A blob with only an address therefore deploys green and reads +/// BadNodeIdUnknown forever — and neither this gate nor the AdminUI model said a word about +/// it until #551. +/// +/// +public sealed class S7InspectAcceptGateTests +{ + private const string RawPath = "cell/s7/dev1/T1"; + + /// Blobs the mapper ACCEPTS — the gate must say nothing about any of them. + public static TheoryData Accepted() => + [ + """{"address":"DB1.DBW0","dataType":"Int16"}""", + """{"address":"M0.0","dataType":"Bool"}""", + """{"address":"DB1.DBB0","dataType":"String","stringLength":254}""", + // stringLength is irrelevant to a non-String tag, so an out-of-range remnant is ACCEPTED. + """{"address":"DB1.DBW0","dataType":"Int16","stringLength":9999}""", + // A non-string dataType token satisfies the PRESENCE rule and then reads as absent ⇒ Int16. + """{"address":"DB1.DBW0","dataType":7}""", + ]; + + /// Blobs the mapper REJECTS — the gate must produce at least one warning for each. + public static TheoryData Rejected() => + [ + """{"dataType":"Int16"}""", // no address + """{"address":"","dataType":"Int16"}""", // blank address + """{"address":123,"dataType":"Int16"}""", // address not a string + """{"address":"DB1.DBW0"}""", // no dataType key at all (#551) + """{"address":"DB1.DBW0","dataType":"Intt16"}""", // typo'd dataType + """{"address":"DB1.DBB0","dataType":"String","stringLength":255}""", // over the 254 S7 limit + """{"address":"DB1.DBB0","dataType":"String","stringLength":-1}""", // negative length + ]; + + [Theory] + [MemberData(nameof(Accepted))] + public void A_config_the_mapper_accepts_draws_no_warning(string tagConfig) + { + S7TagDefinitionFactory.FromTagConfig(tagConfig, RawPath, out _).ShouldBeTrue("fixture must be accepted"); + S7TagDefinitionFactory.Inspect(tagConfig).ShouldBeEmpty(); + } + + [Theory] + [MemberData(nameof(Rejected))] + public void A_config_the_mapper_rejects_is_never_passed_over_in_silence(string tagConfig) + { + S7TagDefinitionFactory.FromTagConfig(tagConfig, RawPath, out _).ShouldBeFalse("fixture must be rejected"); + S7TagDefinitionFactory.Inspect(tagConfig).ShouldNotBeEmpty(); + } + + /// The headline case: present-and-typo'd was reported; ABSENT was not (Gitea #551 item 2). + [Fact] + public void An_absent_dataType_is_named() + => S7TagDefinitionFactory.Inspect("""{"address":"DB1.DBW0"}""") + .ShouldContain(w => w.Contains("no 'dataType'")); + + [Fact] + public void An_absent_address_is_named() + => S7TagDefinitionFactory.Inspect("""{"dataType":"Int16"}""") + .ShouldContain(w => w.Contains("'address'")); + + [Fact] + public void An_out_of_range_string_length_is_named() + => S7TagDefinitionFactory.Inspect("""{"address":"DB1.DBB0","dataType":"String","stringLength":255}""") + .ShouldContain(w => w.Contains("stringLength")); + + // ---- DeclaresDataType is the shared authority the AdminUI model asks (one rule, two doors) ---- + + [Theory] + [InlineData("""{"dataType":"Int16"}""", true)] + [InlineData("""{"dataType":7}""", true)] // presence is judged on the KEY, not the value + [InlineData("""{"dataType":null}""", true)] + [InlineData("""{"address":"DB1.DBW0"}""", false)] + public void DeclaresDataType_judges_the_key_not_the_value(string tagConfig, bool expected) + { + using var doc = System.Text.Json.JsonDocument.Parse(tagConfig); + S7TagDefinitionFactory.DeclaresDataType(doc.RootElement).ShouldBe(expected); + } + + [Fact] + public void A_blank_reference_draws_nothing() + => S7TagDefinitionFactory.Inspect(" ").ShouldBeEmpty(); + + [Fact] + public void Unparseable_json_names_the_json() + => S7TagDefinitionFactory.Inspect("{oops").ShouldContain(w => w.Contains("not valid JSON")); +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATInspectAcceptGateTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATInspectAcceptGateTests.cs new file mode 100644 index 00000000..468e40e0 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests/TwinCATInspectAcceptGateTests.cs @@ -0,0 +1,71 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.TwinCAT.Tests; + +/// +/// Gitea #551: is accept-gate-first — it asks +/// for the verdict and only explains a refusal. +/// See ModbusInspectAcceptGateTests's remarks for why both directions (never stricter, never +/// silent) are pinned. TwinCAT's uncovered rejections were an absent symbolPath and the +/// unsupported Structure data type — a VALID enum name that the mapper still refuses. +/// +public sealed class TwinCATInspectAcceptGateTests +{ + private const string RawPath = "cell/twincat/dev1/T1"; + + /// Blobs the mapper ACCEPTS — the gate must say nothing about any of them. + public static TheoryData Accepted() => + [ + """{"symbolPath":"MAIN.bStart","dataType":"Bool"}""", + """{"symbolPath":"MAIN.nCount"}""", + """{"symbolPath":"MAIN.aData","dataType":"DInt","isArray":true,"arrayLength":8}""", + // A stale arrayLength behind a cleared isArray is scalar, not a rejection. + """{"symbolPath":"MAIN.nCount","dataType":"DInt","arrayLength":8}""", + ]; + + /// Blobs the mapper REJECTS — the gate must produce at least one warning for each. + public static TheoryData Rejected() => + [ + """{"dataType":"Bool"}""", // no symbolPath + """{"symbolPath":"","dataType":"Bool"}""", // blank symbolPath + """{"symbolPath":42}""", // symbolPath not a string + """{"symbolPath":"MAIN.x","dataType":"Booll"}""", // typo'd dataType + """{"symbolPath":"MAIN.udt","dataType":"Structure"}""", // valid NAME the driver still refuses + ]; + + [Theory] + [MemberData(nameof(Accepted))] + public void A_config_the_mapper_accepts_draws_no_warning(string tagConfig) + { + TwinCATTagDefinitionFactory.FromTagConfig(tagConfig, RawPath, out _).ShouldBeTrue("fixture must be accepted"); + TwinCATTagDefinitionFactory.Inspect(tagConfig).ShouldBeEmpty(); + } + + [Theory] + [MemberData(nameof(Rejected))] + public void A_config_the_mapper_rejects_is_never_passed_over_in_silence(string tagConfig) + { + TwinCATTagDefinitionFactory.FromTagConfig(tagConfig, RawPath, out _).ShouldBeFalse("fixture must be rejected"); + TwinCATTagDefinitionFactory.Inspect(tagConfig).ShouldNotBeEmpty(); + } + + [Fact] + public void An_absent_symbolPath_is_named() + => TwinCATTagDefinitionFactory.Inspect("""{"dataType":"Bool"}""") + .ShouldContain(w => w.Contains("'symbolPath'")); + + /// A rejection an enum-spelling check can never see: the NAME parses, the driver still refuses. + [Fact] + public void A_Structure_typed_tag_is_named() + => TwinCATTagDefinitionFactory.Inspect("""{"symbolPath":"MAIN.udt","dataType":"Structure"}""") + .ShouldContain(w => w.Contains("Structure")); + + [Fact] + public void A_blank_reference_draws_nothing() + => TwinCATTagDefinitionFactory.Inspect(" ").ShouldBeEmpty(); + + [Fact] + public void Unparseable_json_names_the_json() + => TwinCATTagDefinitionFactory.Inspect("{oops").ShouldContain(w => w.Contains("not valid JSON")); +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTagConfigGateTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTagConfigGateTests.cs new file mode 100644 index 00000000..5305df94 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTagConfigGateTests.cs @@ -0,0 +1,361 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.AdminUI.Uns; +using ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors; +using ZB.MOM.WW.OtOpcUa.Configuration.Enums; +using ZB.MOM.WW.OtOpcUa.ControlPlane.AdminOperations; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip; +using ZB.MOM.WW.OtOpcUa.Driver.Modbus; +using ZB.MOM.WW.OtOpcUa.Driver.S7; +using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; + +namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns; + +/// +/// Gitea #551 — the authoring half of the four residuals, and the seam that makes it a guarantee. +/// +/// +/// The CSV import runs the driver-typed gate. Its typed columns were always strictly +/// parsed, but the free-form TagConfigJson column reached the database unread. It is +/// checked twice on purpose: in so the staged review grid +/// NAMES the offending row before commit, and in +/// because a grid cannot cover the API or a future caller — and this repo has no bUnit, so a +/// rule only a page enforces is a rule no test can reach. Deleting the service check leaves the +/// mapper tests green and turns the service tests red; that asymmetry is the point. +/// +/// +/// A rejected row is an ERROR, not a skip. The import is all-or-nothing: nothing is +/// inserted and the row is named. "Skipped" would claim the tag is already present. +/// +/// +/// Modbus stringByteOrder, AbCip members[].dataType and the S7 absent +/// dataType — three rules the runtime has always enforced that no authoring surface +/// checked. +/// +/// +/// Every rejection below is paired with an acceptance control, so a gate that simply refuses everything +/// cannot pass this suite. +/// +[Trait("Category", "Unit")] +public sealed class RawTagConfigGateTests +{ + // A blob the S7 driver actually reads — RootDeviceId hangs off the seed's S7 driver. + private const string GoodS7 = """{"address":"DB1.DBW0","dataType":"Int16"}"""; + + private static (RawTreeService Service, string DbName) Seeded() + { + var name = RawTreeTestDb.SeedFresh(); + return (new RawTreeService(RawTreeTestDb.Factory(name)), name); + } + + private static RawTagInput Tag(string name, string tagConfig) => + new(name, "Float", TagAccessLevel.Read, WriteIdempotent: false, PollGroupId: null, tagConfig); + + // ------------------------------------------------------------------ 1. the AdminUI models (item 2/3) + + /// Gitea #551 item 3: the runtime reads it strictly; nothing on the authoring side did. + [Fact] + public void Modbus_rejects_a_typo_in_stringByteOrder() + => ModbusTagConfigModel + .FromJson("""{"region":"HoldingRegisters","address":1,"dataType":"Int16","stringByteOrder":"HighByteFrist"}""") + .Validate() + .ShouldNotBeNullOrEmpty(); + + [Fact] + public void Modbus_accepts_a_correctly_spelled_stringByteOrder() + => ModbusTagConfigModel + .FromJson("""{"region":"HoldingRegisters","address":1,"dataType":"Int16","stringByteOrder":"LowByteFirst"}""") + .Validate() + .ShouldBeNull(); + + /// Gitea #551 item 3: members ride the model's preserved key bag, unread until now. + [Fact] + public void AbCip_rejects_a_typo_in_a_member_dataType() + => AbCipTagConfigModel + .FromJson("""{"tagPath":"Motor","members":[{"name":"Run","dataType":"Booll"}]}""") + .Validate() + .ShouldNotBeNullOrEmpty(); + + [Fact] + public void AbCip_rejects_a_member_with_no_name() + => AbCipTagConfigModel + .FromJson("""{"tagPath":"Motor","members":[{"dataType":"Bool"}]}""") + .Validate() + .ShouldNotBeNullOrEmpty(); + + [Fact] + public void AbCip_accepts_well_formed_members() + => AbCipTagConfigModel + .FromJson("""{"tagPath":"Motor","members":[{"name":"Run","dataType":"Bool"},{"name":"Speed","dataType":"Real"}]}""") + .Validate() + .ShouldBeNull(); + + /// Gitea #551 item 2: S7 is the only driver whose reader requires the key to be PRESENT. + [Fact] + public void S7_rejects_a_config_carrying_no_dataType_key() + => S7TagConfigModel.FromJson("""{"address":"DB1.DBW0"}""").Validate().ShouldNotBeNullOrEmpty(); + + [Fact] + public void S7_accepts_a_config_carrying_a_dataType_key() + => S7TagConfigModel.FromJson(GoodS7).Validate().ShouldBeNull(); + + /// + /// The absent-key rule records a property of the SOURCE blob, so a model built by object initialiser + /// — which is what does — must validate clean: its + /// ToJson() writes the key from the typed field. Getting this backwards would refuse every + /// browse-committed S7 tag. + /// + [Fact] + public void S7_accepts_a_model_that_was_built_rather_than_loaded() + => new S7TagConfigModel { Address = "DB1.DBW0" }.Validate().ShouldBeNull(); + + /// A load→save through the editor always emits the key, so the round-trip stays authorable. + [Fact] + public void S7_round_trip_through_the_model_repairs_a_key_less_blob() + { + var model = S7TagConfigModel.FromJson("""{"address":"DB1.DBW0"}"""); + model.Validate().ShouldNotBeNullOrEmpty(); + + S7TagConfigModel.FromJson(model.ToJson()).Validate().ShouldBeNull(); + } + + // ------------------------------- 2. authoring ⇔ deploy-gate parity for the three new rules (item 2/3) + + /// + /// The pairing that matters: a blob the authoring gate refuses must draw a deploy warning, and a blob + /// it accepts must draw none. Either direction alone is satisfiable by a broken gate — refuse + /// everything, or refuse nothing. + /// + /// The driver type to dispatch both gates on. + /// The blob under test. + /// Whether both gates should refuse it. + [Theory] + [InlineData(DriverTypeNames.Modbus, """{"region":"HoldingRegisters","address":1,"dataType":"Int16","stringByteOrder":"Nope"}""", true)] + [InlineData(DriverTypeNames.Modbus, """{"region":"HoldingRegisters","address":1,"dataType":"Int16","stringByteOrder":"LowByteFirst"}""", false)] + [InlineData(DriverTypeNames.AbCip, """{"tagPath":"Motor","members":[{"name":"Run","dataType":"Booll"}]}""", true)] + [InlineData(DriverTypeNames.AbCip, """{"tagPath":"Motor","members":[{"name":"Run","dataType":"Bool"}]}""", false)] + [InlineData(DriverTypeNames.S7, """{"address":"DB1.DBW0"}""", true)] + [InlineData(DriverTypeNames.S7, """{"address":"DB1.DBW0","dataType":"Int16"}""", false)] + public void Authoring_and_the_deploy_gate_agree(string driverType, string tagConfig, bool expectRejected) + { + var authoringError = TagConfigValidator.Validate(driverType, tagConfig); + var deployWarnings = EquipmentTagConfigInspector.Inspect(driverType, tagConfig); + + (authoringError is not null).ShouldBe(expectRejected, $"authoring verdict for {driverType}"); + (deployWarnings.Count > 0).ShouldBe(expectRejected, $"deploy verdict for {driverType}"); + } + + // ---------------------------------------------- 3. the CSV review grid names the row (item 1, surface) + + /// + /// The gate runs on the FINAL assembled blob — the bytes that reach the database — so the fixture + /// puts the offending value in a key the Modbus typed map does NOT own (stringByteOrder, + /// #551 item 3). A key the map DOES own is round-tripped through + /// ModbusTagConfigModel.FromJson/ToJson by ApplyTypedColumns and silently normalised + /// to the model default before it can ever be judged — a separate, pre-existing defect noted with + /// this work, not one this gate can see. + /// + [Fact] + public void Csv_parse_marks_a_row_whose_TagConfigJson_the_driver_would_reject() + { + const string Csv = """ + Name,DataType,TagConfigJson + good,Float,"{""region"":""HoldingRegisters"",""address"":1,""stringByteOrder"":""LowByteFirst""}" + bad,Float,"{""region"":""HoldingRegisters"",""address"":1,""stringByteOrder"":""Nope""}" + """; + + var result = RawTagCsvMapper.Parse(DriverTypeNames.Modbus, Csv); + + result.FatalError.ShouldBeNull(); + result.Rows.Count.ShouldBe(2); + result.Rows[0].Ok.ShouldBeTrue("the clean row must still import"); + result.Rows[1].Ok.ShouldBeFalse(); + result.Rows[1].Error.ShouldNotBeNull(); + result.Rows[1].Error!.ShouldContain("Nope"); + result.CanCommit.ShouldBeFalse("the commit is blocked while any row is invalid"); + result.ToImportRows().Count.ShouldBe(1); + } + + /// + /// The acceptance control for the driver that has NO typed CSV map, where the fallback cell reaches + /// the database verbatim — the shape the gate most needed to cover. + /// + [Fact] + public void Csv_parse_judges_a_map_less_drivers_fallback_cell_verbatim() + { + const string Good = """ + Name,DataType,TagConfigJson + ok,Double,"{""nodeId"":""ns=2;s=Line3.Temp""}" + """; + const string Bad = """ + Name,DataType,TagConfigJson + broken,Double,"{""node_id"":""ns=2;s=Line3.Temp""}" + """; + + RawTagCsvMapper.Parse(DriverTypeNames.OpcUaClient, Good).CanCommit.ShouldBeTrue(); + + var bad = RawTagCsvMapper.Parse(DriverTypeNames.OpcUaClient, Bad); + bad.CanCommit.ShouldBeFalse(); + bad.Rows[0].Error.ShouldNotBeNull(); + } + + /// + /// Positive control for the half that already worked: a typed column's value has always been + /// strictly parsed (a bad enum throws FormatException into the row's error), so this row was + /// rejected before #551 too. It is here so a reader can tell which half of the CSV path was the gap. + /// + [Fact] + public void Csv_parse_still_marks_a_row_whose_TYPED_column_is_unparseable() + { + const string Csv = """ + Name,DataType,Region,Address,ModbusDataType + bad,Float,HoldingRegisters,1,Intt16 + """; + + var result = RawTagCsvMapper.Parse(DriverTypeNames.Modbus, Csv); + + result.Rows.Count.ShouldBe(1); + result.Rows[0].Ok.ShouldBeFalse(); + } + + // ------------------------------------- 4. the SERVICE is the guarantee (item 1, the enforcement point) + + /// + /// Called with rows the CSV mapper never saw — which is exactly the shape of an API caller. Delete + /// the check in ImportTagsAsync and this goes red while every mapper test above stays green. + /// + [Fact] + public async Task ImportTags_fails_the_whole_batch_and_names_the_row_with_a_rejected_config() + { + var (service, dbName) = Seeded(); + + var rows = new[] + { + new RawTagImportRow(null, Tag("good1", GoodS7)), + new RawTagImportRow(null, Tag("bad", """{"address":"DB1.DBW0"}""")), // no dataType key ⇒ S7 rejects + new RawTagImportRow(null, Tag("good2", GoodS7)), + }; + + var result = await service.ImportTagsAsync(RawTreeTestDb.RootDeviceId, rows); + + result.Inserted.ShouldBe(0); + result.Errors.ShouldContain(e => e.Contains("Row 2") && e.Contains("TagConfig")); + + // All-or-nothing: the two good rows are not quietly kept either. + using var db = RawTreeTestDb.CreateNamed(dbName); + db.Tags.Any(t => t.Name == "good1" || t.Name == "good2").ShouldBeFalse(); + } + + /// The acceptance control — without it, a gate that refuses every import would pass. + [Fact] + public async Task ImportTags_still_imports_a_batch_the_driver_can_read() + { + var (service, _) = Seeded(); + + var result = await service.ImportTagsAsync( + RawTreeTestDb.RootDeviceId, + new[] { new RawTagImportRow(null, Tag("ok1", GoodS7)), new RawTagImportRow(null, Tag("ok2", GoodS7)) }); + + result.Errors.ShouldBeEmpty(); + result.Inserted.ShouldBe(2); + } + + /// + /// Manual entry commits row-by-row through CreateTagAsync, NOT through + /// ImportTagsAsync — so the gate has to live on that path too, or the "enforce in the + /// service" claim covers only the CSV modal. + /// + [Fact] + public async Task CreateTag_refuses_a_config_the_driver_would_reject() + { + var (service, _) = Seeded(); + + var bad = await service.CreateTagAsync( + RawTreeTestDb.RootDeviceId, null, Tag("newTag", """{"address":"DB1.DBW0"}""")); + + bad.Ok.ShouldBeFalse(); + bad.Error.ShouldNotBeNull(); + bad.Error.ShouldContain("TagConfig"); + } + + [Fact] + public async Task CreateTag_accepts_a_config_the_driver_can_read() + { + var (service, _) = Seeded(); + + (await service.CreateTagAsync(RawTreeTestDb.RootDeviceId, null, Tag("newTag", GoodS7))) + .Ok.ShouldBeTrue(); + } + + [Fact] + public async Task UpdateTag_refuses_a_config_the_driver_would_reject() + { + var (service, dbName) = Seeded(); + byte[] rowVersion; + using (var db = RawTreeTestDb.CreateNamed(dbName)) + { + rowVersion = db.Tags.Single(t => t.TagId == RawTreeTestDb.ReferencedTagId).RowVersion; + } + + var bad = await service.UpdateTagAsync( + RawTreeTestDb.ReferencedTagId, Tag("temperature", """{"address":"DB1.DBW0"}"""), rowVersion); + + bad.Ok.ShouldBeFalse(); + bad.Error.ShouldNotBeNull(); + bad.Error.ShouldContain("TagConfig"); + } + + /// + /// A driver type with no typed model (Galaxy) validates to null in the modals, so the service must + /// not invent a rule for it either — otherwise adding this gate would make Galaxy tags unauthorable. + /// + [Fact] + public void An_unmapped_driver_type_is_left_alone() + { + TagConfigValidator.Validate(DriverTypeNames.Galaxy, """{"FullName":"Reactor_001.Temp"}""").ShouldBeNull(); + TagConfigValidator.Validate(DriverTypeNames.Galaxy, "{}").ShouldBeNull(); + } + + /// + /// Browse-commit goes through , so every blob + /// emits must survive the gate — otherwise the + /// commit would author rows the import refuses. + /// + /// Sql is deliberately absent, and that is a FINDING, not an oversight. Its branch + /// emits {"model":"WideRow","table":…,"columnName":…} with no rowSelector, which + /// SqlTagConfigModel.Validate refuses and SqlEquipmentTagParser refuses too — so a + /// browse-committed Sql tag has never been readable, it merely failed later (at the deploy gate, + /// or silently at the driver) instead of at commit. Pinning today's behaviour either way would + /// encode a defect; it is reported for its own fix (the browse session should state a row + /// selector, or Sql should commit through the typed editor). + /// + /// + /// The browsable driver type. + [Theory] + [InlineData(DriverTypeNames.S7)] + [InlineData(DriverTypeNames.AbCip)] + [InlineData(DriverTypeNames.AbLegacy)] + [InlineData(DriverTypeNames.TwinCAT)] + [InlineData(DriverTypeNames.OpcUaClient)] + [InlineData(DriverTypeNames.MTConnect)] + [InlineData(DriverTypeNames.Mqtt)] + [InlineData(DriverTypeNames.Galaxy)] + public void A_browse_committed_config_passes_the_gate(string driverType) + { + var tagConfig = RawBrowseCommitMapper.BuildTagConfig(driverType, "SomeAddress"); + + TagConfigValidator.Validate(driverType, tagConfig).ShouldBeNull( + $"browse-commit for {driverType} must not author a config the import refuses"); + } + + /// Sanity: the enums the three new rules read are the driver's own, not a local copy. + [Fact] + public void The_new_rules_read_the_drivers_own_enums() + { + Enum.GetNames().ShouldContain("LowByteFirst"); + Enum.GetNames().ShouldContain("Bool"); + Enum.GetNames().ShouldContain("Int16"); + Enum.GetNames().ShouldContain("Structure"); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTreeServiceWaveBTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTreeServiceWaveBTests.cs index 2b7d8ad5..77e0e51f 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTreeServiceWaveBTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTreeServiceWaveBTests.cs @@ -34,6 +34,19 @@ public sealed class RawTreeServiceWaveBTests private static RawTagInput Tag(string name, string dataType = "Float", string tagConfig = "{}") => new(name, dataType, TagAccessLevel.ReadWrite, WriteIdempotent: false, PollGroupId: null, tagConfig); + /// + /// A tag input whose TagConfig the S7 driver would actually read — RootDeviceId + /// hangs off the seed's S7 driver, and since Gitea #551 the service runs the driver-typed + /// TagConfigValidator on every tag it writes. S7 requires BOTH an address and a + /// present dataType key, so the bare {} the sibling helper defaults to is a config the + /// driver rejects and the service now refuses. The Modbus-device tests keep {}: Modbus reads + /// only enums out of the blob and accepts an empty one. + /// + /// The tag name. + /// The import-ready tag input. + private static RawTagInput S7Tag(string name) => + Tag(name, tagConfig: """{"address":"DB1.DBW0","dataType":"Int16"}"""); + // -------------------------------------------------------------------------------- CreateTag (WP4) [Fact] @@ -181,9 +194,9 @@ public sealed class RawTreeServiceWaveBTests var rows = new[] { - new RawTagImportRow("Line/A", Tag("t1")), - new RawTagImportRow("Line/A", Tag("t2")), // reuses Line + Line/A - new RawTagImportRow("Line/B", Tag("t3")), // reuses Line, new B + new RawTagImportRow("Line/A", S7Tag("t1")), + new RawTagImportRow("Line/A", S7Tag("t2")), // reuses Line + Line/A + new RawTagImportRow("Line/B", S7Tag("t3")), // reuses Line, new B }; var result = await service.ImportTagsAsync(RawTreeTestDb.RootDeviceId, rows); @@ -207,7 +220,7 @@ public sealed class RawTreeServiceWaveBTests var (service, dbName) = Seeded(); var result = await service.ImportTagsAsync( - RawTreeTestDb.RootDeviceId, new[] { new RawTagImportRow(null, Tag("rootTag")) }); + RawTreeTestDb.RootDeviceId, new[] { new RawTagImportRow(null, S7Tag("rootTag")) }); result.Inserted.ShouldBe(1); using var db = RawTreeTestDb.CreateNamed(dbName); @@ -221,9 +234,9 @@ public sealed class RawTreeServiceWaveBTests var rows = new[] { - new RawTagImportRow("Grp", Tag("good1")), + new RawTagImportRow("Grp", S7Tag("good1")), new RawTagImportRow("Grp", Tag("bad", tagConfig: "{oops")), // malformed JSON ⇒ error - new RawTagImportRow("Grp", Tag("good2")), + new RawTagImportRow("Grp", S7Tag("good2")), }; var result = await service.ImportTagsAsync(RawTreeTestDb.RootDeviceId, rows); @@ -245,8 +258,8 @@ public sealed class RawTreeServiceWaveBTests var rows = new[] { - new RawTagImportRow("Grp", Tag("dup")), - new RawTagImportRow("Grp", Tag("dup")), // same path + name within the batch + new RawTagImportRow("Grp", S7Tag("dup")), + new RawTagImportRow("Grp", S7Tag("dup")), // same path + name within the batch }; var result = await service.ImportTagsAsync(RawTreeTestDb.RootDeviceId, rows); @@ -262,7 +275,7 @@ public sealed class RawTreeServiceWaveBTests // "temperature" already sits at the device root of RootDevice. var result = await service.ImportTagsAsync( - RawTreeTestDb.RootDeviceId, new[] { new RawTagImportRow(null, Tag("temperature")) }); + RawTreeTestDb.RootDeviceId, new[] { new RawTagImportRow(null, S7Tag("temperature")) }); result.Inserted.ShouldBe(0); result.Errors.ShouldContain(e => e.Contains("already exists")); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigValidatorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigValidatorTests.cs index 385b9538..e0f39b49 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigValidatorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigValidatorTests.cs @@ -42,9 +42,15 @@ public sealed class TagConfigValidatorTests public void OpcUaClient_with_node_id_is_valid() => TagConfigValidator.Validate("OpcUaClient", """{"nodeId":"ns=2;s=Line3.Temp"}""").ShouldBeNull(); + // Gitea #551 item 2: S7 is the one driver whose reader REQUIRES the dataType key to be present, so a + // blob carrying only an address is one the driver skips — this used to assert it was valid. [Fact] - public void S7_with_address_is_valid() - => TagConfigValidator.Validate("S7", """{"address":"DB1.DBW0"}""").ShouldBeNull(); + public void S7_with_address_and_data_type_is_valid() + => TagConfigValidator.Validate("S7", """{"address":"DB1.DBW0","dataType":"Int16"}""").ShouldBeNull(); + + [Fact] + public void S7_with_address_but_no_data_type_is_rejected() + => TagConfigValidator.Validate("S7", """{"address":"DB1.DBW0"}""").ShouldNotBeNullOrEmpty(); [Fact] public void AbCip_with_tag_path_is_valid()