fix(mgmt): MV-10 review fixes (ElementDataType fixed-field in LockEnforcer; graceful bad-DataType error; message consistency)

2026-06-16 16:13:38 -04:00
parent 1525670fe7
commit 0164f8a0d6
5 changed files with 144 additions and 4 deletions
@@ -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()
    {
@@ -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()
    {