fix(templates): hoist (DataType,ElementDataType,Value) attribute validation into TemplateService (#92)

2026-06-19 02:19:35 -04:00
parent e51104af5f
commit d844405cec
2 changed files with 239 additions and 0 deletions
@@ -2,6 +2,7 @@ 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.Commons.Types;
+using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums;

 namespace ZB.MOM.WW.ScadaBridge.TemplateEngine;

@@ -266,6 +267,16 @@ public class TemplateService
        string user,
        CancellationToken cancellationToken = default)
    {
+        // Defense-in-depth (#92): hoist the (DataType, ElementDataType, Value)
+        // guard — previously only enforced on the CLI/API path in
+        // ManagementActor.ValidateAttributeTypes — into the service so the
+        // Central UI's direct TemplateService write path shares one server-side
+        // check. Mirrors ValidateAttributeTypes exactly, reusing AttributeValueCodec.
+        var typeError = ValidateAttributeTypes(
+            attribute.Name, attribute.DataType, attribute.ElementDataType, attribute.Value);
+        if (typeError != null)
+            return Result<TemplateAttribute>.Failure(typeError);
+
        var template = await _repository.GetTemplateByIdAsync(templateId, cancellationToken);
        if (template == null)
            return Result<TemplateAttribute>.Failure($"Template with ID {templateId} not found.");
@@ -312,6 +323,17 @@ public class TemplateService
        if (existing == null)
            return Result<TemplateAttribute>.Failure($"Attribute with ID {attributeId} not found.");

+        // Defense-in-depth (#92): hoist the (DataType, ElementDataType, Value)
+        // guard out of ManagementActor.ValidateAttributeTypes so the Central UI's
+        // direct TemplateService write path shares one server-side check. DataType
+        // and ElementDataType are fixed by the defining level (not copied from the
+        // proposed attribute below), so the effective persisted triple is the
+        // existing type pairing with the proposed value. Reuses AttributeValueCodec.
+        var typeError = ValidateAttributeTypes(
+            existing.Name, existing.DataType, existing.ElementDataType, proposed.Value);
+        if (typeError != null)
+            return Result<TemplateAttribute>.Failure(typeError);
+
        // Validate override rules if this is an override (parent has same-named attribute)
        var template = await _repository.GetTemplateByIdAsync(existing.TemplateId, cancellationToken);
        if (template?.ParentTemplateId != null)
@@ -412,6 +434,52 @@ public class TemplateService
        return Result<bool>.Success(true);
    }

+    /// <summary>
+    /// Validates the (DataType, ElementDataType, Value) triple for an attribute.
+    /// Returns a human-readable failure message (for <c>Result.Failure</c>), or
+    /// <c>null</c> when the triple is valid. Mirrors the CLI/API-path guard in
+    /// <c>ManagementActor.ValidateAttributeTypes</c> so both server write paths
+    /// share one server-side check (#92), reusing <see cref="AttributeValueCodec"/>
+    /// for the element-type and value-decode logic:
+    /// <list type="bullet">
+    /// <item>DataType must be a defined enum value.</item>
+    /// <item>List attributes require a valid scalar element type, and a present
+    /// default value must decode against it.</item>
+    /// <item>Scalar attributes may not carry an element type.</item>
+    /// </list>
+    /// </summary>
+    private static string? ValidateAttributeTypes(
+        string name, DataType dataType, DataType? elementType, string? value)
+    {
+        if (!Enum.IsDefined(dataType))
+            return $"Attribute '{name}' has an unrecognised data type.";
+
+        if (dataType == DataType.List)
+        {
+            if (elementType is null || !AttributeValueCodec.IsValidElementType(elementType.Value))
+                return $"List attribute '{name}' requires a valid element type " +
+                       "(String, Int32, Float, Double, Boolean, DateTime).";
+
+            if (!string.IsNullOrWhiteSpace(value))
+            {
+                try
+                {
+                    AttributeValueCodec.Decode(value, DataType.List, elementType);
+                }
+                catch (FormatException ex)
+                {
+                    return $"List attribute '{name}' has an invalid list value: {ex.Message}";
+                }
+            }
+        }
+        else if (elementType is not null)
+        {
+            return $"Attribute '{name}': ElementDataType is only valid on List attributes.";
+        }
+
+        return null;
+    }
+
    // ========================================================================
    // WP-3: Alarm Definitions
    // ========================================================================
@@ -234,6 +234,177 @@ public class TemplateServiceTests
        Assert.Contains("already exists", result.Error);
    }

