From 5184a2e107151900c73c2d29ea250a5367de61cf Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 28 Jul 2026 00:30:52 -0400 Subject: [PATCH] fix(drivers): complete the Sql + FOCAS in-place config re-parse (#516) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first #516 pass deliberately left these two out, because each derives more than options from config: Sql its ISqlDialect and resolved connection string, FOCAS the IFocasClientFactory its Backend key selects — all injected at construction. Adopting new options alone would poll a NEW tag set through the OLD database/backend, and a half-applied config change is worse than a discarded one because it looks like it worked. Rather than accept that exclusion, this fixes the blocker. Each factory now exposes a ParseBinding returning EVERY config-derived dependency as one value, and the driver adopts them atomically: - Sql: ApplyBinding takes (options, dialect, connectionString) and rebuilds everything downstream — provider factory, Endpoint, and the SqlPollReader that captures all three. It runs BEFORE BuildTagTable, so the tag table and the connection it is polled over always come from the same revision. A test-injected DbProviderFactory survives a rebind (_explicitFactory), so a re-derived dialect cannot displace what a test passed in. - FOCAS: options and backend move as a pair in InitializeAsync. Re-resolving Sql's connection string on reinit is a side benefit: a rotated credential is picked up without a process restart. Drivers constructed directly get no rebinder and keep their constructor binding, so every existing "{}"-passing lifecycle test is unaffected by construction rather than by luck. The tests pin ATOMICITY, not merely that a re-parse happened — a test checking only options would have passed against the broken version. Confirmed by simulating the half-fix (adopt options, skip the backend): 2 of 3 FOCAS tests go red. Reverting Sql's rebind turns its connection-string test red. Sql also asserts that a reinit which cannot resolve its new connection leaves the previous binding whole rather than half-adopting. All 12 drivers now honour a changed config in place. Sql.Tests 226, FOCAS.Tests 275 pass. --- deferment.md | 33 +++++ .../FocasDriver.cs | 58 ++++++-- .../FocasDriverFactoryExtensions.cs | 37 ++++- .../ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs | 129 ++++++++++++++---- .../SqlDriverFactoryExtensions.cs | 85 +++++++++--- .../FocasReinitConfigAdoptionTests.cs | 67 +++++++++ .../SqlReinitConfigAdoptionTests.cs | 120 ++++++++++++++++ 7 files changed, 467 insertions(+), 62 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasReinitConfigAdoptionTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlReinitConfigAdoptionTests.cs diff --git a/deferment.md b/deferment.md index 3a4de5b5..aebcf013 100644 --- a/deferment.md +++ b/deferment.md @@ -497,6 +497,39 @@ describes for Tier C no longer exists (Galaxy went to the mxaccessgw sidecar in in-process when its managed wire client landed). No runtime change — turning recycle on for two live drivers deserves its own live gate, not a docs-pass side effect. Tracked as Gitea **#522**. +### 2026-07-28 — the two things §8.3 deliberately did NOT build (1 of 2) + +**Sql and FOCAS now re-parse in place too.** The first pass excluded them for a real reason — each derives +more than options from config, so adopting new options alone would run a **new tag set against an old +connection/backend**, and a half-applied change is worse than a discarded one because it looks like it +worked. Rather than accept that, the blocker itself is fixed: each factory now exposes a **`ParseBinding`** +that returns *every* config-derived dependency as one value, and the driver adopts them **atomically**. + +| Driver | What one parse now yields | Adopted by | +|---|---|---| +| Sql | options + `ISqlDialect` + resolved connection string | `ApplyBinding`, which also rebuilds the provider factory, `Endpoint` and the `SqlPollReader` that captures all three | +| FOCAS | options + the `IFocasClientFactory` the `Backend` key selects | one assignment pair in `InitializeAsync` | + +Details worth keeping: + +- **Sql adopts BEFORE `BuildTagTable`**, so the tag table and the connection it will be polled over always + come from the same revision. +- **A test-injected `DbProviderFactory` survives a rebind** (`_explicitFactory`), so a re-derived dialect + cannot silently displace what a test passed in. +- **Re-resolving the connection string on reinit is a side benefit**: a rotated credential is picked up + without a process restart. +- A driver constructed **directly** gets no rebinder and keeps its constructor-supplied binding — every + existing `"{}"`-passing lifecycle test is unaffected by construction, not by luck. + +**The tests pin ATOMICITY, not merely "a re-parse happened"** — a test that only checked options would have +passed against the broken version. Verified by simulating the *half-fix* (adopt options, skip the backend): +2 of the 3 FOCAS tests go red. Reverting Sql's rebind turns its connection-string test red. Sql also asserts +that a reinit which **cannot resolve** its new connection leaves the previous binding whole, rather than +half-adopting. + +All 12 drivers now honour a changed config in place; `DriverSpawnPlanner`'s stop + respawn remains the outer +guarantee. Sql.Tests 226 / FOCAS.Tests 275 pass. + ### 2026-07-27 — §8.4 dispatch-map parity (G-1 … G-6) **The maps had to become data before they could be guarded.** A Razor `@switch` compiles into diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs index 0d834c59..b01ef4a4 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriver.cs @@ -21,9 +21,18 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS; public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, ISubscribable, IHostConnectivityProbe, IPerCallHostResolver, IAlarmSource, IDisposable, IAsyncDisposable { - private readonly FocasDriverOptions _options; + /// Mutable so can adopt a re-parsed config (#516). Written only + /// on the init path, together with , before either is used. + private FocasDriverOptions _options; private readonly string _driverInstanceId; - private readonly IFocasClientFactory _clientFactory; + /// Mutable for the same reason as — the Backend config key + /// selects it, so the two must move together or a new tag set polls through an old backend. + private IFocasClientFactory _clientFactory; + + /// Re-parses a DriverConfig into every config-derived dependency. Null when the driver + /// was constructed directly (tests, or a caller supplying its own backend), which keeps the + /// constructor-supplied options authoritative — exactly the prior behaviour. + private readonly Func? _rebind; private readonly PollGroupEngine _poll; private readonly ILogger _logger; private readonly Dictionary _devices = new(StringComparer.OrdinalIgnoreCase); @@ -58,14 +67,20 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// The unique identifier for this driver instance. /// Optional factory for creating FOCAS client instances. /// Optional logger instance. + /// Optional re-parse of a DriverConfig into every config-derived dependency, + /// supplied by the factory so can adopt a changed config. Null (direct + /// construction, e.g. a test or a caller supplying its own backend) keeps the constructor-supplied + /// options + backend. public FocasDriver(FocasDriverOptions options, string driverInstanceId, IFocasClientFactory? clientFactory = null, - ILogger? logger = null) + ILogger? logger = null, + Func? rebind = null) { ArgumentNullException.ThrowIfNull(options); _options = options; _driverInstanceId = driverInstanceId; _clientFactory = clientFactory ?? new Wire.WireFocasClientFactory(); + _rebind = rebind; _logger = logger ?? NullLogger.Instance; _resolver = new EquipmentTagRefResolver( r => _tagsByRawPath.TryGetValue(r, out var t) ? t : null); @@ -77,6 +92,16 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, backoffCap: PollBackoffCap); } + /// True when the supplied DriverConfig JSON carries a real body. The bootstrapper always passes + /// a populated document; some unit tests pass "{}" or an empty string to exercise lifecycle shape + /// without a config — those keep the constructor-supplied options + backend. + private static bool HasConfigBody(string? driverConfigJson) + { + if (string.IsNullOrWhiteSpace(driverConfigJson)) return false; + var trimmed = driverConfigJson.Trim(); + return trimmed is not "{}" and not "[]"; + } + /// Upper bound on the poll-loop failure backoff — adopts the S7-proven 30 s cap fleet-wide (05/STAB-8). private static readonly TimeSpan PollBackoffCap = TimeSpan.FromSeconds(30); @@ -102,13 +127,15 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, /// /// Opens the configured CNC handles and builds the authored tag table. - /// is deliberately not re-parsed here, unlike - /// Modbus / AbLegacy / OpcUaClient (Gitea #516). This driver builds more than options from config — - /// its IFocasClientFactory is selected from the Backend key and injected at - /// construction — so adopting a new FocasDriverOptions alone would run a NEW device/tag set - /// against the OLD backend. Half a re-parse is worse than none. - /// A config change is covered by the stop + respawn in DriverSpawnPlanner, which - /// rebuilds options and backend together through the factory. + /// is re-parsed here when the driver was built by + /// the factory (Gitea #516), and the re-parse is all-or-nothing: the typed options and the + /// IFocasClientFactory the Backend key selects come from one ParseBinding call + /// and are adopted together. That pairing is the point — adopting options alone would poll a NEW + /// device/tag set through the OLD backend, which is why this driver was excluded from the first + /// #516 pass. + /// A driver constructed directly (every unit test, or a caller supplying its own backend) gets + /// no rebinder and keeps its constructor-supplied pair. DriverSpawnPlanner's stop + respawn + /// remains the outer guarantee. /// /// The driver configuration JSON (see remarks — not re-parsed). /// Cancellation token for the operation. @@ -118,6 +145,17 @@ public sealed class FocasDriver : IDriver, IReadable, IWritable, ITagDiscovery, Volatile.Write(ref _health, new DriverHealth(DriverState.Initializing, null, null)); try { + // #516: adopt a changed config. Options and the backend client factory are re-derived by ONE + // parse and assigned together — a partial adoption would poll a new device/tag set through the + // old backend. A null rebinder (direct construction) or an empty/placeholder document keeps the + // constructor-supplied pair, which is what every lifecycle test passing "{}" relies on. + if (_rebind is not null && HasConfigBody(driverConfigJson)) + { + var binding = _rebind(driverConfigJson); + _options = binding.Options; + _clientFactory = binding.ClientFactory; + } + // Fail fast if the factory is a stub/unimplemented backend — the operator must // see an actionable error at init rather than a phantom-Healthy driver that fails // every read/write/subscribe silently. diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriverFactoryExtensions.cs index 733e6cc8..8fbf4538 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasDriverFactoryExtensions.cs @@ -47,6 +47,29 @@ public static class FocasDriverFactoryExtensions /// The driver configuration JSON string. /// A configured instance. internal static FocasDriver CreateInstance(string driverInstanceId, string driverConfigJson) + { + var binding = ParseBinding(driverInstanceId, driverConfigJson); + return new FocasDriver( + binding.Options, + driverInstanceId, + binding.ClientFactory, + // Hand the driver the SAME parse it was built with, so InitializeAsync can re-derive EVERY + // config-derived dependency on a reinitialize — options AND the backend client factory + // together (#516). Re-parsing options alone would run a new device/tag set against the old + // backend, which is worse than not re-parsing at all. + rebind: json => ParseBinding(driverInstanceId, json)); + } + + /// + /// Parses a FOCAS DriverConfig JSON document into every dependency the driver derives + /// from config: the typed options and the the Backend key + /// selects. Returned together because they must be adopted together — see the rebind note in + /// . + /// + /// The unique driver instance identifier. + /// The driver configuration JSON string. + /// The parsed options + backend factory. + internal static FocasDriverBinding ParseBinding(string driverInstanceId, string driverConfigJson) { ArgumentException.ThrowIfNullOrWhiteSpace(driverInstanceId); ArgumentException.ThrowIfNullOrWhiteSpace(driverConfigJson); @@ -86,8 +109,7 @@ public static class FocasDriverFactoryExtensions HandleRecycle = BuildHandleRecycle(dto.HandleRecycle), }; - var clientFactory = BuildClientFactory(dto, driverInstanceId); - return new FocasDriver(options, driverInstanceId, clientFactory); + return new FocasDriverBinding(options, BuildClientFactory(dto, driverInstanceId)); } /// @@ -327,3 +349,14 @@ public static class FocasDriverFactoryExtensions public TimeSpan? Interval { get; init; } } } + +/// +/// Everything FocasDriver derives from its DriverConfig JSON, returned as one value so a +/// reinitialize adopts all of it atomically. +/// This exists because a partial adoption is a real hazard, not a theoretical one: the driver's +/// is chosen by the config's Backend key, so re-parsing +/// alone would poll a NEW device/tag set through the OLD backend (Gitea #516). +/// +/// The typed driver options. +/// The backend client factory the Backend key selects. +public sealed record FocasDriverBinding(FocasDriverOptions Options, IFocasClientFactory ClientFactory); 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 6af0b649..c69de1ec 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriver.cs @@ -1,4 +1,5 @@ using System.Data.Common; +using System.Diagnostics.CodeAnalysis; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using ZB.MOM.WW.OtOpcUa.Core.Abstractions; @@ -51,16 +52,19 @@ public sealed class SqlDriver // ---- instance fields (grouped at top for auditability) ---- - private readonly SqlDriverOptions _options; + /// Config-derived, so mutable — see . + private SqlDriverOptions _options; private readonly string _driverInstanceId; - private readonly ISqlDialect _dialect; - private readonly DbProviderFactory _factory; + /// Config-derived, so mutable: swaps it with the connection string + /// and options together on a reinitialize (#516). + private ISqlDialect _dialect; + private DbProviderFactory _factory; /// /// The resolved connection string — a secret. It is passed to the provider and to nothing /// else: every log line, health message and host status carries instead. /// - private readonly string _connectionString; + private string _connectionString; private readonly ILogger _logger; @@ -93,11 +97,19 @@ public sealed class SqlDriver /// Resolves a read/subscribe RawPath to its catalog-validated definition, or a miss. private readonly EquipmentTagRefResolver _resolver; + /// The factory the CALLER passed, or null to take the dialect's. Kept separate from + /// so a re-derived dialect cannot silently displace a test's injected factory. + private readonly DbProviderFactory? _explicitFactory; + + /// Re-parses a DriverConfig into every config-derived dependency. Null when the driver + /// was constructed directly, which keeps the constructor-supplied trio authoritative. + private readonly Func? _rebind; + /// /// The read path. Built once, in the constructor, and not injected: its resolve /// delegate must read this driver's live authored table, which does not exist before the driver does. /// - private readonly SqlPollReader _reader; + private SqlPollReader _reader; /// Polled subscriptions. The driver supplies the reader + change bridge; the engine owns the loop. private readonly PollGroupEngine _poll; @@ -133,6 +145,11 @@ public sealed class SqlDriver /// explicitly by tests that need to observe or delay connection creation. /// /// Optional; defaults to a no-op logger. + /// + /// Optional re-parse of a DriverConfig into every config-derived dependency, supplied by the + /// factory so can adopt a changed config. Null (direct construction — + /// every test) keeps the constructor-supplied options, dialect and connection string. + /// /// A required reference argument is null. /// or is blank. public SqlDriver( @@ -141,7 +158,8 @@ public sealed class SqlDriver ISqlDialect dialect, string connectionString, DbProviderFactory? factory = null, - ILogger? logger = null) + ILogger? logger = null, + Func? rebind = null) { ArgumentNullException.ThrowIfNull(options); ArgumentNullException.ThrowIfNull(dialect); @@ -149,15 +167,47 @@ public sealed class SqlDriver if (string.IsNullOrWhiteSpace(connectionString)) throw new ArgumentException("A Sql driver needs a resolved connection string.", nameof(connectionString)); - _options = options; _driverInstanceId = driverInstanceId; - _dialect = dialect; - _factory = factory ?? dialect.Factory; - _connectionString = connectionString; _logger = logger ?? NullLogger.Instance; - Endpoint = DescribeEndpoint(connectionString); - + _explicitFactory = factory; + _rebind = rebind; _resolver = new EquipmentTagRefResolver(Lookup); + + // Everything below this line is derived from config and is re-derived wholesale on a reinitialize. + ApplyBinding(options, dialect, connectionString); + + _poll = new PollGroupEngine( + reader: ReadAsync, + onChange: (handle, tagRef, snapshot) => + OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, tagRef, snapshot)), + onError: HandlePollError, + backoffCap: PollBackoffCap); + } + + /// + /// Adopts a config-derived trio — options, dialect, connection string — and rebuilds everything + /// downstream of it (the provider factory, the endpoint description, and the poll reader, which + /// captures all three). Called from the constructor and again from + /// when a changed config arrives. + /// All-or-nothing on purpose. The reason Sql was excluded from the first #516 pass is + /// that adopting options alone would poll a NEW tag set through the OLD connection and dialect. Doing + /// it in one method means a future field derived from config gets added here rather than being + /// forgotten on the reinit path. + /// Not thread-safe by design: only the init path calls it, before any poll group is running. + /// The poll engine and the tag resolver survive across a rebind — the engine holds a delegate to + /// ReadAsync and the resolver reads the live tag table, so neither captures the old trio. + /// + /// The typed options to adopt. + /// The provider seam to adopt. + /// The resolved connection string to adopt. Never logged. + [MemberNotNull(nameof(_options), nameof(_dialect), nameof(_factory), nameof(_connectionString), nameof(_reader))] + private void ApplyBinding(SqlDriverOptions options, ISqlDialect dialect, string connectionString) + { + _options = options; + _dialect = dialect; + _factory = _explicitFactory ?? dialect.Factory; + _connectionString = connectionString; + Endpoint = DescribeEndpoint(connectionString); _reader = new SqlPollReader( _factory, connectionString, @@ -168,12 +218,16 @@ public sealed class SqlDriver nullIsBad: options.NullIsBad, resolve: rawPath => _resolver.TryResolve(rawPath, out var definition) ? definition : null, logger: _logger); - _poll = new PollGroupEngine( - reader: ReadAsync, - onChange: (handle, tagRef, snapshot) => - OnDataChange?.Invoke(this, new DataChangeEventArgs(handle, tagRef, snapshot)), - onError: HandlePollError, - backoffCap: PollBackoffCap); + } + + /// True when the supplied DriverConfig JSON carries a real body. The bootstrapper always passes + /// a populated document; some unit tests pass "{}" or an empty string to exercise lifecycle shape + /// without a config — those keep the constructor-supplied trio. + private static bool HasConfigBody(string? driverConfigJson) + { + if (string.IsNullOrWhiteSpace(driverConfigJson)) return false; + var trimmed = driverConfigJson.Trim(); + return trimmed is not "{}" and not "[]"; } /// @@ -187,7 +241,7 @@ public sealed class SqlDriver /// the connection string names both. This is the only rendering of the connection string that may /// appear in a log, a health message, or the Admin UI. /// - public string Endpoint { get; } + public string Endpoint { get; private set; } = ""; /// Host identifier surfaced through — the endpoint description. public string HostName => Endpoint; @@ -215,17 +269,20 @@ public sealed class SqlDriver /// /// Builds the authored RawPath table and proves the database is reachable. - /// is not re-parsed here: the driver serves the - /// typed it was constructed with. That premise — "config parsing - /// belongs to the factory, which builds a fresh instance" — was false when written - /// (Gitea #516): the host reinitialized the EXISTING child in place, so every config edit was - /// silently discarded and the deployment still sealed green. It is true now, because - /// DriverSpawnPlanner routes a changed DriverConfig through a stop + respawn. - /// This driver deliberately does not also re-parse in place, unlike Modbus / AbLegacy / - /// OpcUaClient. It builds more than options from config — the and the - /// resolved connection string are both injected at construction — so adopting a new - /// alone would run a NEW tag set against the OLD connection. Half a - /// re-parse is worse than none; the respawn rebuilds all three together. + /// is re-parsed here when the driver was built by + /// the factory (Gitea #516). The original comment on this method claimed it was not, on the premise + /// that "config parsing belongs to the factory, which builds a fresh instance" — that was false when + /// written, because the host reinitialized the EXISTING child in place and every config edit was + /// silently discarded while the deployment still sealed green. + /// The re-parse is all-or-nothing via : options, the + /// and the resolved connection string are re-derived by one + /// ParseBinding call and adopted together. That is what makes it safe here — adopting options + /// alone would poll a NEW tag set through the OLD database, which is why this driver was excluded + /// from the first #516 pass. It happens BEFORE BuildTagTable, so the tag table and the + /// connection it is polled over always come from the same revision. + /// A driver constructed directly (every unit test) gets no rebinder and keeps its + /// constructor-supplied trio, so "{}"-passing lifecycle tests are unaffected. + /// DriverSpawnPlanner's stop + respawn remains the outer guarantee. /// The table is built first because it is pure and cannot fail; the liveness check is the only /// I/O, and on failure this method records and rethrows — /// DriverInstanceActor reads a throw as InitializeFailed and lands in Reconnecting with its @@ -243,6 +300,18 @@ public sealed class SqlDriver public async Task InitializeAsync(string driverConfigJson, CancellationToken cancellationToken) { WriteHealth(new DriverHealth(DriverState.Initializing, null, null)); + + // #516: adopt a changed config BEFORE the tag table is built, so the table and the connection it + // will be polled over come from the same revision. ApplyBinding takes the options, dialect and + // connection string as one unit; adopting options alone would poll a NEW tag set through the OLD + // database, which is why this driver was excluded from the first pass. A null rebinder (direct + // construction) or an empty/placeholder document keeps the constructor-supplied trio. + if (_rebind is not null && HasConfigBody(driverConfigJson)) + { + var binding = _rebind(driverConfigJson); + ApplyBinding(binding.Options, binding.Dialect, binding.ConnectionString); + } + BuildTagTable(); try diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverFactoryExtensions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverFactoryExtensions.cs index ac7479e3..a4ec55ef 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverFactoryExtensions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql/SqlDriverFactoryExtensions.cs @@ -81,6 +81,43 @@ public static class SqlDriverFactoryExtensions public static SqlDriver CreateInstance(string driverInstanceId, string driverConfigJson) => CreateInstance(driverInstanceId, driverConfigJson, loggerFactory: null); + /// + /// Parses a Sql DriverConfig JSON document into every dependency the driver derives + /// from config: the typed options, the the provider key selects, and + /// the connection string resolved from connectionStringRef. + /// Returned together because they must be adopted together. Re-parsing options alone on a + /// reinitialize would run a NEW tag set against the OLD connection and dialect (Gitea #516) — a + /// half-applied config change is worse than a discarded one, because it looks like it worked. + /// Re-running this on reinitialize also re-resolves the connection string from its environment + /// variable, so a rotated credential is picked up without a process restart. + /// + /// The central config DB's identity for this driver instance. + /// The driver configuration JSON. + /// The parsed options, dialect and resolved connection string. + /// The config is malformed or names no connection string. + internal static SqlDriverBinding ParseBinding(string driverInstanceId, string driverConfigJson) + { + ArgumentException.ThrowIfNullOrWhiteSpace(driverInstanceId); + ArgumentException.ThrowIfNullOrWhiteSpace(driverConfigJson); + + var dto = Deserialize(driverInstanceId, driverConfigJson); + + if (string.IsNullOrWhiteSpace(dto.ConnectionStringRef)) + { + throw new InvalidOperationException( + $"Sql driver config for '{driverInstanceId}' is missing the required connectionStringRef. " + + $"Author the NAME of a connection string (the value itself is supplied out-of-band through " + + $"the '{SqlConnectionStringResolver.EnvironmentVariablePrefix}' environment variable)."); + } + + var dialect = CreateDialect(dto.Provider, driverInstanceId); + + // Resolved BEFORE the knob validation in CreateInstance, so that every subsequent throw is a path + // that HAS the secret in hand — which is exactly where a careless interpolation would leak it. + var connectionString = SqlConnectionStringResolver.Resolve(dto.ConnectionStringRef!); + return new SqlDriverBinding(BuildOptions(dto, driverInstanceId), dialect, connectionString, dto); + } + /// /// Logger-aware overload — used by 's closure when wired through DI. /// Constructs the driver only: the driver builds its own , because @@ -102,25 +139,11 @@ public static class SqlDriverFactoryExtensions public static SqlDriver CreateInstance( string driverInstanceId, string driverConfigJson, ILoggerFactory? loggerFactory) { - ArgumentException.ThrowIfNullOrWhiteSpace(driverInstanceId); - ArgumentException.ThrowIfNullOrWhiteSpace(driverConfigJson); - - var dto = Deserialize(driverInstanceId, driverConfigJson); - - if (string.IsNullOrWhiteSpace(dto.ConnectionStringRef)) - { - throw new InvalidOperationException( - $"Sql driver config for '{driverInstanceId}' is missing the required connectionStringRef. " + - $"Author the NAME of a connection string (the value itself is supplied out-of-band through " + - $"the '{SqlConnectionStringResolver.EnvironmentVariablePrefix}' environment variable)."); - } - - var dialect = CreateDialect(dto.Provider, driverInstanceId); - - // Resolved BEFORE the knob validation below, so that every subsequent throw is a path that HAS the - // secret in hand — which is exactly where a careless interpolation would leak it. - var connectionString = SqlConnectionStringResolver.Resolve(dto.ConnectionStringRef!); - var options = BuildOptions(dto, driverInstanceId); + var binding = ParseBinding(driverInstanceId, driverConfigJson); + var dto = binding.Dto; + var dialect = binding.Dialect; + var connectionString = binding.ConnectionString; + var options = binding.Options; var logger = (ILogger?)loggerFactory?.CreateLogger(typeof(SqlDriverFactoryExtensions).FullName!) ?? NullLogger.Instance; @@ -143,7 +166,11 @@ public static class SqlDriverFactoryExtensions dialect, connectionString, factory: null, - logger: loggerFactory?.CreateLogger()); + logger: loggerFactory?.CreateLogger(), + // Hand the driver the SAME parse it was built with, so InitializeAsync can re-derive options, + // dialect and connection string together on a reinitialize (#516). Re-resolving the connection + // string also picks up a rotated credential without a process restart. + rebind: json => ParseBinding(driverInstanceId, json)); // Endpoint is the credential-free server/database rendering — the ONLY form of the connection string // permitted outside the provider call. @@ -250,3 +277,21 @@ public static class SqlDriverFactoryExtensions } } } + +/// +/// Everything SqlDriver derives from its DriverConfig JSON, returned as one value so a +/// reinitialize adopts all of it atomically. +/// Partial adoption is the hazard this type exists to prevent: the dialect and the resolved +/// connection string are both config-derived, so adopting new alone would poll a +/// NEW tag set against the OLD database (Gitea #516). +/// +/// The typed driver options. +/// The provider seam the provider key selects. +/// The connection string resolved from connectionStringRef. Never logged. +/// The raw parsed DTO, so a caller needing a key the options do not carry (e.g. +/// allowWrites, which the driver ignores but warns about) need not re-deserialize. +public sealed record SqlDriverBinding( + SqlDriverOptions Options, + ISqlDialect Dialect, + string ConnectionString, + SqlDriverConfigDto Dto); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasReinitConfigAdoptionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasReinitConfigAdoptionTests.cs new file mode 100644 index 00000000..11057252 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests/FocasReinitConfigAdoptionTests.cs @@ -0,0 +1,67 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.FOCAS.Tests; + +/// +/// Gitea #516 — FOCAS was excluded from the first pass because it derives MORE than options from config: +/// the Backend key selects its , injected at construction. Adopting +/// a new FocasDriverOptions alone would poll a NEW device/tag set through the OLD backend, which +/// looks like it worked and does not. +/// ParseBinding now returns options + backend together and InitializeAsync adopts both +/// or neither. These tests pin the BACKEND moving — the half that was missing — because a test that only +/// checked options would have passed against the broken version. +/// +[Trait("Category", "Unit")] +public sealed class FocasReinitConfigAdoptionTests +{ + /// + /// Reinitializing onto Backend: "unimplemented" must make the driver adopt that backend, whose + /// EnsureUsable() throws by design. A driver still holding the constructor's wire + /// backend would not throw — so the throw is a direct read of which backend is live. + /// + [Fact] + public async Task Reinitialize_adopts_a_changed_backend_not_just_the_options() + { + var driver = FocasDriverFactoryExtensions.CreateInstance( + "focas-1", """{"backend":"wire","devices":[]}"""); + + var ex = await Record.ExceptionAsync(() => driver.ReinitializeAsync( + """{"backend":"unimplemented","devices":[]}""", CancellationToken.None)); + + ex.ShouldBeOfType( + "FocasDriver adopted a reinitialized config but kept its ORIGINAL backend — the half-applied " + + "adoption (#516) that would poll a new device/tag set through the old backend"); + } + + /// The other direction: moving OFF the unimplemented backend must also take effect, so the + /// adoption is not a one-way latch. + [Fact] + public async Task Reinitialize_away_from_the_unimplemented_backend_also_takes_effect() + { + var driver = FocasDriverFactoryExtensions.CreateInstance( + "focas-1", """{"backend":"unimplemented","devices":[]}"""); + + // Sanity: the constructor-supplied backend is the throwing one. + (await Record.ExceptionAsync(() => driver.InitializeAsync("{}", CancellationToken.None))) + .ShouldBeOfType(); + + var ex = await Record.ExceptionAsync(() => driver.ReinitializeAsync( + """{"backend":"wire","devices":[]}""", CancellationToken.None)); + + ex.ShouldNotBeOfType( + "FocasDriver stayed on the unimplemented backend after a reinitialize that selected 'wire'"); + } + + /// An empty/placeholder document keeps the constructor-supplied pair, so the lifecycle tests + /// that pass "{}" keep meaning what they meant. + [Fact] + public async Task Reinitialize_with_an_empty_document_keeps_the_constructor_backend() + { + var driver = FocasDriverFactoryExtensions.CreateInstance( + "focas-1", """{"backend":"unimplemented","devices":[]}"""); + + (await Record.ExceptionAsync(() => driver.ReinitializeAsync("{}", CancellationToken.None))) + .ShouldBeOfType(); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlReinitConfigAdoptionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlReinitConfigAdoptionTests.cs new file mode 100644 index 00000000..1181de1a --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests/SqlReinitConfigAdoptionTests.cs @@ -0,0 +1,120 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.Sql.Contracts; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Sql.Tests; + +/// +/// Gitea #516 — Sql was excluded from the first pass because it derives MORE than options from config +/// (its and its resolved connection string are both config-derived), so +/// adopting a new SqlDriverOptions alone would poll a NEW tag set through the OLD database. +/// A half-applied config change is worse than a discarded one, because it looks like it worked. +/// The fix is ParseBinding + ApplyBinding: one parse produces all three, and they are +/// adopted together. These tests pin the ATOMICITY, not just that a re-parse happens — that is the +/// property that made this driver special. +/// +[Trait("Category", "Unit")] +public sealed class SqlReinitConfigAdoptionTests +{ + private const string RefA = "SqlReinitTestA"; + private const string RefB = "SqlReinitTestB"; + + /// + /// A reinitialize that changes connectionStringRef must move the driver onto the NEW database. + /// Endpoint is the credential-free rendering of the live connection string, so it is a direct + /// read of which connection the driver actually adopted — no log-string matching. + /// + [Fact] + public async Task Reinitialize_adopts_a_changed_connection_string_not_just_the_options() + { + using var _ = new ScopedEnvironmentVariable( + SqlConnectionStringResolver.EnvironmentVariablePrefix + RefA, + "Server=old-server;Database=OldDb;Integrated Security=true"); + using var __ = new ScopedEnvironmentVariable( + SqlConnectionStringResolver.EnvironmentVariablePrefix + RefB, + "Server=new-server;Database=NewDb;Integrated Security=true"); + + var driver = SqlDriverFactoryExtensions.CreateInstance( + "sql-1", $$"""{"provider":"SqlServer","connectionStringRef":"{{RefA}}"}""", loggerFactory: null); + driver.Endpoint.ShouldContain("old-server"); + + // The database is unreachable, so init faults — irrelevant here. The assertion is which database it + // tried, i.e. whether the connection string moved with the options. + try + { + await driver.ReinitializeAsync( + $$"""{"provider":"SqlServer","connectionStringRef":"{{RefB}}"}""", CancellationToken.None); + } + catch + { + // Liveness failure against a non-existent server is expected. + } + + driver.Endpoint.ShouldContain( + "new-server", + customMessage: + "SqlDriver adopted a reinitialized config but kept its ORIGINAL connection string — the exact " + + "half-applied adoption (#516) that would poll a new tag set against the old database"); + driver.Endpoint.ShouldNotContain("old-server"); + } + + /// + /// A config whose connectionStringRef names an unprovisioned environment variable must make + /// the whole reinitialize FAIL, leaving the previous binding intact — never a partial adoption where + /// new options are live against the old connection. + /// + [Fact] + public async Task A_reinitialize_that_cannot_resolve_its_connection_leaves_the_previous_binding_intact() + { + using var _ = new ScopedEnvironmentVariable( + SqlConnectionStringResolver.EnvironmentVariablePrefix + RefA, + "Server=old-server;Database=OldDb;Integrated Security=true"); + + var driver = SqlDriverFactoryExtensions.CreateInstance( + "sql-1", $$"""{"provider":"SqlServer","connectionStringRef":"{{RefA}}"}""", loggerFactory: null); + + await Should.ThrowAsync(() => driver.ReinitializeAsync( + """{"provider":"SqlServer","connectionStringRef":"SqlReinitTestNeverProvisioned"}""", + CancellationToken.None)); + + driver.Endpoint.ShouldContain( + "old-server", + customMessage: + "a reinitialize that could not resolve its new connection must leave the previous binding whole"); + } + + /// An empty/placeholder document keeps the constructor-supplied binding, so the many lifecycle + /// tests that pass "{}" keep meaning what they meant. + [Fact] + public async Task Reinitialize_with_an_empty_document_keeps_the_constructor_binding() + { + using var _ = new ScopedEnvironmentVariable( + SqlConnectionStringResolver.EnvironmentVariablePrefix + RefA, + "Server=old-server;Database=OldDb;Integrated Security=true"); + + var driver = SqlDriverFactoryExtensions.CreateInstance( + "sql-1", $$"""{"provider":"SqlServer","connectionStringRef":"{{RefA}}"}""", loggerFactory: null); + + try { await driver.ReinitializeAsync("{}", CancellationToken.None); } + catch { /* liveness failure expected */ } + + driver.Endpoint.ShouldContain("old-server"); + } + + /// Sets an environment variable for the duration of a test and restores it after. The resolver + /// reads process-global state, so a leaked variable would race every sibling test. + private sealed class ScopedEnvironmentVariable : IDisposable + { + private readonly string _name; + private readonly string? _previous; + + public ScopedEnvironmentVariable(string name, string value) + { + _name = name; + _previous = Environment.GetEnvironmentVariable(name); + Environment.SetEnvironmentVariable(name, value); + } + + public void Dispose() => Environment.SetEnvironmentVariable(_name, _previous); + } +}