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

# Code Review — Driver.Sql

<!-- Per-module findings file. See ../../REVIEW-PROCESS.md for the full process.
     The base README.md is generated from these files by regen-readme.py — do not
     edit README.md by hand. -->

| Field | Value |
|---|---|
| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql` |
| Reviewer | Claude Code |
| Review date | 2026-08-10 |
| Commit reviewed | `b990adae` |
| Status | Reviewed |
| Open findings | 7 |

## Checklist coverage

| # | Category | Result |
|---|---|---|
| 1 | Correctness & logic bugs | 3 findings (001 fixed, 005, 006) |
| 2 | OtOpcUa conventions | No issues found — read-only capability set, `DriverTypeNames.Sql` chained off the shared constant, logging through `ILogger` only, xUnit + Shouldly suites, no authorization decisions in driver code |
| 3 | Concurrency & thread safety | 1 finding (010). The atomic snapshot swap of the authored/polled tables, the `_gate`-bounded group concurrency and the slot-owned-by-the-work-task discipline are sound |
| 4 | Error handling & resilience | 2 findings (002, 004) |
| 5 | Security | 1 finding (009). The injection boundary itself is sound — see the note under Findings |
| 6 | Performance & resource management | No issues found — one connection per group bounded by `maxConcurrentGroups`, rows materialised before the reader is disposed, abandoned work detached and self-disposing |
| 7 | Design-document adherence | 1 finding (003) — the shipped getting-started guide disagreed with the parser |
| 8 | Code organization & conventions | 1 finding (007) |
| 9 | Testing coverage | No issues found. 365 tests across parser/planner/reader/gate/probe/factory over a real-SQLite poll fixture, with a colon-marker dialect as the provider-assumption control and an injection regression suite; the Postgres/Oracle/SQL Server/blackhole paths are covered by the Docker `*.IntegrationTests` |
| 10 | Documentation & comments | 2 findings (003 fixed, 008) |

## Findings

Security note (category 5), recorded here rather than as a finding because it is the module working as
designed: every authored *value* (`keyValue`, `whereValue`) reaches the database as a `DbParameter`, and
every authored *identifier* is resolved against the live catalog by `SqlCatalogGate` and **replaced with the
catalog's own spelling** before `ISqlDialect.QuoteIdentifier` sees it, so the identifier text in an emitted
statement is a string the server itself returned. `SqlPollReader` resolves only through `_polledByRawPath`,
which is populated exclusively by the gate and cleared by `BuildTagTable`, so there is no path from an
unvalidated identifier to a command text. The resolved connection string is passed to the provider and to
nothing else — every log line, health message and host status carries `Endpoint`'s credential-free
`server/database` rendering, and exception *messages* are reduced to the exception type on every operator
surface.

### Driver.Sql-001

| Field | Value |
|---|---|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlCatalogLoader.cs:139` |
| Status | Resolved |

**Description:** `ReadDefaultSchemaAsync`'s own remarks documented a fallback — "falling back to the single visible schema keeps the common one-schema database working" — that the method did not implement: a null/blank answer returned `string.Empty`. The default schema is then the empty string, `SqlCatalog.TryResolveSchema(null)` hands `""` to `TryResolveTable`, the loader issues the tables query for schema `''`, it matches nothing, and every unqualified authored table on the driver is rejected by the catalog gate and reads `BadNodeIdUnknown` — a whole-driver outage against a healthy database with every tag correctly authored. The blank answer is reachable in production: a SQL Server login with no `DEFAULT_SCHEMA` mapped, and a Postgres connection whose `search_path` resolves to nothing, both answer null. No test covered it, because the test dialect's `DefaultSchemaSql` is the literal `SELECT 'main'`.

**Recommendation:** Implement the documented fallback — when the dialect's answer is null/blank use the sole visible schema and only that one; with several visible schemas there is no defensible guess and the empty string must continue to resolve nothing.

**Resolution:** Fixed on this review branch (`review/sql` `69ad376f`, 2026-08-10). `LoadAsync` now passes the already-loaded schema list into `ReadDefaultSchemaAsync`, which falls back to `schemas[0]` when exactly one schema is visible. TDD: `SqlCatalogDefaultSchemaTests` was written red (the unqualified tag read `BadNodeIdUnknown`) and carries a falsifiability control proving the ambiguous multi-schema case still rejects rather than guessing.

---

### Driver.Sql-002