+    // ── #92: (DataType, ElementDataType, Value) validation hoisted into the
+    //         service so the Central UI's direct write path shares the same
+    //         server-side guard as ManagementActor.ValidateAttributeTypes. ──
+
+    [Fact]
+    public async Task AddAttribute_InvalidDataType_Fails()
+    {
+        var template = new Template("Pump") { Id = 1 };
+        _repoMock.Setup(r => r.GetTemplateByIdAsync(1, It.IsAny<CancellationToken>())).ReturnsAsync(template);
+
+        // An out-of-range enum value is the in-service equivalent of an
+        // unrecognised data-type token (the CLI/API path rejects the token before
+        // the enum is parsed).
+        var attr = new TemplateAttribute("Temperature") { DataType = (DataType)999, Value = "0" };
+        var result = await _service.AddAttributeAsync(1, attr, "admin");
+
+        Assert.True(result.IsFailure);
+        Assert.Contains("data type", result.Error, StringComparison.OrdinalIgnoreCase);
+        // The attribute must never reach the repository.
+        _repoMock.Verify(r => r.AddTemplateAttributeAsync(It.IsAny<TemplateAttribute>(), It.IsAny<CancellationToken>()), Times.Never);
+    }
+
+    [Fact]
+    public async Task AddAttribute_ListMissingElementType_Fails()
+    {
+        var template = new Template("Pump") { Id = 1 };
+        _repoMock.Setup(r => r.GetTemplateByIdAsync(1, It.IsAny<CancellationToken>())).ReturnsAsync(template);
+
+        var attr = new TemplateAttribute("SetPoints") { DataType = DataType.List, ElementDataType = null };
+        var result = await _service.AddAttributeAsync(1, attr, "admin");
+
+        Assert.True(result.IsFailure);
+        Assert.Contains("element type", result.Error, StringComparison.OrdinalIgnoreCase);
+        _repoMock.Verify(r => r.AddTemplateAttributeAsync(It.IsAny<TemplateAttribute>(), It.IsAny<CancellationToken>()), Times.Never);
+    }
+
+    [Fact]
+    public async Task AddAttribute_ListInvalidElementType_Fails()
+    {
+        var template = new Template("Pump") { Id = 1 };
+        _repoMock.Setup(r => r.GetTemplateByIdAsync(1, It.IsAny<CancellationToken>())).ReturnsAsync(template);
+
+        // Binary is a real DataType but not a valid List element scalar.
+        var attr = new TemplateAttribute("SetPoints") { DataType = DataType.List, ElementDataType = DataType.Binary };
+        var result = await _service.AddAttributeAsync(1, attr, "admin");
+
+        Assert.True(result.IsFailure);
+        Assert.Contains("element type", result.Error, StringComparison.OrdinalIgnoreCase);
+        _repoMock.Verify(r => r.AddTemplateAttributeAsync(It.IsAny<TemplateAttribute>(), It.IsAny<CancellationToken>()), Times.Never);
+    }
+
+    [Fact]
+    public async Task AddAttribute_ScalarWithElementType_Fails()
+    {
+        var template = new Template("Pump") { Id = 1 };
+        _repoMock.Setup(r => r.GetTemplateByIdAsync(1, It.IsAny<CancellationToken>())).ReturnsAsync(template);
+
+        var attr = new TemplateAttribute("Temperature") { DataType = DataType.Float, ElementDataType = DataType.Int32, Value = "0" };
+        var result = await _service.AddAttributeAsync(1, attr, "admin");
+
+        Assert.True(result.IsFailure);
+        Assert.Contains("ElementDataType", result.Error);
+        _repoMock.Verify(r => r.AddTemplateAttributeAsync(It.IsAny<TemplateAttribute>(), It.IsAny<CancellationToken>()), Times.Never);
+    }
+
+    [Fact]
+    public async Task AddAttribute_ListValueDoesNotDecode_Fails()
+    {
+        var template = new Template("Pump") { Id = 1 };
+        _repoMock.Setup(r => r.GetTemplateByIdAsync(1, It.IsAny<CancellationToken>())).ReturnsAsync(template);
+
+        // A valid typed List, but the value carries an element that is not an Int32.
+        var attr = new TemplateAttribute("SetPoints")
+        {
+            DataType = DataType.List, ElementDataType = DataType.Int32, Value = """["abc"]"""
+        };
+        var result = await _service.AddAttributeAsync(1, attr, "admin");
+
+        Assert.True(result.IsFailure);
+        Assert.Contains("invalid list value", result.Error, StringComparison.OrdinalIgnoreCase);
+        _repoMock.Verify(r => r.AddTemplateAttributeAsync(It.IsAny<TemplateAttribute>(), It.IsAny<CancellationToken>()), Times.Never);
+    }
+
+    [Fact]
+    public async Task AddAttribute_ScalarValueDoesNotParse_Fails()
+    {
+        // Mirrors what AttributeValueCodec.Decode would accept for the declared
+        // (DataType, ElementDataType). Scalars decode as-is in the codec, so a
+        // free-form scalar value is accepted by both the CLI/API guard and the
+        // service guard — there is no scalar-string parse here. Documenting the
+        // accepted shape: a non-numeric scalar Value is allowed (validated at deploy).
+        var template = new Template("Pump") { Id = 1 };
+        _repoMock.Setup(r => r.GetTemplateByIdAsync(1, It.IsAny<CancellationToken>())).ReturnsAsync(template);
+        _repoMock.Setup(r => r.GetAllTemplatesAsync(It.IsAny<CancellationToken>()))
+            .ReturnsAsync(new List<Template> { template });
+
+        var attr = new TemplateAttribute("Temperature") { DataType = DataType.Int32, Value = "not-a-number" };
+        var result = await _service.AddAttributeAsync(1, attr, "admin");
+
+        // Behaviour-preserving: the codec leaves scalars untouched, so this is
+        // accepted (the deploy-time SemanticValidator remains the scalar backstop).
+        Assert.True(result.IsSuccess);
+    }
+
+    [Fact]
+    public async Task AddAttribute_ValidTypedList_Succeeds()
+    {
+        var template = new Template("Pump") { Id = 1 };
+        _repoMock.Setup(r => r.GetTemplateByIdAsync(1, It.IsAny<CancellationToken>())).ReturnsAsync(template);
+        _repoMock.Setup(r => r.GetAllTemplatesAsync(It.IsAny<CancellationToken>()))
+            .ReturnsAsync(new List<Template> { template });
+
+        var attr = new TemplateAttribute("SetPoints")
+        {
+            DataType = DataType.List, ElementDataType = DataType.Int32, Value = """[10, 20, 30]"""
+        };
+        var result = await _service.AddAttributeAsync(1, attr, "admin");
+
+        Assert.True(result.IsSuccess);
+        Assert.Equal("SetPoints", result.Value.Name);
+        Assert.Equal(DataType.List, result.Value.DataType);
+        Assert.Equal(DataType.Int32, result.Value.ElementDataType);
+    }
+
+    [Fact]
+    public async Task UpdateAttribute_InvalidListValue_Fails()
+    {
+        // DataType/ElementDataType are fixed on the existing row; the proposed
+        // value must still decode against that fixed element type.
+        var existing = new TemplateAttribute("SetPoints")
+        {
+            Id = 1, TemplateId = 1, DataType = DataType.List, ElementDataType = DataType.Int32, Value = """[1, 2]"""
+        };
+        _repoMock.Setup(r => r.GetTemplateAttributeByIdAsync(1, It.IsAny<CancellationToken>())).ReturnsAsync(existing);
+        var template = new Template("Pump") { Id = 1 };
+        _repoMock.Setup(r => r.GetTemplateByIdAsync(1, It.IsAny<CancellationToken>())).ReturnsAsync(template);
+
+        var proposed = new TemplateAttribute("SetPoints")
+        {
+            DataType = DataType.List, ElementDataType = DataType.Int32, Value = """["abc"]"""
+        };
+        var result = await _service.UpdateAttributeAsync(1, proposed, "admin");
+
+        Assert.True(result.IsFailure);
+        Assert.Contains("invalid list value", result.Error, StringComparison.OrdinalIgnoreCase);
+        // The fixed value must not have been mutated.
+        Assert.Equal("""[1, 2]""", existing.Value);
+        _repoMock.Verify(r => r.UpdateTemplateAttributeAsync(It.IsAny<TemplateAttribute>(), It.IsAny<CancellationToken>()), Times.Never);
+    }
+
+    [Fact]
+    public async Task UpdateAttribute_ValidTypedListValue_Succeeds()
+    {
+        var existing = new TemplateAttribute("SetPoints")
+        {
+            Id = 1, TemplateId = 1, DataType = DataType.List, ElementDataType = DataType.Int32, Value = """[1, 2]"""
+        };
+        _repoMock.Setup(r => r.GetTemplateAttributeByIdAsync(1, It.IsAny<CancellationToken>())).ReturnsAsync(existing);
+        var template = new Template("Pump") { Id = 1 };
+        _repoMock.Setup(r => r.GetTemplateByIdAsync(1, It.IsAny<CancellationToken>())).ReturnsAsync(template);
+
+        var proposed = new TemplateAttribute("SetPoints")
+        {
+            DataType = DataType.List, ElementDataType = DataType.Int32, Value = """[10, 20, 30]"""
+        };
+        var result = await _service.UpdateAttributeAsync(1, proposed, "admin");
+
+        Assert.True(result.IsSuccess);
+        Assert.Equal("""[10, 20, 30]""", result.Value.Value);
+    }
+
    // ========================================================================
    // WP-3: Alarm Definitions
    // ========================================================================