feat(options): eager startup validation for Communication + DataConnectionLayer options (arch-review 08 §1.5)

Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
2026-07-10 06:48:06 -04:00
parent 187fbd7cc8
commit af2cfde484
8 changed files with 203 additions and 2 deletions
@@ -0,0 +1,69 @@
+using ZB.MOM.WW.Configuration;
+
+namespace ZB.MOM.WW.ScadaBridge.Communication;
+
+/// <summary>
+/// Validates <see cref="CommunicationOptions"/> at startup (ValidateOnStart) so a
+/// malformed "Communication" appsettings section fails fast at boot with a
+/// key-naming message instead of surfacing later at first Ask/gRPC use. Every
+/// timeout feeds a per-pattern Ask deadline or a gRPC keepalive/stream-lifetime
+/// setting, and a zero/negative value there produces an opaque runtime failure
+/// far from the offending config key.
+/// </summary>
+public sealed class CommunicationOptionsValidator : OptionsValidatorBase<CommunicationOptions>
+{
+    /// <inheritdoc />
+    protected override void Validate(ValidationBuilder builder, CommunicationOptions options)
+    {
+        builder.RequireThat(options.DeploymentTimeout > TimeSpan.Zero,
+            $"Communication:DeploymentTimeout must be a positive duration (was {options.DeploymentTimeout}).");
+
+        builder.RequireThat(options.LifecycleTimeout > TimeSpan.Zero,
+            $"Communication:LifecycleTimeout must be a positive duration (was {options.LifecycleTimeout}).");
+
+        builder.RequireThat(options.ArtifactDeploymentTimeout > TimeSpan.Zero,
+            $"Communication:ArtifactDeploymentTimeout must be a positive duration (was {options.ArtifactDeploymentTimeout}).");
+
+        builder.RequireThat(options.QueryTimeout > TimeSpan.Zero,
+            $"Communication:QueryTimeout must be a positive duration (was {options.QueryTimeout}).");
+
+        builder.RequireThat(options.IntegrationTimeout > TimeSpan.Zero,
+            $"Communication:IntegrationTimeout must be a positive duration (was {options.IntegrationTimeout}).");
+
+        builder.RequireThat(options.DebugViewTimeout > TimeSpan.Zero,
+            $"Communication:DebugViewTimeout must be a positive duration (was {options.DebugViewTimeout}).");
+
+        builder.RequireThat(options.HealthReportTimeout > TimeSpan.Zero,
+            $"Communication:HealthReportTimeout must be a positive duration (was {options.HealthReportTimeout}).");
+
+        builder.RequireThat(options.NotificationForwardTimeout > TimeSpan.Zero,
+            $"Communication:NotificationForwardTimeout must be a positive duration (was {options.NotificationForwardTimeout}).");
+
+        builder.RequireThat(options.GrpcKeepAlivePingDelay > TimeSpan.Zero,
+            $"Communication:GrpcKeepAlivePingDelay must be a positive duration (was {options.GrpcKeepAlivePingDelay}).");
+
+        builder.RequireThat(options.GrpcKeepAlivePingTimeout > TimeSpan.Zero,
+            $"Communication:GrpcKeepAlivePingTimeout must be a positive duration (was {options.GrpcKeepAlivePingTimeout}).");
+
+        builder.RequireThat(options.GrpcMaxStreamLifetime > TimeSpan.Zero,
+            $"Communication:GrpcMaxStreamLifetime must be a positive duration (was {options.GrpcMaxStreamLifetime}).");
+
+        builder.RequireThat(options.TransportHeartbeatInterval > TimeSpan.Zero,
+            $"Communication:TransportHeartbeatInterval must be a positive duration (was {options.TransportHeartbeatInterval}).");
+
+        builder.RequireThat(options.ApplicationHeartbeatInterval > TimeSpan.Zero,
+            $"Communication:ApplicationHeartbeatInterval must be a positive duration (was {options.ApplicationHeartbeatInterval}).");
+
+        builder.RequireThat(options.TransportFailureThreshold > TimeSpan.Zero,
+            $"Communication:TransportFailureThreshold must be a positive duration (was {options.TransportFailureThreshold}).");
+
+        builder.RequireThat(options.PendingDeploymentTtl > TimeSpan.Zero,
+            $"Communication:PendingDeploymentTtl must be a positive duration (was {options.PendingDeploymentTtl}).");
+
+        builder.RequireThat(options.PendingDeploymentPurgeInterval > TimeSpan.Zero,
+            $"Communication:PendingDeploymentPurgeInterval must be a positive duration (was {options.PendingDeploymentPurgeInterval}).");
+
+        builder.RequireThat(options.GrpcMaxConcurrentStreams > 0,
+            $"Communication:GrpcMaxConcurrentStreams must be positive (was {options.GrpcMaxConcurrentStreams}).");
+    }
+}
@@ -1,4 +1,6 @@
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.DependencyInjection.Extensions;
+using Microsoft.Extensions.Options;
 using ZB.MOM.WW.ScadaBridge.Communication.Grpc;

 namespace ZB.MOM.WW.ScadaBridge.Communication;
