docs(driver-expansion): resolve all open design concerns from the review pass
Every concern the 7-agent review parked is now decided and integrated: - universal browser: in-flight capture coalescing keyed (driverType, config-hash) + global cap of 4 concurrent captures; CanBrowse catches a throwing TryCreate and defensively shuts down the throwaway instance; cleanup ShutdownAsync bounded at 10s (R2-01); picker Refresh = close + re-capture. Coalesced sessions share an immutable tree, so DisposeAsync drops the session's reference, not the tree. - mtconnect: UNAVAILABLE pinned to BadNoCommunication (fleet's BadCommunicationError stays reserved for the driver's own transport failures); TIME_SERIES sampleCount flows into ArrayDim; Subscribe timeout bounds only the stream-start handshake; library version/TFM folded into the license checklist. - mqtt: browse rebirth is an explicit DriverOperator-gated "Request rebirth" button (RequestRebirthAsync(scope)) - never fired by open/root/expand; hand-rolled reconnect loop committed for P1 (MQTTnet v5 dropped ManagedMqttClient); Wave-2-start library re-verification checkbox; implement from the Sparkplug v3.0 spec text. - bacnet: P1 opens with a first-spike checklist (package TFM, BBMD/ segmented-RPM/COV API smoke, unicast-I-Am fixture behavior); AdminUI-node browse registers a second foreign device - BBMD FD-table sizing note. - sql-poll: split-node browse needs env parity for Sql__ConnectionStrings__ refs on admin nodes (actionable error + session-only pasted literal); one-row-per-key query contract (last-wins + rate-limited warning); absent key -> BadNoData; operationTimeout > commandTimeout authoring rule; Browser->Driver.Sql SqlClient transitive on AdminUI accepted on record. - omron: FINS framer gated on W227 + live golden vectors; CIP string layout is the first P1 live-gate item; AbCip harness static-init anti-pattern note; writable-defaults-true operator warning. - modbus-rtu: first P1 step confirms pymodbus exposes framer=rtu on a TCP server (custom-script fallback otherwise). - program doc: new cross-cutting rule - driver ctors must be connection-free (the universal browser's CanBrowse throwaway instances depend on it).
This commit is contained in:
Joseph Doherty
@@ -62,7 +62,7 @@ All `net10.0`, `Nullable`/`ImplicitUsings` enabled, `TreatWarningsAsErrors`, mat
|
||||
|---|---|
|
||||
| `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts/` | Options DTOs, `SqlProvider` enum, tag-model records, `SqlEquipmentTagParser`. **Zero transport deps** (refs only `Core.Abstractions`) so AdminUI + browser can reference it without dragging `Microsoft.Data.SqlClient`. |
|
||||
| `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/` | `SqlDriver` (runtime capabilities), `ISqlDialect` + `SqlServerDialect`, `SqlPollReader`, factory extensions. **Owns the `Microsoft.Data.SqlClient` package ref.** |
|
||||
| `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/` | `SqlDriverBrowser : IDriverBrowser`, `SqlBrowseSession : IBrowseSession` (schema walk). Refs `Commons` + `Sql.Contracts` + **`Driver.Sql`** (for `ISqlDialect`/`SqlServerDialect` — the dialect's catalog SQL *is* the browse engine, so it must be shared, not duplicated; `Microsoft.Data.SqlClient` comes transitively). This runtime-project ref is a deliberate deviation from `OpcUaClient.Browser` (which refs only its Contracts and owns the OPC UA client packages itself): the browser still opens its own transient connection. |
|
||||
| `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/` | `SqlDriverBrowser : IDriverBrowser`, `SqlBrowseSession : IBrowseSession` (schema walk). Refs `Commons` + `Sql.Contracts` + **`Driver.Sql`** (for `ISqlDialect`/`SqlServerDialect` — the dialect's catalog SQL *is* the browse engine, so it must be shared, not duplicated; `Microsoft.Data.SqlClient` comes transitively). This runtime-project ref is a deliberate deviation from `OpcUaClient.Browser` (which refs only its Contracts and owns the OPC UA client packages itself): the browser still opens its own transient connection. The resulting **transitive `Microsoft.Data.SqlClient` reference on AdminUI is reviewed and accepted** — AdminUI already uses SqlClient for ConfigDb, so this adds no new package to its closure. Recorded here deliberately so nobody "fixes" the deviation later without this context. |
|
||||
| `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/` | Unit suite (SQLite fixture, §9). |
|
||||
| `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.IntegrationTests/` | Env-gated integration suite (central SQL Server, §9). |
|
||||
| `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/` | Browser unit tests (SQLite `PRAGMA` dialect). |
|
||||
@@ -257,6 +257,16 @@ Three models; **(a) and (b) ship in P1**, (c) is P3.
|
||||
per poll; index rows by key, slice back. Values in the `IN` list are **bound parameters**, one per
|
||||
key — never interpolated.
|
||||
|
||||
**Query contract — one row per key (v1).** The `WHERE <keyColumn> IN (…)` poll assumes the authored
|
||||
source yields **exactly one row per key**. Point the tag at a current-values table, or — for
|
||||
append-only staging/history tables that hold many rows per key — at a **view or query the operator
|
||||
writes** that reduces to latest-per-key (`GROUP BY key` + `MAX(timestamp)`, or
|
||||
`ROW_NUMBER() OVER (PARTITION BY <keyColumn> ORDER BY <timestampColumn> DESC) = 1`); that
|
||||
operator-authored latest-per-key view is the **supported workaround** for append-only sources. If a
|
||||
poll nonetheless returns multiple rows for a key, the driver deterministically takes the **last**
|
||||
occurrence in reader order and emits a **rate-limited warning** log naming the offending key — the
|
||||
value stays usable, but the config is telling you the source violates the contract.
|
||||
|
||||
**(b) Wide row** *(cheap).* One "latest status" row, many columns → many tags. A tag names a `table`,
|
||||
a `columnName`, and a `rowSelector` (`{whereColumn, whereValue}` or `{topByTimestamp: <col>}`).
|
||||
**GroupKey = `(table, rowSelector)`.** All wide-row tags on the same row fold into one
|
||||
@@ -297,7 +307,9 @@ the address-space layer maps to OPC UA built-ins.
|
||||
**override** the inferred type explicitly (`"type": "Int32"`) — the operator sometimes knows better
|
||||
than column metadata (e.g. a `varchar` column holding a number); the driver then coerces the read
|
||||
cell to the declared type. `NULL` → `DataValueSnapshot { Value = null }` with `Good`/`Uncertain` per
|
||||
a `nullIsBad` option (default: treat as `Uncertain`, not `Bad`).
|
||||
a `nullIsBad` option (default: treat as `Uncertain`, not `Bad`). A key **entirely absent** from the
|
||||
result set (row deleted) is distinct from a NULL cell: that tag's snapshot goes **`BadNoData`** —
|
||||
`nullIsBad` governs only a present row whose value cell is NULL.
|
||||
|
||||
**Source timestamp:** if a tag declares `timestampColumn`, that column's value becomes
|
||||
`DataValueSnapshot.SourceTimestampUtc`; otherwise the poll-read wall-clock is used for both source and
|
||||
@@ -376,7 +388,7 @@ vs wide-row) + fills the key/row selector. `SecurityClass = ViewOnly` in read-on
|
||||
|
||||
| Path | Purpose |
|
||||
|---|---|
|
||||
| `Driver.Sql.Browser/SqlDriverBrowser.cs` | `IDriverBrowser`, `DriverType => "Sql"`; `OpenAsync(configJson, ct)` deserializes options, resolves the connection string from the form JSON (same trust boundary as `TestDriverConnect` + the OpcUaClient browser — transient, never cached), opens a `DbConnection`, returns a `SqlBrowseSession`. |
|
||||
| `Driver.Sql.Browser/SqlDriverBrowser.cs` | `IDriverBrowser`, `DriverType => "Sql"`; `OpenAsync(configJson, ct)` deserializes options and resolves the connection string from the `connectionStringRef` in the form JSON via a **direct env-var read** (`Sql__ConnectionStrings__<ref>`) **in the AdminUI process** — the same resolution the driver performs at Initialize (§8.2). If the ref doesn't resolve, the open **fails with an actionable message naming the exact missing env var** (e.g. `Connection string ref 'MesStaging' not found — set env var Sql__ConnectionStrings__MesStaging on this admin node`). The picker form **additionally accepts a pasted literal connection string** for ad-hoc browse — used for that browse session only, **never persisted** into the driver config (same trust boundary as `TestDriverConnect` + the OpcUaClient browser — transient, never cached). Opens a `DbConnection`, returns a `SqlBrowseSession`. |
|
||||
| `Driver.Sql.Browser/SqlBrowseSession.cs` | `IBrowseSession`; `RootAsync`/`ExpandAsync`/`AttributesAsync` over the dialect catalog queries; a `SemaphoreSlim _gate` (one ADO.NET connection is not concurrent). Per-call work is bounded by the AdminUI's existing 20 s per-call timeout. |
|
||||
| AdminUI DI (`EndpointRouteBuilderExtensions.cs`, beside `:49-50`) | `services.AddSingleton<IDriverBrowser, SqlDriverBrowser>();` |
|
||||
| `AdminUI/Uns/TagEditors/TagConfigEditorMap.cs` | `["Sql"] = typeof(Components.Shared.Uns.TagEditors.SqlTagConfigEditor)` (§6). |
|
||||
@@ -386,6 +398,15 @@ The browser reuses the AdminUI `BrowseSessionRegistry` + reaper + 2-min idle TTL
|
||||
unchanged. **The universal browser DI line is NOT added for `Sql`** — its `CanBrowse` is false anyway,
|
||||
and the bespoke registration wins.
|
||||
|
||||
**Deployment requirement — env parity across split nodes.** Because the browser resolves
|
||||
`connectionStringRef` in the **AdminUI process**, on split-role deployments the **admin nodes must
|
||||
carry the same `Sql__ConnectionStrings__<ref>` env vars as the driver nodes** for every ref the
|
||||
operator wants browseable (the same split-role lesson as driver probes on admin nodes). A ref present
|
||||
only on driver nodes still *polls* fine at runtime but cannot be *browsed*; when that happens the
|
||||
browser-open failure names the exact missing env var so the operator knows precisely what to add to
|
||||
the admin node's environment — or falls back to pasting a literal connection string into the picker
|
||||
for a one-off session-only browse.
|
||||
|
||||
---
|
||||
|
||||
## 5. TagConfig + driver-config JSON
|
||||
@@ -398,7 +419,7 @@ and the bespoke registration wins.
|
||||
// Connection string is resolved at Initialize from an env/secret ref — NOT stored here.
|
||||
"connectionStringRef": "MesStaging", // => env Sql__ConnectionStrings__MesStaging
|
||||
"defaultPollInterval": "00:00:05",
|
||||
"operationTimeout": "00:00:15", // per-query wall-clock deadline (→ BadTimeout)
|
||||
"operationTimeout": "00:00:15", // per-query wall-clock deadline (→ BadTimeout); MUST be > commandTimeout (§8.3)
|
||||
"commandTimeout": "00:00:10", // server-side CommandTimeout backstop (seconds granularity)
|
||||
"maxConcurrentGroups": 4, // cap on concurrent group queries (pool guard)
|
||||
"nullIsBad": false, // NULL cell → Uncertain (default) vs Bad
|
||||
@@ -436,7 +457,8 @@ and the bespoke registration wins.
|
||||
```
|
||||
|
||||
GroupKey for batching = `("dbo.TagValues", "tag_name", "num_value", "sample_ts")`; all such tags fold
|
||||
into one `… WHERE tag_name IN (@k0,@k1,…)` per poll.
|
||||
into one `… WHERE tag_name IN (@k0,@k1,…)` per poll. The `table` must yield one row per `keyValue` —
|
||||
for append-only sources, author a latest-per-key view and point `table` at it (§3.6 query contract).
|
||||
|
||||
### 5.3 Per-tag `TagConfig` — wide-row model
|
||||
|
||||
@@ -485,7 +507,9 @@ its raw `TagConfig` JSON, parsed once and cached) — same as Modbus.
|
||||
## 6. Typed editor + validator + the enum-serialization trap
|
||||
|
||||
Add `["Sql"] = typeof(…SqlTagConfigEditor)` to `TagConfigEditorMap` and a matching entry to
|
||||
`TagConfigValidator`. The editor is a thin Razor shell over a pure `SqlTagConfigModel`
|
||||
`TagConfigValidator`. The validation layer also enforces the driver-level timeout-ordering rule
|
||||
(§8.3): `operationTimeout` must be strictly greater than `commandTimeout`, rejected (or warned on)
|
||||
at authoring time. The editor is a thin Razor shell over a pure `SqlTagConfigModel`
|
||||
(`FromJson`/`ToJson`/`Validate`, preserves unknown keys) — copy the Modbus template under
|
||||
`Components/Shared/Uns/TagEditors/` + `Uns/TagEditors/`. The model exposes: `Model` (KeyValue/WideRow/
|
||||
Query), `Table`, `KeyColumn`/`KeyValue`/`ValueColumn`/`TimestampColumn` (key-value), `ColumnName` +
|
||||
@@ -588,9 +612,14 @@ comment) and the env-overridable ConfigDb connection string:
|
||||
- **Never log the resolved connection string.** Log only provider + server host + database name.
|
||||
- Prefer integrated/managed auth where the estate supports it (`Integrated Security=true` / Azure AD /
|
||||
Kerberos) so no password transits config at all.
|
||||
- The `.Browser` path receives the secret in form JSON over the authenticated Blazor circuit (same
|
||||
boundary as every other driver browser + `TestDriverConnect`); it builds one transient connection
|
||||
and releases it — **no `_lastConfigJson` cached field anywhere.**
|
||||
- The `.Browser` path resolves `connectionStringRef` via the **same direct env read, in the AdminUI
|
||||
process** (§4.4) — which is why split-role deployments need **env parity**: admin nodes carry the
|
||||
same `Sql__ConnectionStrings__<ref>` vars as driver nodes for any browseable ref, and an unresolved
|
||||
ref fails the browse open with a message naming the exact missing env var. A pasted ad-hoc literal
|
||||
connection string travels only over the authenticated Blazor circuit (same boundary as every other
|
||||
driver browser + `TestDriverConnect`), is used for that browse session only, and is never persisted
|
||||
into the driver config or logged. Either way the browser builds one transient connection and
|
||||
releases it — **no `_lastConfigJson` cached field anywhere.**
|
||||
|
||||
### 8.3 Timeout / frozen-peer (high, risk #3)
|
||||
|
||||
@@ -599,6 +628,14 @@ Per §3.2: `CommandTimeout` (server-side backstop) **and** `ExecuteReaderAsync(l
|
||||
R2-01 S7 blackhole finding. A frozen DB surfaces `BadTimeout` per group + degrades the driver + backs
|
||||
off the poll loop; it must never wedge a poll thread. Asserted by the blackhole live-gate (§9).
|
||||
|
||||
**Timeout-ordering rule (authoring validation).** `operationTimeout` **must be strictly greater than**
|
||||
`commandTimeout` — the defaults (15 s > 10 s) encode this deliberately. The driver-page /
|
||||
`TagConfigValidator`-level check **rejects** (or at minimum warns loudly on)
|
||||
`operationTimeout <= commandTimeout`: inverting the order means the client-side abort always fires
|
||||
first and **masks the server-side backstop** — every server-side timeout would surface as a generic
|
||||
client cancellation instead of the `CommandTimeout` failure it actually is, hiding the diagnostic
|
||||
that distinguishes "slow query/lock wait" from "driver gave up".
|
||||
|
||||
### 8.4 Connection pooling / exhaustion (medium)
|
||||
|
||||
Open-use-dispose per poll (`await using` on both `DbConnection` and `DbDataReader` — never leak a
|
||||
|
||||
Reference in New Issue
Block a user