From 74594e6083ed91ffa790c56d9e847ca8e2a6b59b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 07:58:04 -0400 Subject: [PATCH 1/4] fix(sql-dialect): enforce Postgres/Oracle identifier ceilings in UTF-8 bytes (Driver.Sql.Dialect-001) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NAMEDATALEN-1 (63) and Oracle 12.2's 128 are byte limits in the database encoding, not character counts, so a 32-character Cyrillic name passed ident.Length and was then silently truncated (Postgres) or rejected by the server (Oracle). Both dialects now measure Encoding.UTF8.GetByteCount and say 'bytes' in the rejection. SqlServerDialect deliberately keeps ident.Length — sysname is nvarchar(128), a real character limit — pinned by a control test. --- .../OracleDialect.cs | 20 ++++++++++-- .../PostgresDialect.cs | 21 ++++++++++-- .../SqlServerDialect.cs | 4 +++ .../OracleDialectTests.cs | 20 +++++++++++- .../PostgresDialectTests.cs | 32 ++++++++++++++++++- .../SqlServerDialectTests.cs | 13 ++++++++ 6 files changed, 102 insertions(+), 8 deletions(-) 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..8d12a01c 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; @@ -187,8 +191,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 +219,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..d2221f3b 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; @@ -149,12 +153,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 +186,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..b1dd602f 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; 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..1fbe2bfd 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,31 @@ 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))); + } + // ---- 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..d68f0594 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() { 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..8545d423 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 @@ -117,6 +117,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() { From 886ce1b87cdd72b8f4f257a97f1dbcc8f87c9f66 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 08:02:44 -0400 Subject: [PATCH 2/4] fix(sql): chunk the key-value IN list at the dialect's bind ceiling (Driver.Sql-002, Driver.Sql.Dialect-002) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ISqlDialect gains MaxBindParameters — the per-command ceiling the seam was missing while modelling everything else that differs per backend: 2098 usable of SQL Server's 2100 (sp_executesql spends two), 1000 for Oracle's IN-list limit (ORA-01795, tighter than its 64k bind count), 65535 for the Postgres Bind message. SqlGroupPlanner.AddKeyValuePlans now emits several plans for one group when its distinct keys exceed that ceiling, instead of one statement the provider refuses after the connection opened — which left every tag in an EAV group publishing BadCommunicationError on every poll, permanently, and reported the database endpoint as down. Chunks share the group key; members follow their own key so a shared keyValue never splits. SqlPollReader is unchanged: it runs one command per plan and slices each plan's own Members back, proven by a fixture read at a ceiling of 1. --- .../ISqlDialect.cs | 25 +- .../OracleDialect.cs | 10 + .../PostgresDialect.cs | 8 + .../SqlServerDialect.cs | 9 + .../SqlGroupPlanner.cs | 103 +++++- .../SqlQueryPlan.cs | 3 + .../ColonMarkerDialect.cs | 3 + .../OracleDialectTests.cs | 14 + .../PostgresDialectTests.cs | 9 + .../SqlCatalogDefaultSchemaTests.cs | 2 + .../SqlCatalogGateDriverTests.cs | 2 + .../SqlGroupPlannerChunkingTests.cs | 318 ++++++++++++++++++ .../SqlServerDialectTests.cs | 12 + .../SqliteDialect.cs | 7 + 14 files changed, 509 insertions(+), 16 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlGroupPlannerChunkingTests.cs 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 8d12a01c..9bfa6ae9 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 @@ -103,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"; 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 d2221f3b..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 @@ -64,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"; 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 b1dd602f..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 @@ -59,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 1fbe2bfd..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 @@ -190,6 +190,20 @@ public class OracleDialectTests 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 d68f0594..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 @@ -219,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 8545d423..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] 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"; From 401d670639f6acc78601d95e7bd3949bcd625ddb Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 08:03:13 -0400 Subject: [PATCH 3/4] docs(sql-dialect): record the browse consequence of Oracle's ALL_USERS schema list (Driver.Sql.Dialect-003) ListSchemasSql has two consumers and the remarks only justified the divergence from the catalog gate's viewpoint. SqlBrowseSession.RootAsync renders every row as an expandable schema, so an Oracle browse offers every database user. The narrower ALL_TABLES/ALL_VIEWS query is named, along with why it is not taken here: it narrows the gate too, and that is a poll-path behaviour change only a real Oracle server can gate. --- .../OracleDialect.cs | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) 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 9bfa6ae9..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 @@ -141,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"; From 1fe00176009bca72feab5c2f36c0c4409ace03f0 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 10 Aug 2026 08:04:40 -0400 Subject: [PATCH 4/4] docs(code-review): close Driver.Sql-002 + Driver.Sql.Dialect-001/002/003 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dialect header 3 -> 0 open. The Driver.Sql header count is left alone for the orchestrator to reconcile — sibling agents are editing that file in parallel. --- code-reviews/Driver.Sql.Dialect/findings.md | 16 +++++++++------- code-reviews/Driver.Sql/findings.md | 10 ++++++++-- 2 files changed, 17 insertions(+), 9 deletions(-) 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 a17eb89a..24047fca 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. ---