fix(localdb): DI review fixes — onReady leak guard, BusyTimeoutMs ctor guard, shared allow-list

2026-07-17 21:47:28 -04:00
parent 529585163c
commit 4f9982c53f
6 changed files with 53 additions and 12 deletions
@@ -23,17 +23,31 @@ public static class LocalDbServiceCollectionExtensions
        IConfiguration configuration,
        Action<ILocalDb>? onReady = null)
    {
+        // Idempotent: a second call must not re-bind options or capture a second onReady closure.
+        if (services.Any(d => d.ServiceType == typeof(ILocalDb)))
+            return services;
+
        services.AddOptions<LocalDbOptions>()
            .Bind(configuration.GetSection("LocalDb"))
            .ValidateOnStart();
        services.TryAddEnumerable(
            ServiceDescriptor.Singleton<IValidateOptions<LocalDbOptions>, LocalDbOptionsValidator>());

-        services.TryAddSingleton<ILocalDb>(sp =>
+        services.AddSingleton<ILocalDb>(sp =>
        {
            var options = sp.GetRequiredService<IOptions<LocalDbOptions>>().Value;
            var db = new SqliteLocalDb(options);
-            onReady?.Invoke(db);
+            try
+            {
+                onReady?.Invoke(db);
+            }
+            catch
+            {
+                // A throwing callback must not leak the open master connection: MS.DI does not
+                // dispose instances a failed factory never returned.
+                db.Dispose();
+                throw;
+            }
            return db;
        });

@@ -14,9 +14,6 @@ namespace ZB.MOM.WW.LocalDb.Internal;
 /// </summary>
 internal sealed class SqliteLocalDb : ILocalDb, IDisposable
 {
-    // Allow-list because the value is interpolated into a PRAGMA statement (pragmas cannot be parameterized).
-    private static readonly string[] AllowedSynchronous = ["OFF", "NORMAL", "FULL", "EXTRA"];
-
    // Gates every registered table name: it is interpolated into PRAGMA table_info and the trigger DDL.
    private static readonly Regex TableNameRegex = new("^[A-Za-z_][A-Za-z0-9_]*$", RegexOptions.Compiled);

@@ -45,10 +42,15 @@ internal sealed class SqliteLocalDb : ILocalDb, IDisposable
        if (string.IsNullOrWhiteSpace(options.Path))
            throw new ArgumentException("LocalDbOptions.Path is required.", nameof(options));

-        _synchronous = Array.Find(AllowedSynchronous,
+        _synchronous = Array.Find(LocalDbOptionsValidator.AllowedSynchronous,
                s => s.Equals(options.Synchronous, StringComparison.OrdinalIgnoreCase))
            ?? throw new ArgumentException(
-                $"LocalDbOptions.Synchronous must be one of {string.Join("/", AllowedSynchronous)}; got '{options.Synchronous}'.",
+                $"LocalDbOptions.Synchronous must be one of {string.Join("/", LocalDbOptionsValidator.AllowedSynchronous)}; got '{options.Synchronous}'.",
+                nameof(options));
+
+        if (options.BusyTimeoutMs <= 0)
+            throw new ArgumentException(
+                $"LocalDbOptions.BusyTimeoutMs must be greater than 0; got {options.BusyTimeoutMs}.",
                nameof(options));
        _busyTimeoutMs = options.BusyTimeoutMs;
        _connectionString = new SqliteConnectionStringBuilder { DataSource = options.Path }.ToString();
@@ -229,9 +231,9 @@ internal sealed class SqliteLocalDb : ILocalDb, IDisposable
            // it is IEnumerable but a legitimate reflection-fallback target is never a bare string anyway.
            case System.Collections.IEnumerable when parameters is not string:
                throw new ArgumentException(
-                    "Unsupported parameters type: pass an anonymous object / POCO (public properties bind as @Name) " +
-                    "or an IDictionary<string, object?> (entries bind as @Key). A Dictionary<string, string>, Hashtable, " +
-                    "array, or other collection is not supported.",
+                    $"Unsupported parameters type '{parameters.GetType()}': pass an anonymous object / POCO " +
+                    "(public properties bind as @Name) or an IDictionary<string, object?> (entries bind as @Key). " +
+                    "A Dictionary<string, string>, Hashtable, array, or other collection is not supported.",
                    nameof(parameters));
            default:
                foreach (var prop in PropertiesOf(parameters.GetType()))
@@ -9,8 +9,9 @@ namespace ZB.MOM.WW.LocalDb;
 /// </summary>
 public sealed class LocalDbOptionsValidator : IValidateOptions<LocalDbOptions>
 {
-    // Interpolated into a PRAGMA statement by the host (pragmas cannot be parameterized), so it is allow-listed.
-    private static readonly string[] AllowedSynchronous = ["OFF", "NORMAL", "FULL", "EXTRA"];
+    // Interpolated into a PRAGMA statement by the host (pragmas cannot be parameterized), so it is
+    // allow-listed. Shared with the SqliteLocalDb constructor so both enforce the identical set.
+    internal static readonly string[] AllowedSynchronous = ["OFF", "NORMAL", "FULL", "EXTRA"];

    public ValidateOptionsResult Validate(string? name, LocalDbOptions options)
    {
@@ -56,6 +56,21 @@ public sealed class DependencyInjectionTests : IDisposable
        Assert.Contains("widget", db.ReplicatedTables.Keys);
    }

+    [Fact]
+    public void AddZbLocalDb_OnReadyThrows_ExceptionPropagates()
+    {
+        var services = new ServiceCollection();
+        services.AddZbLocalDb(Config(), _ => throw new InvalidOperationException("boom"));
+        using var provider = services.BuildServiceProvider();
+
+        var first = Assert.Throws<InvalidOperationException>(() => provider.GetRequiredService<ILocalDb>());
+        Assert.Equal("boom", first.Message);
+
+        // No half-initialized singleton is cached: the next resolution re-runs the factory and fails again.
+        var second = Assert.Throws<InvalidOperationException>(() => provider.GetRequiredService<ILocalDb>());
+        Assert.Equal("boom", second.Message);
+    }
+
    [Fact]
    public void Validator_EmptyPath_Fails()
    {
@@ -141,6 +141,13 @@ public sealed class SqliteLocalDbTests : IDisposable
        Assert.Throws<ArgumentException>(() => new SqliteLocalDb(new LocalDbOptions { Path = " " }));
    }

+    [Fact]
+    public void Ctor_NonPositiveBusyTimeout_Throws()
+    {
+        Assert.Throws<ArgumentException>(() =>
+            new SqliteLocalDb(new LocalDbOptions { Path = _path, BusyTimeoutMs = 0 }));
+    }
+
    [Fact]
    public void Ctor_InvalidSynchronous_Throws()
    {
@@ -294,6 +294,8 @@ END;

 **Steps (TDD):** tests: `AddZbLocalDb_ResolvesILocalDb_Singleton`, `AddZbLocalDb_WithRegistrations_TablesRegisteredOnStart`, `Validator_EmptyPath_Fails`, `Validator_NonPositiveBusyTimeout_Fails`. Implement `AddZbLocalDb(IConfiguration config, Action<LocalDbRegistrationBuilder>? register = null)`: binds `LocalDb` section, `IValidateOptions` validator (plain `IValidateOptions<LocalDbOptions>` — do NOT take a package dep on `ZB.MOM.WW.Configuration`; keep this lib dependency-light like Audit), registers `SqliteLocalDb` as singleton `ILocalDb`; the builder collects table names, applied at first resolution. Commit `feat(localdb): options validation + AddZbLocalDb DI`.

+> **Shipped deviation:** the registration callback shipped as a plain `Action<ILocalDb>? onReady` (invoked once at first resolution, where consumers create tables + call `RegisterReplicated`) instead of the planned `Action<LocalDbRegistrationBuilder>` — the builder indirection added no value over handing consumers the `ILocalDb` directly.
+
 ---

 ### Task 7: Contracts — localdb_sync.v1 proto