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

MQTT runtime TagConfig mappers reject leading-whitespace JSON that every other driver tolerates #559

New Issue
Closed
opened 2026-08-10 03:15:24 -04:00 by dohertj2 · 1 comment
dohertj2 commented 2026-08-10 03:15:24 -04:00
Owner
Copy Link
Copy Source

Found while closing #556 (merge be15ed16).

The inconsistency. MqttTagConfigMapper.FromTagConfig / FromSparkplugTagConfig carry a framing pre-filter (config[0] != '{' ⇒ reject) that runs on the untrimmed string, so a TagConfig blob with leading whitespace (" {\"topic\": ...}") is refused by the MQTT runtime — the tag faults. The other six structural-parse drivers (Modbus, S7, AbCip, AbLegacy, TwinCAT, FOCAS) hand the blob to JsonDocument.Parse, which skips leading whitespace per the JSON spec, so the identical blob shape is served everywhere except MQTT.

Why #556 did not fix it. #556's scope was the Inspect pre-filters (deploy-gate wording): all seven factories now TrimStart() before the framing check, and because MQTT's runtime shares the framing gate, its trimmed Inspect correctly reports the refusal as a genuine rejection — accept-gate-first symmetry holds. Changing the runtime mappers' served behavior was deliberately out of scope for a wording/classification issue.

