min validator bug fix follow up

src/Microsoft.Health.Dicom.Core.UnitTests/Features/Store/DicomStoreServiceTests.cs

@@ -126,7 +126,8 @@ private static IStoreDatasetValidator CreateStoreDatasetValidatorWithDropDataEna
             new ElementMinimumValidator(),
             queryTagService,
             storeMeter,
-            contextAccessor);
+            contextAccessor,
+            NullLogger<StoreDatasetValidator>.Instance);
         return validator;
     }
 

src/Microsoft.Health.Dicom.Core.UnitTests/Features/Store/StoreDatasetValidatorTestsV1.cs

@@ -9,6 +9,7 @@
 using System.Threading;
 using System.Threading.Tasks;
 using FellowOakDicom;
+using Microsoft.Extensions.Logging.Abstractions;
 using Microsoft.Extensions.Options;
 using Microsoft.Health.Dicom.Core.Configs;
 using Microsoft.Health.Dicom.Core.Features.Context;
@@ -44,7 +45,7 @@ public StoreDatasetValidatorTestsV1()
         _queryTags = new List<QueryTag>(QueryTagService.CoreQueryTags);
         _queryTagService.GetQueryTagsAsync(Arg.Any<CancellationToken>()).Returns(_queryTags);
         _storeMeter = new StoreMeter();
-        _dicomDatasetValidator = new StoreDatasetValidator(featureConfiguration, minValidator, _queryTagService, _storeMeter, _dicomRequestContextAccessor);
+        _dicomDatasetValidator = new StoreDatasetValidator(featureConfiguration, minValidator, _queryTagService, _storeMeter, _dicomRequestContextAccessor, NullLogger<StoreDatasetValidator>.Instance);
     }
 
     [Fact]
@@ -67,7 +68,8 @@ public async Task GivenFullValidation_WhenPatientIDInvalid_ExpectErrorProduced()
             minimumValidator,
             _queryTagService,
             _storeMeter,
-            _dicomRequestContextAccessor);
+            _dicomRequestContextAccessor,
+            NullLogger<StoreDatasetValidator>.Instance);
 
         var result = await dicomDatasetValidator.ValidateAsync(
             dicomDataset,
@@ -99,7 +101,8 @@ public async Task GivenPartialValidation_WhenPatientIDInvalid_ExpectTagValidated
             minimumValidator,
             _queryTagService,
             _storeMeter,
-            _dicomRequestContextAccessor);
+            _dicomRequestContextAccessor,
+            NullLogger<StoreDatasetValidator>.Instance);
 
         DicomDataset dicomDataset = Samples.CreateRandomInstanceDataset(
             validateItems: false,
@@ -128,7 +131,7 @@ public async Task GivenDicomTagWithDifferentVR_WhenValidated_ThenShouldReturnInv
         _queryTags.Clear();
         _queryTags.Add(new QueryTag(tag.BuildExtendedQueryTagStoreEntry()));
         IElementMinimumValidator validator = Substitute.For<IElementMinimumValidator>();
-        _dicomDatasetValidator = new StoreDatasetValidator(featureConfiguration, validator, _queryTagService, _storeMeter, _dicomRequestContextAccessor);
+        _dicomDatasetValidator = new StoreDatasetValidator(featureConfiguration, validator, _queryTagService, _storeMeter, _dicomRequestContextAccessor, NullLogger<StoreDatasetValidator>.Instance);
 
         var result = await _dicomDatasetValidator.ValidateAsync(_dicomDataset, requiredStudyInstanceUid: null);
 
@@ -308,7 +311,7 @@ public async Task GivenDatasetWithInvalidVrValue_WhenValidatingWithFullValidatio
         });
         var minValidator = new ElementMinimumValidator();
 
-        _dicomDatasetValidator = new StoreDatasetValidator(featureConfiguration, minValidator, _queryTagService, _storeMeter, _dicomRequestContextAccessor);
+        _dicomDatasetValidator = new StoreDatasetValidator(featureConfiguration, minValidator, _queryTagService, _storeMeter, _dicomRequestContextAccessor, NullLogger<StoreDatasetValidator>.Instance);
 
         // LO VR, invalid characters
         _dicomDataset.Add(DicomTag.SeriesDescription, "CT1 abdomen\u0000");

