min validator bug fix follow up

src/Microsoft.Health.Dicom.Core/Features/Store/StoreService.cs

@@ -264,12 +264,12 @@ private void DropInvalidMetadata(StoreValidationResult storeValidatorResult, Dic
         var identifier = dicomDataset.ToInstanceIdentifier(partition);
         foreach ((DicomTag tag, StoreErrorResult result) in storeValidatorResult.InvalidTagErrors)
         {
-            if (!StoreDatasetValidator.IsCoreTag(error.Key))
+            if (!StoreDatasetValidator.IsCoreTag(tag))
             {
                 // drop invalid metadata if not a core tag
-                dicomDataset.Remove(error.Key);
+                dicomDataset.Remove(tag);
 
-                string message = error.Value.Error;
+                string message = result.Error;
                 _telemetryClient.ForwardLogTrace(
                     $"{message}. This attribute will not be present when retrieving study, series, or instance metadata resources, nor can it be used in searches." +
                     " However, it will still be present when retrieving study, series, or instance resources.",

src/Microsoft.Health.Dicom.Core/Features/Validation/DateValidation.cs

@@ -6,19 +6,18 @@
 using System;
 using System.Globalization;
 using FellowOakDicom;
+using FellowOakDicom.IO.Buffer;
 using Microsoft.Health.Dicom.Core.Exceptions;
 using Microsoft.Health.Dicom.Core.Extensions;
 
 namespace Microsoft.Health.Dicom.Core.Features.Validation;
 
-internal class DateValidation : IElementValidation
+internal class DateValidation : StringElementValidation
 {
     private const string DateFormatDA = "yyyyMMdd";
 
-    public void Validate(DicomElement dicomElement, ValidationLevel validationLevel = ValidationLevel.Strict)
+    protected override void ValidateStringElement(string name, string value, DicomVR vr, IByteBuffer buffer)
     {
-        string name = dicomElement.Tag.GetFriendlyName();
-        string value = BaseStringSanitizer.Sanitize(dicomElement.GetFirstValueOrDefault<string>(), validationLevel);
         if (string.IsNullOrEmpty(value))
         {
             return;
@@ -29,4 +28,10 @@ public void Validate(DicomElement dicomElement, ValidationLevel validationLevel
             throw new ElementValidationException(name, DicomVR.DA, ValidationErrorCode.DateIsInvalid);
         }
     }
+
+    protected override bool GetValue(DicomElement dicomElement, out string value)
+    {
+        value = dicomElement.GetFirstValueOrDefault<string>();
+        return string.IsNullOrEmpty(value);
+    }
 }

src/Microsoft.Health.Dicom.Core/Features/Validation/ElementMaxLengthValidation.cs

@@ -7,12 +7,13 @@
 using System.Globalization;
 using EnsureThat;
 using FellowOakDicom;
+using FellowOakDicom.IO.Buffer;
 using Microsoft.Health.Dicom.Core.Exceptions;
 using Microsoft.Health.Dicom.Core.Extensions;
 
 namespace Microsoft.Health.Dicom.Core.Features.Validation;
 
-internal class ElementMaxLengthValidation : IElementValidation
+internal class ElementMaxLengthValidation : StringElementValidation
 {
     public ElementMaxLengthValidation(int maxLength)
     {
@@ -22,10 +23,9 @@ public ElementMaxLengthValidation(int maxLength)
 
     public int MaxLength { get; }
 
-    public void Validate(DicomElement dicomElement, ValidationLevel validationLevel = ValidationLevel.Strict)
+    protected override void ValidateStringElement(string name, string value, DicomVR vr, IByteBuffer buffer)
     {
-        string value = BaseStringSanitizer.Sanitize(dicomElement.GetFirstValueOrDefault<string>(), validationLevel);
-        Validate(value, MaxLength, dicomElement.Tag.GetFriendlyName(), dicomElement.ValueRepresentation);
+        Validate(value, MaxLength, name, vr);
     }
 
     public static void Validate(string value, int maxLength, string name, DicomVR vr)
@@ -41,4 +41,10 @@ public static void Validate(string value, int maxLength, string name, DicomVR vr
                 string.Format(CultureInfo.CurrentCulture, DicomCoreResource.ErrorMessageExceedMaxLength, maxLength));
         }
     }
+
+    protected override bool GetValue(DicomElement dicomElement, out string value)
+    {
+        value = dicomElement.GetFirstValueOrDefault<string>();
+        return string.IsNullOrEmpty(value);
+    }
 }

src/Microsoft.Health.Dicom.Core/Features/Validation/ElementRequiredLengthValidation.cs

@@ -3,6 +3,7 @@
 // Licensed under the MIT License (MIT). See LICENSE in the repo root for license information.
 // -------------------------------------------------------------------------------------------------
 
+using System;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Globalization;
@@ -13,7 +14,7 @@
 
 namespace Microsoft.Health.Dicom.Core.Features.Validation;
 
-internal class ElementRequiredLengthValidation : IElementValidation
+internal class ElementRequiredLengthValidation : StringElementValidation
 {
     private static readonly HashSet<DicomVR> StringVrs = new HashSet<DicomVR>()
     {
@@ -37,17 +38,15 @@ public ElementRequiredLengthValidation(int expectedLength)
         ExpectedLength = expectedLength;
     }
 
-    public void Validate(DicomElement dicomElement, ValidationLevel validationLevel = ValidationLevel.Strict)
+    protected override void ValidateStringElement(string name, string value, DicomVR vr, IByteBuffer buffer)
     {
-        DicomVR vr = dicomElement.ValueRepresentation;
-        if (TryGetAsString(dicomElement, out string value))
+        if (!String.IsNullOrEmpty(value))
         {
-            value = BaseStringSanitizer.Sanitize(value, validationLevel);
-            ValidateStringLength(vr, dicomElement.Tag.GetFriendlyName(), value);
+            ValidateStringLength(vr, name, value);
         }
         else
         {
-            ValidateByteBufferLength(vr, dicomElement.Tag.GetFriendlyName(), dicomElement.Buffer);
+            ValidateByteBufferLength(vr, name, buffer);
         }
     }
 
@@ -64,7 +63,7 @@ private void ValidateByteBufferLength(DicomVR dicomVR, string name, IByteBuffer
         }
     }
 
-    private static bool TryGetAsString(DicomElement dicomElement, out string value)
+    protected override bool GetValue(DicomElement dicomElement, out string value)
     {
         value = string.Empty;
         if (StringVrs.Contains(dicomElement.ValueRepresentation))

src/Microsoft.Health.Dicom.Core/Features/Validation/EncodedStringElementValidation.cs

@@ -5,36 +5,40 @@
 
 using System;
 using FellowOakDicom;
+using FellowOakDicom.IO.Buffer;
 using Microsoft.Health.Dicom.Core.Exceptions;
 using Microsoft.Health.Dicom.Core.Extensions;
 
 namespace Microsoft.Health.Dicom.Core.Features.Validation;
 
-internal class EncodedStringElementValidation : IElementValidation
+internal class EncodedStringElementValidation : StringElementValidation
 {
-    public void Validate(DicomElement dicomElement, ValidationLevel validationLevel = ValidationLevel.Strict)
+    protected override void ValidateStringElement(string name, string value, DicomVR vr, IByteBuffer buffer)
     {
-        DicomVR vr = dicomElement.ValueRepresentation;
         switch (vr.Code)
         {
             case DicomVRCode.DT:
-                Validate(dicomElement, DicomValidation.ValidateDT, ValidationErrorCode.DateTimeIsInvalid, validationLevel);
+                Validate(name, value, vr, buffer, DicomValidation.ValidateDT, ValidationErrorCode.DateTimeIsInvalid);
                 break;
             case DicomVRCode.IS:
-                Validate(dicomElement, DicomValidation.ValidateIS, ValidationErrorCode.IntegerStringIsInvalid, validationLevel);
+                Validate(name, value, vr, buffer, DicomValidation.ValidateIS, ValidationErrorCode.IntegerStringIsInvalid);
                 break;
             case DicomVRCode.TM:
-                Validate(dicomElement, DicomValidation.ValidateTM, ValidationErrorCode.TimeIsInvalid, validationLevel);
+                Validate(name, value, vr, buffer, DicomValidation.ValidateTM, ValidationErrorCode.TimeIsInvalid);
                 break;
             default:
-                throw new ArgumentOutOfRangeException(nameof(dicomElement));
+                throw new ArgumentOutOfRangeException(nameof(name));
         };
     }
 
-    private static void Validate(DicomElement element, Action<string> validate, ValidationErrorCode errorCode, ValidationLevel validationLevel = ValidationLevel.Strict)
+    protected override bool GetValue(DicomElement dicomElement, out string value)
     {
-        string value = BaseStringSanitizer.Sanitize(element.GetFirstValueOrDefault<string>(), validationLevel);
+        value = dicomElement.GetFirstValueOrDefault<string>();
+        return string.IsNullOrEmpty(value);
+    }
 
+    private static void Validate(string name, string value, DicomVR vr, IByteBuffer buffer, Action<string> validate, ValidationErrorCode errorCode)
+    {
         if (string.IsNullOrEmpty(value))
         {
             return;
@@ -46,7 +50,7 @@ private static void Validate(DicomElement element, Action<string> validate, Vali
         }
         catch (DicomValidationException)
         {
-            throw new ElementValidationException(element.Tag.GetFriendlyName(), element.ValueRepresentation, errorCode);
+            throw new ElementValidationException(name, vr, errorCode);
         }
     }
 }

src/Microsoft.Health.Dicom.Core/Features/Validation/LongStringValidation.cs

@@ -5,6 +5,7 @@
 
 using System;
 using FellowOakDicom;
+using FellowOakDicom.IO.Buffer;
 using Microsoft.Health.Dicom.Core.Exceptions;
 using Microsoft.Health.Dicom.Core.Extensions;
 
@@ -13,12 +14,10 @@ namespace Microsoft.Health.Dicom.Core.Features.Validation;
 /// <summary>
 /// Validate Dicom VR LO 
 /// </summary>
-internal class LongStringValidation : IElementValidation
+internal class LongStringValidation : StringElementValidation
 {
-    public void Validate(DicomElement dicomElement, ValidationLevel validationLevel = ValidationLevel.Strict)
+    protected override void ValidateStringElement(string name, string value, DicomVR vr, IByteBuffer buffer)
     {
-        string value = BaseStringSanitizer.Sanitize(dicomElement.GetFirstValueOrDefault<string>(), validationLevel);
-        string name = dicomElement.Tag.GetFriendlyName();
         Validate(value, name);
     }
 
@@ -36,5 +35,11 @@ public static void Validate(string value, string name)
             throw new ElementValidationException(name, DicomVR.LO, ValidationErrorCode.InvalidCharacters);
         }
     }
+
+    protected override bool GetValue(DicomElement dicomElement, out string value)
+    {
+        value = dicomElement.GetFirstValueOrDefault<string>();
+        return string.IsNullOrEmpty(value);
+    }
 }
 

src/Microsoft.Health.Dicom.Core/Features/Validation/PersonNameValidation.cs

@@ -5,18 +5,16 @@
 
 using System.Linq;
 using FellowOakDicom;
+using FellowOakDicom.IO.Buffer;
 using Microsoft.Health.Dicom.Core.Exceptions;
 using Microsoft.Health.Dicom.Core.Extensions;
 
 namespace Microsoft.Health.Dicom.Core.Features.Validation;
 
-internal class PersonNameValidation : IElementValidation
+internal class PersonNameValidation : StringElementValidation
 {
-    public void Validate(DicomElement dicomElement, ValidationLevel validationLevel = ValidationLevel.Default)
+    protected override void ValidateStringElement(string name, string value, DicomVR vr, IByteBuffer buffer)
     {
-        string name = dicomElement.Tag.GetFriendlyName();
-        DicomVR vr = dicomElement.ValueRepresentation;
-        string value = BaseStringSanitizer.Sanitize(dicomElement.GetFirstValueOrDefault<string>(), validationLevel);
         if (string.IsNullOrEmpty(value))
         {
             // empty values allowed
@@ -33,7 +31,7 @@ public void Validate(DicomElement dicomElement, ValidationLevel validationLevel
         {
             try
             {
-                ElementMaxLengthValidation.Validate(group, 64, name, dicomElement.ValueRepresentation);
+                ElementMaxLengthValidation.Validate(group, 64, name, vr);
             }
             catch (ElementValidationException ex) when (ex.ErrorCode == ValidationErrorCode.ExceedMaxLength)
             {
@@ -52,4 +50,10 @@ public void Validate(DicomElement dicomElement, ValidationLevel validationLevel
             throw new ElementValidationException(name, DicomVR.PN, ValidationErrorCode.PersonNameExceedMaxComponents);
         }
     }
+
+    protected override bool GetValue(DicomElement dicomElement, out string value)
+    {
+        value = dicomElement.GetFirstValueOrDefault<string>();
+        return string.IsNullOrEmpty(value);
+    }
 }

src/Microsoft.Health.Dicom.Core/Features/Validation/StringElementValidation.cs

@@ -0,0 +1,31 @@
+// -------------------------------------------------------------------------------------------------
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information.
+// -------------------------------------------------------------------------------------------------
+
+using EnsureThat;
+using FellowOakDicom;
+using FellowOakDicom.IO.Buffer;
+using Microsoft.Health.Dicom.Core.Extensions;
+
+namespace Microsoft.Health.Dicom.Core.Features.Validation;
+
+
+internal abstract class StringElementValidation : IElementValidation
+{
+    public void Validate(DicomElement dicomElement, ValidationLevel validationLevel = ValidationLevel.Default)
+    {
+        EnsureArg.IsNotNull(dicomElement, nameof(dicomElement));
+
+        string name = dicomElement.Tag.GetFriendlyName();
+        GetValue(dicomElement, out string value);
+        if (!string.IsNullOrEmpty(value) && validationLevel == ValidationLevel.Default)
+            value = value.TrimEnd('\0');
+
+        ValidateStringElement(name, value, dicomElement.ValueRepresentation, dicomElement.Buffer);
+    }
+
+    protected abstract void ValidateStringElement(string name, string value, DicomVR vr, IByteBuffer buffer);
+
+    protected abstract bool GetValue(DicomElement dicomElement, out string value);
+}

src/Microsoft.Health.Dicom.Core/Features/Validation/UidValidation.cs

@@ -5,19 +5,18 @@
 
 using System.Text.RegularExpressions;
 using FellowOakDicom;
+using FellowOakDicom.IO.Buffer;
 using Microsoft.Health.Dicom.Core.Exceptions;
 using Microsoft.Health.Dicom.Core.Extensions;
 
 namespace Microsoft.Health.Dicom.Core.Features.Validation;
 
-internal class UidValidation : IElementValidation
+internal class UidValidation : StringElementValidation
 {
     private static readonly Regex ValidIdentifierCharactersFormat = new Regex("^[0-9\\.]*[0-9]$", RegexOptions.Compiled);
 
-    public void Validate(DicomElement dicomElement, ValidationLevel validationLevel = ValidationLevel.Strict)
+    protected override void ValidateStringElement(string name, string value, DicomVR vr, IByteBuffer buffer)
     {
-        string value = BaseStringSanitizer.Sanitize(dicomElement.GetFirstValueOrDefault<string>(), validationLevel);
-        string name = dicomElement.Tag.GetFriendlyName();
         Validate(value, name, allowEmpty: true);
     }
 
@@ -51,4 +50,10 @@ public static void Validate(string value, string name, bool allowEmpty = false)
             throw new InvalidIdentifierException(name);
         }
     }
+
+    protected override bool GetValue(DicomElement dicomElement, out string value)
+    {
+        value = dicomElement.GetFirstValueOrDefault<string>();
+        return string.IsNullOrEmpty(value);
+    }
 }

src/Microsoft.Health.Dicom.Core/Features/Validation/ValidationLevel.cs

@@ -15,6 +15,7 @@ public enum ValidationLevel
     /// perform all validation after.
     /// </summary>
     Default,
+
     /// <summary>
     /// Strict validation currently fails when a string value has null padding
     /// </summary>