fix(transport): import rejects malformed site gRPC endpoints and non-actor-safe names as blockers, not mid-apply crashes (review 2026-10-21 03/R-1, lane-F ActorNameRules follow-up)

2026-10-22 09:29:54 -04:00
parent 04ac4c0491
commit 39025b28fa
7 changed files with 506 additions and 13 deletions
@@ -22,6 +22,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Messages.Scripting;
 using ZB.MOM.WW.ScadaBridge.Commons.Types.Transport;
 using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums;
 using ZB.MOM.WW.ScadaBridge.Commons.Types.Flattening;
+using ZB.MOM.WW.ScadaBridge.Commons.Validation;
 using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase;
 using ZB.MOM.WW.ScadaBridge.ScriptAnalysis;
 using ZB.MOM.WW.ScadaBridge.TemplateEngine;
@@ -583,6 +584,26 @@ public sealed class BundleImporter : IBundleImporter
        {
            var existing = await ResolveTargetSiteAsync(siteDto.SiteIdentifier).ConfigureAwait(false);
            items.Add(_diff.CompareSite(siteDto, existing));
+
+            // Review 2026-10-21 03/R-1: a malformed gRPC endpoint makes central's
+            // channel build throw for that site. A site the import must CREATE would
+            // certainly write it → Blocker; an existing site only receives the
+            // addresses on Overwrite → Warning (apply re-checks against the chosen
+            // resolution and rejects an Overwrite).
+            var endpointErrors = DescribeSiteEndpointErrors(siteDto);
+            if (endpointErrors.Count > 0)
+            {
+                items.Add(new ImportPreviewItem(
+                    EntityType: "Site",
+                    Name: siteDto.SiteIdentifier,
+                    ExistingVersion: null,
+                    IncomingVersion: null,
+                    Kind: existing is null ? ConflictKind.Blocker : ConflictKind.Warning,
+                    FieldDiffJson: null,
+                    BlockerReason: string.Join(" ", endpointErrors) + (existing is null
+                        ? string.Empty
+                        : " The import will reject an Overwrite of this site; Skip leaves the target's endpoints unchanged.")));
+            }
        }

        // ---- DataConnections (site-scoped; matched by name within the auto-matched target site) ----
@@ -985,9 +1006,152 @@ public sealed class BundleImporter : IBundleImporter
                BlockerReason: message));
        }

+        // Lane-F follow-up (review 2026-10-21): names that become site actor names.
+        foreach (var (entityType, name, message) in CollectActorNameViolations(content, resolutionMap: null))
+        {
+            blockers.Add(new ImportPreviewItem(
+                EntityType: entityType,
+                Name: name,
+                ExistingVersion: null,
+                IncomingVersion: null,
+                Kind: ConflictKind.Blocker,
+                FieldDiffJson: null,
+                BlockerReason: message));
+        }
+
        return blockers;
    }

