From 8d91a3021d171e87a26488c8169dcdf6b8bfae0b Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 1 Jun 2026 09:37:53 -0400 Subject: [PATCH] fix(config): centralize port wording, harden HostPort/key guards, doc null/singleton semantics, add tests --- .../src/ZB.MOM.WW.Configuration/Checks.cs | 14 ++++++++++++++ .../ZB.MOM.WW.Configuration/ConfigPreflight.cs | 16 ++++++++-------- .../ServiceCollectionExtensions.cs | 5 +++++ .../ZB.MOM.WW.Configuration/ValidationBuilder.cs | 6 +++++- .../ConfigPreflightTests.cs | 9 +++++++++ .../ValidationBuilderTests.cs | 10 ++++++++++ 6 files changed, 51 insertions(+), 9 deletions(-) diff --git a/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/Checks.cs b/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/Checks.cs index 1935f35..f9c1e6f 100644 --- a/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/Checks.cs +++ b/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/Checks.cs @@ -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; + /// + /// Validates a raw string as a TCP port (parse + range), returning null when valid. + /// Centralizes the port wording for callers that hold the raw config value. + /// + 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"}')"; + + /// + /// Validates a non-bracketed host:port endpoint (port 1-65535). Bracketed IPv6 + /// literals ([::1]:port) are out of scope and are rejected. + /// 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}')"; diff --git a/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ConfigPreflight.cs b/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ConfigPreflight.cs index 0ebfff6..96a5b96 100644 --- a/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ConfigPreflight.cs +++ b/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ConfigPreflight.cs @@ -31,24 +31,24 @@ public sealed class ConfigPreflight /// Requires the value at to satisfy . public ConfigPreflight Require(string key, Func predicate, string reason) { + ArgumentException.ThrowIfNullOrWhiteSpace(key); ArgumentNullException.ThrowIfNull(predicate); if (!predicate(_configuration[key])) _failures.Add($"{key} {reason}"); return this; } /// Requires a non-empty value at . - 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)); + } /// Requires a valid integer TCP port (1-65535) at . 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)); } /// Runs only when holds (role-conditional rules). diff --git a/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ServiceCollectionExtensions.cs b/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ServiceCollectionExtensions.cs index 050b2fd..7456f72 100644 --- a/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ServiceCollectionExtensions.cs +++ b/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ServiceCollectionExtensions.cs @@ -19,6 +19,11 @@ public static class ServiceCollectionExtensions /// The configuration to bind from. /// The configuration section path (e.g. "ScadaBridge:Cluster"). /// The for further chaining. + /// + /// 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. + /// public static OptionsBuilder AddValidatedOptions( this IServiceCollection services, IConfiguration configuration, string sectionPath) where TOptions : class diff --git a/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ValidationBuilder.cs b/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ValidationBuilder.cs index e221734..7055853 100644 --- a/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ValidationBuilder.cs +++ b/ZB.MOM.WW.Configuration/src/ZB.MOM.WW.Configuration/ValidationBuilder.cs @@ -42,7 +42,11 @@ public sealed class ValidationBuilder /// Requires a strictly positive duration. public ValidationBuilder PositiveTimeSpan(TimeSpan value, string field) => AddIf(Checks.PositiveTimeSpan(value, field)); - /// Requires the value to be one of (case-insensitive). + /// + /// Requires the value to be one of (case-insensitive). A + /// null value fails this rule; call first if the field may be + /// absent and you want a "required" message instead of a "must be one of" message. + /// public ValidationBuilder OneOf(string? value, IReadOnlyCollection allowed, string field) => AddIf(Checks.OneOf(value, allowed, field)); /// Requires a collection with at least items. diff --git a/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/ConfigPreflightTests.cs b/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/ConfigPreflightTests.cs index 8b8428d..a8bdca3 100644 --- a/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/ConfigPreflightTests.cs +++ b/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/ConfigPreflightTests.cs @@ -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() { diff --git a/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/ValidationBuilderTests.cs b/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/ValidationBuilderTests.cs index e28a495..96f22af 100644 --- a/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/ValidationBuilderTests.cs +++ b/ZB.MOM.WW.Configuration/tests/ZB.MOM.WW.Configuration.Tests/ValidationBuilderTests.cs @@ -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() {