diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs index ff397fcd..6cc9236b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser/SqlBrowseSession.cs @@ -12,8 +12,10 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser; /// Every level is the dialect's catalog SQL ( / /// / ) — nothing here /// knows what INFORMATION_SCHEMA is, because Oracle and SQLite do not have it. The catalog SQL is -/// read verbatim and @schema / @table are bound as parameters; no name from a NodeId -/// is ever concatenated into a command text, so a hostile or merely awkward catalog name is inert here. +/// read verbatim and its schema / table parameters are bound as parameters under +/// — so the bind names cannot drift from the markers the +/// dialect's own SQL carries; no name from a NodeId is ever concatenated into a command text, so a +/// hostile or merely awkward catalog name is inert here. /// is deliberately not used on this path — the browse /// never needs an identifier in text. /// Connection ownership: this session owns the connection it is handed and closes it on @@ -47,6 +49,12 @@ internal sealed class SqlBrowseSession : IBrowseSession /// The TABLE_TYPE value marking a view rather than a base table. private const string ViewTableType = "VIEW"; + /// Base name of the schema parameter every dialect's catalog SQL carries. + private const string SchemaParameter = "schema"; + + /// Base name of the table parameter carries. + private const string TableParameter = "table"; + /// /// The security class every browsed column reports. The Sql driver is read-only in v1 /// (design §4.3), so there is nothing else a picked column could be. @@ -195,7 +203,7 @@ internal sealed class SqlBrowseSession : IBrowseSession { var rows = await QueryAsync( _dialect.ListTablesSql, - command => Bind(command, "@schema", schema), + command => Bind(command, _dialect.ParameterBindName(SchemaParameter), schema), static reader => ( Name: ReadString(reader, TableNameColumn), Type: ReadOptionalString(reader, TableTypeColumn)), @@ -248,8 +256,8 @@ internal sealed class SqlBrowseSession : IBrowseSession _dialect.ListColumnsSql, command => { - Bind(command, "@schema", schema); - Bind(command, "@table", table); + Bind(command, _dialect.ParameterBindName(SchemaParameter), schema); + Bind(command, _dialect.ParameterBindName(TableParameter), table); }, static reader => ( Name: ReadString(reader, ColumnNameColumn), @@ -275,6 +283,9 @@ internal sealed class SqlBrowseSession : IBrowseSession var results = new List(); await using var command = _connection.CreateCommand(); + // Before the binds: on ODP.NET this is what switches bind-by-position off, and the two-parameter + // column query is exactly where positional binding swaps the schema and the table silently. + _dialect.ConfigureCommand(command); command.CommandText = sql; bind(command); 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 31dae6ef..35096a4c 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 @@ -50,6 +50,58 @@ public interface ISqlDialect /// The value cannot be a valid catalog identifier. string QuoteIdentifier(string ident); + /// + /// The token that stands for a bound parameter in the command text — "@k0" for + /// SqlClient/Npgsql, ":k0" for ODP.NET — given the caller's bare base name ("k0", + /// "w", "schema"). + /// Why the marker and the bind name are two members. They are the same string on every + /// provider but Oracle, where the SQL text carries :schema while + /// is the bare schema. Modelling one and deriving the + /// other means a call site guessing at a convention it does not own — the exact defect this seam + /// exists to prevent, and one that surfaces as a runtime bind failure (or, worse under + /// bind-by-position, as a silently mis-bound value) rather than a compile error. + /// The base name is caller-owned and never operator input: the poll planner mints + /// k0…kN/w, the catalog path uses schema/table. Authored values + /// ride the parameter; authored identifiers go through . Nothing + /// else reaches a command text. + /// + /// The bare base name, without any provider prefix. + /// The marker to embed in the SQL text. + string ParameterMarker(string baseName); + + /// + /// The value to assign to for the same + /// — "@k0" for SqlClient-style providers (which expect the marker + /// verbatim), the bare "k0" for ODP.NET-style ones (whose documented convention is a + /// prefix-less bind name). + /// Always paired with over the same base name: a plan + /// carries both lists and every binding site copies them index-for-index rather than + /// re-deriving either from the other. + /// + /// The bare base name, without any provider prefix. + /// The parameter name to bind under. + string ParameterBindName(string baseName); + + /// + /// Applies provider-specific setup to a freshly created , before its + /// parameters are added or it is executed. A no-op for providers that need none. + /// Oracle uses this to set OracleCommand.BindByName = true, and that call is the + /// sharpest correctness edge on this seam: ODP.NET binds by position by default, so a + /// multi-parameter statement (the planner's IN (:k0, :k1, …) list, the catalog path's + /// schema + table pair) would bind values to the wrong markers whenever the parameter + /// order and the marker order differ — publishing one tag's value onto another tag's node with no + /// error anywhere. Every command that carries parameters — the poll path, the catalog path, the + /// browse path — therefore calls this immediately after CreateCommand() and before the + /// first parameter is added. (Bind mode cannot affect a parameterless statement, so the liveness + /// ones are indifferent: SqlDriverProbe calls it anyway for uniformity, + /// SqlDriver.PingAsync does not.) + /// Implementations must tolerate any they are handed: the + /// driver can be pointed at an instrumented or substituted factory, so a dialect that casts to its own + /// provider type must do so defensively rather than throwing. + /// + /// The command to configure. + void ConfigureCommand(DbCommand command); + /// Cheapest statement that proves the connection is alive (SELECT 1; Oracle needs FROM DUAL). string LivenessSql { get; } @@ -105,10 +157,19 @@ public interface ISqlDialect /// string DefaultSchemaSql { get; } - /// Catalog query listing tables + views in one schema — the browser's schema-expand level. Bind @schema. + /// + /// Catalog query listing tables + views in one schema — the browser's schema-expand level. Carries one + /// parameter under the base name schema, spelled with this dialect's own + /// (@schema on SqlClient, :schema on ODP.NET); callers bind + /// it under of the same base name. + /// string ListTablesSql { get; } - /// Catalog query listing columns of one table — the browser's table-expand / attributes level. Bind @schema and @table. + /// + /// Catalog query listing columns of one table — the browser's table-expand / attributes level. Carries + /// parameters under the base names schema and table, spelled and bound exactly as + /// describes. + /// string ListColumnsSql { get; } /// 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 1f9b4c5b..2bbb1a0a 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 @@ -27,6 +27,34 @@ public sealed class SqlServerDialect : ISqlDialect /// public DbProviderFactory Factory => SqlClientFactory.Instance; + /// + /// T-SQL markers are @-prefixed — ParameterMarker("k0") is "@k0". + /// + /// The bare base name. + /// The @-prefixed marker. + public string ParameterMarker(string baseName) => "@" + baseName; + + /// + /// Identical to : Microsoft.Data.SqlClient expects + /// to carry the same @ the statement does. (It also + /// accepts a bare name and prepends the @ itself, but spelling it out keeps the parameter + /// collection readable in a profiler trace and matches what the SQL text says.) + /// + /// The bare base name. + /// The @-prefixed bind name. + public string ParameterBindName(string baseName) => "@" + baseName; + + /// + /// No-op. SqlCommand binds by name out of the box, so there is nothing SQL Server needs set on a + /// fresh command — the member exists for Oracle's BindByName (see + /// ). + /// + /// The command to configure; untouched here. + public void ConfigureCommand(DbCommand command) + { + // Intentionally empty: see the summary. + } + /// public string LivenessSql => "SELECT 1"; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlCatalogLoader.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlCatalogLoader.cs index f0802333..289163c2 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlCatalogLoader.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlCatalogLoader.cs @@ -14,8 +14,11 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Sql; /// tables issues a handful of round-trips at Initialize and none thereafter; it never enumerates a /// warehouse. /// Every parameter is bound. Authored names reach the catalog queries as -/// @schema/@table parameters — the same discipline the schema browser follows — so loading -/// the allow-list cannot itself be an injection vector. No identifier is quoted into text on this path. +/// schema/table parameters — the same discipline the schema browser follows — so loading +/// the allow-list cannot itself be an injection vector. No identifier is quoted into text on this path. +/// The bind names come from over those two base names, so +/// they can never disagree with the markers in the dialect's own catalog SQL (@schema on SQL +/// Server, :schema on Oracle). /// Failures throw; they do not degrade to an empty catalog. An empty catalog would reject /// every tag, which is indistinguishable at the OPC UA surface from a database whose objects were all /// dropped. A caller that cannot read the catalog has not learned that the tags are invalid — it has @@ -33,6 +36,12 @@ internal static class SqlCatalogLoader /// Column alias projects. private const string ColumnNameColumn = "COLUMN_NAME"; + /// Base name of the schema parameter every dialect's catalog SQL carries. + private const string SchemaParameter = "schema"; + + /// Base name of the table parameter carries. + private const string TableParameter = "table"; + /// /// Reads the catalog slice covering . /// @@ -53,7 +62,7 @@ internal static class SqlCatalogLoader var timeoutSeconds = Math.Max(1, (int)Math.Ceiling(commandTimeout.TotalSeconds)); var schemas = await QueryColumnAsync( - connection, dialect.ListSchemasSql, SchemaColumn, + connection, dialect, dialect.ListSchemasSql, SchemaColumn, static _ => { }, timeoutSeconds, cancellationToken).ConfigureAwait(false); // Zero schemas is a permissions or visibility symptom, never a real database. Rejecting every tag on @@ -94,8 +103,8 @@ internal static class SqlCatalogLoader if (!tablesBySchema.TryGetValue(schema, out var tables)) { tables = await QueryColumnAsync( - connection, dialect.ListTablesSql, TableNameColumn, - command => Bind(command, "@schema", schema), + connection, dialect, dialect.ListTablesSql, TableNameColumn, + command => Bind(command, dialect.ParameterBindName(SchemaParameter), schema), timeoutSeconds, cancellationToken).ConfigureAwait(false); tablesBySchema[schema] = tables; } @@ -106,11 +115,11 @@ internal static class SqlCatalogLoader if (columnsByTable.ContainsKey(key)) continue; columnsByTable[key] = await QueryColumnAsync( - connection, dialect.ListColumnsSql, ColumnNameColumn, + connection, dialect, dialect.ListColumnsSql, ColumnNameColumn, command => { - Bind(command, "@schema", schema); - Bind(command, "@table", table); + Bind(command, dialect.ParameterBindName(SchemaParameter), schema); + Bind(command, dialect.ParameterBindName(TableParameter), table); }, timeoutSeconds, cancellationToken).ConfigureAwait(false); } @@ -133,6 +142,7 @@ internal static class SqlCatalogLoader var command = connection.CreateCommand(); await using (command.ConfigureAwait(false)) { + dialect.ConfigureCommand(command); command.CommandText = dialect.DefaultSchemaSql; command.CommandTimeout = timeoutSeconds; var value = await command.ExecuteScalarAsync(cancellationToken).ConfigureAwait(false); @@ -144,6 +154,7 @@ internal static class SqlCatalogLoader /// Runs one catalog query and projects a single string column from every row. private static async Task> QueryColumnAsync( DbConnection connection, + ISqlDialect dialect, string sql, string columnName, Action bind, @@ -155,6 +166,8 @@ internal static class SqlCatalogLoader var command = connection.CreateCommand(); await using (command.ConfigureAwait(false)) { + // Before the binds: ODP.NET's BindByName has to be set while the parameter collection is empty. + dialect.ConfigureCommand(command); command.CommandText = sql; command.CommandTimeout = timeoutSeconds; bind(command); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverProbe.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverProbe.cs index 79c5b986..95222661 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverProbe.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverProbe.cs @@ -134,6 +134,9 @@ public sealed class SqlDriverProbe : IDriverProbe var command = connection.CreateCommand(); await using (command.ConfigureAwait(false)) { + // Uniformly applied at every command site, even where this statement takes no + // parameters — a provider hook that is only *sometimes* honoured is the harder bug. + dialect.ConfigureCommand(command); command.CommandText = dialect.LivenessSql; await command.ExecuteScalarAsync(cts.Token).ConfigureAwait(false); } 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 73510d1d..a6aa0854 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlGroupPlanner.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlGroupPlanner.cs @@ -13,16 +13,22 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Sql; /// SqlGroupPlannerTests. /// Injection boundary (design §8.1). Identifiers reach the command text through /// and nothing else; every authored value becomes a -/// @k0/@w marker with the value carried in . There is no -/// interpolation of tag content into SQL anywhere in this type. +/// k0/w parameter — spelled with the dialect's own +/// (@k0 on SQL Server, :k0 on Oracle) — with the value carried in +/// . There is no interpolation of tag content into SQL anywhere in +/// this type. +/// The base names are this planner's, the spelling is the dialect's. Each plan carries the +/// markers it emitted and the matching s, so the +/// reader binds what the planner minted instead of re-deriving a provider convention — the two differ on +/// ODP.NET (:k0 in the text, bare k0 on the parameter). /// public static class SqlGroupPlanner { - /// The parameter-marker prefix for a key-value model's IN list. - private const string KeyMarkerPrefix = "@k"; + /// The base-name prefix for a key-value model's IN-list parameters (k0…kN). + private const string KeyBaseNamePrefix = "k"; - /// The single parameter marker for a wide-row model's row selector. - private const string RowSelectorMarker = "@w"; + /// The single parameter base name for a wide-row model's row selector. + private const string RowSelectorBaseName = "w"; /// /// Groups by source and emits one per group. @@ -132,7 +138,8 @@ public static class SqlGroupPlanner if (timestampColumn is not null) AddDistinct(selected, timestampColumn); // A keyValue may legitimately be the empty string, so only null is a broken invariant. - var names = new List(members.Count); + var markers = new List(members.Count); + var bindNames = new List(members.Count); var values = new List(members.Count); var seen = new HashSet(StringComparer.Ordinal); foreach (var member in members) @@ -140,7 +147,9 @@ public static class SqlGroupPlanner var keyValue = member.KeyValue ?? throw MissingField(nameof(SqlTagDefinition.KeyValue), member); if (!seen.Add(keyValue)) continue; - names.Add(KeyMarkerPrefix + values.Count.ToString(CultureInfo.InvariantCulture)); + var baseName = KeyBaseNamePrefix + values.Count.ToString(CultureInfo.InvariantCulture); + markers.Add(dialect.ParameterMarker(baseName)); + bindNames.Add(dialect.ParameterBindName(baseName)); values.Add(keyValue); } @@ -148,10 +157,11 @@ public static class SqlGroupPlanner .Append(QuoteList(selected, dialect)) .Append(" FROM ").Append(QuoteQualifiedName(first.Table, dialect)) .Append(" WHERE ").Append(dialect.QuoteIdentifier(keyColumn)) - .Append(" IN (").AppendJoin(", ", names).Append(')') + .Append(" IN (").AppendJoin(", ", markers).Append(')') .ToString(); - return new SqlQueryPlan(SqlTagModel.KeyValue, groupKey, sql, names, values, members, selected); + return new SqlQueryPlan( + SqlTagModel.KeyValue, groupKey, sql, markers, bindNames, values, members, selected); } /// @@ -183,12 +193,14 @@ public static class SqlGroupPlanner // A where-pair wins over topByTimestamp; the parser already guarantees exactly one is populated. if (!Blank(first.RowSelectorColumn) && first.RowSelectorValue is not null) { + var marker = dialect.ParameterMarker(RowSelectorBaseName); var sql = string.Concat( "SELECT ", columns, " FROM ", table, - " WHERE ", dialect.QuoteIdentifier(first.RowSelectorColumn!), " = ", RowSelectorMarker); + " WHERE ", dialect.QuoteIdentifier(first.RowSelectorColumn!), " = ", marker); return new SqlQueryPlan( SqlTagModel.WideRow, groupKey, sql, - [RowSelectorMarker], [first.RowSelectorValue], members, selected); + [marker], [dialect.ParameterBindName(RowSelectorBaseName)], + [first.RowSelectorValue], members, selected); } if (!Blank(first.RowSelectorTopByTimestamp)) @@ -200,7 +212,7 @@ public static class SqlGroupPlanner "SELECT ", dialect.SingleRowLimitPrefix, columns, " FROM ", table, " ORDER BY ", dialect.QuoteIdentifier(first.RowSelectorTopByTimestamp!), " DESC", dialect.SingleRowLimitSuffix); - return new SqlQueryPlan(SqlTagModel.WideRow, groupKey, sql, [], [], members, selected); + return new SqlQueryPlan(SqlTagModel.WideRow, groupKey, sql, [], [], [], members, selected); } throw new ArgumentException( diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs index ee1dce0f..a979f59a 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlPollReader.cs @@ -393,13 +393,17 @@ public sealed class SqlPollReader var command = connection.CreateCommand(); await using (command.ConfigureAwait(false)) { + // Before any parameter is added: on ODP.NET this is what turns bind-by-position off, and + // a positionally-bound IN list feeds one tag's value to another tag's node in silence. + _dialect.ConfigureCommand(command); command.CommandText = plan.SqlText; command.CommandTimeout = _commandTimeoutSeconds; - for (var i = 0; i < plan.ParameterNames.Count; i++) + for (var i = 0; i < plan.ParameterBindNames.Count; i++) { var parameter = command.CreateParameter(); - // Bound by the plan's own index pairing — never by re-deriving the marker convention. - parameter.ParameterName = plan.ParameterNames[i]; + // Bound by the plan's own index pairing — never by re-deriving the marker convention, + // and never from ParameterNames: the marker and the bind name differ on ODP.NET. + parameter.ParameterName = plan.ParameterBindNames[i]; parameter.Value = plan.Parameters[i] ?? DBNull.Value; command.Parameters.Add(parameter); } 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 f586bbf6..0ba78717 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlQueryPlan.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlQueryPlan.cs @@ -9,8 +9,12 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Sql; /// per group rather than one query per tag. /// Injection boundary. is built from dialect-quoted identifiers only; /// every authored value lives in and is bound as a -/// named by the positionally-aligned . There is no -/// path by which a tag's keyValue or whereValue reaches the command text. +/// named by the positionally-aligned . There is +/// no path by which a tag's keyValue or whereValue reaches the command text. +/// Markers and bind names are carried separately because they are not the same string on +/// every provider: ODP.NET's text says :k0 while its is the +/// bare k0. Both lists are positionally aligned with , so a binding site +/// copies the pairing the planner minted rather than re-deriving a provider convention it does not own. /// Produced only by ; consumed by the poll reader, which executes /// once and slices the rows back to per-member snapshots. /// @@ -30,9 +34,15 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Sql; /// with . Carried explicitly so the reader never has to re-derive the naming /// convention. /// +/// +/// The to bind each value under, from +/// , aligned index-for-index with and +/// with . Equal to the marker on every provider but ODP.NET, which wants the +/// bare name — which is exactly why it is a carried list and not a transformation of the marker. +/// /// -/// The values to bind, aligned with . Captured verbatim from the authored tags -/// — a hostile value is inert here because it is data, not text. +/// The values to bind, aligned with and . +/// Captured verbatim from the authored tags — a hostile value is inert here because it is data, not text. /// /// /// The tags this plan feeds, in the planner's input order. Never empty. Duplicates are preserved: two tags @@ -47,6 +57,7 @@ public sealed record SqlQueryPlan( object GroupKey, string SqlText, IReadOnlyList ParameterNames, + IReadOnlyList ParameterBindNames, IReadOnlyList Parameters, IReadOnlyList Members, IReadOnlyList SelectedColumns) @@ -57,6 +68,9 @@ public sealed record SqlQueryPlan( /// The parameter markers in , aligned with . public IReadOnlyList ParameterNames { get; } = [.. ParameterNames]; + /// The parameter names to bind under, aligned with . + public IReadOnlyList ParameterBindNames { get; } = [.. ParameterBindNames]; + /// The values to bind, aligned with . public IReadOnlyList Parameters { get; } = [.. Parameters]; diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqlBrowseSessionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqlBrowseSessionTests.cs index d75ec6ec..0e24499c 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqlBrowseSessionTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/SqlBrowseSessionTests.cs @@ -371,4 +371,31 @@ public sealed class SqlBrowseSessionTests await session.AttributesAsync(columnNodeId, CancellationToken.None); session.LastUsedUtc.ShouldBeGreaterThan(afterExpand); } + + /// + /// The browse path binds the dialect's parameter names, not a hardcoded "@schema". + /// Driven through (markers :schema/:table, bare bind + /// names) — ODP.NET's shape, and the only place the two spellings differ. + /// Discriminating in two independent ways: a session still binding "@schema" leaves the + /// colon marker unbound (the expand yields nothing, or the provider refuses the command), and the + /// recorded names would carry the prefix. + /// + [Fact] + public async Task TheCatalogParameters_areBoundUnderTheDialectsBindNames() + { + await using var db = await SqliteBrowseFixture.CreateAsync(); + var dialect = new ColonMarkerDialect(); + await using var session = new SqlBrowseSession(db.Connection, dialect); + + var tableNode = (await session.ExpandAsync(SchemaNodeId, CancellationToken.None)) + .First(n => n.DisplayName == SqliteBrowseFixture.KeyValueTable); + var columns = await session.ExpandAsync(tableNode.NodeId, CancellationToken.None); + + columns.ShouldContain(c => c.DisplayName == SqliteBrowseFixture.RealColumn); + dialect.BoundParameterNames.ShouldContain("schema"); + dialect.BoundParameterNames.ShouldContain("table"); + dialect.BoundParameterNames.ShouldNotContain(n => n.StartsWith(':')); + // Every catalog command went through the provider hook (Oracle's BindByName lives there). + dialect.ConfiguredCommands.ShouldBe(2); + } } diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests.csproj b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests.csproj index b3837b17..b1f73dc6 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests.csproj +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests/ZB.MOM.WW.OtOpcUa.Driver.Sql.Browser.Tests.csproj @@ -45,6 +45,13 @@ — see its class docs. --> + + 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 new file mode 100644 index 00000000..d8f8ae60 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/ColonMarkerDialect.cs @@ -0,0 +1,116 @@ +using System.Data.Common; +using ZB.MOM.WW.OtOpcUa.Core.Abstractions; +using ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests; + +/// +/// A test-only dialect that spells parameters the ODP.NET way — :name in the SQL text and a +/// bare name as the — over the SQLite fixture. It is +/// the falsifiability control for dialect-owned parameter naming: every marker in this repo was hardcoded +/// "@" before, and a planner or loader that still hardcodes it emits @k0 against this +/// dialect and fails loudly. +/// Why the two halves must differ. A dialect whose marker and bind name are the same string +/// (SQL Server, Npgsql, SQLite) cannot tell a call site that copies the plan's bind name apart +/// from one that reuses the marker. Oracle is the provider where they genuinely diverge, so this fake +/// reproduces that divergence rather than merely changing the prefix. +/// It really executes. Everything but the parameter naming (and the catalog SQL's markers) +/// delegates to , and Microsoft.Data.Sqlite accepts both a :-prefixed +/// marker and a prefix-less parameter name — so a poll or a catalog load driven through this dialect runs +/// against the real fixture database rather than being asserted on as text. +/// A decorator rather than a subclass: is sealed, and +/// new-hiding a member would leave interface dispatch calling the original — the substitution +/// would silently not happen and the control would pass for the wrong reason. +/// +public sealed class ColonMarkerDialect : ISqlDialect +{ + private static readonly SqliteDialect Inner = new(); + + /// Every command handed to , kept so its binds can be read back. + private readonly List _commands = []; + + /// How many times has been called on this instance. + /// + /// Oracle's BindByName is set here, and a command created without it mis-binds by position + /// rather than failing — so "was every command configured" has to be observable to be testable. + /// + public int ConfiguredCommands + { + get { lock (_commands) return _commands.Count; } + } + + /// + /// The of every parameter bound on every configured command, + /// read after the fact. + /// Why this and not the query's own success. Microsoft.Data.Sqlite resolves a parameter + /// whether it is named :k0 or bare k0, so a poll that returns the right values proves + /// the SQL parsed — not that the caller bound the dialect's bind name. Only the name actually put on + /// the parameter distinguishes the two, and on ODP.NET the wrong one is a hard bind failure. + /// + public IReadOnlyList BoundParameterNames + { + get + { + lock (_commands) + return _commands + .SelectMany(c => c.Parameters.Cast()) + .Select(p => p.ParameterName) + .ToList(); + } + } + + /// + 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 => Colonize(Inner.ListSchemasSql); + + /// + public string DefaultSchemaSql => Colonize(Inner.DefaultSchemaSql); + + /// + public string ListTablesSql => Colonize(Inner.ListTablesSql); + + /// + public string ListColumnsSql => Colonize(Inner.ListColumnsSql); + + /// + public string ParameterMarker(string baseName) => ":" + baseName; + + /// The bare name, with no prefix — ODP.NET's convention, and deliberately NOT the marker. + /// The planner's/loader's base name. + /// The bind name. + public string ParameterBindName(string baseName) => baseName; + + /// + public void ConfigureCommand(DbCommand command) + { + ArgumentNullException.ThrowIfNull(command); + lock (_commands) _commands.Add(command); + } + + /// + public string QuoteIdentifier(string ident) => Inner.QuoteIdentifier(ident); + + /// + public DriverDataType MapColumnType(string sqlDataType) => Inner.MapColumnType(sqlDataType); + + /// + /// Rewrites the SQLite dialect's @ markers as : markers. The catalog SQL is dialect-owned + /// text, so a colon-marker dialect must carry colon markers in it — that is precisely the coupling the + /// catalog loader's bind names have to stay in step with. + /// + private static string Colonize(string sql) => sql.Replace('@', ':'); +} 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 9236f3e3..afeb8ad1 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 @@ -227,6 +227,50 @@ public sealed class SqlCatalogGateDriverTests driver.GetHealth().State.ShouldBe(DriverState.Faulted); } + /// + /// The catalog path against a dialect whose markers are :schema/:table and whose bind + /// names are the bare schema/table — ODP.NET's shape. The gate still resolves a real + /// table and still refuses an unknown column, so the loader bound names the dialect's SQL actually + /// carries. + /// Behaviourally discriminating, not decorative: a loader still binding the literal + /// "@schema" leaves :schema unbound, the catalog query matches nothing, and + /// every tag — including the honest one — falls to BadNodeIdUnknown. + /// + [Fact] + public async Task The_catalog_load_binds_the_dialects_bind_names_rather_than_a_hardcoded_marker() + { + using var fixture = new SqlitePollFixture(); + var dialect = new ColonMarkerDialect(); + await using var driver = new SqlDriver( + new SqlDriverOptions + { + RawTags = + [ + KvEntry("Speed", SqlitePollFixture.PresentKey), + KvEntry("Bogus", SqlitePollFixture.PresentKey, valueColumn: "no_such_column"), + ], + OperationTimeout = TimeSpan.FromSeconds(15), + CommandTimeout = TimeSpan.FromSeconds(10), + }, + DriverInstanceId, + dialect, + fixture.ConnectionString, + factory: fixture.Factory, + logger: NullLogger.Instance); + + await driver.InitializeAsync(ConfigJson, CancellationToken.None); + + var snapshots = await driver.ReadAsync(["Speed", "Bogus"], CancellationToken.None); + SqlStatusCodes.IsGood(snapshots[0].StatusCode).ShouldBeTrue(); + snapshots[1].StatusCode.ShouldBe(SqlStatusCodes.BadNodeIdUnknown); + + // The catalog queries bound the bare names, and every catalog command went through the hook. + dialect.BoundParameterNames.ShouldContain("schema"); + dialect.BoundParameterNames.ShouldContain("table"); + dialect.BoundParameterNames.ShouldNotContain(n => n.StartsWith(':')); + dialect.ConfiguredCommands.ShouldBeGreaterThan(0); + } + // ---- helpers ---- /// @@ -260,6 +304,12 @@ public sealed class SqlCatalogGateDriverTests public string ListColumnsSql => Inner.ListColumnsSql; + public string ParameterMarker(string baseName) => Inner.ParameterMarker(baseName); + + public string ParameterBindName(string baseName) => Inner.ParameterBindName(baseName); + + public void ConfigureCommand(System.Data.Common.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/SqlGroupPlannerTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlGroupPlannerTests.cs index 85fe8f34..344f4447 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlGroupPlannerTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlGroupPlannerTests.cs @@ -185,6 +185,84 @@ public class SqlGroupPlannerTests plan.Parameters.ShouldBe(new object[] { "k0", "k1", "k2" }); } + // ---- dialect-owned parameter markers ---- + + /// + /// The falsifiability control for dialect-owned markers: against a :-marker dialect the emitted + /// statement must carry :k0, and no @ at all. A planner holding the T-SQL prefix in a + /// const passes every SQL Server test in this class and fails only here. + /// + [Fact] + public void KeyValueMarkers_comeFromTheDialect_ratherThanAHardcodedAtSign() + { + var plans = SqlGroupPlanner.Plan( + [ + KvTag("A", "TagValues", "tag_name", "Line1.Speed", "num_value", null), + KvTag("B", "TagValues", "tag_name", "Line1.Temp", "num_value", null), + ], new ColonMarkerDialect()).ToList(); + + plans[0].SqlText.ShouldEndWith("IN (:k0, :k1)"); + plans[0].SqlText.ShouldNotContain("@"); + plans[0].ParameterNames.ShouldBe(new[] { ":k0", ":k1" }); + } + + [Fact] + public void TheWideRowSelectorMarker_comesFromTheDialect_ratherThanAHardcodedAtSign() + { + var plans = SqlGroupPlanner.Plan( + [WideTag("A", "LatestStatus", "oven_temp", selectorColumn: "station_id", selectorValue: "7")], + new ColonMarkerDialect()).ToList(); + + plans[0].SqlText.ShouldEndWith("= :w"); + plans[0].SqlText.ShouldNotContain("@"); + plans[0].ParameterNames.ShouldBe(new[] { ":w" }); + } + + /// + /// The bind names ride the plan separately from the markers, because on ODP.NET they are a + /// different string. A plan that carried only markers would leave the reader deriving the bind name — + /// the convention it is not entitled to know. + /// + [Fact] + public void ParameterBindNames_areTheDialects_andNeedNotEqualTheMarkers() + { + var plans = SqlGroupPlanner.Plan( + [ + KvTag("A", "TagValues", "tag_name", "Line1.Speed", "num_value", null), + KvTag("B", "TagValues", "tag_name", "Line1.Temp", "num_value", null), + ], new ColonMarkerDialect()).ToList(); + + plans[0].ParameterNames.ShouldBe(new[] { ":k0", ":k1" }); // in the SQL text + plans[0].ParameterBindNames.ShouldBe(new[] { "k0", "k1" }); // on the DbParameter + plans[0].ParameterBindNames.Count.ShouldBe(plans[0].Parameters.Count); + } + + [Fact] + public void TheWideRowSelectorBindName_isTheDialects_andNeedNotEqualTheMarker() + { + var plans = SqlGroupPlanner.Plan( + [WideTag("A", "LatestStatus", "oven_temp", selectorColumn: "station_id", selectorValue: "7")], + new ColonMarkerDialect()).ToList(); + + plans[0].ParameterNames.ShouldBe(new[] { ":w" }); + plans[0].ParameterBindNames.ShouldBe(new[] { "w" }); + } + + /// + /// On a SqlClient-style dialect the two lists coincide — which is exactly why the colon control above + /// exists, and why this one cannot stand in for it. + /// + [Fact] + public void OnSqlServer_theBindNamesAreTheMarkers() + { + var plans = SqlGroupPlanner.Plan( + [KvTag("A", "dbo.TagValues", "tag_name", "Line1.Speed", "num_value", null)], + new SqlServerDialect()).ToList(); + + plans[0].ParameterBindNames.ShouldBe(plans[0].ParameterNames); + plans[0].ParameterBindNames.ShouldBe(new[] { "@k0" }); + } + // ---- wide row ---- [Fact] diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlPollReaderTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlPollReaderTests.cs index cc203d9d..da658517 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlPollReaderTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlPollReaderTests.cs @@ -413,6 +413,47 @@ public sealed class SqlPollReaderTests async () => await reader.ReadAsync(["Speed"], CancellationToken.None)); } + // ---- dialect-owned parameter naming ---- + + /// + /// Runs a real poll through a dialect whose SQL markers (:k0) and bind names (bare k0) + /// differ — ODP.NET's shape — against the SQLite fixture, and asserts both that the + /// values arrive and that the names actually put on the parameters are the dialect's bind names. + /// The name assertion is the load-bearing half. Microsoft.Data.Sqlite happily resolves + /// :k0 and bare k0 alike, so a reader that bound its markers would still return the + /// right values here — and pass a values-only test — while failing outright on ODP.NET. + /// + [Fact] + public async Task ReadAsync_bindsTheDialectsBindNames_whichNeedNotBeTheMarkersInTheSql() + { + using var fixture = new SqlitePollFixture(); + var dialect = new ColonMarkerDialect(); + var reader = new SqlPollReader( + fixture.Factory, fixture.ConnectionString, dialect, + commandTimeout: CommandTimeout, operationTimeout: OperationTimeout, + maxConcurrentGroups: 4, nullIsBad: false, + resolve: Resolver( + KvTag("Speed", SqlitePollFixture.PresentKey), + KvTag("Pressure", SqlitePollFixture.SecondPresentKey), + WideTag("Oven", "oven_temp", selectorValue: SqlitePollFixture.PresentStation))); + + var snapshots = await reader.ReadAsync( + ["Speed", "Pressure", "Oven"], CancellationToken.None); + + // The IN list bound both of its distinct values, and the wide-row selector bound its one. + snapshots[0].Value.ShouldBe(SqlitePollFixture.PresentValue); + snapshots[1].Value.ShouldBe(SqlitePollFixture.SecondPresentValue); + snapshots[2].Value.ShouldBe(SqlitePollFixture.PresentStationOvenTemp); + snapshots.All(s => SqlStatusCodes.IsGood(s.StatusCode)).ShouldBeTrue(); + + // The bind names are the dialect's bare ones, NOT the ":"-prefixed markers in the SQL text. + dialect.BoundParameterNames.ShouldBe(new[] { "k0", "k1", "w" }, ignoreOrder: true); + + // Two groups, two commands — and the provider hook ran on each. On Oracle this is BindByName, whose + // absence mis-binds an IN list by position instead of failing. + dialect.ConfiguredCommands.ShouldBe(2); + } + // ---- connection lifecycle ---- [Theory] diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlQueryPlanImmutabilityTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlQueryPlanImmutabilityTests.cs index b76ddd8a..23a11590 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlQueryPlanImmutabilityTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlQueryPlanImmutabilityTests.cs @@ -12,6 +12,7 @@ public class SqlQueryPlanImmutabilityTests public void Plan_doesNotAliasTheCallersLists() { var names = new List { "@k0" }; + var bindNames = new List { "@k0" }; var values = new List { "v" }; var members = new List { @@ -19,14 +20,17 @@ public class SqlQueryPlanImmutabilityTests }; var selected = new List { "k", "c" }; - var plan = new SqlQueryPlan(SqlTagModel.KeyValue, "gk", "SELECT 1", names, values, members, selected); + var plan = new SqlQueryPlan( + SqlTagModel.KeyValue, "gk", "SELECT 1", names, bindNames, values, members, selected); names.Add("@k1"); + bindNames.Add("@k1"); values.Add("other"); members.Add(members[0]); selected.Add("extra"); plan.ParameterNames.Count.ShouldBe(1); + plan.ParameterBindNames.Count.ShouldBe(1); plan.Parameters.Count.ShouldBe(1); plan.Members.Count.ShouldBe(1); plan.SelectedColumns.Count.ShouldBe(2); 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 a3ec1ca6..b60fdc4a 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 @@ -42,6 +42,28 @@ public sealed class SqliteDialect : ISqlDialect /// public DbProviderFactory Factory => SqliteFactory.Instance; + /// + /// @-prefixed, matching SQL Server — SQLite accepts @, : and $ equally, so + /// this dialect deliberately keeps the same spelling as the production one. Divergent + /// parameter naming is the job of ColonMarkerDialect, which is the control for it; if this + /// dialect diverged too, a SQL-Server-shaped assumption would have nothing left holding it in place. + /// + /// The bare base name. + /// The @-prefixed marker. + public string ParameterMarker(string baseName) => "@" + baseName; + + /// Identical to the marker — Microsoft.Data.Sqlite matches on the prefixed name. + /// The bare base name. + /// The @-prefixed bind name. + public string ParameterBindName(string baseName) => "@" + baseName; + + /// No-op: SqliteCommand binds by name and needs no per-command setup. + /// The command to configure; untouched here. + public void ConfigureCommand(DbCommand command) + { + // Intentionally empty: see the summary. + } + /// public string LivenessSql => "SELECT 1";