fix(management): strict enum parsing for DataType/ElementDataType/AlarmTriggerType (P6-10)

ManagementActor's ParseDataType/ParseElementDataType (bare Enum.TryParse) and the two AlarmTriggerType parse sites in HandleAddAlarm/HandleUpdateAlarm (raw Enum.Parse) all admitted ordinals ("2"), undefined values ("99" — a deploy-time bomb that saves fine and only detonates at deploy), and comma-joined name lists (bitwise-ORed together regardless of [Flags], e.g. "String,Int32" silently becoming DateType.DateTime). A typo against the raw Enum.Parse sites also surfaced as a masked "An internal error occurred" rather than a curated field-naming error. Adds ParseEnumStrict<TEnum>, mirroring the existing ParseDeploymentStatus guard (names only, defined values only, comma-token rejection per the Medium-wave T8 nuance / AuditQueryParamParsers.ParseEnumListStrict pattern), and routes all four call sites through it. Tests: 22 new ManagementActorTests cases per parse site (ordinal, undefined numeric, comma-joined, and typo all rejected with the curated field-naming error; every defined name still parses through to the service layer). Proof of failure recorded in each rejection test's doc comment; verified live by stashing the ManagementActor.cs fix — exactly the 14 new rejection tests fail pre-fix (13 acceptance tests pass unchanged), confirming the defect and the fix.
2026-08-19 16:15:33 -04:00
parent 88fbc9e351
commit 7ad1872454
2 changed files with 297 additions and 10 deletions
@@ -2931,11 +2931,7 @@ public class ManagementActor : ReceiveActor
    /// <see cref="ArgumentException"/> be masked as a generic internal error.
    /// </summary>
    private static Commons.Types.Enums.DataType ParseDataType(string? dataType)
-    {
-        if (!Enum.TryParse<Commons.Types.Enums.DataType>(dataType, ignoreCase: true, out var parsed))
-            throw new ManagementCommandException($"Unrecognised data type '{dataType}'.");
-        return parsed;
-    }
+        => ParseEnumStrict<Commons.Types.Enums.DataType>("data type", dataType);

    /// <summary>
    /// Parses an optional element data type token. Returns null when the token is
@@ -2945,9 +2941,51 @@ public class ManagementActor : ReceiveActor
    private static Commons.Types.Enums.DataType? ParseElementDataType(string? elementDataType)
    {
        if (string.IsNullOrWhiteSpace(elementDataType)) return null;
-        if (!Enum.TryParse<Commons.Types.Enums.DataType>(elementDataType, ignoreCase: true, out var parsed))
-            throw new ManagementCommandException($"Unrecognised element type '{elementDataType}'.");
-        return parsed;
+        return ParseEnumStrict<Commons.Types.Enums.DataType>("element type", elementDataType);
+    }
+
+    /// <summary>
+    /// Parses a single required enum token by NAME ONLY, rejecting ordinals,
+    /// comma-joined name lists, and defined-but-out-of-range composites; throws a
+    /// curated <see cref="ManagementCommandException"/> naming the field and the
+    /// valid value set on any rejection.
+    /// </summary>
+    /// <remarks>
+    /// Register row 88 pattern (see <see cref="ParseDeploymentStatus"/>). Plain
+    /// <c>Enum.TryParse</c>/<c>Enum.Parse</c> admits three traps a wire-contract
+    /// field must not tolerate:
+    /// <list type="number">
+    ///   <item><b>Ordinals.</b> <c>"2"</c> parses to whatever member sits at that
+    ///   position (e.g. <c>DataType.Float</c>) — a caller-facing field is a
+    ///   name-based contract, never a positional one.</item>
+    ///   <item><b>Undefined ordinals.</b> <c>"99"</c> parses to an undefined enum
+    ///   value that saves without error and only detonates at deploy time.</item>
+    ///   <item><b>Comma-joined tokens.</b> <c>Enum.TryParse</c> bitwise-ORs a
+    ///   comma-separated string's pieces together regardless of whether the enum
+    ///   is <c>[Flags]</c> — <c>"String,Int32"</c> (4|1) silently becomes
+    ///   <c>DateTime</c> (5) for a dense 0-based enum. Any token containing a
+    ///   comma is rejected outright, before <c>TryParse</c> ever sees it.</item>
+    /// </list>
+    /// Mirrors <see cref="Commons.Types.Audit.AuditQueryParamParsers.ParseEnumListStrict{TEnum}"/>,
+    /// adapted to a single required value and <see cref="ManagementCommandException"/>.
+    /// </remarks>
+    /// <exception cref="ManagementCommandException">The value is not a defined member name of <typeparamref name="TEnum"/>.</exception>
+    private static TEnum ParseEnumStrict<TEnum>(string fieldName, string? raw)
+        where TEnum : struct, Enum
+    {
+        var trimmed = raw?.Trim() ?? string.Empty;
+        var looksNumeric = trimmed.Length > 0
+            && (char.IsAsciiDigit(trimmed[0]) || trimmed[0] == '+' || trimmed[0] == '-');
+        if (!looksNumeric
+            && !trimmed.Contains(',')
+            && Enum.TryParse<TEnum>(trimmed, ignoreCase: true, out var parsed)
+            && Enum.IsDefined(parsed))
+        {
+            return parsed;
+        }
+
+        throw new ManagementCommandException(
+            $"Unrecognised {fieldName} '{raw}'. Valid values: {string.Join(", ", Enum.GetNames<TEnum>())}.");
    }

    /// <summary>