Two ways to close, pick one:

  1. Align MQTT with the fleet — trim (or drop the framing gate in favour of letting JsonDocument.Parse decide) in the runtime mappers. Inspect stays accept-gate-first automatically. Cross-check MqttTagDefinitionFactoryInspectAcceptGateTests — the whitespace rejection case flips to acceptance.
  2. Declare the framing gate the contract and document that MQTT TagConfig must start with { at byte 0 — then nothing changes, but the cross-driver asymmetry is on record as intended.

Option 1 is almost certainly right: no author intends leading whitespace to be semantic, and CSV/API paths can introduce it invisibly.

Related: #556 (Inspect trim + rejection/advisory split, closed), #549/#554 (authoring-vs-runtime parity arc).

Found while closing #556 (merge `be15ed16`). **The inconsistency.** `MqttTagConfigMapper.FromTagConfig` / `FromSparkplugTagConfig` carry a framing pre-filter (`config[0] != '{'` ⇒ reject) that runs on the *untrimmed* string, so a TagConfig blob with leading whitespace (`" {\"topic\": ...}"`) is **refused by the MQTT runtime** — the tag faults. The other six structural-parse drivers (Modbus, S7, AbCip, AbLegacy, TwinCAT, FOCAS) hand the blob to `JsonDocument.Parse`, which skips leading whitespace per the JSON spec, so the identical blob shape is **served** everywhere except MQTT. **Why #556 did not fix it.** #556's scope was the *Inspect* pre-filters (deploy-gate wording): all seven factories now `TrimStart()` before the framing check, and because MQTT's runtime shares the framing gate, its trimmed Inspect correctly reports the refusal as a genuine rejection — accept-gate-first symmetry holds. Changing the *runtime* mappers' served behavior was deliberately out of scope for a wording/classification issue. **Two ways to close, pick one:** 1. **Align MQTT with the fleet** — trim (or drop the framing gate in favour of letting `JsonDocument.Parse` decide) in the runtime mappers. Inspect stays accept-gate-first automatically. Cross-check `MqttTagDefinitionFactoryInspectAcceptGateTests` — the whitespace rejection case flips to acceptance. 2. **Declare the framing gate the contract** and document that MQTT TagConfig must start with `{` at byte 0 — then nothing changes, but the cross-driver asymmetry is on record as intended. Option 1 is almost certainly right: no author intends leading whitespace to be semantic, and CSV/API paths can introduce it invisibly. Related: #556 (Inspect trim + rejection/advisory split, closed), #549/#554 (authoring-vs-runtime parity arc).
dohertj2 commented 2026-08-10 03:41:18 -04:00
Author
Owner
Copy Link
Copy Source

Closed by the merge of branch fix/559-mqtt-whitespace (fix commit 621a9e5d), verified on the merged tree: clean build + 4 affected suites, 2,790 tests, 0 failures (Mqtt 708, AdminUI 1466, ControlPlane 261, FOCAS 355).

Option 1 taken — MQTT aligned with the fleet. The || tagConfig[0] != '{' clause is removed from both runtime mappers (FromTagConfig, FromSparkplugTagConfig), keeping only the IsNullOrWhiteSpace guard. JsonDocument.Parse + the root-ValueKind == Object check are now the single framing authority, exactly as in the six sibling drivers. This changes the verdict ONLY for whitespace-padded JSON objects (which now earn their content's verdict); every other input class — bare names (JsonException), arrays/numbers/strings (ValueKind), blank — is refused by the same mechanisms as before. Caller sweep confirmed nothing relied on the first byte: MqttSubscriptionManager.Register, SparkplugIngestor.Register, MqttTagConfigModel.Validate (via TagConfigParseGate), and Inspect all pass the authored blob straight through with no name-vs-blob fallback.

Inspect needed no logic change — since #556 it trims before its own pre-filter and hands the RAW reference to the mappers, so its verdict followed the fix automatically. Its comments claiming " {…} is genuinely REFUSED by this driver" are rewritten.

Consequence handled: MQTT's backstop is now unreachable-today, like FOCAS's. The padded blob was the only production-reachable vehicle for the #556 advisory-cannot-swallow-backstop shape; no other exists (a union refusal with a topic requires a named strict-field failure; without topic and without the Sparkplug tuple, the no-address branch names it). The backstop stays as defence-in-depth and is re-pinned FOCAS-style as a count relation (A_rejected_config_always_draws_more_than_its_advisories_alone — a rejected blob draws strictly more than the same advisories an accepted twin draws), with the old constructed-vehicle test replaced by the stronger parity form Leading_whitespace_changes_neither_the_verdict_nor_the_inspection over the accepted AND rejected corpus, plus an anti-vacuity control proving the corpus carries both verdicts.

Falsification: each mapper sabotaged separately (framing clause re-introduced): the Plain sabotage reddened exactly 5 tests, all Plain-specific; the Sparkplug sabotage exactly 4, all Sparkplug-specific; nothing unrelated moved; both reverted and re-confirmed green. New runtime tests assert padded-vs-unpadded definition equality (record value equality), not just the accept bool.

One honest caveat on record: the three padded entries added to the AdminUI parity corpus stayed green under both sabotages — MqttTagConfigModel consults the same union parse via TagConfigParseGate, so authoring moves with runtime by construction. Those entries pin a future divergence (someone unwiring the parse-gate consult), not this defect; the load-bearing coverage is driver-side.

This was the sole open residual of the authoring-vs-deploy-gate drift arc (#549 → #550/#551 → #552/#553 → #554/#555 → #556/#557/#558 → #559). The arc is now closed end to end.

Closed by the merge of branch `fix/559-mqtt-whitespace` (fix commit `621a9e5d`), verified on the merged tree: clean build + 4 affected suites, 2,790 tests, 0 failures (Mqtt 708, AdminUI 1466, ControlPlane 261, FOCAS 355). **Option 1 taken — MQTT aligned with the fleet.** The `|| tagConfig[0] != '{'` clause is removed from both runtime mappers (`FromTagConfig`, `FromSparkplugTagConfig`), keeping only the `IsNullOrWhiteSpace` guard. `JsonDocument.Parse` + the root-`ValueKind == Object` check are now the single framing authority, exactly as in the six sibling drivers. This changes the verdict ONLY for whitespace-padded JSON objects (which now earn their content's verdict); every other input class — bare names (`JsonException`), arrays/numbers/strings (`ValueKind`), blank — is refused by the same mechanisms as before. Caller sweep confirmed nothing relied on the first byte: `MqttSubscriptionManager.Register`, `SparkplugIngestor.Register`, `MqttTagConfigModel.Validate` (via `TagConfigParseGate`), and `Inspect` all pass the authored blob straight through with no name-vs-blob fallback. **`Inspect` needed no logic change** — since #556 it trims before its own pre-filter and hands the RAW reference to the mappers, so its verdict followed the fix automatically. Its comments claiming " {…} is genuinely REFUSED by this driver" are rewritten. **Consequence handled: MQTT's backstop is now unreachable-today, like FOCAS's.** The padded blob was the only production-reachable vehicle for the #556 advisory-cannot-swallow-backstop shape; no other exists (a union refusal with a topic requires a named strict-field failure; without topic and without the Sparkplug tuple, the no-address branch names it). The backstop stays as defence-in-depth and is re-pinned FOCAS-style as a count relation (`A_rejected_config_always_draws_more_than_its_advisories_alone` — a rejected blob draws strictly more than the same advisories an accepted twin draws), with the old constructed-vehicle test replaced by the stronger parity form `Leading_whitespace_changes_neither_the_verdict_nor_the_inspection` over the accepted AND rejected corpus, plus an anti-vacuity control proving the corpus carries both verdicts. **Falsification:** each mapper sabotaged separately (framing clause re-introduced): the Plain sabotage reddened exactly 5 tests, all Plain-specific; the Sparkplug sabotage exactly 4, all Sparkplug-specific; nothing unrelated moved; both reverted and re-confirmed green. New runtime tests assert padded-vs-unpadded **definition equality** (record value equality), not just the accept bool. One honest caveat on record: the three padded entries added to the AdminUI parity corpus stayed green under both sabotages — `MqttTagConfigModel` consults the same union parse via `TagConfigParseGate`, so authoring moves with runtime by construction. Those entries pin a future *divergence* (someone unwiring the parse-gate consult), not this defect; the load-bearing coverage is driver-side. This was the sole open residual of the authoring-vs-deploy-gate drift arc (#549 → #550/#551 → #552/#553 → #554/#555 → #556/#557/#558 → #559). The arc is now closed end to end.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
auto-managed
queue management
 cross-driver
queue management
 driver/abcip
queue management
 driver/ablegacy
queue management
 driver/focas
queue management
 driver/mqtt
MQTT / Sparkplug B broker-subscribe driver
 driver/opcuaclient
queue management
 driver/s7
queue management
 driver/twincat
queue management
 phase/1
queue management
 phase/2
queue management
 phase/3
queue management
 phase/4
queue management
 phase/5
queue management
 phase/6
queue management
 queue/blocked
queue management
 queue/done
queue management
 queue/failed
queue management
 queue/in-progress
queue management
 queue/queued
queue management
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
dohertj2 (dohertj2)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: dohertj2/lmxopcua#559
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?