Joseph Doherty
26 KiB
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 | 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 | 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
BadNoDataforever — a quieter failure than the one being fixed. Two tags on onekeyValuetherefore land in the same chunk (one bound parameter, both fed), pinned byMembersSharingAKeyValue_stayInTheSameChunkandEveryMemberLandsInTheChunkThatBindsItsOwnKey. SqlPollReaderis untouched, as the recommendation predicted: it runs one command per plan and slices each plan's ownMembersback. That claim is not asserted by reading —ThePollReader_readsEveryTagCorrectly_whenOneGroupIsSplitAcrossSeveralPlansdrives 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 stillBadNoData).- 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<DriverHealth, DriverHealth?>) 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.