From 73230a42c5b0777ca34f5ecc41964142748d98ac Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 08:02:51 -0400 Subject: [PATCH] perf(calc-contracts): single-parse Inspect (Driver.Calculation.Contracts-001) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Inspect parsed the TagConfig blob once itself (root kind + the present-but-discarded changeTriggered/timerIntervalMs checks) and then called TryReadTagConfig(reference, out _), which parsed the same string from scratch again for the scriptId rule. The field-by-field read is now TryReadRoot(JsonElement), which both entry points share, so Inspect consults the one authority over the root it already has. The public string-based TryReadTagConfig signature other modules consume is unchanged. Pure refactor: added CalculationTagDefinitionFactoryInspectTests (17 cases pinning the exact wording, the rejection-vs-advisory split and their order, and the never-stricter-than-the-runtime-read invariant). They pass unchanged against the pre-refactor code — verified by stashing the production change. --- .../CalculationTagDefinitionFactory.cs | 60 ++++++---- ...ulationTagDefinitionFactoryInspectTests.cs | 111 ++++++++++++++++++ 2 files changed, 150 insertions(+), 21 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationTagDefinitionFactoryInspectTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Contracts/CalculationTagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Contracts/CalculationTagDefinitionFactory.cs index 37ca4929..1e6954f0 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Contracts/CalculationTagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Contracts/CalculationTagDefinitionFactory.cs @@ -68,26 +68,7 @@ public static class CalculationTagDefinitionFactory try { using var doc = JsonDocument.Parse(tagConfig); - var root = doc.RootElement; - if (root.ValueKind != JsonValueKind.Object) return false; - - var scriptId = root.TryGetProperty("scriptId", out var sidEl) && sidEl.ValueKind == JsonValueKind.String - ? sidEl.GetString() : null; - if (string.IsNullOrWhiteSpace(scriptId)) return false; - - var scriptSource = root.TryGetProperty("scriptSource", out var srcEl) && srcEl.ValueKind == JsonValueKind.String - ? srcEl.GetString() ?? string.Empty : string.Empty; - - // changeTriggered defaults to true (mini-design §2) — only an explicit false disables it. - var changeTriggered = !(root.TryGetProperty("changeTriggered", out var ctEl) - && ctEl.ValueKind == JsonValueKind.False); - - var timerMs = root.TryGetProperty("timerIntervalMs", out var tEl) - && tEl.ValueKind == JsonValueKind.Number && tEl.TryGetInt32(out var ms) && ms > 0 - ? ms : (int?)null; - - fields = new CalculationTagConfigFields(scriptId!, scriptSource, changeTriggered, timerMs); - return true; + return TryReadRoot(doc.RootElement, out fields); } catch (JsonException) { @@ -95,6 +76,41 @@ public static class CalculationTagDefinitionFactory } } + /// + /// The field-by-field read, over an already-parsed root. Split out of + /// so — which has necessarily parsed the blob + /// already to reach its root-kind and present-but-discarded checks — can consult the one authority + /// on what a usable scriptId is without parsing the same string a second time + /// (Driver.Calculation.Contracts-001). The public string-based entry point is what other modules + /// consume, so it keeps its signature. + /// + /// The parsed TagConfig root element. + /// The read fields when this returns . + /// when the root is an object naming a script. + private static bool TryReadRoot(JsonElement root, out CalculationTagConfigFields fields) + { + fields = default; + if (root.ValueKind != JsonValueKind.Object) return false; + + var scriptId = root.TryGetProperty("scriptId", out var sidEl) && sidEl.ValueKind == JsonValueKind.String + ? sidEl.GetString() : null; + if (string.IsNullOrWhiteSpace(scriptId)) return false; + + var scriptSource = root.TryGetProperty("scriptSource", out var srcEl) && srcEl.ValueKind == JsonValueKind.String + ? srcEl.GetString() ?? string.Empty : string.Empty; + + // changeTriggered defaults to true (mini-design §2) — only an explicit false disables it. + var changeTriggered = !(root.TryGetProperty("changeTriggered", out var ctEl) + && ctEl.ValueKind == JsonValueKind.False); + + var timerMs = root.TryGetProperty("timerIntervalMs", out var tEl) + && tEl.ValueKind == JsonValueKind.Number && tEl.TryGetInt32(out var ms) && ms > 0 + ? ms : (int?)null; + + fields = new CalculationTagConfigFields(scriptId!, scriptSource, changeTriggered, timerMs); + return true; + } + /// /// Deploy-time inspection of a calc tag's TagConfig blob. Warns for a blob /// rejects (unparseable JSON, a non-object root, or no usable @@ -130,7 +146,9 @@ public static class CalculationTagDefinitionFactory return warnings; } - if (!TryReadTagConfig(reference, out _)) + // The SAME read the runtime performs, over the root already parsed above — never a second + // parse of the same string, and never a reimplementation of the scriptId rule. + if (!TryReadRoot(root, out _)) { warnings.Add( "Calculation TagConfig has no usable 'scriptId' (a non-blank string) — the driver does not " diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationTagDefinitionFactoryInspectTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationTagDefinitionFactoryInspectTests.cs new file mode 100644 index 00000000..fac0c345 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationTagDefinitionFactoryInspectTests.cs @@ -0,0 +1,111 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests; + +/// +/// Deploy-time gate contract for . Its outcomes +/// were pinned only by the AdminUI parity corpus (a DB-backed suite); these fix the wording and the +/// rejection/advisory split locally, so the single-parse refactor +/// (Driver.Calculation.Contracts-001 — Inspect now reads its own already-parsed root instead of +/// re-parsing the blob through TryReadTagConfig) is provably behaviour-preserving. +/// The invariant underneath all of it: Inspect is never STRICTER than +/// — the deploy gate can be configured +/// to turn warnings into blocking errors, so it may never flag a blob the driver serves happily. +/// +public sealed class CalculationTagDefinitionFactoryInspectTests +{ + [Fact] + public void A_clean_blob_draws_nothing() + { + CalculationTagDefinitionFactory + .Inspect("""{"scriptId":"s1","scriptSource":"return 1.0;","changeTriggered":true,"timerIntervalMs":5000}""") + .ShouldBeEmpty(); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + [InlineData("[1,2]")] // not object-shaped at all — some other authoring surface's blob, not ours + [InlineData("plain text")] + public void A_blank_or_non_object_shaped_blob_is_left_alone(string blob) + { + CalculationTagDefinitionFactory.Inspect(blob).ShouldBeEmpty(); + } + + [Fact] + public void Unparseable_json_is_reported_as_a_skip() + { + CalculationTagDefinitionFactory.Inspect("{\"scriptId\":").ShouldBe( + [ + "Calculation TagConfig is not valid JSON — the driver will skip this tag and it will never compute.", + ]); + } + + // The "root is not a JSON object" warning is unreachable from Inspect by construction: the leading-'{' + // pre-filter above returns early for every non-object root, and anything that starts with '{' and + // parses IS an object. It is kept as a belt-and-braces guard for a future change to that pre-filter, + // so there is nothing to pin here beyond the pre-filter cases already covered. + + [Theory] + [InlineData("{}")] + [InlineData("""{"scriptId":" "}""")] + [InlineData("""{"scriptId":42}""")] + public void A_blob_with_no_usable_scriptId_is_reported_as_a_skip(string blob) + { + CalculationTagDefinitionFactory.Inspect(blob).ShouldContain( + "Calculation TagConfig has no usable 'scriptId' (a non-blank string) — the driver does not " + + "treat this as a calc tag, skips it, and it never computes."); + } + + [Fact] + public void A_non_boolean_changeTriggered_draws_the_present_but_discarded_advisory() + { + CalculationTagDefinitionFactory.Inspect("""{"scriptId":"s1","changeTriggered":"false"}""").ShouldBe( + [ + "value '\"false\"' for 'changeTriggered' is not a JSON boolean; anything but an explicit false " + + "is read as TRUE, so this tag is change-triggered whatever the value looks like.", + ]); + } + + [Theory] + [InlineData("0")] + [InlineData("-5")] + [InlineData("1.5")] + [InlineData("\"5000\"")] + public void A_timerIntervalMs_that_is_not_a_positive_whole_number_draws_the_present_but_discarded_advisory(string raw) + { + var warnings = CalculationTagDefinitionFactory.Inspect($$"""{"scriptId":"s1","timerIntervalMs":{{raw}}}"""); + + warnings.ShouldBe( + [ + $"value '{raw}' for 'timerIntervalMs' is not a positive whole number of milliseconds; it is " + + "discarded and the tag gets NO periodic re-evaluation.", + ]); + } + + /// The rejection and the advisories are independent: a blob can draw both, and the rejection + /// always comes first. + [Fact] + public void A_rejected_blob_still_reports_its_discarded_fields() + { + var warnings = CalculationTagDefinitionFactory.Inspect("""{"changeTriggered":1,"timerIntervalMs":0}"""); + + warnings.Count.ShouldBe(3); + warnings[0].ShouldStartWith("Calculation TagConfig has no usable 'scriptId'"); + warnings[1].ShouldStartWith("value '1' for 'changeTriggered'"); + warnings[2].ShouldStartWith("value '0' for 'timerIntervalMs'"); + } + + /// Falsification control for the "never stricter than the runtime read" invariant: every blob + /// the runtime ACCEPTS but this gate warns about must still be accepted by the runtime read, so an + /// advisory can never be mistaken for a rejection. + [Theory] + [InlineData("""{"scriptId":"s1","changeTriggered":"yes"}""")] + [InlineData("""{"scriptId":"s1","timerIntervalMs":0}""")] + public void An_advisory_never_means_the_runtime_read_rejects_the_blob(string blob) + { + CalculationTagDefinitionFactory.Inspect(blob).ShouldNotBeEmpty(); + CalculationTagDefinitionFactory.TryReadTagConfig(blob, out _).ShouldBeTrue(); + } +}