fix(mqtt): close #509, #510, #512, #513, #514

Five MQTT tracker items — four AdminUI/authoring defects plus the .Browser
layering exception. No runtime driver behaviour changes.

#513 payloadFormat in a Sparkplug blob. MqttTagConfigModel.ToJson wrote the key
unconditionally. Sharper than the issue states: PayloadFormat is the one Plain
field with a NON-NULLABLE typed backing, so the write did not preserve a retyped
tag's leftover — it INVENTED "payloadFormat":"Json" in a Sparkplug tag that never
had one. Now Plain-only; preserve-unknown-keys is pinned unchanged.

#509 slashed Sparkplug metric names. The mapper already carried metricName
verbatim; only the tag Name (a RawPath segment) tripped RawPaths.ValidateSegment,
which refused the batch whole — including Node Control/Rebirth, mandatory on
every Sparkplug edge node. ToRawSegment coerces the Name and leaves the address
untouched. Coercion is not injective, so DescribeNameCollisions refuses a
colliding set BY NAME: silently merging two metrics into one tag would be worse
than the refusal it replaces. Applies to every driver, not just MQTT — the
argument is about what a Name is, not about who produced it. The sanitiser is
pinned against RawPaths.ValidateSegment rather than a copied charset.

#510 stale rebirth affordance. _canRebirth was probed once at browse-open and
sessions are reaped for idleness, so a modal left open offered an action whose
token no longer resolved. Re-probed on arm AND on confirm (the reap can land
between the two, and confirm is the one path that publishes to a live broker);
the affordance retires with an explanation instead of vanishing or erroring.

#514 rebirth on a quiet plant. The tree is birth-driven and births are never
retained, so a plant that birthed before the window opened showed an empty tree —
and the scope came only from a tree selection, so the one action that would fill
the tree could not be armed. The session already accepted a bare {group}/{edgeNode}
("the prime rebirth target"); this adds the affordance. Edge-node scope only, not
a group broadcast. The rule is extracted to SparkplugRebirthScope so it is
testable — this repo has no bUnit — and is advisory: the session re-resolves and
refuses before publishing.

#512 .Browser -> .Driver layering. TWO extractions were needed, not the one the
issue anticipated: it costed this as "the client-options builder", but the browse
session decodes births itself, so moving only that would have left the reference
exactly where it was.
  - MqttClientOptionsFactory (TLS, PEM CA-pin validator, credentials, protocol
    mapping) -> new leaf .Transport project. NOT .Contracts: that is deliberately
    transport-free and MqttClientOptions is an MQTTnet type, so .Contracts would
    hand MQTTnet to ControlPlane. This is the fix the .Browser csproj comment had
    already prescribed, against the issue's suggestion.
  - The Sparkplug protocol layer (SparkplugTopic/MessageType, SparkplugCodec,
    RebirthRequester) -> .Contracts, beside the Tahu wire schema it encodes.
Still exactly one copy of the TLS/CA-pin logic; MqttBrowserLayeringTests guards
the graph both ways.

Measured, not assumed: Driver.Mqtt.dll, Polly.Core and Serilog are now absent
from the AdminUI's output. Core.dll remains, because Driver.Sql.Browser has the
SAME violation, undocumented — the old comment blamed this browser for Core and
was wrong. Filed separately rather than fixed in passing.

Full suite green apart from the 7 known pre-existing failures (4 AbLegacy
integration, compose profiles not up; 3 Host.IntegrationTests).

