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); + } +}