From 368d8e9581149f738677e0cde7acc6b598bf345c Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 06:58:29 -0400 Subject: [PATCH] review(Driver.Sql.Contracts): 2 Low findings, both record-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 001 keyValue is read string-only while whereValue accepts any scalar, and Inspect then reports a numerically-authored key as ABSENT — an authoring surface disagreeing with the runtime about why (the #549 class); 002 cross-module: TagConfigJson's strict enum read rests on Enum.TryParse, which accepts a numeric string, so "type":"99" is Valid and an undefined DriverDataType reaches the address space. Fix belongs in Core.Abstractions. The accept-gate-first Inspect and the strict enum reads are otherwise exactly the #549-#559 arc's rules, implemented structurally. --- code-reviews/Driver.Sql.Contracts/findings.md | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 code-reviews/Driver.Sql.Contracts/findings.md diff --git a/code-reviews/Driver.Sql.Contracts/findings.md b/code-reviews/Driver.Sql.Contracts/findings.md new file mode 100644 index 00000000..0ff51b9d --- /dev/null +++ b/code-reviews/Driver.Sql.Contracts/findings.md @@ -0,0 +1,63 @@ +# Code Review — Driver.Sql.Contracts + + + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts` | +| Reviewer | Claude Code | +| Review date | 2026-08-10 | +| Commit reviewed | `b990adae` | +| Status | Reviewed | +| Open findings | 2 | + +## Checklist coverage + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | 2 findings (001, 002) | +| 2 | OtOpcUa conventions | No issues found — provider-free by construction (the only `ProjectReference` is `Core.Abstractions`), enum-typed DTO fields with the converter applied by the factory rather than by an attribute, `*TagDefinitionFactory.Inspect` named for the reflection-discovered deploy-gate seam | +| 3 | Concurrency & thread safety | No issues found — every type here is a pure function or an immutable record | +| 4 | Error handling & resilience | No issues found — `TryParse` returns false rather than throwing for every refusal, and `Inspect` is total (the backstop warning covers a refusal none of its branches names) | +| 5 | Security | No issues found. `SqlTagDefinition`'s doc states the rule the rest of the driver enforces — value fields are bound, identifier fields are catalog-validated and dialect-quoted — and the parser deliberately does not sanitise either kind, which is correct: sanitising a value corrupts data and sanitising an identifier masks the authoring error the gate reports properly | +| 6 | Performance & resource management | No issues found — one `JsonDocument` per blob, disposed | +| 7 | Design-document adherence | No issues found. The accept-gate-first `Inspect` (run the runtime parser, return empty on success, only then explain) is the #549–#559 arc's rule implemented structurally rather than promised, and the strict enum reads are the deliberate R2-11 behaviour | +| 8 | Code organization & conventions | No issues found | +| 9 | Testing coverage | No issues found — the parser, the strict-enum rejections and the injection-shaped `keyValue` are covered from `Driver.Sql.Tests`, and `Inspect` is covered by the AdminUI parity corpus | +| 10 | Documentation & comments | No issues found | + +## Findings + +### Driver.Sql.Contracts-001 + +| Field | Value | +|---|---| +| 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 | + +**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)_ + +--- + +### Driver.Sql.Contracts-002 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts/SqlEquipmentTagParser.cs:50` | +| Status | Open | + +**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)_