From 5ca4f8041d5899f01c7c1a06357fc79a0876769c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 01:54:17 -0400 Subject: [PATCH] fix(adminui): authoring models consult the driver's own parse for non-enum TagConfig rules (#554) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #549/#551 made every typed editor judge its driver's enum KEYS against the runtime's own reader and stopped there. Every other reason a reader refuses a tag met no gate until deploy, or until the driver silently skipped the tag: an absent Modbus `region`, an `"address":"notanumber"`, an S7 `stringLength` of 300, a TwinCAT `dataType: "Structure"` (a VALID enum name the reader refuses), a Sql blob the parser cannot recognise as Sql at all, an MQTT `"qos":null`. Rather than hand-mirroring N more rules into N models — the drift #551 closed on the deploy side — each `TagConfigModel.Validate()` now asks its driver's own parse whether it would read the AS-LOADED blob, via the new `TagConfigParseGate`. One authority for the verdict AND the wording. Two cautions are load-bearing and are pinned by tests: - The gate is the PARSE VERDICT, never "the inspector said something". FOCAS, MQTT and Calculation inspectors emit advisories for blobs their reader ACCEPTS, so a non-empty-inspection gate would make the modal stricter than the runtime and leave rows nobody can save (#520/#535). Inspect is consulted only to word a refusal that already happened. - It judges the blob the model was LOADED FROM, not the typed fields — the lenient load ("address":"notanumber" ⇒ 0) is what hides the defect. Same capture-the-source shape as S7's `_dataTypeMissingInSource`. Deliberately-stricter editor rules were surveyed and KEPT, not aligned: AbLegacy's arrayLength > 256 (reader clamps, but the un-clamped value still sets ArrayDimensions), FOCAS's unparseable address (#540), MQTT wildcard topics + Sparkplug segment chars, and Calculation's trigger/50 ms floors. They are now recorded fleet-wide in a table the tests assert is still accurate. Behaviour change: Modbus `{}` is now refused. Modbus was the one mapped editor with no required field, and `{}` is exactly what the modal persists for a config nobody touched — the driver skips it and it reads BadNodeIdUnknown forever with the deploy green. Its pinned test asserted the old, weaker behaviour and is rewritten with the rationale. Falsification: neutering only the Modbus consult turns 14 tests red, all Modbus-scoped; every #549 enum test stays green. --- .../Uns/TagEditors/AbCipTagConfigModel.cs | 21 +- .../Uns/TagEditors/AbLegacyTagConfigModel.cs | 28 +- .../TagEditors/CalculationTagConfigModel.cs | 27 +- .../Uns/TagEditors/FocasTagConfigModel.cs | 24 +- .../Uns/TagEditors/MTConnectTagConfigModel.cs | 16 +- .../Uns/TagEditors/ModbusTagConfigModel.cs | 23 +- .../Uns/TagEditors/MqttTagConfigModel.cs | 28 +- .../TagEditors/OpcUaClientTagConfigModel.cs | 24 +- .../Uns/TagEditors/S7TagConfigModel.cs | 27 +- .../Uns/TagEditors/SqlTagConfigModel.cs | 21 +- .../Uns/TagEditors/TagConfigParseGate.cs | 89 +++ .../Uns/TagEditors/TwinCATTagConfigModel.cs | 20 +- .../Uns/ModbusTagConfigModelTests.cs | 15 +- .../Uns/RawTreeServiceWaveBTests.cs | 30 +- .../Uns/TagConfigNonEnumParityTests.cs | 570 ++++++++++++++++++ .../Uns/TagConfigValidatorTests.cs | 22 +- 16 files changed, 940 insertions(+), 45 deletions(-) create mode 100644 src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/TagConfigParseGate.cs create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbCipTagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbCipTagConfigModel.cs index b4cd3f43..eb69b70b 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbCipTagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbCipTagConfigModel.cs @@ -22,6 +22,10 @@ public sealed class AbCipTagConfigModel private JsonObject _bag = new(); + // The blob this model was LOADED FROM, or null when built by object initialiser — the input to the + // Gitea #554 parse consult (see TagConfigParseGate). + private string? _source; + /// Loads a model from a TagConfig JSON string, defaulting any absent field and retaining /// every original key (so fields this editor doesn't expose survive a load→save). /// The TagConfig JSON string to parse, or null/empty for a new tag. @@ -35,6 +39,7 @@ public sealed class AbCipTagConfigModel TagPath = TagConfigJson.GetString(o, "tagPath") ?? "", DataType = TagConfigJson.GetEnum(o, "dataType", AbCipDataType.DInt), Writable = TagConfigJson.GetBoolNullable(o, "writable"), + _source = json ?? "", _bag = o, }; } @@ -48,7 +53,7 @@ public sealed class AbCipTagConfigModel TagConfigJson.Set(_bag, "tagPath", TagPath.Trim()); TagConfigJson.Set(_bag, "dataType", DataType); TagConfigJson.Set(_bag, "writable", Writable); - return TagConfigJson.Serialize(_bag); + return _source = TagConfigJson.Serialize(_bag); } /// Validation hook; returns an error message or null when the model is valid. @@ -76,6 +81,18 @@ public sealed class AbCipTagConfigModel { if (TagConfigJson.DescribeInvalidEnum(_bag, "dataType") is { } dtError) { return dtError; } if (string.IsNullOrWhiteSpace(TagPath)) { return "Tag path is required."; } - return AbCipTagDefinitionFactory.DescribeInvalidMember(TagConfigJson.Serialize(_bag)); + if (AbCipTagDefinitionFactory.DescribeInvalidMember(TagConfigJson.Serialize(_bag)) is { } memberError) + { + return memberError; + } + + // Gitea #554 backstop. The three checks above already mirror the whole of the reader's rejection + // surface, so this is expected to be silent for AbCip — it is here so a future reader rule cannot + // become a gap without anyone noticing, which is the failure #549/#551 kept repeating. + return TagConfigParseGate.Describe( + _source, + j => AbCipTagDefinitionFactory.FromTagConfig(j, TagConfigParseGate.AuthoringRawPath, out _), + AbCipTagDefinitionFactory.Inspect, + "AbCip"); } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbLegacyTagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbLegacyTagConfigModel.cs index c560ddf0..06d9d5c6 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbLegacyTagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/AbLegacyTagConfigModel.cs @@ -22,6 +22,10 @@ public sealed class AbLegacyTagConfigModel private JsonObject _bag = new(); + // The blob this model was LOADED FROM, or null when built by object initialiser — the input to the + // Gitea #554 parse consult (see TagConfigParseGate). + private string? _source; + /// Loads a model from a TagConfig JSON string, defaulting any absent field and retaining /// every original key (so fields this editor doesn't expose survive a load→save). /// The TagConfig JSON string to parse, or null/empty for a new tag. @@ -35,6 +39,7 @@ public sealed class AbLegacyTagConfigModel Address = TagConfigJson.GetString(o, "address") ?? "", DataType = TagConfigJson.GetEnum(o, "dataType", AbLegacyDataType.Int), Writable = TagConfigJson.GetBoolNullable(o, "writable"), + _source = json ?? "", _bag = o, }; } @@ -48,7 +53,7 @@ public sealed class AbLegacyTagConfigModel TagConfigJson.Set(_bag, "address", Address.Trim()); TagConfigJson.Set(_bag, "dataType", DataType); TagConfigJson.Set(_bag, "writable", Writable); - return TagConfigJson.Serialize(_bag); + return _source = TagConfigJson.Serialize(_bag); } /// Validation hook; returns an error message or null when the model is valid. @@ -66,15 +71,26 @@ public sealed class AbLegacyTagConfigModel if (string.IsNullOrWhiteSpace(Address)) return "Address is required."; - // Array-length cap: PCCC data-file elements are limited to AbLegacyArray.MaxElements (256). - // The driver-agnostic materialiser uses the raw arrayLength to set OPC UA ArrayDimensions, but the - // driver clamps to 256 at runtime — so an authored length > 256 produces a Part 6 §5.3.2 mismatch. - // Reject it here at author-time so the error surfaces in the TagModal before the config is saved. + // DELIBERATELY STRICTER THAN THE RUNTIME, and re-confirmed as such by the Gitea #554 survey — do not + // "align" it. PCCC data-file elements are limited to AbLegacyArray.MaxElements (256). The reader + // CLAMPS an authored length above that (Math.Min) and serves the tag, but the driver-agnostic + // materialiser sets OPC UA ArrayDimensions from the RAW authored length, so the served array is + // narrower than the node advertises — a Part 6 §5.3.2 mismatch, and the authored number is silently + // not honoured. Refusing at author-time, where the operator can still fix the number, is the honest + // treatment; the deploy gate deliberately stays silent about it (see AbLegacyTagDefinitionFactory + // .Inspect) because a warning there can be promoted to a blocking error against a tag the runtime + // does serve. var isArray = TagConfigJson.GetBool(_bag, "isArray"); var arrayLength = TagConfigJson.GetInt(_bag, "arrayLength"); if (isArray && arrayLength > AbLegacyArray.MaxElements) return $"AbLegacy array length cannot exceed {AbLegacyArray.MaxElements} (PCCC data-file element limit)."; - return null; + // Gitea #554 backstop: the checks above mirror the reader's whole rejection surface today, so this is + // expected to be silent for AbLegacy. It is here so a future reader rule cannot become a gap. + return TagConfigParseGate.Describe( + _source, + j => AbLegacyTagDefinitionFactory.FromTagConfig(j, TagConfigParseGate.AuthoringRawPath, out _), + AbLegacyTagDefinitionFactory.Inspect, + "AbLegacy"); } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/CalculationTagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/CalculationTagConfigModel.cs index 427acf9c..35d54543 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/CalculationTagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/CalculationTagConfigModel.cs @@ -1,4 +1,5 @@ using System.Text.Json.Nodes; +using ZB.MOM.WW.OtOpcUa.Driver.Calculation; namespace ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors; @@ -21,6 +22,10 @@ public sealed class CalculationTagConfigModel private JsonObject _bag = new(); + // The blob this model was LOADED FROM, or null when built by object initialiser — the input to the + // Gitea #554 parse consult (see TagConfigParseGate). + private string? _source; + /// Loads a model from a TagConfig JSON string, defaulting any absent field and retaining every /// original key (so fields this editor doesn't expose survive a load→save). /// The raw TagConfig JSON string, or null for a new/empty config. @@ -38,6 +43,7 @@ public sealed class CalculationTagConfigModel ScriptId = TagConfigJson.GetString(o, "scriptId") ?? "", ChangeTriggered = TagConfigJson.GetBool(o, "changeTriggered", true), TimerIntervalMs = timer, + _source = json ?? "", _bag = o, }; } @@ -51,11 +57,19 @@ public sealed class CalculationTagConfigModel TagConfigJson.Set(_bag, "scriptId", string.IsNullOrWhiteSpace(ScriptId) ? null : ScriptId); TagConfigJson.Set(_bag, "changeTriggered", ChangeTriggered); TagConfigJson.Set(_bag, "timerIntervalMs", TimerIntervalMs); - return TagConfigJson.Serialize(_bag); + return _source = TagConfigJson.Serialize(_bag); } /// Validates the calc binding: a script is required, at least one trigger (change or timer) /// must be set, and a set timer must be ≥ 50 ms. + /// + /// The trigger and 50 ms rules are DELIBERATELY STRICTER THAN THE RUNTIME, which serves a + /// trigger-less tag (it simply never computes) and honours a 1 ms cadence. The Gitea #554 survey + /// re-confirmed both and left them alone — they are documented on + /// CalculationTagDefinitionFactory as the reason its deploy gate must NOT restate them, since a + /// deploy warning can be promoted to a blocking error against a config the driver serves. The parse + /// consult below is additive and cannot weaken them. + /// /// An error message describing the validation failure, or null when the model is valid. public string? Validate() { @@ -71,6 +85,15 @@ public sealed class CalculationTagConfigModel { return "Timer interval must be at least 50 ms."; } - return null; + + // Gitea #554 backstop — expected silent for Calculation, whose reader refuses only a blob with no + // usable 'scriptId'. ⚠️ It gates on the PARSE VERDICT: this driver's Inspect emits advisories for + // blobs the reader ACCEPTS (a non-boolean 'changeTriggered', a discarded 'timerIntervalMs'), so a + // non-empty-inspection gate would refuse configs the runtime serves. + return TagConfigParseGate.Describe( + _source, + j => CalculationTagDefinitionFactory.TryReadTagConfig(j, out _), + CalculationTagDefinitionFactory.Inspect, + "Calculation"); } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/FocasTagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/FocasTagConfigModel.cs index 3a389e9b..c479498a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/FocasTagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/FocasTagConfigModel.cs @@ -18,6 +18,10 @@ public sealed class FocasTagConfigModel private JsonObject _bag = new(); + // The blob this model was LOADED FROM, or null when built by object initialiser — the input to the + // Gitea #554 parse consult (see TagConfigParseGate). + private string? _source; + /// Loads a model from a TagConfig JSON string, defaulting any absent field and retaining /// every original key (so fields this editor doesn't expose survive a load→save). /// The raw TagConfig JSON string, or null/empty for a fresh model. @@ -30,6 +34,7 @@ public sealed class FocasTagConfigModel DeviceHostAddress = TagConfigJson.GetString(o, "deviceHostAddress") ?? "", Address = TagConfigJson.GetString(o, "address") ?? "", DataType = TagConfigJson.GetEnum(o, "dataType", FocasDataType.Int32), + _source = json ?? "", _bag = o, }; } @@ -42,7 +47,7 @@ public sealed class FocasTagConfigModel TagConfigJson.Set(_bag, "deviceHostAddress", string.IsNullOrWhiteSpace(DeviceHostAddress) ? null : DeviceHostAddress.Trim()); TagConfigJson.Set(_bag, "address", Address.Trim()); TagConfigJson.Set(_bag, "dataType", DataType); - return TagConfigJson.Serialize(_bag); + return _source = TagConfigJson.Serialize(_bag); } /// Validation hook; returns an error message or null when the model is valid. @@ -67,7 +72,22 @@ public sealed class FocasTagConfigModel if (TagConfigJson.DescribeInvalidEnum(_bag, "dataType") is { } dtError) return dtError; if (string.IsNullOrWhiteSpace(Address)) return "Address is required."; - return FocasAddress.TryParse(Address) is null ? DescribeUnparseableAddress(Address) : null; + + // DELIBERATELY STRICTER THAN THE RUNTIME (Gitea #540), and the #554 survey re-confirmed it: the + // reader ACCEPTS an unparseable address — FocasTagDefinitionFactory.Inspect only advises about it — + // and the driver then skips the tag at startup. That is the defect #540 closed, so this check stays + // ahead of the parse consult below and is never replaced by it. + if (FocasAddress.TryParse(Address) is null) return DescribeUnparseableAddress(Address); + + // Gitea #554 backstop. ⚠️ It gates on the PARSE VERDICT, never on Inspect being non-empty: FOCAS's + // inspector emits advisories for blobs the reader ACCEPTS (an unparseable address, an authored + // 'writable':true on a read-only wire), and promoting those to authoring errors would make the modal + // stricter than the runtime and leave rows nobody can save. + return TagConfigParseGate.Describe( + _source, + j => FocasTagDefinitionFactory.FromTagConfig(j, TagConfigParseGate.AuthoringRawPath, out _), + FocasTagDefinitionFactory.Inspect, + "FOCAS"); } /// diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/MTConnectTagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/MTConnectTagConfigModel.cs index 2e83e343..b68a4429 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/MTConnectTagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/MTConnectTagConfigModel.cs @@ -79,6 +79,10 @@ public sealed class MTConnectTagConfigModel // there — the runtime's own Inspect() names the offending field for the same reason. private string _rawDataTypeKey = CanonicalDataTypeKey; + // The blob this model was LOADED FROM, or null when built by object initialiser — the input to the + // Gitea #554 parse consult (see TagConfigParseGate). Same capture-the-source pattern as _rawDataType. + private string? _source; + private JsonObject _bag = new(); /// @@ -141,6 +145,7 @@ public sealed class MTConnectTagConfigModel MtComponent = TagConfigJson.GetString(o, "mtComponent"), _rawDataType = rawDataType, _rawDataTypeKey = rawDataTypeKey, + _source = json ?? "", _bag = o, }; } @@ -172,7 +177,7 @@ public sealed class MTConnectTagConfigModel TagConfigJson.Set(_bag, "units", Blank(Units)); TagConfigJson.Set(_bag, "mtDevice", Blank(MtDevice)); TagConfigJson.Set(_bag, "mtComponent", Blank(MtComponent)); - return TagConfigJson.Serialize(_bag); + return _source = TagConfigJson.Serialize(_bag); } /// Validation hook; returns an error message or null when the model is valid. @@ -201,7 +206,14 @@ public sealed class MTConnectTagConfigModel + $"data type. Valid: {string.Join(", ", Enum.GetNames())}."; } - return null; + // Gitea #554 backstop — expected silent for MTConnect, whose reader refuses only an unparseable + // document, a JSON null, an id under none of the three spellings, and an unreadable coercion type, + // all covered above. It is here so a future reader rule cannot become a gap. + return TagConfigParseGate.Describe( + _source, + j => MTConnectTagDefinitionFactory.TryFromTagConfig(j, out _, out _), + MTConnectTagDefinitionFactory.Inspect, + "MTConnect"); } // The as-loaded coercion-type TOKEN as text (see the _rawDataType and DataTypeKeys remarks), read in diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/ModbusTagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/ModbusTagConfigModel.cs index dad040e5..47bbb1bd 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/ModbusTagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/ModbusTagConfigModel.cs @@ -32,6 +32,11 @@ public sealed class ModbusTagConfigModel private JsonObject _bag = new(); + // The blob this model was LOADED FROM, or null when it was built by object initialiser rather than + // loaded. See TagConfigParseGate's remarks: the lenient load above cannot be the input to the parse + // consult, because it is the leniency ("address":"notanumber" ⇒ 0) that hides the defect. + private string? _source; + /// Loads a model from a TagConfig JSON string, defaulting any absent field and retaining /// every original key (so fields this editor doesn't expose survive a load→save). /// The raw TagConfig JSON string, or null for a new/empty config. @@ -41,6 +46,7 @@ public sealed class ModbusTagConfigModel var o = TagConfigJson.ParseOrNew(json); return new ModbusTagConfigModel { + _source = json ?? "", Region = TagConfigJson.GetEnum(o, "region", ModbusRegion.HoldingRegisters), Address = TagConfigJson.GetInt(o, "address"), DataType = TagConfigJson.GetEnum(o, "dataType", ModbusDataType.Int16), @@ -84,11 +90,26 @@ public sealed class ModbusTagConfigModel /// ever reach the bag from a hand-authored blob, a CSV import or the API — exactly the paths /// this gate exists for. /// + /// + /// Everything the mapper refuses for a NON-enum reason is covered by the parse consult + /// (Gitea #554), not by rules restated here: an absent region, an absent or non-numeric or + /// out-of-0–65535 address, a String tag with no stringLength, a + /// BitInRegister tag whose bitIndex narrows above 15. Modbus was the one editor with + /// no required field at all, so {} — what the modal persists for a tag whose config was + /// never touched — validated clean and then read BadNodeIdUnknown forever. The enum checks + /// stay ahead of the consult because they name the offending spelling, which the mapper's boolean + /// verdict cannot. + /// /// /// An error message describing the validation failure, or null when the model is valid. public string? Validate() => TagConfigJson.DescribeInvalidEnum(_bag, "region") ?? TagConfigJson.DescribeInvalidEnum(_bag, "dataType") ?? TagConfigJson.DescribeInvalidEnum(_bag, "byteOrder") - ?? TagConfigJson.DescribeInvalidEnum(_bag, "stringByteOrder"); + ?? TagConfigJson.DescribeInvalidEnum(_bag, "stringByteOrder") + ?? TagConfigParseGate.Describe( + _source, + j => ModbusTagDefinitionFactory.FromTagConfig(j, TagConfigParseGate.AuthoringRawPath, out _), + ModbusTagDefinitionFactory.Inspect, + "Modbus"); } 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 f5ba7e68..521957c5 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 @@ -154,6 +154,10 @@ public sealed class MqttTagConfigModel private JsonObject _bag = new(); + // The blob this model was LOADED FROM, or null when built by object initialiser — the input to the + // Gitea #554 parse consult (see TagConfigParseGate). + private string? _source; + /// /// Loads a model from a TagConfig JSON string, defaulting any absent field and retaining every /// original key (so fields this editor doesn't expose — history intent, array/alarm objects, and @@ -193,6 +197,7 @@ public sealed class MqttTagConfigModel // Same "dataType" key as Plain's DataType above, read as an optional override — see the // MetricDataType doc comment for why there are two typed fields over one JSON key. MetricDataType = GetEnumNullable(o, DataTypeKey), + _source = json ?? "", _bag = o, }; } @@ -245,7 +250,7 @@ public sealed class MqttTagConfigModel TagConfigJson.Set(_bag, EdgeNodeIdKey, string.IsNullOrWhiteSpace(EdgeNodeId) ? null : EdgeNodeId.Trim()); TagConfigJson.Set(_bag, DeviceIdKey, string.IsNullOrWhiteSpace(DeviceId) ? null : DeviceId.Trim()); TagConfigJson.Set(_bag, MetricNameKey, string.IsNullOrWhiteSpace(MetricName) ? null : MetricName.Trim()); - return TagConfigJson.Serialize(_bag); + return _source = TagConfigJson.Serialize(_bag); } /// @@ -296,8 +301,27 @@ public sealed class MqttTagConfigModel /// An error message describing the validation failure, or null when valid. public string? Validate() { - if (Mode == MqttMode.SparkplugB) { return ValidateSparkplug(); } + var error = Mode == MqttMode.SparkplugB ? ValidateSparkplug() : ValidatePlain(); + if (error is not null) { return error; } + // Gitea #554 backstop, gating on the PARSE VERDICT (never on Inspect being non-empty — the MQTT + // inspector advises about a wildcard topic on blobs the reader ACCEPTS). The verdict is the UNION of + // the two readers, matching MqttTagDefinitionFactory.Inspect's own accept gate: a blob is served if + // EITHER shape reads it, and this model's Mode is only an inference from the descriptor keys. + // Expected to be near-silent — one shape it does newly catch is a `"qos": null`, which the reader + // refuses (Null is not a Number) while the model's own QoS check treats as absent. + return TagConfigParseGate.Describe( + _source, + j => MqttTagDefinitionFactory.FromTagConfig(j, TagConfigParseGate.AuthoringRawPath, out _) + || MqttTagDefinitionFactory.FromSparkplugTagConfig(j, TagConfigParseGate.AuthoringRawPath, out _), + MqttTagDefinitionFactory.Inspect, + "Mqtt"); + } + + /// Plain-mode half of — see its remarks for the full rationale. + /// An error message describing the validation failure, or null when valid. + private string? ValidatePlain() + { if (DescribeInvalidEnum(_bag, PayloadFormatKey) is { } pfError) { return pfError; } if (DescribeInvalidEnum(_bag, DataTypeKey) is { } dtError) { return dtError; } if (DescribeInvalidQos(_bag) is { } qosError) { return qosError; } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/OpcUaClientTagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/OpcUaClientTagConfigModel.cs index f3925231..ccf7d646 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/OpcUaClientTagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/OpcUaClientTagConfigModel.cs @@ -1,4 +1,5 @@ using System.Text.Json.Nodes; +using ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient; namespace ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors; @@ -24,6 +25,10 @@ public sealed class OpcUaClientTagConfigModel private JsonObject _bag = new(); + // The blob this model was LOADED FROM, or null when built by object initialiser — the input to the + // Gitea #554 parse consult (see TagConfigParseGate). + private string? _source; + /// Loads a model from a TagConfig JSON string, defaulting any absent field and retaining /// every original key (so fields this editor doesn't expose survive a load→save). /// The tag's TagConfig JSON (null/blank/malformed ⇒ defaults). @@ -34,6 +39,7 @@ public sealed class OpcUaClientTagConfigModel return new OpcUaClientTagConfigModel { NodeId = TagConfigJson.GetString(o, NodeIdKey) ?? "", + _source = json ?? "", _bag = o, }; } @@ -46,11 +52,25 @@ public sealed class OpcUaClientTagConfigModel public string ToJson() { TagConfigJson.Set(_bag, NodeIdKey, NodeId.Trim()); - return TagConfigJson.Serialize(_bag); + return _source = TagConfigJson.Serialize(_bag); } /// Validation hook; returns an error message or null when the model is valid. + /// + /// The Gitea #554 parse consult is a backstop here: the required-nodeId check above already + /// covers the reader's whole rejection surface, and this model is in fact stricter in one + /// narrow shape — the reader still honours a legacy PascalCase NodeId key when nodeId + /// is wholly absent, while this model reads only the camelCase key and so refuses that blob. That + /// predates #554 and is deliberately left alone; the fix for such a blob is to re-enter the + /// reference, which rewrites it to the v3 key. + /// /// An error message describing the validation failure, or null when valid. public string? Validate() - => string.IsNullOrWhiteSpace(NodeId) ? "An upstream node reference (nodeId) is required." : null; + => string.IsNullOrWhiteSpace(NodeId) + ? "An upstream node reference (nodeId) is required." + : TagConfigParseGate.Describe( + _source, + j => OpcUaClientTagDefinitionFactory.TryReadNodeId(j, out _), + OpcUaClientTagDefinitionFactory.Inspect, + "OpcUaClient"); } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs index f6a4f748..1692f39a 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/S7TagConfigModel.cs @@ -35,6 +35,10 @@ public sealed class S7TagConfigModel // load of a blob authored elsewhere (hand-edited, CSV import, API). private bool _dataTypeMissingInSource; + // The blob this model was LOADED FROM, or null when built by object initialiser — the parse consult's + // input (Gitea #554). Same capture-the-source reasoning as _dataTypeMissingInSource above. + private string? _source; + /// Loads a model from a TagConfig JSON string, defaulting any absent field and retaining /// every original key (so fields this editor doesn't expose survive a load→save). /// The raw TagConfig JSON string, or null/empty for a fresh model. @@ -49,6 +53,7 @@ public sealed class S7TagConfigModel StringLength = TagConfigJson.GetInt(o, "stringLength"), Writable = TagConfigJson.GetBoolNullable(o, "writable"), _dataTypeMissingInSource = !o.ContainsKey(S7TagDefinitionFactory.DataTypeKey), + _source = json ?? "", _bag = o, }; } @@ -63,7 +68,7 @@ public sealed class S7TagConfigModel _dataTypeMissingInSource = false; TagConfigJson.Set(_bag, "stringLength", StringLength); TagConfigJson.Set(_bag, "writable", Writable); - return TagConfigJson.Serialize(_bag); + return _source = TagConfigJson.Serialize(_bag); } /// Validation hook; returns an error message or null when the model is valid. @@ -100,10 +105,20 @@ public sealed class S7TagConfigModel if (string.IsNullOrWhiteSpace(Address)) { return "Address is required."; } - return _dataTypeMissingInSource - ? "A data type is required — the S7 driver rejects a tag whose TagConfig carries no " - + $"'{S7TagDefinitionFactory.DataTypeKey}' key (it would read BadNodeIdUnknown). Valid: " - + string.Join(", ", Enum.GetNames()) + "." - : null; + if (_dataTypeMissingInSource) + { + return "A data type is required — the S7 driver rejects a tag whose TagConfig carries no " + + $"'{S7TagDefinitionFactory.DataTypeKey}' key (it would read BadNodeIdUnknown). Valid: " + + string.Join(", ", Enum.GetNames()) + "."; + } + + // Gitea #554: the reader's remaining, non-enum refusal — a String tag whose 'stringLength' is + // negative or above 254 (S7 REJECTS it rather than clamping) — is not restated here. The verdict and + // the wording both come from the driver's own reader, via TagConfigParseGate. + return TagConfigParseGate.Describe( + _source, + j => S7TagDefinitionFactory.FromTagConfig(j, TagConfigParseGate.AuthoringRawPath, out _), + S7TagDefinitionFactory.Inspect, + "S7"); } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/SqlTagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/SqlTagConfigModel.cs index 4ac7c9c2..e55b6e28 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/SqlTagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/SqlTagConfigModel.cs @@ -50,6 +50,10 @@ public sealed class SqlTagConfigModel private string? _invalidModelRaw; private string? _invalidTypeRaw; + // The blob this model was LOADED FROM, or null when built by object initialiser — the input to the + // Gitea #554 parse consult (see TagConfigParseGate). + private string? _source; + /// Loads a model from a TagConfig JSON string, defaulting any absent field and retaining every /// original key (so fields this editor doesn't expose survive a load→save). /// The raw TagConfig JSON string, or null for a new/empty config. @@ -74,6 +78,7 @@ public sealed class SqlTagConfigModel RowSelector = SqlRowSelectorModel.FromJson(selectorObj), Type = type, _bag = o, + _source = json ?? "", _invalidModelRaw = invalidModel, _invalidTypeRaw = invalidType, }; @@ -98,7 +103,7 @@ public sealed class SqlTagConfigModel if (selector is null) { _bag.Remove("rowSelector"); } else { _bag["rowSelector"] = selector; } - return TagConfigJson.Serialize(_bag); + return _source = TagConfigJson.Serialize(_bag); } /// Validates the model against the exact accept/reject boundary of @@ -121,7 +126,7 @@ public sealed class SqlTagConfigModel if (string.IsNullOrWhiteSpace(KeyColumn)) { return "keyColumn is required for a KeyValue tag."; } if (string.IsNullOrWhiteSpace(ValueColumn)) { return "valueColumn is required for a KeyValue tag."; } if (KeyValue is null) { return "keyValue is required for a KeyValue tag."; } - return null; + break; case SqlTagModel.WideRow: if (string.IsNullOrWhiteSpace(ColumnName)) { return "columnName is required for a WideRow tag."; } @@ -132,11 +137,21 @@ public sealed class SqlTagConfigModel { return "a WideRow tag needs a row selector: a whereColumn/whereValue pair, or topByTimestamp."; } - return null; + break; default: return "model is not a supported Sql tag model."; } + + // Gitea #554: the parser's RECOGNITION check is the rule nothing above states, and it is the one Sql + // rejection an otherwise-complete blob can hit — TryParse refuses a config that carries neither a + // 'model' key nor "driver":"Sql", because it cannot tell a Sql tag from any other driver's blob. The + // editor always writes 'model' on save, so this only ever fires for a hand-authored / CSV / API blob. + return TagConfigParseGate.Describe( + _source, + j => SqlEquipmentTagParser.TryParse(j, TagConfigParseGate.AuthoringRawPath, out _), + SqlTagDefinitionFactory.Inspect, + "Sql"); } // Reads an enum-valued string field the way the parser's TryReadEnumStrict does: absent OR present-but- diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/TagConfigParseGate.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/TagConfigParseGate.cs new file mode 100644 index 00000000..1a344775 --- /dev/null +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/TagConfigParseGate.cs @@ -0,0 +1,89 @@ +namespace ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors; + +/// +/// The non-enum half of the authoring gate (Gitea #554): a driver's <Driver>TagConfigModel.Validate() +/// asks that driver's own runtime parse whether it would read the blob, and refuses the save when it +/// would not. One authority for the VERDICT and for the WORDING — the same shape #551 gave the deploy +/// gate, pointed at the other end of the pipe. +/// +/// Why the models could not simply grow more rules. Before this, Validate() judged the +/// enum keys (against the runtime's own DescribeInvalidEnumText, Gitea #549/#551) plus a few +/// hand-written required-field checks, and nothing else. Every remaining reason a driver's reader +/// refuses a tag — an absent region, an "address":"notanumber", an S7 +/// stringLength of 300, a TwinCAT dataType:"Structure" — met no gate until deploy, or +/// until the driver skipped the tag. Re-stating each of those in each model would have been N more +/// hand-mirrored rules to drift, which is exactly the defect #551 closed on the deploy side. +/// +/// +/// +/// +/// ⚠️ The gate is the PARSE verdict, never the inspector's output. An Inspect is +/// allowed to say things about a blob its parse ACCEPTS — FOCAS warns that an unparseable address +/// will be skipped and that writable:true cannot write, MQTT warns about a wildcard topic, +/// Calculation warns about a discarded timerIntervalMs. Treating a non-empty inspection as an +/// authoring error would make the modal STRICTER than the runtime and leave rows nobody can save — +/// the #520/#535 trap. So calls the inspector only once the parse has already +/// refused, purely to word the refusal. +/// +/// +/// ⚠️ It judges the blob the model was LOADED FROM, not the model's current fields. The +/// lenient load is what hides these defects in the first place — "address":"notanumber" loads +/// as 0 — so a check against the typed fields would accept exactly the blobs this exists to +/// refuse. It is the same capture-the-source reasoning as S7TagConfigModel's +/// _dataTypeMissingInSource, and it is also the honest input: on every authoring path the +/// string handed to TagConfigValidator is the string about to be persisted. +/// +/// +/// A model built by object initialiser rather than loaded (browse-commit's mapper, tests) passes +/// and is not parse-checked — it has no source, and its ToJson() output +/// is what gets persisted. That output is covered instead by the fleet acceptance-parity theory in +/// TagConfigAuthoringDeployParityTests, which drives every mapped driver's freshly-authored +/// blob through both gates. +/// +/// +internal static class TagConfigParseGate +{ + /// + /// The RawPath handed to a driver's parse during authoring inspection. Every mapper only copies it + /// onto the produced definition's identity and never validates it, so a placeholder is correct — the + /// gate is judging a blob, not a placed tag (the same reasoning as each factory's own + /// InspectionRawPath). + /// + internal const string AuthoringRawPath = "(authoring)"; + + /// + /// Returns an operator-facing error when refuses + /// , worded by ; when the + /// parse accepts it or when there is no source to judge. + /// + /// The blob the model was loaded from, or when it was not + /// loaded from JSON at all (see the class remarks). + /// The driver's own runtime parse, wrapped to a verdict. + /// The driver's Inspect, consulted ONLY after a refusal, for wording. + /// The driver name used by the backstop message. + /// The error message, or when the config is acceptable. + internal static string? Describe( + string? source, + Func parseAccepts, + Func> inspect, + string driverLabel) + { + ArgumentNullException.ThrowIfNull(parseAccepts); + ArgumentNullException.ThrowIfNull(inspect); + + if (source is null) { return null; } + + // A blank config is persisted as "{}" by every authoring path (RawTagModal, the manual-entry grid, + // RawTreeService), so that is the blob whose verdict the operator actually needs. + var blob = string.IsNullOrWhiteSpace(source) ? "{}" : source; + if (parseAccepts(blob)) { return null; } + + var warnings = inspect(blob); + return warnings.Count > 0 + ? string.Join(" ", warnings) + // Every inspector short-circuits on a blob that does not start with '{', and some cannot name + // every refusal their mapper makes. Saying so beats letting a tag the driver skips be saved. + : $"{driverLabel} TagConfig is rejected by the driver's tag reader — the driver would skip this " + + "tag and it would read BadNodeIdUnknown."; + } +} diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/TwinCATTagConfigModel.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/TwinCATTagConfigModel.cs index 4cdff97e..68880125 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/TwinCATTagConfigModel.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/TwinCATTagConfigModel.cs @@ -22,6 +22,10 @@ public sealed class TwinCATTagConfigModel private JsonObject _bag = new(); + // The blob this model was LOADED FROM, or null when built by object initialiser — the input to the + // Gitea #554 parse consult (see TagConfigParseGate). + private string? _source; + /// Loads a model from a TagConfig JSON string, defaulting any absent field and retaining /// every original key (so fields this editor doesn't expose survive a load→save). /// The TagConfig JSON string to parse, or null/empty for a fresh model. @@ -35,6 +39,7 @@ public sealed class TwinCATTagConfigModel SymbolPath = TagConfigJson.GetString(o, "symbolPath") ?? "", DataType = TagConfigJson.GetEnum(o, "dataType", TwinCATDataType.DInt), Writable = TagConfigJson.GetBoolNullable(o, "writable"), + _source = json ?? "", _bag = o, }; } @@ -48,7 +53,7 @@ public sealed class TwinCATTagConfigModel TagConfigJson.Set(_bag, "symbolPath", SymbolPath.Trim()); TagConfigJson.Set(_bag, "dataType", DataType); TagConfigJson.Set(_bag, "writable", Writable); - return TagConfigJson.Serialize(_bag); + return _source = TagConfigJson.Serialize(_bag); } /// Validation hook; returns an error message or null when the model is valid. @@ -60,10 +65,21 @@ public sealed class TwinCATTagConfigModel /// (BadNodeIdUnknown) where the load silently substitutes DInt. Accepting it here /// would let the CSV-import and API paths — which save the blob verbatim — deploy an unreadable /// tag (Gitea #549). + /// + /// dataType: "Structure" is the reason this model needed more than an enum check + /// (Gitea #554): it is a perfectly valid NAME, so every spelling + /// gate passes it, and the reader refuses it anyway — the driver does not serve UDT / FB-instance + /// tags. The rule is not restated here; the parse consult asks the reader and quotes its answer. + /// /// public string? Validate() { if (TagConfigJson.DescribeInvalidEnum(_bag, "dataType") is { } dtError) { return dtError; } - return string.IsNullOrWhiteSpace(SymbolPath) ? "Symbol path is required." : null; + if (string.IsNullOrWhiteSpace(SymbolPath)) { return "Symbol path is required."; } + return TagConfigParseGate.Describe( + _source, + j => TwinCATTagDefinitionFactory.FromTagConfig(j, TagConfigParseGate.AuthoringRawPath, out _), + TwinCATTagDefinitionFactory.Inspect, + "TwinCAT"); } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/ModbusTagConfigModelTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/ModbusTagConfigModelTests.cs index 353b2611..2bb03383 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/ModbusTagConfigModelTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/ModbusTagConfigModelTests.cs @@ -105,12 +105,19 @@ public sealed class ModbusTagConfigModelTests => ModbusTagConfigModel.FromJson(json).Validate().ShouldNotBeNull().ShouldContain(offending); /// The other direction, which matters just as much: the check is the runtime's rule and not a - /// stricter one. Valid names in any casing are accepted, an absent key is accepted (the runtime - /// defaults it), and so is a present-but-NON-STRING token — TryReadEnum calls that "absent" and - /// defaults it too, so refusing it here would block a blob the driver serves. + /// stricter one. Valid names in any casing are accepted, an absent dataType/byteOrder is + /// accepted (the runtime defaults them), and so is a present-but-NON-STRING token — TryReadEnum + /// calls that "absent" and defaults it too, so refusing it here would block a blob the driver serves. + /// + /// Gitea #554: every case carries a region KEY, because the reader requires one to be present + /// (it is the only enum here that is not defaulted from absent) and the model now asks the reader. + /// The bare {"address":1} this theory used to carry was never a blob the driver would serve — + /// it read BadNodeIdUnknown, which is the gap #554 closes. Note the third case keeps a + /// non-string region: present-enough for the reader, so still accepted by both. + /// [Theory] [InlineData("""{"region":"holdingregisters","address":1,"dataType":"float32","byteOrder":"wordswap"}""")] - [InlineData("""{"address":1}""")] + [InlineData("""{"region":"HoldingRegisters","address":1}""")] [InlineData("""{"region":3,"address":1,"dataType":true}""")] public void Validate_accepts_every_enum_value_the_driver_accepts(string json) => ModbusTagConfigModel.FromJson(json).Validate().ShouldBeNull(); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTreeServiceWaveBTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTreeServiceWaveBTests.cs index 77e0e51f..3315b79c 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTreeServiceWaveBTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/RawTreeServiceWaveBTests.cs @@ -31,16 +31,23 @@ public sealed class RawTreeServiceWaveBTests return selector(db); } - private static RawTagInput Tag(string name, string dataType = "Float", string tagConfig = "{}") => + /// + /// A TagConfig the Modbus driver would actually read — FolderedDeviceId hangs off + /// the seed's Modbus driver, and the service runs the driver-typed TagConfigValidator on every + /// tag it writes. Gitea #554 made the Modbus editor ask the driver's own reader, which requires a + /// region and an in-range address, so the bare {} this helper used to default to + /// is a config the driver skips (BadNodeIdUnknown) and the service now refuses. None of the + /// tests below is about the config blob — they are tree-structure and concurrency tests — so they + /// take the minimal readable one. + /// + private const string ModbusConfig = """{"region":"HoldingRegisters","address":1}"""; + + private static RawTagInput Tag(string name, string dataType = "Float", string tagConfig = ModbusConfig) => new(name, dataType, TagAccessLevel.ReadWrite, WriteIdempotent: false, PollGroupId: null, tagConfig); /// - /// A tag input whose TagConfig the S7 driver would actually read — RootDeviceId - /// hangs off the seed's S7 driver, and since Gitea #551 the service runs the driver-typed - /// TagConfigValidator on every tag it writes. S7 requires BOTH an address and a - /// present dataType key, so the bare {} the sibling helper defaults to is a config the - /// driver rejects and the service now refuses. The Modbus-device tests keep {}: Modbus reads - /// only enums out of the blob and accepts an empty one. + /// The same, for the S7 device — RootDeviceId hangs off the seed's S7 driver, which + /// requires BOTH an address and a present dataType key (Gitea #551). /// /// The tag name. /// The import-ready tag input. @@ -74,7 +81,7 @@ public sealed class RawTreeServiceWaveBTests var result = await service.CreateTagAsync( RawTreeTestDb.FolderedDeviceId, null, - new RawTagInput("flow", "Float", TagAccessLevel.Read, false, " ", "{}")); + new RawTagInput("flow", "Float", TagAccessLevel.Read, false, " ", ModbusConfig)); result.Ok.ShouldBeTrue(); using var db = RawTreeTestDb.CreateNamed(dbName); @@ -126,7 +133,12 @@ public sealed class RawTreeServiceWaveBTests var result = await service.UpdateTagAsync( RawTreeTestDb.PlainTagId, - new RawTagInput("running", "Boolean", TagAccessLevel.ReadWrite, true, "pg1", "{\"scale\":2}"), + // The unknown "scale" key rides a readable Modbus config: this test is about field edits and + // unknown-key survival, and since Gitea #554 the service's driver-typed gate refuses a blob the + // Modbus reader would skip. + new RawTagInput( + "running", "Boolean", TagAccessLevel.ReadWrite, true, "pg1", + """{"region":"HoldingRegisters","address":1,"scale":2}"""), rv); result.Ok.ShouldBeTrue(); diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs new file mode 100644 index 00000000..fcadcc90 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs @@ -0,0 +1,570 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip; +using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy; +using ZB.MOM.WW.OtOpcUa.Driver.Calculation; +using ZB.MOM.WW.OtOpcUa.Driver.FOCAS; +using ZB.MOM.WW.OtOpcUa.Driver.Modbus; +using ZB.MOM.WW.OtOpcUa.Driver.Mqtt; +using ZB.MOM.WW.OtOpcUa.Driver.MTConnect; +using ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient; +using ZB.MOM.WW.OtOpcUa.Driver.S7; +using ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts; +using ZB.MOM.WW.OtOpcUa.Driver.TwinCAT; + +namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns; + +/// +/// Gitea #554 — the NON-ENUM half of the authoring gate. Gitea #549/#551 made every typed editor judge +/// its driver's enum KEYS against the runtime's own reader, and stopped there: every other reason a +/// driver's reader refuses a tag — an absent Modbus region, an "address":"notanumber", an +/// S7 stringLength of 300, a TwinCAT dataType:"Structure", a Sql blob the parser cannot +/// recognise as Sql at all — met no gate until deploy, or until the driver skipped the tag. Each model +/// now consults its driver's own parse (see TagConfigParseGate). +/// +/// Every rejection below is paired with its nearest VALID sibling, so a gate that simply +/// refuses everything cannot pass this suite — the same discipline RawTagConfigGateTests uses. +/// +/// +/// ⚠️ The two directions are not symmetric, and the asymmetry is the design. +/// Authoring-accepts ⇒ runtime-accepts is an ABSOLUTE rule: a blob the modal lets an operator save +/// and the driver then skips is the defect this issue names. Runtime-accepts ⇒ authoring-accepts +/// holds only where the editor is not deliberately stricter — several are, for reasons that are +/// load-bearing and documented on the models themselves. Those are exempted BY NAME below, never by +/// weakening the theory. +/// +/// +[Trait("Category", "Unit")] +public sealed class TagConfigNonEnumParityTests +{ + // --------------------------------------------------------------------------------------------- + // The runtime side of the parity claim. + // + // This is a second dispatch map, and deliberately so: EquipmentTagConfigInspector cannot stand in for + // it, because three inspectors (FOCAS, MQTT, Calculation) emit ADVISORIES for blobs their parse + // accepts — "Inspect is empty" is therefore a strictly stronger statement than "the parse accepts", + // and using it here would quietly assert the very over-strictness caution this suite exists to guard. + // The map is covered by RuntimeParses_covers_every_mapped_driver below, so it cannot silently drift. + // --------------------------------------------------------------------------------------------- + private const string P = "(test)"; + + private static readonly IReadOnlyDictionary> RuntimeParses = + new Dictionary>(StringComparer.OrdinalIgnoreCase) + { + [DriverTypeNames.Modbus] = j => ModbusTagDefinitionFactory.FromTagConfig(j, P, out _), + [DriverTypeNames.S7] = j => S7TagDefinitionFactory.FromTagConfig(j, P, out _), + [DriverTypeNames.AbCip] = j => AbCipTagDefinitionFactory.FromTagConfig(j, P, out _), + [DriverTypeNames.AbLegacy] = j => AbLegacyTagDefinitionFactory.FromTagConfig(j, P, out _), + [DriverTypeNames.TwinCAT] = j => TwinCATTagDefinitionFactory.FromTagConfig(j, P, out _), + [DriverTypeNames.FOCAS] = j => FocasTagDefinitionFactory.FromTagConfig(j, P, out _), + // The union, matching MqttTagDefinitionFactory.Inspect's own accept gate: a blob is served if + // EITHER shape reads it. + [DriverTypeNames.Mqtt] = j => MqttTagDefinitionFactory.FromTagConfig(j, P, out _) + || MqttTagDefinitionFactory.FromSparkplugTagConfig(j, P, out _), + [DriverTypeNames.Sql] = j => SqlEquipmentTagParser.TryParse(j, P, out _), + [DriverTypeNames.MTConnect] = j => MTConnectTagDefinitionFactory.TryFromTagConfig(j, out _, out _), + [DriverTypeNames.Calculation] = j => CalculationTagDefinitionFactory.TryReadTagConfig(j, out _), + [DriverTypeNames.OpcUaClient] = j => OpcUaClientTagDefinitionFactory.TryReadNodeId(j, out _), + }; + + private static bool RuntimeAccepts(string driverType, string blob) => RuntimeParses[driverType](blob); + + private static bool AuthoringAccepts(string driverType, string blob) + => TagConfigValidator.Validate(driverType, blob) is null; + + /// The map above must name every driver the authoring gate dispatches on, or a driver could + /// drop out of the fleet theories without a single test going red. + [Fact] + public void RuntimeParses_covers_every_mapped_driver() + { + string[] mapped = + [ + DriverTypeNames.Modbus, DriverTypeNames.S7, DriverTypeNames.AbCip, DriverTypeNames.AbLegacy, + DriverTypeNames.TwinCAT, DriverTypeNames.FOCAS, DriverTypeNames.Mqtt, DriverTypeNames.Sql, + DriverTypeNames.MTConnect, DriverTypeNames.Calculation, DriverTypeNames.OpcUaClient, + ]; + + foreach (var driverType in mapped) + { + TagConfigValidator.Validate(driverType, "{}").ShouldNotBeNull( + $"{driverType} is expected to have a typed authoring validator that refuses an empty blob " + + "(every mapped driver's reader does)."); + RuntimeParses.ShouldContainKey(driverType); + } + + RuntimeParses.Count.ShouldBe(mapped.Length); + } + + // ============================================================================================= + // 1. Per-driver: the formerly-invisible rejections, each with its nearest valid sibling. + // ============================================================================================= + + /// + /// Modbus is where the issue's own example lives ("address":"notanumber"), and it was the one + /// mapped editor with NO required field at all — {}, which is exactly what the modal persists + /// for a tag whose config the operator never touched, validated clean and then read + /// BadNodeIdUnknown forever. + /// + [Theory] + [InlineData("{}", "an empty blob names neither a region nor an address")] + [InlineData("""{"address":1}""", "no 'region' key — the one Modbus enum that is not defaulted from absent")] + [InlineData("""{"region":"HoldingRegisters"}""", "no 'address'")] + [InlineData("""{"region":"HoldingRegisters","address":"notanumber"}""", "the issue's own example")] + [InlineData("""{"region":"HoldingRegisters","address":-1}""", "below the 0–65535 register range")] + [InlineData("""{"region":"HoldingRegisters","address":70000}""", "above the 0–65535 register range")] + [InlineData("""{"region":"HoldingRegisters","address":1.5}""", "not a whole register address")] + [InlineData("""{"region":"HoldingRegisters","address":1,"dataType":"String","stringLength":0}""", + "a zero-length string reads zero registers — an illegal Modbus request")] + [InlineData("""{"region":"HoldingRegisters","address":1,"dataType":"BitInRegister","bitIndex":16}""", + "a bit index above 15 overflows the 16-bit register mask")] + public void Modbus_refuses_what_its_reader_refuses(string blob, string why) + { + RuntimeAccepts(DriverTypeNames.Modbus, blob).ShouldBeFalse( + $"the reader must refuse this blob ({why}), or the authoring assertion below proves nothing."); + TagConfigValidator.Validate(DriverTypeNames.Modbus, blob).ShouldNotBeNullOrEmpty( + $"authoring accepted a Modbus blob the driver skips: {why}."); + } + + /// The paired acceptance controls — including two the narrowing casts make legal. + [Theory] + [InlineData("""{"region":"HoldingRegisters","address":1}""")] + [InlineData("""{"region":"HoldingRegisters","address":0}""")] + [InlineData("""{"region":"Coils","address":65535}""")] + [InlineData("""{"region":"HoldingRegisters","address":1,"dataType":"String","stringLength":16}""")] + [InlineData("""{"region":"HoldingRegisters","address":1,"dataType":"BitInRegister","bitIndex":15}""")] + // ⚠️ THE NARROWING CAST IS PART OF THE RULE, not an accident of the mapper: it reads `(byte)bitIndex`, + // so 256 narrows to bit 0 and the driver SERVES the tag. Authoring must accept it too — refusing the + // un-narrowed number here would make the modal stricter than the runtime. + [InlineData("""{"region":"HoldingRegisters","address":1,"dataType":"BitInRegister","bitIndex":256}""")] + // Same for stringLength, which narrows to ushort: 65537 becomes 1, which is >= 1 and is served. + [InlineData("""{"region":"HoldingRegisters","address":1,"dataType":"String","stringLength":65537}""")] + // A stringLength of 0 only matters for a String tag; on an Int16 tag the reader ignores it. + [InlineData("""{"region":"HoldingRegisters","address":1,"dataType":"Int16","stringLength":0}""")] + public void Modbus_accepts_what_its_reader_accepts(string blob) + { + RuntimeAccepts(DriverTypeNames.Modbus, blob).ShouldBeTrue( + "the reader must accept this blob, or this control is testing the wrong thing."); + TagConfigValidator.Validate(DriverTypeNames.Modbus, blob).ShouldBeNull( + "authoring refused a Modbus blob the driver serves."); + } + + /// S7's remaining non-enum refusal: a String tag whose stringLength falls outside + /// 0–254. The reader REJECTS it (it does not clamp), so the tag would never resolve. + [Theory] + [InlineData("""{"address":"DB1.DBS0","dataType":"String","stringLength":255}""")] + [InlineData("""{"address":"DB1.DBS0","dataType":"String","stringLength":300}""")] + [InlineData("""{"address":"DB1.DBS0","dataType":"String","stringLength":-1}""")] + public void S7_refuses_a_string_length_its_reader_refuses(string blob) + { + RuntimeAccepts(DriverTypeNames.S7, blob).ShouldBeFalse( + "the reader must refuse this blob, or the authoring assertion below proves nothing."); + TagConfigValidator.Validate(DriverTypeNames.S7, blob).ShouldNotBeNullOrEmpty(); + } + + /// The paired controls: an in-range String tag, the boundary value, and — the shape that + /// stops this becoming an over-rejection — an out-of-range stringLength on a tag that is not a + /// String at all, which the reader ignores entirely. + [Theory] + [InlineData("""{"address":"DB1.DBS0","dataType":"String","stringLength":10}""")] + [InlineData("""{"address":"DB1.DBS0","dataType":"String","stringLength":254}""")] + [InlineData("""{"address":"DB1.DBS0","dataType":"String","stringLength":0}""")] + [InlineData("""{"address":"DB1.DBW0","dataType":"Int16","stringLength":300}""")] + public void S7_accepts_a_string_length_its_reader_accepts(string blob) + { + RuntimeAccepts(DriverTypeNames.S7, blob).ShouldBeTrue( + "the reader must accept this blob, or this control is testing the wrong thing."); + TagConfigValidator.Validate(DriverTypeNames.S7, blob).ShouldBeNull(); + } + + /// + /// TwinCAT's Structure is the sharpest case in the whole survey: it is a perfectly VALID + /// name, so every spelling gate — including the #549 enum check — + /// waves it through, and the reader refuses it anyway because the driver does not serve UDT / + /// FB-instance tags. Nothing but asking the reader could have caught it. + /// + [Fact] + public void TwinCAT_refuses_a_Structure_tag_its_reader_refuses() + { + const string blob = """{"symbolPath":"MAIN.stMotor","dataType":"Structure"}"""; + + Enum.TryParse("Structure", out _).ShouldBeTrue( + "if 'Structure' stopped being a valid enum name the #549 check would catch it and this test " + + "would no longer be about #554 at all."); + RuntimeAccepts(DriverTypeNames.TwinCAT, blob).ShouldBeFalse(); + TagConfigValidator.Validate(DriverTypeNames.TwinCAT, blob).ShouldNotBeNullOrEmpty(); + } + + /// The paired acceptance: the same symbol with an atomic type. + [Fact] + public void TwinCAT_accepts_an_atomic_tag_its_reader_accepts() + { + const string blob = """{"symbolPath":"MAIN.stMotor","dataType":"DInt"}"""; + RuntimeAccepts(DriverTypeNames.TwinCAT, blob).ShouldBeTrue(); + TagConfigValidator.Validate(DriverTypeNames.TwinCAT, blob).ShouldBeNull(); + } + + /// + /// Sql's RECOGNITION rule — SqlEquipmentTagParser refuses a blob that carries neither a + /// model key nor "driver":"Sql", because it cannot tell a Sql tag from any other + /// driver's config. Every field below is otherwise complete and correct, which is what made this + /// invisible: nothing the model checks is wrong with it. + /// + [Fact] + public void Sql_refuses_a_blob_its_parser_does_not_recognise_as_Sql() + { + const string blob = """{"table":"dbo.TagValues","keyColumn":"TagName","valueColumn":"Value","keyValue":"L1"}"""; + RuntimeAccepts(DriverTypeNames.Sql, blob).ShouldBeFalse(); + TagConfigValidator.Validate(DriverTypeNames.Sql, blob).ShouldNotBeNullOrEmpty(); + } + + /// The paired acceptances: either recognition key rescues the identical blob. + [Theory] + [InlineData("""{"model":"KeyValue","table":"dbo.TagValues","keyColumn":"TagName","valueColumn":"Value","keyValue":"L1"}""")] + [InlineData("""{"driver":"Sql","table":"dbo.TagValues","keyColumn":"TagName","valueColumn":"Value","keyValue":"L1"}""")] + public void Sql_accepts_a_recognised_blob(string blob) + { + RuntimeAccepts(DriverTypeNames.Sql, blob).ShouldBeTrue(); + TagConfigValidator.Validate(DriverTypeNames.Sql, blob).ShouldBeNull(); + } + + /// MQTT's qos: the reader takes a JSON NUMBER in 0–2 and refuses anything else outright + /// (it does not clamp). A JSON null is the shape the model's own QoS check read as "absent". + [Theory] + [InlineData("""{"topic":"otopcua/fixture/a","qos":null}""")] + [InlineData("""{"topic":"otopcua/fixture/a","qos":3}""")] + [InlineData("""{"topic":"otopcua/fixture/a","qos":"1"}""")] + public void Mqtt_refuses_a_qos_its_reader_refuses(string blob) + { + RuntimeAccepts(DriverTypeNames.Mqtt, blob).ShouldBeFalse(); + TagConfigValidator.Validate(DriverTypeNames.Mqtt, blob).ShouldNotBeNullOrEmpty(); + } + + /// The paired acceptances: a legal QoS, and an absent one. + [Theory] + [InlineData("""{"topic":"otopcua/fixture/a","qos":1}""")] + [InlineData("""{"topic":"otopcua/fixture/a","qos":0}""")] + [InlineData("""{"topic":"otopcua/fixture/a","qos":2}""")] + [InlineData("""{"topic":"otopcua/fixture/a"}""")] + public void Mqtt_accepts_a_qos_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. + // ============================================================================================= + + /// + /// ⚠️ An inspector's advisory must never become an authoring error. FOCAS's Inspect + /// emits warnings for blobs its reader ACCEPTS — an authored writable:true on a read-only + /// wire is one — so a gate built on "the inspection is non-empty" would refuse configs the driver + /// serves and leave rows nobody can save (the #520/#535 uneditable-rows trap). The gate is the PARSE + /// verdict; the inspector is consulted only to word a refusal that has already happened. + /// + /// Both halves are asserted, so this cannot pass vacuously: the advisory really is emitted, and + /// authoring really does accept anyway. + /// + /// + [Fact] + public void A_FOCAS_blob_that_draws_an_advisory_but_parses_clean_is_accepted_at_authoring() + { + const string blob = """{"address":"R100","dataType":"Int32","writable":true}"""; + + FocasTagDefinitionFactory.Inspect(blob).ShouldNotBeEmpty( + "this blob must actually draw an advisory, or the pin below proves nothing."); + RuntimeAccepts(DriverTypeNames.FOCAS, blob).ShouldBeTrue( + "the FOCAS reader accepts an authored writable:true — it is the WIRE that refuses the write."); + TagConfigValidator.Validate(DriverTypeNames.FOCAS, blob).ShouldBeNull( + "authoring refused a FOCAS blob the driver serves — an advisory was promoted to an error."); + } + + /// The same shape at the other driver whose inspector advises on accepted blobs: a + /// Calculation tag with a timerIntervalMs the reader silently discards. + [Fact] + public void A_Calculation_blob_that_draws_an_advisory_but_parses_clean_is_accepted_at_authoring() + { + const string blob = """{"scriptId":"S1","changeTriggered":true,"timerIntervalMs":"500"}"""; + + CalculationTagDefinitionFactory.Inspect(blob).ShouldNotBeEmpty( + "this blob must actually draw an advisory, or the pin below proves nothing."); + RuntimeAccepts(DriverTypeNames.Calculation, blob).ShouldBeTrue(); + TagConfigValidator.Validate(DriverTypeNames.Calculation, blob).ShouldBeNull(); + } + + // ============================================================================================= + // 3. The fleet theories, over one shared corpus. + // ============================================================================================= + + /// + /// Representative blobs spanning all eleven mapped drivers, both readable and unreadable shapes. + /// One corpus feeds both directions below, so a blob added for one is checked by the other. + /// + public static TheoryData Corpus() + { + var data = new TheoryData(); + foreach (var (driverType, blob) in Blobs) { data.Add(driverType, blob); } + return data; + } + + private static readonly (string DriverType, string Blob)[] Blobs = + [ + (DriverTypeNames.Modbus, "{}"), + (DriverTypeNames.Modbus, """{"region":"HoldingRegisters","address":1}"""), + (DriverTypeNames.Modbus, """{"region":"HoldingRegisters","address":"notanumber"}"""), + (DriverTypeNames.Modbus, """{"region":"HoldingRegisters","address":1,"dataType":"String","stringLength":8}"""), + (DriverTypeNames.Modbus, """{"region":"HoldingRegisters","address":1,"dataType":"String"}"""), + (DriverTypeNames.Modbus, """{"region":"HoldingRegisterz","address":1}"""), + + (DriverTypeNames.S7, "{}"), + (DriverTypeNames.S7, """{"address":"DB1.DBW0","dataType":"Int16"}"""), + (DriverTypeNames.S7, """{"address":"DB1.DBW0"}"""), + (DriverTypeNames.S7, """{"address":" ","dataType":"Int16"}"""), + (DriverTypeNames.S7, """{"address":"DB1.DBS0","dataType":"String","stringLength":300}"""), + (DriverTypeNames.S7, """{"address":"DB1.DBS0","dataType":"String","stringLength":24}"""), + + (DriverTypeNames.AbCip, "{}"), + (DriverTypeNames.AbCip, """{"tagPath":"Program:Main.Speed"}"""), + (DriverTypeNames.AbCip, """{"tagPath":"Program:Main.Speed","dataType":"Structure","members":[{"name":"Run","dataType":"Bool"}]}"""), + (DriverTypeNames.AbCip, """{"tagPath":"Motor","members":[{"dataType":"Bool"}]}"""), + (DriverTypeNames.AbCip, """{"tagPath":"Motor","members":[]}"""), + (DriverTypeNames.AbCip, """{"tagPath":1}"""), + + (DriverTypeNames.AbLegacy, "{}"), + (DriverTypeNames.AbLegacy, """{"address":"N7:0","dataType":"Int"}"""), + (DriverTypeNames.AbLegacy, """{"address":"N7:0","isArray":true,"arrayLength":10}"""), + + (DriverTypeNames.TwinCAT, "{}"), + (DriverTypeNames.TwinCAT, """{"symbolPath":"MAIN.bStart","dataType":"Bool"}"""), + (DriverTypeNames.TwinCAT, """{"symbolPath":"MAIN.stM","dataType":"Structure"}"""), + (DriverTypeNames.TwinCAT, """{"symbolPath":"MAIN.x","isArray":true,"arrayLength":0}"""), + + (DriverTypeNames.FOCAS, "{}"), + (DriverTypeNames.FOCAS, """{"address":"R100","dataType":"Int32"}"""), + (DriverTypeNames.FOCAS, """{"address":"PARAM:1815/0"}"""), + (DriverTypeNames.FOCAS, """{"address":"R100","writable":true}"""), + + (DriverTypeNames.Mqtt, "{}"), + (DriverTypeNames.Mqtt, """{"topic":"otopcua/fixture/a","dataType":"Float64"}"""), + (DriverTypeNames.Mqtt, """{"topic":"otopcua/fixture/a","qos":3}"""), + (DriverTypeNames.Mqtt, """{"topic":"otopcua/fixture/a","qos":2}"""), + (DriverTypeNames.Mqtt, """{"groupId":"OtOpcUaSim","edgeNodeId":"EdgeA","metricName":"Filler/Speed"}"""), + (DriverTypeNames.Mqtt, """{"groupId":"OtOpcUaSim","edgeNodeId":"EdgeA"}"""), + + (DriverTypeNames.Sql, "{}"), + (DriverTypeNames.Sql, """{"model":"KeyValue","table":"dbo.T","keyColumn":"k","valueColumn":"v","keyValue":"x"}"""), + (DriverTypeNames.Sql, """{"table":"dbo.T","keyColumn":"k","valueColumn":"v","keyValue":"x"}"""), + (DriverTypeNames.Sql, """{"model":"Query","table":"dbo.T"}"""), + (DriverTypeNames.Sql, """{"model":"WideRow","table":"dbo.T","columnName":"Speed","rowSelector":{"whereColumn":"LineId","whereValue":"L1"}}"""), + (DriverTypeNames.Sql, """{"model":"WideRow","table":"dbo.T","columnName":"Speed"}"""), + + (DriverTypeNames.MTConnect, "{}"), + (DriverTypeNames.MTConnect, """{"fullName":"Xact"}"""), + (DriverTypeNames.MTConnect, """{"fullName":"Xact","dataType":"Float64"}"""), + (DriverTypeNames.MTConnect, """{"dataItemId":"Xact"}"""), + (DriverTypeNames.MTConnect, """{"fullName":"Xact","driverDataType":"Strng"}"""), + + (DriverTypeNames.Calculation, "{}"), + (DriverTypeNames.Calculation, """{"scriptId":"S1"}"""), + (DriverTypeNames.Calculation, """{"scriptId":"S1","changeTriggered":true,"timerIntervalMs":5000}"""), + (DriverTypeNames.Calculation, """{"scriptId":" "}"""), + + (DriverTypeNames.OpcUaClient, "{}"), + (DriverTypeNames.OpcUaClient, """{"nodeId":"nsu=urn:plc:sim;s=Line1.Speed"}"""), + (DriverTypeNames.OpcUaClient, """{"nodeId":""}"""), + (DriverTypeNames.OpcUaClient, """{"nodeId":5}"""), + ]; + + /// + /// ⚠️ The anti-vacuity guard for both fleet theories. Each of them early-returns on the half + /// of the corpus it does not apply to, so a corpus that happened to be all-rejected (or all-accepted) + /// for some driver would let that driver pass with nothing asserted about it at all. Every mapped + /// driver must therefore contribute at least one blob its reader SERVES and one it REFUSES. + /// + [Fact] + public void The_corpus_exercises_both_verdicts_for_every_mapped_driver() + { + foreach (var driverType in RuntimeParses.Keys) + { + var mine = Blobs.Where(b => b.DriverType == driverType).ToList(); + mine.ShouldNotBeEmpty($"{driverType} contributes no blob to the corpus."); + mine.ShouldContain( + b => RuntimeAccepts(b.DriverType, b.Blob), + $"{driverType}: every corpus blob is REFUSED by its reader, so " + + "Whatever_the_runtime_parse_accepts_is_authorable passes vacuously for it."); + mine.ShouldContain( + b => !RuntimeAccepts(b.DriverType, b.Blob), + $"{driverType}: every corpus blob is SERVED by its reader, so " + + "Whatever_authoring_accepts_the_runtime_parse_accepts never exercises a refusal for it."); + } + } + + /// + /// The absolute direction. Anything the modal lets an operator save must be something the + /// driver's reader will actually read — otherwise the tag deploys green and reads + /// BadNodeIdUnknown forever, which is the whole of Gitea #554. There are no exemptions from + /// this one, and there must never be. + /// + [Theory] + [MemberData(nameof(Corpus))] + public void Whatever_authoring_accepts_the_runtime_parse_accepts(string driverType, string blob) + { + if (!AuthoringAccepts(driverType, blob)) { return; } + + RuntimeAccepts(driverType, blob).ShouldBeTrue( + $"{driverType}: authoring accepted a blob its driver's reader refuses — the tag would deploy " + + $"clean and read BadNodeIdUnknown forever. Blob: {blob}"); + } + + /// + /// The conditional direction. A blob the driver SERVES should normally be authorable, or the + /// modal blocks configuration the runtime would have honoured. It is conditional because four + /// editors are deliberately stricter than their reader for reasons that are load-bearing — see + /// , where each is exempted by name with its reason. + /// + [Theory] + [MemberData(nameof(Corpus))] + public void Whatever_the_runtime_parse_accepts_is_authorable_unless_the_editor_is_deliberately_stricter( + string driverType, string blob) + { + if (!RuntimeAccepts(driverType, blob)) { return; } + if (DeliberatelyStricter.ContainsKey((driverType, blob))) { return; } + + AuthoringAccepts(driverType, blob).ShouldBeTrue( + $"{driverType}: authoring refused a blob the driver serves. If that is intended, add it to " + + $"DeliberatelyStricter with its reason rather than weakening this theory. Blob: {blob}"); + } + + /// + /// Every place an editor refuses a config its driver would serve, with the reason. These are NOT + /// #554 gaps — #554 closes the opposite direction — and none was removed while closing it. Each is + /// documented on the model itself; this table is the fleet-level record of them. + /// + private static readonly IReadOnlyDictionary<(string DriverType, string Blob), string> DeliberatelyStricter = + new Dictionary<(string, string), string> + { + // The reader CLAMPS a length above 256 (Math.Min) and serves the tag, but the materialiser sets + // OPC UA ArrayDimensions from the RAW authored number — so the served array is narrower than the + // node advertises, and the authored value is silently not honoured. + [(DriverTypeNames.AbLegacy, """{"address":"N7:0","isArray":true,"arrayLength":300}""")] = + "runtime clamps to 256; the un-clamped length still sets ArrayDimensions (Part 6 §5.3.2 mismatch)", + + // Gitea #540: the reader accepts any non-blank address and the DRIVER then skips the tag at + // startup. 20 tags carrying a browse full reference deployed green this way. + [(DriverTypeNames.FOCAS, """{"address":"10.201.31.5:8193/Identity/SeriesNumber"}""")] = + "an address the FOCAS parser cannot read is a tag the driver skips at startup (Gitea #540)", + + // One Tag holds one value; '+'/'#' would feed it from many source topics. + [(DriverTypeNames.Mqtt, """{"topic":"otopcua/fixture/+"}""")] = + "a wildcard topic has no legitimate single-Tag interpretation", + + // A decoded incoming id can never contain '/', '+' or '#', so such a binding is permanently dead. + [(DriverTypeNames.Mqtt, """{"groupId":"Ot/Sim","edgeNodeId":"EdgeA","metricName":"M"}""")] = + "a Sparkplug id segment carrying '/', '+' or '#' could never bind", + + // The runtime serves a trigger-less calc tag; it simply never computes. + [(DriverTypeNames.Calculation, """{"scriptId":"S1","changeTriggered":false}""")] = + "a calc tag with no trigger can never fire — the editor refuses it, the runtime tolerates it", + + // The runtime honours a 1 ms cadence. + [(DriverTypeNames.Calculation, """{"scriptId":"S1","timerIntervalMs":10}""")] = + "sub-50 ms timer: deliberate editor floor, honoured verbatim by the runtime", + + // The reader still falls back to a legacy PascalCase NodeId when the v3 camelCase key is absent. + // Pre-dates #554 and is left alone: re-entering the reference rewrites it to the v3 key. + [(DriverTypeNames.OpcUaClient, """{"NodeId":"nsu=urn:plc:sim;s=Line1.Speed"}""")] = + "legacy PascalCase key: the reader still honours it, this editor reads only camelCase 'nodeId'", + }; + + /// + /// The exemption table must not rot into a list of shapes nobody checks. Every entry is asserted to + /// still BE an exemption: the runtime accepts it and authoring refuses it. An entry that stops being + /// either — because the editor was aligned, or the runtime tightened — fails here rather than + /// silently excusing a blob from the theory above. + /// + [Fact] + public void Every_deliberately_stricter_entry_is_still_exactly_that() + { + foreach (var ((driverType, blob), reason) in DeliberatelyStricter) + { + RuntimeAccepts(driverType, blob).ShouldBeTrue( + $"{driverType}: the runtime no longer serves this blob, so it is not an over-strictness " + + $"exemption any more — remove the entry. ({reason}) Blob: {blob}"); + AuthoringAccepts(driverType, blob).ShouldBeFalse( + $"{driverType}: authoring now accepts this blob, so the exemption is stale — remove the " + + $"entry so the parity theory covers it. ({reason}) Blob: {blob}"); + } + } + + // ============================================================================================= + // 4. The editor-flow constraint: a freshly authored config must sail through. + // ============================================================================================= + + /// + /// Every mapped driver's editor, given minimal valid operator input, must produce a blob its own + /// reader accepts and its own save gate accepts. This is the guard against the failure mode the + /// #554 design cautions about most: a parse consult that refuses configs the editor itself writes, + /// which would leave the modal unable to save anything at all. + /// + /// The configs come from the models' own ToJson(), never hand-written — a hand-written + /// blob is a guess at what the editor writes, and several editors normalise keys on save. + /// + /// + public static TheoryData FreshlyAuthored() + { + var data = new TheoryData(); + data.Add(DriverTypeNames.Modbus, new ModbusTagConfigModel { Address = 1 }.ToJson()); + data.Add(DriverTypeNames.S7, new S7TagConfigModel { Address = "DB1.DBW0" }.ToJson()); + data.Add(DriverTypeNames.S7, + new S7TagConfigModel { Address = "DB1.DBS0", DataType = S7DataType.String, StringLength = 24 }.ToJson()); + data.Add(DriverTypeNames.AbCip, new AbCipTagConfigModel { TagPath = "Program:Main.Speed" }.ToJson()); + data.Add(DriverTypeNames.AbLegacy, new AbLegacyTagConfigModel { Address = "N7:0" }.ToJson()); + data.Add(DriverTypeNames.TwinCAT, new TwinCATTagConfigModel { SymbolPath = "MAIN.bStart" }.ToJson()); + data.Add(DriverTypeNames.FOCAS, new FocasTagConfigModel { Address = "R100" }.ToJson()); + data.Add(DriverTypeNames.OpcUaClient, + new OpcUaClientTagConfigModel { NodeId = "nsu=urn:plc:sim;s=Line1.Speed" }.ToJson()); + data.Add(DriverTypeNames.Calculation, + new CalculationTagConfigModel { ScriptId = "S1", ChangeTriggered = true }.ToJson()); + data.Add(DriverTypeNames.MTConnect, + new MTConnectTagConfigModel { FullName = "Xact", DataType = DriverDataType.Float64 }.ToJson()); + data.Add(DriverTypeNames.Mqtt, + new MqttTagConfigModel { Mode = MqttMode.Plain, Topic = "otopcua/fixture/a" }.ToJson()); + data.Add(DriverTypeNames.Mqtt, new MqttTagConfigModel + { + Mode = MqttMode.SparkplugB, + GroupId = "OtOpcUaSim", + EdgeNodeId = "EdgeA", + MetricName = "Filler/Speed", + }.ToJson()); + data.Add(DriverTypeNames.Sql, new SqlTagConfigModel + { + Model = SqlTagModel.KeyValue, + Table = "dbo.TagValues", + KeyColumn = "TagName", + KeyValue = "Line1.Speed", + ValueColumn = "Value", + }.ToJson()); + data.Add(DriverTypeNames.Sql, new SqlTagConfigModel + { + Model = SqlTagModel.WideRow, + Table = "dbo.LineSnapshot", + ColumnName = "Speed", + RowSelector = new SqlRowSelectorModel { WhereColumn = "LineId", WhereValue = "L1" }, + }.ToJson()); + return data; + } + + [Theory] + [MemberData(nameof(FreshlyAuthored))] + public void A_freshly_authored_config_is_read_by_its_driver_and_accepted_by_its_save_gate( + string driverType, string blob) + { + RuntimeAccepts(driverType, blob).ShouldBeTrue( + $"{driverType}: the editor writes a blob its own driver's reader refuses. Blob: {blob}"); + TagConfigValidator.Validate(driverType, blob).ShouldBeNull( + $"{driverType}: the save gate refuses a config its own editor just produced — the modal could " + + $"not save anything. Blob: {blob}"); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigValidatorTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigValidatorTests.cs index e0f39b49..beb3a0d9 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigValidatorTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigValidatorTests.cs @@ -14,9 +14,27 @@ public sealed class TagConfigValidatorTests public void Unmapped_driver_has_no_typed_validator_so_is_valid() => TagConfigValidator.Validate("GalaxyMxGateway", "{}").ShouldBeNull(); + /// + /// Gitea #554. This used to assert that {} was VALID for Modbus — the one mapped driver with + /// no required-field check at all — which made the modal's save gate weaker than the driver's reader: + /// ModbusTagDefinitionFactory.FromTagConfig refuses a blob carrying no region and no + /// address, so a tag saved that way was skipped by the driver and read + /// BadNodeIdUnknown forever with the deployment green. It is reachable from the modal (the + /// typed editor only pushes its ToJson() once a field CHANGES, so a config nobody touched is + /// persisted as {}) as well as from CSV import and the API. + /// [Fact] - public void Modbus_has_no_required_field_so_empty_config_is_valid() - => TagConfigValidator.Validate("Modbus", "{}").ShouldBeNull(); + public void Modbus_empty_config_is_rejected_because_the_driver_would_skip_the_tag() + { + TagConfigValidator.Validate("Modbus", "{}").ShouldNotBeNullOrEmpty(); + TagConfigValidator.Validate("Modbus", null).ShouldNotBeNullOrEmpty(); + } + + /// The paired acceptance control: the moment the operator names a region and an address — the + /// two keys the reader needs — the same gate accepts. + [Fact] + public void Modbus_with_a_region_and_an_address_is_valid() + => TagConfigValidator.Validate("Modbus", """{"region":"HoldingRegisters","address":1}""").ShouldBeNull(); [Fact] public void Resolve_Modbus_dispatches_typed_editor()