@@ -11,7 +13,10 @@ public static class ServiceCollectionExtensions
    public static IServiceCollection AddCommunication(this IServiceCollection services)
    {
        services.AddOptions<CommunicationOptions>()
-            .BindConfiguration("Communication");
+            .BindConfiguration("Communication")
+            .ValidateOnStart();
+        services.TryAddEnumerable(
+            ServiceDescriptor.Singleton<IValidateOptions<CommunicationOptions>, CommunicationOptionsValidator>());

        services.AddSingleton<CommunicationService>();
        services.AddSingleton<SiteStreamGrpcClientFactory>();
@@ -24,6 +24,7 @@
    <PackageReference Include="Google.Protobuf" />
    <PackageReference Include="Grpc.Net.Client" />
    <PackageReference Include="Grpc.Tools" PrivateAssets="All" />
+    <PackageReference Include="ZB.MOM.WW.Configuration" />
  </ItemGroup>

  <ItemGroup>
@@ -0,0 +1,39 @@
+using ZB.MOM.WW.Configuration;
+
+namespace ZB.MOM.WW.ScadaBridge.DataConnectionLayer;
+
+/// <summary>
+/// Validates <see cref="DataConnectionOptions"/> at startup (ValidateOnStart) so a
+/// malformed "DataConnectionLayer" appsettings section fails fast at boot with a
+/// key-naming message instead of surfacing later at runtime. The intervals drive
+/// the reconnect/tag-resolution timers and the seed-read retry loop; a
+/// zero/negative duration (or a non-positive <c>SeedReadMaxAttempts</c>) would
+/// otherwise produce an opaque failure far from the offending config key.
+/// </summary>
+public sealed class DataConnectionOptionsValidator : OptionsValidatorBase<DataConnectionOptions>
+{
+    /// <inheritdoc />
+    protected override void Validate(ValidationBuilder builder, DataConnectionOptions options)
+    {
+        builder.RequireThat(options.ReconnectInterval > TimeSpan.Zero,
+            $"DataConnectionLayer:ReconnectInterval must be a positive duration (was {options.ReconnectInterval}).");
+
+        builder.RequireThat(options.TagResolutionRetryInterval > TimeSpan.Zero,
+            $"DataConnectionLayer:TagResolutionRetryInterval must be a positive duration (was {options.TagResolutionRetryInterval}).");
+
+        builder.RequireThat(options.WriteTimeout > TimeSpan.Zero,
+            $"DataConnectionLayer:WriteTimeout must be a positive duration (was {options.WriteTimeout}).");
+
+        builder.RequireThat(options.SeedReadTimeout > TimeSpan.Zero,
+            $"DataConnectionLayer:SeedReadTimeout must be a positive duration (was {options.SeedReadTimeout}).");
+
+        builder.RequireThat(options.StableConnectionThreshold > TimeSpan.Zero,
+            $"DataConnectionLayer:StableConnectionThreshold must be a positive duration (was {options.StableConnectionThreshold}).");
+
+        builder.RequireThat(options.SeedReadRetryDelay > TimeSpan.Zero,
+            $"DataConnectionLayer:SeedReadRetryDelay must be a positive duration (was {options.SeedReadRetryDelay}).");
+
+        builder.RequireThat(options.SeedReadMaxAttempts > 0,
+            $"DataConnectionLayer:SeedReadMaxAttempts must be positive (was {options.SeedReadMaxAttempts}).");
+    }
+}
@@ -1,4 +1,6 @@
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.DependencyInjection.Extensions;
+using Microsoft.Extensions.Options;

 namespace ZB.MOM.WW.ScadaBridge.DataConnectionLayer;

@@ -13,8 +15,14 @@ public static class ServiceCollectionExtensions
    public static IServiceCollection AddDataConnectionLayer(this IServiceCollection services)
    {
        services.AddOptions<DataConnectionOptions>()
-            .BindConfiguration("DataConnectionLayer");
+            .BindConfiguration("DataConnectionLayer")
+            .ValidateOnStart();
+        services.TryAddEnumerable(
+            ServiceDescriptor.Singleton<IValidateOptions<DataConnectionOptions>, DataConnectionOptionsValidator>());

+        // OpcUaGlobalOptions / MxGatewayGlobalOptions carry only string identity/path
+        // fields (no numeric or duration knobs), so there is nothing to range-validate
+        // — no dedicated validator is registered for them.
        services.AddOptions<OpcUaGlobalOptions>()
            .BindConfiguration("OpcUa");

@@ -17,6 +17,7 @@
    <PackageReference Include="Microsoft.Extensions.Options.ConfigurationExtensions" />
    <PackageReference Include="OPCFoundation.NetStandard.Opc.Ua.Client" />
    <PackageReference Include="ZB.MOM.WW.MxGateway.Client" />
+    <PackageReference Include="ZB.MOM.WW.Configuration" />
  </ItemGroup>

  <ItemGroup>
@@ -0,0 +1,39 @@
+using Microsoft.Extensions.Options;
+
+namespace ZB.MOM.WW.ScadaBridge.Communication.Tests;
+
+/// <summary>
+/// Regression: <see cref="CommunicationOptions"/> timeouts feed per-pattern Ask
+/// deadlines and gRPC keepalive/stream-lifetime settings; a zero/negative value
+/// (or a non-positive <c>GrpcMaxConcurrentStreams</c>) must be rejected at startup
+/// by an <see cref="IValidateOptions{TOptions}"/> with a clear, key-naming message
+/// rather than surfacing at first Ask/gRPC use.
+/// </summary>
+public class CommunicationOptionsValidatorTests
+{
+    private static ValidateOptionsResult Validate(CommunicationOptions options) =>
+        new CommunicationOptionsValidator().Validate(Options.DefaultName, options);
+
+    [Fact]
+    public void DefaultOptions_AreValid()
+    {
+        var result = Validate(new CommunicationOptions());
+        Assert.True(result.Succeeded, result.FailureMessage);
+    }
+
+    [Fact]
+    public void ZeroDeploymentTimeout_IsRejected()
+    {
+        var result = Validate(new CommunicationOptions { DeploymentTimeout = TimeSpan.Zero });
+        Assert.True(result.Failed);
+        Assert.Contains("DeploymentTimeout", result.FailureMessage);
+    }
+
+    [Fact]
+    public void NonPositiveGrpcMaxConcurrentStreams_IsRejected()
+    {
+        var result = Validate(new CommunicationOptions { GrpcMaxConcurrentStreams = 0 });
+        Assert.True(result.Failed);
+        Assert.Contains("GrpcMaxConcurrentStreams", result.FailureMessage);
+    }
+}
@@ -0,0 +1,39 @@
+using Microsoft.Extensions.Options;
+
+namespace ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests;
+
+/// <summary>
+/// Regression: <see cref="DataConnectionOptions"/> intervals/timeouts drive the
+/// DCL reconnect/tag-resolution timers and the seed-read retry loop; a
+/// zero/negative duration (or a non-positive <c>SeedReadMaxAttempts</c>) must be
+/// rejected at startup by an <see cref="IValidateOptions{TOptions}"/> with a
+/// clear, key-naming message rather than surfacing later at runtime.
+/// </summary>
+public class DataConnectionOptionsValidatorTests
+{
+    private static ValidateOptionsResult Validate(DataConnectionOptions options) =>
+        new DataConnectionOptionsValidator().Validate(Options.DefaultName, options);
+
+    [Fact]
+    public void DefaultOptions_AreValid()
+    {
+        var result = Validate(new DataConnectionOptions());
+        Assert.True(result.Succeeded, result.FailureMessage);
+    }
+
+    [Fact]
+    public void ZeroReconnectInterval_IsRejected()
+    {
+        var result = Validate(new DataConnectionOptions { ReconnectInterval = TimeSpan.Zero });
+        Assert.True(result.Failed);
+        Assert.Contains("ReconnectInterval", result.FailureMessage);
+    }
+
+    [Fact]
+    public void NonPositiveSeedReadMaxAttempts_IsRejected()
+    {
+        var result = Validate(new DataConnectionOptions { SeedReadMaxAttempts = 0 });
+        Assert.True(result.Failed);
+        Assert.Contains("SeedReadMaxAttempts", result.FailureMessage);
+    }
+}