diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs index c52e07e2..4a1b5491 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs @@ -220,10 +220,15 @@ public sealed class SqlDriver } catch (Exception ex) { - // Never interpolate _connectionString — Endpoint is the credential-free rendering. + // Never interpolate _connectionString OR ex.Message onto the operator surface. The driver is + // dialect-agnostic over an arbitrary DbProviderFactory, so it cannot rely on any one provider's + // message discipline — Microsoft.Data.SqlClient's connection-string parser, for one, echoes an + // unrecognised keyword lower-cased, which a value-based password redactor misses entirely. Only + // the exception TYPE reaches LastError/host status; the full exception object goes to the log + // SINK via the structured logger's exception parameter (below), never into a rendered string. var message = - $"Sql driver '{_driverInstanceId}' could not reach {Endpoint}: {ex.Message} " + - $"Check the database is up, the network path is open, and the configured credentials are valid."; + $"Sql driver '{_driverInstanceId}' could not reach {Endpoint}: {ex.GetType().Name}. " + + $"Check the database is reachable and the connection string is valid."; _logger.LogError(ex, "Sql driver {DriverInstanceId} liveness check against {Endpoint} failed.", _driverInstanceId, Endpoint); WriteHealth(new DriverHealth(DriverState.Faulted, null, message)); @@ -296,6 +301,20 @@ public sealed class SqlDriver var folder = builder.Folder(DriverTypeName, DriverTypeName); foreach (var tag in Tags.Values) { + if (tag.DeclaredType is null) + { + // Discovery cannot infer a column's real type — that needs live result-set metadata, which + // only a poll returns — so an omitted-type tag materialises as String (the same fallback + // ISqlDialect.MapColumnType uses) while the reader coerces and publishes the column's actual, + // possibly numeric, CLR value against that String node. Nothing else signals the operator to + // this declared-vs-published mismatch, so warn and point them at the fix. + _logger.LogWarning( + "Sql tag '{Tag}' has no authored \"type\" and is discovered as String; a numeric column " + + "will then publish numeric values against a String node. Author an explicit \"type\" on " + + "numeric tags. Driver={DriverInstanceId}", + tag.Name, _driverInstanceId); + } + folder.Variable(tag.Name, tag.Name, new DriverAttributeInfo( FullName: tag.Name, DriverDataType: tag.DeclaredType ?? DriverDataType.String, @@ -344,7 +363,9 @@ public sealed class SqlDriver // The reader throws for one reason only: opening a connection failed (IReadable's contract). _logger.LogWarning(ex, "Sql driver {DriverInstanceId} could not reach {Endpoint} during a read.", _driverInstanceId, Endpoint); - Degrade($"Sql driver '{_driverInstanceId}' could not reach {Endpoint}: {ex.Message}"); + // Type name only — never ex.Message (see InitializeAsync for why); the full exception is already + // on the log sink via the LogWarning exception parameter above. + Degrade($"Sql driver '{_driverInstanceId}' could not reach {Endpoint}: {ex.GetType().Name}."); TransitionHostTo(HostState.Stopped); throw; } @@ -414,7 +435,7 @@ public sealed class SqlDriver /// cadence rather than backed off from. /// /// The snapshots the reader returned for one poll. - private void ObservePollOutcome(IReadOnlyList snapshots) + internal void ObservePollOutcome(IReadOnlyList snapshots) { if (snapshots.Count == 0) return; @@ -447,7 +468,11 @@ public sealed class SqlDriver if (good == 0) return; // authoring-class Bad only — not a statement about the database. - WriteHealth(new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null)); + // Route through the Faulted guard, not a raw WriteHealth: a poll still in flight when a concurrent + // ReinitializeAsync has recorded Faulted (the engine's ~5 s teardown wait is shorter than the 15 s + // default operationTimeout, so a stale poll can still complete afterwards) must not flip Faulted back + // to Healthy with no real recovery. Only a successful (re)initialize clears Faulted. + SetHealthUnlessFaulted(new DriverHealth(DriverState.Healthy, DateTime.UtcNow, null)); TransitionHostTo(HostState.Running); } @@ -464,6 +489,15 @@ public sealed class SqlDriver /// The exception the poll engine caught. internal void HandlePollError(Exception ex) { + // A subscribed-read outage surfaces as a DbException the reader threw, which ReadAsync has ALREADY + // classified — good, endpoint-bearing LastError + host Stopped — before rethrowing. The poll engine + // then routes that same exception here. Re-degrading would overwrite the good LastError with a worse, + // guidance-free one (and, for a provider whose text echoes credentials, re-open the C1 leak) and log + // the outage a second time. So skip the connection class ReadAsync owns; the only exceptions that + // reach past this guard are the engine's own contract-violation throws — an InvalidOperationException + // carrying no provider text — which nothing else classifies. + if (ex is DbException) return; + _logger.LogWarning(ex, "Sql poll failed. Driver={DriverInstanceId} Endpoint={Endpoint}", _driverInstanceId, Endpoint); Degrade(ex.Message); @@ -478,8 +512,21 @@ public sealed class SqlDriver private void Degrade(string reason) { var current = ReadHealth(); - if (current.State == DriverState.Faulted) return; - WriteHealth(new DriverHealth(DriverState.Degraded, current.LastSuccessfulRead, reason)); + SetHealthUnlessFaulted(new DriverHealth(DriverState.Degraded, current.LastSuccessfulRead, reason)); + } + + /// + /// Publishes unless the driver is already + /// — a liveness/config verdict that only a successful (re)initialize may clear. Every non-Initialize + /// health write routes through here: and the poll's Healthy verdict in + /// , so a late in-flight poll cannot un-fault a driver a concurrent + /// just faulted. + /// + /// The health snapshot to publish when the driver is not Faulted. + private void SetHealthUnlessFaulted(DriverHealth value) + { + if (ReadHealth().State == DriverState.Faulted) return; + WriteHealth(value); } /// Advances LastSuccessfulRead without changing the current state or error. @@ -524,12 +571,26 @@ public sealed class SqlDriver var table = new Dictionary(_options.RawTags.Count, StringComparer.Ordinal); foreach (var entry in _options.RawTags) { - if (SqlEquipmentTagParser.TryParse(entry.TagConfig, entry.RawPath, out var definition)) - table[entry.RawPath] = definition; - else - _logger.LogWarning( - "Sql tag config did not map to a definition; skipping. Driver={DriverInstanceId} RawPath={RawPath}", + try + { + if (SqlEquipmentTagParser.TryParse(entry.TagConfig, entry.RawPath, out var definition)) + table[entry.RawPath] = definition; + else + _logger.LogWarning( + "Sql tag config did not map to a definition; skipping. Driver={DriverInstanceId} RawPath={RawPath}", + _driverInstanceId, entry.RawPath); + } + catch (Exception ex) + { + // TryParse is contracted never to throw, but BuildTagTable runs BEFORE InitializeAsync's + // try/catch, so a stray throw here (a parser bug on one blob) would escape Initialize and + // strand health at Initializing across every DriverInstanceActor retry. Skip the offending + // tag — the same "one bad tag must not take the whole driver down" rule the malformed-blob + // branch above follows — rather than fault the driver over a single entry. + _logger.LogError( + ex, "Sql tag config threw while parsing; skipping. Driver={DriverInstanceId} RawPath={RawPath}", _driverInstanceId, entry.RawPath); + } } Volatile.Write(ref _tagsByRawPath, table); @@ -618,6 +679,8 @@ public sealed class SqlDriver /// /// Observes an abandoned liveness attempt's eventual fault so it never surfaces as an unobserved /// task exception. The task still owns — and disposes — its own connection. + /// TODO(M2): a verbatim duplicate of SqlPollReader.Detach; both live in Driver.Sql and + /// could hoist to one internal helper. Left in place here to keep this change off the reader. /// private static void Detach(Task work) => _ = work.ContinueWith( diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlDriverTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlDriverTests.cs index eb4bee5c..9b7a5bf8 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlDriverTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlDriverTests.cs @@ -1,3 +1,4 @@ +using System.Data; using System.Data.Common; using System.Globalization; using Microsoft.Data.Sqlite; @@ -342,6 +343,132 @@ public sealed class SqlDriverTests } } + // ---- C1a: the credential guarantee, proven load-bearing ---- + + [Fact] + public async Task Initialize_whenTheProviderExceptionCarriesCredentials_leaksThemIntoNoOperatorSurface() + { + // The vacuous sibling test drives SQLite's "unable to open database file" path, whose message + // structurally never contains the connection string — so it passes with or without any defensive + // code. This one fabricates a DbException whose OWN .Message carries a credential-like token and + // drives it through Initialize's liveness-failure path via the injectable factory seam. It is RED + // against the pre-fix code that interpolated ex.Message into LastError and the thrown message. + const string leakyMessage = "login failed for user; Password=" + SecretToken; + var driver = new SqlDriver( + new SqlDriverOptions + { + RawTags = [KvEntry("Speed", SqlitePollFixture.PresentKey)], + OperationTimeout = TimeSpan.FromSeconds(15), + CommandTimeout = TimeSpan.FromSeconds(10), + }, + DriverInstanceId, + new SqliteDialect(), + // A credential-free connection string: the ONLY place the token can come from is the exception. + "Server=sqlsrv01;Initial Catalog=Mes", + factory: new ThrowingConnectionFactory(leakyMessage), + logger: CapturingLogger.Null); + await using var _ = driver; + + var thrown = await Should.ThrowAsync( + async () => await driver.InitializeAsync(ConfigJson, CancellationToken.None)); + + var health = driver.GetHealth(); + health.State.ShouldBe(DriverState.Faulted); + health.LastError.ShouldNotBeNullOrWhiteSpace(); + // The token appears in NEITHER the operator-facing LastError NOR the thrown message NOR host status. + health.LastError!.ShouldNotContain(SecretToken); + thrown.Message.ShouldNotContain(SecretToken); + driver.GetHostStatuses()[0].HostName.ShouldNotContain(SecretToken); + driver.GetHostStatuses()[0].State.ShouldBe(HostState.Stopped); + // …and it is still actionable: it names the endpoint the operator has to go look at. + health.LastError.ShouldContain("sqlsrv01"); + } + + // ---- I1: no double health-classification on a subscribed-read outage ---- + + [Fact] + public async Task HandlePollError_forADbException_defersToReadAsync_soHealthIsClassifiedOnce() + { + // On a subscribed-read outage the reader throws a DbException; ReadAsync classifies health with a + // good, endpoint-bearing message + host Stopped and rethrows the SAME exception, which the poll + // engine then routes to HandlePollError. HandlePollError must ignore the DbException class ReadAsync + // owns — pre-fix it re-degraded with the bare ex.Message (the WORSE of the two LastErrors, and a + // credential-leak path) and logged the outage a second time. + using var fixture = new SqlitePollFixture(); + var logger = new CapturingLogger(); + await using var driver = NewDriver(fixture, logger, KvEntry("Speed", SqlitePollFixture.PresentKey)); + await driver.InitializeAsync(ConfigJson, CancellationToken.None); + driver.GetHealth().State.ShouldBe(DriverState.Healthy); + + driver.HandlePollError(new FakeDbException("connection failed; Password=" + SecretToken)); + + var health = driver.GetHealth(); + // Untouched: HandlePollError left the DbException to ReadAsync, so it neither degraded nor leaked… + health.State.ShouldBe(DriverState.Healthy); + health.LastError.ShouldBeNull(); + // …and it logged nothing (ReadAsync owns the single outage warning). + logger.Entries.Count(e => e.Level == LogLevel.Warning).ShouldBe(0); + } + + [Fact] + public void HandlePollError_forAnEngineContractViolation_stillDegrades() + { + // The falsifiability control for the guard above: a non-DbException (the poll engine's own + // reader-contract-violation throw) is NOT owned by ReadAsync, so it must still reach the health + // surface. The guard skips only the DbException connection class. + using var fixture = new SqlitePollFixture(); + var driver = NewDriver(fixture, KvEntry("Speed", SqlitePollFixture.PresentKey)); + + driver.HandlePollError(new InvalidOperationException("Reader contract violation: expected 1 snapshots")); + + driver.GetHealth().State.ShouldBe(DriverState.Degraded); + } + + // ---- I2: a late poll cannot un-fault a Faulted driver ---- + + [Fact] + public async Task ObservePollOutcome_whenALateGoodPollLands_cannotUnFaultTheDriver() + { + // The engine's teardown waits ~5 s for loops to wind down, but a poll may still be in flight up to + // the 15 s default operationTimeout — so a poll can complete AFTER a concurrent ReinitializeAsync has + // recorded Faulted. If that stale poll reports all-Good it must NOT flip Faulted back to Healthy: only + // a successful (re)initialize clears Faulted. RED against the pre-fix unconditional Healthy write. + using var fixture = new SqlitePollFixture(); + await using var driver = NewDriver( + fixture, Unreachable(), CapturingLogger.Null, KvEntry("Speed", SqlitePollFixture.PresentKey)); + await Should.ThrowAsync( + async () => await driver.InitializeAsync(ConfigJson, CancellationToken.None)); + driver.GetHealth().State.ShouldBe(DriverState.Faulted); + + // The stale in-flight poll completes with an all-Good batch. + driver.ObservePollOutcome( + [new DataValueSnapshot(42, SqlStatusCodes.Good, DateTime.UtcNow, DateTime.UtcNow)]); + + driver.GetHealth().State.ShouldBe(DriverState.Faulted); // stayed Faulted + } + + // ---- I3: an omitted-type tag warns at discovery ---- + + [Fact] + public async Task Discover_warnsForAnOmittedTypeTag_butNotForATypedOne() + { + using var fixture = new SqlitePollFixture(); + var logger = new CapturingLogger(); + await using var driver = NewDriver( + fixture, logger, + KvEntry("Speed", SqlitePollFixture.PresentKey), // no "type" → discovered String + TypedKvEntry("Rpm", SqlitePollFixture.PresentKey, "Int32")); // explicit "type" + await driver.InitializeAsync(ConfigJson, CancellationToken.None); + + var capture = new CapturingBuilder(); + await ((ITagDiscovery)driver).DiscoverAsync(capture, CancellationToken.None); + + logger.Entries.ShouldContain(e => + e.Level == LogLevel.Warning && e.Message.Contains("Speed") && e.Message.Contains("no authored")); + logger.Entries.ShouldNotContain(e => + e.Level == LogLevel.Warning && e.Message.Contains("Rpm") && e.Message.Contains("no authored")); + } + // ---- helpers ---- /// A distinctive password token: any assertion that finds it has found a credential leak. @@ -417,6 +544,21 @@ public sealed class SqlDriverTests } """), WriteIdempotent: false); + /// A key-value entry carrying an explicit "type" — so DeclaredType is non-null. + private static RawTagEntry TypedKvEntry(string rawPath, string keyValue, string type) + => new(rawPath, string.Create(CultureInfo.InvariantCulture, $$""" + { + "driver": "Sql", + "model": "KeyValue", + "table": "{{SqlitePollFixture.KeyValueTable}}", + "keyColumn": "{{SqlitePollFixture.KeyColumn}}", + "keyValue": "{{keyValue}}", + "valueColumn": "{{SqlitePollFixture.ValueColumn}}", + "timestampColumn": "{{SqlitePollFixture.TimestampColumn}}", + "type": "{{type}}" + } + """), WriteIdempotent: false); + /// The same tag as , already typed — for the reference reader. private static SqlTagDefinition KvDefinition(string rawPath, string keyValue) => new(rawPath, SqlTagModel.KeyValue, SqlitePollFixture.KeyValueTable, @@ -487,4 +629,47 @@ public sealed class SqlDriverTests public void Dispose() { } } } + + /// + /// A whose message is under the test's control — stands in for the + /// credential-echoing exceptions a real ADO.NET provider can raise, so the credential-hygiene + /// guarantee can be exercised without a live database. + /// + private sealed class FakeDbException(string message) : DbException(message); + + /// + /// A whose connections always fail to open, with a + /// caller-supplied (credential-bearing) message — the seam that drives the driver's + /// unreachable-database path with a fabricated provider exception. + /// + private sealed class ThrowingConnectionFactory(string openFailureMessage) : DbProviderFactory + { + public override DbConnection CreateConnection() => new ThrowingConnection(openFailureMessage); + } + + /// A connection that throws on open; every other member is inert. + private sealed class ThrowingConnection(string openFailureMessage) : DbConnection + { + [System.Diagnostics.CodeAnalysis.AllowNull] + public override string ConnectionString { get; set; } = string.Empty; + + public override string Database => string.Empty; + + public override string DataSource => string.Empty; + + public override string ServerVersion => string.Empty; + + public override ConnectionState State => ConnectionState.Closed; + + public override void Open() => throw new FakeDbException(openFailureMessage); + + public override void ChangeDatabase(string databaseName) => throw new NotSupportedException(); + + public override void Close() { } + + protected override DbTransaction BeginDbTransaction(IsolationLevel isolationLevel) + => throw new NotSupportedException(); + + protected override DbCommand CreateDbCommand() => throw new NotSupportedException(); + } }