From 8d4ca5489a8c03a779cb53204a78863365b4e01a Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 08:02:18 -0400 Subject: [PATCH 1/3] =?UTF-8?q?fix(tagconfig):=20a=20numerically-spelled?= =?UTF-8?q?=20enum=20names=20no=20member=20=E2=80=94=20reject=20it=20fleet?= =?UTF-8?q?-wide=20(Driver.Sql.Contracts-002)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TagConfigJson.ClassifyEnumText classified via Enum.TryParse(text, ignoreCase: true), which accepts a numeric STRING for any enum: "1" returns the member at that ordinal and an out-of-range "99" parses just as happily. Every driver contract's strict enum read inherits that, so a Sql tag authored "type":"99" was ACCEPTED, carried (DriverDataType)99, and SqlPollReader.TryCoerce fell through its switch to the String arm — a stringified cell published Good under a type nothing can map. Refuse a digit- or sign-leading spelling before the parse (judged on the TRIMMED text, because Enum.TryParse trims), and require Enum.IsDefined — which covers what the leading-character check cannot, since TryParse also ORs a comma-separated list for a non-[Flags] enum. Mirrors RawBrowseCommitMapper.ParseNativeType and MTConnectTagDefinitionFactory.TryParseDataType, which already guard their own parses. Three AdminUI restatements of the same rule now ask ClassifyEnumText instead of Enum.TryParse (TagConfigJson.GetEnum, SqlTagConfigModel.ReadEnumStrict, MqttTagConfigModel.DescribeInvalidEnum/GetEnumNullable) — leaving them would have opened the #549 direction the moment the runtime tightened: a blob the modal accepts and the driver skips. RED first: 10 failing in Core.Abstractions.Tests, 4 in Driver.Sql.Tests, and 10 of the 11 new fleet-wide parity cases (re-confirmed by reverting the Core fix). --- .../TagConfigJson.cs | 36 ++++++++- .../Uns/TagEditors/MqttTagConfigModel.cs | 15 +++- .../Uns/TagEditors/SqlTagConfigModel.cs | 9 ++- .../Uns/TagEditors/TagConfigJson.cs | 16 +++- .../TagConfigJsonTests.cs | 75 +++++++++++++++++++ .../SqlEquipmentTagParserTests.cs | 27 +++++++ .../TagConfigAuthoringDeployParityTests.cs | 31 ++++++++ 7 files changed, 198 insertions(+), 11 deletions(-) diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/TagConfigJson.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/TagConfigJson.cs index 034dfe4c..4ed9c856 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/TagConfigJson.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions/TagConfigJson.cs @@ -8,10 +8,13 @@ public enum JsonEnumRead /// The property is absent, or present but not a JSON string (nothing to validate). Absent, - /// The property is a JSON string that parses (case-insensitively) to the enum. + /// The property is a JSON string naming (case-insensitively) a declared enum member. Valid, - /// The property is a JSON string that does NOT parse to the enum (a typo'd value). + /// + /// The property is a JSON string that names no declared enum member — a typo, or a numeric + /// spelling such as "8", which is not a name at all. + /// Invalid, } @@ -67,6 +70,19 @@ public static class TagConfigJson /// treatment of an absent or present-but-non-string property — so a caller that extracts "the /// string value, or null" gets the runtime's leniency for free. /// + /// + /// Only a member NAME is valid — a numeric spelling is refused before the parse. + /// accepts "1" for any enum + /// and hands back whatever member sits at that ordinal, and an out-of-range "99" parses + /// just as readily — so a TagConfig carrying a numerically-serialised enum (the repo-wide + /// authoring trap) read at all six driver parsers and bound a + /// tag to a type nobody named, under Good quality. The same idiom guards + /// RawBrowseCommitMapper.NativeType and MTConnectTagDefinitionFactory's own type + /// parse. then covers what the leading-character + /// check cannot: TryParse also bitwise-ORs a comma-separated list for a + /// non-[Flags] enum, so "Beta, Gamma" would otherwise parse to a value no member + /// declares. + /// /// /// The enum type to parse. /// The authored value's text, or when there is none. @@ -80,12 +96,26 @@ public static class TagConfigJson value = default; return JsonEnumRead.Absent; } - if (Enum.TryParse(text, ignoreCase: true, out value)) + if (IsMemberNameSpelling(text) && Enum.TryParse(text, ignoreCase: true, out value) && Enum.IsDefined(value)) return JsonEnumRead.Valid; value = default; return JsonEnumRead.Invalid; } + /// + /// Whether is spelled as a member name rather than as a number. Judged on + /// the TRIMMED text because trims + /// before parsing, so " 8 " is the numeric spelling too; a C# member name can never begin + /// with a digit or a sign, so no valid name is lost. + /// + /// The authored value's text. + /// when the text could be a member name. + private static bool IsMemberNameSpelling(string text) + { + var span = text.AsSpan().Trim(); + return span.Length > 0 && !char.IsAsciiDigit(span[0]) && span[0] is not ('-' or '+'); + } + /// /// Lenient enum read with today's exact semantics: absent OR invalid ⇒ ; /// only a present, parseable string yields the parsed value. Bit-for-bit equivalent to the six 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 521957c5..f05bf28c 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 @@ -1,6 +1,8 @@ using System.Text.Json.Nodes; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Driver.Mqtt; +using RuntimeJsonEnumRead = ZB.MOM.WW.OtOpcUa.Core.Abstractions.JsonEnumRead; +using RuntimeTagConfigJson = ZB.MOM.WW.OtOpcUa.Core.Abstractions.TagConfigJson; namespace ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors; @@ -415,13 +417,20 @@ public sealed class MqttTagConfigModel /// The property name to read. /// The parsed enum value, or null. private static TEnum? GetEnumNullable(JsonObject o, string name) where TEnum : struct, Enum - => TagConfigJson.GetString(o, name) is { } s && Enum.TryParse(s, ignoreCase: true, out var v) ? v : null; + => RuntimeTagConfigJson.ClassifyEnumText(TagConfigJson.GetString(o, name), out var v) + == RuntimeJsonEnumRead.Valid + ? v + : null; /// /// Describes a present-but-invalid enum field, or null when it is absent or valid. /// Mirrors TagConfigJson.TryReadEnum's absent/valid/invalid split exactly — including its /// treatment of a present-but-non-string value as ABSENT — so the editor never blocks a blob the - /// driver would happily default. + /// driver would happily default. The VERDICT is the runtime's own + /// , never a local + /// : a numeric spelling such as + /// "1" parses there and names no member (Driver.Sql.Contracts-002), so restating the rule + /// here would accept a blob the driver refuses — the Gitea #549 direction. /// /// The enum type expected. /// The TagConfig key bag. @@ -433,7 +442,7 @@ public sealed class MqttTagConfigModel { return null; } - return Enum.TryParse(raw, ignoreCase: true, out _) + return RuntimeTagConfigJson.ClassifyEnumText(raw, out _) == RuntimeJsonEnumRead.Valid ? null : $"'{raw}' is not a valid {name}; valid: {string.Join(", ", Enum.GetNames())}."; } 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 e55b6e28..24d62a85 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 @@ -1,6 +1,7 @@ using System.Text.Json.Nodes; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts; +using RuntimeTagConfigJson = ZB.MOM.WW.OtOpcUa.Core.Abstractions.TagConfigJson; namespace ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors; @@ -156,7 +157,9 @@ public sealed class SqlTagConfigModel // Reads an enum-valued string field the way the parser's TryReadEnumStrict does: absent OR present-but- // non-string ⇒ (null, null) — nothing to validate; a present string that parses ⇒ (value, null); a - // present string that does NOT parse ⇒ (null, offendingString) so Validate can reject it. + // present string that does NOT parse ⇒ (null, offendingString) so Validate can reject it. The verdict is + // the RUNTIME's own ClassifyEnumText, not a local Enum.TryParse — restating it is how the two doors drift + // (Gitea #549), and a numeric spelling like "1" is exactly where they would have (Driver.Sql.Contracts-002). private static (TEnum? value, string? invalidRaw) ReadEnumStrict(JsonObject o, string name) where TEnum : struct, Enum { @@ -164,7 +167,9 @@ public sealed class SqlTagConfigModel { return (null, null); } - return Enum.TryParse(s, ignoreCase: true, out var parsed) ? (parsed, null) : (null, s); + return RuntimeTagConfigJson.ClassifyEnumText(s, out var parsed) == JsonEnumRead.Valid + ? (parsed, null) + : (null, s); } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/TagConfigJson.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/TagConfigJson.cs index 47c925a7..6537f462 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/TagConfigJson.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/TagConfigJson.cs @@ -1,5 +1,6 @@ using System.Text.Json; using System.Text.Json.Nodes; +using JsonEnumRead = ZB.MOM.WW.OtOpcUa.Core.Abstractions.JsonEnumRead; using RuntimeTagConfigJson = ZB.MOM.WW.OtOpcUa.Core.Abstractions.TagConfigJson; namespace ZB.MOM.WW.OtOpcUa.AdminUI.Uns.TagEditors; @@ -57,14 +58,23 @@ public static class TagConfigJson public static bool? GetBoolNullable(JsonObject o, string name) => o.TryGetPropertyValue(name, out var n) && n is JsonValue v && v.TryGetValue(out var b) ? b : null; - /// Reads an enum by its serialised name, or if absent/unparseable. + /// + /// Reads an enum by its serialised NAME, or if absent/unreadable. The + /// name/number distinction comes from + /// rather than from + /// directly, so a numerically-spelled + /// value the runtime refuses is not DISPLAYED here as the member sitting at that ordinal — the + /// editor would show, and then save, a type nobody authored. + /// /// The enum type to parse. /// The JSON object to read from. /// The property name to read. - /// The value to return if the property is absent or unparseable. + /// The value to return if the property is absent or unreadable. /// The parsed enum value, or . public static TEnum GetEnum(JsonObject o, string name, TEnum fallback) where TEnum : struct, Enum - => GetString(o, name) is { } s && Enum.TryParse(s, ignoreCase: true, out var v) ? v : fallback; + => RuntimeTagConfigJson.ClassifyEnumText(GetString(o, name), out var v) == JsonEnumRead.Valid + ? v + : fallback; /// /// Describes a present-but-invalid enum field the way the RUNTIME parser judges it, or diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/TagConfigJsonTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/TagConfigJsonTests.cs index 5a574b4a..7dae33c5 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/TagConfigJsonTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Abstractions.Tests/TagConfigJsonTests.cs @@ -35,6 +35,79 @@ public sealed class TagConfigJsonTests v.ShouldBe(expectedValue); } + // ---- A numeric spelling is not a member NAME (Driver.Sql.Contracts-002) ---- + + /// + /// accepts a numeric STRING for any + /// enum — "1" hands back whatever member sits at ordinal 1, and an out-of-range "99" + /// parses just as happily — so a TagConfig carrying a numerically-serialised enum (the repo-wide + /// authoring trap: a page that serialises enums as numbers into a string-typed field) used to read + /// at every one of the six driver parsers. The tag was then served + /// under a data type nobody named, publishing a confident Good — the exact failure R2-11's + /// strict read exists to prevent. A digit- or sign-leading value is refused BEFORE the parse. + /// + [Theory] + [InlineData("\"1\"")] // in range — parses today, and IsDefined would confirm it + [InlineData("\"99\"")] // out of range — parses today too + [InlineData("\"0\"")] + [InlineData("\"-1\"")] // sign-leading + [InlineData("\"+1\"")] + [InlineData("\" 2 \"")] // Enum.TryParse trims, so the guard must judge the TRIMMED text + public void TryReadEnum_aNumericSpelling_isInvalid_notAMemberName(string token) + { + TagConfigJson.TryReadEnum(Root($"{{\"k\":{token}}}"), "k", out var v) + .ShouldBe(JsonEnumRead.Invalid); + v.ShouldBe(default(Sample)); + } + + /// + /// The half of the guard, which the digit-leading check + /// cannot cover: also accepts a + /// COMMA-SEPARATED list for a non-[Flags] enum and bitwise-ORs the members, so + /// "Beta, Gamma" parses to 3 — a value no member declares. Deleting the + /// IsDefined check turns this case green again. + /// + [Fact] + public void TryReadEnum_aCombinationThatNoMemberDeclares_isInvalid() + { + TagConfigJson.TryReadEnum(Root("{\"k\":\"Beta, Gamma\"}"), "k", out var v) + .ShouldBe(JsonEnumRead.Invalid); + v.ShouldBe(default(Sample)); + } + + /// + /// Falsifiability control for the two guards above: a guard that refused everything would satisfy + /// them all. Member NAMES — the only spelling a TagConfig may carry — keep parsing, whatever their + /// case and surrounding whitespace. + /// + [Theory] + [InlineData("Alpha", Sample.Alpha)] + [InlineData("beta", Sample.Beta)] + [InlineData("GAMMA", Sample.Gamma)] + [InlineData(" Gamma ", Sample.Gamma)] + public void ClassifyEnumText_stillAcceptsEveryMemberName(string text, Sample expected) + { + TagConfigJson.ClassifyEnumText(text, out var v).ShouldBe(JsonEnumRead.Valid); + v.ShouldBe(expected); + } + + /// + /// A numeric spelling is refused with the SAME wording every other rejection carries — naming the + /// field, the offending value and the members that are allowed — so the operator is told what to + /// write instead of being told nothing. + /// + [Fact] + public void DescribeInvalidEnum_aNumericSpelling_namesTheAllowedMembers() + { + var msg = TagConfigJson.DescribeInvalidEnum(Root("{\"dataType\":\"8\"}"), "dataType"); + msg.ShouldNotBeNull(); + msg.ShouldContain("'8'"); + msg.ShouldContain("dataType"); + msg.ShouldContain("Alpha"); + msg.ShouldContain("Beta"); + msg.ShouldContain("Gamma"); + } + [Theory] [InlineData("{\"k\":\"Gamma\"}", Sample.Gamma)] // valid → parsed [InlineData("{\"k\":\"typo\"}", Sample.Alpha)] // invalid → fallback @@ -50,6 +123,8 @@ public sealed class TagConfigJsonTests [InlineData("{}", true, Sample.Alpha)] // absent → fallback, ok [InlineData("{\"k\":42}", true, Sample.Alpha)] // non-string → Absent → fallback, ok [InlineData("{\"k\":\"typo\"}", false, default(Sample))] // invalid (typo) → REJECT (false) + [InlineData("{\"k\":\"1\"}", false, default(Sample))] // numeric spelling → REJECT (Driver.Sql.Contracts-002) + [InlineData("{\"k\":\"99\"}", false, default(Sample))] // …in range or out of it public void TryReadEnumStrict_rejects_only_invalid(string json, bool expectedOk, Sample expectedValue) { var ok = TagConfigJson.TryReadEnumStrict(Root(json), "k", Sample.Alpha, out var v); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlEquipmentTagParserTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlEquipmentTagParserTests.cs index bec219f7..ae5e37c7 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlEquipmentTagParserTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlEquipmentTagParserTests.cs @@ -196,4 +196,31 @@ public class SqlEquipmentTagParserTests => SqlEquipmentTagParser.TryParse( """{"driver":"Sql","model":"KeyValue","table":"T","keyColumn":"k","keyValue":"v","valueColumn":"c","type":"Nonsense"}""", "raw", out _).ShouldBeFalse(); + + /// + /// Driver.Sql.Contracts-002. A numerically-spelled enum is not a member name, and the strict read + /// must refuse it exactly as it refuses a typo. It used to parse — "99" gave + /// (DriverDataType)99, a declared type nothing can map, and SqlPollReader.TryCoerce + /// then fell through its switch to the String arm and published a stringified cell under it. + /// + [Theory] + [InlineData("""{"driver":"Sql","model":"KeyValue","table":"T","keyColumn":"k","keyValue":"v","valueColumn":"c","type":"99"}""")] + [InlineData("""{"driver":"Sql","model":"KeyValue","table":"T","keyColumn":"k","keyValue":"v","valueColumn":"c","type":"8"}""")] + // "1" is SqlTagModel.WideRow's ordinal — an otherwise-complete WideRow blob, so the rejection can only + // come from the enum read and not from a missing field. + [InlineData("""{"driver":"Sql","model":"1","table":"T","columnName":"c","rowSelector":{"topByTimestamp":"ts"}}""")] + public void TryParse_aNumericallySpelledEnum_rejectsTag(string json) + => SqlEquipmentTagParser.TryParse(json, "raw", out _).ShouldBeFalse(); + + /// + /// …and the deploy gate explains it in the same words every other invalid enum draws, rather than + /// falling through to the backstop warning that names no field at all. + /// + [Fact] + public void Inspect_aNumericallySpelledType_namesTheFieldAndTheValidMembers() + { + var warnings = SqlTagDefinitionFactory.Inspect( + """{"driver":"Sql","model":"KeyValue","table":"T","keyColumn":"k","keyValue":"v","valueColumn":"c","type":"99"}"""); + warnings.ShouldContain(w => w.Contains("'99'") && w.Contains("type") && w.Contains(nameof(DriverDataType.Float64))); + } } 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 6bb8c2c0..ec558401 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 @@ -220,6 +220,37 @@ public sealed class TagConfigAuthoringDeployParityTests + "import or API is unchecked — the exact G-7 asymmetry."); } + /// + /// Driver.Sql.Contracts-002, fleet-wide. A numerically-SPELLED enum ("1", not the JSON + /// number 1) used to satisfy Enum.TryParse at every driver, so both doors accepted it + /// and the tag was served under whatever member sat at that ordinal — or, out of range, under a + /// value no member declares. The shared TagConfigJson.ClassifyEnumText now calls it what it + /// is, and because BOTH gates ask that one method, they moved together: the fix is only real if + /// each driver refuses it at the modal AND at the deploy gate. + /// + [Theory] + // Complete but for the enum spelling, so the refusal can only come from the enum read: "1" is + // SqlTagModel.WideRow's ordinal, and every WideRow-required field is present. + [InlineData(DriverTypeNames.Sql, """{"model":"1","table":"dbo.T","columnName":"c","rowSelector":{"topByTimestamp":"ts"}}""")] + [InlineData(DriverTypeNames.Sql, """{"model":"KeyValue","table":"dbo.T","keyColumn":"k","keyValue":"v","valueColumn":"c","type":"8"}""")] + [InlineData(DriverTypeNames.Modbus, """{"region":"1","address":1}""")] + [InlineData(DriverTypeNames.Modbus, """{"region":"HoldingRegisters","address":1,"dataType":"99"}""")] + [InlineData(DriverTypeNames.S7, """{"address":"DB1.DBW0","dataType":"1"}""")] + [InlineData(DriverTypeNames.AbCip, """{"tagPath":"Program:Main.Speed","dataType":"1"}""")] + [InlineData(DriverTypeNames.AbLegacy, """{"address":"N7:0","dataType":"1"}""")] + [InlineData(DriverTypeNames.TwinCAT, """{"symbolPath":"MAIN.bStart","dataType":"1"}""")] + [InlineData(DriverTypeNames.FOCAS, """{"address":"R100","dataType":"1"}""")] + [InlineData(DriverTypeNames.Mqtt, """{"topic":"otopcua/fixture/a","payloadFormat":"1"}""")] + [InlineData(DriverTypeNames.Mqtt, """{"topic":"otopcua/fixture/a","dataType":"8"}""")] + public void A_numerically_spelled_enum_is_refused_by_both_gates(string driverType, string tagConfig) + { + TagConfigValidator.Validate(driverType, tagConfig).ShouldNotBeNullOrEmpty( + $"{driverType}: the modal would let an operator save an enum spelling the driver refuses."); + EquipmentTagConfigInspector.Inspect(driverType, tagConfig).ShouldNotBeEmpty( + $"{driverType}: the deploy gate accepted a numeric enum spelling, so a CSV-imported or " + + "API-authored tag binds to a type nobody named."); + } + // ------------------------------------------------------------------------------------------------- // Gitea #550 — MTConnect's second spelling of the coercion type. The assertions below drive the REAL // runtime reader (MTConnectTagDefinitionFactory.TryFromTagConfig) rather than the deploy gate alone, From 678aa3aa538cd4ea5b1a257ca585d404b6dc7c66 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 08:07:15 -0400 Subject: [PATCH 2/3] fix(sql): read keyValue with the same scalar read as whereValue (Driver.Sql.Contracts-001) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two value-bearing fields were read asymmetrically and nothing said why. whereValue went through ReadScalar (string/number/boolean — the design doc's own wide-row example authors it as the number 7); keyValue went through the string-only ReadString, so a numerically-authored key refused the tag outright. Nothing downstream justified the split: both are BOUND as DbParameters and both are matched back as text — SqlPollReader.IndexByKey stringifies the key cell precisely so an authored key can face an INTEGER key column. The refusal was also mis-explained. Inspect branched on ReadString(root, "keyValue") is null, so a numerically-authored key drew "Sql KeyValue tag has no 'keyValue'" — telling the operator the field is missing when it is right there in the blob, the #549 shape of an authoring surface disagreeing with the runtime about WHY. keyValue now uses ReadScalar; Inspect mirrors it with HasScalar and says what a usable key is; SqlTagConfigModel loads it with the shared TagConfigJson.GetScalarText (which also replaces SqlRowSelectorModel's private copy), so the editor is not left stricter than the parser. An object/array/null key is still refused. docs/drivers/Sql.md updated. RED first: 4 failing in Driver.Sql.Tests. --- docs/drivers/Sql.md | 9 +++- .../SqlEquipmentTagParser.cs | 15 ++++-- .../SqlTagDefinitionFactory.cs | 9 +++- .../Uns/TagEditors/SqlTagConfigModel.cs | 16 ++---- .../Uns/TagEditors/TagConfigJson.cs | 17 +++++++ .../SqlEquipmentTagParserTests.cs | 49 +++++++++++++++++++ .../Uns/SqlTagConfigModelTests.cs | 26 ++++++++++ 7 files changed, 123 insertions(+), 18 deletions(-) diff --git a/docs/drivers/Sql.md b/docs/drivers/Sql.md index 38163f50..b5bf2128 100644 --- a/docs/drivers/Sql.md +++ b/docs/drivers/Sql.md @@ -80,8 +80,13 @@ the nested form. } ``` -`whereValue` may be authored as a JSON string, number or boolean (it is bound as a parameter either -way). `keyValue` on the `KeyValue` model must be a **string**. +Both value-bearing fields — `whereValue` here and `keyValue` on the `KeyValue` model — may be +authored as a JSON string, number or boolean, and are captured as text. They are **bound as +parameters** either way, and matched back as text (the key cell is stringified), so a key column +that holds an INTEGER station id can be authored either `"7"` or `7`. A JSON object, array or `null` +is not a value and is refused. (They were read asymmetrically until 2026-08-10, so a numerically +authored `keyValue` refused the tag while the identically-bound `whereValue` was served — and the +deploy gate reported the key as *missing* when it was right there in the blob.) ### `Query` — deferred to P3 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts/SqlEquipmentTagParser.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts/SqlEquipmentTagParser.cs index 006089f8..1414da48 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts/SqlEquipmentTagParser.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts/SqlEquipmentTagParser.cs @@ -62,7 +62,7 @@ public static class SqlEquipmentTagParser case SqlTagModel.KeyValue: { var keyColumn = ReadString(root, "keyColumn"); - var keyValue = ReadString(root, "keyValue"); + var keyValue = ReadScalar(root, "keyValue"); var valueColumn = ReadString(root, "valueColumn"); if (string.IsNullOrWhiteSpace(keyColumn) || string.IsNullOrWhiteSpace(valueColumn) @@ -115,8 +115,17 @@ public static class SqlEquipmentTagParser /// /// Reads a scalar property as its textual form — a JSON string yields its contents, a number or - /// boolean yields its raw token — so an authored whereValue of either shape is captured - /// verbatim for later parameter binding. Null when absent or not a scalar. + /// boolean yields its raw token — so an authored value of any of those shapes is captured verbatim + /// for later parameter binding. Null when absent or not a scalar. + /// + /// Both value-bearing fields go through here, keyValue as well as + /// whereValue, because there is no downstream difference between them: the reader BINDS + /// each as a and matches each back as text — + /// SqlPollReader.IndexByKey stringifies the key cell precisely so an authored key can + /// face an INTEGER key column. Reading one of them string-only refused a tag the other shape + /// of the same authoring mistake was served under, and the deploy gate then reported the field + /// as MISSING when it was right there in the blob. + /// /// private static string? ReadScalar(JsonElement o, string name) { diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts/SqlTagDefinitionFactory.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts/SqlTagDefinitionFactory.cs index cf06e1d3..bad2b98d 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts/SqlTagDefinitionFactory.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts/SqlTagDefinitionFactory.cs @@ -129,8 +129,13 @@ public static class SqlTagDefinitionFactory warnings.Add("Sql KeyValue tag has no 'keyColumn'."); if (string.IsNullOrWhiteSpace(ReadString(root, "valueColumn"))) warnings.Add("Sql KeyValue tag has no 'valueColumn'."); - if (ReadString(root, "keyValue") is null) - warnings.Add("Sql KeyValue tag has no 'keyValue' (an empty string is allowed; an absent key is not)."); + if (!HasScalar(root, "keyValue")) + { + warnings.Add( + "Sql KeyValue tag has no usable 'keyValue' — it must be a string, number or " + + "boolean (an empty string is allowed; an absent key, or an object/array, is not)."); + } + break; case SqlTagModel.WideRow: 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 24d62a85..48928649 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 @@ -25,7 +25,9 @@ public sealed class SqlTagConfigModel /// : the column matched against . public string? KeyColumn { get; set; } - /// : this tag's key value (bound as a parameter at read time). + /// : this tag's key value (bound as a parameter at read time). + /// Captured as text whether the blob authored it as a JSON string or a JSON number/boolean, mirroring + /// the parser's scalar read. public string? KeyValue { get; set; } /// : the column the value is read from. @@ -72,7 +74,7 @@ public sealed class SqlTagConfigModel Model = model ?? SqlTagModel.KeyValue, Table = TagConfigJson.GetString(o, "table"), KeyColumn = TagConfigJson.GetString(o, "keyColumn"), - KeyValue = TagConfigJson.GetString(o, "keyValue"), + KeyValue = TagConfigJson.GetScalarText(o, "keyValue"), ValueColumn = TagConfigJson.GetString(o, "valueColumn"), TimestampColumn = TagConfigJson.GetString(o, "timestampColumn"), ColumnName = TagConfigJson.GetString(o, "columnName"), @@ -204,7 +206,7 @@ public sealed class SqlRowSelectorModel return new SqlRowSelectorModel { WhereColumn = TagConfigJson.GetString(bag, "whereColumn"), - WhereValue = ReadScalarText(bag, "whereValue"), + WhereValue = TagConfigJson.GetScalarText(bag, "whereValue"), TopByTimestamp = TagConfigJson.GetString(bag, "topByTimestamp"), _bag = bag, }; @@ -221,12 +223,4 @@ public sealed class SqlRowSelectorModel TagConfigJson.Set(o, "topByTimestamp", TopByTimestamp); return o.Count == 0 ? null : o; } - - // Reads a scalar as its textual form: a JSON string yields its contents, a number/boolean yields its raw - // token — so an authored whereValue of either shape is captured verbatim (mirrors the parser's ReadScalar). - private static string? ReadScalarText(JsonObject o, string name) - { - if (!o.TryGetPropertyValue(name, out var n) || n is not JsonValue v) { return null; } - return v.TryGetValue(out var s) ? s : v.ToJsonString(); - } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/TagConfigJson.cs b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/TagConfigJson.cs index 6537f462..38d4a5d8 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/TagConfigJson.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Uns/TagEditors/TagConfigJson.cs @@ -34,6 +34,23 @@ public static class TagConfigJson public static string? GetString(JsonObject o, string name) => o.TryGetPropertyValue(name, out var n) && n is JsonValue v && v.TryGetValue(out var s) ? s : null; + /// + /// Reads a scalar as its textual form — a JSON string yields its contents, a number or boolean + /// yields its raw token; null when absent, JSON null, or a non-scalar node. Mirrors + /// the runtime parsers' ReadScalar, for the value-bearing fields a driver BINDS as a + /// parameter and so may legitimately author as a number (a Sql keyValue facing an INTEGER + /// key column, say). Reading such a field with makes the editor refuse a + /// blob the driver serves. + /// + /// The JSON object to read from. + /// The property name to read. + /// The scalar's text, or null when absent or not a scalar. + public static string? GetScalarText(JsonObject o, string name) + { + if (!o.TryGetPropertyValue(name, out var n) || n is not JsonValue v) { return null; } + return v.TryGetValue(out var s) ? s : v.ToJsonString(); + } + /// Reads an int value, or if absent/null/non-numeric (incl. object/array nodes). /// The JSON object to read from. /// The property name to read. diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlEquipmentTagParserTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlEquipmentTagParserTests.cs index ae5e37c7..4734ee30 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlEquipmentTagParserTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlEquipmentTagParserTests.cs @@ -28,6 +28,55 @@ public class SqlEquipmentTagParserTests def.Name.ShouldBe("Plant/Sql/dev1/Speed"); // Name == RawPath (forward-router key) } + /// + /// Driver.Sql.Contracts-001. The two value-bearing fields are read the same way: a JSON string, + /// number or boolean is captured as text, because both end up BOUND as a DbParameter and + /// both are matched back as text — SqlPollReader.IndexByKey stringifies the key cell + /// precisely so an authored key can face an INTEGER key column. keyValue used to go through + /// the narrower string-only read, so a numerically-authored key refused the tag outright while the + /// identically-bound whereValue sailed through as the design doc's own example authors it. + /// + [Theory] + [InlineData("7", "7")] + [InlineData("-12.5", "-12.5")] + [InlineData("true", "true")] + public void TryParse_KeyValue_aNonStringScalarKey_isCapturedAsTextLikeWhereValue(string token, string expected) + { + var json = + $$"""{"driver":"Sql","model":"KeyValue","table":"dbo.TagValues","keyColumn":"station_id","keyValue":{{token}},"valueColumn":"num_value"}"""; + SqlEquipmentTagParser.TryParse(json, "raw", out var def).ShouldBeTrue(); + def.KeyValue.ShouldBe(expected); + } + + /// + /// …and a non-scalar key is still refused — the widening is to the scalar shapes a parameter can + /// carry, not to anything at all. + /// + [Theory] + [InlineData("""{"driver":"Sql","model":"KeyValue","table":"T","keyColumn":"k","keyValue":{"a":1},"valueColumn":"c"}""")] + [InlineData("""{"driver":"Sql","model":"KeyValue","table":"T","keyColumn":"k","keyValue":[1],"valueColumn":"c"}""")] + [InlineData("""{"driver":"Sql","model":"KeyValue","table":"T","keyColumn":"k","keyValue":null,"valueColumn":"c"}""")] + [InlineData("""{"driver":"Sql","model":"KeyValue","table":"T","keyColumn":"k","valueColumn":"c"}""")] + public void TryParse_KeyValue_aNonScalarOrAbsentKey_rejectsTag(string json) + => SqlEquipmentTagParser.TryParse(json, "raw", out _).ShouldBeFalse(); + + /// + /// The Inspect half of the same rule. A numerically-authored key is served, so the deploy + /// gate must say nothing about it; an absent one is refused, and the gate must still name it. + /// + [Fact] + public void Inspect_aNumericallyAuthoredKeyValue_drawsNoWarning() + => SqlTagDefinitionFactory.Inspect( + """{"driver":"Sql","model":"KeyValue","table":"T","keyColumn":"k","keyValue":7,"valueColumn":"c"}""") + .ShouldBeEmpty(); + + /// Falsifiability control for the assertion above: a genuinely absent key still warns. + [Fact] + public void Inspect_anAbsentKeyValue_stillNamesTheField() + => SqlTagDefinitionFactory.Inspect( + """{"driver":"Sql","model":"KeyValue","table":"T","keyColumn":"k","valueColumn":"c"}""") + .ShouldContain(w => w.Contains("keyValue")); + [Fact] public void TryParse_invalidModelEnum_rejectsTag() => SqlEquipmentTagParser.TryParse( diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/SqlTagConfigModelTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/SqlTagConfigModelTests.cs index bfd0ad86..370de1ad 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/SqlTagConfigModelTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/SqlTagConfigModelTests.cs @@ -184,6 +184,32 @@ public sealed class SqlTagConfigModelTests => SqlTagConfigModel.FromJson("""{"model":"KeyValue","table":"t","keyColumn":"k","valueColumn":"c"}""") .Validate().ShouldNotBeNull(); + /// + /// Driver.Sql.Contracts-001. The parser reads keyValue with the same scalar read it uses for + /// whereValue, so a numerically-authored key is SERVED — the editor must load it (rather + /// than seeing an absent key), let it save, and normalise it to a string on the way out, exactly as + /// it already does for whereValue. An editor that refused it would be stricter than the + /// runtime, which is the drift this parity suite exists to catch. + /// + [Fact] + public void A_numerically_authored_keyValue_loads_validates_and_normalises_to_a_string() + { + const string json = + """{"model":"KeyValue","table":"t","keyColumn":"station_id","keyValue":7,"valueColumn":"c"}"""; + SqlEquipmentTagParser.TryParse(json, "raw", out var served).ShouldBeTrue( + "the runtime must serve this blob, or the authoring parity below proves nothing."); + served.KeyValue.ShouldBe("7"); + + var m = SqlTagConfigModel.FromJson(json); + m.KeyValue.ShouldBe("7"); + m.Validate().ShouldBeNull(); + + var saved = m.ToJson(); + saved.ShouldContain("\"keyValue\":\"7\""); + SqlEquipmentTagParser.TryParse(saved, "raw", out var afterSave).ShouldBeTrue(); + afterSave.KeyValue.ShouldBe("7"); + } + [Fact] public void Validate_rejects_wideRow_without_a_selector() => SqlTagConfigModel.FromJson("""{"model":"WideRow","table":"t","columnName":"c"}""") From ad9ad00cd27bb6e186f317fb72e4258c7c43c640 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 08:08:13 -0400 Subject: [PATCH 3/3] =?UTF-8?q?review(Driver.Sql.Contracts):=20both=20find?= =?UTF-8?q?ings=20resolved=20=E2=80=94=20open=20count=202=20=E2=86=92=200?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 001 aligned keyValue onto the same scalar read as whereValue (678aa3aa); 002 fixed the numeric-enum-spelling accept in Core.Abstractions.TagConfigJson.ClassifyEnumText, fleet-wide (8d4ca548). --- code-reviews/Driver.Sql.Contracts/findings.md | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/code-reviews/Driver.Sql.Contracts/findings.md b/code-reviews/Driver.Sql.Contracts/findings.md index 0ff51b9d..701e551d 100644 --- a/code-reviews/Driver.Sql.Contracts/findings.md +++ b/code-reviews/Driver.Sql.Contracts/findings.md @@ -11,7 +11,7 @@ | Review date | 2026-08-10 | | Commit reviewed | `b990adae` | | Status | Reviewed | -| Open findings | 2 | +| Open findings | 0 | ## Checklist coverage @@ -37,13 +37,13 @@ | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts/SqlEquipmentTagParser.cs:65`, `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts/SqlTagDefinitionFactory.cs:132` | -| Status | Open | +| Status | Resolved | **Description:** The two value-bearing fields are read asymmetrically and nothing documents it. `whereValue` goes through `ReadScalar`, which accepts a JSON string, number or boolean and captures the raw token — the design doc's own wide-row example authors it as the number `7`. `keyValue` goes through `ReadString`, so a JSON number is read as null and the tag is refused outright. Both end up bound as a `DbParameter` and both are matched back as text (`IndexByKey` stringifies the key cell), so there is no downstream reason for the difference. The refusal is also mis-explained: `Inspect`'s branch is `ReadString(root, "keyValue") is null`, so a numerically-authored key draws "Sql KeyValue tag has no 'keyValue'" — telling the operator the field is missing when it is right there in the blob. **Recommendation:** Read `keyValue` with the same `ReadScalar` the row selector uses (the value is bound either way), and mirror the change in `Inspect` so the accept gate and its explanation stay the same parse. If the string-only rule is intended, say so in the rejection text and in `docs/drivers/Sql.md` instead — the current wording is the failure mode the #549 arc calls out, an authoring surface disagreeing with the runtime about *why*. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed by **aligning**, in `678aa3aa`. `keyValue` now goes through the same `ReadScalar` the row selector uses, so a JSON string, number or boolean is captured as text and an object/array/`null` is still refused. Aligning rather than documenting is what the runtime already assumed: `SqlPollReader.IndexByKey` stringifies the key cell *because* "the bound keyValue is authored text while the column may be any type (an INTEGER station id, say)" — its own comment — so the string-only read was refusing a shape the reader was built to serve. `Inspect` mirrors it through `HasScalar` and its wording now names what a usable key is ("must be a string, number or boolean") instead of reporting a field that is present as missing. The editor was carried along in the same change, or it would have been left stricter than the parser: `SqlTagConfigModel` loads `keyValue` with a shared `TagConfigJson.GetScalarText` (which also replaced `SqlRowSelectorModel`'s private copy of the same read), so a numerically-authored key loads, validates and normalises to a string on save exactly as `whereValue` does. `docs/drivers/Sql.md` replaces its "`keyValue` must be a string" line with the joint rule. TDD: 4 RED in `Driver.Sql.Tests` first — three `keyValue` scalar shapes plus the `Inspect` mis-explanation — including a falsifiability control that a genuinely *absent* key still warns. --- @@ -54,10 +54,16 @@ | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts/SqlEquipmentTagParser.cs:50` | -| Status | Open | +| Status | Resolved | **Description:** Cross-module, and reachable through every driver rather than only this one. The strict enum read is `TagConfigJson.TryReadEnumStrict`, which classifies via `Enum.TryParse(text, ignoreCase: true, …)` — and that method accepts a **numeric string**, returning `true` for a value that is not a declared member. So `"type": "99"` reads as `JsonEnumRead.Valid`, the parser accepts the tag, and `SqlTagDefinition.DeclaredType` carries `(DriverDataType)99`: `DiscoverAsync` materialises a variable whose declared type is undefined, and `SqlPollReader.TryCoerce` falls through its switch to the `String` arm, so the node reads a stringified cell under a data type nothing can map. `"model": "7"` is harmless here only by luck — the planner's `default` arm rejects it. Nothing in the module can fix this alone, because the accept/reject authority is deliberately the shared helper. **Recommendation:** Add an `Enum.IsDefined` check to `TagConfigJson.ClassifyEnumText` so a numeric spelling is classified `Invalid` (and therefore refused by the strict read and named by `DescribeInvalidEnum`) — one change covering all six parsers and both the runtime and authoring doors. Cross-module, so recorded rather than fixed here; verify the parity corpus in `AdminUI.Tests` after the change, since it exercises every driver's `Inspect`. -**Resolution:** _(open)_ +**Resolution (2026-08-10):** Fixed in Core.Abstractions, in `8d4ca548`. `TagConfigJson.ClassifyEnumText` now refuses a digit- or sign-leading spelling **before** `Enum.TryParse` and requires `Enum.IsDefined` after it — mirroring the guard `RawBrowseCommitMapper.ParseNativeType` and `MTConnectTagDefinitionFactory.TryParseDataType` already carry over their own parses. Two things the recommendation's single `IsDefined` check would not have covered, and both have a test that goes red when the other guard is deleted: `IsDefined` alone still accepts an **in-range** ordinal (`"1"` binds whatever member sits there — the common shape, since a page serialising enums numerically emits small values), and the leading-character check must judge the **trimmed** text because `Enum.TryParse` trims before parsing, so `" 8 "` is the numeric spelling too. `IsDefined` earns its place on the case the character check cannot see: `TryParse` also bitwise-ORs a comma-separated list for a non-`[Flags]` enum, so `"Beta, Gamma"` parses to a value no member declares. (Only one `[Flags]` enum exists in `src/`, `NodePermissions`, and it never reaches these readers.) + +Three AdminUI **restatements** of the same rule were routed to `ClassifyEnumText` in the same commit — `TagConfigJson.GetEnum`, `SqlTagConfigModel.ReadEnumStrict`, and `MqttTagConfigModel.DescribeInvalidEnum`/`GetEnumNullable` all called `Enum.TryParse` directly. Left alone they would have opened the Gitea #549 direction the instant the runtime tightened: a numeric spelling the modal accepts and rewrites, and the driver then skips with `BadNodeIdUnknown`. Every other editor already consults `DescribeInvalidEnumText`, so it moved for free. + +TDD: 10 RED in `Core.Abstractions.Tests` (the numeric matrix, the strict read, the message wording, and the comma-list case) plus 4 in `Driver.Sql.Tests`. The canary the recommendation asks for was made explicit rather than only run — a new fleet-wide `TagConfigAuthoringDeployParityTests.A_numerically_spelled_enum_is_refused_by_both_gates` covers eight driver types across both gates; 10 of its 11 cases were re-confirmed RED by reverting the Core fix. No pre-existing corpus blob changed verdict: `AdminUI.Tests` was green at its true baseline of 1466 both before and after (the 1474 figure in the brief was stale). + +**Suites:** `Core.Abstractions.Tests` 298 ✅ · `Driver.Sql.Tests` 378 ✅ · `AdminUI.Tests` 1478 ✅.