src/Microsoft.Health.Dicom.Core.UnitTests/Features/Store/StoreDatasetValidatorTestsV2.cs

@@ -8,6 +8,7 @@
 using System.Threading;
 using System.Threading.Tasks;
 using FellowOakDicom;
+using Microsoft.Extensions.Logging.Abstractions;
 using Microsoft.Extensions.Options;
 using Microsoft.Health.Dicom.Core.Configs;
 using Microsoft.Health.Dicom.Core.Features.Context;
@@ -43,7 +44,7 @@ public StoreDatasetValidatorTestsV2()
         _storeMeter = new StoreMeter();
         _dicomRequestContextV2.Version.Returns(2);
         _dicomRequestContextAccessorV2.RequestContext.Returns(_dicomRequestContextV2);
-        _dicomDatasetValidator = new StoreDatasetValidator(featureConfiguration, _minimumValidator, _queryTagService, _storeMeter, _dicomRequestContextAccessorV2);
+        _dicomDatasetValidator = new StoreDatasetValidator(featureConfiguration, _minimumValidator, _queryTagService, _storeMeter, _dicomRequestContextAccessorV2, NullLogger<StoreDatasetValidator>.Instance);
     }
 
     [Fact]
@@ -62,7 +63,8 @@ public async Task GivenFullValidation_WhenPatientIDInvalid_ExpectErrorProduced()
             _minimumValidator,
             _queryTagService,
             _storeMeter,
-            _dicomRequestContextAccessorV2);
+            _dicomRequestContextAccessorV2,
+            NullLogger<StoreDatasetValidator>.Instance);
 
         var result = await validator.ValidateAsync(
             dicomDataset,
@@ -73,7 +75,7 @@ public async Task GivenFullValidation_WhenPatientIDInvalid_ExpectErrorProduced()
             """does not validate VR LO: value contains invalid character""",
             result.InvalidTagErrors[DicomTag.PatientID].Error);
 
-        _minimumValidator.DidNotReceive().Validate(Arg.Any<DicomElement>(), true);
+        _minimumValidator.DidNotReceive().Validate(Arg.Any<DicomElement>(), ValidationLevel.Default);
     }
 
     [Fact]

src/Microsoft.Health.Dicom.Core.UnitTests/Features/Validation/ElementMinimumValidatorTests.cs

@@ -25,7 +25,7 @@ public ElementMinimumValidatorTests()
     [MemberData(nameof(SupportedDicomElements))]
     public void GivenSupportedVR_WhenValidating_ThenShouldPass(DicomElement dicomElement)
     {
-        _validator.Validate(dicomElement, false);
+        _validator.Validate(dicomElement);
     }
 
     public static IEnumerable<object[]> SupportedDicomElements()

src/Microsoft.Health.Dicom.Core/Extensions/DicomDatasetExtensions.cs

@@ -405,8 +405,8 @@ public static ValidationWarnings ValidateQueryTag(this DicomDataset dataset, Que
     /// <param name="dataset">The dicom dataset.</param>
     /// <param name="dicomTag">The dicom tag being validated.</param>
     /// <param name="minimumValidator">The minimum validator.</param>
-    /// <param name="withLeniency">Whether or not to validate with additional leniency</param>
-    public static ValidationWarnings ValidateDicomTag(this DicomDataset dataset, DicomTag dicomTag, IElementMinimumValidator minimumValidator, bool withLeniency = false)
+    /// <param name="validationLevel">Style of validation to enforce on running rules</param>
+    public static ValidationWarnings ValidateDicomTag(this DicomDataset dataset, DicomTag dicomTag, IElementMinimumValidator minimumValidator, ValidationLevel validationLevel = ValidationLevel.Strict)
     {
         EnsureArg.IsNotNull(dataset, nameof(dataset));
         EnsureArg.IsNotNull(dicomTag, nameof(dicomTag));
@@ -432,7 +432,7 @@ public static ValidationWarnings ValidateDicomTag(this DicomDataset dataset, Dic
                 warning |= ValidationWarnings.IndexedDicomTagHasMultipleValues;
             }
 
-            minimumValidator.Validate(dicomElement, withLeniency);
+            minimumValidator.Validate(dicomElement, validationLevel);
         }
         return warning;
     }

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

