From ff62b62a55b1c11d8fd84cdf4e5f0eec634b883e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 24 Jul 2026 17:37:13 -0400 Subject: [PATCH] refactor(mqtt): accept a blank jsonPath; guide with a seeded "$" instead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. Dropped the hard Validate rejection of a blank jsonPath under a Json payload: the runtime defaults it to the document root, which is the real "publisher puts a bare JSON scalar on the topic" case, so rejecting it made the editor refuse what the driver accepts — inverting "authoring surface accepts <=> publish accepts" — and blocked whole CSV-import batches, since this validator also gates RawManualTagEntryModal's review grid. Replaced with guidance: an UNAUTHORED tag (no topic AND no jsonPath) seeds the field with "$", so a fresh tag emits an explicit "jsonPath":"$" while an existing path-less tag keeps the key ABSENT and the driver defaults it. The wildcard-topic rule stays strict — a wildcard has no legitimate single-Tag interpretation. Also: omit a blank "topic" rather than writing "topic":"", and record why the _lastConfigJson guard diverges from the Modbus template — the landmine is a DERIVED, NON-PERSISTED UI FIELD INFERRED FROM BLOB CONTENT (Mode), which Task 24's Sparkplug field group will be tempted to add more of. Named the host-side dependency (RawTagModal only mutates through this callback) that keeps the staleness trade benign today. Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW --- .../Uns/TagEditors/MqttTagConfigEditor.razor | 18 ++++- .../Uns/TagEditors/MqttTagConfigModel.cs | 71 ++++++++++++++----- .../Uns/MqttTagConfigModelTests.cs | 46 ++++++++++-- 3 files changed, 109 insertions(+), 26 deletions(-) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Uns/TagEditors/MqttTagConfigEditor.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Uns/TagEditors/MqttTagConfigEditor.razor index 810fe331..416e58ed 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Uns/TagEditors/MqttTagConfigEditor.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Shared/Uns/TagEditors/MqttTagConfigEditor.razor @@ -133,8 +133,22 @@ apply(); _validationError = _m.Validate(); var json = _m.ToJson(); - // Keep the guard in OnParametersSet in step with what we just emitted, so the host echoing the - // new JSON back as a parameter cannot re-parse and clobber an in-progress edit. + + // Keep the OnParametersSet guard in step with what we just emitted, so the host echoing the new + // JSON straight back as a parameter cannot re-parse and clobber an in-progress edit. + // + // WHY THIS DIVERGES FROM THE MODBUS TEMPLATE (which does not do this): the general landmine is a + // DERIVED, NON-PERSISTED UI FIELD INFERRED FROM BLOB CONTENT. Mode is exactly that — it is + // re-inferred by FromJson from the Sparkplug keys and never serialised, so a plain re-parse of + // our own output would silently reset the operator's shape selection to Plain on the very next + // render. Modbus has no such field, which is why it needs no guard. Task 24's Sparkplug field + // group will be tempted to add more of them; each one needs this guard to hold. + // + // TRADE (does not manifest today): the editor will not pick up a change the HOST makes to the + // JSON while it is open. That is safe only because RawTagModal/RawManualTagEntryModal mutate + // _form.TagConfig exclusively through this callback. A host-side feature that rewrites the blob + // out-of-band — a "reformat JSON" action, a bulk address re-pick — would reintroduce staleness + // here and must re-key the guard (e.g. compare against a host-supplied revision, not the text). _lastConfigJson = json; await ConfigJsonChanged.InvokeAsync(json); } 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 6d52fbac..6b9e3b4c 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 @@ -38,6 +38,13 @@ public sealed class MqttTagConfigModel private const string QosKey = "qos"; private const string RetainSeedKey = "retainSeed"; + /// + /// The JSONPath the driver applies when the blob omits jsonPath — the document root. + /// Seeded into an unauthored tag's field so the "no extraction needed" case is one click away + /// rather than something the operator has to know. + /// + private const string RootJsonPath = "$"; + /// The Sparkplug B descriptor keys — authored by Task 24, preserved (never dropped) in P1. private static readonly string[] SparkplugKeys = ["groupId", "edgeNodeId", "deviceId", "metricName"]; @@ -57,8 +64,11 @@ public sealed class MqttTagConfigModel public MqttPayloadFormat PayloadFormat { get; set; } = MqttPayloadFormat.Json; /// - /// JSONPath selecting the value inside a payload. Blank ⇒ - /// the key is omitted and the driver applies the document root ($). + /// JSONPath selecting the value inside a payload. Blank is + /// legal and NOT a validation failure — the key is omitted and the driver applies the document + /// root ($), which is the real "publisher puts a bare JSON scalar on the topic" case. + /// An unauthored tag is seeded with $ by so that common case + /// is visible and one click away; see . /// public string JsonPath { get; set; } = ""; @@ -84,17 +94,29 @@ public sealed class MqttTagConfigModel /// original key (so fields this editor doesn't expose — history intent, array/alarm objects, and /// the Sparkplug descriptor keys — survive a load→save). /// + /// + /// An unauthored Plain tag — no topic AND no jsonPath, i.e. a brand-new tag + /// rather than an existing one the operator deliberately left path-less — has its + /// seeded to . The condition is deliberately + /// narrow: an existing tag that carries a topic and no jsonPath keeps the key ABSENT + /// through a load→save, so this can never rewrite already-deployed blobs. + /// /// The raw TagConfig JSON string, or null for a new/empty config. /// The populated . public static MqttTagConfigModel FromJson(string? json) { var o = TagConfigJson.ParseOrNew(json); + var topic = TagConfigJson.GetString(o, TopicKey) ?? ""; + var jsonPath = TagConfigJson.GetString(o, JsonPathKey) ?? ""; + var mode = InferMode(o); + if (mode == MqttMode.Plain && topic.Length == 0 && jsonPath.Length == 0) { jsonPath = RootJsonPath; } + return new MqttTagConfigModel { - Mode = InferMode(o), - Topic = TagConfigJson.GetString(o, TopicKey) ?? "", + Mode = mode, + Topic = topic, PayloadFormat = TagConfigJson.GetEnum(o, PayloadFormatKey, MqttPayloadFormat.Json), - JsonPath = TagConfigJson.GetString(o, JsonPathKey) ?? "", + JsonPath = jsonPath, DataType = TagConfigJson.GetEnum(o, DataTypeKey, DriverDataType.String), Qos = GetIntNullable(o, QosKey), RetainSeed = TagConfigJson.GetBoolNullable(o, RetainSeedKey), @@ -111,7 +133,9 @@ public sealed class MqttTagConfigModel /// The serialised TagConfig JSON string. public string ToJson() { - TagConfigJson.Set(_bag, TopicKey, Topic.Trim()); + // Blank ⇒ omit, like every other optional here: toggling the shape dropdown on an untouched tag + // must not leave a stray "topic":"" behind. + TagConfigJson.Set(_bag, TopicKey, string.IsNullOrWhiteSpace(Topic) ? null : Topic.Trim()); TagConfigJson.Set(_bag, PayloadFormatKey, PayloadFormat); TagConfigJson.Set(_bag, DataTypeKey, DataType); TagConfigJson.Set(_bag, JsonPathKey, string.IsNullOrWhiteSpace(JsonPath) ? null : JsonPath.Trim()); @@ -125,14 +149,26 @@ public sealed class MqttTagConfigModel /// Returns the first error, or null when the config is valid. /// /// - /// Two rules are deliberately stricter than the runtime parser, because this is an - /// authoring surface and the cheapest place to stop the mistake: a wildcard topic (the runtime - /// accepts it and Inspect only warns at deploy) and a Json payload with no - /// jsonPath (the runtime defaults to the document root). Everything else — required - /// topic, strict enums, QoS 0–2 — matches MqttTagDefinitionFactory exactly. The strict - /// enum/QoS checks read the ORIGINAL key bag, not the defaulted typed fields, so a blob that - /// never went through this editor (e.g. a CSV import) cannot pass validation while the driver - /// would reject it. + /// + /// Exactly ONE rule is deliberately stricter than the runtime parser: a wildcard topic, + /// which the runtime accepts and Inspect only warns about at deploy. It earns the + /// strictness because a wildcard has no legitimate single-Tag interpretation — one Tag holds one + /// value, and +/# would feed it from many source topics. Everything else — + /// required topic, strict enums, QoS 0–2 — matches MqttTagDefinitionFactory exactly. + /// + /// + /// A blank jsonPath under a Json payload is explicitly accepted, matching + /// the runtime's document-root default. Rejecting it would make this editor refuse a config the + /// driver handles happily — inverting the "authoring surface accepts ⇔ publish accepts" + /// principle — and, because this validator also gates the CSV-import review grid + /// (RawManualTagEntryModal), would block whole import batches over a sane default. The + /// operator is guided by the seeded $ from instead of a blocker. + /// + /// + /// The strict enum/QoS checks read the ORIGINAL key bag, not the defaulted typed fields, so a + /// blob that never went through this editor (e.g. a CSV import) cannot pass validation while the + /// driver would reject it. + /// /// /// An error message describing the validation failure, or null when valid. public string? Validate() @@ -153,11 +189,8 @@ public sealed class MqttTagConfigModel + "or the tag would be fed by every matching topic."; } - if (PayloadFormat == MqttPayloadFormat.Json && string.IsNullOrWhiteSpace(JsonPath)) - { - return "A jsonPath is required when the payload format is Json (use \"$\" for the whole document)."; - } - + // NB no jsonPath rule — see the remarks above. A blank path is the driver's document-root + // default, not an authoring error. return null; } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/MqttTagConfigModelTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/MqttTagConfigModelTests.cs index 3dfb3fbb..b803fbd4 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/MqttTagConfigModelTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/MqttTagConfigModelTests.cs @@ -30,7 +30,9 @@ public sealed class MqttTagConfigModelTests m.Mode.ShouldBe(MqttMode.Plain); m.Topic.ShouldBe(""); m.PayloadFormat.ShouldBe(MqttPayloadFormat.Json); - m.JsonPath.ShouldBe(""); + // An UNAUTHORED tag seeds the document-root path so the common "no extraction needed" case is + // visible and one click away — it is a guide, not a requirement (blank is accepted, see below). + m.JsonPath.ShouldBe("$"); m.DataType.ShouldBe(DriverDataType.String); m.Qos.ShouldBeNull(); m.RetainSeed.ShouldBeNull(); @@ -107,6 +109,15 @@ public sealed class MqttTagConfigModelTests json.ShouldNotContain("jsonPath"); } + [Fact] + public void ToJson_omits_a_blank_topic() + { + // Toggling the shape dropdown on an untouched tag must not leave a stray "topic":"" behind. + var json = new MqttTagConfigModel { PayloadFormat = MqttPayloadFormat.Raw }.ToJson(); + + json.ShouldNotContain("topic"); + } + [Fact] public void FromJson_then_ToJson_preserves_unknown_keys() { @@ -171,13 +182,38 @@ public sealed class MqttTagConfigModelTests => MqttTagConfigModel.FromJson($$"""{"topic":"{{topic}}","payloadFormat":"Raw","dataType":"String"}""") .Validate().ShouldNotBeNullOrWhiteSpace(); + // A blank jsonPath under a Json payload is ACCEPTED — the runtime defaults it to the document root + // ("$"), which is the real "publisher puts a bare JSON scalar on the topic" case. Rejecting it would + // make the editor refuse what the driver accepts, and would block CSV-import batches (this validator + // also gates RawManualTagEntryModal's review grid) over a sane default. [Fact] - public void Validate_JsonWithoutPath_Fails() + public void Validate_JsonWithoutPath_IsAccepted() => MqttTagConfigModel.FromJson("""{"topic":"a/b","payloadFormat":"Json","dataType":"Float64"}""") - .Validate().ShouldNotBeNullOrWhiteSpace(); + .Validate().ShouldBeNull(); + + // …and the key stays ABSENT through a load→save, so the driver applies its own default rather than + // this editor freezing "$" into an already-deployed blob. + [Fact] + public void ToJson_leaves_an_existing_blank_jsonPath_absent() + { + var json = MqttTagConfigModel + .FromJson("""{"topic":"a/b","payloadFormat":"Json","dataType":"Float64"}""") + .ToJson(); + + json.ShouldNotContain("jsonPath"); + } + + // The seed applies ONLY to an unauthored tag (no topic AND no jsonPath) — a brand-new tag emits an + // explicit "$" once the operator touches anything, which is the guided half of the deal. + [Fact] + public void ToJson_seeds_the_root_jsonPath_on_a_fresh_tag() + { + var m = MqttTagConfigModel.FromJson(null); + m.Topic = "a/b"; + + m.ToJson().ShouldContain("\"jsonPath\":\"$\""); + } - // Falsifiability control for the test above: the SAME blob with a jsonPath must validate clean, so - // the failure above is provably the missing jsonPath and not the topic or the dataType. [Fact] public void Validate_JsonWithPath_ReturnsNull() => MqttTagConfigModel.FromJson("""{"topic":"a/b","payloadFormat":"Json","jsonPath":"$","dataType":"Float64"}""")