From 7943d73e710b2ca7b3ed5b3bd534f783944ed569 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 10:58:28 -0400 Subject: [PATCH] fix(options): canonicalize Communication/DataConnection config sections to ScadaBridge:*; drop the duplicate Host bindings that were masking the drift (plan R2-08 T9, arch-review 08r2 NF8) --- .../CommunicationOptionsValidator.cs | 44 +++++++++---------- .../ServiceCollectionExtensions.cs | 6 ++- .../DataConnectionOptionsValidator.cs | 16 +++---- .../MxGatewayGlobalOptions.cs | 4 +- .../OpcUaGlobalOptions.cs | 4 +- .../ServiceCollectionExtensions.cs | 14 ++++-- .../SiteServiceRegistration.cs | 13 ++++-- .../CommunicationOptionsBindingTests.cs | 30 +++++++++++++ .../DataConnectionOptionsBindingTests.cs | 30 +++++++++++++ 9 files changed, 119 insertions(+), 42 deletions(-) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/CommunicationOptionsBindingTests.cs create mode 100644 tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/DataConnectionOptionsBindingTests.cs diff --git a/src/ZB.MOM.WW.ScadaBridge.Communication/CommunicationOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.Communication/CommunicationOptionsValidator.cs index 19236555..f2f8cb99 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Communication/CommunicationOptionsValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Communication/CommunicationOptionsValidator.cs @@ -4,7 +4,7 @@ namespace ZB.MOM.WW.ScadaBridge.Communication; /// /// Validates at startup (ValidateOnStart) so a -/// malformed "Communication" appsettings section fails fast at boot with a +/// malformed "ScadaBridge: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 @@ -16,74 +16,74 @@ public sealed class CommunicationOptionsValidator : OptionsValidatorBase TimeSpan.Zero, - $"Communication:DeploymentTimeout must be a positive duration (was {options.DeploymentTimeout})."); + $"ScadaBridge: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})."); + $"ScadaBridge: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})."); + $"ScadaBridge: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})."); + $"ScadaBridge: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})."); + $"ScadaBridge: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})."); + $"ScadaBridge: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})."); + $"ScadaBridge: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})."); + $"ScadaBridge: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})."); + $"ScadaBridge: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})."); + $"ScadaBridge: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})."); + $"ScadaBridge: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})."); + $"ScadaBridge: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})."); + $"ScadaBridge: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})."); + $"ScadaBridge: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})."); + $"ScadaBridge: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})."); + $"ScadaBridge:Communication:PendingDeploymentPurgeInterval must be a positive duration (was {options.PendingDeploymentPurgeInterval})."); builder.RequireThat(options.GrpcMaxConcurrentStreams > 0, - $"Communication:GrpcMaxConcurrentStreams must be positive (was {options.GrpcMaxConcurrentStreams})."); + $"ScadaBridge:Communication:GrpcMaxConcurrentStreams must be positive (was {options.GrpcMaxConcurrentStreams})."); // ── Aggregated live alarm cache (plan #10, Task 6) ─────────────────────── // Linger drives a Timer dueTime; TimeSpan.Zero is valid (stop the aggregator // immediately when the last viewer leaves), only a negative value is invalid. builder.RequireThat(options.LiveAlarmCacheLinger >= TimeSpan.Zero, - $"Communication:LiveAlarmCacheLinger must be zero or a positive duration (was {options.LiveAlarmCacheLinger})."); + $"ScadaBridge:Communication:LiveAlarmCacheLinger must be zero or a positive duration (was {options.LiveAlarmCacheLinger})."); // Reconcile interval is a periodic timer cadence AND the start-retry cadence; a // zero/negative value would spin or never fire. builder.RequireThat(options.LiveAlarmCacheReconcileInterval > TimeSpan.Zero, - $"Communication:LiveAlarmCacheReconcileInterval must be a positive duration (was {options.LiveAlarmCacheReconcileInterval})."); + $"ScadaBridge:Communication:LiveAlarmCacheReconcileInterval must be a positive duration (was {options.LiveAlarmCacheReconcileInterval})."); // Seed fan-out concurrency gates a SemaphoreSlim; at least 1, capped so a // fat-fingered config can't open an unbounded burst of cross-cluster Asks. builder.RequireThat(options.LiveAlarmCacheSeedConcurrency is >= 1 and <= 64, - $"Communication:LiveAlarmCacheSeedConcurrency must be between 1 and 64 (was {options.LiveAlarmCacheSeedConcurrency})."); + $"ScadaBridge:Communication:LiveAlarmCacheSeedConcurrency must be between 1 and 64 (was {options.LiveAlarmCacheSeedConcurrency})."); // Per-site viewer cap must admit at least one viewer, else the page could never go live. builder.RequireThat(options.LiveAlarmCacheMaxSubscribersPerSite >= 1, - $"Communication:LiveAlarmCacheMaxSubscribersPerSite must be at least 1 (was {options.LiveAlarmCacheMaxSubscribersPerSite})."); + $"ScadaBridge:Communication:LiveAlarmCacheMaxSubscribersPerSite must be at least 1 (was {options.LiveAlarmCacheMaxSubscribersPerSite})."); } } diff --git a/src/ZB.MOM.WW.ScadaBridge.Communication/ServiceCollectionExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.Communication/ServiceCollectionExtensions.cs index 0fb9ccdd..27838bc4 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Communication/ServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Communication/ServiceCollectionExtensions.cs @@ -12,8 +12,12 @@ public static class ServiceCollectionExtensions /// The same to allow chaining. public static IServiceCollection AddCommunication(this IServiceCollection services) { + // Canonical section is ScadaBridge:Communication — the section every real + // appsettings actually writes (arch-review 08 round 2 NF8). The Host used to + // supply the real values via a duplicate Configure<> binding; that duplicate is + // gone, so AddCommunication must bind the canonical section itself. services.AddOptions() - .BindConfiguration("Communication") + .BindConfiguration("ScadaBridge:Communication") .ValidateOnStart(); services.TryAddEnumerable( ServiceDescriptor.Singleton, CommunicationOptionsValidator>()); diff --git a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/DataConnectionOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/DataConnectionOptionsValidator.cs index 1379bddd..e80dce81 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/DataConnectionOptionsValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/DataConnectionOptionsValidator.cs @@ -4,7 +4,7 @@ namespace ZB.MOM.WW.ScadaBridge.DataConnectionLayer; /// /// Validates at startup (ValidateOnStart) so a -/// malformed "DataConnectionLayer" appsettings section fails fast at boot with a +/// malformed "ScadaBridge:DataConnection" 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 SeedReadMaxAttempts) would @@ -16,24 +16,24 @@ public sealed class DataConnectionOptionsValidator : OptionsValidatorBase TimeSpan.Zero, - $"DataConnectionLayer:ReconnectInterval must be a positive duration (was {options.ReconnectInterval})."); + $"ScadaBridge:DataConnection: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})."); + $"ScadaBridge:DataConnection: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})."); + $"ScadaBridge:DataConnection: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})."); + $"ScadaBridge:DataConnection: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})."); + $"ScadaBridge:DataConnection: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})."); + $"ScadaBridge:DataConnection:SeedReadRetryDelay must be a positive duration (was {options.SeedReadRetryDelay})."); builder.RequireThat(options.SeedReadMaxAttempts > 0, - $"DataConnectionLayer:SeedReadMaxAttempts must be positive (was {options.SeedReadMaxAttempts})."); + $"ScadaBridge:DataConnection:SeedReadMaxAttempts must be positive (was {options.SeedReadMaxAttempts})."); } } diff --git a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/MxGatewayGlobalOptions.cs b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/MxGatewayGlobalOptions.cs index 190f1017..464f6782 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/MxGatewayGlobalOptions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/MxGatewayGlobalOptions.cs @@ -1,8 +1,8 @@ namespace ZB.MOM.WW.ScadaBridge.DataConnectionLayer; /// -/// Deployment-wide MxGateway defaults, bound from the "MxGateway" section of -/// appsettings.json. Per-endpoint behavior lives on MxGatewayEndpointConfig. +/// Deployment-wide MxGateway defaults, bound from the "ScadaBridge:MxGateway" section +/// of appsettings.json. Per-endpoint behavior lives on MxGatewayEndpointConfig. /// public class MxGatewayGlobalOptions { diff --git a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/OpcUaGlobalOptions.cs b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/OpcUaGlobalOptions.cs index 85754ed0..d423005f 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/OpcUaGlobalOptions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/OpcUaGlobalOptions.cs @@ -1,8 +1,8 @@ namespace ZB.MOM.WW.ScadaBridge.DataConnectionLayer; /// -/// Deployment-wide OPC UA application identity. Bound from the "OpcUa" section -/// of appsettings.json. Per-endpoint behavior lives on OpcUaEndpointConfig. +/// Deployment-wide OPC UA application identity. Bound from the "ScadaBridge:OpcUa" +/// section of appsettings.json. Per-endpoint behavior lives on OpcUaEndpointConfig. /// Empty paths fall back to a default under Path.GetTempPath() so dev runs /// work without explicit configuration. /// diff --git a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/ServiceCollectionExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/ServiceCollectionExtensions.cs index 5f175071..f4268d9c 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/ServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DataConnectionLayer/ServiceCollectionExtensions.cs @@ -14,20 +14,26 @@ public static class ServiceCollectionExtensions /// The same instance for chaining. public static IServiceCollection AddDataConnectionLayer(this IServiceCollection services) { + // Canonical section is ScadaBridge:DataConnection — the section every real + // appsettings actually writes (arch-review 08 round 2 NF8). The Host used to + // supply the real values via a duplicate Configure<> binding; that duplicate is + // gone, so AddDataConnectionLayer must bind the canonical section itself. services.AddOptions() - .BindConfiguration("DataConnectionLayer") + .BindConfiguration("ScadaBridge:DataConnection") .ValidateOnStart(); services.TryAddEnumerable( ServiceDescriptor.Singleton, 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. + // — no dedicated validator is registered for them. Canonical sections are + // ScadaBridge:OpcUa / ScadaBridge:MxGateway (NF8): the bare "OpcUa"/"MxGateway" + // sections exist in no appsettings, so binding them read nothing. services.AddOptions() - .BindConfiguration("OpcUa"); + .BindConfiguration("ScadaBridge:OpcUa"); services.AddOptions() - .BindConfiguration("MxGateway"); + .BindConfiguration("ScadaBridge:MxGateway"); // Register the factory for protocol extensibility services.AddSingleton(); diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs b/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs index b0d33603..36327763 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/SiteServiceRegistration.cs @@ -149,7 +149,9 @@ public static class SiteServiceRegistration ServiceDescriptor.Singleton, SiteRuntime.Tracking.OperationTrackingOptionsValidator>()); - services.Configure(config.GetSection("ScadaBridge:DataConnection")); + // NF8: DataConnectionOptions is now bound by AddDataConnectionLayer() itself + // (canonical ScadaBridge:DataConnection section) — the duplicate Host binding + // that used to mask the SCE's wrong section name is deleted. services.AddOptions() .Bind(config.GetSection("ScadaBridge:StoreAndForward")) @@ -184,12 +186,17 @@ public static class SiteServiceRegistration services.AddOptions().Bind(config.GetSection("ScadaBridge:Database")).ValidateOnStart(); services.TryAddEnumerable( ServiceDescriptor.Singleton, DatabaseOptionsValidator>()); - services.Configure(config.GetSection("ScadaBridge:Communication")); + // NF8: CommunicationOptions is now bound by AddCommunication() itself (canonical + // ScadaBridge:Communication section) — the duplicate Host binding that used to mask + // the SCE's wrong section name is deleted. // Bind + eagerly validate: HealthMonitoringOptionsValidator is registered (TryAddEnumerable) // by the HealthMonitoring module, so chaining ValidateOnStart() here makes a bad // ScadaBridge:HealthMonitoring section fail fast at host build instead of lazily on first resolve. services.AddOptions().Bind(config.GetSection("ScadaBridge:HealthMonitoring")).ValidateOnStart(); - services.Configure(config.GetSection("ScadaBridge:Notification")); + // NF8: NotificationOptions is bound by AddNotificationService() (central-only, + // canonical ScadaBridge:Notification section). Sites don't deliver notifications + // and have no IOptions consumer, so the redundant Host + // duplicate that bound it on both paths is deleted. // Bind + eagerly validate: an unrecognised MinimumLevel would silently fall back // to Information; fail fast at boot so the operator's intended floor is honoured // (arch-review 08r2 NF4). diff --git a/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/CommunicationOptionsBindingTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/CommunicationOptionsBindingTests.cs new file mode 100644 index 00000000..ee9e292a --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.Communication.Tests/CommunicationOptionsBindingTests.cs @@ -0,0 +1,30 @@ +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; + +namespace ZB.MOM.WW.ScadaBridge.Communication.Tests; + +/// +/// Locks the config section +/// binds (arch-review 08 round 2 NF8). Every real appsettings nests this section under +/// ScadaBridge:; binding a root-level Communication section reads config +/// that no deployment writes — the drift the Host duplicate was silently masking. +/// AddCommunication alone must bind the canonical ScadaBridge:Communication section. +/// +public class CommunicationOptionsBindingTests +{ + [Fact] + public void AddCommunication_BindsTheScadaBridgeCommunicationSection() + { + var config = new ConfigurationBuilder().AddInMemoryCollection(new Dictionary + { + ["ScadaBridge:Communication:DeploymentTimeout"] = "00:01:23", + }).Build(); + var services = new ServiceCollection(); + services.AddSingleton(config); + services.AddCommunication(); + using var sp = services.BuildServiceProvider(); + Assert.Equal(TimeSpan.FromSeconds(83), + sp.GetRequiredService>().Value.DeploymentTimeout); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/DataConnectionOptionsBindingTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/DataConnectionOptionsBindingTests.cs new file mode 100644 index 00000000..59ec2a02 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests/DataConnectionOptionsBindingTests.cs @@ -0,0 +1,30 @@ +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; + +namespace ZB.MOM.WW.ScadaBridge.DataConnectionLayer.Tests; + +/// +/// Locks the config section +/// binds (arch-review 08 round 2 NF8). Every real appsettings nests this section under +/// ScadaBridge:; binding a root-level DataConnectionLayer section reads config +/// that no deployment writes — the drift the Host duplicate was silently masking. +/// AddDataConnectionLayer alone must bind the canonical ScadaBridge:DataConnection section. +/// +public class DataConnectionOptionsBindingTests +{ + [Fact] + public void AddDataConnectionLayer_BindsTheScadaBridgeDataConnectionSection() + { + var config = new ConfigurationBuilder().AddInMemoryCollection(new Dictionary + { + ["ScadaBridge:DataConnection:ReconnectInterval"] = "00:00:42", + }).Build(); + var services = new ServiceCollection(); + services.AddSingleton(config); + services.AddDataConnectionLayer(); + using var sp = services.BuildServiceProvider(); + Assert.Equal(TimeSpan.FromSeconds(42), + sp.GetRequiredService>().Value.ReconnectInterval); + } +}