diff --git a/docs/requirements/Component-DeploymentManager.md b/docs/requirements/Component-DeploymentManager.md index 76abee0f..47d84f0b 100644 --- a/docs/requirements/Component-DeploymentManager.md +++ b/docs/requirements/Component-DeploymentManager.md @@ -142,6 +142,20 @@ The system maintains two views per instance: These are compared to determine staleness and generate diffs. +The comparison path is **hash-only by design**. When the Template-Derived +Configuration is re-flattened for a comparison (the Deployments comparison page) +or for the Transport stale-instance probe +(`IStaleInstanceProbe.GetCurrentRevisionHashAsync`, used per-instance across a +bundle import), the pipeline is invoked with `validateScripts: false`, which +skips **only** the expensive Roslyn script-compilation stage +(`ValidationService.ValidateScriptCompilation` — a non-collectible assembly load +per script). Structural and semantic validation still run. These read-only paths +need only the flattened config and its revision hash, not a compile, so a script +that does not currently compile still yields a comparison. The **deploy gate +remains the authoritative compile**: instance deployment re-flattens with +`validateScripts: true` (the default), and a script that fails to compile blocks +the deployment (see Site-Side Apply Atomicity). + ## Deployable Artifacts A deployment to a site includes the flattened instance configuration plus any system-wide artifacts that have changed: diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs index 71c0be5e..a7fbfa62 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/DeploymentService.cs @@ -648,8 +648,12 @@ public class DeploymentService if (snapshot == null) return Result.Failure("No deployed snapshot found for this instance."); - // Compute current template-derived config - var currentResult = await _flatteningPipeline.FlattenAndValidateAsync(instanceId, cancellationToken); + // Compute current template-derived config. This read-only comparison only needs + // the revision hash (staleness) + the flattened config (structured diff) — NOT a + // script compile — so skip the expensive Roslyn ValidateScriptCompilation stage. + // The deploy gate remains the authoritative compile. + var currentResult = await _flatteningPipeline.FlattenAndValidateAsync( + instanceId, cancellationToken, validateScripts: false); if (currentResult.IsFailure) return Result.Failure($"Cannot compute current config: {currentResult.Error}"); diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/FlatteningPipeline.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/FlatteningPipeline.cs index 3336325e..fe7560f9 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/FlatteningPipeline.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/FlatteningPipeline.cs @@ -54,7 +54,8 @@ public class FlatteningPipeline : IFlatteningPipeline /// public async Task> FlattenAndValidateAsync( int instanceId, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + bool validateScripts = true) { // Load instance with full graph var instance = await _templateRepo.GetInstanceByIdAsync(instanceId, cancellationToken); @@ -158,13 +159,19 @@ public class FlatteningPipeline : IFlatteningPipeline // on the site — blocks the deployment. (The template DESIGN-TIME validate path in // ManagementActor leaves this non-blocking by NOT enforcing, since bindings are // set later at instance/deploy time.) + // validateScripts == false is the read-only staleness/comparison path: skip + // only the expensive Roslyn ValidateScriptCompilation (which loads a + // non-collectible InteractiveAssemblyLoader assembly per script). Structural + // and semantic validation still run so a genuinely broken flatten is surfaced; + // the deploy path (validateScripts: true) remains the authoritative compile gate. var validation = _validationService.Validate( config, resolvedSharedScripts, alarmCapableConnectionNames, enforceConnectionBindings: true, siteConnectionNames: siteConnectionNames, - resolveSchemaRef: resolveSchemaRef); + resolveSchemaRef: resolveSchemaRef, + validateScriptCompilation: validateScripts); // Compute revision hash var hash = _revisionHashService.ComputeHash(config); diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/IFlatteningPipeline.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/IFlatteningPipeline.cs index 98857859..eaddc710 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/IFlatteningPipeline.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/IFlatteningPipeline.cs @@ -15,10 +15,19 @@ public interface IFlatteningPipeline /// /// Id of the instance to flatten and validate. /// Cancellation token. + /// + /// When true (the deploy default), the authoritative Roslyn script-compilation + /// gate runs. When false, that compile step is skipped — used by the read-only + /// staleness/comparison paths (Deployments comparison page, + /// ) + /// which need only the flattened config + revision hash, not a compile. Structural and + /// semantic validation still run in both cases. + /// /// A task that resolves to the flattened configuration, revision hash, and validation result; or a failure result if flattening could not complete. Task> FlattenAndValidateAsync( int instanceId, - CancellationToken cancellationToken = default); + CancellationToken cancellationToken = default, + bool validateScripts = true); } /// diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/StaleInstanceProbe.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/StaleInstanceProbe.cs index 87d501ee..48d429a0 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/StaleInstanceProbe.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/StaleInstanceProbe.cs @@ -30,8 +30,11 @@ public sealed class StaleInstanceProbe : IStaleInstanceProbe // instance as "staleness indeterminate" and skips it. Flattening reuses // the scoped ITemplateEngineRepository, so it observes template rows the // in-flight import has staged on the shared change tracker. + // Staleness only needs the revision hash — skip the expensive Roslyn + // script-compilation stage (this probe is called per-instance across a whole + // bundle import in BundleImporter.ComputeStaleInstanceIdsAsync). var result = await _flatteningPipeline - .FlattenAndValidateAsync(instanceId, cancellationToken) + .FlattenAndValidateAsync(instanceId, cancellationToken, validateScripts: false) .ConfigureAwait(false); return result.IsSuccess ? result.Value.RevisionHash : null; } diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs index 50ca5ea2..c29b3053 100644 --- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs @@ -95,6 +95,14 @@ public class ValidationService /// $ref are unaffected (behavior unchanged), and a $ref with no /// resolver is treated as dangling (the safe option). /// + /// + /// true (default) runs the authoritative Roslyn + /// stage — the single most expensive check (a non-collectible assembly load per script). + /// false skips ONLY that stage; every other structural/semantic check still runs. + /// Read-only staleness/comparison callers (DeploymentManager's FlatteningPipeline on its + /// comparison/probe paths) pass false because they need only the flattened config and + /// revision hash, not a compile. The deploy gate keeps the default (true). + /// /// A merged aggregating all pipeline stage outcomes. public ValidationResult Validate( FlattenedConfiguration configuration, @@ -102,7 +110,8 @@ public class ValidationService IReadOnlySet? alarmCapableConnectionNames = null, bool enforceConnectionBindings = false, IReadOnlySet? siteConnectionNames = null, - Func? resolveSchemaRef = null) + Func? resolveSchemaRef = null, + bool validateScriptCompilation = true) { ArgumentNullException.ThrowIfNull(configuration); @@ -110,7 +119,13 @@ public class ValidationService { ValidateFlatteningSuccess(configuration), ValidateNamingCollisions(configuration), - ValidateScriptCompilation(configuration), + // The Roslyn compile is the single most expensive validation stage (a + // non-collectible InteractiveAssemblyLoader assembly load per script). + // Read-only staleness/comparison callers pass validateScriptCompilation: + // false to skip ONLY this stage; every other structural/semantic check + // still runs. The deploy gate keeps the default (true) — it is the + // authoritative compile. + validateScriptCompilation ? ValidateScriptCompilation(configuration) : ValidationResult.Success(), ValidateAlarmTriggerReferences(configuration), ValidateScriptTriggerReferences(configuration), ValidateExpressionTriggers(configuration), diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/TopologyPageTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/TopologyPageTests.cs index 008735de..5d41048e 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/TopologyPageTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/TopologyPageTests.cs @@ -360,7 +360,7 @@ public class TopologyPageTests : BunitContext } } }; - _pipeline.FlattenAndValidateAsync(100, Arg.Any()) + _pipeline.FlattenAndValidateAsync(100, Arg.Any(), Arg.Any()) .Returns(Task.FromResult(Result.Success( new FlatteningPipelineResult(currentConfig, "hash-new", ValidationResult.Success())))); @@ -410,7 +410,7 @@ public class TopologyPageTests : BunitContext JsonSerializer.Serialize(deployedConfig)))); var currentConfig = new FlattenedConfiguration { InstanceUniqueName = "Pump-001" }; - _pipeline.FlattenAndValidateAsync(100, Arg.Any()) + _pipeline.FlattenAndValidateAsync(100, Arg.Any(), Arg.Any()) .Returns(Task.FromResult(Result.Success( new FlatteningPipelineResult(currentConfig, "hash-new", ValidationResult.Success())))); } diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentComparisonTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentComparisonTests.cs index 0e23b0b9..09ac70dd 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentComparisonTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentComparisonTests.cs @@ -1,7 +1,23 @@ +using NSubstitute; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Instances; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Sites; +using ZB.MOM.WW.ScadaBridge.Commons.Entities.Templates; +using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Flattening; +using ZB.MOM.WW.ScadaBridge.TemplateEngine.Flattening; +using ZB.MOM.WW.ScadaBridge.TemplateEngine.Validation; + namespace ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests; /// /// WP-8: Tests for deployed vs template-derived state comparison. +/// +/// PLAN-05 Task 16: adds behavioral coverage proving the read-only +/// staleness/comparison path can flatten (and thus hash) an instance whose +/// script does NOT compile — because validateScripts: false skips the +/// expensive Roslyn +/// step while keeping structural/semantic validation. The default (deploy) path +/// still compiles. /// public class DeploymentComparisonTests { @@ -34,4 +50,81 @@ public class DeploymentComparisonTests Assert.Equal(deployedAt, result.DeployedAt); } + + // ── PLAN-05 Task 16: read-only paths skip Roslyn script compilation ── + + private const int InstanceId = 1; + private const int TemplateId = 10; + private const int SiteId = 100; + + // A deliberately non-compiling script body. The deploy gate + // (validateScripts: true) flags this as a ScriptCompilation error; the + // read-only staleness/comparison path (validateScripts: false) must skip + // the compile entirely and still produce a flattened config + hash. + private const string NonCompilingScript = "int x = ;"; + + private static FlatteningPipeline BuildPipeline( + ITemplateEngineRepository templateRepo, + ISiteRepository siteRepo, + ISharedSchemaRepository sharedSchemaRepo) + { + sharedSchemaRepo.ListAsync(Arg.Any()).Returns([]); + return new FlatteningPipeline( + templateRepo, + siteRepo, + new FlatteningService(), + new ValidationService(), + new RevisionHashService(), + sharedSchemaRepo); + } + + private static (ITemplateEngineRepository, ISiteRepository, ISharedSchemaRepository) ArrangeNonCompilingScript() + { + var templateRepo = Substitute.For(); + var siteRepo = Substitute.For(); + var sharedSchemaRepo = Substitute.For(); + + var template = new Template("Tank") { Id = TemplateId }; + template.Scripts.Add(new TemplateScript("BadScript", NonCompilingScript)); + + var instance = new Instance("Tank-01") { Id = InstanceId, TemplateId = TemplateId, SiteId = SiteId }; + + templateRepo.GetInstanceByIdAsync(InstanceId, Arg.Any()).Returns(instance); + templateRepo.GetTemplateWithChildrenAsync(TemplateId, Arg.Any()).Returns(template); + templateRepo.GetCompositionsByTemplateIdAsync(TemplateId, Arg.Any()).Returns([]); + templateRepo.GetAllSharedScriptsAsync(Arg.Any()).Returns([]); + siteRepo.GetDataConnectionsBySiteIdAsync(SiteId, Arg.Any()).Returns([]); + + return (templateRepo, siteRepo, sharedSchemaRepo); + } + + [Fact] + public async Task FlattenAndValidate_ValidateScriptsFalse_NonCompilingScript_SucceedsWithHashAndNoCompilationError() + { + var (templateRepo, siteRepo, sharedSchemaRepo) = ArrangeNonCompilingScript(); + var sut = BuildPipeline(templateRepo, siteRepo, sharedSchemaRepo); + + var result = await sut.FlattenAndValidateAsync(InstanceId, validateScripts: false); + + Assert.True(result.IsSuccess); + Assert.False(string.IsNullOrEmpty(result.Value.RevisionHash)); + // The Roslyn compile step was skipped, so no ScriptCompilation error is raised + // even though the script body does not compile. + Assert.DoesNotContain(result.Value.Validation.Errors, + e => e.Category == ValidationCategory.ScriptCompilation); + } + + [Fact] + public async Task FlattenAndValidate_DefaultValidateScripts_NonCompilingScript_ReportsCompilationError() + { + var (templateRepo, siteRepo, sharedSchemaRepo) = ArrangeNonCompilingScript(); + var sut = BuildPipeline(templateRepo, siteRepo, sharedSchemaRepo); + + // Default (deploy) path — compilation is the authoritative gate. + var result = await sut.FlattenAndValidateAsync(InstanceId); + + Assert.True(result.IsSuccess); + Assert.Contains(result.Value.Validation.Errors, + e => e.Category == ValidationCategory.ScriptCompilation); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs index b79cc6c9..e8581351 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/DeploymentServiceTests.cs @@ -520,7 +520,7 @@ public class DeploymentServiceTests : TestKit _repo.GetDeployedSnapshotByInstanceIdAsync(1).Returns(snapshot); var currentConfig = deployedConfig with { }; - _pipeline.FlattenAndValidateAsync(1, Arg.Any()) + _pipeline.FlattenAndValidateAsync(1, Arg.Any(), Arg.Any()) .Returns(Result.Success( new FlatteningPipelineResult(currentConfig, sameHash, ValidationResult.Success()))); @@ -557,7 +557,7 @@ public class DeploymentServiceTests : TestKit Attributes = [new ResolvedAttribute { CanonicalName = "Setpoint", Value = "9", DataType = "Int32" }] }; var currentHash = new RevisionHashService().ComputeHash(currentConfig); - _pipeline.FlattenAndValidateAsync(1, Arg.Any()) + _pipeline.FlattenAndValidateAsync(1, Arg.Any(), Arg.Any()) .Returns(Result.Success( new FlatteningPipelineResult(currentConfig, currentHash, ValidationResult.Success()))); @@ -567,6 +567,72 @@ public class DeploymentServiceTests : TestKit Assert.True(result.Value.IsStale); } + // ── PLAN-05 Task 16: read-only paths skip Roslyn script compilation ── + + [Fact] + public async Task GetDeploymentComparisonAsync_RequestsValidateScriptsFalse() + { + // The comparison page only needs the revision hash for staleness — the + // expensive Roslyn compile must be skipped (validateScripts: false). + var deployedConfig = new FlattenedConfiguration + { + InstanceUniqueName = "TestInst", + Attributes = [new ResolvedAttribute { CanonicalName = "Setpoint", Value = "5", DataType = "Int32" }] + }; + var hash = new RevisionHashService().ComputeHash(deployedConfig); + var snapshot = new DeployedConfigSnapshot( + "dep1", hash, System.Text.Json.JsonSerializer.Serialize(deployedConfig)) + { + InstanceId = 1, + DeployedAt = DateTimeOffset.UtcNow + }; + _repo.GetDeployedSnapshotByInstanceIdAsync(1).Returns(snapshot); + _pipeline.FlattenAndValidateAsync(1, Arg.Any(), Arg.Any()) + .Returns(Result.Success( + new FlatteningPipelineResult(deployedConfig with { }, hash, ValidationResult.Success()))); + + var result = await _service.GetDeploymentComparisonAsync(1); + + Assert.True(result.IsSuccess); + await _pipeline.Received(1).FlattenAndValidateAsync(1, Arg.Any(), false); + await _pipeline.DidNotReceive().FlattenAndValidateAsync(1, Arg.Any(), true); + } + + [Fact] + public async Task DeployInstanceAsync_RequestsValidateScriptsTrue() + { + // The deploy gate stays the authoritative compile — validateScripts defaults to true. + var instance = new Instance("TestInst") { Id = 1, SiteId = 1, State = InstanceState.NotDeployed }; + _repo.GetInstanceByIdAsync(1).Returns(instance); + + var validationResult = new ValidationResult + { + Errors = [ValidationEntry.Error(ValidationCategory.ScriptCompilation, "Compile error")] + }; + _pipeline.FlattenAndValidateAsync(1, Arg.Any(), Arg.Any()) + .Returns(Result.Success( + new FlatteningPipelineResult(new FlattenedConfiguration(), "hash1", validationResult))); + + await _service.DeployInstanceAsync(1, "admin"); + + await _pipeline.Received().FlattenAndValidateAsync(1, Arg.Any(), true); + } + + [Fact] + public async Task StaleInstanceProbe_RequestsValidateScriptsFalse() + { + // The Transport stale-instance probe also only needs the hash — same skip. + _pipeline.FlattenAndValidateAsync(5, Arg.Any(), Arg.Any()) + .Returns(Result.Success( + new FlatteningPipelineResult(new FlattenedConfiguration(), "sha256:probe", ValidationResult.Success()))); + var probe = new StaleInstanceProbe(_pipeline); + + var hash = await probe.GetCurrentRevisionHashAsync(5); + + Assert.Equal("sha256:probe", hash); + await _pipeline.Received(1).FlattenAndValidateAsync(5, Arg.Any(), false); + } + // ── DeploymentManager-007: comparison must produce a structured diff ── [Fact] @@ -594,7 +660,7 @@ public class DeploymentServiceTests : TestKit InstanceUniqueName = "DiffInst", Attributes = [new ResolvedAttribute { CanonicalName = "NewAttr", DataType = "Int" }] }; - _pipeline.FlattenAndValidateAsync(40, Arg.Any()) + _pipeline.FlattenAndValidateAsync(40, Arg.Any(), Arg.Any()) .Returns(Result.Success( new FlatteningPipelineResult(currentConfig, "sha256:new", ValidationResult.Success()))); @@ -668,7 +734,7 @@ public class DeploymentServiceTests : TestKit ] }; var nativeHash = hasher.ComputeHash(nativeConfig); - _pipeline.FlattenAndValidateAsync(50, Arg.Any()) + _pipeline.FlattenAndValidateAsync(50, Arg.Any(), Arg.Any()) .Returns(Result.Success( new FlatteningPipelineResult(nativeConfig, nativeHash, ValidationResult.Success()))); @@ -736,7 +802,7 @@ public class DeploymentServiceTests : TestKit ] }; var currentHash = hasher.ComputeHash(currentConfig); - _pipeline.FlattenAndValidateAsync(51, Arg.Any()) + _pipeline.FlattenAndValidateAsync(51, Arg.Any(), Arg.Any()) .Returns(Result.Success( new FlatteningPipelineResult(currentConfig, currentHash, ValidationResult.Success())));