dohertj2/lmxopcua
1
0
Fork 0
Code Issues 32 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(mqtt-contracts): strict retainSeed, real bounds instead of dead [Range]s, DateTime edges, closed topic formatting

Browse Source 
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.
This commit is contained in:
Joseph Doherty
2026-08-10 08:18:30 -04:00
parent eddf5f57a5
commit 93f70504bd
11 changed files with 441 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttSubscriptionManagerTests.cs
+32
View File
@@ -423,6 +423,38 @@ public sealed class MqttSubscriptionManagerTests
press.SeedRetained.ShouldBeFalse();
}
/// <summary>
/// An out-of-range driver-level <c>defaultQos</c> 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 <c>MaxPayloadBytes</c> is bounded. The
/// <c>[Range(0, 2)]</c> attribute on <c>MqttPlainOptions.DefaultQos</c> never ran: nothing in the
/// four MQTT projects calls <c>Validator.TryValidateObject</c>, and the options arrive by
/// <c>JsonSerializer.Deserialize</c>, 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.
/// </summary>
/// <param name="authored">The authored driver-level default QoS.</param>
/// <param name="expected">The QoS the SUBSCRIBE must carry.</param>
[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
// ---------------------------------------------------------------------------------
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttTagDefinitionFactoryTests.cs
+57
View File
@@ -177,6 +177,63 @@ public sealed class MqttTagDefinitionFactoryTests
=> MqttTagDefinitionFactory.FromTagConfig(
$$"""{"topic":"a/b","payloadFormat":"Raw","qos":{{qosToken}}}""", RawPath, out _).ShouldBeFalse();
/// <summary>
/// <c>retainSeed</c> is a BEHAVIOURAL key and is held to the same strictness as
/// <c>qos</c> / <c>payloadFormat</c> / <c>dataType</c>. It was the last one read leniently, so
/// <c>"retainSeed":"false"</c> — which a CSV import or a hand-edited blob produces naturally —
/// silently became <see langword="true"/>, 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.
/// </summary>
/// <param name="token">The present-but-invalid <c>retainSeed</c> value.</param>
[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();
/// <summary>The Sparkplug reader reads the same key and must reach the same verdict.</summary>
/// <param name="token">The present-but-invalid <c>retainSeed</c> value.</param>
[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();
/// <summary>
/// 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.
/// </summary>
[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(
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttWriteEncoderTests.cs
+61
View File
@@ -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);
/// <summary>
/// A Sparkplug <c>DateTime</c> is an UNSIGNED milliseconds-since-epoch, so a pre-1970 instant has
/// no representation at all. The <c>(ulong)</c> cast wrapped the negative <c>long</c> around, so
/// writing <c>1969-12-31</c> 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.
/// </summary>
/// <param name="iso">The authored instant, in UTC.</param>
[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);
}
/// <summary>
/// Falsification control for the epoch bound: a post-1970 instant still encodes, and encodes to
/// the right number. A guard that refused every <c>DateTime</c> would satisfy the theory above.
/// </summary>
[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());
}
/// <summary>
/// A <see cref="DateTimeKind.Unspecified"/> instant must encode to the SAME wire value on every
/// node. <c>ToUniversalTime()</c> 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
/// <c>Convert.ToDateTime("2026-08-10T12:00:00")</c> — an ordinary authored string — produces
/// exactly this kind.
/// </summary>
[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
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/SparkplugTopicTests.cs
+72
View File
@@ -150,6 +150,78 @@ public sealed class SparkplugTopicTests
public void ToTopicString_RoundTrips(string topic)
=> SparkplugTopic.Parse(topic).ToTopicString().ShouldBe(topic);
/// <summary>
/// <see cref="SparkplugTopic.Format"/> must enforce the same per-segment rule
/// <see cref="SparkplugTopic.TryParse"/> does. It validated only null/empty on
/// <c>groupId</c>/<c>edgeNodeId</c> and NOTHING at all on <c>deviceId</c>, so the type was not
/// round-trip-closed: a <c>+</c>/<c>#</c> id produced a topic MQTT-3.3.2-2 forbids in a PUBLISH,
/// and a <c>/</c> in a <c>deviceId</c> 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.
/// </summary>
/// <param name="groupId">The group id to format with.</param>
/// <param name="edgeNodeId">The edge-node id to format with.</param>
/// <param name="deviceId">The device id to format with, or <see langword="null"/>.</param>
[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<ArgumentException>(
() => SparkplugTopic.Format(groupId, SparkplugMessageType.DCMD, edgeNodeId, deviceId));
[Theory]
[InlineData("host+1")]
[InlineData("host#")]
public void FormatState_HostIdThatIsNotALegalTopicSegment_Throws(string hostId)
=> Should.Throw<ArgumentException>(() => SparkplugTopic.FormatState(hostId));
/// <summary>
/// Falsification control: the rule is the parser's <c>IsValidSegment</c>, 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.
/// </summary>
/// <param name="id">A legal, if unusual, segment.</param>
[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));
/// <summary>
/// The closure property the asymmetry broke: anything <see cref="SparkplugTopic.Format"/> 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.
/// </summary>
[Fact]
public void Format_Output_AlwaysReParsesToTheSameInstance()
{
SparkplugMessageType[] deviceScoped =
[SparkplugMessageType.DBIRTH, SparkplugMessageType.DDATA, SparkplugMessageType.DDEATH, SparkplugMessageType.DCMD];
foreach (var type in Enum.GetValues<SparkplugMessageType>())
{
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.
tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs
+31
View File
@@ -260,6 +260,37 @@ public sealed class TagConfigNonEnumParityTests
TagConfigValidator.Validate(DriverTypeNames.Mqtt, blob).ShouldBeNull();
}
/// <summary>
/// MQTT's <c>retainSeed</c>, held to the same rule as <c>qos</c>: 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 <c>true</c> — the INVERSE of the authored intent. The model reads the key
/// leniently into its own nullable field, so this parity holds only because
/// <c>TagConfigParseGate</c> asks the driver's own reader rather than the model's typed fields.
/// </summary>
/// <param name="blob">A blob whose only fault is <c>retainSeed</c>.</param>
[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();
}
/// <summary>The paired acceptances: both real booleans, and an absent key.</summary>
/// <param name="blob">A blob whose <c>retainSeed</c> is well-formed or absent.</param>
[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.
// =============================================================================================