From 7c14a69091cefe2396fc00697c32bce46feaad68 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 16 Jun 2026 05:27:58 -0400 Subject: [PATCH] feat(#23): elevate connection-binding completeness to a deploy-gating Error (M2.8) Pre-deployment validation only WARNED when a data-sourced attribute had no connection binding, so an instance with unresolved bindings still passed IsValid and could deploy. There was also no check that a binding resolves to a connection that actually exists at the target site. - ValidationService.Validate gains an opt-in `enforceConnectionBindings` flag (default false) plus a `siteConnectionNames` set. Default-false keeps the template DESIGN-TIME path (ManagementActor.HandleValidateTemplate) non-blocking, since bindings are legitimately set later at instance/deploy time. The DEPLOY path (FlatteningPipeline) opts in (true) so: * a data-sourced attribute with no binding is now a deploy-gating Error; * a binding to a connection that does not exist on the target site is an Error. Static (non-data-sourced) attributes are never flagged. - FlatteningPipeline computes the site-connection-names set from the loaded site data connections (mirroring M2.1's alarmCapableConnectionNames) and threads it in. - Tests: TemplateEngine.Tests covers design-time warning / deploy-time error / static-ok / exists-at-site / non-existent-connection. New FlatteningPipelineConnectionBindingTests proves the deploy path enforces it. Mark M2.7 + M2.8 completed in the plan task tracker. --- ...illpending-m2-implementation.md.tasks.json | 4 +- .../FlatteningPipeline.cs | 21 +++- .../Validation/ValidationService.cs | 102 +++++++++++++-- ...latteningPipelineConnectionBindingTests.cs | 99 +++++++++++++++ .../Validation/ValidationServiceTests.cs | 118 +++++++++++++++++- 5 files changed, 330 insertions(+), 14 deletions(-) create mode 100644 tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/FlatteningPipelineConnectionBindingTests.cs diff --git a/docs/plans/2026-06-15-stillpending-m2-implementation.md.tasks.json b/docs/plans/2026-06-15-stillpending-m2-implementation.md.tasks.json index b7941708..414f6fd5 100644 --- a/docs/plans/2026-06-15-stillpending-m2-implementation.md.tasks.json +++ b/docs/plans/2026-06-15-stillpending-m2-implementation.md.tasks.json @@ -8,8 +8,8 @@ {"id": 36, "ref": "M2.4", "subject": "M2.4 #8: alarm conditionFilter applied (OPC UA WhereClause + client routing)", "class": "high-risk", "status": "completed", "commits": ["8825df5", "00304a2"]}, {"id": 37, "ref": "M2.5", "subject": "M2.5 #9: per-script execution timeout (entity+migration+flatten+actor)", "class": "standard", "status": "completed", "blockedBy": [32], "commits": ["3edef09", "3032faa"]}, {"id": 38, "ref": "M2.6", "subject": "M2.6 #13: nested Object/List extended-type validation", "class": "standard", "status": "completed", "commits": ["4b6187c", "411d0c0"]}, - {"id": 39, "ref": "M2.7", "subject": "M2.7 #20+#21: return-type + argument-type compatibility checks", "class": "standard", "status": "pending"}, - {"id": 40, "ref": "M2.8", "subject": "M2.8 #23: binding-completeness Error + name-exists-at-site", "class": "standard", "status": "pending"}, + {"id": 39, "ref": "M2.7", "subject": "M2.7 #20+#21: return-type + argument-type compatibility checks", "class": "standard", "status": "completed", "commits": ["958229e", "a8e9e99"]}, + {"id": 40, "ref": "M2.8", "subject": "M2.8 #23: binding-completeness Error + name-exists-at-site", "class": "standard", "status": "completed", "commits": ["3522335"]}, {"id": 41, "ref": "M2.9", "subject": "M2.9 #17: MachineDataDb fail-fast (reverts Host-008)", "class": "small", "status": "pending"}, {"id": 42, "ref": "M2.10", "subject": "M2.10 #18: CI grep-guard against UPDATE/DELETE on AuditLog", "class": "small", "status": "pending"}, {"id": 43, "ref": "M2.11", "subject": "M2.11 #24: debug snapshot unknown-instance returns error", "class": "small", "status": "pending"}, diff --git a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/FlatteningPipeline.cs b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/FlatteningPipeline.cs index df78fe6c..a036b348 100644 --- a/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/FlatteningPipeline.cs +++ b/src/ZB.MOM.WW.ScadaBridge.DeploymentManager/FlatteningPipeline.cs @@ -127,9 +127,26 @@ public class FlatteningPipeline : IFlatteningPipeline .Select(c => c.Name) .ToHashSet(StringComparer.Ordinal); - // Validate + // M2.8 (#23): the set of data-connection names that actually exist on the + // target site, used to verify each bound connection resolves to a real site + // connection. Same StringComparer.Ordinal as the rest of the binding-resolution + // path (connection names are matched as-authored throughout the pipeline). + var siteConnectionNames = dataConnections.Values + .Select(c => c.Name) + .ToHashSet(StringComparer.Ordinal); + + // Validate. This is the deploy-gating path, so connection-binding completeness + // is enforced as an Error (enforceConnectionBindings: true): a data-sourced + // attribute with no binding — or one bound to a connection that no longer exists + // 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.) var validation = _validationService.Validate( - config, resolvedSharedScripts, alarmCapableConnectionNames); + config, + resolvedSharedScripts, + alarmCapableConnectionNames, + enforceConnectionBindings: true, + siteConnectionNames: siteConnectionNames); // Compute revision hash var hash = _revisionHashService.ComputeHash(config); diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs index 7093f506..548e5d60 100644 --- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/Validation/ValidationService.cs @@ -14,7 +14,10 @@ namespace ZB.MOM.WW.ScadaBridge.TemplateEngine.Validation; /// 4. Alarm trigger references exist (referenced attributes must be in the flattened config) /// 5. Script trigger references exist (referenced attributes must be in the flattened config) /// 6. Expression triggers — blank check, syntax check, and attribute-reference scan -/// 7. Connection binding completeness (all data-sourced attributes must have a binding) +/// 7. Connection binding completeness — every data-sourced attribute must have a binding, +/// and (on the deploy path) the bound connection must exist on the target site. +/// Severity is context-dependent: a non-blocking Warning at template design time +/// (bindings are set later) and a deploy-gating Error when enforced (M2.8 / #23). /// 8. Does NOT verify tag path resolution on devices /// public class ValidationService @@ -52,11 +55,37 @@ public class ValidationService /// the semantic validator gates every native-alarm-source binding against it. /// null skips the capability check (its absence makes the check inert). /// + /// + /// M2.8 (#23): controls the severity of the connection-binding-completeness check. + /// + /// false (default) — template DESIGN-TIME: a data-sourced attribute that is + /// not yet bound produces only a non-blocking Warning. Bindings are set later, + /// at instance/deploy time, so an unbound data-sourced template attribute is legitimate + /// here (see 's ValidateTemplate path, which builds a + /// config straight from raw template members with no bindings). + /// + /// + /// true — DEPLOY path ('s FlatteningPipeline): + /// an unbound data-sourced attribute becomes a deploy-gating Error (IsValid false), + /// and — when is supplied — a binding pointing at a + /// connection that does not exist on the target site is also an Error. + /// + /// + /// + /// M2.8 (#23): optional set of the data-connection names that actually exist on the + /// target site (computed by the deploy pipeline from the site's loaded connections, + /// mirroring ). When supplied (and + /// is true), every bound + /// connection is checked against this set so a binding to a phantom/stale connection + /// is caught. null skips the "exists at site" half (it stays inert). + /// /// A merged aggregating all pipeline stage outcomes. public ValidationResult Validate( FlattenedConfiguration configuration, IReadOnlyList? sharedScripts = null, - IReadOnlySet? alarmCapableConnectionNames = null) + IReadOnlySet? alarmCapableConnectionNames = null, + bool enforceConnectionBindings = false, + IReadOnlySet? siteConnectionNames = null) { ArgumentNullException.ThrowIfNull(configuration); @@ -68,7 +97,7 @@ public class ValidationService ValidateAlarmTriggerReferences(configuration), ValidateScriptTriggerReferences(configuration), ValidateExpressionTriggers(configuration), - ValidateConnectionBindingCompleteness(configuration), + ValidateConnectionBindingCompleteness(configuration, enforceConnectionBindings, siteConnectionNames), _semanticValidator.Validate(configuration, sharedScripts, alarmCapableConnectionNames) }; @@ -507,21 +536,76 @@ public class ValidationService } /// - /// Validates that all data-sourced attributes have connection bindings. + /// Validates connection bindings on data-sourced attributes. Only DATA-SOURCED + /// attributes ( != null) + /// require a binding; static attributes are never flagged. + /// + /// M2.8 (#23): the severity is context-dependent (see ). + /// At template design time (enforce == false) an unbound data-sourced + /// attribute is legitimate (bindings are set later) so it is only a non-blocking + /// Warning. On the deploy path (enforce == true) an unbound + /// data-sourced attribute is a deploy-gating Error, and — when + /// is supplied — a binding to a connection + /// that does not exist on the target site is also an Error. /// /// The flattened configuration to validate. - /// A with warnings for each data-sourced attribute that lacks a connection binding. - public static ValidationResult ValidateConnectionBindingCompleteness(FlattenedConfiguration configuration) + /// + /// true on the deploy path (unbound → Error + "exists at site" check); + /// false at design time (unbound → Warning only). Defaults to false + /// so design-time validation stays non-blocking. + /// + /// + /// Optional set of data-connection names that actually exist on the target site. + /// When non-null and is true, every bound + /// connection name is checked against this set. null skips the "exists at + /// site" check. + /// + /// A with the binding findings at the appropriate severity. + public static ValidationResult ValidateConnectionBindingCompleteness( + FlattenedConfiguration configuration, + bool enforce = false, + IReadOnlySet? siteConnectionNames = null) { var errors = new List(); var warnings = new List(); foreach (var attr in configuration.Attributes) { - if (attr.DataSourceReference != null && attr.BoundDataConnectionId == null) + // Only data-sourced attributes participate in binding validation. + if (attr.DataSourceReference == null) + continue; + + if (attr.BoundDataConnectionId == null) { - warnings.Add(ValidationEntry.Warning(ValidationCategory.ConnectionBinding, - $"Attribute '{attr.CanonicalName}' has a data source reference but no connection binding.", + // Unbound data-sourced attribute. At deploy time this gates the + // deployment; at design time the binding is set later, so it is + // only advisory. + if (enforce) + { + errors.Add(ValidationEntry.Error(ValidationCategory.ConnectionBinding, + $"Attribute '{attr.CanonicalName}' has a data source reference but no connection binding.", + attr.CanonicalName)); + } + else + { + warnings.Add(ValidationEntry.Warning(ValidationCategory.ConnectionBinding, + $"Attribute '{attr.CanonicalName}' has a data source reference but no connection binding.", + attr.CanonicalName)); + } + continue; + } + + // The attribute IS bound. On the deploy path, verify the bound connection + // actually exists on the target site (resolve against the site's connection + // set, not just name presence in the config). A binding pointing at a + // non-existent/stale site connection is a deploy-gating Error. + if (enforce && siteConnectionNames != null && + attr.BoundDataConnectionName != null && + !siteConnectionNames.Contains(attr.BoundDataConnectionName)) + { + errors.Add(ValidationEntry.Error(ValidationCategory.ConnectionBinding, + $"Attribute '{attr.CanonicalName}' is bound to data connection '{attr.BoundDataConnectionName}' " + + "which does not exist on the target site.", attr.CanonicalName)); } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/FlatteningPipelineConnectionBindingTests.cs b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/FlatteningPipelineConnectionBindingTests.cs new file mode 100644 index 00000000..a6c60fa9 --- /dev/null +++ b/tests/ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests/FlatteningPipelineConnectionBindingTests.cs @@ -0,0 +1,99 @@ +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.Enums; +using ZB.MOM.WW.ScadaBridge.Commons.Types.Flattening; +using ZB.MOM.WW.ScadaBridge.DeploymentManager; +using ZB.MOM.WW.ScadaBridge.TemplateEngine.Flattening; +using ZB.MOM.WW.ScadaBridge.TemplateEngine.Validation; + +namespace ZB.MOM.WW.ScadaBridge.DeploymentManager.Tests; + +/// +/// M2.8 (#23): proves the deploy path (FlatteningPipeline.FlattenAndValidateAsync) +/// opts into connection-binding enforcement, so a data-sourced attribute with no +/// binding gates the deployment as an ERROR (not just a warning), and that a binding +/// resolving to a connection that actually exists at the target site passes. +/// +public class FlatteningPipelineConnectionBindingTests +{ + private const int InstanceId = 1; + private const int TemplateId = 10; + private const int SiteId = 100; + private const int ConnectionId = 7; + + private readonly ITemplateEngineRepository _templateRepo = Substitute.For(); + private readonly ISiteRepository _siteRepo = Substitute.For(); + private readonly FlatteningPipeline _sut; + + public FlatteningPipelineConnectionBindingTests() + { + _sut = new FlatteningPipeline( + _templateRepo, + _siteRepo, + new FlatteningService(), + new ValidationService(), + new RevisionHashService()); + } + + /// + /// Seeds a single-template chain with one data-sourced attribute ("Temp") and a + /// site that owns a single "PlantBus" data connection. The instance optionally + /// binds "Temp" to . + /// + private void Arrange(int? boundConnectionId) + { + var template = new Template("Tank") { Id = TemplateId }; + template.Attributes.Add(new TemplateAttribute("Temp") + { + DataType = DataType.Double, + DataSourceReference = "ns=2;s=Temp" + }); + + var instance = new Instance("Tank-01") { Id = InstanceId, TemplateId = TemplateId, SiteId = SiteId }; + if (boundConnectionId.HasValue) + { + instance.ConnectionBindings.Add(new InstanceConnectionBinding("Temp") + { + InstanceId = InstanceId, + DataConnectionId = boundConnectionId.Value + }); + } + + _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([]); + + var connection = new DataConnection("PlantBus", "OpcUa", SiteId) { Id = ConnectionId }; + _siteRepo.GetDataConnectionsBySiteIdAsync(SiteId, Arg.Any()) + .Returns([connection]); + } + + [Fact] + public async Task FlattenAndValidate_DataSourcedAttributeWithNoBinding_ReportsBindingError() + { + Arrange(boundConnectionId: null); + + var result = await _sut.FlattenAndValidateAsync(InstanceId); + + Assert.True(result.IsSuccess); + Assert.False(result.Value.Validation.IsValid); + Assert.Contains(result.Value.Validation.Errors, + e => e.Category == ValidationCategory.ConnectionBinding); + } + + [Fact] + public async Task FlattenAndValidate_BindingToExistingSiteConnection_NoBindingError() + { + Arrange(boundConnectionId: ConnectionId); + + var result = await _sut.FlattenAndValidateAsync(InstanceId); + + Assert.True(result.IsSuccess); + Assert.DoesNotContain(result.Value.Validation.Errors, + e => e.Category == ValidationCategory.ConnectionBinding); + } +} diff --git a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ValidationServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ValidationServiceTests.cs index c71b278c..d5f1b5e8 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ValidationServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/Validation/ValidationServiceTests.cs @@ -161,8 +161,11 @@ public class ValidationServiceTests } [Fact] - public void Validate_UnboundDataSourceAttribute_ReturnsWarning() + public void Validate_UnboundDataSourceAttribute_DesignTime_ReturnsWarningNotError() { + // M2.8 (#23): at template design time (the default, enforceConnectionBindings:false) + // a data-sourced attribute is legitimately unbound — bindings are set later at + // instance/deploy time. So this must stay a non-blocking WARNING and IsValid true. var config = new FlattenedConfiguration { InstanceUniqueName = "Instance1", @@ -180,6 +183,119 @@ public class ValidationServiceTests var result = _sut.Validate(config); Assert.Contains(result.Warnings, w => w.Category == ValidationCategory.ConnectionBinding); + Assert.DoesNotContain(result.Errors, e => e.Category == ValidationCategory.ConnectionBinding); + Assert.True(result.IsValid); + } + + [Fact] + public void Validate_UnboundDataSourceAttribute_DeployTime_ReturnsErrorAndBlocks() + { + // M2.8 (#23): the deploy path opts in (enforceConnectionBindings:true). A data-sourced + // attribute with no binding now gates the deployment as an ERROR (IsValid false). + var config = new FlattenedConfiguration + { + InstanceUniqueName = "Instance1", + Attributes = + [ + new ResolvedAttribute + { + CanonicalName = "Temp", + DataType = "Double", + DataSourceReference = "ns=2;s=Temp", + BoundDataConnectionId = null // No binding! + } + ] + }; + + var result = _sut.Validate(config, enforceConnectionBindings: true); + Assert.Contains(result.Errors, e => e.Category == ValidationCategory.ConnectionBinding); + Assert.False(result.IsValid); + } + + [Fact] + public void Validate_StaticAttributeWithoutBinding_DeployTime_NoBindingError() + { + // M2.8 (#23): only DATA-SOURCED attributes require a binding. A static attribute + // (DataSourceReference == null) must remain OK even under deploy-time enforcement. + var config = new FlattenedConfiguration + { + InstanceUniqueName = "Instance1", + Attributes = + [ + new ResolvedAttribute + { + CanonicalName = "Setpoint", + DataType = "Double", + Value = "42", + DataSourceReference = null, + BoundDataConnectionId = null + } + ] + }; + + var result = _sut.Validate(config, enforceConnectionBindings: true); + Assert.DoesNotContain(result.Errors, e => e.Category == ValidationCategory.ConnectionBinding); + Assert.True(result.IsValid); + } + + [Fact] + public void Validate_BoundToExistingSiteConnection_DeployTime_NoBindingError() + { + // M2.8 (#23): a data-sourced attribute bound to a connection that exists at the + // target site passes the binding gate. + var config = new FlattenedConfiguration + { + InstanceUniqueName = "Instance1", + Attributes = + [ + new ResolvedAttribute + { + CanonicalName = "Temp", + DataType = "Double", + DataSourceReference = "ns=2;s=Temp", + BoundDataConnectionId = 7, + BoundDataConnectionName = "PlantBus" + } + ] + }; + + var result = _sut.Validate( + config, + enforceConnectionBindings: true, + siteConnectionNames: new HashSet(StringComparer.Ordinal) { "PlantBus" }); + + Assert.DoesNotContain(result.Errors, e => e.Category == ValidationCategory.ConnectionBinding); + Assert.True(result.IsValid); + } + + [Fact] + public void Validate_BoundToNonExistentSiteConnection_DeployTime_ReturnsError() + { + // M2.8 (#23): a binding pointing at a connection that does NOT exist on the + // target site is an ERROR that blocks deployment. + var config = new FlattenedConfiguration + { + InstanceUniqueName = "Instance1", + Attributes = + [ + new ResolvedAttribute + { + CanonicalName = "Temp", + DataType = "Double", + DataSourceReference = "ns=2;s=Temp", + BoundDataConnectionId = 99, + BoundDataConnectionName = "GhostBus" + } + ] + }; + + var result = _sut.Validate( + config, + enforceConnectionBindings: true, + siteConnectionNames: new HashSet(StringComparer.Ordinal) { "PlantBus" }); + + Assert.Contains(result.Errors, e => e.Category == ValidationCategory.ConnectionBinding); + Assert.False(result.IsValid); } [Fact]