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

fix(mqtt): browse-commit writes a bindable address + a Request-rebirth affordance

Browse Source 
Two gaps the Task 23/24 review found, both blocking Task 26's live gate.

Gap 1 — browse-commit produced a silently dead MQTT tag. RawBrowseCommitMapper
had no `Mqtt` case, so a committed leaf fell through to the generic
`{"address": …}` key. Neither MqttTagDefinitionFactory entry point reads
`address`: the tag deployed clean and reported BadNodeIdUnknown forever, with
no signal at commit time. Predates Task 23 (Plain was affected too), but Task 23
built the Sparkplug metric tree precisely so an operator could browse and commit
a binding.

The address is a DESCRIPTOR, not a reference string, and it cannot be recovered
from the browse node id: `{group}/{node}[/{device}]::{metric}` where a metric
name legitimately contains `/` (`Node Control/Rebirth`) — the ambiguity
MetricSeparator's remarks already name. So the session STATES it, via a new
`AttributeInfo.AddressFields` seam, and the mapper reads it. Which keys are
emitted is also how the mapper learns Plain vs Sparkplug — the driver type
reaching it is just `Mqtt`, and the mode lives on the driver config. Key names
are single-sourced in the new `MqttTagConfigKeys` (producer, mapper, factory),
with the literals pinned by a test so a symmetric rename cannot silently unbind
already-persisted blobs. A leaf with no stated address is refused at commit in
words rather than committed dead.

Gap 2 — RequestRebirthAsync had no UI. Task 23 shipped it backend-only; Task 26
step 1 assumes the button, and Task 25's runbook notes the picker tree stays
empty until a birth lands, so it is the only way to fill it on demand. Added to
the /raw browse modal: scope = the clicked tree node (device/metric resolve up to
their edge node, group fans out and is refused whole past 32), an explicit
two-click confirm naming the resolved scope and its consequence, the outcome or
the refusal shown rather than swallowed. Offered only for a Sparkplug session
(new `IRebirthCapableBrowseSession.RebirthAvailable` — one session class serves
both modes, so a type test alone would draw a button that can only throw) and
only to a DriverOperator; the server-side gate remains the boundary.

Tests: MQTT 545 → 581, AdminUI 781 → 800. The round-trip tests feed the emitted
blob to the REAL factory and were falsified by mutating the emitted key name.

