From 0164f8a0d67514470700145a9419553cbf2295ea Mon Sep 17 00:00:00 2001
From: Joseph Doherty <dohejw01@gmail.com>
Date: Tue, 16 Jun 2026 16:13:38 -0400
Subject: [PATCH] fix(mgmt): MV-10 review fixes (ElementDataType fixed-field in
 LockEnforcer; graceful bad-DataType error; message consistency)

---
 .../ManagementActor.cs                        | 19 +++++-
 .../LockEnforcer.cs                           |  8 ++-
 .../TemplateService.cs                        |  1 +
 .../ManagementActorTests.cs                   | 62 +++++++++++++++++++
 .../LockEnforcerTests.cs                      | 58 +++++++++++++++++
 5 files changed, 144 insertions(+), 4 deletions(-)

diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs
index a99a0729..a1d6e950 100644
--- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs
+++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs
@@ -1442,7 +1442,7 @@ public class ManagementActor : ReceiveActor
     private static async Task<object?> HandleAddAttribute(IServiceProvider sp, AddTemplateAttributeCommand cmd, string user)
     {
         var svc = sp.GetRequiredService<TemplateService>();
-        var dataType = Enum.Parse<Commons.Types.Enums.DataType>(cmd.DataType, ignoreCase: true);
+        var dataType = ParseDataType(cmd.DataType);
         var elementType = ParseElementDataType(cmd.ElementDataType);
         ValidateAttributeTypes(cmd.Name, dataType, elementType, cmd.Value);
         var attr = new TemplateAttribute(cmd.Name)
@@ -1461,7 +1461,7 @@ public class ManagementActor : ReceiveActor
     private static async Task<object?> HandleUpdateAttribute(IServiceProvider sp, UpdateTemplateAttributeCommand cmd, string user)
     {
         var svc = sp.GetRequiredService<TemplateService>();
-        var dataType = Enum.Parse<Commons.Types.Enums.DataType>(cmd.DataType, ignoreCase: true);
+        var dataType = ParseDataType(cmd.DataType);
         var elementType = ParseElementDataType(cmd.ElementDataType);
         ValidateAttributeTypes(cmd.Name, dataType, elementType, cmd.Value);
         var attr = new TemplateAttribute(cmd.Name)
@@ -1477,6 +1477,19 @@ public class ManagementActor : ReceiveActor
         return result.IsSuccess ? result.Value : throw new ManagementCommandException(result.Error);
     }
 
+    /// <summary>
+    /// Parses a required data type token. Throws <see cref="ManagementCommandException"/>
+    /// (surfaced to the caller as a curated COMMAND_FAILED message) on an
+    /// unrecognised type name, rather than letting <c>Enum.Parse</c>'s
+    /// <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;
+    }
+
     /// <summary>
     /// Parses an optional element data type token. Returns null when the token is
     /// empty/whitespace; throws <see cref="ManagementCommandException"/> on an
@@ -1524,7 +1537,7 @@ public class ManagementActor : ReceiveActor
         }
         else if (elementType is not null)
         {
-            throw new ManagementCommandException("Element type is only valid on List attributes.");
+            throw new ManagementCommandException($"Attribute '{name}': ElementDataType is only valid on List attributes.");
         }
     }
 
diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/LockEnforcer.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/LockEnforcer.cs
index d6d62c67..60c6d0ac 100644
--- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/LockEnforcer.cs
+++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/LockEnforcer.cs
@@ -15,7 +15,7 @@ namespace ZB.MOM.WW.ScadaBridge.TemplateEngine;
 ///   overrides that were previously blocked (TemplateEngine-022).
 ///
 /// Override granularity:
-/// - Attributes: Value and Description overridable; DataType and DataSourceReference fixed.
+/// - Attributes: Value and Description overridable; DataType, ElementDataType and DataSourceReference fixed.
 /// - Alarms: Priority, TriggerConfiguration, Description, OnTriggerScript overridable; Name and TriggerType fixed.
 /// - Scripts: Code, TriggerConfiguration, MinTimeBetweenRuns, ExecutionTimeoutSeconds, params/return overridable; Name fixed.
 /// - Lock flag applies to the entire member (attribute/alarm/script).
@@ -43,6 +43,12 @@ public static class LockEnforcer
             return $"Attribute '{original.Name}': DataType cannot be overridden (fixed).";
         }
 