Claude-Session: https://claude.ai/code/session_015p7wGqy3YpZNCpDzTpGMKo
This commit is contained in:
Joseph Doherty
2026-07-31 06:17:18 -04:00
parent 3207089f01
commit 7f53bd96ec
23 changed files with 1029 additions and 269 deletions
@@ -25,7 +25,10 @@ public static class RawBrowseCommitMapper
/// <param name="driverType">The owning device's driver type (a <see cref="DriverTypeNames"/> value).</param>
/// <param name="fullName">The leaf's driver-side full reference (the <c>BrowseNode.NodeId</c> = the
/// captured <c>DriverAttributeInfo.FullName</c>) — becomes the driver-typed address field.</param>
/// <param name="browseName">The leaf's browse name — becomes the raw tag <c>Name</c> (a RawPath segment).</param>
/// <param name="browseName">The leaf's browse name. Becomes the raw tag <c>Name</c> after
/// <see cref="ToRawSegment"/> coerces it into a legal RawPath segment — a Sparkplug metric name
/// legitimately contains <c>/</c> and a Name may not (Gitea #509). The leaf's binding address is
/// unaffected: it travels separately into the <c>TagConfig</c>.</param>
/// <param name="driverDataType">The leaf's <see cref="DriverDataType"/> name (from the browse
/// attribute side-channel), or null when the browser cannot report a type (e.g. the OPC UA Client tree).</param>
/// <param name="defaultDataType">The OPC-UA built-in type name to fall back to when
@@ -52,6 +55,7 @@ public static class RawBrowseCommitMapper
{
var dataType = MapDataType(driverDataType) ?? defaultDataType;
var tagConfig = BuildTagConfig(driverType, fullName, addressFields);
var name = ToRawSegment(browseName);
var mirrored = createGroups && folderPath is { Count: > 0 }
? string.Join(RawPaths.Separator, folderPath)
: null;
@@ -61,9 +65,99 @@ public static class RawBrowseCommitMapper
// no poll-group (batching is authored later).
return new RawTagImportRow(
groupPath,
new RawTagInput(browseName, dataType, TagAccessLevel.Read, WriteIdempotent: false, PollGroupId: null, tagConfig));
new RawTagInput(name, dataType, TagAccessLevel.Read, WriteIdempotent: false, PollGroupId: null, tagConfig));
}
/// <summary>
/// Coerces a browse leaf's name into a legal RawPath segment (Gitea #509): trims the ends and
/// replaces every <see cref="RawPaths.Separator"/> with <see cref="SeparatorReplacement"/>.
/// </summary>
/// <param name="browseName">The leaf's browse name, as the driver reported it.</param>
/// <returns>A <see cref="RawPaths.ValidateSegment"/>-clean segment; blank only if the input was blank.</returns>
/// <remarks>
/// <para>
/// <b>The Name and the address are two fields, and only the Name is constrained.</b> A tag Name is
/// a RawPath segment, so it may not contain <c>/</c>; the driver-side reference it binds by has no
/// such rule and is written into the <c>TagConfig</c> untouched by <see cref="BuildTagConfig"/>.
/// Refusing the commit outright conflated the two — and it refused a metric the Sparkplug B spec
/// makes <em>mandatory</em> on every edge node (<c>Node Control/Rebirth</c>), so the one metric an
/// operator is most likely to browse for was the one that could not be committed.
/// </para>
/// <para>
/// <b>This deliberately covers every driver, not just MQTT.</b> The argument is about what a Name
/// is, not about who produced it: any browser whose leaf name carries a <c>/</c> hits the same
/// wall, and for all of them the binding address travels separately and survives intact.
/// </para>
/// <para>
/// <b>The rule set is <see cref="RawPaths.ValidateSegment"/>'s, not an invented one</b> — empty,
/// an embedded separator, and leading/trailing whitespace are exactly what it rejects, so trimming
/// and replacing is total rather than a best-effort guess. <c>RawBrowseCommitMapperTests</c> pins
/// that by asserting the output validates, so a new rule there fails here instead of silently
/// letting an illegal Name through to the import.
/// </para>
/// <para>
/// Sanitising is not injective, so two distinct leaves can land on one Name — see
/// <see cref="DescribeNameCollisions"/>, which is why a caller must run that check rather than
/// trusting this in isolation.
/// </para>
/// </remarks>
public static string ToRawSegment(string? browseName)
{
var trimmed = (browseName ?? "").Trim();
return trimmed.Length == 0 ? "" : trimmed.Replace(RawPaths.Separator, SeparatorReplacement);
}
/// <summary>What <see cref="ToRawSegment"/> substitutes for a separator inside a browse name.</summary>
private const char SeparatorReplacement = '_';
/// <summary>
/// Names the browse leaves that <see cref="ToRawSegment"/> would land on one tag Name within one
/// target group, or an empty list when the mapped rows are all distinct.
/// </summary>
/// <param name="browseNames">The ORIGINAL leaf names, in the same order as <paramref name="rows"/>.</param>
/// <param name="rows">The mapped import rows.</param>
/// <returns>One operator-facing message per colliding set; empty when there is nothing to refuse.</returns>
/// <remarks>
/// Sanitising maps many names onto one — <c>A/B</c> and <c>A_B</c> both become <c>A_B</c> — and a
/// silent collision is materially worse than the refusal #509 replaced: two distinct metrics would
/// become one tag bound to whichever address the import happened to keep. So the coercion is only
/// safe when paired with this check.
/// <para>
/// Scoped per target group, because the Name only has to be unique within its group — the same
/// metric name under two devices is not a collision.
/// </para>
/// <para>
/// The message names the specific pair rather than failing the batch anonymously. The commit
/// itself stays all-or-nothing, matching the CSV import's staged-review semantics: nothing is
/// written while any row is unresolvable.
/// </para>
/// </remarks>
public static IReadOnlyList<string> DescribeNameCollisions(
IReadOnlyList<string> browseNames,
IReadOnlyList<RawTagImportRow> rows)
{
ArgumentNullException.ThrowIfNull(browseNames);
ArgumentNullException.ThrowIfNull(rows);
return rows
.Select((row, i) => (row, original: i < browseNames.Count ? browseNames[i] : row.Tag.Name))
.GroupBy(x => (x.row.TagGroupPath ?? "", x.row.Tag.Name), TupleComparer)
.Where(g => g.Count() > 1)
.Select(g =>
$"'{g.Key.Item2}' would be the name of {g.Count()} different tags"
+ (string.IsNullOrEmpty(g.Key.Item1) ? "" : $" in group '{g.Key.Item1}'")
+ $": {string.Join(", ", g.Select(x => $"'{x.original}'"))}. "
+ "Their names differ only by characters that are not legal in a tag name. Commit them "
+ "separately, or author them by manual entry with distinct names.")
.ToList();
}
/// <summary>Ordinal comparer for the (group path, name) collision key — RawPath comparison is ordinal.</summary>
private static readonly IEqualityComparer<(string, string)> TupleComparer =
EqualityComparer<(string, string)>.Create(
(a, b) => RawPaths.Comparer.Equals(a.Item1, b.Item1) && RawPaths.Comparer.Equals(a.Item2, b.Item2),
x => HashCode.Combine(RawPaths.Comparer.GetHashCode(x.Item1), RawPaths.Comparer.GetHashCode(x.Item2)));
/// <summary>
/// Maps a <see cref="DriverDataType"/> enum NAME (as reported by the browse attribute side-channel) to the
/// OPC-UA built-in type name a <see cref="RawTagInput.DataType"/> carries. Mirrors
@@ -196,10 +196,12 @@ public sealed class MqttTagConfigModel
/// <c>dataType</c> is the one key both modes write, from two different typed fields
/// (<see cref="DataType"/> for Plain — always present; <see cref="MetricDataType"/> for
/// Sparkplug — omitted unless the operator set an override): which field wins is decided by
/// <see cref="Mode"/> at write time. Every other key is unconditional — mode-inapplicable keys
/// (e.g. <c>topic</c> while Sparkplug, or <c>groupId</c> while Plain) are written/cleared exactly
/// as their typed field says, which naturally preserves a retyped tag's other-mode leftovers
/// untouched until the operator edits that field too.
/// <see cref="Mode"/> at write time. <c>payloadFormat</c> is the one key written for <b>Plain
/// only</b> (Gitea #513) — see its line below for why it alone needs the mode test. Every other
/// key is unconditional: mode-inapplicable keys (e.g. <c>topic</c> while Sparkplug, or
/// <c>groupId</c> while Plain) are written/cleared exactly as their typed field says, and because
/// each of those fields is blank-⇒-omit, an absent key stays absent while a retyped tag's real
/// other-mode leftovers survive untouched until the operator edits that field too.
/// </remarks>
/// <returns>The serialised TagConfig JSON string.</returns>
public string ToJson()
@@ -207,7 +209,14 @@ public sealed class MqttTagConfigModel
// 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);
// Plain-only (Gitea #513). Unlike every other Plain key here, PayloadFormat is a NON-NULLABLE
// typed field, so an unconditional write does not merely preserve a retyped tag's leftover — it
// INVENTS "payloadFormat":"Json" in the blob of a Sparkplug tag that never had one. The key names
// a decode path that Sparkplug does not use (MqttTagDefinitionFactory routes on the descriptor
// keys and never reads it), so it is a trap for anyone diagnosing a decode problem from the blob.
TagConfigJson.Set(_bag, PayloadFormatKey, Mode == MqttMode.SparkplugB ? null : (object)PayloadFormat);
TagConfigJson.Set(_bag, JsonPathKey, string.IsNullOrWhiteSpace(JsonPath) ? null : JsonPath.Trim());
TagConfigJson.Set(_bag, QosKey, Qos);
TagConfigJson.Set(_bag, RetainSeedKey, RetainSeed);