# Code Review — Driver.Sql | 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 | 0 | ## 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 | Resolved | **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 (2026-08-10):** Fixed the recommended way, in the same commit as its seam half Driver.Sql.Dialect-002 (`886ce1b8`) — the deploy-time tag-count gate was rejected as refusing a configuration the driver can serve perfectly well. `SqlGroupPlanner.AddKeyValuePlans` splits a key-value group's distinct keys into chunks of `dialect.MaxBindParameters` and emits one plan per chunk, all carrying that group's key. Three decisions worth knowing: - **Members follow their key, never their position.** A member placed in a chunk that does not bind its own key would read a result set that cannot contain its row and publish `BadNoData` forever — a quieter failure than the one being fixed. Two tags on one `keyValue` therefore land in the same chunk (one bound parameter, both fed), pinned by `MembersSharingAKeyValue_stayInTheSameChunk` and `EveryMemberLandsInTheChunkThatBindsItsOwnKey`. - **`SqlPollReader` is untouched**, as the recommendation predicted: it runs one command per plan and slices each plan's own `Members` back. That claim is not asserted by reading — `ThePollReader_readsEveryTagCorrectly_whenOneGroupIsSplitAcrossSeveralPlans` drives the real read path over the SQLite fixture at a ceiling of **1**, so three keys become three commands, and every tag still gets its own value (and the absent key still `BadNoData`). - **The load-bearing test is the falsifiability control**, `AGroupAtOrUnderTheCeiling_stillEmitsExactlyOnePlan` (1/2/3 keys under a ceiling of 3): a planner that split eagerly, or off a stale count, passes every "it splits" assertion above while silently turning one round-trip per source into N. A dialect reporting a ceiling below 1 is rejected loudly rather than clamped — chunking to zero loops forever, and a one-per-chunk fallback would hide a broken dialect behind a poll that suddenly costs a round-trip per tag. RED evidence: with the shipped `OracleDialect` (1000-entry `IN` limit) and 1001 distinct keys, `plans.Count should be 2 but was 1` before the change; green after. The mis-diagnosis this finding also describes — `BadCommunicationError` classified as connection-class — belongs to Driver.Sql-005 and is untouched here; chunking removes the input that produced it on this path. --- ### 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 | Resolved | **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 (2026-08-10):** Fixed — `90f3016a`. `ReadAsync`'s catch now records the exception *instance* in a `_classifiedByRead` field before rethrowing, and `HandlePollError` defers only when `Interlocked.CompareExchange` matches that same object (consume-once); the `ex is DbException` type test is gone. A field rather than `Exception.Data` or a wrapper: `Data` is provider-owned and may be read-only, and wrapping would change the type `IReadable`'s callers and the poll engine see. TDD: `HandlePollError_forANonDbExceptionReadAsyncClassified_stillDefers_soTheGoodLastErrorSurvives` drives a `BreakableFactory` that hands back no connection (the reader's `InvalidOperationException` path) and was RED — the endpoint-bearing `LastError` was overwritten with "poll failed: InvalidOperationException". Its falsifiability control, `HandlePollError_forAnExceptionReadAsyncNeverSaw_degrades_evenWhenItIsADbException`, was RED against the type test too (a `DbException` the engine raised was silently swallowed), and pins that the marker is per-instance rather than per-type. The pre-existing DbException test was rewritten to drive the real ReadAsync→HandlePollError round trip instead of handing `HandlePollError` a bare exception. --- ### Driver.Sql-005 | Field | Value | |---|---| | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs:641` | | Status | Resolved | **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 (2026-08-10):** Fixed — `ee1f1b28`. `IsConnectionClass` is now `BadTimeout` only, and `BadCommunicationError` moves to a new `IsQueryFailureClass` with its own branch in `ObservePollOutcome`: still Degraded (every tag in that group is Bad), but the host stays **Running** and the `LastError` points at the table/column, the grant and any lock rather than at the network. The published status code is unchanged — the drift was in what the driver *read into* it, not in what the reader emits, so the wire behaviour and the `GroupResult.Failed` contract stay put. The first recommendation was rejected on its own: restricting the class without adding the branch would have left an all-groups-failing driver reporting Healthy. TDD: `ReadAsync_whenTheQueryFailsAfterTheConnectionOpened_degradesButLeavesTheEndpointUp` drops the fixture's table out from under a driver whose catalog gate has already passed, and was RED (`LastError` blamed communication; the host went `Stopped`). `ReadAsync_whenEveryTagTimesOut_degradesHealthAndReportsTheHostStopped` is the standing control that a genuine no-answer outage still reports `Stopped`. --- ### Driver.Sql-006 | Field | Value | |---|---| | Severity | Low | | Category | Correctness & logic bugs | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs:599` | | Status | Resolved | **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 (2026-08-10):** Fixed — `a0a49d33`. A named `IsAnswerEvidence` predicate now decides the clock, separately from the health verdict: `Good`, `BadNoData`, `Uncertain`, `Bad`, `BadTypeMismatch` and `BadCommunicationError` (post-Driver.Sql-005 that one means the connection opened and the server refused the query — so the transport works and a forced reconnect could not fix it). Deliberately excluded, and each pinned by a control: `BadTimeout` (the database did not answer) and `BadNodeIdUnknown` / `BadConfigurationError` / `BadInternalError` (minted before any statement was executed — a blanket "not a timeout" rule would keep the clock fresh on a driver that never reached the database, disabling the watchdog rather than fixing it). The health *state* classification is untouched: an answered-but-not-Good poll advances the clock and reports the host Running, and changes nothing else. TDD: `ReadAsync_whenTheDatabaseAnswersWithNoRow_advancesTheEvidenceClock_withoutChangingHealth` (degrade first, then read a deleted key) was RED with a frozen clock, as were four of the six `ObservePollOutcome_everyEvidenceOfAnswerCode_advancesTheEvidenceClock` theory cases; the two negative theories and `ReadAsync_anUnresolvableRef_isNoEvidenceTheDatabaseAnswered_soTheEvidenceClockStands` were green throughout and are the falsifiability controls. --- ### 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 | Resolved | **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 (2026-08-10):** Fixed — `9d4760fd`, **documentation only**. `SelectWideRow`'s over-claim ("the `ORDER BY` makes 'the first row' a defined answer") now reads that the `ORDER BY` fixes only *which* rows can be first, followed by an explicit caveat: with a non-unique ordering column the server returns an arbitrary row among the tied ones and this method can never detect it, because the discarded rows never reach it — there is no `Rows.Count > 1` left to warn on, unlike the where-pair form. `docs/drivers/Sql.md` carries the same warning beside the `topByTimestamp` example, naming the reachable shape (a `date`-typed column ties for every row written on the same day, and browse-commit will nominate one) and the fix (a column unique per row, or a latest-row view). Behaviour deliberately unchanged: the entry argues for the doc, and the only behavioural remedy — a tie-break column in the emitted `ORDER BY` — lives in `SqlGroupPlanner`, which is out of scope here and would need an authored notion of what breaks the tie. --- ### Driver.Sql-009 | Field | Value | |---|---| | Severity | Low | | Category | Security | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverProbe.cs:83` | | Status | Resolved | **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 (2026-08-10):** Fixed in `073930e0`. The `JsonException`/`ArgumentNullException` catch now returns the browser's own wording — "Config JSON is not valid JSON. (The parser's message is withheld because the malformed text may carry a connection string.)" — and the class summary states the rule beside the existing "never the connection string" paragraph, so the two disciplines read as one. Pinned by `SqlDriverProbeTests.Probe_malformedConfig_withholdsTheParserMessage`, whose malformed input is a `connectionString` literal mis-quoted the way an operator's paste is (`Password=Sup3r;SecretTail`, unquoted `;`) so the parse fails partway *through* the credential. RED before the fix on `"LineNumber"` — the old message was `Config JSON is invalid: 'P' is an invalid start of a value. Path: $.connectionString | LineNumber: 0…`. The test asserts on the parser's structural tokens (`LineNumber`, `BytePositionInLine`) rather than only on the secret, because `System.Text.Json` quotes just the offending token: a leak-only assertion passes vacuously today and would go on passing if the message ever got *more* verbose. --- ### Driver.Sql-010 | Field | Value | |---|---| | Severity | Low | | Category | Concurrency & thread safety | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs:675` | | Status | Resolved | **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 (2026-08-10):** Fixed — `336e79ea`. Every poll-driven transition now goes through one `UpdateHealth(Func)` compare-and-swap retry loop: `Degrade` and the new `RecordHealthyRead` via `UpdateHealthUnlessFaulted` (so the Faulted guard is evaluated against the *same* snapshot the CAS swaps out, closing the "guard reads before the write" half), `TouchLastSuccessfulRead` directly. Two details are load-bearing. The transform is re-run per attempt and must derive its timestamp from the clock rather than from the call site — the Healthy write previously computed `DateTime.UtcNow` before the call, which a retry would republish *stale* and rewind the clock; that is why the Healthy path became `RecordHealthyRead` instead of passing a prebuilt record. And `WriteHealth` survives, unconditional, for the Initialize/Shutdown lifecycle writes that are last-writer-wins by definition — a Volatile write racing a CAS loop only makes the loop retry and re-read, which is strictly safer than before. TDD: `HealthTransitions_fromConcurrentPolls_neverRewindTheEvidenceClock` runs an advancing and a degrading `ObservePollOutcome` loop against one driver while an observer watches `LastSuccessfulRead`; monotonicity is the observable a lost update violates (a `Degrade` that read the clock at t0 republishes it after an advance to t1). RED 3/3 runs before the fix, green 3/3 after.