lmxopcua/code-reviews/Driver.Sql.Contracts/findings.md

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	0

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	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 (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.

---

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	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 (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 ✅.