diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/OpcUaClientTagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/OpcUaClientTagDefinitionFactory.cs index 7a63848a..e5670a5b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/OpcUaClientTagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Contracts/OpcUaClientTagDefinitionFactory.cs @@ -32,6 +32,15 @@ public static class OpcUaClientTagDefinitionFactory /// Read the upstream node id string from an authored TagConfig blob's nodeId key /// (camelCase, per the AdminUI OpcUaClientTagConfigModel; a legacy PascalCase NodeId /// is also accepted). Never throws — a malformed blob yields . + /// + /// ⚠️ The two spellings are PRESENCE-shadows, not first-non-blank. The || below + /// short-circuits on the mere presence of nodeId — whatever it holds — and the string / + /// non-blank check then runs ONCE, on that one element. So the legacy NodeId is consulted + /// only when nodeId is wholly absent: a blank or non-string nodeId beside a + /// valid legacy NodeId is a rejected blob, not a fallback. The AdminUI model mirrors this + /// walk exactly (Gitea #557 item 1) rather than the looser first-non-blank shape MTConnect's + /// reader uses, which would let authoring accept a blob this reader refuses. + /// /// /// The authored equipment-tag TagConfig JSON. /// The upstream node id string when this returns . 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 ccf7d646..6854d16b 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 @@ -10,17 +10,50 @@ namespace ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors; /// /// Under the v3 identity contract, TagConfig no longer carries identity — the upstream /// node reference is an ordinary driver-specific address field. The key is the camelCase -/// nodeId (replacing the legacy PascalCase FullName identity convention). The +/// nodeId (replacing the legacy PascalCase FullName identity convention); a legacy +/// PascalCase NodeId is still READ, at the driver reader's own precedence, and normalised away +/// on save — see . The /// driver-agnostic server-side HistoryRead intent keys (isHistorized / /// historianTagname) are NOT modelled here — they are owned by the TagModal-merge seam /// () and survive a load→save of this model as preserved unknown keys. /// public sealed class OpcUaClientTagConfigModel { - /// The camelCase TagConfig JSON key holding the upstream node reference. + /// The camelCase TagConfig JSON key holding the upstream node reference — the spelling this + /// model writes, and the LAST of , i.e. the one the reader consults only when + /// no earlier spelling is present. internal const string NodeIdKey = "nodeId"; - /// Upstream OPC UA node reference the tag binds to (the driver-side full reference). Required. + /// + /// The accepted spellings of the upstream node reference, in the reader's precedence order, + /// mirroring 's + /// TryGetProperty("nodeId", …) || TryGetProperty("NodeId", …): the v3 camelCase + /// nodeId first, then the legacy PascalCase NodeId a pre-v3 blob carries. + /// + /// ⚠️ This precedence is PRESENCE-shadows, not first-non-blank — see + /// . The reader's || short-circuits on the mere PRESENCE of + /// nodeId and only then asks whether the value is a non-blank string, so a present-but-blank + /// or present-but-non-string nodeId beside a perfectly good legacy NodeId is a + /// REJECTED blob, not a fallback. Copying MTConnectTagConfigModel's FirstNonBlank + /// walk here would make this editor accept a blob the driver skips — the absolute direction + /// Gitea #554 forbids. + /// + /// + /// Reading only nodeId was Gitea #557 item 1: a legacy blob was SERVED by the driver + /// yet opened with an empty reference field and could not be saved untouched, so the editor was + /// stricter than the reader — authoring blocking config the runtime honours. + /// therefore normalises onto and DROPS the legacy + /// spelling, exactly as MTConnectTagConfigModel does for its own aliases. Both halves are + /// load-bearing together: the precedence read alone would leave the legacy key in the bag to be + /// re-read on the next load, and the drop alone would blank every legacy tag's reference on its + /// next save. + /// + /// + private static readonly string[] NodeIdKeys = [NodeIdKey, "NodeId"]; + + /// Upstream OPC UA node reference the tag binds to (the driver-side full reference). Required. + /// Loaded from nodeId, or from the legacy PascalCase NodeId when nodeId is wholly + /// absent (see ); always saved as nodeId, with the legacy key dropped. public string NodeId { get; set; } = ""; private JsonObject _bag = new(); @@ -38,31 +71,43 @@ public sealed class OpcUaClientTagConfigModel var o = TagConfigJson.ParseOrNew(json); return new OpcUaClientTagConfigModel { - NodeId = TagConfigJson.GetString(o, NodeIdKey) ?? "", + NodeId = ReadNodeId(o), _source = json ?? "", _bag = o, }; } /// Serialises this model back to a TagConfig JSON string over the preserved key bag. - /// nodeId is written camelCase (the v3 address key); any history keys merged by the TagModal - /// (isHistorized / historianTagname) are carried through untouched as preserved unknown - /// keys. + /// nodeId is written camelCase (the v3 address key) and every legacy spelling of it is DROPPED + /// (see ); any history keys merged by the TagModal (isHistorized / + /// historianTagname) are carried through untouched as preserved unknown keys — they are + /// disjoint from the address key, so the normalisation below cannot disturb that merge. /// The serialised TagConfig JSON string. public string ToJson() { + // Normalise onto the canonical key: a blob loaded via the legacy spelling must not keep it, or the + // two spellings drift on the next edit and a later reader change could pick the stale one. Driven + // off NodeIdKeys rather than naming "NodeId" a second time, so a spelling added to the READ can + // never be left behind by the DROP. + foreach (var legacy in NodeIdKeys) + { + if (legacy != NodeIdKey) { TagConfigJson.Set(_bag, legacy, null); } + } + TagConfigJson.Set(_bag, NodeIdKey, NodeId.Trim()); 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. + /// The Gitea #554 parse consult is a backstop here, and the required-reference check above is now + /// exactly as precedence-aware as the reader (Gitea #557 item 1): yields + /// the very token would consult — blank + /// for every shape it refuses, non-blank for every shape it serves — so the fast path agrees with + /// the parse verdict on the whole rejection surface rather than under-reading one spelling of it. + /// Neither half is redundant: the fast path is the only one that fires for a model built by object + /// initialiser (no source to parse), and the parse consult is what stops a future reader rule from + /// silently becoming an authoring gap. /// /// An error message describing the validation failure, or null when valid. public string? Validate() @@ -73,4 +118,26 @@ public sealed class OpcUaClientTagConfigModel j => OpcUaClientTagDefinitionFactory.TryReadNodeId(j, out _), OpcUaClientTagDefinitionFactory.Inspect, "OpcUaClient"); + + // The as-loaded node reference, read in the READER's precedence (see NodeIdKeys). Per spelling, + // mirroring `TryGetProperty("nodeId", out n) || TryGetProperty("NodeId", out n)` followed by ONE + // `n.ValueKind == String && !IsNullOrWhiteSpace(s)` check: + // · wholly absent → the || moves on: try the next spelling. + // · present, any JSON null + // or non-string token → STOP. The || has already short-circuited on this spelling, and the single + // kind check then refuses the whole blob — it can NEVER fall through to the + // legacy key. Yield "" so Validate() refuses exactly what the driver does. + // · present string → STOP, this is the effective token, blank or not. A blank one likewise + // shadows the legacy key and is a refusal, not a fallback. + // Nothing present ⇒ "", the no-reference-authored shape the reader also refuses. + private static string ReadNodeId(JsonObject o) + { + foreach (var key in NodeIdKeys) + { + if (!o.TryGetPropertyValue(key, out var node)) { continue; } + return node is JsonValue v && v.TryGetValue(out var s) ? s : ""; + } + + return ""; + } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/OpcUaClientTagConfigModelTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/OpcUaClientTagConfigModelTests.cs index 376344cf..1eb9a13a 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/OpcUaClientTagConfigModelTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/OpcUaClientTagConfigModelTests.cs @@ -1,6 +1,7 @@ using Shouldly; using Xunit; using ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors; +using ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient; namespace ZB.MOM.WW.OtOpcUa.AdminUI.Tests.Uns; @@ -102,4 +103,130 @@ public sealed class OpcUaClientTagConfigModelTests { new OpcUaClientTagConfigModel { NodeId = "ns=2;s=X" }.Validate().ShouldBeNull(); } + + // ============================================================================================= + // Gitea #557 item 1 — the legacy PascalCase "NodeId" spelling. + // + // The reader (OpcUaClientTagDefinitionFactory.TryReadNodeId) still honours it when the v3 camelCase + // "nodeId" is WHOLLY ABSENT, while this model read only "nodeId" — so a legacy blob was SERVED by the + // driver yet opened with an empty field and could not be saved untouched. Authoring stricter than the + // reader, which #554 names as never legitimate. The fix is the #550 pattern applied to this reader's + // own precedence: read both spellings at the reader's exact walk, write only "nodeId", drop the legacy. + // + // ⚠️ Retiring the spelling from the READER instead was rejected: it would break every legacy blob the + // driver currently serves at the next deploy — a data-plane regression to fix an editor asymmetry. + // ============================================================================================= + + /// + /// ⚠️ The precedence is PRESENCE-shadows, not first-non-blank, and every shape below is a + /// shape those two rules disagree about. The reader's + /// TryGetProperty("nodeId", out n) || TryGetProperty("NodeId", out n) short-circuits on the + /// mere PRESENCE of the camelCase key and then checks string-and-non-blank ONCE — so a blank or + /// non-string nodeId beside a valid legacy NodeId is a REJECTED blob, not a fallback. + /// Had this model copied MTConnectTagConfigModel's FirstNonBlank walk it would accept + /// those three shapes and the driver would skip the tag — the exact defect #554 forbids, introduced + /// while fixing its mirror image. + /// + /// The verdict is asserted against the READER for every shape, so the expectations below cannot + /// drift from the authority they claim to mirror, and it is asserted on the ToJson() + /// output as well as the original — a normalisation that changed the verdict would be a + /// save that silently made a tag readable or unreadable. + /// + /// + [Theory] + // 1. camelCase only — the v3 shape. + [InlineData("""{"nodeId":"ns=2;s=A"}""", true, "ns=2;s=A", "camelCase only")] + // 2. legacy only — the #557 shape: the reader serves it, so authoring must accept it. + [InlineData("""{"NodeId":"ns=2;s=A"}""", true, "ns=2;s=A", "legacy PascalCase only, camelCase wholly absent")] + // 3. both present — camelCase wins; the legacy value must never be displayed or saved. + [InlineData("""{"nodeId":"ns=2;s=New","NodeId":"ns=2;s=Old"}""", true, "ns=2;s=New", "camelCase out-ranks legacy")] + // 4. blank camelCase beside a valid legacy — SHADOWS. first-non-blank would wrongly accept this. + [InlineData("""{"nodeId":"","NodeId":"ns=2;s=Old"}""", false, "", "a present-but-blank camelCase key shadows the legacy one")] + // 5. non-string camelCase beside a valid legacy — SHADOWS, same reason. + [InlineData("""{"nodeId":5,"NodeId":"ns=2;s=Old"}""", false, "", "a present-but-non-string camelCase key shadows the legacy one")] + // 5b. JSON null is 'present' to TryGetProperty, so it shadows too — ValueKind.Null is not String. + [InlineData("""{"nodeId":null,"NodeId":"ns=2;s=Old"}""", false, "", "a present JSON-null camelCase key shadows the legacy one")] + // 6. blank legacy, no camelCase — nothing usable anywhere. + [InlineData("""{"NodeId":""}""", false, "", "a blank legacy key is no reference at all")] + [InlineData("""{"NodeId":5}""", false, "", "a non-string legacy key is no reference at all")] + // 7. neither spelling. + [InlineData("{}", false, "", "no reference under either spelling")] + public void Authoring_judges_every_spelling_shape_exactly_as_the_reader_does( + string blob, bool readerAccepts, string expectedNodeId, string why) + { + // The authority, first: if the reader's own verdict ever changes, this row fails here rather than + // quietly re-basing the parity claim onto whatever the model happens to do. + OpcUaClientTagDefinitionFactory.TryReadNodeId(blob, out var readerNodeId).ShouldBe( + readerAccepts, $"the reader's verdict for this blob is the premise of the row ({why}). Blob: {blob}"); + + var model = OpcUaClientTagConfigModel.FromJson(blob); + + model.NodeId.ShouldBe( + expectedNodeId, + $"the editor must show the reference the DRIVER would read, not a different spelling's ({why})."); + if (readerAccepts) { model.NodeId.ShouldBe(readerNodeId); } + + (model.Validate() is null).ShouldBe( + readerAccepts, + $"authoring and the reader disagree about this blob ({why}). Blob: {blob}"); + + // And the same verdict must survive the save: normalisation may rewrite the spelling, never the + // answer. (A refused blob is still serialisable — the operator has to be able to open and fix it.) + OpcUaClientTagDefinitionFactory.TryReadNodeId(model.ToJson(), out _).ShouldBe( + readerAccepts, $"ToJson() changed the reader's verdict ({why}). Blob: {blob}"); + } + + /// + /// The no-retype guarantee, which is the operator-visible half of #557: a legacy blob loads showing + /// its real reference, saves as the canonical camelCase key, and the DRIVER reads the same node id + /// before and after. Before the fix the field opened empty and a save blanked the binding. + /// + [Fact] + public void A_legacy_blob_saves_untouched_to_the_canonical_key_without_retyping_the_reference() + { + const string legacy = """{"NodeId":"nsu=urn:plc:sim;s=Line1.Speed"}"""; + OpcUaClientTagDefinitionFactory.TryReadNodeId(legacy, out var before).ShouldBeTrue(); + + var saved = OpcUaClientTagConfigModel.FromJson(legacy).ToJson(); + + saved.ShouldContain(""" + "nodeId":"nsu=urn:plc:sim;s=Line1.Speed" + """.Trim()); + OpcUaClientTagDefinitionFactory.TryReadNodeId(saved, out var after).ShouldBeTrue( + "a save must never make a served tag unreadable."); + after.ShouldBe(before, "the save re-bound the tag to a different upstream node."); + } + + /// + /// The drop half of the #550 pattern. Leaving the legacy key in the bag would let the two spellings + /// drift apart on the next edit, and would leave a key that only exists to be misread. Every save + /// path — a loaded legacy blob, a loaded both-spellings blob, a freshly authored model — emits the + /// canonical key alone. + /// + [Theory] + [InlineData("""{"NodeId":"ns=2;s=A"}""")] + [InlineData("""{"nodeId":"ns=2;s=New","NodeId":"ns=2;s=Old"}""")] + [InlineData("""{"NodeId":"ns=2;s=A","samplingIntervalMs":250}""")] + [InlineData("""{"NodeId":""}""")] + public void ToJson_never_re_emits_the_legacy_PascalCase_key(string blob) + { + var saved = OpcUaClientTagConfigModel.FromJson(blob).ToJson(); + + saved.ShouldNotContain("\"NodeId\"", Case.Sensitive, + "the legacy spelling must be normalised away on save, not carried alongside the canonical key."); + saved.ShouldContain("\"nodeId\":", Case.Sensitive); + } + + /// An unrelated key still survives a legacy-spelled load→save — the drop is surgical. + [Fact] + public void Normalising_the_legacy_key_preserves_every_other_key() + { + var saved = OpcUaClientTagConfigModel + .FromJson("""{"NodeId":"ns=2;s=A","samplingIntervalMs":250,"isHistorized":true}""") + .ToJson(); + + saved.ShouldContain("\"samplingIntervalMs\":250"); + saved.ShouldContain("\"isHistorized\":true"); + saved.ShouldContain("\"nodeId\":\"ns=2;s=A\""); + } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigAuthoringDeployParityTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigAuthoringDeployParityTests.cs index 095234bb..6bb8c2c0 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigAuthoringDeployParityTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigAuthoringDeployParityTests.cs @@ -164,6 +164,23 @@ public sealed class TagConfigAuthoringDeployParityTests AssertParity(DriverTypeNames.OpcUaClient, model.Validate(), model.ToJson()); } + /// + /// Gitea #557 item 1 — a LEGACY PascalCase NodeId blob, which the reader has always served. + /// Both directions are asserted at once: the editor now accepts it as loaded (it used to refuse a + /// config the driver honours), and the blob it saves is still clean at the deploy gate. The two + /// configs differ — the second is the normalised nodeId rewrite — so passing this needs the + /// read half AND the drop half to agree. + /// + [Fact] + public void OpcUaClient_legacy_spelled_config_is_authorable_and_stays_clean_after_normalisation() + { + const string legacy = """{"NodeId":"nsu=urn:plc:sim;s=Line1.Speed"}"""; + var model = OpcUaClientTagConfigModel.FromJson(legacy); + + AssertParity(DriverTypeNames.OpcUaClient, model.Validate(), legacy); + AssertParity(DriverTypeNames.OpcUaClient, model.Validate(), model.ToJson()); + } + /// /// Falsifiability control. Every assertion above would also pass against a build where the four G-7 /// inspectors were never wired — an unmapped driver type returns no warnings for anything. This one 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 index fcadcc90..3828cbca 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs @@ -375,6 +375,17 @@ public sealed class TagConfigNonEnumParityTests (DriverTypeNames.OpcUaClient, """{"nodeId":"nsu=urn:plc:sim;s=Line1.Speed"}"""), (DriverTypeNames.OpcUaClient, """{"nodeId":""}"""), (DriverTypeNames.OpcUaClient, """{"nodeId":5}"""), + // Gitea #557 item 1 — the legacy PascalCase spelling, in every shape the reader's PRESENCE-shadows + // precedence distinguishes. These used to live one-line in DeliberatelyStricter; the fleet theories + // now cover them, in both directions, which is what removing that exemption bought. + (DriverTypeNames.OpcUaClient, """{"NodeId":"nsu=urn:plc:sim;s=Line1.Speed"}"""), + (DriverTypeNames.OpcUaClient, """{"nodeId":"nsu=urn:plc:sim;s=New","NodeId":"nsu=urn:plc:sim;s=Old"}"""), + // ⚠️ The three that a first-non-blank walk would get WRONG: a present-but-unusable camelCase key + // SHADOWS the legacy one, so these are refusals, not fallbacks to the perfectly good "NodeId". + (DriverTypeNames.OpcUaClient, """{"nodeId":"","NodeId":"nsu=urn:plc:sim;s=Old"}"""), + (DriverTypeNames.OpcUaClient, """{"nodeId":5,"NodeId":"nsu=urn:plc:sim;s=Old"}"""), + (DriverTypeNames.OpcUaClient, """{"nodeId":null,"NodeId":"nsu=urn:plc:sim;s=Old"}"""), + (DriverTypeNames.OpcUaClient, """{"NodeId":""}"""), ]; /// @@ -472,10 +483,11 @@ public sealed class TagConfigNonEnumParityTests [(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'", + // Gitea #557 item 1 REMOVED the OpcUaClient legacy-PascalCase entry that sat here. It was the one + // exemption that was not a deliberate editor opinion at all — it was the editor being STRICTER + // than its reader for no reason but an under-read, so a blob the driver SERVED could not be saved + // untouched. OpcUaClientTagConfigModel now reads both spellings at the reader's exact precedence + // and normalises onto 'nodeId' on save; the legacy shapes are in the corpus above instead. }; /// 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 beb3a0d9..d05460df 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 @@ -60,6 +60,12 @@ public sealed class TagConfigValidatorTests public void OpcUaClient_with_node_id_is_valid() => TagConfigValidator.Validate("OpcUaClient", """{"nodeId":"ns=2;s=Line3.Temp"}""").ShouldBeNull(); + // Gitea #557 item 1: the driver's reader still honours the pre-v3 PascalCase spelling when the camelCase + // key is wholly absent, so the dispatched validator must too — it used to refuse a blob the driver serves. + [Fact] + public void OpcUaClient_with_a_legacy_PascalCase_node_id_is_valid() + => TagConfigValidator.Validate("OpcUaClient", """{"NodeId":"ns=2;s=Line3.Temp"}""").ShouldBeNull(); + // Gitea #551 item 2: S7 is the one driver whose reader REQUIRES the dataType key to be present, so a // blob carrying only an address is one the driver skips — this used to assert it was valid. [Fact]