fix(templateengine): reject instance/script/alarm/slot names that are not valid actor names at save (review 2026-10-21 01/SR-C1)

2026-10-22 06:23:47 -04:00
parent 5919a71b8b
commit 7d11a1db8d
4 changed files with 149 additions and 0 deletions
@@ -93,6 +93,23 @@ When a template composes a feature module, members from that module are addresse
 - The composing template's own members (not from a module) have no prefix — they are top-level names.
 - Naming collision detection operates on canonical names, so two modules can define the same member name as long as their module instance names differ.

+### Runtime actor-name rules (review 2026-10-21 01/SR-C1)
+
+Some names become Akka actor names at the site: the instance **unique name** (the
+Instance Actor), and every **script**, **alarm** and **native alarm source**
+canonical name (`script-…`, `alarm-…`, `native-alarm-…` children of the Instance
+Actor) — which also embeds every **composition slot** name on its path. Akka accepts
+only ASCII letters, digits and `-_.*$+:@&=,!~';` (plus `%HH` escapes) and no leading
+`$`; anything else (a space, `#`, `/`, non-ASCII letters) makes `ActorOf` throw at
+deploy — for an instance name, inside the site's Deployment Manager singleton. The
+Template Engine therefore rejects such names **at save** with a readable error, using
+the shared `Commons.Validation.ActorNameRules`: `InstanceService.CreateInstanceAsync`
+(unique name), `TemplateService.AddScriptAsync` / `AddAlarmAsync` (member names are
+immutable after creation, so updates need no check) and `AddCompositionAsync` /
+`RenameCompositionAsync` (slot names). Attribute names and template names are not
+actor names and are unrestricted. Native alarm sources are created outside
+`TemplateService` (Management Service / Transport import); those paths must apply the same rule.
+
 ### Derived template naming

 A composition slot is materialized as its own *derived* template. A derived
@@ -4,6 +4,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services;
 using ZB.MOM.WW.ScadaBridge.Commons.Messages.Management;
 using ZB.MOM.WW.ScadaBridge.Commons.Types;
 using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums;
+using ZB.MOM.WW.ScadaBridge.Commons.Validation;
 using ZB.MOM.WW.ScadaBridge.TemplateEngine;

 namespace ZB.MOM.WW.ScadaBridge.TemplateEngine.Services;
@@ -59,6 +60,13 @@ public class InstanceService
        if (string.IsNullOrWhiteSpace(uniqueName))
            return Result<Instance>.Failure("Instance unique name is required.");

+        // The unique name becomes the site's Instance Actor name; a character Akka
+        // rejects would make ActorOf throw inside the Deployment Manager singleton on
+        // deploy (review 2026-10-21 01/SR-C1). Reject it here with a readable error.
+        var nameError = ActorNameRules.Describe(uniqueName);
+        if (nameError != null)
+            return Result<Instance>.Failure($"Invalid instance unique name: {nameError}");
+
        // Verify template exists
        var template = await _repository.GetTemplateByIdAsync(templateId, cancellationToken);
        if (template == null)
@@ -4,6 +4,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services;
 using ZB.MOM.WW.ScadaBridge.Commons.Messages.Management;
 using ZB.MOM.WW.ScadaBridge.Commons.Types;
 using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums;
+using ZB.MOM.WW.ScadaBridge.Commons.Validation;

 namespace ZB.MOM.WW.ScadaBridge.TemplateEngine;

@@ -518,6 +519,10 @@ public class TemplateService
        string user,
        CancellationToken cancellationToken = default)
    {
+        var nameError = DescribeRuntimeNameError("Alarm", alarm.Name);
+        if (nameError != null)
+            return Result<TemplateAlarm>.Failure(nameError);
+
        var template = await _repository.GetTemplateByIdAsync(templateId, cancellationToken);
        if (template == null)
            return Result<TemplateAlarm>.Failure($"Template with ID {templateId} not found.");
@@ -693,6 +698,10 @@ public class TemplateService
        string user,
        CancellationToken cancellationToken = default)
    {
+        var nameError = DescribeRuntimeNameError("Script", script.Name);
+        if (nameError != null)
+            return Result<TemplateScript>.Failure(nameError);
+
        var template = await _repository.GetTemplateByIdAsync(templateId, cancellationToken);
        if (template == null)
            return Result<TemplateScript>.Failure($"Template with ID {templateId} not found.");
@@ -869,6 +878,10 @@ public class TemplateService
        if (string.IsNullOrWhiteSpace(instanceName))
            return Result<TemplateComposition>.Failure("Instance name is required for composition.");

+        var nameError = DescribeRuntimeNameError("Composition slot", instanceName);
+        if (nameError != null)
+            return Result<TemplateComposition>.Failure(nameError);
+
        var template = await _repository.GetTemplateByIdAsync(templateId, cancellationToken);
        if (template == null)
            return Result<TemplateComposition>.Failure($"Template with ID {templateId} not found.");
@@ -987,6 +1000,10 @@ public class TemplateService
        if (string.IsNullOrWhiteSpace(newInstanceName))
            return Result<TemplateComposition>.Failure("Slot name is required.");

+        var nameError = DescribeRuntimeNameError("Composition slot", newInstanceName);
+        if (nameError != null)
+            return Result<TemplateComposition>.Failure(nameError);
+
        var composition = await _repository.GetTemplateCompositionByIdAsync(compositionId, cancellationToken);
        if (composition == null)
            return Result<TemplateComposition>.Failure($"Composition with ID {compositionId} not found.");
@@ -1788,4 +1805,21 @@ public class TemplateService
        clone.Compositions.Add(newComp);
        return clone;
    }
