From eddf5f57a5a715ef43aed070ab0fe00f4d9258ff Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 08:03:47 -0400 Subject: [PATCH 1/3] fix(mqtt-browser): converge the plain-mode label guard, make the node cap atomic, total the sibling sort MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Driver.Mqtt.Browser-002/-003/-004/-005. -002: ObservePlainTopic's pre-pass judged segment LENGTH only, so a plain topic segment carrying a newline or an ANSI escape became the picker's displayed label — a rule ObserveSparkplugBirth already applied via IsUsableLabel. The control-character half is now single-sourced (HasControlCharacter) and the plain marker text converges with the Sparkplug one. An EMPTY topic level stays accepted on purpose: it is legal MQTT and its terminal leaf is a real committable address. -003: GetOrCreate read _nodeCount and incremented it later, so N observers could each see cap-1 and all insert. The slot is now reserved with one Interlocked.Increment and given back on both non-consuming paths. -004: Project ordered a level by Segment alone, but the level is keyed by Path precisely because two Sparkplug siblings CAN share a Segment — leaving the tie to ConcurrentDictionary enumeration order, which .NET's randomised string hashing makes differ per process. .ThenBy(Path) makes the sort total. -005: no behaviour change — DefaultNodeCap's doc now states the real per-session ceiling and records that the aggregate across sessions is bounded by the reaper's idle TTL, not by any session cap. --- .../MqttBrowseSession.cs | 92 +++++++++-- .../MqttBrowseSessionTests.cs | 154 ++++++++++++++++++ 2 files changed, 233 insertions(+), 13 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs index 1c563e83..432d539c 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs @@ -55,7 +55,27 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser; /// internal sealed class MqttBrowseSession : IRebirthCapableBrowseSession { - /// Default cap on recorded nodes; a busy broker's # firehose is unbounded. + /// + /// Default cap on recorded nodes; a busy broker's # firehose is unbounded. + /// + /// Read this as a memory bound, not just a node count. Each node retains its full + /// Path (up to ), its Segment (up to + /// ) and, in plain mode, a snippet (up to + /// ), plus its own child dictionary — and every node is + /// held twice, once in its parent's children and once in the flat path index. So a session + /// near these ceilings costs on the order of 100 MB, not the few MB "50 000 nodes" suggests, + /// and a plain-mode open with no TopicPrefix subscribes # — the whole broker — + /// which is exactly the shape that approaches them. + /// + /// + /// The aggregate across sessions is bounded by LIFETIME, not by a count. + /// BrowseSessionRegistry caps neither concurrent sessions nor sessions per user; + /// BrowseSessionReaper evicts and disposes anything untouched for its 2-minute idle + /// TTL on a 30-second tick, and DisposeAsync clears both maps. If a per-session bound + /// is ever needed, cap the number of concurrent sessions there rather than lowering this + /// constant, which would make the picker less useful on a genuinely large broker. + /// + /// internal const int DefaultNodeCap = 50_000; /// Maximum characters of a payload retained as the picker's confirm-it's-the-right-topic snippet. @@ -198,8 +218,9 @@ internal sealed class MqttBrowseSession : IRebirthCapableBrowseSession internal bool Truncated => Volatile.Read(ref _truncated) != 0; /// - /// True once a topic was rejected for breaching / - /// ; surfaced as its own marker node from + /// True once a topic or metric was rejected as unusable — for breaching + /// / , or for carrying a control + /// character in a segment / identifier; surfaced as its own marker node from /// . Kept distinct from because the operator's /// remedy differs — narrow the prefix, versus this broker publishes absurd topic names. /// @@ -245,9 +266,11 @@ internal sealed class MqttBrowseSession : IRebirthCapableBrowseSession { nodes.Add(new BrowseNode( OversizedNodeId, + // Both modes word it the same way: since the plain path rejects a control-character + // segment as well as an oversized one, "too long" would name only half the cause. Mode == MqttMode.SparkplugB ? "⚠ Some metrics were unusable (oversized or malformed names) — use manual entry for those" - : "⚠ Some topics were too long to show — use manual entry for those", + : "⚠ Some topics were unusable (oversized or malformed names) — use manual entry for those", BrowseNodeKind.Folder, HasChildrenHint: false)); } @@ -477,9 +500,21 @@ internal sealed class MqttBrowseSession : IRebirthCapableBrowseSession var segments = topic.Split('/'); // Checked as a pre-pass so a rejected topic leaves no half-built path behind. + // + // The control-character half is the same rule IsUsableLabel applies to a Sparkplug identifier, + // and for the same reason: a segment becomes the picker's displayed label, MQTT forbids only + // U+0000 in a topic NAME, and an embedded newline or ANSI escape breaks a single-line row. It is + // a REJECTION rather than a scrub because a scrubbed segment is a different topic, and the + // picker would commit it as the tag's address. + // + // An EMPTY segment is deliberately NOT rejected, so this is not simply IsUsableLabel: `a//b` and + // a trailing '/' are legal MQTT, and such a topic's terminal leaf is a real committable address + // (the commit reads AddressFields[topic], which carries the node's full path). Refusing the whole + // topic would cost the operator a legal address to tidy a blank row. The Sparkplug path has no + // such case — every id it labels came off SparkplugTopic.TryParse, which rejects an empty segment. foreach (var segment in segments) { - if (segment.Length > MaxSegmentChars) + if (segment.Length > MaxSegmentChars || HasControlCharacter(segment)) { Volatile.Write(ref _oversized, 1); return; @@ -682,8 +717,14 @@ internal sealed class MqttBrowseSession : IRebirthCapableBrowseSession // different nodes, and keying by label would silently drop whichever arrived second. if (level.TryGetValue(path, out var existing)) return existing; - if (Volatile.Read(ref _nodeCount) >= _nodeCap) + // The slot is RESERVED before the node is created, in one interlocked step, and given back on + // either path that does not consume it. Reading the count and then incrementing it would let N + // observers each see `cap - 1` and all insert, overshooting the cap by up to N-1 — the cap is + // the only bound between a `#` firehose and unbounded memory, so it must not depend on MQTTnet + // happening to deliver on a single dispatcher thread. + if (Interlocked.Increment(ref _nodeCount) > _nodeCap) { + Interlocked.Decrement(ref _nodeCount); Volatile.Write(ref _truncated, 1); return null; } @@ -692,9 +733,12 @@ internal sealed class MqttBrowseSession : IRebirthCapableBrowseSession var child = level.GetOrAdd(path, created); if (ReferenceEquals(child, created)) { - Interlocked.Increment(ref _nodeCount); _byPath[path] = child; } + else + { + Interlocked.Decrement(ref _nodeCount); // lost the insert race; the reservation was not consumed + } return child; } @@ -709,15 +753,24 @@ internal sealed class MqttBrowseSession : IRebirthCapableBrowseSession /// /// The candidate label. /// when the label may be recorded. - private static bool IsUsableLabel(string label) + private static bool IsUsableLabel(string label) => + label.Length != 0 && label.Length <= MaxSegmentChars && !HasControlCharacter(label); + + /// + /// Whether carries a character that would break the picker's single-line + /// rows. Single-sourced so the plain and Sparkplug ingest paths cannot drift on what may become a + /// tree label — see for the one rule they deliberately differ on. + /// + /// The candidate label. + /// when a control character is present. + private static bool HasControlCharacter(string text) { - if (label.Length == 0 || label.Length > MaxSegmentChars) return false; - foreach (var c in label) + foreach (var c in text) { - if (char.IsControl(c)) return false; + if (char.IsControl(c)) return true; } - return true; + return false; } /// @@ -1095,10 +1148,23 @@ internal sealed class MqttBrowseSession : IRebirthCapableBrowseSession /// Projects one level of the tree into browse nodes, ordered so the picker is stable. /// The child map to project. - /// The projected nodes, sorted by segment. + /// The projected nodes, sorted by segment and then by path. + /// + /// The sort must be TOTAL, which is why the path is the tiebreaker. Sorting by segment + /// alone is only a stable order when labels are unique, and the whole reason this map is keyed by + /// path (see ) is that in Sparkplug mode two siblings CAN share one — + /// a device folder Plant1/EdgeA/Temp and an edge-node-level metric + /// Plant1/EdgeA::Temp. is stable with respect to its input, but the input is + /// , whose enumeration order is not + /// guaranteed and can change as the tree grows — so a tie left to it lets two visually identical + /// rows swap places between successive calls, and the operator cannot + /// see which one they are clicking. + /// private static List Project(ConcurrentDictionary level) => level.Values .OrderBy(n => n.Segment, StringComparer.Ordinal) + .ThenBy(n => n.Path, StringComparer.Ordinal) .Select(n => { // Plain mode infers: a childless node is always the terminal segment of a topic that diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttBrowseSessionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttBrowseSessionTests.cs index b84fdb7d..bbc14bc1 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttBrowseSessionTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttBrowseSessionTests.cs @@ -296,6 +296,123 @@ public sealed class MqttBrowseSessionTests (await s.RootAsync(TestContext.Current.CancellationToken)).ShouldHaveSingleItem(); } + /// + /// The two ingest paths must agree on what may become a tree label. MQTT forbids only + /// U+0000 in a topic NAME, so a publisher can legally put a newline or an ANSI escape in + /// one — and a plain segment goes straight into the picker's displayed label, exactly where the + /// Sparkplug path already rejects one. + /// + /// + /// The node cap must hold under concurrent observation. MQTTnet delivers on a single dispatcher + /// per client today, so this is a guard on the shape rather than a live defect — but the cap is + /// the only thing standing between a # firehose and unbounded memory, and a read-then- + /// increment pair lets N threads each see cap - 1 and all insert. + /// + [Fact] + public async Task NodeCap_HoldsUnderConcurrentObservation() + { + // One round would pass by luck; the overshoot is a race. Each round drives many threads at a + // cap of 1 through a barrier, so every thread reaches the cap check at once. + const int rounds = 40; + var workers = Math.Max(Environment.ProcessorCount * 2, 8); + + for (var round = 0; round < rounds; round++) + { + await using var s = new MqttBrowseSession(MqttMode.Plain, nodeCap: 1); + using var barrier = new Barrier(workers); + + // Dedicated threads, not Task.Run: every worker BLOCKS on the barrier, and blocking that + // many pool threads makes the pool inject new ones on its ~500 ms starvation timer — which + // both slows the test enormously and staggers the very arrivals it is trying to align. + var threads = Enumerable.Range(0, workers) + .Select(i => new Thread(() => + { + barrier.SignalAndWait(); + s.ObserveTopicForTest($"t{i}", "1"); + }) { IsBackground = true }) + .ToList(); + + foreach (var t in threads) t.Start(); + foreach (var t in threads) t.Join(); + + var recorded = (await s.RootAsync(TestContext.Current.CancellationToken)) + .Count(n => n.NodeId != MqttBrowseSession.TruncatedNodeId + && n.NodeId != MqttBrowseSession.OversizedNodeId); + recorded.ShouldBeLessThanOrEqualTo(1, $"round {round} overshot the node cap"); + } + } + + [Fact] + public async Task PlainTopicSegmentWithAControlCharacter_IsRejectedWhole_AndSurfacesAMarker() + { + // Every segment here is well under MaxSegmentChars, so ONLY the control-character rule can + // reject it — the length guard must not be what makes this test pass. + await using var s = new MqttBrowseSession(MqttMode.Plain); + s.ObserveTopicForTest("good/topic", "1"); + s.ObserveTopicForTest("bad/oven\nname/temp", "1"); + + s.OversizedRejected.ShouldBeTrue(); + var root = await s.RootAsync(TestContext.Current.CancellationToken); + root.ShouldContain(n => n.NodeId == MqttBrowseSession.OversizedNodeId); + root.ShouldContain(n => n.DisplayName == "good"); + root.ShouldNotContain(n => n.DisplayName == "bad"); // no half-built path left behind + } + + [Theory] + [InlineData("\n")] + [InlineData("\r")] + [InlineData("\t")] + [InlineData("\u001b")] // ANSI escape + [InlineData("\u0000")] + public async Task PlainTopic_EachControlCharacterForm_IsRejected(string control) + { + await using var s = new MqttBrowseSession(MqttMode.Plain); + s.ObserveTopicForTest($"a/b{control}c/d", "1"); + + s.OversizedRejected.ShouldBeTrue(); + (await s.RootAsync(TestContext.Current.CancellationToken)) + .ShouldNotContain(n => n.DisplayName == "a"); + } + + /// + /// Falsification control for the rule above: the guard rejects CONTROL characters, not + /// "anything unusual". A topic segment is arbitrary UTF-8 and legitimately carries accents, + /// spaces and punctuation — rejecting those would drop real plant topics from the picker. + /// + [Fact] + public async Task PlainTopic_PrintableNonAsciiSegments_AreStillAccepted() + { + await using var s = new MqttBrowseSession(MqttMode.Plain); + s.ObserveTopicForTest("usiné/four n°3/temp %", "1"); + + s.OversizedRejected.ShouldBeFalse(); + (await s.RootAsync(TestContext.Current.CancellationToken)) + .ShouldHaveSingleItem().DisplayName.ShouldBe("usiné"); + } + + /// + /// An EMPTY topic level (a//b, or a trailing /) is legal MQTT and stays accepted — + /// deliberately NOT converged with IsUsableLabel's non-empty rule. Its terminal leaf is a + /// real, committable address (the commit reads AddressFields[topic], which carries the + /// node's full path), so rejecting the topic whole would cost the operator a legal address to + /// tidy a blank picker row. The control-character rule is the one that had to converge: a label + /// carrying a newline breaks the row it renders in. + /// + [Fact] + public async Task PlainTopic_WithAnEmptyLevel_IsStillRecorded() + { + await using var s = new MqttBrowseSession(MqttMode.Plain); + s.ObserveTopicForTest("a//b", "1"); + + s.OversizedRejected.ShouldBeFalse(); + var leaf = (await s.ExpandAsync("a/", TestContext.Current.CancellationToken)).ShouldHaveSingleItem(); + leaf.NodeId.ShouldBe("a//b"); + + var attr = (await s.AttributesAsync("a//b", TestContext.Current.CancellationToken)) + .ShouldHaveSingleItem(); + attr.AddressFields.ShouldNotBeNull()[MqttTagConfigKeys.Topic].ShouldBe("a//b"); + } + // ---------------------------------------------------------------- payload bounds [Fact] @@ -554,6 +671,43 @@ public sealed class MqttBrowseSessionTests .ShouldHaveSingleItem().Name.ShouldBe("Temp"); } + /// + /// Siblings that share a label must still have a TOTAL order. The level is projected from a + /// ConcurrentDictionary keyed by path — precisely because two Sparkplug siblings CAN share + /// a segment — and its enumeration order is not guaranteed, so ordering by segment alone leaves + /// the tie broken by whatever the dictionary happens to yield. The two rows are visually + /// identical, so a reorder between successive expands is a picker that changes what a click means + /// with nothing on screen to show it. + /// + [Fact] + public async Task SparkplugTree_SiblingsSharingALabel_AreOrderedDeterministically() + { + // MANY colliding pairs, not one. .NET string hashing is randomised per process, so the tie order + // a segment-only sort falls through to is whatever THIS run's dictionary yields — with a single + // pair it lands the right way round about half the time and the test would pass vacuously. With + // 24 pairs, every tie has to agree by accident for that to happen. + var names = Enumerable.Range(0, 24).Select(i => $"N{i:D2}").ToList(); + + await using var s = new MqttBrowseSession(MqttMode.SparkplugB); + foreach (var name in names) + { + s.ObserveBirthForTest("Plant1", "EdgeA", null, name, SparkplugDataType.Float); // metric + s.ObserveBirthForTest("Plant1", "EdgeA", name, "Value", SparkplugDataType.Float); // device + } + + // Segment first (the names, in ordinal order), then path — and Ordinal puts '/' (0x2F) before + // ':' (0x3A), so each device folder precedes the same-named node-level metric. + var expected = names.SelectMany(n => new[] { $"Plant1/EdgeA/{n}", $"Plant1/EdgeA::{n}" }).ToList(); + + var children = await s.ExpandAsync("Plant1/EdgeA", TestContext.Current.CancellationToken); + children.Select(n => n.NodeId).ShouldBe(expected); + + // …and it is stable: expanding the same level again yields the same order. + (await s.ExpandAsync("Plant1/EdgeA", TestContext.Current.CancellationToken)) + .Select(n => n.NodeId) + .ShouldBe(expected); + } + [Fact] public async Task SparkplugTree_EmptyBirth_StillRendersItsDeviceAsAFolder() { From 93f70504bd54f10fdbc6d998cbf688a0d4aebc6a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 08:18:30 -0400 Subject: [PATCH 2/3] fix(mqtt-contracts): strict retainSeed, real bounds instead of dead [Range]s, DateTime edges, closed topic formatting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Driver.Mqtt.Contracts-004/-005/-006/-007. -004: retainSeed was the last behavioural key read leniently, so "retainSeed":"false" — what a CSV import produces — silently became the default TRUE, the INVERSE of the authored intent. Both mappers now read it through TryReadBoolStrict (the qos absent/valid/present-but-invalid split) and DescribeRejection names it. The authoring side needed no hand-mirrored rule: MqttTagConfigModel.Validate already consults this same parse through TagConfigParseGate, so the save is blocked with the inspector's wording — pinned by a new AdminUI parity theory pair. -005: the seven [Range] attributes on MqttDriverOptions/MqttPlainOptions never ran — the options are bound by JsonSerializer, and nothing in the four MQTT projects calls Validator.TryValidateObject. Two of them CONTRADICTED the real authority (KeepAlive's [Range(1,…)] forbade the zero Normalize admits as legal MQTT; MaxPayloadBytes' forbade the non-positive value its own remarks document as falling back to 1 MiB), so wiring them up would have broken deliberate behaviour. Deleted, with each property naming where its bound actually lives. The one escape they papered over is closed: an out-of-range defaultQos rode into the SUBSCRIBE verbatim, and MqttSubscriptionManager's constructor now clamps it exactly as the AdminUI driver form already persists. -006: the DateTime write arm cast a negative long to ulong, so 1969-12-31 published a timestamp ~584 million years in the future; it is now refused as ValueNotRepresentable. An Unspecified kind was handed to ToUniversalTime(), which reads it as LOCAL — the same authored instant encoded differently on the two nodes of a redundant pair; it is now read as UTC. -007: Format/FormatState enforced only null/empty, and nothing at all on deviceId, so the type was not round-trip-closed. They now apply the parser's IsValidSegment (plus '/', which only the format side can see). MqttWriteEncoder's own guards stay — TryEncode returns an error rather than throwing, and that contract does not change. --- .../MqttDriverOptions.cs | 48 ++++++++---- .../MqttTagDefinitionFactory.cs | 73 ++++++++++++++++--- .../MqttWriteEncoder.cs | 24 +++++- .../Sparkplug/SparkplugTopic.cs | 50 ++++++++++++- .../MqttSubscriptionManager.cs | 16 +++- .../Uns/TagEditors/MqttTagConfigModel.cs | 8 ++ .../MqttSubscriptionManagerTests.cs | 32 ++++++++ .../MqttTagDefinitionFactoryTests.cs | 57 +++++++++++++++ .../MqttWriteEncoderTests.cs | 61 ++++++++++++++++ .../SparkplugTopicTests.cs | 72 ++++++++++++++++++ .../Uns/TagConfigNonEnumParityTests.cs | 31 ++++++++ 11 files changed, 441 insertions(+), 31 deletions(-) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttDriverOptions.cs index c40bae79..3360d1a4 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttDriverOptions.cs @@ -1,4 +1,3 @@ -using System.ComponentModel.DataAnnotations; using System.Text; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; @@ -15,14 +14,30 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Mqtt; /// OpcUaClientDriverOptions. selects the ingest shape; /// and are nullable sub-objects — only the one /// matching is populated. +/// +/// ⚠️ There are deliberately no DataAnnotations range attributes on this DTO, and adding +/// any would be documentation that reads like enforcement. The options are bound by +/// JsonSerializer.Deserialize through , never by +/// IOptions + ValidateDataAnnotations, and nothing in the four MQTT projects calls +/// Validator.TryValidateObject — so the seven [Range]s this type used to carry never +/// ran once. Worse, two of them contradicted the real authority: [Range(1, …)] on +/// forbade the zero that MqttDriver.Normalize deliberately +/// admits as legal MQTT, and the same on forbade the non-positive +/// value this type's own remarks document as "falls back to the 1 MiB default". Wiring them up +/// would therefore have broken deliberate behaviour, so the bounds live where they are actually +/// applied — MqttDriver.Normalize, MqttSubscriptionManager's constructor and +/// MqttConnection.NextBackoff — and each property below names its own. +/// /// public sealed record MqttDriverOptions { /// Broker hostname or IP address. public string Host { get; init; } = "localhost"; - /// Broker TCP port. - [Range(1, 65535)] + /// + /// Broker TCP port. Range-checked by MqttDriverProbe and the AdminUI driver form; there is + /// no [Range] attribute here because nothing would ever run it — see the type remarks. + /// public int Port { get; init; } = 8883; /// @@ -67,20 +82,22 @@ public sealed record MqttDriverOptions /// Whether to request a clean session (v3.1.1) / clean start (v5.0) at CONNECT. public bool CleanSession { get; init; } = true; - /// Keep-alive interval sent at CONNECT. - [Range(1, int.MaxValue)] + /// + /// Keep-alive interval sent at CONNECT. Bounded by MqttDriver.Normalize, which admits + /// zero — legal MQTT for "no keep-alive" — and only replaces a negative value. + /// public int KeepAliveSeconds { get; init; } = 30; - /// Bounded connect deadline (see design §8) — the driver never hangs past this. - [Range(1, int.MaxValue)] + /// + /// Bounded connect deadline (see design §8) — the driver never hangs past this. A non-positive + /// value is replaced by MqttDriver.Normalize, which is the enforcement point. + /// public int ConnectTimeoutSeconds { get; init; } = 15; - /// Initial reconnect backoff after a connection drop. - [Range(1, int.MaxValue)] + /// Initial reconnect backoff after a connection drop; clamped by MqttConnection.NextBackoff. public int ReconnectMinBackoffSeconds { get; init; } = 1; - /// Cap on the exponential reconnect backoff. - [Range(1, int.MaxValue)] + /// Cap on the exponential reconnect backoff; clamped by MqttConnection.NextBackoff. public int ReconnectMaxBackoffSeconds { get; init; } = 30; /// @@ -96,7 +113,6 @@ public sealed record MqttDriverOptions /// because this .Contracts assembly is referenced by the driver assembly and /// cannot reference it back; the manager's constructor is the single enforcement point. /// - [Range(1, int.MaxValue)] public int MaxPayloadBytes { get; init; } = 1024 * 1024; /// Selects the ingest shape — Plain topics or Sparkplug B. @@ -208,7 +224,6 @@ public sealed record MqttSparkplugOptions /// How long, in seconds, browse/discovery collects NBIRTH/DBIRTH traffic before /// considering the observed metric set stable. /// - [Range(1, int.MaxValue)] public int BirthObservationWindowSeconds { get; init; } = 15; } @@ -225,7 +240,10 @@ public sealed record MqttPlainOptions /// public string TopicPrefix { get; init; } = ""; - /// Default QoS used when a tag's own qos is unset. - [Range(0, 2)] + /// + /// Default QoS used when a tag's own qos is unset. MQTT defines exactly three levels; + /// MqttSubscriptionManager's constructor clamps this to 0–2 before it can reach a + /// SUBSCRIBE, matching what the AdminUI driver form persists. + /// public int DefaultQos { get; init; } = 1; } 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 241dc8e6..2f193445 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 @@ -61,10 +61,11 @@ public static class MqttTagDefinitionFactory /// and framing is decided solely by JsonDocument.Parse plus the /// root- check — so leading whitespace is skipped exactly as the JSON /// spec says, and a padded blob reaches the same verdict its unpadded form does (Gitea #559). - /// Enum fields (payloadFormat / dataType) and qos are read STRICTLY — a - /// present-but-invalid (typo'd) value rejects the whole tag (returns ⇒ the - /// driver surfaces BadNodeIdUnknown) rather than silently defaulting to a wrong-typed Good or - /// a downgraded delivery guarantee. Never throws. + /// Enum fields (payloadFormat / dataType), qos and retainSeed are read + /// STRICTLY — a present-but-invalid (typo'd) value rejects the whole tag (returns + /// ⇒ the driver surfaces BadNodeIdUnknown) rather than silently + /// defaulting to a wrong-typed Good, a downgraded delivery guarantee, or the inverse of the + /// authored seeding behaviour. Never throws. /// /// A wildcard topic (+ / #) is deliberately NOT rejected here: it is /// ambiguous rather than unparseable, and is the operator-visible surface @@ -119,6 +120,12 @@ public static class MqttTagDefinitionFactory // WEAKER delivery guarantee than the one they authored, with nothing to show for it. if (!TryReadQosStrict(root, out var qos)) return false; + // …and so is retainSeed, the last behavioural key that was not. A lenient read turned + // "retainSeed":"false" — which a CSV import or a hand-edited blob produces naturally — into + // the default TRUE, the OPPOSITE of what was authored, and the tag then seeded its initial + // value from the broker's retained message the author asked it to ignore. + if (!TryReadBoolStrict(root, "retainSeed", defaultValue: true, out var retainSeed)) return false; + var jsonPath = ReadString(root, "jsonPath"); if (string.IsNullOrEmpty(jsonPath)) jsonPath = RootJsonPath; @@ -129,7 +136,7 @@ public static class MqttTagDefinitionFactory JsonPath: jsonPath, DataType: dataType, Qos: qos, - RetainSeed: ReadBoolOrDefault(root, "retainSeed", defaultValue: true), + RetainSeed: retainSeed, DataTypeAuthored: dataTypeAuthored, // Optional, and absence is meaningful: no writeTopic ⇒ not writable (Gitea #508). // Blank normalises to null so "" and absent cannot mean different things. @@ -230,6 +237,10 @@ public static class MqttTagDefinitionFactory if (!TryReadQosStrict(root, out var qos)) return false; + // Same key, same strictness — retainSeed is read identically by both shapes, so a blob's + // verdict on it must not depend on which reader took it. + if (!TryReadBoolStrict(root, "retainSeed", defaultValue: true, out var retainSeed)) return false; + def = new MqttTagDefinition( Name: rawPath, // Plain-shape fields carried through verbatim; the Sparkplug ingest path reads none of @@ -239,7 +250,7 @@ public static class MqttTagDefinitionFactory JsonPath: RootJsonPath, DataType: dataType, Qos: qos, - RetainSeed: ReadBoolOrDefault(root, "retainSeed", defaultValue: true), + RetainSeed: retainSeed, DataTypeAuthored: dataTypeAuthored, GroupId: groupId, EdgeNodeId: edgeNodeId, @@ -267,7 +278,7 @@ public static class MqttTagDefinitionFactory /// /// 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 + /// qos / retainSeed 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. /// @@ -446,6 +457,9 @@ public static class MqttTagDefinitionFactory TagConfigJson.DescribeInvalidEnum(root, "payloadFormat"), TagConfigJson.DescribeInvalidEnum(root, "dataType"), DescribeInvalidQos(root), + // Read strictly by BOTH mappers, so — unlike payloadFormat — it can never land on an + // accepted blob and is a rejection cause outright. + DescribeInvalidBool(root, "retainSeed"), }) { if (w is not null) rejections.Add(w); @@ -515,8 +529,45 @@ public static class MqttTagDefinitionFactory private static string? NullIfBlank(string value) => string.IsNullOrWhiteSpace(value) ? null : value.Trim(); - private static bool ReadBoolOrDefault(JsonElement o, string name, bool defaultValue) - => o.TryGetProperty(name, out var e) && e.ValueKind is JsonValueKind.True or JsonValueKind.False - ? e.GetBoolean() - : defaultValue; + /// + /// Strict boolean read, mirroring 's absent / valid / + /// present-but-invalid split: absent ⇒ ; a JSON true/ + /// false ⇒ that value; anything else present (a string, a number, an explicit + /// null) ⇒ the read FAILS. + /// + /// The TagConfig root object. + /// The property name to read. + /// The value an ABSENT key takes. + /// The parsed flag. + /// only when the key is present but not a JSON boolean. + /// + /// Lenient-with-a-default is the wrong shape for a BEHAVIOURAL flag, which is the whole + /// qos argument again: absorbing a malformed value into the default silently hands the + /// operator the opposite of what they authored, with nothing to show for it. It is worse here + /// than for qos, in fact — a defaulted qos is a weaker delivery guarantee, whereas + /// a defaulted retainSeed is the INVERSE of the authored intent whenever the author wrote + /// false, which is the only value worth writing at all. + /// + private static bool TryReadBoolStrict(JsonElement o, string name, bool defaultValue, out bool value) + { + value = defaultValue; + if (!o.TryGetProperty(name, out var e)) return true; // absent + if (e.ValueKind is not (JsonValueKind.True or JsonValueKind.False)) return false; + value = e.GetBoolean(); + return true; + } + + /// + /// A human-readable warning for a present-but-invalid boolean field, or + /// when it is absent or valid — the boolean counterpart of . + /// + /// The TagConfig root object. + /// The property name to describe. + /// The warning text, or . + private static string? DescribeInvalidBool(JsonElement o, string name) + { + if (!o.TryGetProperty(name, out var e)) return null; + if (e.ValueKind is JsonValueKind.True or JsonValueKind.False) return null; + return $"value '{e.GetRawText()}' for '{name}' is not a JSON boolean; valid: true, false"; + } } 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 649d49f0..6d0b1125 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 @@ -304,11 +304,29 @@ public static class MqttWriteEncoder return MqttWriteEncodeError.None; case DriverDataType.DateTime: + { + var authored = Convert.ToDateTime(value, CultureInfo.InvariantCulture); + + // An Unspecified kind is read as UTC, not passed to ToUniversalTime() — that treats + // it as LOCAL, so the identical authored instant would encode differently on the two + // nodes of a redundant pair, which are not required to share a timezone. Reading it + // as UTC rather than refusing it keeps an ordinary authored "2026-08-10T12:00:00" + // writable: Convert.ToDateTime produces exactly that kind for one. + var utc = authored.Kind == DateTimeKind.Unspecified + ? DateTime.SpecifyKind(authored, DateTimeKind.Utc) + : authored.ToUniversalTime(); + + // Sparkplug's DateTime is an UNSIGNED milliseconds-since-epoch, so a pre-1970 instant + // has no representation at all — the cast below would wrap a negative long into a + // timestamp ~584 million years in the future. Refuse it: unrepresentable values are + // refused, never coerced. + var unixMs = new DateTimeOffset(utc).ToUnixTimeMilliseconds(); + if (unixMs < 0) return MqttWriteEncodeError.ValueNotRepresentable; + metric.Datatype = (uint)TahuDataType.DateTime; - metric.LongValue = (ulong)new DateTimeOffset( - Convert.ToDateTime(value, CultureInfo.InvariantCulture).ToUniversalTime()) - .ToUnixTimeMilliseconds(); + metric.LongValue = (ulong)unixMs; return MqttWriteEncodeError.None; + } default: // DriverDataType.Reference and anything added later: refuse rather than guess a diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/Sparkplug/SparkplugTopic.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/Sparkplug/SparkplugTopic.cs index 6f65be5b..236804e8 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/Sparkplug/SparkplugTopic.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/Sparkplug/SparkplugTopic.cs @@ -195,10 +195,24 @@ public sealed record SparkplugTopic( /// it does not have to hand-concatenate segments itself. /// /// - /// / is null/empty, or + /// / is null/empty, any supplied id is + /// not a legal single topic segment (see ), or /// is (use /// instead — STATE has no group/edge-node/device segments). /// + /// + /// The per-segment rule is enforced here, not left to the caller, so the type is + /// round-trip-closed: everything this emits re-parses through to the + /// instance it was built from. Without it, a +/# id produced a topic MQTT-3.3.2-2 + /// forbids in a PUBLISH — a protocol violation a conformant broker answers by dropping the + /// connection — and a deviceId containing / silently added a segment, i.e. a + /// well-formed topic addressing a different device than the caller named. + /// + /// ⚠️ This THROWS where it previously succeeded, so a caller that must not throw keeps its own + /// guard: MqttWriteEncoder.TryEncode returns NoWriteDestination for these ids + /// rather than propagating, and that contract does not change. + /// + /// public static string Format(string groupId, SparkplugMessageType type, string edgeNodeId, string? deviceId = null) { ArgumentException.ThrowIfNullOrEmpty(groupId); @@ -208,15 +222,26 @@ public sealed record SparkplugTopic( throw new ArgumentException("Use FormatState to build a STATE topic.", nameof(type)); } + ThrowIfNotASegment(groupId, nameof(groupId)); + ThrowIfNotASegment(edgeNodeId, nameof(edgeNodeId)); + if (deviceId is not null) ThrowIfNotASegment(deviceId, nameof(deviceId)); + return deviceId is null ? $"{Namespace}/{groupId}/{type}/{edgeNodeId}" : $"{Namespace}/{groupId}/{type}/{edgeNodeId}/{deviceId}"; } /// Builds the v3.0 STATE topic string spBv1.0/STATE/{hostId}. + /// The primary-host id; must be a legal single topic segment. + /// The STATE topic string. + /// + /// is null/empty or is not a legal single topic segment — see the + /// round-trip-closure remarks on . + /// public static string FormatState(string hostId) { ArgumentException.ThrowIfNullOrEmpty(hostId); + ThrowIfNotASegment(hostId, nameof(hostId)); return $"{Namespace}/STATE/{hostId}"; } @@ -288,4 +313,27 @@ public sealed record SparkplugTopic( private static bool IsValidSegment(string segment) => segment.Length > 0 && segment.IndexOf('+') < 0 && segment.IndexOf('#') < 0; + + /// + /// Applies on the FORMAT side, so the parser and the formatter judge + /// an id by one rule — plus /, which only the format side can see. + /// + /// The candidate segment. + /// The argument name to report. + /// The segment is empty, carries an MQTT wildcard, or carries /. + /// + /// / cannot appear in a parsed id — has already split on it — so + /// has no reason to test for one and does not. On this side it is + /// the difference between an id and two ids: a formatted value carrying one produces a topic with + /// an extra segment, which addresses something other than what the caller named and no longer + /// re-parses at all. Rejecting it is what makes the round trip total. + /// + private static void ThrowIfNotASegment(string segment, string paramName) + { + if (IsValidSegment(segment) && segment.IndexOf('/') < 0) return; + throw new ArgumentException( + $"'{segment}' is not a legal Sparkplug topic segment: it must be non-empty and carry no '/' " + + "and no MQTT wildcard (+ or #).", + paramName); + } } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/MqttSubscriptionManager.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/MqttSubscriptionManager.cs index 14dc9d1d..35071696 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/MqttSubscriptionManager.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt/MqttSubscriptionManager.cs @@ -65,6 +65,14 @@ public sealed class MqttSubscriptionManager /// public const int DefaultMaxPayloadBytes = 1024 * 1024; + /// + /// Subscription QoS applied when neither the tag nor the driver's Plain section authored + /// one. Mirrors ' own default — duplicated rather than + /// referenced because that value is an instance property, and this is the fallback for the case + /// where the whole section is absent. + /// + public const int DefaultQos = 1; + // OPC UA status codes (values verified against Opc.Ua.StatusCodes, not recalled). private const uint StatusGood = 0x00000000u; @@ -137,7 +145,13 @@ public sealed class MqttSubscriptionManager MaxPayloadBytes = maxPayloadBytes > 0 ? maxPayloadBytes : DefaultMaxPayloadBytes; _mode = options.Mode; - _defaultQos = options.Plain?.DefaultQos ?? 1; + // Clamped here for the same reason MaxPayloadBytes is: this constructor is the single + // enforcement point for the driver options the manager captures, and MQTT defines exactly three + // QoS levels — an out-of-range value would otherwise ride into BuildFilters and out onto the + // SUBSCRIBE verbatim. Clamp rather than fall back to the default, matching what + // MqttDriverFormModel.Normalize already persists, so the authoring surface and the driver cannot + // reach different answers for the same authored number. + _defaultQos = Math.Clamp(options.Plain?.DefaultQos ?? DefaultQos, 0, 2); _driverId = driverId; _transport = transport; _logger = logger; diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/MqttTagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/MqttTagConfigModel.cs index 521957c5..80ddecb6 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/MqttTagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/MqttTagConfigModel.cs @@ -117,6 +117,14 @@ public sealed class MqttTagConfigModel /// the key and inherit the driver's default (true). An absent key stays absent. Shared by /// both modes — MqttTagDefinitionFactory reads retainSeed identically for Plain and /// Sparkplug. + /// + /// The load is lenient (a non-boolean value reads as null, i.e. "unset"), but the + /// driver reads the key STRICTLY — "retainSeed":"false", which a CSV import produces + /// naturally, rejects the whole tag rather than silently taking the default, which is the + /// INVERSE of that author's intent. No hand-mirrored rule is needed here for that: + /// 's consult asks the driver's own reader + /// about the loaded blob, so the save is blocked with the inspector's own wording. + /// /// public bool? RetainSeed { get; set; } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttSubscriptionManagerTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttSubscriptionManagerTests.cs index 354caf4c..9c4344c4 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttSubscriptionManagerTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttSubscriptionManagerTests.cs @@ -423,6 +423,38 @@ public sealed class MqttSubscriptionManagerTests press.SeedRetained.ShouldBeFalse(); } + /// + /// An out-of-range driver-level defaultQos must never reach the wire. MQTT defines exactly + /// three QoS levels, and this manager's constructor is the single enforcement point for the + /// driver options it captures — the same place MaxPayloadBytes is bounded. The + /// [Range(0, 2)] attribute on MqttPlainOptions.DefaultQos never ran: nothing in the + /// four MQTT projects calls Validator.TryValidateObject, and the options arrive by + /// JsonSerializer.Deserialize, so a hand-edited or API-supplied driver config put an + /// illegal QoS straight into the SUBSCRIBE. Clamping (rather than falling back to 1) matches what + /// the AdminUI driver form already persists, so the two cannot disagree. + /// + /// The authored driver-level default QoS. + /// The QoS the SUBSCRIBE must carry. + [Theory] + [InlineData(5, 2)] + [InlineData(-1, 0)] + [InlineData(0, 0)] + [InlineData(2, 2)] + public async Task DefaultQos_OutsideTheLegalRange_NeverReachesTheSubscribe(int authored, int expected) + { + var transport = new FakeTransport(); + var mgr = new MqttSubscriptionManager( + new MqttDriverOptions { Mode = MqttMode.Plain, Plain = new MqttPlainOptions { DefaultQos = authored } }, + "mqtt-1", + transport); + + // The tag authors no qos of its own, so the driver-level default is what rides out. + mgr.Register([Tag(OvenTempPath, $$"""{"topic":"{{OvenTopic}}","payloadFormat":"Raw"}""")]); + await mgr.SubscribeAsync([OvenTempPath], TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken); + + transport.Calls.ShouldHaveSingleItem().ShouldHaveSingleItem().Qos.ShouldBe(expected); + } + // --------------------------------------------------------------------------------- // Subscribe / SUBACK contract // --------------------------------------------------------------------------------- diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttTagDefinitionFactoryTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttTagDefinitionFactoryTests.cs index 9a16f744..74a41eec 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttTagDefinitionFactoryTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttTagDefinitionFactoryTests.cs @@ -177,6 +177,63 @@ public sealed class MqttTagDefinitionFactoryTests => MqttTagDefinitionFactory.FromTagConfig( $$"""{"topic":"a/b","payloadFormat":"Raw","qos":{{qosToken}}}""", RawPath, out _).ShouldBeFalse(); + /// + /// retainSeed is a BEHAVIOURAL key and is held to the same strictness as + /// qos / payloadFormat / dataType. It was the last one read leniently, so + /// "retainSeed":"false" — which a CSV import or a hand-edited blob produces naturally — + /// silently became , the OPPOSITE of what was authored: the tag then seeds + /// its initial value from the broker's retained message when the author asked it not to. + /// + /// The present-but-invalid retainSeed value. + [Theory] + [InlineData("\"false\"")] // stringly-typed bool — what a CSV import produces + [InlineData("\"true\"")] + [InlineData("0")] // wrong JSON type + [InlineData("1")] + [InlineData("\"yes\"")] + [InlineData("null")] // an explicit null is not "absent" + public void FromTagConfig_MalformedRetainSeed_RejectsStrict(string token) + => MqttTagDefinitionFactory.FromTagConfig( + $$"""{"topic":"a/b","payloadFormat":"Raw","retainSeed":{{token}}}""", RawPath, out _) + .ShouldBeFalse(); + + /// The Sparkplug reader reads the same key and must reach the same verdict. + /// The present-but-invalid retainSeed value. + [Theory] + [InlineData("\"false\"")] + [InlineData("0")] + [InlineData("null")] + public void FromSparkplugTagConfig_MalformedRetainSeed_RejectsStrict(string token) + => MqttTagDefinitionFactory.FromSparkplugTagConfig( + $$"""{"groupId":"G","edgeNodeId":"E","metricName":"M","retainSeed":{{token}}}""", RawPath, out _) + .ShouldBeFalse(); + + /// + /// Falsification control: the strict read must still take both real booleans and an absent key. + /// A reader that simply rejected every blob carrying the key would pass the theories above. + /// + [Theory] + [InlineData("true", true)] + [InlineData("false", false)] + public void FromTagConfig_WellFormedRetainSeed_IsStillHonoured(string token, bool expected) + { + MqttTagDefinitionFactory.FromTagConfig( + $$"""{"topic":"a/b","payloadFormat":"Raw","retainSeed":{{token}}}""", RawPath, out var def) + .ShouldBeTrue(); + def.RetainSeed.ShouldBe(expected); + } + + [Theory] + [InlineData("\"false\"")] + [InlineData("0")] + public void Inspect_MalformedRetainSeed_Warns(string token) + { + // Otherwise clean, so retainSeed is the ONLY thing that can produce a warning. + var warnings = MqttTagDefinitionFactory.Inspect( + $$"""{"topic":"a/b","payloadFormat":"Raw","retainSeed":{{token}}}"""); + warnings.ShouldHaveSingleItem().ShouldContain("retainSeed", Case.Sensitive); + } + [Fact] public void Inspect_CleanBlob_ReturnsEmpty() => MqttTagDefinitionFactory.Inspect( 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 d6ce97e9..20984a5b 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 @@ -1,3 +1,4 @@ +using System.Globalization; using System.Text; using Google.Protobuf; using Org.Eclipse.Tahu.Protobuf; @@ -218,6 +219,66 @@ public sealed class MqttWriteEncoderTests => MqttWriteEncoder.TryEncode(MqttMode.SparkplugB, Sparkplug("Filler1"), "not-a-number", out _) .ShouldBe(MqttWriteEncodeError.ValueNotRepresentable); + /// + /// A Sparkplug DateTime is an UNSIGNED milliseconds-since-epoch, so a pre-1970 instant has + /// no representation at all. The (ulong) cast wrapped the negative long around, so + /// writing 1969-12-31 published a timestamp roughly 584 million years in the future — a + /// coercion, and the file's own rule is that unrepresentable values are refused, never coerced. + /// + /// The authored instant, in UTC. + [Theory] + [InlineData("1969-12-31T23:59:59Z")] + [InlineData("1900-01-01T00:00:00Z")] + [InlineData("0001-01-01T00:00:00Z")] + public void A_pre_epoch_date_time_is_refused_rather_than_wrapped(string iso) + { + var value = DateTime.Parse(iso, CultureInfo.InvariantCulture, DateTimeStyles.AdjustToUniversal | DateTimeStyles.AssumeUniversal); + + MqttWriteEncoder.TryEncode( + MqttMode.SparkplugB, Sparkplug("Filler1", DriverDataType.DateTime), value, out _) + .ShouldBe(MqttWriteEncodeError.ValueNotRepresentable); + } + + /// + /// Falsification control for the epoch bound: a post-1970 instant still encodes, and encodes to + /// the right number. A guard that refused every DateTime would satisfy the theory above. + /// + [Fact] + public void A_post_epoch_date_time_still_encodes_to_its_unix_milliseconds() + { + var value = new DateTime(2026, 8, 10, 12, 0, 0, DateTimeKind.Utc); + + var payload = Payload.Parser.ParseFrom(EncodeOrThrow( + MqttMode.SparkplugB, Sparkplug("Filler1", DriverDataType.DateTime), value)); + + payload.Metrics[0].Datatype.ShouldBe((uint)TahuDataType.DateTime); + payload.Metrics[0].LongValue.ShouldBe((ulong)new DateTimeOffset(value).ToUnixTimeMilliseconds()); + } + + /// + /// A instant must encode to the SAME wire value on every + /// node. ToUniversalTime() treats an unspecified kind as LOCAL, so the identical authored + /// value encoded differently depending on the node's timezone — which the two nodes of a + /// redundant pair are not required to share. It is read as UTC rather than refused because + /// Convert.ToDateTime("2026-08-10T12:00:00") — an ordinary authored string — produces + /// exactly this kind. + /// + [Fact] + public void An_unspecified_kind_date_time_encodes_as_utc_not_as_node_local_time() + { + var unspecified = new DateTime(2026, 8, 10, 12, 0, 0, DateTimeKind.Unspecified); + var sameInstantUtc = DateTime.SpecifyKind(unspecified, DateTimeKind.Utc); + + var fromUnspecified = Payload.Parser.ParseFrom(EncodeOrThrow( + MqttMode.SparkplugB, Sparkplug("Filler1", DriverDataType.DateTime), unspecified)); + var fromUtc = Payload.Parser.ParseFrom(EncodeOrThrow( + MqttMode.SparkplugB, Sparkplug("Filler1", DriverDataType.DateTime), sameInstantUtc)); + + fromUnspecified.Metrics[0].LongValue.ShouldBe(fromUtc.Metrics[0].LongValue); + fromUnspecified.Metrics[0].LongValue + .ShouldBe((ulong)new DateTimeOffset(sameInstantUtc).ToUnixTimeMilliseconds()); + } + [Fact] public void A_type_with_no_command_encoding_is_refused_rather_than_stringified() // Reference has no Sparkplug wire representation. Falling back to String would put a value on diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/SparkplugTopicTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/SparkplugTopicTests.cs index acad75aa..e4ab8194 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/SparkplugTopicTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/SparkplugTopicTests.cs @@ -150,6 +150,78 @@ public sealed class SparkplugTopicTests public void ToTopicString_RoundTrips(string topic) => SparkplugTopic.Parse(topic).ToTopicString().ShouldBe(topic); + /// + /// must enforce the same per-segment rule + /// does. It validated only null/empty on + /// groupId/edgeNodeId and NOTHING at all on deviceId, so the type was not + /// round-trip-closed: a +/# id produced a topic MQTT-3.3.2-2 forbids in a PUBLISH, + /// and a / in a deviceId silently added a segment. Every production caller is + /// already guarded, so the invariant belongs on the type that owns it rather than on the next + /// caller to arrive. + /// + /// The group id to format with. + /// The edge-node id to format with. + /// The device id to format with, or . + [Theory] + [InlineData("Plant+1", "EdgeA", null)] + [InlineData("Plant#", "EdgeA", null)] + [InlineData("Plant1", "Edge+A", null)] + [InlineData("Plant1", "Edge#A", null)] + [InlineData("Plant1", "EdgeA", "Fill+er")] + [InlineData("Plant1", "EdgeA", "Fill#er")] + [InlineData("Ot/Sim", "EdgeA", null)] // two segments, not one — a DIFFERENT node than named + [InlineData("Plant1", "EdgeA", "Fill/er")] + [InlineData("Plant1", "EdgeA", "")] // would emit a trailing '/', which never re-parses + public void Format_SegmentThatIsNotALegalTopicSegment_Throws(string groupId, string edgeNodeId, string? deviceId) + => Should.Throw( + () => SparkplugTopic.Format(groupId, SparkplugMessageType.DCMD, edgeNodeId, deviceId)); + + [Theory] + [InlineData("host+1")] + [InlineData("host#")] + public void FormatState_HostIdThatIsNotALegalTopicSegment_Throws(string hostId) + => Should.Throw(() => SparkplugTopic.FormatState(hostId)); + + /// + /// Falsification control: the rule is the parser's IsValidSegment, not "reject anything + /// unusual". A guard that rejected every id would satisfy both theories above, and a Sparkplug id + /// is otherwise an opaque identifier — spaces, dots and dashes are all legitimate. + /// + /// A legal, if unusual, segment. + [Theory] + [InlineData("Plant 1")] + [InlineData("plant.one")] + [InlineData("plant-one_2")] + [InlineData("usiné")] + public void Format_UnusualButLegalSegments_AreStillAccepted(string id) + => SparkplugTopic.Parse(SparkplugTopic.Format(id, SparkplugMessageType.DCMD, id, id)) + .ShouldBe(new SparkplugTopic(SparkplugMessageType.DCMD, id, id, id, null)); + + /// + /// The closure property the asymmetry broke: anything emits + /// must parse back to the instance it was built from. It is asserted over the FORMATTER's own + /// output rather than over a hand-written topic list, so a future formatting change cannot leave + /// the corpus describing a shape the type no longer emits. + /// + [Fact] + public void Format_Output_AlwaysReParsesToTheSameInstance() + { + SparkplugMessageType[] deviceScoped = + [SparkplugMessageType.DBIRTH, SparkplugMessageType.DDATA, SparkplugMessageType.DDEATH, SparkplugMessageType.DCMD]; + + foreach (var type in Enum.GetValues()) + { + if (type == SparkplugMessageType.STATE) continue; + var deviceId = deviceScoped.Contains(type) ? "Filler 1" : null; + var topic = SparkplugTopic.Format("Plant 1", type, "Edge.A", deviceId); + + SparkplugTopic.Parse(topic).ShouldBe(new SparkplugTopic(type, "Plant 1", "Edge.A", deviceId, null)); + } + + SparkplugTopic.Parse(SparkplugTopic.FormatState("otopcua host 1")) + .ShouldBe(new SparkplugTopic(SparkplugMessageType.STATE, null, null, null, "otopcua host 1")); + } + [Fact] public void ToTopicString_LegacyStateTopic_NormalisesToV3Form() // Parsed from the legacy no-prefix form, but formatting always emits the v3.0 shape. diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs index 3481c394..019498e4 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs @@ -260,6 +260,37 @@ public sealed class TagConfigNonEnumParityTests TagConfigValidator.Validate(DriverTypeNames.Mqtt, blob).ShouldBeNull(); } + /// + /// MQTT's retainSeed, held to the same rule as qos: a JSON boolean or absent, and + /// nothing else. The stringly-typed form is what a CSV import produces, and a lenient read turned + /// it into the default true — the INVERSE of the authored intent. The model reads the key + /// leniently into its own nullable field, so this parity holds only because + /// TagConfigParseGate asks the driver's own reader rather than the model's typed fields. + /// + /// A blob whose only fault is retainSeed. + [Theory] + [InlineData("""{"topic":"otopcua/fixture/a","retainSeed":"false"}""")] + [InlineData("""{"topic":"otopcua/fixture/a","retainSeed":0}""")] + [InlineData("""{"topic":"otopcua/fixture/a","retainSeed":null}""")] + [InlineData("""{"groupId":"OtOpcUaSim","edgeNodeId":"EdgeA","metricName":"M","retainSeed":"false"}""")] + public void Mqtt_refuses_a_retainSeed_its_reader_refuses(string blob) + { + RuntimeAccepts(DriverTypeNames.Mqtt, blob).ShouldBeFalse(); + TagConfigValidator.Validate(DriverTypeNames.Mqtt, blob).ShouldNotBeNullOrEmpty(); + } + + /// The paired acceptances: both real booleans, and an absent key. + /// A blob whose retainSeed is well-formed or absent. + [Theory] + [InlineData("""{"topic":"otopcua/fixture/a","retainSeed":true}""")] + [InlineData("""{"topic":"otopcua/fixture/a","retainSeed":false}""")] + [InlineData("""{"topic":"otopcua/fixture/a"}""")] + public void Mqtt_accepts_a_retainSeed_its_reader_accepts(string blob) + { + RuntimeAccepts(DriverTypeNames.Mqtt, blob).ShouldBeTrue(); + TagConfigValidator.Validate(DriverTypeNames.Mqtt, blob).ShouldBeNull(); + } + // ============================================================================================= // 2. The advisory pin — the caution that shapes the whole implementation. // ============================================================================================= From 2dc2075d05111eb6be51c0d9853bbc33f35337eb Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 08:22:47 -0400 Subject: [PATCH 3/3] docs(code-review): close all 8 open Mqtt.Browser + Mqtt.Contracts findings Browser-002/-003/-004 fixed and -005 resolved as a documented constants change (eddf5f57); Contracts-004/-005/-006/-007 fixed (93f70504). Both module headers drop to 0 open findings. Each resolution records the disposition, the RED evidence, and the deviations from the recommendation where one was taken. --- code-reviews/Driver.Mqtt.Browser/findings.md | 81 ++++++++++-- .../Driver.Mqtt.Contracts/findings.md | 122 ++++++++++++++++-- 2 files changed, 185 insertions(+), 18 deletions(-) diff --git a/code-reviews/Driver.Mqtt.Browser/findings.md b/code-reviews/Driver.Mqtt.Browser/findings.md index 9308f7b3..b6f80e4f 100644 --- a/code-reviews/Driver.Mqtt.Browser/findings.md +++ b/code-reviews/Driver.Mqtt.Browser/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-08-10 | | Commit reviewed | `b990adae` | | Status | Reviewed | -| Open findings | 4 | +| Open findings | 0 | ## Scope @@ -86,7 +86,7 @@ four wildcard shapes, each asserting `PublishCountForTest == 0`. | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:469` | -| Status | Open | +| Status | Resolved | **Description:** The plain-mode and Sparkplug-mode paths disagree about what may become a tree label. `ObserveSparkplugBirth` runs every group/edge-node/device/metric identifier @@ -109,7 +109,26 @@ marker. The plain-mode marker text would need widening from "Some topics were to show" to cover malformed names — the Sparkplug branch already words its marker that way ("oversized or malformed names"), so the two can converge. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed in `eddf5f57` — `ObservePlainTopic`'s pre-pass now rejects a +segment carrying a control character as well as an oversized one, and the rule is single-sourced +in a new `HasControlCharacter` that `IsUsableLabel` also calls, so the two ingest paths cannot +drift again. The plain marker converges on the Sparkplug wording ("oversized or malformed +names"). Pinned by +`MqttBrowseSessionTests.PlainTopicSegmentWithAControlCharacter_IsRejectedWhole_AndSurfacesAMarker` +(every segment well under `MaxSegmentChars`, so the length guard cannot be what passes it) plus +`PlainTopic_EachControlCharacterForm_IsRejected` over `\n` / `\r` / `\t` / ESC / NUL — 6 RED +before the fix — with `PlainTopic_PrintableNonAsciiSegments_AreStillAccepted` as the +falsification control: the rule is *control characters*, not "anything unusual", and a topic +segment legitimately carries accents, spaces and punctuation. + +The second, smaller half — an **empty** topic level — was deliberately **not** converged with +`IsUsableLabel`'s non-empty rule, and `PlainTopic_WithAnEmptyLevel_IsStillRecorded` pins that +choice. `a//b` is legal MQTT and its terminal leaf is a real committable address (the commit +reads `AddressFields[topic]`, which carries the node's full path), so rejecting the whole topic +would cost the operator a legal address to tidy a blank row — a worse trade than the display +defect it fixes. The control-character half is the one that had to converge, because a label +carrying a newline breaks the row it renders in. The Sparkplug path has no equivalent case: every +id it labels came off `SparkplugTopic.TryParse`, which already rejects an empty segment. ### Driver.Mqtt.Browser-003 @@ -118,7 +137,7 @@ show" to cover malformed names — the Sparkplug branch already words its marker | Severity | Low | | Category | Concurrency & thread safety | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:685` | -| Status | Open | +| Status | Resolved | **Description:** `GetOrCreate` reads `_nodeCount` and compares it to `_nodeCap`, then increments after a successful `GetOrAdd`. The read and the increment are not atomic with @@ -138,7 +157,22 @@ pair with a single `Interlocked.Increment` compared against the cap, decrementin lost-race path (`!ReferenceEquals(child, created)`). Until then, a comment stating the single-dispatcher assumption the current shape relies on would be enough. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed in `eddf5f57` rather than commented — the recommendation's +own first option. `GetOrCreate` reserves the slot with a single `Interlocked.Increment` compared +against the cap and gives it back on **both** paths that do not consume it (the cap trip, and the +lost `GetOrAdd` race), so the count is exact rather than merely bounded. Pinned by +`MqttBrowseSessionTests.NodeCap_HoldsUnderConcurrentObservation`, which drives +`2 × ProcessorCount` dedicated threads through a `Barrier` at a cap of **1**, over 40 rounds: +against the read-then-increment shape it overshot to **31** nodes on round 0, and it stays at 1 +with the fix (falsifiability re-checked by reverting the production change after the test was +made fast). Dedicated threads rather than `Task.Run` on purpose — every worker blocks on the +barrier, and blocking that many pool threads makes the pool inject on its ~500 ms starvation +timer, which both slowed the test to 23 s and staggered the very arrivals it aligns. + +The second race the entry notes (a node visible in `Children` before `_byPath`) is unchanged and +still accepted: `ExpandAsync` returning empty for a node the tree already holds self-heals on the +next call, and the class remarks say so. The comment the recommendation offered as the minimum is +now moot — the shape no longer rests on a single-dispatcher assumption at all. ### Driver.Mqtt.Browser-004 @@ -147,7 +181,7 @@ single-dispatcher assumption the current shape relies on would be enough. | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:1079` | -| Status | Open | +| Status | Resolved | **Description:** `Project` orders a level by `n.Segment`, but the dictionary is keyed by `n.Path` — and the whole reason for that keying (documented on `GetOrCreate`) is that in @@ -167,7 +201,20 @@ and/or order folders before leaves so a device and a same-named metric are visua separated. Either is a one-line change; a test observing a device and an edge-node metric of the same name would pin it. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed in `eddf5f57` — `Project` now sorts +`.OrderBy(Segment).ThenBy(Path, Ordinal)`, making the order total. The folders-before-leaves +variant was not taken: the path tiebreaker already separates them (Ordinal puts `/` (0x2F) before +`:` (0x3A), so a device folder precedes its same-named node-level metric) without introducing a +second ordering concept. `Project`'s comment now states the reason instead of the over-claim the +entry quotes. + +⚠️ **The single-pair test the recommendation suggests passes vacuously**, and did: .NET string +hashing is randomised per process, so the tie order a segment-only sort falls through to is +whatever *that run's* `ConcurrentDictionary` yields — with one pair it lands the right way round +about half the time, and the first attempt at this test went GREEN against the unfixed code. +`SparkplugTree_SiblingsSharingALabel_AreOrderedDeterministically` therefore uses **24** colliding +device/metric pairs, observed metric-first, and asserts the whole level: it failed 3/3 runs +against the unfixed sort and passes with it. ### Driver.Mqtt.Browser-005 @@ -176,7 +223,7 @@ the same name would pin it. | Severity | Low | | Category | Performance & resource management | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser/MqttBrowseSession.cs:59`, `:81` | -| Status | Open | +| Status | Resolved | **Description:** The per-session memory ceiling is higher than the constants suggest at a glance. `DefaultNodeCap` is 50,000 nodes, and each `TopicNode` retains its full `Path` @@ -197,4 +244,20 @@ tree"). What is missing is any bound on the *aggregate* across sessions. is short enough to matter; if it does not, cap there rather than lowering `DefaultNodeCap`, which would make the picker less useful on a genuinely large broker. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Resolved as a **documentation change with no behaviour change**, in +`eddf5f57` — the recommendation's own verdict ("no code change needed in this module"). + +The confirmation it asks for was done, and the answer is **no**: `BrowseSessionRegistry` is a bare +`ConcurrentDictionary` that caps neither total sessions nor sessions per user. What bounds the +aggregate is **lifetime** — `BrowseSessionReaper` evicts and disposes anything untouched for a +2-minute `IdleTtl` on a 30-second tick, and `DisposeAsync` clears both maps — so the ceiling is +"sessions opened within a 2-minute window", not unbounded accumulation. That is short enough to +matter, so no cap was added; capping the session count there stays the right lever if one is ever +needed, rather than lowering `DefaultNodeCap`. + +`DefaultNodeCap`'s doc comment now carries the honest per-session arithmetic (each node retains +its full `Path`, its `Segment` and a snippet, plus its own child dictionary, and every node is +held twice — so a session near the ceilings costs on the order of 100 MB, not the few MB "50 000 +nodes" suggests), names the `#`-with-no-`TopicPrefix` open as the shape that approaches it, and +records where a future bound belongs. No test accompanies this: nothing behavioural changed, and +a test over a constant's doc comment would pin prose. diff --git a/code-reviews/Driver.Mqtt.Contracts/findings.md b/code-reviews/Driver.Mqtt.Contracts/findings.md index 348eb820..49269fab 100644 --- a/code-reviews/Driver.Mqtt.Contracts/findings.md +++ b/code-reviews/Driver.Mqtt.Contracts/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-08-10 | | Commit reviewed | `b990adae` | | Status | Reviewed | -| Open findings | 4 | +| Open findings | 0 | ## Scope @@ -162,7 +162,7 @@ deliberately excludes `metricName`); pinned by | Severity | Low | | Category | Documentation & comments | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttTagDefinitionFactory.cs:124`, `:234`, `:476` | -| Status | Open | +| Status | Resolved | **Description:** `retainSeed` is the one behavioural key still read leniently. `ReadBoolOrDefault` returns the default for any non-boolean JSON value, so @@ -186,7 +186,36 @@ so both gates stay in step; or (b) add an `Inspect` advisory naming the key and default; or (c) at minimum document the deliberate exception on `ReadBoolOrDefault`. Option (b) is lowest-risk — it changes no verdict. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed in `93f70504` — option (a), the strict read, chosen over the +"lowest-risk" advisory. The #549–#559 arc's settled contract is that **the parse is the accept +authority** and advisories ride blobs the parse has already ACCEPTED; an advisory here would have +left the driver still serving the inverse of what was authored, and the review's own reason for +recording rather than fixing ("the two surfaces are currently consistent") is satisfied without a +hand-mirrored rule — see below. Every sibling behavioural key in this factory (`payloadFormat`, +`dataType`, `qos`) is already strict, and `retainSeed` is the sharper case of the two: a defaulted +`qos` is a weaker delivery guarantee, whereas a defaulted `retainSeed` is the **inverse** of the +authored intent whenever the author wrote `false` — the only value worth writing at all. + +Both mappers now read it through a new `TryReadBoolStrict`, mirroring `TryReadQosStrict`'s +absent / valid / present-but-invalid split, and `DescribeRejection` gained a matching +`DescribeInvalidBool` branch (a rejection, never an advisory: unlike `payloadFormat`, both mappers +read the key, so it can never land on an accepted blob). + +**No second gate was bolted on outside Parse, and `MqttTagConfigModel` needed no mirrored rule.** +Its `Validate()` already ends in `TagConfigParseGate.Describe`, which asks *this* parse about the +blob the model was loaded from and words the refusal from *this* `Inspect` — so tightening the +factory tightened the authoring surface in the same change, with no drift surface created. The +model's `RetainSeed` doc now records that its own load stays lenient (a non-boolean reads as +"unset") and why that is safe. + +Pinned by `MqttTagDefinitionFactoryTests.FromTagConfig_MalformedRetainSeed_RejectsStrict` / +`FromSparkplugTagConfig_MalformedRetainSeed_RejectsStrict` (over `"false"`, `"true"`, `0`, `1`, +`"yes"`, `null`), `Inspect_MalformedRetainSeed_Warns`, and — as the falsification control that a +reader simply refusing every blob carrying the key would fail — +`FromTagConfig_WellFormedRetainSeed_IsStillHonoured`. 11 RED before the fix. The +authoring↔deploy parity is pinned separately, on the corpus that would catch a future divergence: +`TagConfigNonEnumParityTests.Mqtt_refuses_a_retainSeed_its_reader_refuses` and its paired +`Mqtt_accepts_a_retainSeed_its_reader_accepts`. ### Driver.Mqtt.Contracts-005 @@ -195,7 +224,7 @@ Option (b) is lowest-risk — it changes no verdict. | Severity | Low | | Category | Testing coverage | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttDriverOptions.cs:25`, `:71`, `:75`, `:79`, `:83`, `:99`, `:229` | -| Status | Open | +| Status | Resolved | **Description:** `MqttDriverOptions` / `MqttPlainOptions` carry seven `System.ComponentModel.DataAnnotations` `[Range]` attributes and **nothing ever runs them**. @@ -221,7 +250,35 @@ checks the probe and the AdminUI form already carry. If neither, note on each at that it is descriptive. The enforcing consumer (`MqttSubscriptionManager.BuildFilters`) lives in `Driver.Mqtt`, outside this module. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed in `93f70504` — the attributes were **deleted**, not wired up, +and the one consequence that actually escaped was closed at the enforcing consumer. + +**Wiring them would have broken deliberate behaviour**, which settles the "either/or" the +recommendation leaves open. Two of the seven *contradicted* the real authority: `[Range(1, …)]` +on `KeepAliveSeconds` forbids the **zero** `MqttDriver.Normalize` explicitly admits ("zero +keep-alive is legal MQTT; only a negative one is nonsense"), and the same attribute on +`MaxPayloadBytes` forbids the non-positive value this type's own remarks document as "falls back +to the driver's own 1 MiB default". Running the annotations would have refused two configurations +the driver is written to accept — the "two disagreeing authorities" outcome, arrived at by +enforcement rather than by neglect. So the bounds stay where they are applied +(`MqttDriver.Normalize`, `MqttSubscriptionManager`'s constructor, `MqttConnection.NextBackoff`), +each property names its own, and the type remarks record why there are no `DataAnnotations` here +and why adding one would be documentation that reads like enforcement. + +**`MqttPlainOptions.DefaultQos` — the escape the entry identifies — is now clamped** in +`MqttSubscriptionManager`'s constructor, the same single enforcement point that already bounds +`MaxPayloadBytes`. Clamp rather than fall back to the default, deliberately: it matches what +`MqttDriverFormModel.Normalize` already persists (`Math.Clamp(DefaultQos, 0, 2)`), so the +authoring surface and the driver cannot reach different answers for one authored number. Pinned +by `MqttSubscriptionManagerTests.DefaultQos_OutsideTheLegalRange_NeverReachesTheSubscribe`, which +asserts the QoS on the **fake transport's actual SUBSCRIBE** rather than on a property — `5 ⇒ 2` +and `-1 ⇒ 0` were RED, with `0 ⇒ 0` / `2 ⇒ 2` green throughout as the controls. + +Scope note: this closes the MQTT half only. The entry's observation that `[Range]` on driver +options is a **repo-wide** convention (`ModbusDriverOptions`, `S7DriverOptions`) still stands, and +those attributes are still unenforced — this change does not touch them, and the evidence above +argues the fleet-wide answer is deletion plus real bounds at each use site rather than a +composition-seam `Validator.TryValidateObject`. ### Driver.Mqtt.Contracts-006 @@ -230,7 +287,7 @@ lives in `Driver.Mqtt`, outside this module. | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/MqttWriteEncoder.cs:281` | -| Status | Open | +| Status | Resolved | **Description:** Two edge cases in the `DriverDataType.DateTime` write arm. First, `(ulong)…ToUnixTimeMilliseconds()` on a pre-1970 value wraps a negative `long` into a very @@ -250,7 +307,27 @@ negative, and convert with an explicit `DateTime.SpecifyKind(…, DateTimeKind.U `Unspecified` input (or refuse it, which is more in keeping with the file's own posture). Add a test for each. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Both fixed in `93f70504`. + +The epoch bound: `ToUnixTimeMilliseconds()` is now taken into a `long`, and a negative one returns +`ValueNotRepresentable` before anything is written to the metric — Sparkplug's `DateTime` is an +*unsigned* ms-since-epoch, so a pre-1970 instant has no representation at all and the surrounding +"refused, never coerced" rule applies unchanged. + +The kind: an `Unspecified` input is converted with `DateTime.SpecifyKind(…, DateTimeKind.Utc)` +rather than `ToUniversalTime()`. The entry offers refusal as "more in keeping with the file's own +posture", and it was not taken — `Convert.ToDateTime("2026-08-10T12:00:00")`, an ordinary authored +string, produces exactly this kind, so refusing would reject a perfectly reasonable write to fix a +determinism problem that reading it as UTC also fixes. + +Pinned by `MqttWriteEncoderTests.A_pre_epoch_date_time_is_refused_rather_than_wrapped` (three +instants, incl. `DateTime.MinValue`) with `A_post_epoch_date_time_still_encodes_to_its_unix_milliseconds` +as the falsification control — a guard that refused *every* `DateTime` would satisfy the theory +alone — and `An_unspecified_kind_date_time_encodes_as_utc_not_as_node_local_time`, which asserts +the Unspecified and the same-instant-UTC encodings are byte-identical. ⚠️ That last one can only +go red on a host whose local timezone is not UTC (it was RED here under both `EDT` and +`TZ=Asia/Tokyo`, and is green under `TZ=UTC` either way) — which is the defect itself, and the +reason it is written as an equality between two encodings rather than against a hard-coded number. ### Driver.Mqtt.Contracts-007 @@ -259,7 +336,7 @@ Add a test for each. | Severity | Low | | Category | Error handling & resilience | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Contracts/Sparkplug/SparkplugTopic.cs:202`, `:232` | -| Status | Open | +| Status | Resolved | **Description:** `SparkplugTopic` validates asymmetrically. `TryParse` enforces `IsValidSegment` (non-empty, no `+`/`#`) on every segment it reads, but `Format` and @@ -279,4 +356,31 @@ succeeded, so the encoder's own guards must stay — `TryEncode` returns an erro throwing, and that contract must not change. A round-trip property test (`Parse(t.ToTopicString()) == t` over the parser's own accepted corpus) would pin it. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed in `93f70504` — `Format` and `FormatState` now apply the +parser's own `IsValidSegment` to `groupId` / `edgeNodeId` / `deviceId` / `hostId` through a shared +`ThrowIfNotASegment`, throwing `ArgumentException` as the existing contract does. + +One deliberate addition beyond the recommendation's literal wording: the format-side check also +rejects **`/`**. `IsValidSegment` has no reason to test for one — `TryParse` has already split on +it — but on this side it is the difference between one id and two, and it is precisely the shape +the entry describes as "silently adds a segment". Without it the round-trip property the +recommendation asks for would not actually be total, since such a topic re-parses to nothing. + +The caution the entry raises is respected: `MqttWriteEncoder`'s own guards stay, so `TryEncode` +still *returns* `NoWriteDestination` for these ids rather than propagating a throw, and that +contract is unchanged. The one production caller that can now throw where it previously produced +a malformed topic is `SparkplugPrimaryHost.StateTopic` (a wildcard `hostId`); it is reached only +from `MqttDriver.StartPrimaryHostAsync`, which is wrapped in a catch that logs a warning and +leaves ingest untouched by design — so the effect is "the host claim declines with a reason" +instead of "publish a protocol violation and lose the connection". + +Pinned by `SparkplugTopicTests.Format_SegmentThatIsNotALegalTopicSegment_Throws` (9 cases: +`+`/`#` in each id, `/` in group and device, and an empty `deviceId`) and +`FormatState_HostIdThatIsNotALegalTopicSegment_Throws` — 9 RED before the fix — with +`Format_UnusualButLegalSegments_AreStillAccepted` as the falsification control (a Sparkplug id is +otherwise opaque: spaces, dots, dashes and accents are all legitimate, and a guard that refused +everything would satisfy the throw theories). The closure property is +`Format_Output_AlwaysReParsesToTheSameInstance`, asserted over the **formatter's own output** +across every non-STATE message type plus `FormatState`, rather than over a hand-written topic +list, so a future formatting change cannot leave the corpus describing a shape the type no longer +emits.