| Field | Value |
|---|---|
| Severity | Medium |
| Category | Error handling & resilience |
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlGroupPlanner.cs:127` |
| Status | Open |

**Description:** `BuildKeyValuePlan` emits one bind parameter per distinct `keyValue` in a group, with no upper bound and no chunking. Every provider caps this: `Microsoft.Data.SqlClient` allows 2100 parameters per command, and Oracle limits an `IN` expression list to 1000 entries. A key-value (EAV) staging table is precisely the shape that attracts thousands of tags on one table — which is one group by the group-key rule — so above the cap the provider rejects the statement after the connection opened, `QueryAsync` returns `GroupResult.Failed`, and every tag in that group publishes `BadCommunicationError` on every poll, permanently. The failure is also mis-diagnosed: `ObservePollOutcome` classifies `BadCommunicationError` as connection-class, so the driver reports Degraded and the host `Stopped` (see Driver.Sql-005), pointing the operator at the network rather than at a tag count. Nothing upstream caps the tag count per driver or per table, and the log line names the SQL but not the cause.

**Recommendation:** Chunk the `IN` list. The natural shape is a `MaxBindParameters` member on `ISqlDialect` (see Driver.Sql.Dialect-002) and a planner that emits several plans for one group key when the member count exceeds it — `SqlPollReader.Slice` already maps per `plan.Members`, so several plans sharing a group key need no reader change. Failing that, gate the tag count at deploy time with a message naming the provider limit, rather than letting it surface as a permanent per-poll communication error.

**Resolution:** _(open)_

---

### Driver.Sql-003

| Field | Value |
|---|---|
| Severity | Medium |
| Category | Design-document adherence |
| Location | `docs/drivers/Sql.md:55`, `docs/drivers/Sql.md:90` |
| Status | Resolved |

**Description:** Two claims in the shipped getting-started guide disagreed with the code. (1) Both `WideRow` examples spelled the row selector at the top level (`"whereColumn": …`, `"topByTimestamp": …`), but `SqlEquipmentTagParser.TryParse` reads those keys only from a nested `rowSelector` object — a blob authored from the doc carries no row selector, so the parser refuses the tag, the driver skips it, and it reads `BadNodeIdUnknown` forever. This is the same defect class as Gitea #552, arriving through the documentation rather than through the browse-commit mapper. (2) The "Optional per-tag fields" table documented a `pollIntervalMs` per-tag override; no such field exists anywhere in the tree — `SqlTagDefinition` has no member for it and nothing reads the key, so an operator authoring it silently gets the instance default.

**Recommendation:** Correct both: nest the selector keys inside `rowSelector` in both examples with a warning that the top-level spelling is refused, and delete the `pollIntervalMs` row, stating where the interval actually comes from.

**Resolution:** Fixed on this review branch (`review/sql` `69ad376f`, 2026-08-10). Both examples now use the nested `rowSelector` form and carry an explicit warning about the top-level spelling; the `pollIntervalMs` row is replaced by a paragraph naming the subscriber interval / `defaultPollInterval` / `PollGroupEngine.DefaultMinInterval` chain and recording that the field never existed. The scalar/string asymmetry between `whereValue` and `keyValue` (Driver.Sql.Contracts-001) is documented in the same edit.

---

### Driver.Sql-004

| Field | Value |
|---|---|
| Severity | Low |
| Category | Error handling & resilience |
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs:651` |
| Status | Open |

**Description:** `HandlePollError` skips the connection class by testing `ex is DbException`, on the stated premise that "a subscribed-read outage surfaces as a DbException the reader threw, which ReadAsync has ALREADY classified". Not every unreachable-database throw is a `DbException`: `SqlPollReader.QueryAsync` raises `InvalidOperationException` when the provider factory returns no connection, and a provider may raise `ArgumentException`/`FormatException` from a malformed connection string on the same open path. For those, `ReadAsync` writes the good endpoint-bearing `LastError` and transitions the host to `Stopped`, rethrows, and the poll engine then routes the same exception here — where `Degrade` overwrites that message with the guidance-free "poll failed: TypeName" and the outage is logged a second time. The comment describes the intended behaviour; the type test does not implement it.

**Recommendation:** Mark the exception as already-classified at the one place that knows (e.g. `ReadAsync` wrapping the throw, or a per-call flag) rather than inferring it from the exception type, so the reader's richer `LastError` survives.

**Resolution:** _(open)_

---

### Driver.Sql-005

| Field | Value |
|---|---|
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs:641` |
| Status | Open |

**Description:** `IsConnectionClass` treats `BadCommunicationError` as "the database did not answer", so a poll carrying it reports the endpoint `Stopped`. But that code is minted by `GroupResult.Failed`, which the reader raises only after the connection has opened — its own comment says "the connection opened, so the server is up — this query is what failed (an identifier that is not in the catalog, a lock, a permission)". A single group failing on a lock, a revoked `SELECT`, a schema change or an over-long `IN` list (see Driver.Sql-002) therefore reports the whole database endpoint as down. The two types disagree about what the same status code means.

**Recommendation:** Either restrict the connection class to `BadTimeout` (a wedged socket is the case the host-state signal is for), or give a post-open query failure its own status so the two classifications cannot drift. Note the host state is currently only consumed through `IHostConnectivityProbe`, which is unwired (Gitea #521) — the operator-visible half today is the `LastError` wording and the Degraded verdict.

**Resolution:** _(open)_

---

### Driver.Sql-006

| Field | Value |
|---|---|
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs:599` |
| Status | Open |

