From e0dddac0be327b3b79e2b4e35bef51ca91ce22d9 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 13 Jul 2026 10:36:46 -0400 Subject: [PATCH] =?UTF-8?q?fix(options):=20eager=20startup=20validation=20?= =?UTF-8?q?for=20AuditLog=20site=20sub-options=20=E2=80=94=20SiteWriter,?= =?UTF-8?q?=20SiteTelemetry,=20SiteRetention=20(plan=20R2-08=20T5,=20arch-?= =?UTF-8?q?review=2008r2=20NF4)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../ServiceCollectionExtensions.cs | 29 ++++---- .../SiteAuditRetentionOptionsValidator.cs | 31 ++++++++ .../Site/SqliteAuditWriterOptionsValidator.cs | 38 ++++++++++ .../SiteAuditTelemetryOptionsValidator.cs | 36 ++++++++++ ...SiteAuditRetentionOptionsValidatorTests.cs | 53 ++++++++++++++ ...SiteAuditTelemetryOptionsValidatorTests.cs | 61 ++++++++++++++++ .../SqliteAuditWriterOptionsValidatorTests.cs | 72 +++++++++++++++++++ 7 files changed, 307 insertions(+), 13 deletions(-) create mode 100644 src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SiteAuditRetentionOptionsValidator.cs create mode 100644 src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriterOptionsValidator.cs create mode 100644 src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/Telemetry/SiteAuditTelemetryOptionsValidator.cs create mode 100644 tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditRetentionOptionsValidatorTests.cs create mode 100644 tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditTelemetryOptionsValidatorTests.cs create mode 100644 tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SqliteAuditWriterOptionsValidatorTests.cs diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/ServiceCollectionExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/ServiceCollectionExtensions.cs index ef000387..f1d68167 100644 --- a/src/ZB.MOM.WW.ScadaBridge.AuditLog/ServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/ServiceCollectionExtensions.cs @@ -92,20 +92,23 @@ public static class ServiceCollectionExtensions // failures as AuditRedactionFailure on the site health report. services.TryAddSingleton(); - // Site writer + telemetry options bindings. - // BindConfiguration is not used because the configuration root supplied - // by the caller may not be the application root — we go through the - // section explicitly so a partial IConfiguration (e.g. a test stub - // anchored on the AuditLog section's parent) still works. - services.AddOptions() - .Bind(config.GetSection(SiteWriterSectionName)); - services.AddOptions() - .Bind(config.GetSection(SiteTelemetrySectionName)); + // Site writer + telemetry options bindings — now bind-and-validate via + // the shared helper so a bad site config fails fast at boot (NF4). The + // helper's GetSection(sectionPath) still resolves the section explicitly + // (NOT BindConfiguration), preserving the partial-config-root rationale: + // the caller's IConfiguration may be a test stub anchored on the AuditLog + // section's parent rather than the application root. + services.AddValidatedOptions( + config, SiteWriterSectionName); + services.AddValidatedOptions( + config, SiteTelemetrySectionName); // Retention purge options (the SiteAuditRetentionService hosted job is - // registered site-only in AddAuditLogHealthMetricsBridge). Binding here is - // inert on central — no hosted service consumes it there. - services.AddOptions() - .Bind(config.GetSection(SiteAuditRetentionOptions.SectionName)); + // registered site-only in AddAuditLogHealthMetricsBridge). The binding + // here is inert on central — no hosted service consumes it there — but + // the defaults validate clean, so eager validation on the central root is + // harmless. + services.AddValidatedOptions( + config, SiteAuditRetentionOptions.SectionName); // SqliteAuditWriter is a singleton with a single owned SqliteConnection // and a background writer Task; multiple instances would race on the diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SiteAuditRetentionOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SiteAuditRetentionOptionsValidator.cs new file mode 100644 index 00000000..6c6ae34f --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SiteAuditRetentionOptionsValidator.cs @@ -0,0 +1,31 @@ +using ZB.MOM.WW.Configuration; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Site; + +/// +/// Validates at host startup +/// (arch-review 08 round 2 NF4). The option class self-clamps at resolve time, +/// but a plainly-wrong config (zero retention window, zero/negative purge +/// period, negative initial delay) should still fail the boot so the operator +/// learns immediately rather than trusting a silent clamp. +/// is test-only +/// (per the class remarks) and is deliberately not validated. +/// +public sealed class SiteAuditRetentionOptionsValidator : OptionsValidatorBase +{ + /// + protected override void Validate(ValidationBuilder builder, SiteAuditRetentionOptions options) + { + builder.RequireThat(options.RetentionDays > 0, + $"AuditLog:SiteRetention:{nameof(SiteAuditRetentionOptions.RetentionDays)} " + + $"({options.RetentionDays}) must be > 0."); + + builder.RequireThat(options.PurgeInterval > TimeSpan.Zero, + $"AuditLog:SiteRetention:{nameof(SiteAuditRetentionOptions.PurgeInterval)} " + + $"({options.PurgeInterval}) must be > 0."); + + builder.RequireThat(options.InitialDelay >= TimeSpan.Zero, + $"AuditLog:SiteRetention:{nameof(SiteAuditRetentionOptions.InitialDelay)} " + + $"({options.InitialDelay}) must be >= 0."); + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriterOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriterOptionsValidator.cs new file mode 100644 index 00000000..68c586ce --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/SqliteAuditWriterOptionsValidator.cs @@ -0,0 +1,38 @@ +using ZB.MOM.WW.Configuration; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Site; + +/// +/// Validates at host startup +/// (arch-review 08 round 2 NF4). The channel/batch/flush knobs feed the +/// background writer task; a zero anywhere stalls it (nothing drains, nothing +/// flushes), and an empty DatabasePath leaves the SQLite writer with no +/// backing store. +/// is intentionally NOT validated — a non-positive value has a documented +/// fall-back-to-30s contract in SiteAuditBacklogReporter. +/// +public sealed class SqliteAuditWriterOptionsValidator : OptionsValidatorBase +{ + /// + protected override void Validate(ValidationBuilder builder, SqliteAuditWriterOptions options) + { + builder.RequireThat(!string.IsNullOrWhiteSpace(options.DatabasePath), + $"AuditLog:SiteWriter:{nameof(SqliteAuditWriterOptions.DatabasePath)} must be a non-empty path."); + + builder.RequireThat(options.ChannelCapacity > 0, + $"AuditLog:SiteWriter:{nameof(SqliteAuditWriterOptions.ChannelCapacity)} " + + $"({options.ChannelCapacity}) must be > 0."); + + builder.RequireThat(options.BatchSize > 0, + $"AuditLog:SiteWriter:{nameof(SqliteAuditWriterOptions.BatchSize)} " + + $"({options.BatchSize}) must be > 0."); + + builder.RequireThat(options.FlushIntervalMs > 0, + $"AuditLog:SiteWriter:{nameof(SqliteAuditWriterOptions.FlushIntervalMs)} " + + $"({options.FlushIntervalMs}) must be > 0."); + + // BacklogPollIntervalSeconds is deliberately unchecked: a non-positive + // value falls back to the reporter's 30s default (see the option's + // remarks + register row 21 resolution). + } +} diff --git a/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/Telemetry/SiteAuditTelemetryOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/Telemetry/SiteAuditTelemetryOptionsValidator.cs new file mode 100644 index 00000000..30ebee9d --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.AuditLog/Site/Telemetry/SiteAuditTelemetryOptionsValidator.cs @@ -0,0 +1,36 @@ +using ZB.MOM.WW.Configuration; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Site.Telemetry; + +/// +/// Validates at host startup +/// (arch-review 08 round 2 NF4). SiteAuditTelemetryActor uses the two +/// interval knobs as raw scheduling delays (busy after a productive/faulted +/// drain, idle after an empty one), so a zero interval spins the drain loop and +/// a zero batch drains nothing. Idle must be >= busy: a shorter idle interval +/// inverts the actor's back-off-when-empty intent. +/// +public sealed class SiteAuditTelemetryOptionsValidator : OptionsValidatorBase +{ + /// + protected override void Validate(ValidationBuilder builder, SiteAuditTelemetryOptions options) + { + builder.RequireThat(options.BatchSize > 0, + $"AuditLog:SiteTelemetry:{nameof(SiteAuditTelemetryOptions.BatchSize)} " + + $"({options.BatchSize}) must be > 0."); + + builder.RequireThat(options.BusyIntervalSeconds > 0, + $"AuditLog:SiteTelemetry:{nameof(SiteAuditTelemetryOptions.BusyIntervalSeconds)} " + + $"({options.BusyIntervalSeconds}) must be > 0."); + + builder.RequireThat(options.IdleIntervalSeconds > 0, + $"AuditLog:SiteTelemetry:{nameof(SiteAuditTelemetryOptions.IdleIntervalSeconds)} " + + $"({options.IdleIntervalSeconds}) must be > 0."); + + builder.RequireThat(options.IdleIntervalSeconds >= options.BusyIntervalSeconds, + $"AuditLog:SiteTelemetry:{nameof(SiteAuditTelemetryOptions.IdleIntervalSeconds)} " + + $"({options.IdleIntervalSeconds}) must be >= " + + $"{nameof(SiteAuditTelemetryOptions.BusyIntervalSeconds)} ({options.BusyIntervalSeconds}) " + + "— the idle drain must back off at least as far as the busy drain."); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditRetentionOptionsValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditRetentionOptionsValidatorTests.cs new file mode 100644 index 00000000..42c8d777 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditRetentionOptionsValidatorTests.cs @@ -0,0 +1,53 @@ +using ZB.MOM.WW.ScadaBridge.AuditLog.Site; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Tests.Site; + +/// +/// Eager startup validation for +/// (arch-review 08 round 2 NF4). Although the option class self-clamps at +/// resolve time, the validator still fails a boot with a plainly-wrong config +/// (zero retention window, zero/negative purge period, negative initial delay) +/// so the operator learns at startup rather than trusting a silent clamp. +/// PurgeIntervalOverride is test-only and left unvalidated. +/// +public class SiteAuditRetentionOptionsValidatorTests +{ + [Fact] + public void DefaultOptions_AreValid() + { + var validator = new SiteAuditRetentionOptionsValidator(); + Assert.True(validator.Validate(null, new SiteAuditRetentionOptions()).Succeeded); + } + + [Fact] + public void ZeroRetentionDays_Fails() + { + var validator = new SiteAuditRetentionOptionsValidator(); + var result = validator.Validate(null, new SiteAuditRetentionOptions { RetentionDays = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SiteAuditRetentionOptions.RetentionDays), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroPurgeInterval_Fails() + { + var validator = new SiteAuditRetentionOptionsValidator(); + var result = validator.Validate(null, + new SiteAuditRetentionOptions { PurgeInterval = TimeSpan.Zero }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SiteAuditRetentionOptions.PurgeInterval), StringComparison.Ordinal)); + } + + [Fact] + public void NegativeInitialDelay_Fails() + { + var validator = new SiteAuditRetentionOptionsValidator(); + var result = validator.Validate(null, + new SiteAuditRetentionOptions { InitialDelay = TimeSpan.FromSeconds(-1) }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SiteAuditRetentionOptions.InitialDelay), StringComparison.Ordinal)); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditTelemetryOptionsValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditTelemetryOptionsValidatorTests.cs new file mode 100644 index 00000000..4d7c05a8 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SiteAuditTelemetryOptionsValidatorTests.cs @@ -0,0 +1,61 @@ +using ZB.MOM.WW.ScadaBridge.AuditLog.Site.Telemetry; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Tests.Site; + +/// +/// Eager startup validation for +/// (arch-review 08 round 2 NF4). The drain-loop cadences are used as raw +/// scheduling delays in SiteAuditTelemetryActor; a zero interval spins +/// the drain and a zero batch drains nothing. Idle must back off at least as +/// far as busy (idle < busy inverts the actor's intent). +/// +public class SiteAuditTelemetryOptionsValidatorTests +{ + [Fact] + public void DefaultOptions_AreValid() + { + var validator = new SiteAuditTelemetryOptionsValidator(); + Assert.True(validator.Validate(null, new SiteAuditTelemetryOptions()).Succeeded); + } + + [Fact] + public void ZeroBatchSize_Fails() + { + var validator = new SiteAuditTelemetryOptionsValidator(); + var result = validator.Validate(null, new SiteAuditTelemetryOptions { BatchSize = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SiteAuditTelemetryOptions.BatchSize), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroBusyInterval_Fails() + { + var validator = new SiteAuditTelemetryOptionsValidator(); + var result = validator.Validate(null, new SiteAuditTelemetryOptions { BusyIntervalSeconds = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SiteAuditTelemetryOptions.BusyIntervalSeconds), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroIdleInterval_Fails() + { + var validator = new SiteAuditTelemetryOptionsValidator(); + var result = validator.Validate(null, new SiteAuditTelemetryOptions { IdleIntervalSeconds = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SiteAuditTelemetryOptions.IdleIntervalSeconds), StringComparison.Ordinal)); + } + + [Fact] + public void IdleShorterThanBusy_Fails() + { + var validator = new SiteAuditTelemetryOptionsValidator(); + var result = validator.Validate(null, + new SiteAuditTelemetryOptions { BusyIntervalSeconds = 30, IdleIntervalSeconds = 5 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SiteAuditTelemetryOptions.IdleIntervalSeconds), StringComparison.Ordinal)); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SqliteAuditWriterOptionsValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SqliteAuditWriterOptionsValidatorTests.cs new file mode 100644 index 00000000..05ba0570 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.AuditLog.Tests/Site/SqliteAuditWriterOptionsValidatorTests.cs @@ -0,0 +1,72 @@ +using ZB.MOM.WW.ScadaBridge.AuditLog.Site; + +namespace ZB.MOM.WW.ScadaBridge.AuditLog.Tests.Site; + +/// +/// Eager startup validation for +/// (arch-review 08 round 2 NF4). The site hot-path writer's channel/batch/flush +/// knobs must be positive or the background writer task cannot make progress; +/// an empty DatabasePath would leave the SQLite writer with nowhere to go. +/// BacklogPollIntervalSeconds is deliberately NOT validated — a +/// non-positive value has a documented fall-back-to-30s contract. +/// +public class SqliteAuditWriterOptionsValidatorTests +{ + [Fact] + public void DefaultOptions_AreValid() + { + var validator = new SqliteAuditWriterOptionsValidator(); + Assert.True(validator.Validate(null, new SqliteAuditWriterOptions()).Succeeded); + } + + [Fact] + public void EmptyDatabasePath_Fails() + { + var validator = new SqliteAuditWriterOptionsValidator(); + var result = validator.Validate(null, new SqliteAuditWriterOptions { DatabasePath = "" }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SqliteAuditWriterOptions.DatabasePath), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroChannelCapacity_Fails() + { + var validator = new SqliteAuditWriterOptionsValidator(); + var result = validator.Validate(null, new SqliteAuditWriterOptions { ChannelCapacity = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SqliteAuditWriterOptions.ChannelCapacity), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroBatchSize_Fails() + { + var validator = new SqliteAuditWriterOptionsValidator(); + var result = validator.Validate(null, new SqliteAuditWriterOptions { BatchSize = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SqliteAuditWriterOptions.BatchSize), StringComparison.Ordinal)); + } + + [Fact] + public void ZeroFlushInterval_Fails() + { + var validator = new SqliteAuditWriterOptionsValidator(); + var result = validator.Validate(null, new SqliteAuditWriterOptions { FlushIntervalMs = 0 }); + Assert.False(result.Succeeded); + Assert.Contains(result.Failures!, + f => f.Contains(nameof(SqliteAuditWriterOptions.FlushIntervalMs), StringComparison.Ordinal)); + } + + [Theory] + [InlineData(0)] + [InlineData(-1)] + public void NonPositiveBacklogPollInterval_StillValid(int value) + { + // Documented fall-back-to-30s contract — must NOT be rejected. + var validator = new SqliteAuditWriterOptionsValidator(); + Assert.True(validator.Validate(null, + new SqliteAuditWriterOptions { BacklogPollIntervalSeconds = value }).Succeeded); + } +}