fix(config): centralize port wording, harden HostPort/key guards, doc null/singleton semantics, add tests

2026-06-01 09:37:53 -04:00
parent 8145d79dc6
commit 8d91a3021d
6 changed files with 51 additions and 9 deletions
@@ -14,11 +14,25 @@ internal static class Checks
    internal static string? Port(int value, string field) =>
        value is < 1 or > 65535 ? $"{field} must be between 1 and 65535 (was {value})" : null;

+    /// <summary>
+    /// Validates a raw string as a TCP port (parse + range), returning <c>null</c> when valid.
+    /// Centralizes the port wording for callers that hold the raw config value.
+    /// </summary>
+    internal static string? PortValue(string? raw, string field) =>
+        int.TryParse(raw, out var port)
+            ? Port(port, field)
+            : $"{field} must be between 1 and 65535 (was '{raw ?? "null"}')";
+
+    /// <summary>
+    /// Validates a non-bracketed <c>host:port</c> endpoint (port 1-65535). Bracketed IPv6
+    /// literals (<c>[::1]:port</c>) are out of scope and are rejected.
+    /// </summary>
    internal static string? HostPort(string? value, string field)
    {
        if (string.IsNullOrWhiteSpace(value)) return $"{field} is required";
        var idx = value.LastIndexOf(':');
        if (idx <= 0 || idx == value.Length - 1
+            || value.AsSpan(0, idx).Contains(':')
            || !int.TryParse(value[(idx + 1)..], out var port)
            || port is < 1 or > 65535)
            return $"{field} must be 'host:port' with port 1-65535 (was '{value}')";
@@ -31,24 +31,24 @@ public sealed class ConfigPreflight
    /// <summary>Requires the value at <paramref name="key"/> to satisfy <paramref name="predicate"/>.</summary>
    public ConfigPreflight Require(string key, Func<string?, bool> predicate, string reason)
    {
+        ArgumentException.ThrowIfNullOrWhiteSpace(key);
        ArgumentNullException.ThrowIfNull(predicate);
        if (!predicate(_configuration[key])) _failures.Add($"{key} {reason}");
        return this;
    }

    /// <summary>Requires a non-empty value at <paramref name="key"/>.</summary>
-    public ConfigPreflight RequireValue(string key) => AddIf(Checks.Required(_configuration[key], key));
+    public ConfigPreflight RequireValue(string key)
+    {
+        ArgumentException.ThrowIfNullOrWhiteSpace(key);
+        return AddIf(Checks.Required(_configuration[key], key));
+    }

    /// <summary>Requires a valid integer TCP port (1-65535) at <paramref name="key"/>.</summary>
    public ConfigPreflight RequirePort(string key)
    {
-        var raw = _configuration[key];
-        if (!int.TryParse(raw, out var port))
-        {
-            _failures.Add($"{key} must be an integer port 1-65535 (was '{raw ?? "null"}')");
-            return this;
-        }
-        return AddIf(Checks.Port(port, key));
+        ArgumentException.ThrowIfNullOrWhiteSpace(key);
+        return AddIf(Checks.PortValue(_configuration[key], key));
    }

    /// <summary>Runs <paramref name="block"/> only when <paramref name="condition"/> holds (role-conditional rules).</summary>
@@ -19,6 +19,11 @@ public static class ServiceCollectionExtensions
    /// <param name="configuration">The configuration to bind from.</param>
    /// <param name="sectionPath">The configuration section path (e.g. <c>"ScadaBridge:Cluster"</c>).</param>
    /// <returns>The <see cref="OptionsBuilder{TOptions}"/> for further chaining.</returns>
+    /// <remarks>
+    /// <typeparamref name="TValidator"/> is registered as a singleton (it is consumed by the
+    /// singleton options factory). It must therefore be safe to use as a singleton — do not
+    /// inject scoped dependencies into it.
+    /// </remarks>
    public static OptionsBuilder<TOptions> AddValidatedOptions<TOptions, TValidator>(
        this IServiceCollection services, IConfiguration configuration, string sectionPath)
        where TOptions : class
@@ -42,7 +42,11 @@ public sealed class ValidationBuilder
    /// <summary>Requires a strictly positive duration.</summary>
    public ValidationBuilder PositiveTimeSpan(TimeSpan value, string field) => AddIf(Checks.PositiveTimeSpan(value, field));

-    /// <summary>Requires the value to be one of <paramref name="allowed"/> (case-insensitive).</summary>
+    /// <summary>
+    /// Requires the value to be one of <paramref name="allowed"/> (case-insensitive). A
+    /// <c>null</c> value fails this rule; call <see cref="Required"/> first if the field may be
+    /// absent and you want a "required" message instead of a "must be one of" message.
+    /// </summary>
    public ValidationBuilder OneOf(string? value, IReadOnlyCollection<string> allowed, string field) => AddIf(Checks.OneOf(value, allowed, field));

    /// <summary>Requires a collection with at least <paramref name="min"/> items.</summary>
@@ -30,6 +30,15 @@ public sealed class ConfigPreflightTests
        Assert.Contains(pf.Failures, f => f.Contains("Node:SiteId"));
    }

+    [Fact]
+    public void When_false_does_not_run_block()
+    {
+        var cfg = Config(new() { ["Node:Role"] = "Central" });
+        var pf = ConfigPreflight.For(cfg)
+            .When(cfg["Node:Role"] == "Site", p => p.RequireValue("Node:SiteId"));
+        Assert.True(pf.IsValid);   // block skipped, no failure recorded
+    }
+
    [Fact]
    public void ThrowIfInvalid_throws_aggregated_message()
    {
@@ -33,6 +33,7 @@ public sealed class ValidationBuilderTests
    [InlineData("host", false)]
    [InlineData("host:0", false)]
    [InlineData("host:notaport", false)]
+    [InlineData("::1", false)]
    public void HostPort_validates_endpoint(string value, bool valid)
    {
        var b = new ValidationBuilder();
@@ -56,6 +57,15 @@ public sealed class ValidationBuilderTests
        Assert.True(b.IsValid);
    }

+    [Fact]
+    public void OneOf_null_value_fails()
+    {
+        var b = new ValidationBuilder();
+        b.OneOf(null, new[] { "Central", "Site" }, "X:Role");
+        Assert.False(b.IsValid);
+        Assert.Contains(b.Failures, f => f.Contains("X:Role"));
+    }
+
    [Fact]
    public void MinCount_requires_minimum()
    {