**Description:** `ObservePollOutcome` advances `LastSuccessfulRead` only when at least one snapshot is Good. Several codes are proof the database answered without being Good: `BadNoData` (the query ran, the row is absent), `Uncertain`/`Bad` (a present row with a NULL cell) and `BadTypeMismatch` (a cell came back and would not coerce). A driver whose every tag reads one of those — a truncated staging table, a key set that was deleted — leaves `LastSuccessfulRead` frozen. That is benign on its own (health is correctly left unchanged, because these are authoring/data facts), but it interacts with the comms-loss watchdog: once anything has degraded the driver, the conjunction "non-Healthy AND `LastSuccessfulRead` frozen for 90 s" (CLAUDE.md, Gitea #527) trips against a database that is answering every poll on time, and the node force-reconnects a healthy connection.

**Recommendation:** Advance `LastSuccessfulRead` on evidence-of-answer codes, not only on Good — the honest reading of "the database answered" the watchdog was designed around — while leaving the health state classification exactly as it is.

**Resolution:** _(open)_

---

### Driver.Sql-007

| Field | Value |
|---|---|
| Severity | Low |
| Category | Code organization & conventions |
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/ZB.MOM.WW.OtOpcUa.Driver.Sql.csproj:21` |
| Status | Resolved |

**Description:** The project carried a direct `Microsoft.Data.SqlClient` `PackageReference` that no source file in it uses — every type speaks `System.Data.Common` only, and the concrete provider packages moved behind the `.Dialect` leaf when `ISqlDialect`/`SqlServerDialect` were extracted (Gitea #526). Beyond the dead dependency, it reads as "this project is SQL-Server-specific", which is the assumption the dialect seam exists to prevent.

**Recommendation:** Drop the reference (the `.Dialect` project reference already supplies it transitively for anything that needs it) and record why the project deliberately carries no provider package.

**Resolution:** Fixed on this review branch (`review/sql` `69ad376f`, 2026-08-10). Reference removed and replaced with a comment stating the rule; the module's 365 tests still pass.

---

### Driver.Sql-008

| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs:556` |
| Status | Open |

**Description:** `SelectWideRow`'s remarks justify giving the `topByTimestamp` form the dialect's single-row limit on the grounds that "there the `ORDER BY` makes 'the first row' a defined answer". That holds only when the ordering column is unique. `ORDER BY ts DESC` plus a one-row limit over a column with ties returns an arbitrary row among the tied ones, and — unlike the where-pair form, whose whole argument for not taking the limit is that `Rows.Count > 1` is the evidence of ambiguity — the reader can never detect it, because the server has already discarded the other rows. The browse-commit default makes the tie shape reachable rather than theoretical: `SqlBrowseSession.PreferredTimestampColumn` will happily nominate a `date`-typed column (see Driver.Sql.Browser-001), which ties for every row written on the same day.

**Recommendation:** State the tie caveat where the claim is made, and in `docs/drivers/Sql.md` alongside the `topByTimestamp` example: the model requires a selector that identifies one row, and a non-unique ordering column silently does not.

**Resolution:** _(open)_

---

### Driver.Sql-009

| Field | Value |
|---|---|
| Severity | Low |
| Category | Security |
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverProbe.cs:83` |
| Status | Open |

**Description:** The probe surfaces the raw parser text on a malformed config ("Config JSON is invalid: " + `ex.Message`) where `SqlDriverBrowser.ParseDocument` deliberately withholds the identical message "because the malformed text may carry a connection string". The two read the same blob shape: the AdminUI Sql picker composes an ad-hoc document carrying a top-level `connectionString` literal, so a malformed paste is exactly the case that reaches both. `System.Text.Json`'s message quotes the offending token and the JSON path rather than the property value, so the current exposure is small — but the rule is stated absolutely elsewhere in this driver ("never `ex.Message` on an operator surface") and the probe is the one place that breaks it.

**Recommendation:** Match the browser: report the failure class, not the parser's message.

**Resolution:** _(open)_

---

### Driver.Sql-010

| Field | Value |
|---|---|
| Severity | Low |
| Category | Concurrency & thread safety |
| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs:675` |
| Status | Open |

**Description:** Every health transition is a read-modify-write over a `Volatile`-accessed field with no compare-and-swap: `Degrade`, `TouchLastSuccessfulRead` and `SetHealthUnlessFaulted` each `ReadHealth()`, build a new record and `WriteHealth()` it. Concurrent poll loops (one per subscription, plus an ad-hoc `ReadAsync`) can interleave between the read and the write, so a `LastSuccessfulRead` advance can be lost, or a `Faulted` verdict recorded by a concurrent reinitialize can be overwritten by a stale non-Faulted snapshot despite the guard — the guard reads the field before the write rather than atomically with it. Impact is limited to the health surface (a stale timestamp or state until the next poll), which is why this is Low.

**Recommendation:** Use `Interlocked.CompareExchange` in a small retry loop for the read-modify-write helpers, or funnel health mutation through a lock — the `_probeLock` the host-state transition already uses would do.

**Resolution:** _(open)_