+        // ElementDataType is fixed (the element scalar type of a List attribute) — cannot change
+        if (proposed.ElementDataType != original.ElementDataType)
+        {
+            return $"Attribute '{original.Name}': ElementDataType cannot be overridden (fixed).";
+        }
+
         // DataSourceReference is fixed — cannot change
         if (proposed.DataSourceReference != original.DataSourceReference)
         {
diff --git a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/TemplateService.cs b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/TemplateService.cs
index f099c389..c71deae9 100644
--- a/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/TemplateService.cs
+++ b/src/ZB.MOM.WW.ScadaBridge.TemplateEngine/TemplateService.cs
@@ -982,6 +982,7 @@ public class TemplateService
             {
                 Value = attr.Value,
                 DataType = attr.DataType,
+                ElementDataType = attr.ElementDataType,
                 IsLocked = attr.IsLocked,
                 Description = attr.Description,
                 DataSourceReference = attr.DataSourceReference,
diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs
index da5eea7b..8f14aba3 100644
--- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs
+++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/ManagementActorTests.cs
@@ -521,6 +521,68 @@ public class ManagementActorTests : TestKit, IDisposable
         Assert.Contains("only valid on List attributes", response.Error);
     }
 
+    [Fact]
+    public void AddAttribute_WithUnrecognisedDataType_ReturnsCuratedManagementError()
+    {
+        // MV-10 review fix: a bogus DataType string used to throw ArgumentException
+        // from Enum.Parse, which MapFault masks as a generic "An internal error
+        // occurred" message. TryParse now raises a curated ManagementCommandException
+        // whose message names the offending token.
+        _services.AddScoped<TemplateService>();
+        var actor = CreateActor();
+        var envelope = Envelope(
+            new AddTemplateAttributeCommand(1, "Whatever", "Nonsense", 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("Nonsense", response.Error);
+        Assert.DoesNotContain("internal error", response.Error);
+    }
+
+    [Fact]
+    public void UpdateAttribute_ValidScalar_PersistsValueAndKeepsFixedFields()
+    {
+        // MV-10 review fix: documents the fixed-field contract on update —
+        // ValidateAttributeOverride(existing, proposed) runs for every update, so
+        // a matching DataType/ElementDataType lets the Value change persist while
+        // the fixed type columns are never copied from the proposed attribute.
+        var template = new Template("T1") { Id = 1 };
+        var existing = new TemplateAttribute("Count")
+        {
+            Id = 5, TemplateId = 1, DataType = Commons.Types.Enums.DataType.Int32,
+            ElementDataType = null, Value = "0"
+        };
+        _templateRepo.GetTemplateAttributeByIdAsync(5, Arg.Any<CancellationToken>()).Returns(existing);
+        _templateRepo.GetTemplateByIdAsync(1, Arg.Any<CancellationToken>()).Returns(template);
+        _templateRepo.GetAllTemplatesAsync(Arg.Any<CancellationToken>())
+            .Returns(new List<Template> { template });
+
+        TemplateAttribute? updated = null;
+        _templateRepo
+            .When(r => r.UpdateTemplateAttributeAsync(Arg.Any<TemplateAttribute>(), Arg.Any<CancellationToken>()))
+            .Do(ci => updated = ci.Arg<TemplateAttribute>());
+        _templateRepo.SaveChangesAsync(Arg.Any<CancellationToken>()).Returns(1);
+        _services.AddScoped<TemplateService>();
+
+        var actor = CreateActor();
+        var envelope = Envelope(
+            new UpdateTemplateAttributeCommand(5, "Count", "Int32", "42", null, null, false, null),
+            "Designer");
+
+        actor.Tell(envelope);
+
+        var response = ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(5));
+        Assert.Equal(envelope.CorrelationId, response.CorrelationId);
+        Assert.NotNull(updated);
+        Assert.Equal("42", updated!.Value);                                       // Value updated
+        Assert.Equal(Commons.Types.Enums.DataType.Int32, updated.DataType);       // fixed
+        Assert.Null(updated.ElementDataType);                                     // fixed
+    }
+
     [Fact]
     public void UpdateApiKey_WithDesignRole_ReturnsUnauthorized()
     {
diff --git a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/LockEnforcerTests.cs b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/LockEnforcerTests.cs
index afa4bbb1..64d5c707 100644
--- a/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/LockEnforcerTests.cs
+++ b/tests/ZB.MOM.WW.ScadaBridge.TemplateEngine.Tests/LockEnforcerTests.cs
@@ -80,6 +80,64 @@ public class LockEnforcerTests
         Assert.Null(result);
     }
 
+    [Fact]
+    public void ValidateAttributeOverride_ElementDataTypeChanged_ReturnsError()
+    {
+        // MV-10 review fix: ElementDataType is the element scalar type of a List
+        // attribute and is fixed by the defining level, exactly like DataType.
+        // TemplateService.UpdateAttributeAsync never copies it onto the persisted
+        // row, so a mismatch must be rejected before the Value (validated against
+        // the real element type) is persisted against the wrong type.
+        var original = new TemplateAttribute("Tags")
+        {
+            DataType = DataType.List, ElementDataType = DataType.Int32, IsLocked = false
+        };
+        var proposed = new TemplateAttribute("Tags")
+        {
+            DataType = DataType.List, ElementDataType = DataType.String, IsLocked = false // changed!
+        };
+
+        var result = LockEnforcer.ValidateAttributeOverride(original, proposed);
+
+        Assert.NotNull(result);
+        Assert.Contains("ElementDataType", result);
+    }
+
+    [Fact]
+    public void ValidateAttributeOverride_ElementDataTypeMatches_ReturnsNull()
+    {
+        var original = new TemplateAttribute("Tags")
+        {
+            DataType = DataType.List, ElementDataType = DataType.Int32, IsLocked = false, Value = "[1]"
+        };
+        var proposed = new TemplateAttribute("Tags")
+        {
+            DataType = DataType.List, ElementDataType = DataType.Int32, IsLocked = false, Value = "[1,2]"
+        };
+
+        var result = LockEnforcer.ValidateAttributeOverride(original, proposed);
+
+        Assert.Null(result);
+    }
+
+    [Fact]
+    public void ValidateAttributeOverride_ElementDataTypeBothNull_ReturnsNull()
+    {
+        // Scalar attributes carry no element type on either side — not a change.
+        var original = new TemplateAttribute("Speed")
+        {
+            DataType = DataType.Float, ElementDataType = null, IsLocked = false, Value = "0"
+        };
+        var proposed = new TemplateAttribute("Speed")
+        {
+            DataType = DataType.Float, ElementDataType = null, IsLocked = false, Value = "100"
+        };
+
+        var result = LockEnforcer.ValidateAttributeOverride(original, proposed);
+
+        Assert.Null(result);
+    }
+
     [Fact]
     public void ValidateAlarmOverride_LockedAlarm_ReturnsError()
     {