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. // =============================================================================================