Claude-Session: https://claude.ai/code/session_01GASWkNEi68FSCtvr6rLoEW
This commit is contained in:
Joseph Doherty
2026-07-24 23:43:07 -04:00
parent 767e7031b3
commit 8d9155682d
15 changed files with 988 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttBrowseSessionTests.cs
+94
View File
@@ -619,6 +619,100 @@ public sealed class MqttBrowseSessionTests
a.SecurityClass.ShouldContain("unsupported");
}
[Fact]
public async Task SparkplugAttributes_StateTheBindingTuple_SoTheCommitNeverParsesTheNodeId()
{
// The address of a Sparkplug tag is (group, edgeNode, device?, metric) — NOT the node id. The
// session already holds the decomposition (it parsed the birth topic), so it states it; the
// AdminUI's browse-commit mapper reads these keys and never splits the id.
await using var s = new MqttBrowseSession(MqttMode.SparkplugB);
s.ObserveBirthForTest("OtOpcUaSim", "EdgeA", "Filler1", "Temperature", SparkplugDataType.Float);
var a = (await s.AttributesAsync(
"OtOpcUaSim/EdgeA/Filler1::Temperature", TestContext.Current.CancellationToken)).ShouldHaveSingleItem();
a.AddressFields.ShouldNotBeNull();
a.AddressFields![MqttTagConfigKeys.GroupId].ShouldBe("OtOpcUaSim");
a.AddressFields[MqttTagConfigKeys.EdgeNodeId].ShouldBe("EdgeA");
a.AddressFields[MqttTagConfigKeys.DeviceId].ShouldBe("Filler1");
a.AddressFields[MqttTagConfigKeys.MetricName].ShouldBe("Temperature");
}
[Fact]
public async Task SparkplugAttributes_NodeLevelMetric_StatesNoDeviceId()
{
// An NBIRTH metric belongs to the edge node itself. An invented/blank device id would be a
// device that does not exist — the same reason the tree hangs it directly under the edge node.
await using var s = new MqttBrowseSession(MqttMode.SparkplugB);
s.ObserveBirthForTest("OtOpcUaSim", "EdgeA", null, "Uptime", SparkplugDataType.Int64);
var a = (await s.AttributesAsync(
"OtOpcUaSim/EdgeA::Uptime", TestContext.Current.CancellationToken)).ShouldHaveSingleItem();
a.AddressFields.ShouldNotBeNull();
a.AddressFields!.ContainsKey(MqttTagConfigKeys.DeviceId).ShouldBeFalse();
a.AddressFields[MqttTagConfigKeys.EdgeNodeId].ShouldBe("EdgeA");
}
[Fact]
public async Task SparkplugAttributes_MetricNameContainingASlash_IsStatedWhole()
{
// 'Node Control/Rebirth' is a canonical Sparkplug metric name. Its node id is
// 'OtOpcUaSim/EdgeA::Node Control/Rebirth' — un-splittable back into the tuple, which is why the
// tuple travels rather than the id.
await using var s = new MqttBrowseSession(MqttMode.SparkplugB);
s.ObserveBirthForTest("OtOpcUaSim", "EdgeA", null, "Node Control/Rebirth", SparkplugDataType.Boolean);
var a = (await s.AttributesAsync(
"OtOpcUaSim/EdgeA::Node Control/Rebirth", TestContext.Current.CancellationToken)).ShouldHaveSingleItem();
a.AddressFields.ShouldNotBeNull();
a.AddressFields![MqttTagConfigKeys.MetricName].ShouldBe("Node Control/Rebirth");
a.AddressFields[MqttTagConfigKeys.EdgeNodeId].ShouldBe("EdgeA");
}
[Fact]
public async Task SparkplugAttributes_DeviceAndNodeLevelMetricsOfTheSameName_StateDifferentAddresses()
{
// The two are siblings sharing a label ('EdgeA/Temp' the device folder vs 'EdgeA::Temp' the
// node-level metric). Anything that re-derived the address from the label would collapse them.
await using var s = new MqttBrowseSession(MqttMode.SparkplugB);
s.ObserveBirthForTest("OtOpcUaSim", "EdgeA", null, "Temp", SparkplugDataType.Float);
s.ObserveBirthForTest("OtOpcUaSim", "EdgeA", "Filler1", "Temp", SparkplugDataType.Float);
var nodeLevel = (await s.AttributesAsync(
"OtOpcUaSim/EdgeA::Temp", TestContext.Current.CancellationToken)).ShouldHaveSingleItem();
var deviceLevel = (await s.AttributesAsync(
"OtOpcUaSim/EdgeA/Filler1::Temp", TestContext.Current.CancellationToken)).ShouldHaveSingleItem();
nodeLevel.AddressFields!.ContainsKey(MqttTagConfigKeys.DeviceId).ShouldBeFalse();
deviceLevel.AddressFields![MqttTagConfigKeys.DeviceId].ShouldBe("Filler1");
}
[Fact]
public async Task PlainAttributes_StateTheTopic_AsTheAddress()
{
await using var s = new MqttBrowseSession(MqttMode.Plain);
s.ObserveTopicForTest("otopcua/fixture/oven/temp", "21.5");
var a = (await s.AttributesAsync(
"otopcua/fixture/oven/temp", TestContext.Current.CancellationToken)).ShouldHaveSingleItem();
a.AddressFields.ShouldNotBeNull();
a.AddressFields![MqttTagConfigKeys.Topic].ShouldBe("otopcua/fixture/oven/temp");
a.AddressFields.ContainsKey(MqttTagConfigKeys.MetricName).ShouldBeFalse(); // Plain has no metric
}
[Theory]
[InlineData(MqttMode.SparkplugB, true)]
[InlineData(MqttMode.Plain, false)]
public void RebirthAvailable_IsSparkplugOnly(MqttMode mode, bool expected)
{
// The picker draws its Request-rebirth affordance off this. A plain window has no re-announce
// action at all — RequestRebirthAsync refuses one — so offering the button would be a lie.
new MqttBrowseSession(mode).RebirthAvailable.ShouldBe(expected);
}
[Fact]
public async Task SparkplugAttributes_ForAFolderNode_AreEmpty()
{
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Tests/MqttTagDefinitionFactoryTests.cs
+38
View File
@@ -221,4 +221,42 @@ public sealed class MqttTagDefinitionFactoryTests
[InlineData("[1,2,3]")] // no leading '{' ⇒ not a TagConfig blob at all (mirrors Modbus)
public void Inspect_NotATagConfigBlob_ReturnsEmpty(string? reference)
=> MqttTagDefinitionFactory.Inspect(reference!).ShouldBeEmpty();
[Fact]
public void TagConfigKeys_AreTheWireNames_AndAreNotFreeToRename()
{
// MqttTagConfigKeys single-sources the address key names across the producer (the AdminUI
// browse-commit mapper) and the consumer (this factory) — which means a SYMMETRIC rename would
// keep every round-trip test green while silently unbinding every ALREADY-PERSISTED TagConfig
// blob in the config DB. These are wire names; pin them to the literals.
MqttTagConfigKeys.Topic.ShouldBe("topic");
MqttTagConfigKeys.GroupId.ShouldBe("groupId");
MqttTagConfigKeys.EdgeNodeId.ShouldBe("edgeNodeId");
MqttTagConfigKeys.DeviceId.ShouldBe("deviceId");
MqttTagConfigKeys.MetricName.ShouldBe("metricName");
}
[Fact]
public void TagConfigKeys_AreTheKeysTheFactoryActuallyReads()
{
// The pin above is only worth having if the factory really reads THESE keys — a blob written
// entirely from the constants must parse.
var blob = $$"""
{"{{MqttTagConfigKeys.GroupId}}":"G",
"{{MqttTagConfigKeys.EdgeNodeId}}":"E",
"{{MqttTagConfigKeys.DeviceId}}":"D",
"{{MqttTagConfigKeys.MetricName}}":"M"}
""";
MqttTagDefinitionFactory.FromSparkplugTagConfig(blob, RawPath, out var def).ShouldBeTrue();
def.GroupId.ShouldBe("G");
def.EdgeNodeId.ShouldBe("E");
def.DeviceId.ShouldBe("D");
def.MetricName.ShouldBe("M");
MqttTagDefinitionFactory
.FromTagConfig($$"""{"{{MqttTagConfigKeys.Topic}}":"a/b"}""", RawPath, out var plain)
.ShouldBeTrue();
plain.Topic.ShouldBe("a/b");
}
}
tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Browsing/BrowserSessionServiceTests.cs
+24
View File
@@ -290,6 +290,27 @@ public sealed class BrowserSessionServiceTests
() => service.RequestRebirthAsync(Guid.NewGuid(), "Plant1/EdgeA", CancellationToken.None));
}
[Fact]
public void CanRequestRebirth_IsTrue_OnlyForASessionThatAdvertisesTheAction()
{
// The picker draws its Request-rebirth affordance off this. A session type CAN re-announce
// (MqttBrowseSession implements the interface) while a given INSTANCE cannot — a plain-MQTT
// window publishes nothing, ever — so a type test alone would draw a button that only throws.
var registry = new BrowseSessionRegistry();
var sparkplug = new FakeRebirthCapableBrowseSession();
var plain = new FakeRebirthCapableBrowseSession { RebirthAvailable = false };
var plainOldSession = new FakeBrowseSession();
registry.Register(sparkplug);
registry.Register(plain);
registry.Register(plainOldSession);
var service = NewServiceWithAuthz(registry, authorized: true);
service.CanRequestRebirth(sparkplug.Token).ShouldBeTrue();
service.CanRequestRebirth(plain.Token).ShouldBeFalse();
service.CanRequestRebirth(plainOldSession.Token).ShouldBeFalse();
service.CanRequestRebirth(Guid.NewGuid()).ShouldBeFalse(); // unknown/reaped token
}
/// <summary>A browse session that records the rebirth scopes it was asked for.</summary>
private sealed class FakeRebirthCapableBrowseSession : IRebirthCapableBrowseSession
{
@@ -299,6 +320,9 @@ public sealed class BrowserSessionServiceTests
public int Published { get; init; } = 1;
/// <summary>Whether this fake session advertises the action (a plain-MQTT window does not).</summary>
public bool RebirthAvailable { get; init; } = true;
public List<string> Scopes { get; } = [];
public Task<int> RequestRebirthAsync(string scope, CancellationToken cancellationToken)
tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawBrowseCommitMapperTests.cs
+199
View File
@@ -5,6 +5,7 @@ using Xunit;
using ZB.MOM.WW.OtOpcUa.AdminUI.Uns;
using ZB.MOM.WW.OtOpcUa.Configuration.Enums;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
using ZB.MOM.WW.OtOpcUa.Driver.Mqtt;
namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns;
@@ -96,6 +97,204 @@ public sealed class RawBrowseCommitMapperTests
JsonNode.Parse(json)!.AsObject()["address"]!.GetValue<string>().ShouldBe("40001");
}
// ---- BuildTagConfig: MQTT (the address is a DESCRIPTOR, not a single reference string) --------
/// <summary>The Sparkplug address a browse session states for a metric under a device.</summary>
/// <param name="metricName">The metric name to state.</param>
/// <returns>The stated address fields.</returns>
private static Dictionary<string, string> SparkplugFields(string metricName = "Temperature") => new()
{
[MqttTagConfigKeys.GroupId] = "OtOpcUaSim",
[MqttTagConfigKeys.EdgeNodeId] = "EdgeA",
[MqttTagConfigKeys.DeviceId] = "Filler1",
[MqttTagConfigKeys.MetricName] = metricName,
};
[Fact]
public void BuildTagConfig_Mqtt_plain_writes_the_topic_key_the_factory_actually_reads()
{
// The generic "address" fallback produced a blob MqttTagDefinitionFactory cannot read at all —
// a browse-committed tag that deploys clean and then reports BadNodeIdUnknown forever.
var json = RawBrowseCommitMapper.BuildTagConfig(
"Mqtt",
"otopcua/fixture/oven/temp",
new Dictionary<string, string> { [MqttTagConfigKeys.Topic] = "otopcua/fixture/oven/temp" });
JsonNode.Parse(json)!.AsObject()["topic"]!.GetValue<string>().ShouldBe("otopcua/fixture/oven/temp");
}
[Fact]
public void BuildTagConfig_Mqtt_plain_falls_back_to_the_node_id_when_no_field_was_stated()
{
// A Plain browse node id IS the topic, so the fallback is exact — unlike Sparkplug, where the
// tuple is unrecoverable and DescribeUncommittableLeaf refuses instead.
var json = RawBrowseCommitMapper.BuildTagConfig("Mqtt", "otopcua/fixture/oven/temp");
JsonNode.Parse(json)!.AsObject()["topic"]!.GetValue<string>().ShouldBe("otopcua/fixture/oven/temp");
}
[Fact]
public void BuildTagConfig_Mqtt_sparkplug_writes_the_binding_tuple_from_the_stated_fields()
{
var json = RawBrowseCommitMapper.BuildTagConfig(
"Mqtt", "OtOpcUaSim/EdgeA/Filler1::Temperature", SparkplugFields());
var o = JsonNode.Parse(json)!.AsObject();
o["groupId"]!.GetValue<string>().ShouldBe("OtOpcUaSim");
o["edgeNodeId"]!.GetValue<string>().ShouldBe("EdgeA");
o["deviceId"]!.GetValue<string>().ShouldBe("Filler1");
o["metricName"]!.GetValue<string>().ShouldBe("Temperature");
o.ContainsKey("topic").ShouldBeFalse(); // a Sparkplug tag has no per-tag topic
o.ContainsKey("address").ShouldBeFalse(); // and never the generic fallback key
}
[Fact]
public void BuildTagConfig_Mqtt_sparkplug_node_level_metric_omits_deviceId_entirely()
{
var fields = SparkplugFields();
fields.Remove(MqttTagConfigKeys.DeviceId);
var json = RawBrowseCommitMapper.BuildTagConfig("Mqtt", "OtOpcUaSim/EdgeA::Temperature", fields);
JsonNode.Parse(json)!.AsObject().ContainsKey("deviceId").ShouldBeFalse();
}
[Fact]
public void BuildTagConfig_Mqtt_sparkplug_takes_the_metric_name_from_the_fields_not_the_node_id()
{
// THE case the whole seam exists for: a metric name containing '/' makes the node id
// '{group}/{edge}/{device}::{metric}' un-splittable — any parse would bind the wrong metric.
var json = RawBrowseCommitMapper.BuildTagConfig(
"Mqtt",
"OtOpcUaSim/EdgeA/Filler1::Node Control/Rebirth",
SparkplugFields("Node Control/Rebirth"));
var o = JsonNode.Parse(json)!.AsObject();
o["metricName"]!.GetValue<string>().ShouldBe("Node Control/Rebirth");
o["deviceId"]!.GetValue<string>().ShouldBe("Filler1");
}
[Fact]
public void BuildTagConfig_Mqtt_sparkplug_survives_a_group_id_containing_the_metric_separator()
{
// A group id is an arbitrary MQTT topic segment and MAY contain '::'. Splitting the node id on
// the first '::' would read the group as 'odd' and the metric as 'group/EdgeA::Temperature'.
var fields = SparkplugFields();
fields[MqttTagConfigKeys.GroupId] = "odd::group";
var json = RawBrowseCommitMapper.BuildTagConfig(
"Mqtt", "odd::group/EdgeA/Filler1::Temperature", fields);
var o = JsonNode.Parse(json)!.AsObject();
o["groupId"]!.GetValue<string>().ShouldBe("odd::group");
o["metricName"]!.GetValue<string>().ShouldBe("Temperature");
}
[Fact]
public void BuildTagConfig_Mqtt_sparkplug_blob_deserializes_through_the_REAL_driver_factory()
{
// The round trip that would have caught the defect: the committed blob is fed to the factory the
// deployed driver actually uses. Anything else is a test of the mapper agreeing with itself.
var json = RawBrowseCommitMapper.BuildTagConfig(
"Mqtt",
"OtOpcUaSim/EdgeA/Filler1::Node Control/Rebirth",
SparkplugFields("Node Control/Rebirth"));
MqttTagDefinitionFactory
.FromSparkplugTagConfig(json, "Plant/Mqtt/dev1/Rebirth", out var def)
.ShouldBeTrue();
def.Name.ShouldBe("Plant/Mqtt/dev1/Rebirth"); // v3: identity is the RawPath
def.GroupId.ShouldBe("OtOpcUaSim");
def.EdgeNodeId.ShouldBe("EdgeA");
def.DeviceId.ShouldBe("Filler1");
def.MetricName.ShouldBe("Node Control/Rebirth");
def.DataTypeAuthored.ShouldBeFalse(); // no dataType key ⇒ take the birth's declared type
}
[Fact]
public void BuildTagConfig_Mqtt_plain_blob_deserializes_through_the_REAL_driver_factory()
{
var json = RawBrowseCommitMapper.BuildTagConfig(
"Mqtt",
"otopcua/fixture/oven/temp",
new Dictionary<string, string> { [MqttTagConfigKeys.Topic] = "otopcua/fixture/oven/temp" });
MqttTagDefinitionFactory.FromTagConfig(json, "Plant/Mqtt/dev1/Temp", out var def).ShouldBeTrue();
def.Topic.ShouldBe("otopcua/fixture/oven/temp");
def.Name.ShouldBe("Plant/Mqtt/dev1/Temp");
}
[Fact]
public void The_round_trip_test_is_falsifiable_a_wrong_key_name_is_rejected_by_the_factory()
{
// Control: mutate the emitted key name (what the pre-fix mapper effectively did, writing
// "address") and the factory must REFUSE it. Without this, the round-trip assertions above could
// pass off any blob the factory happened to tolerate.
var wrong = new JsonObject
{
["group"] = "OtOpcUaSim", // not "groupId"
["edgeNodeId"] = "EdgeA",
["metricName"] = "Temperature",
}.ToJsonString();
MqttTagDefinitionFactory.FromSparkplugTagConfig(wrong, "Plant/Mqtt/dev1/T", out _).ShouldBeFalse();
// And the exact blob the pre-fix generic fallback produced.
var legacy = new JsonObject { ["address"] = "OtOpcUaSim/EdgeA/Filler1::Temperature" }.ToJsonString();
MqttTagDefinitionFactory.FromSparkplugTagConfig(legacy, "Plant/Mqtt/dev1/T", out _).ShouldBeFalse();
MqttTagDefinitionFactory.FromTagConfig(legacy, "Plant/Mqtt/dev1/T", out _).ShouldBeFalse();
}
[Fact]
public void DescribeUncommittableLeaf_refuses_an_mqtt_leaf_whose_address_was_never_stated()
{
// No stated address ⇒ no bindable tag. Failing here, in words, is the whole point: the old path
// committed it silently and the operator learned about it as a runtime BadNodeIdUnknown.
var error = RawBrowseCommitMapper.DescribeUncommittableLeaf("Mqtt", "Temperature", addressFields: null);
error.ShouldNotBeNull();
error!.ShouldContain("Temperature");
}
[Theory]
[InlineData(MqttTagConfigKeys.Topic, "otopcua/fixture/oven/temp")]
[InlineData(MqttTagConfigKeys.MetricName, "Temperature")]
public void DescribeUncommittableLeaf_accepts_either_stated_mqtt_shape(string key, string value)
=> RawBrowseCommitMapper
.DescribeUncommittableLeaf("Mqtt", "leaf", new Dictionary<string, string> { [key] = value })
.ShouldBeNull();
[Fact]
public void DescribeUncommittableLeaf_never_blocks_a_single_reference_driver()
{
// Every other driver's address IS the node id — they state no fields and must stay committable.
foreach (var driver in new[] { "OpcUaClient", "AbCip", "TwinCAT", "S7", "GalaxyMxGateway", "Modbus" })
RawBrowseCommitMapper.DescribeUncommittableLeaf(driver, "leaf", addressFields: null).ShouldBeNull();
}
[Fact]
public void MapLeaf_flows_the_stated_address_fields_into_the_committed_row()
{
var row = RawBrowseCommitMapper.MapLeaf(
driverType: "Mqtt",
fullName: "OtOpcUaSim/EdgeA/Filler1::Temperature",
browseName: "Temperature",
driverDataType: "Float32",
defaultDataType: "Double",
groupPrefix: null,
folderPath: new[] { "OtOpcUaSim", "EdgeA", "Filler1" },
createGroups: false,
addressFields: SparkplugFields());
row.Tag.Name.ShouldBe("Temperature");
row.Tag.DataType.ShouldBe("Float");
MqttTagDefinitionFactory
.FromSparkplugTagConfig(row.Tag.TagConfig, "Plant/Mqtt/dev1/Temperature", out var def)
.ShouldBeTrue();
def.MetricName.ShouldBe("Temperature");
}
// ---- CombineGroupPath -----------------------------------------------------------------------
[Theory]