merge: SQL dialect+chunking review findings resolved (Driver.Sql-002, Driver.Sql.Dialect-001/-002/-003)
This commit is contained in:
Joseph Doherty
@@ -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.
|
||||
|
||||
@@ -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.
|
||||
|
||||
---
|
||||
|
||||
|
||||
@@ -6,9 +6,10 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Sql;
|
||||
|
||||
/// <summary>
|
||||
/// The provider seam (design §2.2). ADO.NET's <see cref="System.Data.Common"/> base types abstract
|
||||
/// connections, commands, parameters and readers; they do <b>not</b> abstract the three things that differ
|
||||
/// per backend — <b>identifier quoting</b>, the <b>metadata-catalog SQL</b>, and <b>row-limit syntax</b>.
|
||||
/// 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 <b>not</b> abstract the four things that differ
|
||||
/// per backend — <b>identifier quoting</b>, the <b>metadata-catalog SQL</b>, <b>row-limit syntax</b>, and
|
||||
/// the <b>per-command bind-parameter ceiling</b>. Those live here, so no dialect assumption leaks into the
|
||||
/// reader, the poll planner, or the schema browser.
|
||||
/// <para><b>This interface is the driver's SQL-injection boundary.</b> Every runtime <em>value</em> is
|
||||
/// bound as a <see cref="DbParameter"/> 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
|
||||
/// <param name="command">The command to configure.</param>
|
||||
void ConfigureCommand(DbCommand command);
|
||||
|
||||
/// <summary>
|
||||
/// The most bound parameters this backend accepts in <b>one</b> command — 2098 usable of SQL Server's
|
||||
/// 2100, 1000 entries in an Oracle <c>IN</c> expression list, 65535 on the Postgres wire protocol.
|
||||
/// Callers that build a variable-length parameter list must chunk at this number.
|
||||
/// <para><b>Why it belongs on the seam.</b> 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: <c>SqlGroupPlanner</c> emitted one
|
||||
/// parameter per distinct <c>keyValue</c> with no bound, so a key-value (EAV) group above the cap was
|
||||
/// rejected by the provider <em>after</em> the connection opened and every tag in it published
|
||||
/// <c>BadCommunicationError</c> on every poll, permanently (Driver.Sql-002 /
|
||||
/// Driver.Sql.Dialect-002).</para>
|
||||
/// <para><b>The number is a ceiling on what the planner may emit, not the raw provider constant</b> —
|
||||
/// 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 <c>IN ()</c>.</para>
|
||||
/// </summary>
|
||||
int MaxBindParameters { get; }
|
||||
|
||||
/// <summary>Cheapest statement that proves the connection is alive (<c>SELECT 1</c>; Oracle needs <c>FROM DUAL</c>).</summary>
|
||||
string LivenessSql { get; }
|
||||
|
||||
|
||||
@@ -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
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// <para><b>The unit is BYTES in the database character set, not characters</b> — see
|
||||
/// <see cref="QuoteIdentifier"/>. Practically that means UTF-8 (<c>AL32UTF8</c>) on any modern estate,
|
||||
/// so a 65-character Cyrillic name is already over the limit.</para>
|
||||
/// </summary>
|
||||
internal const int MaxIdentifierLength = 128;
|
||||
|
||||
@@ -99,6 +103,16 @@ public sealed class OracleDialect : ISqlDialect
|
||||
if (command is OracleCommand oracle) oracle.BindByName = true;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 1000 — <b>the tightest of the three ceilings, and the one that actually binds.</b> Oracle allows
|
||||
/// 64k bind variables per statement, but an <c>IN</c> <em>expression list</em> is limited to 1000
|
||||
/// entries (<c>ORA-01795</c>), and the key-value model's <c>IN (:p_k0, …)</c> 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.
|
||||
/// </summary>
|
||||
public int MaxBindParameters => 1000;
|
||||
|
||||
/// <summary>Oracle has no bare <c>SELECT</c>; every projection needs a table, and <c>DUAL</c> is the one-row one.</summary>
|
||||
public string LivenessSql => "SELECT 1 FROM DUAL";
|
||||
|
||||
@@ -127,10 +141,26 @@ public sealed class OracleDialect : ISqlDialect
|
||||
/// user), aliased to the <c>TABLE_SCHEMA</c> column name the catalog readers project by.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Broader than the SQL Server dialect's <c>DISTINCT TABLE_SCHEMA FROM INFORMATION_SCHEMA.TABLES</c>,
|
||||
/// which lists only schemas that own an object: <c>ALL_USERS</c> 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.
|
||||
/// <para>Broader than the SQL Server dialect's <c>DISTINCT TABLE_SCHEMA FROM
|
||||
/// INFORMATION_SCHEMA.TABLES</c>, which lists only schemas that own an object: <c>ALL_USERS</c>
|
||||
/// includes empty ones. Harmless for the <b>catalog gate</b> — 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.</para>
|
||||
/// <para>⚠️ <b>It is NOT harmless for the browse tree, and that is a known, accepted trade rather than
|
||||
/// an oversight (Driver.Sql.Dialect-003).</b> This query has two consumers, and only the gate wants the
|
||||
/// broad list: <c>SqlBrowseSession.RootAsync</c> renders every row as an expandable schema folder, so
|
||||
/// on a real Oracle estate the operator is offered every database <em>user</em> — hundreds of them,
|
||||
/// nearly all empty, with nothing in the tree distinguishing the ones that hold a relation until each
|
||||
/// is expanded.</para>
|
||||
/// <para><b>The narrower query is
|
||||
/// <c>SELECT DISTINCT owner FROM ALL_TABLES UNION SELECT DISTINCT owner FROM ALL_VIEWS</c></b>, which
|
||||
/// would match what the other two dialects list and what the browse tree wants. It is deliberately not
|
||||
/// taken here because it also <em>narrows the gate</em>: 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 (<c>OracleReadTests</c>), never offline, for the same reason the <c>p_</c> 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.</para>
|
||||
/// </remarks>
|
||||
public string ListSchemasSql =>
|
||||
"SELECT username AS TABLE_SCHEMA FROM ALL_USERS ORDER BY username";
|
||||
@@ -187,8 +217,14 @@ public sealed class OracleDialect : ISqlDialect
|
||||
/// (<c>ALL_TAB_COLUMNS.column_name</c>, i.e. what the server actually stored) before anything reaches
|
||||
/// this method — an operator who authored <c>speed</c> against an Oracle <c>SPEED</c> column resolves
|
||||
/// to <c>SPEED</c> at Initialize, and it is <c>SPEED</c> that gets quoted.</para>
|
||||
/// <para><b>The length ceiling is measured in UTF-8 bytes, not characters.</b> Oracle's 128 bounds the
|
||||
/// name in the <em>database character set</em> (<c>AL32UTF8</c> 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 <c>ORA-00972</c> rather than here, where the message can name the tag.
|
||||
/// <c>SqlServerDialect</c> deliberately does <b>not</b> do this: T-SQL's <c>sysname</c> is
|
||||
/// <c>nvarchar(128)</c>, a genuine 128-character limit.</para>
|
||||
/// <para><b>Rejects</b> (all as <see cref="ArgumentException"/>): null / empty / all-whitespace; longer
|
||||
/// than <see cref="MaxIdentifierLength"/>; any control character (which covers embedded NUL, and the
|
||||
/// than <see cref="MaxIdentifierLength"/> <b>bytes</b>; any control character (which covers embedded NUL, and the
|
||||
/// C0/C1 ranges that can truncate or corrupt a logged/audited statement); and any Unicode
|
||||
/// <see cref="UnicodeCategory.Format"/> character — bidi overrides, zero-width spaces, soft hyphen,
|
||||
/// BOM. Those last cannot escape the quotes, but they spoof the <em>rendered</em> 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)
|
||||
|
||||
@@ -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 — <c>NAMEDATALEN - 1</c>, 63 by default. Enforced because the
|
||||
/// server <b>silently truncates</b> anything longer, so a 70-character authored name would resolve to
|
||||
/// a different object than it spells; rejecting it surfaces the config mistake instead.
|
||||
/// <para><b>The unit is BYTES in the database encoding, not characters</b> — see
|
||||
/// <see cref="QuoteIdentifier"/>. Practically that means UTF-8 on any modern estate, so a 32-character
|
||||
/// Cyrillic name is already over the limit.</para>
|
||||
/// </summary>
|
||||
internal const int MaxIdentifierLength = 63;
|
||||
|
||||
@@ -60,6 +64,14 @@ public sealed class PostgresDialect : ISqlDialect
|
||||
// Intentionally empty: see the summary.
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 65535 — the extended-query protocol carries the parameter count as a signed/unsigned 16-bit field
|
||||
/// in <c>Bind</c>, 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".
|
||||
/// </summary>
|
||||
public int MaxBindParameters => 65535;
|
||||
|
||||
/// <inheritdoc/>
|
||||
public string LivenessSql => "SELECT 1";
|
||||
|
||||
@@ -149,12 +161,19 @@ public sealed class PostgresDialect : ISqlDialect
|
||||
/// <para><b>Rejects</b> (all as <see cref="ArgumentException"/>, mirroring
|
||||
/// <c>SqlServerDialect.QuoteIdentifier</c> so a test written against one dialect fails the same way
|
||||
/// against the other): null / empty / all-whitespace; longer than
|
||||
/// <see cref="MaxIdentifierLength"/>; any control character (which covers embedded NUL and the C0/C1
|
||||
/// <see cref="MaxIdentifierLength"/> <b>bytes</b>; any control character (which covers embedded NUL and the C0/C1
|
||||
/// ranges that can truncate or corrupt a logged/audited statement); and any Unicode
|
||||
/// <see cref="UnicodeCategory.Format"/> character — bidi overrides, zero-width spaces, soft hyphen,
|
||||
/// BOM. Those last cannot escape the quotes, but they spoof the <em>rendered</em> 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).</para>
|
||||
/// <para><b>The length ceiling is measured in UTF-8 bytes, not characters</b> — <c>NAMEDATALEN</c>
|
||||
/// bounds the stored name in the database encoding, and every modern estate is UTF-8. Counting UTF-16
|
||||
/// code units (<c>ident.Length</c>) would accept a 32-character Cyrillic name, which is 64 bytes and
|
||||
/// is therefore truncated server-side to a name that denotes a <em>different</em> object — precisely
|
||||
/// the silent-truncation failure this rule exists to prevent. <c>SqlServerDialect</c> deliberately does
|
||||
/// <b>not</b> do this: T-SQL's <c>sysname</c> is <c>nvarchar(128)</c>, a genuine 128-character limit,
|
||||
/// so counting characters is correct there and counting bytes would reject legal names.</para>
|
||||
/// <para>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.</para>
|
||||
@@ -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)
|
||||
|
||||
@@ -18,6 +18,10 @@ public sealed class SqlServerDialect : ISqlDialect
|
||||
/// T-SQL's regular-identifier ceiling (<c>sysname</c> = <c>nvarchar(128)</c>). 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.
|
||||
/// <para><b>The unit here is genuinely CHARACTERS</b> (UTF-16 code units, which is what
|
||||
/// <c>nvarchar(128)</c> counts), deliberately unlike <c>PostgresDialect</c> and <c>OracleDialect</c>,
|
||||
/// whose ceilings are byte limits in the database encoding. Measuring bytes here would reject a legal
|
||||
/// 128-character non-ASCII name.</para>
|
||||
/// </summary>
|
||||
internal const int MaxIdentifierLength = 128;
|
||||
|
||||
@@ -55,6 +59,15 @@ public sealed class SqlServerDialect : ISqlDialect
|
||||
// Intentionally empty: see the summary.
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 2098 — SQL Server's hard cap is <b>2100 parameters per statement</b>, and
|
||||
/// <c>Microsoft.Data.SqlClient</c> spends two of them itself: a parameterized command is executed
|
||||
/// through <c>sp_executesql</c>, whose first two arguments are the statement text and the parameter
|
||||
/// declaration list. A caller filling 2100 would therefore be rejected with
|
||||
/// <c>"The incoming request has too many parameters"</c> for a limit it appeared to respect.
|
||||
/// </summary>
|
||||
public int MaxBindParameters => 2098;
|
||||
|
||||
/// <inheritdoc/>
|
||||
public string LivenessSql => "SELECT 1";
|
||||
|
||||
|
||||
@@ -7,6 +7,9 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Sql;
|
||||
/// <summary>
|
||||
/// Folds a set of <see cref="SqlTagDefinition"/>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.
|
||||
/// <para><b>"Minimum" is bounded by the provider, not only by the group key.</b> A key-value group whose
|
||||
/// distinct keys exceed <see cref="ISqlDialect.MaxBindParameters"/> is emitted as several plans over the
|
||||
/// same group key rather than as one statement the backend refuses — see <see cref="AddKeyValuePlans"/>.</para>
|
||||
/// <para><b>The GroupKey is the correctness contract.</b> Two tags share a query <em>only</em> 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";
|
||||
|
||||
/// <summary>
|
||||
/// Groups <paramref name="tags"/> by source and emits one <see cref="SqlQueryPlan"/> per group.
|
||||
/// <para>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.</para>
|
||||
/// Groups <paramref name="tags"/> by source and emits one <see cref="SqlQueryPlan"/> per group — or
|
||||
/// <b>several</b>, when a key-value group holds more distinct keys than the dialect's
|
||||
/// <see cref="ISqlDialect.MaxBindParameters"/> allows in one command (see
|
||||
/// <see cref="AddKeyValuePlans"/>). Plans from one group share that group's key.
|
||||
/// <para>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.</para>
|
||||
/// </summary>
|
||||
/// <param name="tags">The tags to plan. An empty sequence yields no plans.</param>
|
||||
/// <param name="dialect">Supplies identifier quoting and the provider's row-limit syntax.</param>
|
||||
/// <returns>One plan per distinct group key.</returns>
|
||||
/// <param name="dialect">
|
||||
/// Supplies identifier quoting, the provider's row-limit syntax, and its per-command bind-parameter
|
||||
/// ceiling.
|
||||
/// </param>
|
||||
/// <returns>At least one plan per distinct group key; more when a key-value group had to be chunked.</returns>
|
||||
/// <exception cref="ArgumentNullException"><paramref name="tags"/> or <paramref name="dialect"/> is null.</exception>
|
||||
/// <exception cref="ArgumentException">
|
||||
/// 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
|
||||
};
|
||||
|
||||
/// <summary>
|
||||
/// Emits <c>SELECT <key>, <value>[, <ts>] FROM <table> WHERE <key> IN (@k0..@kN)</c>.
|
||||
/// The key column is selected because the reader indexes rows by it to slice values back per member.
|
||||
/// Emits one key-value plan per <b>chunk</b> of the group's distinct keys, sized by
|
||||
/// <see cref="ISqlDialect.MaxBindParameters"/>. A group at or under the ceiling — every ordinary group
|
||||
/// — still yields exactly one plan.
|
||||
/// <para><b>Why chunk rather than let the provider refuse.</b> Every backend caps parameters per
|
||||
/// command (2100 on SQL Server, 1000 <c>IN</c>-list entries on Oracle), and a key-value (EAV) staging
|
||||
/// table is precisely the shape that attracts thousands of tags on <em>one</em> table — which the
|
||||
/// group-key rule folds into one group. Over the cap the provider rejects the statement <em>after</em>
|
||||
/// the connection opened, so every tag in the group publishes <c>BadCommunicationError</c> on every
|
||||
/// poll, permanently, and the driver reports a communication fault against a database that is
|
||||
/// answering (Driver.Sql-002).</para>
|
||||
/// <para><b>Chunks share the group key, and no consumer needs to change.</b> The reader executes one
|
||||
/// command per plan and slices each plan's own <see cref="SqlQueryPlan.Members"/> 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.</para>
|
||||
/// <para><b>Members follow their key, never their position.</b> Two tags authored on the same
|
||||
/// <c>keyValue</c> 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
|
||||
/// <c>BadNoData</c> forever.</para>
|
||||
/// </summary>
|
||||
private static void AddKeyValuePlans(
|
||||
List<SqlQueryPlan> plans, object groupKey, List<SqlTagDefinition> 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<string, int>(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<SqlTagDefinition>[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));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Emits <c>SELECT <key>, <value>[, <ts>] FROM <table> WHERE <key> IN (@k0..@kN)</c>
|
||||
/// for one chunk of a group. The key column is selected because the reader indexes rows by it to slice
|
||||
/// values back per member.
|
||||
/// <para>Keys are de-duplicated (ordinal) before binding: two tags authored against the same
|
||||
/// <c>keyValue</c> bind one parameter but stay two members, so both nodes are fed from the one row.</para>
|
||||
/// <para>Markers are numbered from <c>k0</c> within the chunk — each chunk is its own command, so
|
||||
/// carrying a group-wide index forward would leave a three-parameter command spelling
|
||||
/// <c>IN (@k3, @k4, @k5)</c>.</para>
|
||||
/// </summary>
|
||||
private static SqlQueryPlan BuildKeyValuePlan(
|
||||
object groupKey, List<SqlTagDefinition> members, ISqlDialect dialect)
|
||||
|
||||
@@ -27,6 +27,9 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Sql;
|
||||
/// <param name="GroupKey">
|
||||
/// 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.
|
||||
/// <para><b>Not unique across plans.</b> A key-value group holding more distinct keys than the dialect's
|
||||
/// <see cref="ISqlDialect.MaxBindParameters"/> is emitted as several plans carrying <em>this same key</em>
|
||||
/// — the chunking is an execution detail of one group, not a regrouping.</para>
|
||||
/// </param>
|
||||
/// <param name="SqlText">The single parameterized statement to execute for this group.</param>
|
||||
/// <param name="ParameterNames">
|
||||
|
||||
@@ -65,6 +65,9 @@ public sealed class ColonMarkerDialect : ISqlDialect
|
||||
/// <inheritdoc/>
|
||||
public DbProviderFactory Factory => Inner.Factory;
|
||||
|
||||
/// <inheritdoc/>
|
||||
public int MaxBindParameters => Inner.MaxBindParameters;
|
||||
|
||||
/// <inheritdoc/>
|
||||
public string LivenessSql => Inner.LivenessSql;
|
||||
|
||||
|
||||
@@ -165,13 +165,45 @@ public class OracleDialectTests
|
||||
=> Should.Throw<ArgumentException>(() => 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<ArgumentException>(() => d.QuoteIdentifier(new string('a', 129)));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Oracle 12.2's 128 is a <b>byte</b> 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
|
||||
/// <c>ORA-00972</c>. Measuring UTF-16 code units here would pass it through to fail at the server
|
||||
/// instead — for a reason this dialect could have named.
|
||||
/// </summary>
|
||||
[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<ArgumentException>(() => d.QuoteIdentifier(new string('б', 65)));
|
||||
}
|
||||
|
||||
// ---- bind-parameter ceiling ----
|
||||
|
||||
/// <summary>
|
||||
/// 1000 is the <c>IN</c>-expression-list limit (<c>ORA-01795</c>), NOT Oracle's much larger 64k bind
|
||||
/// count — the seam has to report the limit the planner's <c>IN (:p_k0, …)</c> list actually hits, or
|
||||
/// it would chunk to a size the server still refuses.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void MaxBindParameters_isTheInListLimit_notTheLargerBindVariableCount()
|
||||
{
|
||||
new OracleDialect().MaxBindParameters.ShouldBe(1000);
|
||||
new OracleDialect().MaxBindParameters.ShouldBeLessThan(new PostgresDialect().MaxBindParameters);
|
||||
}
|
||||
|
||||
// ---- liveness + row limit ----
|
||||
|
||||
[Fact]
|
||||
|
||||
@@ -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<ArgumentException>(() => d.QuoteIdentifier(new string('a', 64)));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// <c>NAMEDATALEN - 1</c> is 63 <b>bytes in the database encoding</b>, 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.
|
||||
/// </summary>
|
||||
[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<ArgumentException>(() => d.QuoteIdentifier(new string('б', 32)));
|
||||
}
|
||||
|
||||
/// <summary>A non-BMP character is 4 UTF-8 bytes (and 2 UTF-16 code units) — 16 of them fill the limit.</summary>
|
||||
[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<ArgumentException>(
|
||||
() => 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");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 65535 — the <c>Bind</c> 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".
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public void MaxBindParameters_isTheWireProtocolsSixteenBitCount()
|
||||
=> new PostgresDialect().MaxBindParameters.ShouldBe(65535);
|
||||
|
||||
[Fact]
|
||||
public void Dialect_identifiesItselfAsPostgres_andExposesTheAbstractFactory()
|
||||
{
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// Pins the bind-parameter ceiling (Driver.Sql-002 / Driver.Sql.Dialect-002): a key-value group holding
|
||||
/// more distinct <c>keyValue</c>s than the dialect's <see cref="ISqlDialect.MaxBindParameters"/> is split
|
||||
/// into several plans over the <b>same</b> group key, rather than emitting one statement the provider
|
||||
/// rejects.
|
||||
/// <para><b>Why this is a correctness test and not a tuning one.</b> Over the ceiling the provider refuses
|
||||
/// the statement <em>after</em> the connection opened, so every tag in the group publishes
|
||||
/// <c>BadCommunicationError</c> 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.</para>
|
||||
/// <para>A single group emitting several plans needs <b>no reader change</b>: <c>SqlPollReader</c> runs
|
||||
/// one command per plan and slices each plan's own <c>Members</c> back, so the last test here drives the
|
||||
/// whole read path over a real database to prove exactly that.</para>
|
||||
/// </summary>
|
||||
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 ----
|
||||
|
||||
/// <summary>
|
||||
/// Oracle's real ceiling, on the real dialect: an <c>IN</c> 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.
|
||||
/// </summary>
|
||||
[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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The same for SQL Server, whose 2100-parameter command cap is the other limit the finding names.
|
||||
/// </summary>
|
||||
[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))));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Every member must land in the chunk that binds <em>its own</em> key — a member whose key is bound
|
||||
/// by a different plan reads a result set that cannot contain its row and publishes
|
||||
/// <c>BadNoData</c> forever, which is the quiet way to get this wrong.
|
||||
/// </summary>
|
||||
[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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Two tags on one <c>keyValue</c> 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.
|
||||
/// </summary>
|
||||
[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" });
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Each chunk is its own command, so its markers restart at <c>k0</c> — carrying the global index
|
||||
/// forward would emit <c>IN (@k3, @k4, @k5)</c> against a three-parameter command, which is legal but
|
||||
/// makes a plan's SQL unreadable next to its own parameter list.
|
||||
/// </summary>
|
||||
[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) + ")");
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>All chunks of one group carry that group's key — the split is an execution detail, not a regrouping.</summary>
|
||||
[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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
[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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
[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"]);
|
||||
}
|
||||
|
||||
/// <summary>A wide-row plan binds at most one value, so a ceiling of 1 must not disturb it.</summary>
|
||||
[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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A dialect reporting a ceiling below 1 cannot be chunked to — it must fail loudly rather than loop
|
||||
/// forever or emit an empty <c>IN ()</c> list, which is a syntax error on every backend.
|
||||
/// </summary>
|
||||
[Theory]
|
||||
[InlineData(0)]
|
||||
[InlineData(-1)]
|
||||
public void ADialectReportingANonPositiveCeiling_isRejected(int ceiling)
|
||||
=> Should.Throw<ArgumentException>(
|
||||
() => SqlGroupPlanner.Plan([KvTag("A", "K1")], new BindCeilingDialect(ceiling)));
|
||||
|
||||
// ---- the whole read path, over a real database ----
|
||||
|
||||
/// <summary>
|
||||
/// Drives <see cref="SqlPollReader"/> 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 <b>no reader change</b> — the claim the chunking design rests on.
|
||||
/// </summary>
|
||||
[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);
|
||||
|
||||
/// <summary>
|
||||
/// A <b>test-only</b> dialect that reports a caller-chosen <see cref="ISqlDialect.MaxBindParameters"/>
|
||||
/// and delegates everything else to <see cref="SqliteDialect"/> — so a chunked plan can be asserted on
|
||||
/// at a readable size <em>and</em> actually executed against the fixture database.
|
||||
/// <para>A decorator rather than a subclass, for the reason the other fakes here give:
|
||||
/// <see cref="SqliteDialect"/> is sealed, and <c>new</c>-hiding a member would leave interface dispatch
|
||||
/// calling the original, so the substitution would silently not happen.</para>
|
||||
/// </summary>
|
||||
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);
|
||||
}
|
||||
}
|
||||
@@ -84,6 +84,18 @@ public class SqlServerDialectTests
|
||||
.ShouldBe("SELECT TOP 1 [a]");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 2098, not 2100: <c>Microsoft.Data.SqlClient</c> routes a parameterized command through
|
||||
/// <c>sp_executesql</c>, 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.
|
||||
/// </summary>
|
||||
[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<ArgumentException>(() => d.QuoteIdentifier(new string('a', 129)));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The falsifiability control for the Postgres/Oracle byte ceilings: T-SQL's <c>sysname</c> is
|
||||
/// <c>nvarchar(128)</c>, a genuine <b>128-character</b> (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.
|
||||
/// </summary>
|
||||
[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()
|
||||
{
|
||||
|
||||
@@ -64,6 +64,13 @@ public sealed class SqliteDialect : ISqlDialect
|
||||
// Intentionally empty: see the summary.
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 999 — <c>SQLITE_MAX_VARIABLE_NUMBER</c>'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.
|
||||
/// </summary>
|
||||
public int MaxBindParameters => 999;
|
||||
|
||||
/// <inheritdoc/>
|
||||
public string LivenessSql => "SELECT 1";
|
||||
|
||||
|
||||
Reference in New Issue
Block a user