+
+    /// <summary>
+    /// Script, alarm and composition-slot names end up inside site actor names —
+    /// <c>script-{canonical}</c>, <c>alarm-{canonical}</c>, <c>native-alarm-{canonical}</c>,
+    /// where the canonical name is the dotted slot path plus the member name. A
+    /// character Akka rejects makes the Instance Actor's <c>ActorOf</c> throw at
+    /// deploy, so reject it at save with a readable error (review 2026-10-21 01/SR-C1).
+    /// Attributes are not actors and are not checked.
+    /// </summary>
+    /// <param name="kind">The member kind, for the message.</param>
+    /// <param name="name">The proposed name.</param>
+    /// <returns>An error message, or <c>null</c> when the name is usable.</returns>
+    private static string? DescribeRuntimeNameError(string kind, string? name)
+    {
+        var error = ActorNameRules.Describe(name);
+        return error == null ? null : $"{kind} name is invalid: {error}";
+    }
 }
@@ -0,0 +1,90 @@
+using Moq;
+using ZB.MOM.WW.ScadaBridge.Commons.Entities.Instances;
+using ZB.MOM.WW.ScadaBridge.Commons.Entities.Templates;
+using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories;
+using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services;
+using ZB.MOM.WW.ScadaBridge.TemplateEngine.Services;
+
+namespace ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests;
+
+/// <summary>
+/// Review 2026-10-21 01/SR-C1 (central half): names that become Akka actor names at
+/// the site — the instance unique name (the Instance Actor), and the script / alarm /
+/// composition-slot names that make up the <c>script-</c>/<c>alarm-</c>/
+/// <c>native-alarm-</c> child canonical names — are rejected at save with a readable
+/// error, instead of crashing the site's Deployment Manager / failing the deploy when
+/// <c>ActorOf</c> throws <c>InvalidActorNameException</c>.
+/// </summary>
+public class ActorNameValidationTests
+{
+    private readonly Mock<ITemplateEngineRepository> _repoMock = new();
+    private readonly Mock<IAuditService> _auditMock = new();
+
+    private TemplateService TemplateSut => new(_repoMock.Object, _auditMock.Object);
+
+    /// <summary>Verifies that an instance unique name with invalid actor-name characters is rejected.</summary>
+    /// <param name="name">An instance name Akka cannot use as an actor name.</param>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Theory]
+    [InlineData("Furnace #3")]
+    [InlineData("Line 1")]
+    [InlineData("Ofen Süd")]
+    [InlineData("a/b")]
+    [InlineData("$system")]
+    public async Task CreateInstance_InvalidActorName_FailsBeforeAnyWrite(string name)
+    {
+        var sut = new InstanceService(_repoMock.Object, _auditMock.Object);
+
+        var result = await sut.CreateInstanceAsync(name, 1, 1, null, "admin");
+
+        Assert.True(result.IsFailure);
+        Assert.Contains("not allowed", result.Error);
+        _repoMock.Verify(r => r.AddInstanceAsync(It.IsAny<Instance>(), It.IsAny<CancellationToken>()), Times.Never);
+    }
+
+    /// <summary>Verifies that a script name with invalid actor-name characters is rejected.</summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task AddScript_InvalidActorName_Fails()
+    {
+        var result = await TemplateSut.AddScriptAsync(1, new TemplateScript("Calc #1", "return 1;"), "admin");
+
+        Assert.True(result.IsFailure);
+        Assert.Contains("Calc #1", result.Error);
+        _repoMock.Verify(r => r.AddTemplateScriptAsync(It.IsAny<TemplateScript>(), It.IsAny<CancellationToken>()), Times.Never);
+    }
+
+    /// <summary>Verifies that an alarm name with invalid actor-name characters is rejected.</summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task AddAlarm_InvalidActorName_Fails()
+    {
+        var result = await TemplateSut.AddAlarmAsync(1, new TemplateAlarm("High Temp"), "admin");
+
+        Assert.True(result.IsFailure);
+        Assert.Contains("High Temp", result.Error);
+        _repoMock.Verify(r => r.AddTemplateAlarmAsync(It.IsAny<TemplateAlarm>(), It.IsAny<CancellationToken>()), Times.Never);
+    }
+
+    /// <summary>Verifies that a composition slot name with invalid actor-name characters is rejected.</summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task AddComposition_InvalidActorName_Fails()
+    {
+        var result = await TemplateSut.AddCompositionAsync(1, 2, "Left Sensor", "admin");
+
+        Assert.True(result.IsFailure);
+        Assert.Contains("Left Sensor", result.Error);
+    }
+
+    /// <summary>Verifies that renaming a composition slot to an invalid actor name is rejected.</summary>
+    /// <returns>A task that represents the asynchronous operation.</returns>
+    [Fact]
+    public async Task RenameComposition_InvalidActorName_Fails()
+    {
+        var result = await TemplateSut.RenameCompositionAsync(50, "new slot", "admin");
+
+        Assert.True(result.IsFailure);
+        Assert.Contains("new slot", result.Error);
+    }
+}