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.
2026-06-16 05:27:58 -04:00
parent a8e9e9952d
commit 7c14a69091
5 changed files with 330 additions and 14 deletions
@@ -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"},
@@ -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);
@@ -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
 /// </summary>
 public class ValidationService
@@ -52,11 +55,37 @@ public class ValidationService
    /// the semantic validator gates every native-alarm-source binding against it.
    /// <c>null</c> skips the capability check (its absence makes the check inert).
    /// </param>
+    /// <param name="enforceConnectionBindings">
+    /// M2.8 (#23): controls the severity of the connection-binding-completeness check.
+    /// <para>
+    /// <c>false</c> (default) — template DESIGN-TIME: a data-sourced attribute that is
+    /// not yet bound produces only a non-blocking <c>Warning</c>. Bindings are set later,
+    /// at instance/deploy time, so an unbound data-sourced template attribute is legitimate
+    /// here (see <see cref="ManagementService"/>'s ValidateTemplate path, which builds a
+    /// config straight from raw template members with no bindings).
+    /// </para>
+    /// <para>
+    /// <c>true</c> — DEPLOY path (<see cref="DeploymentManager"/>'s FlatteningPipeline):
+    /// an unbound data-sourced attribute becomes a deploy-gating <c>Error</c> (IsValid false),
+    /// and — when <paramref name="siteConnectionNames"/> is supplied — a binding pointing at a
+    /// connection that does not exist on the target site is also an <c>Error</c>.
+    /// </para>
+    /// </param>
+    /// <param name="siteConnectionNames">
+    /// 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 <paramref name="alarmCapableConnectionNames"/>). When supplied (and
+    /// <paramref name="enforceConnectionBindings"/> is <c>true</c>), every bound
+    /// connection is checked against this set so a binding to a phantom/stale connection
+    /// is caught. <c>null</c> skips the "exists at site" half (it stays inert).
+    /// </param>
    /// <returns>A merged <see cref="ValidationResult"/> aggregating all pipeline stage outcomes.</returns>
    public ValidationResult Validate(
        FlattenedConfiguration configuration,
        IReadOnlyList<ResolvedScript>? sharedScripts = null,
-        IReadOnlySet<string>? alarmCapableConnectionNames = null)
+        IReadOnlySet<string>? alarmCapableConnectionNames = null,
+        bool enforceConnectionBindings = false,
+        IReadOnlySet<string>? 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
    }

    /// <summary>
-    /// Validates that all data-sourced attributes have connection bindings.
+    /// Validates connection bindings on data-sourced attributes. Only DATA-SOURCED
+    /// attributes (<see cref="ResolvedAttribute.DataSourceReference"/> != <c>null</c>)
+    /// require a binding; static attributes are never flagged.
+    ///
+    /// M2.8 (#23): the severity is context-dependent (see <paramref name="enforce"/>).
+    /// At template design time (<c>enforce == false</c>) an unbound data-sourced
+    /// attribute is legitimate (bindings are set later) so it is only a non-blocking
+    /// <c>Warning</c>. On the deploy path (<c>enforce == true</c>) an unbound
+    /// data-sourced attribute is a deploy-gating <c>Error</c>, and — when
+    /// <paramref name="siteConnectionNames"/> is supplied — a binding to a connection
+    /// that does not exist on the target site is also an <c>Error</c>.
    /// </summary>
    /// <param name="configuration">The flattened configuration to validate.</param>
-    /// <returns>A <see cref="ValidationResult"/> with warnings for each data-sourced attribute that lacks a connection binding.</returns>
-    public static ValidationResult ValidateConnectionBindingCompleteness(FlattenedConfiguration configuration)
+    /// <param name="enforce">
+    /// <c>true</c> on the deploy path (unbound → Error + "exists at site" check);
+    /// <c>false</c> at design time (unbound → Warning only). Defaults to <c>false</c>
+    /// so design-time validation stays non-blocking.
+    /// </param>
+    /// <param name="siteConnectionNames">
+    /// Optional set of data-connection names that actually exist on the target site.
+    /// When non-<c>null</c> and <paramref name="enforce"/> is <c>true</c>, every bound
+    /// connection name is checked against this set. <c>null</c> skips the "exists at
+    /// site" check.
+    /// </param>
+    /// <returns>A <see cref="ValidationResult"/> with the binding findings at the appropriate severity.</returns>
+    public static ValidationResult ValidateConnectionBindingCompleteness(
+        FlattenedConfiguration configuration,
+        bool enforce = false,
+        IReadOnlySet<string>? siteConnectionNames = null)
    {
        var errors = new List<ValidationEntry>();
        var warnings = new List<ValidationEntry>();

        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));
            }
        }
@@ -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;
+
+/// <summary>
+/// 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.
+/// </summary>
+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<ITemplateEngineRepository>();
+    private readonly ISiteRepository _siteRepo = Substitute.For<ISiteRepository>();
+    private readonly FlatteningPipeline _sut;
+
+    public FlatteningPipelineConnectionBindingTests()
+    {
+        _sut = new FlatteningPipeline(
+            _templateRepo,
+            _siteRepo,
+            new FlatteningService(),
+            new ValidationService(),
+            new RevisionHashService());
+    }
+
+    /// <summary>
+    /// 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 <paramref name="boundConnectionId"/>.
+    /// </summary>
+    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<CancellationToken>()).Returns(instance);
+        _templateRepo.GetTemplateWithChildrenAsync(TemplateId, Arg.Any<CancellationToken>()).Returns(template);
+        _templateRepo.GetCompositionsByTemplateIdAsync(TemplateId, Arg.Any<CancellationToken>()).Returns([]);
+        _templateRepo.GetAllSharedScriptsAsync(Arg.Any<CancellationToken>()).Returns([]);
+
+        var connection = new DataConnection("PlantBus", "OpcUa", SiteId) { Id = ConnectionId };
+        _siteRepo.GetDataConnectionsBySiteIdAsync(SiteId, Arg.Any<CancellationToken>())
+            .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);
+    }
+}
@@ -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<string>(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<string>(StringComparer.Ordinal) { "PlantBus" });
+
+        Assert.Contains(result.Errors, e => e.Category == ValidationCategory.ConnectionBinding);
+        Assert.False(result.IsValid);
    }

    [Fact]