@@ -3000,7 +3038,7 @@ public class ManagementActor : ReceiveActor
        var svc = sp.GetRequiredService<TemplateService>();
        var alarm = new TemplateAlarm(cmd.Name)
        {
-            TriggerType = Enum.Parse<ZB.MOM.WW.ScadaBridge.Commons.Types.Enums.AlarmTriggerType>(cmd.TriggerType, ignoreCase: true),
+            TriggerType = ParseEnumStrict<ZB.MOM.WW.ScadaBridge.Commons.Types.Enums.AlarmTriggerType>("alarm trigger type", cmd.TriggerType),
            PriorityLevel = cmd.PriorityLevel,
            Description = cmd.Description,
            TriggerConfiguration = cmd.TriggerConfiguration,
@@ -3015,7 +3053,7 @@ public class ManagementActor : ReceiveActor
        var svc = sp.GetRequiredService<TemplateService>();
        var alarm = new TemplateAlarm(cmd.Name)
        {
-            TriggerType = Enum.Parse<ZB.MOM.WW.ScadaBridge.Commons.Types.Enums.AlarmTriggerType>(cmd.TriggerType, ignoreCase: true),
+            TriggerType = ParseEnumStrict<ZB.MOM.WW.ScadaBridge.Commons.Types.Enums.AlarmTriggerType>("alarm trigger type", cmd.TriggerType),
            PriorityLevel = cmd.PriorityLevel,
            Description = cmd.Description,
            TriggerConfiguration = cmd.TriggerConfiguration,
@@ -765,6 +765,255 @@ public class ManagementActorTests : TestKit, IDisposable
        Assert.DoesNotContain("internal error", response.Error);
    }

+    // ========================================================================
+    // P6-10 (register row-88 class): strict single-value enum parsing in
+    // ParseDataType / ParseElementDataType / the AlarmTriggerType parse sites.
+    // Raw Enum.Parse/TryParse admits ordinals ("2"), undefined values ("99" —
+    // a deploy-time bomb that saves fine and only detonates at deploy), and
+    // (Medium-wave T8 nuance) comma-joined name lists that TryParse/Parse
+    // bitwise-OR together regardless of [Flags]. ParseEnumStrict rejects all
+    // three, and — for AlarmTriggerType specifically — replaces a masked
+    // "An internal error occurred" (raw Enum.Parse's uncaught
+    // ArgumentException) with a curated, field-naming error.
+    // ========================================================================
+
+    [Theory]
+    [InlineData("2")]                 // ordinal for a DEFINED member (Float)
+    [InlineData("99")]                // undefined ordinal — the deploy-time-bomb case
+    [InlineData("String,Int32")]      // comma-joined names — TryParse ORs 4|1 into DateTime (5)
+    public void AddAttribute_WithNonNameDataTypeToken_ReturnsCuratedError(string token)
+    {
+        // PROOF OF FAILURE (pre-fix): the unguarded `Enum.TryParse` in the old
+        // ParseDataType has no numeric guard, no comma guard, and never checks
+        // Enum.IsDefined. Each of these tokens parsed successfully pre-fix (to
+        // Float, an undefined value, and DateTime respectively) and the command
+        // proceeded past the parser into TemplateService, failing later with
+        // "Template with ID 1 not found." instead of this curated message —
+        // verified by stashing this fix and re-running (see task report).
+        _services.AddScoped<TemplateService>();
+        var actor = CreateActor();
+        var envelope = Envelope(
+            new AddTemplateAttributeCommand(1, "Whatever", token, null, null, null, false, null),
+            "Designer");
+
+        actor.Tell(envelope);
+
+        var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
+        Assert.Equal("COMMAND_FAILED", response.ErrorCode);
+        Assert.Contains("Unrecognised data type", response.Error);
+        Assert.Contains(token, response.Error);
+        Assert.Contains("Valid values:", response.Error);
+        Assert.DoesNotContain("internal error", response.Error);
+    }
+
+    [Fact]
+    public void AddAttribute_WithTypoDataType_ReturnsCuratedErrorNamingValidValues()
+    {
+        // The pre-fix ParseDataType already curated a plain typo (that part of
+        // MV-10 was correct), but its message named no valid set — this asserts
+        // the new "Valid values:" suffix, which is absent pre-fix.
+        _services.AddScoped<TemplateService>();
+        var actor = CreateActor();
+        var envelope = Envelope(
+            new AddTemplateAttributeCommand(1, "Whatever", "Sting", null, null, null, false, null),
+            "Designer");
+
+        actor.Tell(envelope);
+
+        var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
+        Assert.Contains("Unrecognised data type 'Sting'", response.Error);
+        Assert.Contains("Valid values:", response.Error);
+        Assert.Contains("Boolean", response.Error);
+        Assert.Contains("List", response.Error);
+    }
+
+    [Theory]
+    [InlineData("Boolean")]
+    [InlineData("Int32")]
+    [InlineData("Float")]
+    [InlineData("Double")]
+    [InlineData("String")]
+    [InlineData("DateTime")]
+    [InlineData("Binary")]
+    public void AddAttribute_WithDefinedDataTypeName_ParsesAndProceedsPastGuard(string name)
+    {
+        // No repository stub: GetTemplateByIdAsync returns null (NSubstitute
+        // default), so a token the guard ACCEPTS reaches AddAttributeAsync's
+        // "Template not found" failure. A token the guard REJECTS would never
+        // reach the service at all (see the tests above), so this proves every
+        // defined name still parses.
+        _services.AddScoped<TemplateService>();
+        var actor = CreateActor();
+        var envelope = Envelope(
+            new AddTemplateAttributeCommand(1, "Whatever", name, null, null, null, false, null),
+            "Designer");
+
+        actor.Tell(envelope);
+
+        var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
+        Assert.Contains("Template with ID 1 not found", response.Error);
+    }
+
+    [Fact]
+    public void AddAttribute_WithDefinedListDataTypeNameAndElementType_ParsesAndProceedsPastGuard()
+    {
+        _services.AddScoped<TemplateService>();
+        var actor = CreateActor();
+        var envelope = Envelope(
+            new AddTemplateAttributeCommand(1, "Whatever", "List", null, null, null, false, "String"),
+            "Designer");
+
+        actor.Tell(envelope);
+
+        var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
+        Assert.Contains("Template with ID 1 not found", response.Error);
+    }
+
+    [Fact]
+    public void UpdateAttribute_WithCommaJoinedDataType_ReturnsCuratedError()
+    {
+        _services.AddScoped<TemplateService>();
+        var actor = CreateActor();
+        var envelope = Envelope(
+            new UpdateTemplateAttributeCommand(5, "Count", "String,Int32", null, null, null, false, null),
+            "Designer");
+
+        actor.Tell(envelope);
+
+        var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
+        Assert.Contains("Unrecognised data type", response.Error);
+        Assert.Contains("Valid values:", response.Error);
+    }
+
+    [Theory]
+    [InlineData("2")]                 // ordinal for a DEFINED, valid element type (Float)
+    [InlineData("99")]                // undefined ordinal
+    [InlineData("String,Int32")]      // comma-joined — combines to DateTime (also a valid element type)
+    public void AddListAttribute_WithNonNameElementTypeToken_ReturnsCuratedError(string token)
+    {
+        // PROOF OF FAILURE (pre-fix): each of these tokens parsed successfully
+        // (Float / an undefined value that fails a LATER, differently-worded
+        // "requires a valid element type" check / DateTime) rather than being
+        // rejected at the parse boundary with this message.
+        _services.AddScoped<TemplateService>();
+        var actor = CreateActor();
+        var envelope = Envelope(
+            new AddTemplateAttributeCommand(1, "Tags", "List", null, null, null, false, token),
+            "Designer");
+
+        actor.Tell(envelope);
+
+        var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
+        Assert.Equal("COMMAND_FAILED", response.ErrorCode);
+        Assert.Contains("Unrecognised element type", response.Error);
+        Assert.Contains(token, response.Error);
+        Assert.Contains("Valid values:", response.Error);
+    }
+
+    [Fact]
+    public void AddListAttribute_WithTypoElementType_ReturnsCuratedErrorNamingValidValues()
+    {
+        _services.AddScoped<TemplateService>();
+        var actor = CreateActor();
+        var envelope = Envelope(
+            new AddTemplateAttributeCommand(1, "Tags", "List", null, null, null, false, "Sting"),
+            "Designer");
+
+        actor.Tell(envelope);
+
+        var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
+        Assert.Contains("Unrecognised element type 'Sting'", response.Error);
+        Assert.Contains("Valid values:", response.Error);
+    }
+
+    [Theory]
+    [InlineData("0")]                              // ordinal for a DEFINED member (ValueMatch)
+    [InlineData("99")]                              // undefined ordinal — the deploy-time-bomb case
+    [InlineData("ValueMatch,RangeViolation")]       // comma-joined — Enum.Parse ORs 0|1 into RangeViolation
+    public void AddAlarm_WithNonNameTriggerTypeToken_ReturnsCuratedError(string token)
+    {
+        // PROOF OF FAILURE (pre-fix): AddAlarm/UpdateAlarm built TriggerType via
+        // raw `Enum.Parse`, which has none of ParseDeploymentStatus's guards.
+        // Each of these tokens parsed SILENTLY (to ValueMatch, an undefined
+        // value, and RangeViolation respectively — no exception at all) and the
+        // command proceeded into TemplateService, failing later with
+        // "Template with ID 1 not found." instead of this curated message.
+        _services.AddScoped<TemplateService>();
+        var actor = CreateActor();
+        var envelope = Envelope(
+            new AddTemplateAlarmCommand(1, "AlarmX", token, 5, null, null, false),
+            "Designer");
+
+        actor.Tell(envelope);
+
+        var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
+        Assert.Equal("COMMAND_FAILED", response.ErrorCode);
+        Assert.Contains("Unrecognised alarm trigger type", response.Error);
+        Assert.Contains(token, response.Error);
+        Assert.Contains("Valid values:", response.Error);
+    }
+
+    [Fact]
+    public void AddAlarm_WithTypoTriggerType_ReturnsCuratedErrorInsteadOfMaskedInternalError()
+    {
+        // PROOF OF FAILURE (pre-fix): unlike ParseDataType (already curated by
+        // MV-10), the TriggerType sites used raw `Enum.Parse`. A typo's
+        // ArgumentException is uncaught by any handler-local guard and reaches
+        // MapFault as an unanticipated fault — masked to a generic
+        // "An internal error occurred (CorrelationId=...)" message, exactly the
+        // masking this task's defect description calls out. Post-fix,
+        // ParseEnumStrict raises a curated ManagementCommandException instead.
+        _services.AddScoped<TemplateService>();
+        var actor = CreateActor();
+        var envelope = Envelope(
+            new AddTemplateAlarmCommand(1, "AlarmX", "ValueMach", 5, null, null, false),
+            "Designer");
+
+        actor.Tell(envelope);
+
+        var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
+        Assert.Equal("COMMAND_FAILED", response.ErrorCode);
+        Assert.Contains("Unrecognised alarm trigger type 'ValueMach'", response.Error);
+        Assert.Contains("Valid values:", response.Error);
+        Assert.DoesNotContain("internal error", response.Error);
+    }
+
+    [Theory]
+    [InlineData("ValueMatch")]
+    [InlineData("RangeViolation")]
+    [InlineData("RateOfChange")]
+    [InlineData("HiLo")]
+    [InlineData("Expression")]
+    public void AddAlarm_WithDefinedTriggerTypeName_ParsesAndProceedsPastGuard(string name)
+    {
+        _services.AddScoped<TemplateService>();
+        var actor = CreateActor();
+        var envelope = Envelope(
+            new AddTemplateAlarmCommand(1, "AlarmX", name, 5, null, null, false),
+            "Designer");
+
+        actor.Tell(envelope);
+
+        var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
+        Assert.Contains("Template with ID 1 not found", response.Error);
+    }
+
+    [Fact]
+    public void UpdateAlarm_WithOrdinalTriggerType_ReturnsCuratedError()
+    {
+        _services.AddScoped<TemplateService>();
+        var actor = CreateActor();
+        var envelope = Envelope(
+            new UpdateTemplateAlarmCommand(5, "AlarmX", "0", 5, null, null, false),
+            "Designer");
+
+        actor.Tell(envelope);
+
+        var response = ExpectMsg<ManagementError>(TimeSpan.FromSeconds(5));
+        Assert.Contains("Unrecognised alarm trigger type", response.Error);
+        Assert.Contains("Valid values:", response.Error);
+    }
+
    [Fact]
    public void UpdateAttribute_ValidScalar_PersistsValueAndKeepsFixedFields()
    {