+    /// <summary>
+    /// Lane-F follow-up to review 2026-10-21 01/SR-C1: every bundled name that the
+    /// site runtime turns into an Akka actor name — an instance's unique name (or its
+    /// Rename target), and a template's script, alarm, composition-slot and
+    /// native-alarm-source names (they form the <c>script-</c>/<c>alarm-</c>/
+    /// <c>native-alarm-</c> actor names under the Instance Actor) — must satisfy
+    /// <see cref="ActorNameRules"/>. The Central UI / CLI save paths already enforce
+    /// this; an import that skipped it would persist a name that makes
+    /// <c>ActorOf</c> throw at deploy. Skip-resolved entities are not written, so they
+    /// are not checked (<paramref name="resolutionMap"/> is null at preview: check all).
+    /// </summary>
+    /// <param name="content">The bundle content.</param>
+    /// <param name="resolutionMap">The apply-time resolutions, or null at preview.</param>
+    /// <returns>One (entity type, item name, readable message) tuple per violation.</returns>
+    private static IReadOnlyList<(string EntityType, string Name, string Message)> CollectActorNameViolations(
+        BundleContentDto content,
+        Dictionary<(string, string), ImportResolution>? resolutionMap)
+    {
+        var violations = new List<(string, string, string)>();
+
+        void CheckMember(TemplateDto t, string kind, string memberName)
+        {
+            var error = ActorNameRules.Describe(memberName);
+            if (error is not null)
+            {
+                violations.Add(("Template", $"{t.Name}.{memberName}",
+                    $"Template '{t.Name}' {kind} name is invalid: {error}"));
+            }
+        }
+
+        foreach (var t in content.Templates)
+        {
+            if (IsSkipResolution(resolutionMap, "Template", t.Name)) continue;
+            foreach (var s in t.Scripts) CheckMember(t, "Script", s.Name);
+            foreach (var a in t.Alarms) CheckMember(t, "Alarm", a.Name);
+            foreach (var c in t.Compositions) CheckMember(t, "Composition slot", c.InstanceName);
+            foreach (var n in t.NativeAlarmSources) CheckMember(t, "Native alarm source", n.Name);
+        }
+
+        foreach (var inst in content.Instances)
+        {
+            if (IsSkipResolution(resolutionMap, "Instance", inst.UniqueName)) continue;
+            var effectiveName = inst.UniqueName;
+            if (resolutionMap is not null
+                && resolutionMap.TryGetValue(("Instance", inst.UniqueName), out var r)
+                && r.Action == ResolutionAction.Rename
+                && !string.IsNullOrEmpty(r.RenameTo))
+            {
+                effectiveName = r.RenameTo;
+            }
+
+            var error = ActorNameRules.Describe(effectiveName);
+            if (error is not null)
+            {
+                violations.Add(("Instance", inst.UniqueName,
+                    $"Instance '{effectiveName}' unique name is invalid: {error}"));
+            }
+        }
+
+        return violations;
+    }
+
+    /// <summary>
+    /// Review 2026-10-21 03/R-1: validates a bundled site's gRPC endpoints with
+    /// <see cref="GrpcEndpointRules"/> (empty = "not configured", allowed). Central's
+    /// per-site channel build throws on a value that is not an absolute http(s) URL.
+    /// </summary>
+    /// <param name="dto">The bundled site.</param>
+    /// <returns>Readable errors; empty when both endpoints are acceptable.</returns>
+    private static IReadOnlyList<string> DescribeSiteEndpointErrors(SiteDto dto)
+    {
+        var errors = new List<string>(2);
+        if (!GrpcEndpointRules.TryValidate(dto.GrpcNodeAAddress, allowEmpty: true, out var a))
+        {
+            errors.Add($"Site '{dto.SiteIdentifier}' GrpcNodeAAddress: {a}");
+        }
+        if (!GrpcEndpointRules.TryValidate(dto.GrpcNodeBAddress, allowEmpty: true, out var b))
+        {
+            errors.Add($"Site '{dto.SiteIdentifier}' GrpcNodeBAddress: {b}");
+        }
+        return errors;
+    }
+
+    /// <summary>
+    /// Review 2026-10-21 03/R-1, apply side: endpoint errors for every bundled site
+    /// whose addresses <see cref="ApplySitesAsync"/> would actually write — a site it
+    /// creates (CreateNew, or no resolvable target) or an existing target it
+    /// Overwrites. Mirrors that method's mapping decision so a malformed address on a
+    /// Skip-resolved existing site (never written) does not block the import.
+    /// </summary>
+    private async Task<IReadOnlyList<string>> CollectSiteEndpointErrorsAsync(
+        BundleContentDto content,
+        Dictionary<(string, string), ImportResolution> resolutionMap,
+        BundleNameMap nameMap,
+        CancellationToken ct)
+    {
+        var errors = new List<string>();
+        var siteMappingByIdentifier = nameMap.Sites
+            .GroupBy(m => m.SourceSiteIdentifier, StringComparer.Ordinal)
+            .ToDictionary(g => g.Key, g => g.First(), StringComparer.Ordinal);
+
+        foreach (var siteDto in content.Sites)
+        {
+            var endpointErrors = DescribeSiteEndpointErrors(siteDto);
+            if (endpointErrors.Count == 0) continue;
+
+            siteMappingByIdentifier.TryGetValue(siteDto.SiteIdentifier, out var mapping);
+            var existing = await _siteRepo.GetSiteByIdentifierAsync(siteDto.SiteIdentifier, ct).ConfigureAwait(false);
+            var action = mapping?.Action
+                ?? (existing is not null ? MappingAction.MapToExisting : MappingAction.CreateNew);
+            var target = existing;
+            if (action == MappingAction.MapToExisting
+                && mapping?.TargetSiteIdentifier is { Length: > 0 } targetId
+                && !string.Equals(targetId, siteDto.SiteIdentifier, StringComparison.Ordinal))
+            {
+                target = await _siteRepo.GetSiteByIdentifierAsync(targetId, ct).ConfigureAwait(false);
+            }
+
+            var writes = action == MappingAction.CreateNew
+                || target is null
+                || ResolveOrDefault(resolutionMap, "Site", siteDto.SiteIdentifier).Action == ResolutionAction.Overwrite;
+            if (writes)
+            {
+                errors.AddRange(endpointErrors);
+            }
+        }
+
+        return errors;
+    }
+
    private static void CollectCallIdentifiers(string? body, HashSet<string> sink)
    {
        if (string.IsNullOrEmpty(body)) return;
@@ -1572,9 +1736,18 @@ public sealed class BundleImporter : IBundleImporter
        validationErrors = validationErrors.Count > 0
            ? validationErrors
            : await ValidateSiteInstanceReferencesAsync(content, resolutionMap, nameMap, ct).ConfigureAwait(false);
-        if (validationErrors.Count > 0)
+
+        // Review 2026-10-21 03/R-1 + lane-F follow-up: values the runtime later
+        // trusts blindly — site gRPC endpoints and names that become actor names —
+        // are rejected here, with the change tracker still empty, rather than
+        // surfacing as a channel-build throw on central or an ActorOf throw at deploy.
+        var ruleErrors = CollectActorNameViolations(content, resolutionMap)
+            .Select(v => v.Message)
+            .Concat(await CollectSiteEndpointErrorsAsync(content, resolutionMap, nameMap, ct).ConfigureAwait(false))
+            .ToList();
+        if (ruleErrors.Count > 0 || validationErrors.Count > 0)
        {
-            throw new SemanticValidationException(validationErrors);
+            throw new SemanticValidationException(ruleErrors.Concat(validationErrors).ToList());
        }

        // Advisory template-script reference findings never block the import;