Joseph Doherty
1fe0017600
Dialect header 3 -> 0 open. The Driver.Sql header count is left alone for the orchestrator to reconcile — sibling agents are editing that file in parallel.
10 KiB
Code Review — Driver.Sql.Dialect
| Field | Value |
|---|---|
| Module | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect |
| Reviewer | Claude Code |
| Review date | 2026-08-10 |
| Commit reviewed | b990adae |
| Status | Reviewed |
| Open findings | 0 |
Checklist coverage
| # | Category | Result |
|---|---|---|
| 1 | Correctness & logic bugs | 1 finding (001). The three implementations agree member-for-member on the contract, and the two places a dialect must diverge — ODP.NET's marker/bind-name split and BindByName — are modelled explicitly rather than derived |
| 2 | OtOpcUa conventions | No issues found. The assembly is .Dialect while the namespace stays ZB.MOM.WW.OtOpcUa.Driver.Sql, which is the documented Driver.Mqtt.Transport precedent — the split is a build-graph concern, and the csproj says so |
| 3 | Concurrency & thread safety | No issues found — every dialect is stateless and immutable, which is what lets one instance be shared by the poll reader and a browse session |
| 4 | Error handling & resilience | No issues found. MapColumnType never throws and falls back to String; ConfigureCommand tolerates a foreign DbCommand rather than casting blind |
| 5 | Security | No issues found. QuoteIdentifier escapes the one metacharacter that can terminate a quoted identifier on each backend, rejects control and Unicode-format characters (the Trojan-Source class), and never echoes the offending value; the catalog SQL binds every runtime value as a parameter |
| 6 | Performance & resource management | No issues found — the catalog statements are bounded by their WHERE clauses and hold no state |
| 7 | Design-document adherence | No issues found — the constructed set (SqlServer, Postgres, Oracle) matches docs/drivers/Sql.md and the reserved MySql/Odbc names fail loudly at every construction site |
| 8 | Code organization & conventions | 1 finding (002) — a member the consumer needs and the seam does not carry |
| 9 | Testing coverage | No issues found for the offline surface (SqlServerDialectTests, PostgresDialectTests, OracleDialectTests pin quoting, markers, row-limit ends and the type maps). The catalog SQL itself is only ever parsed by a real server, which is what the Docker *.IntegrationTests exist for — the p_ bind prefix defect that shipped was invisible offline for exactly this reason |
| 10 | Documentation & comments | 1 finding (003) |
Findings
Driver.Sql.Dialect-001
| Field | Value |
|---|---|
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/PostgresDialect.cs:178, src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/OracleDialect.cs:212 |
| Status | Resolved |
Description: Both identifier ceilings are enforced as ident.Length, i.e. UTF-16 code units, but the server limits they stand in for are byte limits: PostgreSQL's NAMEDATALEN - 1 is 63 bytes and Oracle 12.2's is 128 bytes, both measured in the database's encoding (UTF-8 in any modern estate). A non-ASCII identifier therefore passes this check and is still truncated (Postgres) or rejected (Oracle) server-side — 40 Cyrillic characters are 80 UTF-8 bytes. Postgres' truncation is the failure this check documents itself as existing to prevent: "the server silently truncates anything longer, so a 70-character authored name would resolve to a different object than it spells". The residual risk is small because the catalog gate substitutes catalog spellings, and a name that came back from the catalog is by construction within the server's own limit — so this bites only the "future caller that has no catalog to check against" the same doc contemplates.
Recommendation: Measure the limit the way the server does — Encoding.UTF8.GetByteCount(ident) — for the two byte-limited dialects, and note in the remarks that T-SQL's sysname is genuinely a 128-character limit so SqlServerDialect is correct as written.
Resolution (2026-08-10): Fixed as recommended (74594e60). PostgresDialect.QuoteIdentifier and OracleDialect.QuoteIdentifier now measure Encoding.UTF8.GetByteCount(ident) against their ceilings and the rejection message says bytes (…may not exceed 63 bytes (got 64)); both constants' remarks state the unit and name the encoding. SqlServerDialect deliberately keeps ident.Length — sysname is nvarchar(128), a genuine UTF-16 code-unit limit — and that asymmetry is now pinned by a control test (QuoteIdentifier_countsCharacters_notBytes_becauseSysnameIsNvarchar128, a 128-character Cyrillic name that must still be accepted), so a future sweep applying the byte rule uniformly goes red. RED evidence: QuoteIdentifier_measuresTheCeilingInUtf8Bytes_notCharacters failed on both dialects before the change (32 Cyrillic characters = 64 UTF-8 bytes "should throw … but did not"), plus a supplementary-plane case (U+1D400 × 16 = 64 bytes, 32 code units).
Driver.Sql.Dialect-002
| Field | Value |
|---|---|
| Severity | Low |
| Category | Code organization & conventions |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/ISqlDialect.cs:29 |
| Status | Resolved |
Description: The seam models identifier quoting, catalog SQL, row-limit syntax, parameter spelling and command setup — everything that differs per backend except the per-command bind-parameter ceiling, which also differs per backend (2100 for Microsoft.Data.SqlClient, 1000 entries in an Oracle IN expression list, 65535 for Npgsql). Its absence is why SqlGroupPlanner cannot chunk a key-value IN list and a large EAV group fails permanently at poll time (Driver.Sql-002). The seam's own summary is the argument for putting it here: a call site that guesses a provider limit it does not own is the exact defect the interface exists to prevent.
Recommendation: Add a MaxBindParameters (or MaxInListEntries) member with each dialect stating its own value, and have the planner emit several plans per group key above it. Recorded rather than fixed because the consumer-side change is a planner restructure, not a seam edit — see Driver.Sql-002.
Resolution (2026-08-10): Fixed as recommended, together with its consumer (886ce1b8) — the two findings were one design, so the seam member and the planner chunking landed in the same commit. ISqlDialect.MaxBindParameters is documented as the ceiling on what a caller may emit, not the raw provider constant: SqlServerDialect reports 2098, not 2100, because Microsoft.Data.SqlClient routes a parameterized command through sp_executesql and spends two of the server's parameters on the statement text and the parameter-declaration list — a caller filling 2100 exactly would be rejected for a limit it appeared to respect. OracleDialect reports 1000, the IN-expression-list limit (ORA-01795), deliberately not Oracle's much larger 64k bind-variable count: the seam must report the limit the planner's IN (:p_k0, …) list actually hits, or chunking would produce a size the server still refuses. PostgresDialect reports 65535, the Bind message's 16-bit count. Each value is pinned by its own dialect test. The seam summary's "three things that differ per backend" is now four.
The two test dialects (SqliteDialect at 999 — SQLITE_MAX_VARIABLE_NUMBER's historical default, the value that always executes whatever native bundle ships — and the three decorators) implement the member too; Driver.Sql.Browser.Tests links SqliteDialect.cs and stayed green (79/79).
Driver.Sql.Dialect-003
| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/OracleDialect.cs:135 |
| Status | Resolved |
Description: OracleDialect.ListSchemasSql reads ALL_USERS, and its remarks justify the divergence from the other two dialects (which list only schemas that own a visible relation) purely from the catalog gate's point of view: "harmless here — the list is used to resolve an authored schema name … a schema holding no tables simply resolves and then matches no relation". That is true of the gate and false of the other consumer: SqlBrowseSession.RootAsync renders the same query as the browse tree's root level, so on an Oracle estate the operator is offered every database user as an expandable schema — hundreds of them on a real installation, nearly all empty, with no way to tell which hold anything. The two consumers of one query have different needs and only one is considered.
Recommendation: Either restrict the query to owners of a visible relation (SELECT DISTINCT owner FROM ALL_TABLES UNION SELECT DISTINCT owner FROM ALL_VIEWS), matching what the other two dialects list and what the browse tree wants, or state the browse consequence in the remarks so it is a known trade rather than an oversight. Note the change would also narrow the gate, which is a behaviour change for a tag naming an empty schema — currently it resolves and then fails at the table level; afterwards it would fail at the schema level with a different message.
Resolution (2026-08-10): Documentation fix — the recommendation's second option (401d6706). The claim was verified against the code first: SqlBrowseSession.RootAsync renders every row of ListSchemasSql as an expandable BrowseNodeKind.Folder with HasChildrenHint: true, so on Oracle every ALL_USERS row does become an offered schema. The remarks now split the two consumers explicitly, mark the browse cost with a ⚠️ as a known accepted trade, spell out the narrower ALL_TABLES/ALL_VIEWS query verbatim, and record why it was not taken here: it also narrows the catalog gate, which is a poll-path behaviour change validatable only against a real Oracle server (OracleReadTests) — the same blind spot that let the p_ bind-prefix defect ship. The note tells a future author to make the swap when an Oracle estate is available to gate it on, and to change both consumers' expectations in one commit.