diff --git a/code-reviews/Driver.Sql.Dialect/findings.md b/code-reviews/Driver.Sql.Dialect/findings.md index c28937f5..b34cf50c 100644 --- a/code-reviews/Driver.Sql.Dialect/findings.md +++ b/code-reviews/Driver.Sql.Dialect/findings.md @@ -11,7 +11,7 @@ | Review date | 2026-08-10 | | Commit reviewed | `b990adae` | | Status | Reviewed | -| Open findings | 3 | +| Open findings | 0 | ## Checklist coverage @@ -37,13 +37,13 @@ | 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 | +| Status | Resolved | **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)_ +**Resolution (2026-08-10):** Fixed as recommended (`74594e60`). `PostgresDialect.QuoteIdentifier` and `OracleDialect.QuoteIdentifier` now measure `Encoding.UTF8.GetByteCount(ident)` against their ceilings and the rejection message says *bytes* (`…may not exceed 63 bytes (got 64)`); both constants' remarks state the unit and name the encoding. `SqlServerDialect` deliberately keeps `ident.Length` — `sysname` is `nvarchar(128)`, a genuine UTF-16 code-unit limit — and that asymmetry is now pinned by a **control test** (`QuoteIdentifier_countsCharacters_notBytes_becauseSysnameIsNvarchar128`, a 128-character Cyrillic name that must still be accepted), so a future sweep applying the byte rule uniformly goes red. RED evidence: `QuoteIdentifier_measuresTheCeilingInUtf8Bytes_notCharacters` failed on both dialects before the change (32 Cyrillic characters = 64 UTF-8 bytes "should throw … but did not"), plus a supplementary-plane case (`U+1D400` × 16 = 64 bytes, 32 code units). --- @@ -54,13 +54,15 @@ | Severity | Low | | Category | Code organization & conventions | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/ISqlDialect.cs:29` | -| Status | Open | +| Status | Resolved | **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)_ +**Resolution (2026-08-10):** Fixed as recommended, together with its consumer (`886ce1b8`) — the two findings were one design, so the seam member and the planner chunking landed in the same commit. `ISqlDialect.MaxBindParameters` is documented as **the ceiling on what a caller may emit, not the raw provider constant**: `SqlServerDialect` reports **2098**, not 2100, because `Microsoft.Data.SqlClient` routes a parameterized command through `sp_executesql` and spends two of the server's parameters on the statement text and the parameter-declaration list — a caller filling 2100 exactly would be rejected for a limit it appeared to respect. `OracleDialect` reports **1000**, the `IN`-expression-list limit (`ORA-01795`), deliberately *not* Oracle's much larger 64k bind-variable count: the seam must report the limit the planner's `IN (:p_k0, …)` list actually hits, or chunking would produce a size the server still refuses. `PostgresDialect` reports **65535**, the `Bind` message's 16-bit count. Each value is pinned by its own dialect test. The seam summary's "three things that differ per backend" is now four. + +The two test dialects (`SqliteDialect` at 999 — `SQLITE_MAX_VARIABLE_NUMBER`'s historical default, the value that always executes whatever native bundle ships — and the three decorators) implement the member too; `Driver.Sql.Browser.Tests` links `SqliteDialect.cs` and stayed green (79/79). --- @@ -71,10 +73,10 @@ | Severity | Low | | Category | Documentation & comments | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/OracleDialect.cs:135` | -| Status | Open | +| Status | Resolved | **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)_ +**Resolution (2026-08-10):** Documentation fix — the recommendation's **second** option (`401d6706`). The claim was verified against the code first: `SqlBrowseSession.RootAsync` renders every row of `ListSchemasSql` as an expandable `BrowseNodeKind.Folder` with `HasChildrenHint: true`, so on Oracle every `ALL_USERS` row does become an offered schema. The remarks now split the two consumers explicitly, mark the browse cost with a ⚠️ as a known accepted trade, spell out the narrower `ALL_TABLES`/`ALL_VIEWS` query verbatim, and record why it was **not** taken here: it also narrows the catalog gate, which is a poll-path behaviour change validatable only against a real Oracle server (`OracleReadTests`) — the same blind spot that let the `p_` bind-prefix defect ship. The note tells a future author to make the swap when an Oracle estate is available to gate it on, and to change both consumers' expectations in one commit. diff --git a/code-reviews/Driver.Sql/findings.md b/code-reviews/Driver.Sql/findings.md index 27dfb7d6..03cb3016 100644 --- a/code-reviews/Driver.Sql/findings.md +++ b/code-reviews/Driver.Sql/findings.md @@ -65,13 +65,19 @@ surface. | Severity | Medium | | Category | Error handling & resilience | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlGroupPlanner.cs:127` | -| Status | Open | +| 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:** _(open)_ +**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. --- diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/ISqlDialect.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/ISqlDialect.cs index 2dd04716..d968389e 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/ISqlDialect.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/ISqlDialect.cs @@ -6,9 +6,10 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Sql; /// /// The provider seam (design §2.2). ADO.NET's base types abstract -/// connections, commands, parameters and readers; they do not abstract the three things that differ -/// per backend — identifier quoting, the metadata-catalog SQL, and row-limit syntax. -/// Those live here, so no dialect assumption leaks into the reader, the poll planner, or the schema browser. +/// connections, commands, parameters and readers; they do not abstract the four things that differ +/// per backend — identifier quoting, the metadata-catalog SQL, row-limit syntax, and +/// the per-command bind-parameter ceiling. Those live here, so no dialect assumption leaks into the +/// reader, the poll planner, or the schema browser. /// This interface is the driver's SQL-injection boundary. Every runtime value is /// bound as a and never reaches a command text. Identifiers cannot be /// parameterized in SQL, so the few that must appear as text are emitted through @@ -106,6 +107,24 @@ public interface ISqlDialect /// The command to configure. void ConfigureCommand(DbCommand command); + /// + /// The most bound parameters this backend accepts in one command — 2098 usable of SQL Server's + /// 2100, 1000 entries in an Oracle IN expression list, 65535 on the Postgres wire protocol. + /// Callers that build a variable-length parameter list must chunk at this number. + /// Why it belongs on the seam. It differs per backend exactly as quoting, catalog SQL and + /// row-limit syntax do, and a call site that guesses a provider limit it does not own is the defect + /// this interface exists to prevent. Its absence was a shipped one: SqlGroupPlanner emitted one + /// parameter per distinct keyValue with no bound, so a key-value (EAV) group above the cap was + /// rejected by the provider after the connection opened and every tag in it published + /// BadCommunicationError on every poll, permanently (Driver.Sql-002 / + /// Driver.Sql.Dialect-002). + /// The number is a ceiling on what the planner may emit, not the raw provider constant — + /// a dialect whose provider spends parameters of its own subtracts them here, so a caller filling the + /// value exactly always produces an executable command. Must be at least 1; a dialect reporting less + /// is rejected at the call site rather than looping or emitting an empty IN (). + /// + int MaxBindParameters { get; } + /// Cheapest statement that proves the connection is alive (SELECT 1; Oracle needs FROM DUAL). string LivenessSql { get; } diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/OracleDialect.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/OracleDialect.cs index a6fd198e..8ffc1003 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/OracleDialect.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/OracleDialect.cs @@ -1,5 +1,6 @@ using System.Data.Common; using System.Globalization; +using System.Text; using Oracle.ManagedDataAccess.Client; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts; @@ -24,6 +25,9 @@ public sealed class OracleDialect : ISqlDialect /// /// Oracle 12.2+'s identifier ceiling. Enforced because a longer string cannot name a real catalog /// object — it is either a config mistake or padding, and rejecting it keeps the quoted output bounded. + /// The unit is BYTES in the database character set, not characters — see + /// . Practically that means UTF-8 (AL32UTF8) on any modern estate, + /// so a 65-character Cyrillic name is already over the limit. /// internal const int MaxIdentifierLength = 128; @@ -99,6 +103,16 @@ public sealed class OracleDialect : ISqlDialect if (command is OracleCommand oracle) oracle.BindByName = true; } + /// + /// 1000 — the tightest of the three ceilings, and the one that actually binds. Oracle allows + /// 64k bind variables per statement, but an IN expression list is limited to 1000 + /// entries (ORA-01795), and the key-value model's IN (:p_k0, …) list is the only + /// variable-length parameter list this driver emits. The seam therefore reports the limit the caller + /// will hit rather than the larger one it never reaches — a dialect that reported 64000 here would be + /// technically accurate and would still produce a statement the server refuses. + /// + public int MaxBindParameters => 1000; + /// Oracle has no bare SELECT; every projection needs a table, and DUAL is the one-row one. public string LivenessSql => "SELECT 1 FROM DUAL"; @@ -127,10 +141,26 @@ public sealed class OracleDialect : ISqlDialect /// user), aliased to the TABLE_SCHEMA column name the catalog readers project by. /// /// - /// Broader than the SQL Server dialect's DISTINCT TABLE_SCHEMA FROM INFORMATION_SCHEMA.TABLES, - /// which lists only schemas that own an object: ALL_USERS includes empty ones. Harmless here — - /// the list is used to resolve an authored schema name to the catalog's own spelling, and a schema - /// holding no tables simply resolves and then matches no relation. + /// Broader than the SQL Server dialect's DISTINCT TABLE_SCHEMA FROM + /// INFORMATION_SCHEMA.TABLES, which lists only schemas that own an object: ALL_USERS + /// includes empty ones. Harmless for the catalog gate — the list is used to resolve an authored + /// schema name to the catalog's own spelling, and a schema holding no tables simply resolves and then + /// matches no relation. + /// ⚠️ It is NOT harmless for the browse tree, and that is a known, accepted trade rather than + /// an oversight (Driver.Sql.Dialect-003). This query has two consumers, and only the gate wants the + /// broad list: SqlBrowseSession.RootAsync renders every row as an expandable schema folder, so + /// on a real Oracle estate the operator is offered every database user — hundreds of them, + /// nearly all empty, with nothing in the tree distinguishing the ones that hold a relation until each + /// is expanded. + /// The narrower query is + /// SELECT DISTINCT owner FROM ALL_TABLES UNION SELECT DISTINCT owner FROM ALL_VIEWS, which + /// would match what the other two dialects list and what the browse tree wants. It is deliberately not + /// taken here because it also narrows the gate: a tag naming an empty schema today resolves and + /// then fails at the table level, and afterwards would fail at the schema level with a different + /// message — a behaviour change on the poll path that can only be validated against a real Oracle + /// server (OracleReadTests), never offline, for the same reason the p_ bind-prefix defect + /// was invisible until it ran. Make the swap when an Oracle estate is available to gate it on, and + /// change both consumers' expectations in the same commit. /// public string ListSchemasSql => "SELECT username AS TABLE_SCHEMA FROM ALL_USERS ORDER BY username"; @@ -187,8 +217,14 @@ public sealed class OracleDialect : ISqlDialect /// (ALL_TAB_COLUMNS.column_name, i.e. what the server actually stored) before anything reaches /// this method — an operator who authored speed against an Oracle SPEED column resolves /// to SPEED at Initialize, and it is SPEED that gets quoted. + /// The length ceiling is measured in UTF-8 bytes, not characters. Oracle's 128 bounds the + /// name in the database character set (AL32UTF8 on any modern estate), so counting + /// UTF-16 code units would accept a 65-character Cyrillic name — 130 bytes — and let it fail at the + /// server with ORA-00972 rather than here, where the message can name the tag. + /// SqlServerDialect deliberately does not do this: T-SQL's sysname is + /// nvarchar(128), a genuine 128-character limit. /// Rejects (all as ): null / empty / all-whitespace; longer - /// than ; any control character (which covers embedded NUL, and the + /// than bytes; any control character (which covers embedded NUL, and the /// C0/C1 ranges that can truncate or corrupt a logged/audited statement); and any Unicode /// character — bidi overrides, zero-width spaces, soft hyphen, /// BOM. Those last cannot escape the quotes, but they spoof the rendered text of a log line or @@ -209,9 +245,13 @@ public sealed class OracleDialect : ISqlDialect throw new ArgumentException( "A SQL identifier must be a non-empty, non-whitespace name.", nameof(ident)); - if (ident.Length > MaxIdentifierLength) + // Byte count, not ident.Length: Oracle's 128 is measured in the database character set, so a + // 65-character Cyrillic name is 130 UTF-8 bytes and the server answers ORA-00972. Counting UTF-16 + // code units would send it anyway, failing at the server for a reason this dialect could have named. + var byteLength = Encoding.UTF8.GetByteCount(ident); + if (byteLength > MaxIdentifierLength) throw new ArgumentException( - $"An Oracle identifier may not exceed {MaxIdentifierLength} characters (got {ident.Length}).", + $"An Oracle identifier may not exceed {MaxIdentifierLength} bytes (got {byteLength}).", nameof(ident)); foreach (var ch in ident) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/PostgresDialect.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/PostgresDialect.cs index eaa5a9b6..8457d1c2 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/PostgresDialect.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/PostgresDialect.cs @@ -1,5 +1,6 @@ using System.Data.Common; using System.Globalization; +using System.Text; using Npgsql; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; using ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts; @@ -20,6 +21,9 @@ public sealed class PostgresDialect : ISqlDialect /// PostgreSQL's identifier ceiling — NAMEDATALEN - 1, 63 by default. Enforced because the /// server silently truncates anything longer, so a 70-character authored name would resolve to /// a different object than it spells; rejecting it surfaces the config mistake instead. + /// The unit is BYTES in the database encoding, not characters — see + /// . Practically that means UTF-8 on any modern estate, so a 32-character + /// Cyrillic name is already over the limit. /// internal const int MaxIdentifierLength = 63; @@ -60,6 +64,14 @@ public sealed class PostgresDialect : ISqlDialect // Intentionally empty: see the summary. } + /// + /// 65535 — the extended-query protocol carries the parameter count as a signed/unsigned 16-bit field + /// in Bind, so that is a protocol ceiling rather than a server setting, and Npgsql raises it + /// client-side before a round-trip. Far above anything a poll group reaches in practice; it is here so + /// the planner never has to special-case "this dialect has no meaningful limit". + /// + public int MaxBindParameters => 65535; + /// public string LivenessSql => "SELECT 1"; @@ -149,12 +161,19 @@ public sealed class PostgresDialect : ISqlDialect /// Rejects (all as , mirroring /// SqlServerDialect.QuoteIdentifier so a test written against one dialect fails the same way /// against the other): null / empty / all-whitespace; longer than - /// ; any control character (which covers embedded NUL and the C0/C1 + /// bytes; any control character (which covers embedded NUL and the C0/C1 /// ranges that can truncate or corrupt a logged/audited statement); and any Unicode /// character — bidi overrides, zero-width spaces, soft hyphen, /// BOM. Those last cannot escape the quotes, but they spoof the rendered text of a log line /// or an AdminUI label while comparing byte-different from the real catalog name (the Trojan-Source /// class, CVE-2021-42574). + /// The length ceiling is measured in UTF-8 bytes, not characters — NAMEDATALEN + /// bounds the stored name in the database encoding, and every modern estate is UTF-8. Counting UTF-16 + /// code units (ident.Length) would accept a 32-character Cyrillic name, which is 64 bytes and + /// is therefore truncated server-side to a name that denotes a different object — precisely + /// the silent-truncation failure this rule exists to prevent. SqlServerDialect deliberately does + /// not do this: T-SQL's sysname is nvarchar(128), a genuine 128-character limit, + /// so counting characters is correct there and counting bytes would reject legal names. /// NUL deserves its own note here: Postgres rejects a NUL byte anywhere in a query string /// outright, so passing one through would fail the whole statement rather than just the one /// identifier. @@ -175,9 +194,13 @@ public sealed class PostgresDialect : ISqlDialect throw new ArgumentException( "A SQL identifier must be a non-empty, non-whitespace name.", nameof(ident)); - if (ident.Length > MaxIdentifierLength) + // Byte count, not ident.Length: NAMEDATALEN bounds the stored name in the database encoding, so a + // 32-character Cyrillic name is 64 UTF-8 bytes and the server truncates it — the exact silent + // wrong-object outcome this check exists to prevent, and one a UTF-16 code-unit count waves through. + var byteLength = Encoding.UTF8.GetByteCount(ident); + if (byteLength > MaxIdentifierLength) throw new ArgumentException( - $"A PostgreSQL identifier may not exceed {MaxIdentifierLength} characters (got {ident.Length}).", + $"A PostgreSQL identifier may not exceed {MaxIdentifierLength} bytes (got {byteLength}).", nameof(ident)); foreach (var ch in ident) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/SqlServerDialect.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/SqlServerDialect.cs index aafce4e7..2e3c422b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/SqlServerDialect.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect/SqlServerDialect.cs @@ -18,6 +18,10 @@ public sealed class SqlServerDialect : ISqlDialect /// T-SQL's regular-identifier ceiling (sysname = nvarchar(128)). Enforced because a /// longer string cannot name a real catalog object — it is either a config mistake or padding, and /// rejecting it keeps the quoted output bounded. + /// The unit here is genuinely CHARACTERS (UTF-16 code units, which is what + /// nvarchar(128) counts), deliberately unlike PostgresDialect and OracleDialect, + /// whose ceilings are byte limits in the database encoding. Measuring bytes here would reject a legal + /// 128-character non-ASCII name. /// internal const int MaxIdentifierLength = 128; @@ -55,6 +59,15 @@ public sealed class SqlServerDialect : ISqlDialect // Intentionally empty: see the summary. } + /// + /// 2098 — SQL Server's hard cap is 2100 parameters per statement, and + /// Microsoft.Data.SqlClient spends two of them itself: a parameterized command is executed + /// through sp_executesql, whose first two arguments are the statement text and the parameter + /// declaration list. A caller filling 2100 would therefore be rejected with + /// "The incoming request has too many parameters" for a limit it appeared to respect. + /// + public int MaxBindParameters => 2098; + /// public string LivenessSql => "SELECT 1"; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlGroupPlanner.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlGroupPlanner.cs index 1dfc08bf..88001b5a 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlGroupPlanner.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlGroupPlanner.cs @@ -7,6 +7,9 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Sql; /// /// Folds a set of s into the minimum number of parameterized queries /// (design §3.6) — the driver's batching core. Pure and side-effect free: no connection, no I/O, no clock. +/// "Minimum" is bounded by the provider, not only by the group key. A key-value group whose +/// distinct keys exceed is emitted as several plans over the +/// same group key rather than as one statement the backend refuses — see . /// The GroupKey is the correctness contract. Two tags share a query only when every /// field that shapes the result set is identical. Too coarse and a tag silently reads a neighbour's cell; /// too fine and batching degenerates to one round-trip per tag. Both directions are pinned by @@ -31,14 +34,21 @@ public static class SqlGroupPlanner private const string RowSelectorBaseName = "w"; /// - /// Groups by source and emits one per group. - /// Deterministic: groups appear in the input order of their first member, members keep their input - /// order within a group, and parameters keep the first-appearance order of their distinct values — so a - /// given tag list always yields byte-identical SQL. + /// Groups by source and emits one per group — or + /// several, when a key-value group holds more distinct keys than the dialect's + /// allows in one command (see + /// ). Plans from one group share that group's key. + /// Deterministic: groups appear in the input order of their first member, a split group's chunks + /// stay adjacent and in key order, members keep their input order within a chunk, and parameters keep + /// the first-appearance order of their distinct values — so a given tag list always yields + /// byte-identical SQL. /// /// The tags to plan. An empty sequence yields no plans. - /// Supplies identifier quoting and the provider's row-limit syntax. - /// One plan per distinct group key. + /// + /// Supplies identifier quoting, the provider's row-limit syntax, and its per-command bind-parameter + /// ceiling. + /// + /// At least one plan per distinct group key; more when a key-value group had to be chunked. /// or is null. /// /// A definition is missing a field its model requires (e.g. a wide-row tag with no row selector), or an @@ -78,12 +88,18 @@ public static class SqlGroupPlanner foreach (var key in order) { var members = groups[key]; - plans.Add(members[0].Model switch + switch (members[0].Model) { - SqlTagModel.KeyValue => BuildKeyValuePlan(key, members, dialect), - SqlTagModel.WideRow => BuildWideRowPlan(key, members, dialect), - _ => throw UnsupportedModel(members[0]), - }); + case SqlTagModel.KeyValue: + // One group can yield SEVERAL plans: the IN list is chunked at the dialect's ceiling. + AddKeyValuePlans(plans, key, members, dialect); + break; + case SqlTagModel.WideRow: + plans.Add(BuildWideRowPlan(key, members, dialect)); + break; + default: + throw UnsupportedModel(members[0]); + } } return plans; @@ -119,10 +135,71 @@ public static class SqlGroupPlanner }; /// - /// Emits SELECT <key>, <value>[, <ts>] FROM <table> WHERE <key> IN (@k0..@kN). - /// The key column is selected because the reader indexes rows by it to slice values back per member. + /// Emits one key-value plan per chunk of the group's distinct keys, sized by + /// . A group at or under the ceiling — every ordinary group + /// — still yields exactly one plan. + /// Why chunk rather than let the provider refuse. Every backend caps parameters per + /// command (2100 on SQL Server, 1000 IN-list entries on Oracle), and a key-value (EAV) staging + /// table is precisely the shape that attracts thousands of tags on one table — which the + /// group-key rule folds into one group. Over the cap the provider rejects the statement after + /// the connection opened, so every tag in the group publishes BadCommunicationError on every + /// poll, permanently, and the driver reports a communication fault against a database that is + /// answering (Driver.Sql-002). + /// Chunks share the group key, and no consumer needs to change. The reader executes one + /// command per plan and slices each plan's own back, so several + /// plans over one group key cost extra round-trips and nothing else. The alternative — a deploy-time + /// tag-count gate — would refuse a configuration the driver can serve perfectly well. + /// Members follow their key, never their position. Two tags authored on the same + /// keyValue land in the same chunk (one bound parameter, both fed); a member placed in a chunk + /// that does not bind its key would read a result set that cannot contain its row and publish + /// BadNoData forever. + /// + private static void AddKeyValuePlans( + List plans, object groupKey, List members, ISqlDialect dialect) + { + var ceiling = dialect.MaxBindParameters; + if (ceiling < 1) + { + // Not a silent clamp: chunking to zero would loop forever, and a one-per-chunk fallback would + // hide a broken dialect behind a poll that suddenly costs one round-trip per tag. + throw new ArgumentException( + $"The {dialect.Provider} dialect reports a per-command bind-parameter ceiling of {ceiling}; " + + "it must be at least 1.", nameof(dialect)); + } + + // Distinct keys in first-appearance order decide the chunk boundaries; every member then follows its + // own key. Both halves keep the planner's determinism: a given tag list yields byte-identical SQL. + var chunkOfKey = new Dictionary(StringComparer.Ordinal); + foreach (var member in members) + { + var keyValue = member.KeyValue + ?? throw MissingField(nameof(SqlTagDefinition.KeyValue), member); + if (!chunkOfKey.ContainsKey(keyValue)) chunkOfKey[keyValue] = chunkOfKey.Count / ceiling; + } + + var chunkCount = ((chunkOfKey.Count - 1) / ceiling) + 1; + if (chunkCount == 1) + { + plans.Add(BuildKeyValuePlan(groupKey, members, dialect)); + return; + } + + var chunks = new List[chunkCount]; + for (var c = 0; c < chunkCount; c++) chunks[c] = []; + foreach (var member in members) chunks[chunkOfKey[member.KeyValue!]].Add(member); + + foreach (var chunk in chunks) plans.Add(BuildKeyValuePlan(groupKey, chunk, dialect)); + } + + /// + /// Emits SELECT <key>, <value>[, <ts>] FROM <table> WHERE <key> IN (@k0..@kN) + /// for one chunk of a group. The key column is selected because the reader indexes rows by it to slice + /// values back per member. /// Keys are de-duplicated (ordinal) before binding: two tags authored against the same /// keyValue bind one parameter but stay two members, so both nodes are fed from the one row. + /// Markers are numbered from k0 within the chunk — each chunk is its own command, so + /// carrying a group-wide index forward would leave a three-parameter command spelling + /// IN (@k3, @k4, @k5). /// private static SqlQueryPlan BuildKeyValuePlan( object groupKey, List members, ISqlDialect dialect) diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlQueryPlan.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlQueryPlan.cs index d9c14dbe..8e20c989 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlQueryPlan.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlQueryPlan.cs @@ -27,6 +27,9 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Sql; /// /// The opaque equality key that decided this grouping — a value tuple, so two plans compare equal exactly /// when they would have folded together. Exposed for diagnostics and plan caching, never for parsing. +/// Not unique across plans. A key-value group holding more distinct keys than the dialect's +/// is emitted as several plans carrying this same key +/// — the chunking is an execution detail of one group, not a regrouping. /// /// The single parameterized statement to execute for this group. /// diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/ColonMarkerDialect.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/ColonMarkerDialect.cs index d8f8ae60..fa8dd2b3 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/ColonMarkerDialect.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/ColonMarkerDialect.cs @@ -65,6 +65,9 @@ public sealed class ColonMarkerDialect : ISqlDialect /// public DbProviderFactory Factory => Inner.Factory; + /// + public int MaxBindParameters => Inner.MaxBindParameters; + /// public string LivenessSql => Inner.LivenessSql; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/OracleDialectTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/OracleDialectTests.cs index e8b0c375..a0050cf7 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/OracleDialectTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/OracleDialectTests.cs @@ -165,13 +165,45 @@ public class OracleDialectTests => Should.Throw(() => new OracleDialect().QuoteIdentifier(ident)); [Fact] - public void QuoteIdentifier_acceptsExactly128Chars_andRejects129() + public void QuoteIdentifier_acceptsExactly128Bytes_andRejects129() { var d = new OracleDialect(); d.QuoteIdentifier(new string('a', 128)).ShouldBe("\"" + new string('a', 128) + "\""); Should.Throw(() => d.QuoteIdentifier(new string('a', 129))); } + /// + /// Oracle 12.2's 128 is a byte ceiling in the database character set (UTF-8 on any modern + /// estate), so a 65-character Cyrillic name is 130 bytes and the server rejects it with + /// ORA-00972. Measuring UTF-16 code units here would pass it through to fail at the server + /// instead — for a reason this dialect could have named. + /// + [Fact] + public void QuoteIdentifier_measuresTheCeilingInUtf8Bytes_notCharacters() + { + var d = new OracleDialect(); + + // 64 Cyrillic characters = 128 UTF-8 bytes — exactly at the limit. + d.QuoteIdentifier(new string('б', 64)).ShouldBe("\"" + new string('б', 64) + "\""); + + // 65 characters = 130 bytes. A character-counting ceiling waves this through. + Should.Throw(() => d.QuoteIdentifier(new string('б', 65))); + } + + // ---- bind-parameter ceiling ---- + + /// + /// 1000 is the IN-expression-list limit (ORA-01795), NOT Oracle's much larger 64k bind + /// count — the seam has to report the limit the planner's IN (:p_k0, …) list actually hits, or + /// it would chunk to a size the server still refuses. + /// + [Fact] + public void MaxBindParameters_isTheInListLimit_notTheLargerBindVariableCount() + { + new OracleDialect().MaxBindParameters.ShouldBe(1000); + new OracleDialect().MaxBindParameters.ShouldBeLessThan(new PostgresDialect().MaxBindParameters); + } + // ---- liveness + row limit ---- [Fact] diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/PostgresDialectTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/PostgresDialectTests.cs index 0ca35ec1..bfd979f8 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/PostgresDialectTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/PostgresDialectTests.cs @@ -69,13 +69,43 @@ public class PostgresDialectTests // NAMEDATALEN - 1: a longer string cannot name a real Postgres object (the server truncates it), so it // is a config mistake rather than something to quote and send. [Fact] - public void QuoteIdentifier_acceptsExactly63Chars_andRejects64() + public void QuoteIdentifier_acceptsExactly63Bytes_andRejects64() { var d = new PostgresDialect(); d.QuoteIdentifier(new string('a', 63)).ShouldBe("\"" + new string('a', 63) + "\""); Should.Throw(() => d.QuoteIdentifier(new string('a', 64))); } + /// + /// NAMEDATALEN - 1 is 63 bytes in the database encoding, not 63 characters — so a + /// 32-character Cyrillic name is 64 UTF-8 bytes and the server truncates it to something that names a + /// different object. A ceiling measured in UTF-16 code units lets that name straight through, which + /// is precisely the silent-truncation failure this rule documents itself as preventing. + /// + [Fact] + public void QuoteIdentifier_measuresTheCeilingInUtf8Bytes_notCharacters() + { + var d = new PostgresDialect(); + + // 31 Cyrillic characters = 62 UTF-8 bytes — inside the limit, and well inside a character count. + d.QuoteIdentifier(new string('б', 31)).ShouldBe("\"" + new string('б', 31) + "\""); + + // 32 characters = 64 bytes. Under a character-counting ceiling this passes; the server truncates it. + Should.Throw(() => d.QuoteIdentifier(new string('б', 32))); + } + + /// A non-BMP character is 4 UTF-8 bytes (and 2 UTF-16 code units) — 16 of them fill the limit. + [Fact] + public void QuoteIdentifier_countsASupplementaryCharacterAsItsFourUtf8Bytes() + { + var d = new PostgresDialect(); + // U+1D400 MATHEMATICAL BOLD CAPITAL A, as a surrogate pair. + const string a = "\U0001D400"; + d.QuoteIdentifier(string.Concat(Enumerable.Repeat(a, 15))).ShouldNotBeNullOrEmpty(); // 60 bytes + Should.Throw( + () => d.QuoteIdentifier(string.Concat(Enumerable.Repeat(a, 16)))); // 64 bytes + } + [Fact] public void QuoteIdentifier_neutralisesAClassicInjectionPayload() { @@ -189,6 +219,15 @@ public class PostgresDialectTests .ShouldBe("SELECT \"a\" LIMIT 1"); } + /// + /// 65535 — the Bind message's 16-bit parameter count, a protocol ceiling rather than a server + /// setting. Pinned so the planner's chunking has a real number here instead of a special case for + /// "this dialect has no meaningful limit". + /// + [Fact] + public void MaxBindParameters_isTheWireProtocolsSixteenBitCount() + => new PostgresDialect().MaxBindParameters.ShouldBe(65535); + [Fact] public void Dialect_identifiesItselfAsPostgres_andExposesTheAbstractFactory() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlCatalogDefaultSchemaTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlCatalogDefaultSchemaTests.cs index 76663354..0c72ca4a 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlCatalogDefaultSchemaTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlCatalogDefaultSchemaTests.cs @@ -105,6 +105,8 @@ public sealed class SqlCatalogDefaultSchemaTests public DbProviderFactory Factory => Inner.Factory; + public int MaxBindParameters => Inner.MaxBindParameters; + public string LivenessSql => Inner.LivenessSql; public string SingleRowLimitPrefix => Inner.SingleRowLimitPrefix; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlCatalogGateDriverTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlCatalogGateDriverTests.cs index a7243fb0..f94714c3 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlCatalogGateDriverTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlCatalogGateDriverTests.cs @@ -337,6 +337,8 @@ public sealed class SqlCatalogGateDriverTests public System.Data.Common.DbProviderFactory Factory => Inner.Factory; + public int MaxBindParameters => Inner.MaxBindParameters; + public string LivenessSql => Inner.LivenessSql; public string SingleRowLimitPrefix => Inner.SingleRowLimitPrefix; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlGroupPlannerChunkingTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlGroupPlannerChunkingTests.cs new file mode 100644 index 00000000..d8bc6c8a --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlGroupPlannerChunkingTests.cs @@ -0,0 +1,318 @@ +using System.Data.Common; +using System.Globalization; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests; + +/// +/// Pins the bind-parameter ceiling (Driver.Sql-002 / Driver.Sql.Dialect-002): a key-value group holding +/// more distinct keyValues than the dialect's is split +/// into several plans over the same group key, rather than emitting one statement the provider +/// rejects. +/// Why this is a correctness test and not a tuning one. Over the ceiling the provider refuses +/// the statement after the connection opened, so every tag in the group publishes +/// BadCommunicationError on every poll, permanently — and a key-value (EAV) staging table is +/// precisely the shape that attracts thousands of tags on one table, which the group-key rule folds into +/// one group. +/// A single group emitting several plans needs no reader change: SqlPollReader runs +/// one command per plan and slices each plan's own Members back, so the last test here drives the +/// whole read path over a real database to prove exactly that. +/// +public sealed class SqlGroupPlannerChunkingTests +{ + private static SqlTagDefinition KvTag(string name, string keyValue, string table = "dbo.TagValues") + => new(name, SqlTagModel.KeyValue, table, + KeyColumn: "tag_name", KeyValue: keyValue, ValueColumn: "num_value"); + + // ---- the shipped ceilings ---- + + /// + /// Oracle's real ceiling, on the real dialect: an IN expression list may hold 1000 entries, so + /// 1001 distinct keys cannot be one statement. This is the shape the finding describes — no test-only + /// dialect involved. + /// + [Fact] + public void AGroupBeyondTheOracleInListLimit_isSplit_ratherThanEmittedAsOneRejectedStatement() + { + var dialect = new OracleDialect(); + var tags = Enumerable.Range(0, 1001) + .Select(i => KvTag("T" + i.ToString(CultureInfo.InvariantCulture), + "Key" + i.ToString(CultureInfo.InvariantCulture))) + .ToArray(); + + var plans = SqlGroupPlanner.Plan(tags, dialect); + + plans.Count.ShouldBe(2); + plans.ShouldAllBe(p => p.Parameters.Count <= 1000); + plans.Sum(p => p.Parameters.Count).ShouldBe(1001); + } + + /// + /// The same for SQL Server, whose 2100-parameter command cap is the other limit the finding names. + /// + [Fact] + public void AGroupBeyondTheSqlServerParameterCap_isSplit() + { + var dialect = new SqlServerDialect(); + var tags = Enumerable.Range(0, dialect.MaxBindParameters + 1) + .Select(i => KvTag("T" + i.ToString(CultureInfo.InvariantCulture), + "Key" + i.ToString(CultureInfo.InvariantCulture))) + .ToArray(); + + var plans = SqlGroupPlanner.Plan(tags, dialect); + + plans.Count.ShouldBe(2); + plans.ShouldAllBe(p => p.Parameters.Count <= dialect.MaxBindParameters); + } + + // ---- how a split group is shaped ---- + + [Fact] + public void EachChunkBindsAtMostTheCeiling_andTheChunksCoverEveryKeyExactlyOnce() + { + var tags = Enumerable.Range(0, 7) + .Select(i => KvTag("T" + i.ToString(CultureInfo.InvariantCulture), + "Key" + i.ToString(CultureInfo.InvariantCulture))) + .ToArray(); + + var plans = SqlGroupPlanner.Plan(tags, new BindCeilingDialect(3)); + + plans.Count.ShouldBe(3); + plans.Select(p => p.Parameters.Count).ShouldBe(new[] { 3, 3, 1 }); + plans.SelectMany(p => p.Parameters) + .ShouldBe(Enumerable.Range(0, 7).Select(i => (object?)("Key" + i.ToString(CultureInfo.InvariantCulture)))); + } + + /// + /// Every member must land in the chunk that binds its own key — a member whose key is bound + /// by a different plan reads a result set that cannot contain its row and publishes + /// BadNoData forever, which is the quiet way to get this wrong. + /// + [Fact] + public void EveryMemberLandsInTheChunkThatBindsItsOwnKey() + { + var tags = Enumerable.Range(0, 7) + .Select(i => KvTag("T" + i.ToString(CultureInfo.InvariantCulture), + "Key" + i.ToString(CultureInfo.InvariantCulture))) + .ToArray(); + + var plans = SqlGroupPlanner.Plan(tags, new BindCeilingDialect(3)); + + foreach (var plan in plans) + foreach (var member in plan.Members) + plan.Parameters.ShouldContain(member.KeyValue); + + // …and no member is dropped or duplicated across the split. + plans.SelectMany(p => p.Members).Select(m => m.Name) + .ShouldBe(tags.Select(t => t.Name), ignoreOrder: true); + plans.Sum(p => p.Members.Count).ShouldBe(tags.Length); + } + + /// + /// Two tags on one keyValue bind one parameter and must stay in one chunk — splitting them + /// would bind the same key twice and cost a whole extra round-trip for a row already fetched. + /// + [Fact] + public void MembersSharingAKeyValue_stayInTheSameChunk() + { + var plans = SqlGroupPlanner.Plan( + [ + KvTag("A", "K1"), KvTag("B", "K2"), KvTag("C", "K1"), KvTag("D", "K3"), + ], new BindCeilingDialect(2)); + + plans.Count.ShouldBe(2); + plans[0].Parameters.ShouldBe(new object[] { "K1", "K2" }); + plans[0].Members.Select(m => m.Name).ShouldBe(new[] { "A", "B", "C" }); + plans[1].Parameters.ShouldBe(new object[] { "K3" }); + plans[1].Members.Select(m => m.Name).ShouldBe(new[] { "D" }); + } + + /// + /// Each chunk is its own command, so its markers restart at k0 — carrying the global index + /// forward would emit IN (@k3, @k4, @k5) against a three-parameter command, which is legal but + /// makes a plan's SQL unreadable next to its own parameter list. + /// + [Fact] + public void EachChunkNumbersItsMarkersFromZero_andTheSqlMatchesItsOwnParameters() + { + var tags = Enumerable.Range(0, 5) + .Select(i => KvTag("T" + i.ToString(CultureInfo.InvariantCulture), + "Key" + i.ToString(CultureInfo.InvariantCulture))) + .ToArray(); + + var plans = SqlGroupPlanner.Plan(tags, new BindCeilingDialect(2)); + + foreach (var plan in plans) + { + plan.ParameterNames.ShouldBe( + Enumerable.Range(0, plan.Parameters.Count) + .Select(i => "@k" + i.ToString(CultureInfo.InvariantCulture))); + plan.SqlText.ShouldEndWith("IN (" + string.Join(", ", plan.ParameterNames) + ")"); + } + } + + /// All chunks of one group carry that group's key — the split is an execution detail, not a regrouping. + [Fact] + public void EveryChunkOfOneGroupCarriesTheSameGroupKey() + { + var tags = Enumerable.Range(0, 5) + .Select(i => KvTag("T" + i.ToString(CultureInfo.InvariantCulture), + "Key" + i.ToString(CultureInfo.InvariantCulture))) + .ToArray(); + + var plans = SqlGroupPlanner.Plan(tags, new BindCeilingDialect(2)); + + plans.Count.ShouldBeGreaterThan(1); + plans.Select(p => p.GroupKey).Distinct().Count().ShouldBe(1); + } + + /// + /// The falsifiability control. Chunking must not fragment an ordinary group: a planner that split + /// eagerly (or off a stale count) would pass every test above and quietly turn one round-trip per + /// source into N. + /// + [Theory] + [InlineData(1)] + [InlineData(2)] + [InlineData(3)] + public void AGroupAtOrUnderTheCeiling_stillEmitsExactlyOnePlan(int distinctKeys) + { + var tags = Enumerable.Range(0, distinctKeys) + .Select(i => KvTag("T" + i.ToString(CultureInfo.InvariantCulture), + "Key" + i.ToString(CultureInfo.InvariantCulture))) + .ToArray(); + + var plans = SqlGroupPlanner.Plan(tags, new BindCeilingDialect(3)); + + plans.Count.ShouldBe(1); + plans[0].Members.Count.ShouldBe(distinctKeys); + } + + /// + /// Two groups that each need splitting stay separate, and the chunks of one never mix with the other's + /// — the split happens inside a group, after grouping. + /// + [Fact] + public void TwoOversizedGroups_areSplitIndependently() + { + var tags = Enumerable.Range(0, 3).Select(i => KvTag("A" + i, "K" + i, "dbo.First")) + .Concat(Enumerable.Range(0, 3).Select(i => KvTag("B" + i, "K" + i, "dbo.Second"))) + .ToArray(); + + var plans = SqlGroupPlanner.Plan(tags, new BindCeilingDialect(2)); + + plans.Count.ShouldBe(4); + // Group order still follows the input order of each group's first member (dbo.First, then dbo.Second), + // and a group's own chunks stay adjacent and in key order. + plans.Select(p => p.Members[0].Table).ShouldBe(["dbo.First", "dbo.First", "dbo.Second", "dbo.Second"]); + } + + /// A wide-row plan binds at most one value, so a ceiling of 1 must not disturb it. + [Fact] + public void AWideRowPlan_isUnaffectedByTheCeiling() + { + var tag = new SqlTagDefinition("A", SqlTagModel.WideRow, "dbo.LatestStatus", + ColumnName: "oven_temp", RowSelectorColumn: "station_id", RowSelectorValue: "7"); + + var plans = SqlGroupPlanner.Plan([tag], new BindCeilingDialect(1)); + + plans.Count.ShouldBe(1); + plans[0].Parameters.Count.ShouldBe(1); + } + + /// + /// A dialect reporting a ceiling below 1 cannot be chunked to — it must fail loudly rather than loop + /// forever or emit an empty IN () list, which is a syntax error on every backend. + /// + [Theory] + [InlineData(0)] + [InlineData(-1)] + public void ADialectReportingANonPositiveCeiling_isRejected(int ceiling) + => Should.Throw( + () => SqlGroupPlanner.Plan([KvTag("A", "K1")], new BindCeilingDialect(ceiling))); + + // ---- the whole read path, over a real database ---- + + /// + /// Drives over the SQLite fixture with a ceiling of 1, so three keys become + /// three plans, and asserts every tag still reads its own value. This is what proves several plans per + /// group need no reader change — the claim the chunking design rests on. + /// + [Fact] + public async Task ThePollReader_readsEveryTagCorrectly_whenOneGroupIsSplitAcrossSeveralPlans() + { + using var fixture = new SqlitePollFixture(); + SqlTagDefinition[] tags = + [ + Fixture("Speed", SqlitePollFixture.PresentKey), + Fixture("Pressure", SqlitePollFixture.SecondPresentKey), + Fixture("Missing", SqlitePollFixture.AbsentKey), + ]; + + var reader = new SqlPollReader( + fixture.Factory, fixture.ConnectionString, new BindCeilingDialect(1), + commandTimeout: TimeSpan.FromSeconds(10), operationTimeout: TimeSpan.FromSeconds(15), + maxConcurrentGroups: 4, nullIsBad: false, + resolve: reference => tags.FirstOrDefault(t => t.Name == reference)); + + var snapshots = await reader.ReadAsync(["Speed", "Pressure", "Missing"], CancellationToken.None); + + snapshots[0].Value.ShouldBe(SqlitePollFixture.PresentValue); + snapshots[0].StatusCode.ShouldBe(SqlStatusCodes.Good); + snapshots[1].Value.ShouldBe(SqlitePollFixture.SecondPresentValue); + snapshots[1].StatusCode.ShouldBe(SqlStatusCodes.Good); + snapshots[2].StatusCode.ShouldBe(SqlStatusCodes.BadNoData); + } + + private static SqlTagDefinition Fixture(string name, string keyValue) + => new(name, SqlTagModel.KeyValue, SqlitePollFixture.KeyValueTable, + KeyColumn: SqlitePollFixture.KeyColumn, KeyValue: keyValue, + ValueColumn: SqlitePollFixture.ValueColumn, + TimestampColumn: SqlitePollFixture.TimestampColumn); + + /// + /// A test-only dialect that reports a caller-chosen + /// and delegates everything else to — so a chunked plan can be asserted on + /// at a readable size and actually executed against the fixture database. + /// A decorator rather than a subclass, for the reason the other fakes here give: + /// is sealed, and new-hiding a member would leave interface dispatch + /// calling the original, so the substitution would silently not happen. + /// + private sealed class BindCeilingDialect(int maxBindParameters) : ISqlDialect + { + private static readonly SqliteDialect Inner = new(); + + public int MaxBindParameters { get; } = maxBindParameters; + + public SqlProvider Provider => Inner.Provider; + + public DbProviderFactory Factory => Inner.Factory; + + public string LivenessSql => Inner.LivenessSql; + + public string SingleRowLimitPrefix => Inner.SingleRowLimitPrefix; + + public string SingleRowLimitSuffix => Inner.SingleRowLimitSuffix; + + public string ListSchemasSql => Inner.ListSchemasSql; + + public string DefaultSchemaSql => Inner.DefaultSchemaSql; + + public string ListTablesSql => Inner.ListTablesSql; + + public string ListColumnsSql => Inner.ListColumnsSql; + + public string ParameterMarker(string baseName) => Inner.ParameterMarker(baseName); + + public string ParameterBindName(string baseName) => Inner.ParameterBindName(baseName); + + public void ConfigureCommand(DbCommand command) => Inner.ConfigureCommand(command); + + public string QuoteIdentifier(string ident) => Inner.QuoteIdentifier(ident); + + public DriverDataType MapColumnType(string sqlDataType) => Inner.MapColumnType(sqlDataType); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlServerDialectTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlServerDialectTests.cs index e10995bc..901382f1 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlServerDialectTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlServerDialectTests.cs @@ -84,6 +84,18 @@ public class SqlServerDialectTests .ShouldBe("SELECT TOP 1 [a]"); } + /// + /// 2098, not 2100: Microsoft.Data.SqlClient routes a parameterized command through + /// sp_executesql, whose statement text and parameter-declaration arguments consume two of the + /// server's 2100. A ceiling of 2100 would be rejected by a caller that filled it exactly. + /// + [Fact] + public void MaxBindParameters_leavesRoomForSpExecutesqlsOwnTwoParameters() + { + new SqlServerDialect().MaxBindParameters.ShouldBe(2098); + new SqlServerDialect().MaxBindParameters.ShouldBeLessThan(2100); + } + // ---- extra guards on the injection boundary (beyond the plan's golden set) ---- [Fact] @@ -117,6 +129,19 @@ public class SqlServerDialectTests Should.Throw(() => d.QuoteIdentifier(new string('a', 129))); } + /// + /// The falsifiability control for the Postgres/Oracle byte ceilings: T-SQL's sysname is + /// nvarchar(128), a genuine 128-character (UTF-16 code unit) limit, so a 128-character + /// Cyrillic name — 256 UTF-8 bytes — is legal here and must still be accepted. A byte rule applied + /// uniformly across the three dialects would reject it, and only this test would say so. + /// + [Fact] + public void QuoteIdentifier_countsCharacters_notBytes_becauseSysnameIsNvarchar128() + { + var ident = new string('б', 128); // 128 chars, 256 UTF-8 bytes + new SqlServerDialect().QuoteIdentifier(ident).ShouldBe("[" + ident + "]"); + } + [Fact] public void QuoteIdentifier_neutralisesAClassicInjectionPayload() { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqliteDialect.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqliteDialect.cs index b60fdc4a..62b9f3f4 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqliteDialect.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqliteDialect.cs @@ -64,6 +64,13 @@ public sealed class SqliteDialect : ISqlDialect // Intentionally empty: see the summary. } + /// + /// 999 — SQLITE_MAX_VARIABLE_NUMBER's historical default, and still the safe floor: SQLite + /// raised it to 32766 in 3.32, but the value is a compile-time setting of whatever native library the + /// bundle happens to carry, so the lower number is the one that always executes. + /// + public int MaxBindParameters => 999; + /// public string LivenessSql => "SELECT 1";