diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttAddressRules.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttAddressRules.cs new file mode 100644 index 00000000..205913f8 --- /dev/null +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttAddressRules.cs @@ -0,0 +1,63 @@ +namespace ZB.MOM.WW.OtOpcUa.Driver.Mqtt; + +/// +/// The two shape rules an authored MQTT address must satisfy to be publishable, shared by +/// the deploy-time inspector () and the write encoder +/// () so the warning an operator is shown and the refusal the driver +/// makes can never disagree. +/// +/// +/// +/// Neither rule gates the READ path, and that asymmetry is the point. A tag whose +/// subscription topic carries a wildcard still reads (ambiguously — hence +/// 's long-standing advisory), and a Sparkplug tag +/// with a malformed id still parses. What these rules decide is whether an outbound +/// message could legally be addressed at all, so they belong to the write encoder and to the +/// inspector, never to the mappers' accept gate — which must stay exactly as strict as the +/// runtime reader (Gitea #551). +/// +/// +/// Why a wildcard write destination is a refusal rather than a best effort. MQTT-3.3.2-2 +/// forbids a wildcard in a PUBLISH topic, and a conformant broker answers a protocol violation by +/// closing the network connection — which for this driver is the one connection every +/// subscription lives on. Publishing anyway would turn one mis-authored tag into a whole-driver +/// outage; refusing costs that tag its write and nothing else. +/// +/// +internal static class MqttAddressRules +{ + /// The MQTT wildcard characters — illegal in any concrete (published or subscribed) topic. + private static readonly char[] TopicWildcards = ['+', '#']; + + /// + /// Characters that cannot appear in a single MQTT topic segment, which is what a Sparkplug + /// group / edge-node / device id is. / joins the wildcards here because it would silently + /// re-shape the command topic into a different address rather than making it unpublishable — and + /// because a decoded incoming id can never contain one, so an authored id that does is a binding + /// that could never have resolved. + /// + private static readonly char[] SegmentIllegalChars = ['/', '+', '#']; + + /// + /// Whether carries an MQTT wildcard (+ / #). + /// + /// A topic string; a /empty one carries none. + /// when a wildcard is present. + internal static bool HasWildcard(string? topic) => + !string.IsNullOrEmpty(topic) && topic.IndexOfAny(TopicWildcards) >= 0; + + /// + /// Whether is unusable as a single MQTT topic segment — i.e. as a Sparkplug + /// group, edge-node or device id. + /// + /// The authored id; a /empty one is not judged here (absence + /// is handled by the mappers' own presence checks, and deviceId is legitimately absent). + /// when the id contains /, + or #. + /// + /// Deliberately NOT applied to metricName: a metric name rides in the Sparkplug + /// payload, not in the topic, and Sparkplug's own canonical names use / (e.g. + /// Node Control/Rebirth). Applying it there would refuse every conformant node-control tag. + /// + internal static bool IsIllegalTopicSegment(string? id) => + !string.IsNullOrEmpty(id) && id.IndexOfAny(SegmentIllegalChars) >= 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 f3c725a9..241dc8e6 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 @@ -47,6 +47,14 @@ public static class MqttTagDefinitionFactory /// The MQTT wildcard characters — illegal in a tag's (concrete) subscription topic. private static readonly char[] TopicWildcards = ['+', '#']; + /// + /// The Sparkplug address keys that are single MQTT topic segments, and so cannot carry /, + /// + or #. is deliberately absent — it + /// rides in the payload, not the topic. See . + /// + private static readonly string[] SparkplugSegmentKeys = + [MqttTagConfigKeys.GroupId, MqttTagConfigKeys.EdgeNodeId, MqttTagConfigKeys.DeviceId]; + /// /// Maps an authored TagConfig object to a typed definition keyed by . /// The input is always an authored TagConfig JSON object (there is no name-vs-blob heuristic in v3), @@ -265,9 +273,13 @@ public static class MqttTagDefinitionFactory /// /// /// Advisories — a wildcard tag topic (+ / #), which the mapper - /// deliberately ACCEPTS (ambiguous, not unparseable) so this runs either way; and a - /// present-but-invalid payloadFormat on an ACCEPTED blob, which can only mean the - /// Sparkplug reader took it and the Plain reader would not (Gitea #557 item 2). + /// deliberately ACCEPTS (ambiguous, not unparseable) so this runs either way; a wildcard + /// writeTopic, which reads fine but can never be published to, so every write is + /// refused BadNotWritable; a Sparkplug groupId/edgeNodeId/ + /// deviceId carrying a character no MQTT topic segment can hold, which makes the + /// tag permanently unbindable; and a present-but-invalid payloadFormat on an + /// ACCEPTED blob, which can only mean the Sparkplug reader took it and the Plain reader + /// would not (Gitea #557 item 2). /// /// /// Empty for a clean, readable blob, for a blank one, or for one that is not an equipment-tag @@ -370,6 +382,36 @@ public static class MqttTagDefinitionFactory "concrete, or the tag will be fed by every matching topic."); } + // The write destination gets the same treatment as the read topic, and for a sharper reason: a + // wildcard cannot be PUBLISHED to at all (MQTT-3.3.2-2), so the tag reads perfectly while every + // write is refused with BadNotWritable — a split nothing else surfaces. The AdminUI's typed + // editor already refuses this, but it is only ONE authoring path (the raw-JSON textarea, CSV + // import and the API all reach the deploy gate directly), so leaving it out here made the + // authoring surface stricter than the deploy gate — the drift this inspector exists to close. + var writeTopic = ReadString(root, MqttTagConfigKeys.WriteTopic); + if (MqttAddressRules.HasWildcard(writeTopic)) + { + advisories.Add( + $"value '{writeTopic}' for 'writeTopic' contains an MQTT wildcard (+ or #); a wildcard " + + "cannot be published to, so the tag reads normally but every write is refused " + + "(BadNotWritable)."); + } + + // Same shape, Sparkplug side: a group/edge-node/device id is a single MQTT topic segment, so a + // decoded incoming id can never contain '/', '+' or '#'. An authored one that does is a binding + // that can never resolve — the mapper accepts it (it checks presence, not shape), so this is the + // only place it can be said. `metricName` is deliberately exempt: it rides in the payload, and + // Sparkplug's own canonical names use '/'. + foreach (var key in SparkplugSegmentKeys) + { + var value = ReadString(root, key); + if (!MqttAddressRules.IsIllegalTopicSegment(value)) continue; + advisories.Add( + $"value '{value}' for '{key}' contains a character ('/', '+' or '#') that cannot appear " + + "in an MQTT topic segment; a decoded Sparkplug id never contains one, so this tag can " + + "never bind and will read BadNodeIdUnknown in Sparkplug B mode."); + } + // Gitea #557 item 2 — the ONE key whose verdict depends on the DRIVER'S mode, reported as an // advisory because no tag-level gate can see that mode. A present-but-invalid payloadFormat is // fatal to the Plain parse and invisible to the Sparkplug one, so a union accept here can only diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttWriteEncoder.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttWriteEncoder.cs index 45ee2981..649d49f0 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttWriteEncoder.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttWriteEncoder.cs @@ -19,9 +19,11 @@ public enum MqttWriteEncodeError None = 0, /// - /// The tag declares no write destination: a Plain tag with no writeTopic. Deliberately a - /// hard refusal — see 's remarks on why a silent no-op would be - /// worse than the honest read-only behaviour this replaced. + /// The tag declares no usable write destination: a Plain tag with no writeTopic or + /// one carrying an MQTT wildcard, or a Sparkplug tag whose binding tuple is incomplete or whose + /// group/edge-node/device id is not a legal topic segment (see ). + /// Deliberately a hard refusal — see 's remarks on why a silent + /// no-op would be worse than the honest read-only behaviour this replaced. /// NoWriteDestination, @@ -121,6 +123,17 @@ public static class MqttWriteEncoder return MqttWriteEncodeError.NoWriteDestination; } + // A wildcard topic is not a destination — MQTT-3.3.2-2 forbids one in a PUBLISH, and a + // conformant broker answers a protocol violation by CLOSING the connection, which is the same + // connection every one of this driver's subscriptions lives on. So publishing anyway would turn + // one mis-authored tag into a whole-driver outage. `NoWriteDestination` (→ BadNotWritable) is the + // right verdict rather than a new error: the tag genuinely has nowhere a write could be sent, and + // the read path is untouched. See MqttAddressRules. + if (MqttAddressRules.HasWildcard(def.WriteTopic)) + { + return MqttWriteEncodeError.NoWriteDestination; + } + if (value is null) { // No MQTT payload means "the value is null" — a zero-length payload is a legitimate @@ -175,6 +188,18 @@ public static class MqttWriteEncoder return MqttWriteEncodeError.NoWriteDestination; } + // …and an id that is not a legal topic SEGMENT is the same failure by a different route: the + // tuple is assembled into the NCMD/DCMD topic, so a '/' would silently address a different node + // than the tag names while '+'/'#' would make the PUBLISH a protocol violation (see the Plain + // branch). `metricName` is exempt on purpose — it rides in the payload, and Sparkplug's own + // canonical names use '/'. + if (MqttAddressRules.IsIllegalTopicSegment(groupId) + || MqttAddressRules.IsIllegalTopicSegment(edgeNodeId) + || MqttAddressRules.IsIllegalTopicSegment(def.DeviceId)) + { + return MqttWriteEncodeError.NoWriteDestination; + } + if (value is null) { return MqttWriteEncodeError.ValueNotRepresentable; 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 index 498648f0..36a87ca9 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttInspectAcceptGateTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttInspectAcceptGateTests.cs @@ -252,6 +252,54 @@ public sealed class MqttInspectAcceptGateTests warnings.ShouldNotContain(w => w.Contains("Plain mode"), "an unserved tag is not a mode-dependent case"); } + // ---- The two authoring-only rules the deploy gate used to pass over in silence ---- + + /// + /// MqttTagConfigModel.ValidatePlain refuses a wildcard writeTopic outright, but the + /// typed editor is only ONE authoring path — the raw-JSON textarea, CSV import and the API all + /// reach the deploy gate directly. A wildcard cannot be published to at all (MQTT-3.3.2-2), so the + /// tag reads perfectly while every write is refused; that has to be said somewhere the operator + /// sees it, and an ADVISORY is the right class because the blob IS accepted. + /// + [Fact] + public void A_wildcard_write_topic_is_advised_on_an_accepted_blob() + { + const string Blob = """{"topic":"plant/line1/temp","writeTopic":"plant/+/set"}"""; + + EitherModeAccepts(Blob).ShouldBeTrue("the tag still READS fine — only the write is unaddressable"); + MqttTagDefinitionFactory.Inspect(Blob) + .ShouldContain(w => w.Contains("writeTopic") && w.Contains("wildcard")); + } + + /// + /// A Sparkplug group / edge-node / device id is a single MQTT topic segment, so a decoded incoming + /// id can never contain /, + or # — an authored one that does is a binding + /// that can never resolve, and the mapper accepts it (it validates presence, not shape). The + /// AdminUI model calls this out in words and refuses the save; the deploy gate said nothing. + /// + /// The key the advisory must name. + /// The tag config carrying an unbindable id. + [Theory] + [InlineData("groupId", """{"groupId":"Ot/Sim","edgeNodeId":"EdgeA","metricName":"T"}""")] + [InlineData("edgeNodeId", """{"groupId":"OtSim","edgeNodeId":"Edge+A","metricName":"T"}""")] + [InlineData("deviceId", """{"groupId":"OtSim","edgeNodeId":"EdgeA","deviceId":"F#1","metricName":"T"}""")] + public void A_sparkplug_id_that_is_not_a_topic_segment_is_advised(string key, string blob) + { + EitherModeAccepts(blob).ShouldBeTrue("the reader accepts it — which is exactly why it must be advised"); + MqttTagDefinitionFactory.Inspect(blob).ShouldContain(w => w.Contains(key)); + } + + /// + /// The falsification control for the rule above: metricName is NOT a topic segment — it + /// rides in the payload — and Sparkplug's own canonical names use /. Advising it would + /// report every conformant Node Control/Rebirth tag as broken. + /// + [Fact] + public void A_metric_name_containing_a_slash_is_never_advised() + => MqttTagDefinitionFactory + .Inspect("""{"groupId":"OtSim","edgeNodeId":"EdgeA","metricName":"Node Control/Rebirth"}""") + .ShouldBeEmpty(); + /// /// Both gates must keep agreeing on the VERDICT (Gitea #557 item 2). The #554 authoring consult in /// MqttTagConfigModel asks the identical union and only words a REFUSAL, so an advisory can diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttWriteEncoderTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttWriteEncoderTests.cs index 1629f299..d6ce97e9 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttWriteEncoderTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttWriteEncoderTests.cs @@ -237,6 +237,58 @@ public sealed class MqttWriteEncoderTests EncodeTopic(MqttMode.SparkplugB, both).ShouldBe("spBv1.0/OtOpcUaSim/DCMD/EdgeA/Filler1"); } + // ---- A destination that cannot be published to is not a destination ------------------------- + + [Theory] + [InlineData("factory/+/set")] + [InlineData("factory/oven/#")] + public void A_wildcard_write_topic_is_refused_rather_than_published(string writeTopic) + { + // MQTT-3.3.2-2: a PUBLISH topic MUST NOT contain a wildcard, and a conformant broker answers a + // protocol violation by CLOSING the connection — which, for this driver, is the same connection + // every subscription lives on. So one write to a mis-authored tag takes the WHOLE driver dark, + // not just that tag. The typed AdminUI editor already refuses this, but a raw-JSON / CSV / + // API-authored blob reaches the encoder untouched, so the refusal has to live here too. + MqttWriteEncoder.TryEncode(MqttMode.Plain, Plain(writeTopic), 1, out var encoded) + .ShouldBe(MqttWriteEncodeError.NoWriteDestination); + encoded.Topic.ShouldBeNull(); + } + + [Theory] + [InlineData("Ot/Sim", "EdgeA", null)] + [InlineData("OtSim", "Edge+A", null)] + [InlineData("OtSim", "EdgeA", "Fill#er")] + [InlineData("OtSim", "EdgeA", "Fill/er")] + public void A_binding_tuple_that_is_not_a_legal_topic_segment_has_no_command_address( + string groupId, string edgeNodeId, string? deviceId) + { + // The same protocol hazard on the Sparkplug side, plus one the Plain side does not have: the + // tuple IS the command topic, so a '/' silently re-shapes it into a DIFFERENT address (an + // NCMD for group "Ot" node "Sim" rather than the tag's own node) while '+'/'#' make it + // unpublishable. Refusing is the only honest verdict for all three. + var def = Sparkplug(deviceId) with { GroupId = groupId, EdgeNodeId = edgeNodeId }; + + MqttWriteEncoder.TryEncode(MqttMode.SparkplugB, def, 1.0, out _) + .ShouldBe(MqttWriteEncodeError.NoWriteDestination); + } + + [Fact] + public void A_metric_name_containing_a_slash_still_encodes() + { + // The falsification control for the rule above. Sparkplug's own canonical metric names use '/' + // (`Node Control/Rebirth`), and metricName rides in the PAYLOAD, never in the topic — so the + // segment rule must not reach it or every conformant node-control write would be refused. + MqttWriteEncoder.TryEncode( + MqttMode.SparkplugB, + Sparkplug(deviceId: null, DriverDataType.Boolean, metricName: "Node Control/Rebirth"), + true, + out var encoded) + .ShouldBe(MqttWriteEncodeError.None); + + encoded.Topic.ShouldBe("spBv1.0/OtOpcUaSim/NCMD/EdgeA"); + Payload.Parser.ParseFrom(encoded.Payload).Metrics[0].Name.ShouldBe("Node Control/Rebirth"); + } + private static string EncodeTopic(MqttMode mode, MqttTagDefinition def) { MqttWriteEncoder.TryEncode(mode, def, 1.0, out var encoded).ShouldBe(MqttWriteEncodeError.None);