From 256d943f1e0c005cc6c5be91678147894bc6970f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 06:58:41 -0400 Subject: [PATCH] review(Driver.Sql.Dialect): 3 Low findings, all record-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 001 the Postgres (63) and Oracle (128) identifier ceilings are BYTE limits but are enforced as UTF-16 char counts, so a non-ASCII name passes the check and is still truncated server-side — the exact silent truncation the check documents itself as preventing; 002 the seam carries no max-bind-parameter member, which is why the planner cannot chunk a key-value IN list (Driver.Sql-002); 003 Oracle's ListSchemasSql reads ALL_USERS, justified only from the catalog gate's point of view — the browse tree shares the query and renders every database user as an expandable schema. Quoting, marker/bind-name split, BindByName and the type maps are all sound. --- code-reviews/Driver.Sql.Dialect/findings.md | 80 +++++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 code-reviews/Driver.Sql.Dialect/findings.md diff --git a/code-reviews/Driver.Sql.Dialect/findings.md b/code-reviews/Driver.Sql.Dialect/findings.md new file mode 100644 index 00000000..c28937f5 --- /dev/null +++ b/code-reviews/Driver.Sql.Dialect/findings.md @@ -0,0 +1,80 @@ +# 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 | 3 | + +## 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 | Open | + +**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:** _(open)_ + +--- + +### 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 | Open | + +**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:** _(open)_ + +--- + +### 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 | Open | + +**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:** _(open)_