From ad9ad00cd27bb6e186f317fb72e4258c7c43c640 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 08:08:13 -0400 Subject: [PATCH] =?UTF-8?q?review(Driver.Sql.Contracts):=20both=20findings?= =?UTF-8?q?=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 ✅.