refactor(sql): split ISqlDialect/SqlServerDialect into a .Dialect leaf (#526)

`Driver.Sql.Browser` referenced the runtime `Driver.Sql` project. The AdminUI
references that browser, so the whole SQL driver runtime — and
`ZB.MOM.WW.OtOpcUa.Core` through it — sat in the AdminUI's build/deploy graph.
This is the same layering violation #512 removed from the MQTT browser, in a
second driver, and it is why #512's expected removal of `Core.dll` from the
AdminUI's output did not happen.

MEASURED, both directions, on a clean AdminUI build:

  before: ZB.MOM.WW.OtOpcUa.Core.dll + ZB.MOM.WW.OtOpcUa.Driver.Sql.dll present
  after:  both absent

The old csproj comment's premise was sound and survives — the dialect's catalog
SQL *is* the browse engine, so it must be shared rather than duplicated. What
was wrong was the conclusion that sharing it required referencing the runtime
driver. `ISqlDialect`/`SqlServerDialect` depend on nothing in `Driver.Sql`, so
they move to a `.Dialect` leaf that both consume.

This is the `.Transport` shape, not the cheap one: `ISqlDialect` exposes
`DbProviderFactory` and `SqlServerDialect` returns `SqlClientFactory.Instance`,
so `.Contracts` (deliberately provider-free) cannot hold them. Assembly name is
`.Dialect`; the NAMESPACE stays `ZB.MOM.WW.OtOpcUa.Driver.Sql`, matching the
`Driver.Mqtt.Transport` precedent — the extraction cost zero `using` changes.

Two dependencies the cut edge surfaced as the implicit ones they were:

- The AdminUI itself read `SqlDriver.DriverTypeName` in three places — the tag
  editor map, the tag config validator, and the Sql address picker. All three
  now read `DriverTypeNames.Sql`, which is exactly what that constant resolves
  to and exactly the repoint `SqlDriverBrowser`'s own comment had deferred
  until a factory was registered against the shared constant. One exists now.
- `SqlDriverFormContractTests` asserts the AdminUI form's emitted JSON against
  the runtime factory that reads it, so it genuinely needs the runtime driver.
  `AdminUI.Tests` now references it explicitly. A test project referencing a
  runtime driver is fine — tests are not in the AdminUI's deploy graph.

New guard: `DriverBrowserLayeringTests` (Commons.Tests) scans the csproj graph
of every `*.Browser` project in the tree, transitively. It reads csproj files
rather than loaded assemblies because the cost is a build-graph cost — the
compiler emits no assembly reference for an unused `ProjectReference`, so
`MqttBrowserLayeringTests`' assembly-reference approach cannot see that case.
The two are complementary and each now points at the other.

Falsified three ways, each red then green again: the direct reference re-added
to the browser; the same reference one hop out via `.Dialect` (proving the walk
is really transitive); and both controls — browsers-discovered and
references-parsed — which exist so an empty discovery cannot pass the absence
assertion vacuously.

#526's claim that the reference "carries no comment at all" was wrong; it
carried a detailed one from the scaffold commit. Corrected on the issue and in
the design doc's project table, which had recorded the deviation as deliberate.

Claude-Session: https://claude.ai/code/session_015p7wGqy3YpZNCpDzTpGMKo
This commit is contained in:
Joseph Doherty
2026-07-31 08:38:57 -04:00
parent 857a10b4f7
commit 803f4e6efe
15 changed files with 249 additions and 27 deletions
@@ -0,0 +1,123 @@
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;
/// <summary>
/// The provider seam (design §2.2). ADO.NET's <see cref="System.Data.Common"/> base types abstract
/// connections, commands, parameters and readers; they do <b>not</b> abstract the three things that differ
/// per backend — <b>identifier quoting</b>, the <b>metadata-catalog SQL</b>, and <b>row-limit syntax</b>.
/// Those live here, so no dialect assumption leaks into the reader, the poll planner, or the schema browser.
/// <para><b>This interface is the driver's SQL-injection boundary.</b> Every runtime <em>value</em> is
/// bound as a <see cref="DbParameter"/> and never reaches a command text. Identifiers cannot be
/// parameterized in SQL, so the few that must appear as text are emitted through
/// <see cref="QuoteIdentifier"/>. Any code path that builds SQL by concatenating an authored tag field
/// <em>without</em> going through <see cref="QuoteIdentifier"/> is a defect (design §8.1).</para>
/// <para><b>Quoting is the backstop, not the only defence.</b> Design §8.1's catalog gate is
/// implemented: <c>SqlCatalogGate</c> resolves every authored table/column against the live
/// catalog at Initialize and <b>replaces it with the catalog's own spelling</b>, so an identifier
/// reaching <see cref="QuoteIdentifier"/> on the poll path is a string this driver read back out of
/// <see cref="ListSchemasSql"/> / <see cref="ListTablesSql"/> / <see cref="ListColumnsSql"/> — not
/// operator input. An identifier that resolves to nothing rejects its tag (which then publishes
/// <c>BadNodeIdUnknown</c>) rather than being quoted into a query against a nonexistent object.</para>
/// <para>Public because <c>Driver.Sql.Browser</c> consumes it — the catalog SQL <em>is</em> the browse
/// engine, so it is shared rather than duplicated. Implementations own their provider package;
/// <b>no provider-specific type appears in this signature</b> (<see cref="Factory"/> is the abstract
/// <see cref="DbProviderFactory"/>).</para>
/// </summary>
public interface ISqlDialect
{
/// <summary>Which backend this dialect speaks. v1 constructs <see cref="SqlProvider.SqlServer"/> only.</summary>
SqlProvider Provider { get; }
/// <summary>
/// The provider's factory singleton, used to create connections/commands/parameters. Deliberately the
/// abstract base type so consumers never bind to a concrete provider package.
/// </summary>
DbProviderFactory Factory { get; }
/// <summary>
/// Quotes <b>one</b> identifier part so it can be safely embedded in a command text, escaping the
/// dialect's own quote character. Rejects anything that cannot be a real catalog identifier rather
/// than emitting it.
/// <para><b>One part only.</b> A multi-part name such as <c>dbo.TagValues</c> must be split by the
/// caller and each part quoted separately; passing the dotted form yields a single (nonexistent)
/// identifier — safe, but not what the caller meant.</para>
/// </summary>
/// <param name="ident">The bare, unquoted identifier — as returned by the catalog, not pre-quoted.</param>
/// <returns>The quoted identifier, ready to concatenate into a command text.</returns>
/// <exception cref="ArgumentException">The value cannot be a valid catalog identifier.</exception>
string QuoteIdentifier(string ident);
/// <summary>Cheapest statement that proves the connection is alive (<c>SELECT 1</c>; Oracle needs <c>FROM DUAL</c>).</summary>
string LivenessSql { get; }
/// <summary>
/// The fragment that goes <b>immediately after <c>SELECT</c></b> to limit a statement to one row —
/// T-SQL's <c>"TOP 1 "</c>. Empty for a dialect that spells the limit at the end of the statement (see
/// <see cref="SingleRowLimitSuffix"/>).
/// <para><b>Include the trailing space</b> when non-empty: the planner concatenates
/// <c>"SELECT " + SingleRowLimitPrefix + columns</c> with no separator of its own, so an empty fragment
/// must leave the statement byte-identical to an unlimited one.</para>
/// </summary>
/// <remarks>
/// <para><b>Why a prefix/suffix pair rather than one <c>ApplyRowLimit(sql, n)</c> method.</b> The two
/// spellings sit at opposite ends of the statement — SQL Server writes
/// <c>SELECT TOP 1 … ORDER BY … DESC</c>, while SQLite/Postgres/MySQL write
/// <c>SELECT … ORDER BY … DESC LIMIT 1</c>. A single method taking the SELECT list could only serve the
/// prefix position; a single method taking the whole statement would move statement assembly out of the
/// planner and into every dialect. Two fragments keep the planner the sole author of statement shape
/// and let each dialect fill in the end it uses.</para>
/// <para><b>Why "single row" rather than a row count.</b> The only limit v1 emits is the wide-row
/// <c>topByTimestamp</c> selector's newest-row pick. Modelling an arbitrary <c>n</c> would be untested
/// surface, and <c>FETCH FIRST</c>/<c>ROWNUM</c> dialects need more than a substituted number anyway.
/// Widen this to a method when a feature actually needs <c>n &gt; 1</c>.</para>
/// </remarks>
string SingleRowLimitPrefix { get; }
/// <summary>
/// The fragment appended to the <b>very end</b> of a statement to limit it to one row —
/// <c>" LIMIT 1"</c> for SQLite/Postgres/MySQL. Empty for a dialect that limits after <c>SELECT</c>
/// (see <see cref="SingleRowLimitPrefix"/>).
/// <para><b>Include the leading space</b> when non-empty, for the same reason the prefix carries a
/// trailing one: the planner appends it directly to the finished statement.</para>
/// <para>Exactly one of the two fragments is non-empty in every dialect modelled so far, but the planner
/// emits <b>both</b> unconditionally — a dialect needing both ends (or neither) is expressible without
/// touching the call site.</para>
/// </summary>
string SingleRowLimitSuffix { get; }
/// <summary>Catalog query listing schemas — the browser's <c>RootAsync</c> level. Takes no parameters.</summary>
string ListSchemasSql { get; }
/// <summary>
/// Scalar query returning the schema an <b>unqualified</b> object name resolves to for the connecting
/// principal — T-SQL's <c>SELECT SCHEMA_NAME()</c>. Takes no parameters.
/// </summary>
/// <remarks>
/// <para>Needed by <c>SqlCatalogGate</c>: a tag authored as <c>TagValues</c> rather than
/// <c>dbo.TagValues</c> has to be looked up in <em>some</em> schema, and guessing <c>dbo</c> would be a
/// silent lie on any estate that maps its service accounts to their own default schema. Asking the
/// server is the only answer that matches how the poll query itself will resolve the name.</para>
/// <para>It is a <em>query</em> rather than a name because the answer is per-connection, not per-dialect
/// — the same dialect resolves differently for two different logins.</para>
/// </remarks>
string DefaultSchemaSql { get; }
/// <summary>Catalog query listing tables + views in one schema — the browser's schema-expand level. Bind <c>@schema</c>.</summary>
string ListTablesSql { get; }
/// <summary>Catalog query listing columns of one table — the browser's table-expand / attributes level. Bind <c>@schema</c> and <c>@table</c>.</summary>
string ListColumnsSql { get; }
/// <summary>
/// Folds a catalog data-type family name (as <c>INFORMATION_SCHEMA.COLUMNS.DATA_TYPE</c> reports it —
/// e.g. <c>nvarchar</c>, never <c>nvarchar(50)</c>) onto a <see cref="DriverDataType"/>.
/// <para>Must <b>never throw</b>: a browse over a table holding one exotic column must still render.
/// An unrecognised family falls back to <see cref="DriverDataType.String"/>.</para>
/// </summary>
/// <param name="sqlDataType">The bare SQL type family name; matched case-insensitively.</param>
/// <returns>The mapped driver data type, or <see cref="DriverDataType.String"/> when unrecognised.</returns>
DriverDataType MapColumnType(string sqlDataType);
}
@@ -0,0 +1,159 @@
using System.Data.Common;
using System.Globalization;
using Microsoft.Data.SqlClient;
using ZB.MOM.WW.OtOpcUa.Core.Abstractions;
using ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts;
namespace ZB.MOM.WW.OtOpcUa.Driver.Sql;
/// <summary>
/// Microsoft SQL Server dialect (design §2.2) — the only provider v1 constructs. Bracket quoting,
/// <c>INFORMATION_SCHEMA</c> catalog SQL, and the design §3.7 type-family map.
/// <para>Stateless and immutable, so a single instance is safely shared by the driver's poll reader and
/// the AdminUI browse session.</para>
/// </summary>
public sealed class SqlServerDialect : ISqlDialect
{
/// <summary>
/// T-SQL's regular-identifier ceiling (<c>sysname</c> = <c>nvarchar(128)</c>). Enforced because a
/// longer string cannot name a real catalog object — it is either a config mistake or padding, and
/// rejecting it keeps the quoted output bounded.
/// </summary>
internal const int MaxIdentifierLength = 128;
/// <inheritdoc/>
public SqlProvider Provider => SqlProvider.SqlServer;
/// <inheritdoc/>
public DbProviderFactory Factory => SqlClientFactory.Instance;
/// <inheritdoc/>
public string LivenessSql => "SELECT 1";
/// <summary>
/// T-SQL limits after the <c>SELECT</c> keyword, so the whole limit lives in the prefix —
/// <c>"TOP 1 "</c>, trailing space included so <c>SELECT TOP 1 [col]</c> composes with no extra
/// separator at the call site.
/// </summary>
public string SingleRowLimitPrefix => "TOP 1 ";
/// <summary>Empty — T-SQL has no trailing row-limit clause (<c>OFFSET/FETCH</c> is a paging construct, not this).</summary>
public string SingleRowLimitSuffix => string.Empty;
/// <inheritdoc/>
public string ListSchemasSql =>
"SELECT DISTINCT TABLE_SCHEMA FROM INFORMATION_SCHEMA.TABLES ORDER BY TABLE_SCHEMA";
/// <summary>
/// <c>SCHEMA_NAME()</c> with no argument returns the calling principal's default schema — the one an
/// unqualified object name resolves in, which is precisely the question the catalog gate asks.
/// </summary>
public string DefaultSchemaSql => "SELECT SCHEMA_NAME()";
/// <inheritdoc/>
public string ListTablesSql =>
"SELECT TABLE_NAME, TABLE_TYPE FROM INFORMATION_SCHEMA.TABLES " +
"WHERE TABLE_SCHEMA = @schema ORDER BY TABLE_NAME";
/// <inheritdoc/>
public string ListColumnsSql =>
"SELECT COLUMN_NAME, DATA_TYPE, IS_NULLABLE FROM INFORMATION_SCHEMA.COLUMNS " +
"WHERE TABLE_SCHEMA = @schema AND TABLE_NAME = @table ORDER BY ORDINAL_POSITION";
/// <summary>
/// Brackets one identifier part, doubling any embedded <c>]</c> — the T-SQL escape rule, and the only
/// metacharacter that could terminate a bracketed identifier early. <c>[</c>, quotes, semicolons and
/// comment markers are inert inside brackets and are therefore passed through unchanged.
/// </summary>
/// <remarks>
/// <para><b>Rejects</b> (all as <see cref="ArgumentException"/>): null / empty / all-whitespace; longer
/// than <see cref="MaxIdentifierLength"/>; any control character (which covers embedded NUL, and the
/// C0/C1 ranges that can truncate or corrupt a logged/audited statement); and any Unicode
/// <see cref="System.Globalization.UnicodeCategory.Format"/> character — bidi overrides, zero-width
/// spaces, soft hyphen, BOM. Those last cannot escape the brackets, but they spoof the <em>rendered</em>
/// text of a log line or an AdminUI label while comparing byte-different from the real catalog name
/// (the Trojan-Source class, CVE-2021-42574) — the same log/audit-integrity concern that motivates the
/// control-character rule.</para>
/// <para>The rejection messages deliberately do <b>not</b> echo the offending value — it is untrusted
/// input and this exception's text reaches the driver log.</para>
/// <para><b>This is a backstop, not the only defence.</b> Design §8.1's catalog gate
/// (<c>SqlCatalogGate</c>) resolves an authored table/column against the live catalog at
/// Initialize and substitutes the catalog's own spelling, so on the poll path the value arriving here
/// is a string read back out of the catalog. The rules below still run — the gate uses them as its own
/// charset filter, and they must hold for any future caller that has no catalog to check against.</para>
/// </remarks>
/// <param name="ident">The bare, unquoted identifier part.</param>
/// <returns>The bracket-quoted identifier.</returns>
/// <exception cref="ArgumentException">The value cannot be a valid SQL Server identifier.</exception>
public string QuoteIdentifier(string ident)
{
if (string.IsNullOrWhiteSpace(ident))
throw new ArgumentException(
"A SQL identifier must be a non-empty, non-whitespace name.", nameof(ident));
if (ident.Length > MaxIdentifierLength)
throw new ArgumentException(
$"A SQL Server identifier may not exceed {MaxIdentifierLength} characters (got {ident.Length}).",
nameof(ident));
foreach (var ch in ident)
{
if (char.IsControl(ch))
throw new ArgumentException(
"A SQL identifier may not contain control characters (including NUL).", nameof(ident));
// Cf is a *separate* category from Cc, so char.IsControl misses every bidi override and
// zero-width character. They are inert inside brackets but spoof rendered log/UI text.
if (CharUnicodeInfo.GetUnicodeCategory(ch) == UnicodeCategory.Format)
throw new ArgumentException(
"A SQL identifier may not contain Unicode format characters "
+ "(bidi overrides, zero-width spaces, soft hyphen, BOM).", nameof(ident));
}
return string.Concat("[", ident.Replace("]", "]]", StringComparison.Ordinal), "]");
}
/// <summary>
/// Folds an <c>INFORMATION_SCHEMA.COLUMNS.DATA_TYPE</c> family name onto a
/// <see cref="DriverDataType"/> per design §3.7.
/// </summary>
/// <remarks>
/// <para><b>Precision caveat:</b> <c>decimal</c> / <c>numeric</c> / <c>money</c> / <c>smallmoney</c>
/// collapse to <see cref="DriverDataType.Float64"/> in v1. A .NET <c>decimal</c> carries more
/// significant digits than a <c>double</c>, so a high-precision column (e.g. <c>decimal(38,10)</c>)
/// loses low-order digits. Authoring an explicit <c>"type": "String"</c> on the tag preserves the exact
/// textual value when that matters.</para>
/// <para><b>Fallback:</b> an unrecognised family maps to <see cref="DriverDataType.String"/> and never
/// throws — a schema browse must render a table containing an exotic column (<c>xml</c>,
/// <c>geography</c>, <c>hierarchyid</c>, <c>sql_variant</c>, a CLR UDT, <c>varbinary</c>) rather than
/// crash on it. Null/blank input takes the same fallback.</para>
/// <para><b>Deliberate non-mappings:</b> SQL Server's <c>timestamp</c>/<c>rowversion</c> is an opaque
/// 8-byte row version, <em>not</em> a date — it falls back to <see cref="DriverDataType.String"/>
/// rather than being mistaken for a <see cref="DriverDataType.DateTime"/>. <c>time</c> is a
/// time-of-day span, likewise not a point in time, and takes the same fallback.</para>
/// <para><c>tinyint</c> widens to <see cref="DriverDataType.Int16"/> (it is unsigned 0–255 in T-SQL, so
/// a signed 8-bit type would not hold it, and the driver type set has no <c>Byte</c>).</para>
/// </remarks>
/// <param name="sqlDataType">The bare SQL type family name; matched case-insensitively.</param>
/// <returns>The mapped driver data type, or <see cref="DriverDataType.String"/> when unrecognised.</returns>
public DriverDataType MapColumnType(string sqlDataType)
{
if (string.IsNullOrWhiteSpace(sqlDataType)) return DriverDataType.String;
return sqlDataType.Trim().ToLowerInvariant() switch
{
"bit" or "boolean" => DriverDataType.Boolean,
"tinyint" or "smallint" => DriverDataType.Int16,
"int" or "integer" => DriverDataType.Int32,
"bigint" => DriverDataType.Int64,
"real" => DriverDataType.Float32,
"float" or "double" or "double precision"
or "decimal" or "numeric" or "money" or "smallmoney" => DriverDataType.Float64,
"char" or "nchar" or "varchar" or "nvarchar" or "text" or "ntext"
or "uniqueidentifier" => DriverDataType.String,
"date" or "datetime" or "datetime2" or "smalldatetime"
or "datetimeoffset" => DriverDataType.DateTime,
_ => DriverDataType.String,
};
}
}
@@ -0,0 +1,30 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFramework>net10.0</TargetFramework>
<Nullable>enable</Nullable>
<ImplicitUsings>enable</ImplicitUsings>
<LangVersion>latest</LangVersion>
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<NoWarn>$(NoWarn);CS1591</NoWarn>
<!-- Assembly is .Dialect; the NAMESPACE stays ZB.MOM.WW.OtOpcUa.Driver.Sql, matching the
Driver.Mqtt.Transport precedent. The split is a build-graph concern, not an API one, so
extracting these two types cost zero `using` changes anywhere in the tree. -->
<RootNamespace>ZB.MOM.WW.OtOpcUa.Driver.Sql</RootNamespace>
<AssemblyName>ZB.MOM.WW.OtOpcUa.Driver.Sql.Dialect</AssemblyName>
</PropertyGroup>
<ItemGroup>
<ProjectReference Include="..\..\Core\ZB.MOM.WW.OtOpcUa.Core.Abstractions\ZB.MOM.WW.OtOpcUa.Core.Abstractions.csproj" />
<ProjectReference Include="..\ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts\ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts.csproj" />
</ItemGroup>
<ItemGroup>
<!-- The reason this project exists rather than the dialect living in .Contracts: ISqlDialect
exposes DbProviderFactory and SqlServerDialect returns SqlClientFactory.Instance, so the
provider package is unavoidable here. .Contracts is deliberately provider-free. This is
the same shape as Driver.Mqtt.Transport, whose BuildClientOptions returns an MQTTnet type. -->
<PackageReference Include="Microsoft.Data.SqlClient" />
</ItemGroup>
<ItemGroup>
<InternalsVisibleTo Include="ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests"/>
</ItemGroup>
</Project>