@@ -11,6 +11,7 @@
 using System.Threading.Tasks;
 using EnsureThat;
 using FellowOakDicom;
+using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 using Microsoft.Health.Dicom.Core.Configs;
 using Microsoft.Health.Dicom.Core.Exceptions;
@@ -32,9 +33,9 @@ public class StoreDatasetValidator : IStoreDatasetValidator
     private readonly IQueryTagService _queryTagService;
     private readonly StoreMeter _storeMeter;
     private readonly IDicomRequestContextAccessor _dicomRequestContextAccessor;
+    private readonly ILogger _logger;
 
-
-    public static readonly HashSet<DicomTag> RequiredCoreTags = new HashSet<DicomTag>()
+    private static readonly IReadOnlySet<DicomTag> RequiredCoreTags = new HashSet<DicomTag>()
     {
         DicomTag.StudyInstanceUID,
         DicomTag.SeriesInstanceUID,
@@ -48,7 +49,8 @@ public StoreDatasetValidator(
         IElementMinimumValidator minimumValidator,
         IQueryTagService queryTagService,
         StoreMeter storeMeter,
-        IDicomRequestContextAccessor dicomRequestContextAccessor)
+        IDicomRequestContextAccessor dicomRequestContextAccessor,
+        ILogger<StoreDatasetValidator> logger)
     {
         EnsureArg.IsNotNull(featureConfiguration?.Value, nameof(featureConfiguration));
         EnsureArg.IsNotNull(minimumValidator, nameof(minimumValidator));
@@ -60,6 +62,7 @@ public StoreDatasetValidator(
         _minimumValidator = minimumValidator;
         _queryTagService = queryTagService;
         _storeMeter = EnsureArg.IsNotNull(storeMeter, nameof(storeMeter));
+        _logger = EnsureArg.IsNotNull(logger, nameof(logger));
     }
 
     /// <inheritdoc/>
@@ -234,7 +237,7 @@ private async Task ValidateAllItemsWithLeniencyAsync(
     /// </remarks>
     /// <param name="dicomDataset"></param>
     /// <param name="validationResultBuilder"></param>
-    /// <param name="queryTags"></param>
+    /// <param name="queryTags">Queryable dicom tags</param>
     private void GenerateErrors(DicomDataset dicomDataset, StoreValidationResultBuilder validationResultBuilder,
         IReadOnlyCollection<QueryTag> queryTags)
     {
@@ -244,13 +247,14 @@ private void GenerateErrors(DicomDataset dicomDataset, StoreValidationResultBuil
             {
                 // validate with additional leniency
                 var validationWarning =
-                    dicomDataset.ValidateDicomTag(requiredCoreTag, _minimumValidator, withLeniency: true);
+                    dicomDataset.ValidateDicomTag(requiredCoreTag, _minimumValidator, validationLevel: ValidationLevel.Default);
 
                 validationResultBuilder.Add(validationWarning, requiredCoreTag);
             }
             catch (ElementValidationException ex)
             {
                 validationResultBuilder.Add(ex, requiredCoreTag, isCoreTag: true);
+                _logger.LogInformation("Dicom instance validation failed with error on required core tag {Tag} with id {Id}", requiredCoreTag.DictionaryEntry.Keyword, requiredCoreTag);
                 _storeMeter.V2ValidationError.Add(1,
                     TelemetryDimension(requiredCoreTag, IsIndexableTag(queryTags, requiredCoreTag)));
             }
@@ -305,6 +309,14 @@ private void GenerateValidationWarnings(DicomDataset dicomDataset, StoreValidati
                     catch (DicomValidationException ex)
                     {
                         validationResultBuilder.Add(ex, item.Tag, isCoreTag: false);
+                        if (item.Tag.IsPrivate)
+                        {
+                            _logger.LogInformation("Dicom instance validation succeeded but with warning on a private tag");
+                        }
+                        else
+                        {
+                            _logger.LogInformation("Dicom instance validation succeeded but with warning on non-private tag {Tag} with id {Id}, where tag is indexable: {Indexable} and VR is {VR}", item.Tag.DictionaryEntry.Keyword, item.Tag, IsIndexableTag(queryTags, item), item.ValueRepresentation);
+                        }
                         _storeMeter.V2ValidationError.Add(1, TelemetryDimension(item, IsIndexableTag(queryTags, item)));
                     }
                 }
@@ -326,14 +338,24 @@ private void ValidateStringItemWithLeniency(string value, DicomElement de, IRead
 
     private static bool IsIndexableTag(IReadOnlyCollection<QueryTag> queryTags, DicomItem de)
     {
-        return queryTags.Any(x => x.Tag == de.Tag);
+        return IsIndexableTag(queryTags, de.Tag);
     }
 
     private static bool IsIndexableTag(IReadOnlyCollection<QueryTag> queryTags, DicomTag tag)
     {
         return queryTags.Any(x => x.Tag == tag);
     }
 
+    /// <summary>
+    /// Check if a tag is a required core tag
+    /// </summary>
+    /// <param name="tag">tag to check if it is required</param>
+    /// <returns>whether or not tag is required</returns>
+    public static bool IsCoreTag(DicomTag tag)
+    {
+        return RequiredCoreTags.Contains(tag);
+    }
+
     private void ValidateWithoutNullPadding(string value, DicomElement de, IReadOnlyCollection<QueryTag> queryTags)
     {
         de.ValueRepresentation.ValidateString(value.TrimEnd('\0'));
@@ -349,7 +371,7 @@ private static KeyValuePair<string, object>[] TelemetryDimension(DicomTag tag, b
         new[]
         {
             new KeyValuePair<string, object>("TagKeyword", tag.DictionaryEntry.Keyword),
-            new KeyValuePair<string, object>("VR", tag.GetDefaultVR()),
+            new KeyValuePair<string, object>("VR", tag.GetDefaultVR().ToString()),
             new KeyValuePair<string, object>("Tag", tag.ToString()),
             new KeyValuePair<string, object>("IsIndexable", isIndexableTag.ToString())
         };

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

@@ -167,7 +167,7 @@ public async Task<StoreResponse> ProcessAsync(
             if (dropMetadata)
             {
                 // if any core tag errors occured, log as failure and return. otherwise we drop the invalid tag
-                if (storeValidatorResult.InvalidCoreTagErrorsPresent)
+                if (storeValidatorResult.HasCoreTagError)
                 {
                     LogFailure(index, dicomDataset, storeValidatorResult);
                     return null;
@@ -262,14 +262,14 @@ private void LogFailure(int index, DicomDataset dicomDataset, StoreValidationRes
     private void DropInvalidMetadata(StoreValidationResult storeValidatorResult, DicomDataset dicomDataset, Partition partition)
     {
         var identifier = dicomDataset.ToInstanceIdentifier(partition);
-        foreach (DicomTag tag in storeValidatorResult.InvalidTagErrors.Keys)
+        foreach ((DicomTag tag, StoreErrorResult result) in storeValidatorResult.InvalidTagErrors)
         {
-            if (!StoreDatasetValidator.RequiredCoreTags.Contains(tag))
+            if (!StoreDatasetValidator.IsCoreTag(error.Key))
             {
                 // drop invalid metadata if not a core tag
-                dicomDataset.Remove(tag);
+                dicomDataset.Remove(error.Key);
 
-                string message = storeValidatorResult.InvalidTagErrors[tag].Error;
+                string message = error.Value.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/Store/StoreValidationResult.cs

@@ -27,8 +27,5 @@ public StoreValidationResult(
 
     public IReadOnlyDictionary<DicomTag, StoreErrorResult> InvalidTagErrors { get; }
 
-    public bool InvalidCoreTagErrorsPresent
-    {
-        get => InvalidTagErrors.Any(invalidTagError => invalidTagError.Value.IsRequiredCoreTag);
-    }
+    public bool HasCoreTagError => InvalidTagErrors.Any(invalidTagError => invalidTagError.Value.IsRequiredCoreTag);
 }

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

@@ -0,0 +1,27 @@
+// -------------------------------------------------------------------------------------------------
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information.
+// -------------------------------------------------------------------------------------------------
+
+namespace Microsoft.Health.Dicom.Core.Features.Validation;
+
+/// <summary>
+/// Use for base string sanitization shared across all string related validators
+/// </summary>
+public static class BaseStringSanitizer
+{
+    /// <summary>
+    /// Sanitizes string value to prep for validation
+    /// </summary>
+    public static string Sanitize(string value, ValidationLevel validationLevel)
+    {
+        return validationLevel == ValidationLevel.Default
+            ? string.IsNullOrEmpty(value) ? value : RemoveNullPadding(value)
+            : value;
+    }
+
+    private static string RemoveNullPadding(string value)
+    {
+        return value.TrimEnd('\0');
+    }
+}

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

@@ -15,14 +15,10 @@ internal class DateValidation : IElementValidation
 {
     private const string DateFormatDA = "yyyyMMdd";
 
-    public void Validate(DicomElement dicomElement, bool withLeniency = false)
+    public void Validate(DicomElement dicomElement, ValidationLevel validationLevel = ValidationLevel.Strict)
     {
-        string value = dicomElement.GetFirstValueOrDefault<string>();
         string name = dicomElement.Tag.GetFriendlyName();
-        if (withLeniency)
-        {
-            value = value.TrimEnd('\0');
-        }
+        string value = BaseStringSanitizer.Sanitize(dicomElement.GetFirstValueOrDefault<string>(), validationLevel);
         if (string.IsNullOrEmpty(value))
         {
             return;

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

@@ -22,13 +22,9 @@ public ElementMaxLengthValidation(int maxLength)
 
     public int MaxLength { get; }
 
-    public void Validate(DicomElement dicomElement, bool withLeniency = false)
+    public void Validate(DicomElement dicomElement, ValidationLevel validationLevel = ValidationLevel.Strict)
     {
-        string value = dicomElement.GetFirstValueOrDefault<string>();
-        if (withLeniency)
-        {
-            value = value.TrimEnd('\0');
-        }
+        string value = BaseStringSanitizer.Sanitize(dicomElement.GetFirstValueOrDefault<string>(), validationLevel);
         Validate(value, MaxLength, dicomElement.Tag.GetFriendlyName(), dicomElement.ValueRepresentation);
     }
 

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

@@ -33,7 +33,7 @@ public class ElementMinimumValidator : IElementMinimumValidator
         { DicomVR.US, new ElementRequiredLengthValidation(2) },
     };
 
-    public void Validate(DicomElement dicomElement, bool withLeniency = false)
+    public void Validate(DicomElement dicomElement, ValidationLevel validationLevel = ValidationLevel.Strict)
     {
         EnsureArg.IsNotNull(dicomElement, nameof(dicomElement));
         DicomVR vr = dicomElement.ValueRepresentation;
@@ -43,7 +43,7 @@ public void Validate(DicomElement dicomElement, bool withLeniency = false)
         }
         if (Validations.TryGetValue(vr, out IElementValidation validationRule))
         {
-            validationRule.Validate(dicomElement, withLeniency);
+            validationRule.Validate(dicomElement, validationLevel);
         }
         else
         {

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

@@ -37,15 +37,12 @@ public ElementRequiredLengthValidation(int expectedLength)
         ExpectedLength = expectedLength;
     }
 
-    public void Validate(DicomElement dicomElement, bool withLeniency = false)
+    public void Validate(DicomElement dicomElement, ValidationLevel validationLevel = ValidationLevel.Strict)
     {
         DicomVR vr = dicomElement.ValueRepresentation;
         if (TryGetAsString(dicomElement, out string value))
         {
-            if (withLeniency)
-            {
-                value = value.TrimEnd('\0');
-            }
+            value = BaseStringSanitizer.Sanitize(value, validationLevel);
             ValidateStringLength(vr, dicomElement.Tag.GetFriendlyName(), value);
         }
         else