use min validator on core tags for latest api (#3056)

* - Use minimum validator to generate error for core tags and know when to fail a post
- continue to use fo-dicom validator to generate warnings, but continue to omit warning when is valid string aside from being padded with null values

* add test that fails in main branch on core tag uid validation that passes minimum validator but not fo-dicom validator

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

@@ -73,7 +73,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>());
+        _minimumValidator.DidNotReceive().Validate(Arg.Any<DicomElement>(), true);
     }
 
     [Fact]
@@ -90,7 +90,6 @@ public async Task GivenV2Enabled_WhenNonCoreTagInvalid_ExpectTagValidatedAndErro
         Assert.True(result.InvalidTagErrors.Any());
         Assert.Single(result.InvalidTagErrors);
         Assert.Equal("""DICOM100: (300e,0004) - Content "NotAValidReviewDate" does not validate VR DA: one of the date values does not match the pattern YYYYMMDD""", result.InvalidTagErrors[DicomTag.ReviewDate].Error);
-        _minimumValidator.DidNotReceive().Validate(Arg.Any<DicomElement>());
     }
 
     [Fact]
@@ -188,6 +187,27 @@ public async Task GivenV2Enabled_WhenNonRequiredTagNull_ExpectTagValidatedAndNoE
         Assert.Empty(result.InvalidTagErrors);
     }
 
+    [Fact]
+    public async Task GivenV2Enabled_WhenCoreTagUidWithLeadingZeroes_ExpectTagValidatedAndNoOnlyWarningProduced()
+    {
+        // For Core Tag validation like studyInstanceUid, we expect to use minimum validator which is more lenient
+        // than fo-dicom's validator and allows things like leading zeroes in the UID
+        // We want the validation to *not* produce any errors and therefore not cause any failures
+        // However, we do want to still produce a warning for the end user so they are aware their instance may have issues
+        DicomDataset dicomDataset = Samples.CreateRandomInstanceDataset(
+            validateItems: false,
+            studyInstanceUid: "1.3.6.1.4.1.55648.014924617884283217793330176991551322645.2.1");
+
+        var result = await _dicomDatasetValidator.ValidateAsync(
+            dicomDataset,
+            null,
+            new CancellationToken());
+
+        Assert.Single(result.InvalidTagErrors);
+        Assert.False(result.InvalidTagErrors.Values.First().IsRequiredCoreTag); // we only fail when invalid core tags are present
+        Assert.Contains("does not validate VR UI: components must not have leading zeros", result.InvalidTagErrors.Values.First().Error);
+    }
+
     [Fact]
     public async Task GivenV2Enabled_WhenValidSequenceTag_ExpectTagValidatedAndNoErrorProduced()
     {
@@ -259,7 +279,6 @@ public async Task GivenV2Enabled_WhenValidSequenceTagInvalidInnerNullPaddedValue
 
         Assert.Single(result.InvalidTagErrors);
         Assert.Equal("""DICOM100: (300e,0004) - Content "NotAValidReviewDate" does not validate VR DA: one of the date values does not match the pattern YYYYMMDD""", result.InvalidTagErrors[DicomTag.ReviewDate].Error);
-        _minimumValidator.DidNotReceive().Validate(Arg.Any<DicomElement>());
     }
 
     [Fact]
@@ -287,7 +306,6 @@ public async Task GivenV2Enabled_WhenValidSequenceTagWithInvalidNestedValue_Expe
 
         Assert.Single(result.InvalidTagErrors);
         Assert.Equal("""DICOM100: (300e,0004) - Content "NotAValidReviewDate" does not validate VR DA: one of the date values does not match the pattern YYYYMMDD""", result.InvalidTagErrors[DicomTag.ReviewDate].Error);
-        _minimumValidator.DidNotReceive().Validate(Arg.Any<DicomElement>());
     }
 
     [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);
+        _validator.Validate(dicomElement, false);
     }
 
     public static IEnumerable<object[]> SupportedDicomElements()

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

@@ -395,29 +395,44 @@ public static ValidationWarnings ValidateQueryTag(this DicomDataset dataset, Que
     {
         EnsureArg.IsNotNull(dataset, nameof(dataset));
         EnsureArg.IsNotNull(queryTag, nameof(queryTag));
+
+        return dataset.ValidateDicomTag(queryTag.Tag, minimumValidator);
+    }
+
+    /// <summary>
+    /// Validate dicom tag in Dicom dataset.
+    /// </summary>
+    /// <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)
+    {
+        EnsureArg.IsNotNull(dataset, nameof(dataset));
+        EnsureArg.IsNotNull(dicomTag, nameof(dicomTag));
         EnsureArg.IsNotNull(minimumValidator, nameof(minimumValidator));
-        DicomElement dicomElement = dataset.GetDicomItem<DicomElement>(queryTag.Tag);
+        DicomElement dicomElement = dataset.GetDicomItem<DicomElement>(dicomTag);
 
         ValidationWarnings warning = ValidationWarnings.None;
         if (dicomElement != null)
         {
-            if (dicomElement.ValueRepresentation != queryTag.VR)
+            if (dicomElement.ValueRepresentation != dicomTag.GetDefaultVR())
             {
                 string name = dicomElement.Tag.GetFriendlyName();
                 DicomVR actualVR = dicomElement.ValueRepresentation;
                 throw new ElementValidationException(
                     name,
                     actualVR,
                     ValidationErrorCode.UnexpectedVR,
-                    string.Format(CultureInfo.InvariantCulture, DicomCoreResource.ErrorMessageUnexpectedVR, name, queryTag.VR, actualVR));
+                    string.Format(CultureInfo.InvariantCulture, DicomCoreResource.ErrorMessageUnexpectedVR, name, dicomTag.GetDefaultVR(), actualVR));
             }
 
             if (dicomElement.Count > 1)
             {
                 warning |= ValidationWarnings.IndexedDicomTagHasMultipleValues;
             }
 
-            minimumValidator.Validate(dicomElement);
+            minimumValidator.Validate(dicomElement, withLeniency);
         }
         return warning;
     }

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

@@ -33,7 +33,8 @@ public class StoreDatasetValidator : IStoreDatasetValidator
     private readonly StoreMeter _storeMeter;
     private readonly IDicomRequestContextAccessor _dicomRequestContextAccessor;
 
-    private static readonly HashSet<DicomTag> RequiredCoreTags = new HashSet<DicomTag>()
+
+    public static readonly HashSet<DicomTag> RequiredCoreTags = new HashSet<DicomTag>()
     {
         DicomTag.StudyInstanceUID,
         DicomTag.SeriesInstanceUID,
@@ -203,10 +204,10 @@ private static void ValidateAllItems(
     }
 
     /// <summary>
-    /// Validate all items with leniency applied.
+    /// Validate all items. Generate errors for core tags with leniency applied and generate warnings for all of DS items.
     /// </summary>
     /// <param name="dicomDataset">Dataset to validate</param>
-    /// <param name="validationResultBuilder">Result builder to keep validation results in as validation runs</param>
+    /// <param name="validationResultBuilder">Result builder to keep errors and warnings in as validation runs</param>
     /// <remarks>
     /// We only need to validate SQ and DicomElement types. The only other type under DicomItem
     /// is DicomFragmentSequence, which does not implement validation and can be skipped.
@@ -218,6 +219,59 @@ private async Task ValidateAllItemsWithLeniencyAsync(
         StoreValidationResultBuilder validationResultBuilder)
     {
         IReadOnlyCollection<QueryTag> queryTags = await _queryTagService.GetQueryTagsAsync();
+
+        GenerateErrors(dicomDataset, validationResultBuilder, queryTags);
+
+        GenerateValidationWarnings(dicomDataset, validationResultBuilder, queryTags);
+    }
+
+    /// <summary>
+    ///  Generate errors on result by validating each core tag in dataset
+    /// </summary>
+    /// <remarks>
+    /// isCoreTag is utilized in store service when building a response and to know whether or not to create a failure or
+    /// success response. Anything added here results in a 409 conflict with errors in body.
+    /// </remarks>
+    /// <param name="dicomDataset"></param>
+    /// <param name="validationResultBuilder"></param>
+    /// <param name="queryTags"></param>
+    private void GenerateErrors(DicomDataset dicomDataset, StoreValidationResultBuilder validationResultBuilder,
+        IReadOnlyCollection<QueryTag> queryTags)
+    {
+        foreach (var requiredCoreTag in RequiredCoreTags)
+        {
+            try
+            {
+                // validate with additional leniency
+                var validationWarning =
+                    dicomDataset.ValidateDicomTag(requiredCoreTag, _minimumValidator, withLeniency: true);
+
+                validationResultBuilder.Add(validationWarning, requiredCoreTag);
+            }
+            catch (ElementValidationException ex)
+            {
+                validationResultBuilder.Add(ex, requiredCoreTag, isCoreTag: true);
+                _storeMeter.V2ValidationError.Add(1,
+                    TelemetryDimension(requiredCoreTag, IsIndexableTag(queryTags, requiredCoreTag)));
+            }
+        }
+    }
+
+    /// <summary>
+    /// Generate warnings on the results by validating each item in dataset 
+    /// </summary>
+    /// <remarks>
+    /// isCoreTag is utilized in store service when building a response and to know whether or not to create a failure or
+    /// success response. Anything added here results in a 202 Accepted with warnings in body.
+    /// Since we are providing warnings as informational value, we want to use full fo-dicom validation and no leniency
+    /// or our minimum validators.
+    /// We also need to iterate through each item at a time to capture all validation issues for each and all items in
+    /// the dataset instead of just excepting at first issue as fo-dicom does.
+    /// Do not produce a warning when string invalid only due to null padding. 
+    /// </remarks>
+    private void GenerateValidationWarnings(DicomDataset dicomDataset, StoreValidationResultBuilder validationResultBuilder,
+        IReadOnlyCollection<QueryTag> queryTags)
+    {
         var stack = new Stack<DicomDataset>();
         stack.Push(dicomDataset);
         while (stack.Count > 0)
@@ -241,7 +295,7 @@ private async Task ValidateAllItemsWithLeniencyAsync(
                         if (de.ValueRepresentation.ValueType == typeof(string))
                         {
                             string value = ds.GetString(de.Tag);
-                            ValidateItemWithLeniency(value, de, queryTags);
+                            ValidateStringItemWithLeniency(value, de, queryTags);
                         }
                         else
                         {
@@ -250,15 +304,15 @@ private async Task ValidateAllItemsWithLeniencyAsync(
                     }
                     catch (DicomValidationException ex)
                     {
-                        validationResultBuilder.Add(ex, item.Tag, isCoreTag: RequiredCoreTags.Contains(item.Tag));
+                        validationResultBuilder.Add(ex, item.Tag, isCoreTag: false);
                         _storeMeter.V2ValidationError.Add(1, TelemetryDimension(item, IsIndexableTag(queryTags, item)));
                     }
                 }
             }
         }
     }
 
-    private void ValidateItemWithLeniency(string value, DicomElement de, IReadOnlyCollection<QueryTag> queryTags)
+    private void ValidateStringItemWithLeniency(string value, DicomElement de, IReadOnlyCollection<QueryTag> queryTags)
     {
         if (value != null && value.EndsWith('\0'))
         {
@@ -275,6 +329,11 @@ private static bool IsIndexableTag(IReadOnlyCollection<QueryTag> queryTags, Dico
         return queryTags.Any(x => x.Tag == de.Tag);
     }
 
+    private static bool IsIndexableTag(IReadOnlyCollection<QueryTag> queryTags, DicomTag tag)
+    {
+        return queryTags.Any(x => x.Tag == tag);
+    }
+
     private void ValidateWithoutNullPadding(string value, DicomElement de, IReadOnlyCollection<QueryTag> queryTags)
     {
         de.ValueRepresentation.ValidateString(value.TrimEnd('\0'));
@@ -284,11 +343,14 @@ private void ValidateWithoutNullPadding(string value, DicomElement de, IReadOnly
     }
 
     private static KeyValuePair<string, object>[] TelemetryDimension(DicomItem item, bool isIndexableTag) =>
+        TelemetryDimension(item.Tag, isIndexableTag);
+
+    private static KeyValuePair<string, object>[] TelemetryDimension(DicomTag tag, bool isIndexableTag) =>
         new[]
         {
-            new KeyValuePair<string, object>("TagKeyword", item.Tag.DictionaryEntry.Keyword),
-            new KeyValuePair<string, object>("VR", item.ValueRepresentation.ToString()),
-            new KeyValuePair<string, object>("Tag", item.Tag.ToString()),
+            new KeyValuePair<string, object>("TagKeyword", tag.DictionaryEntry.Keyword),
+            new KeyValuePair<string, object>("VR", tag.GetDefaultVR()),
+            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.InvalidTagErrors.Any(x => x.Value.IsRequiredCoreTag))
+                if (storeValidatorResult.InvalidCoreTagErrorsPresent)
                 {
                     LogFailure(index, dicomDataset, storeValidatorResult);
                     return null;
@@ -264,14 +264,17 @@ private void DropInvalidMetadata(StoreValidationResult storeValidatorResult, Dic
         var identifier = dicomDataset.ToInstanceIdentifier(partition);
         foreach (DicomTag tag in storeValidatorResult.InvalidTagErrors.Keys)
         {
-            // drop invalid metadata
-            dicomDataset.Remove(tag);
-
-            string message = storeValidatorResult.InvalidTagErrors[tag].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.",
-                identifier);
+            if (!StoreDatasetValidator.RequiredCoreTags.Contains(tag))
+            {
+                // drop invalid metadata if not a core tag
+                dicomDataset.Remove(tag);
+
+                string message = storeValidatorResult.InvalidTagErrors[tag].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.",
+                    identifier);
+            }
         }
     }
 

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

@@ -4,6 +4,7 @@
 // -------------------------------------------------------------------------------------------------
 
 using System.Collections.Generic;
+using System.Linq;
 using FellowOakDicom;
 
 namespace Microsoft.Health.Dicom.Core.Features.Store;
@@ -25,4 +26,9 @@ public StoreValidationResult(
     public ValidationWarnings WarningCodes { get; }
 
     public IReadOnlyDictionary<DicomTag, StoreErrorResult> InvalidTagErrors { get; }
+
+    public bool InvalidCoreTagErrorsPresent
+    {
+        get => InvalidTagErrors.Any(invalidTagError => invalidTagError.Value.IsRequiredCoreTag);
+    }
 }

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

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

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

@@ -22,9 +22,13 @@ public ElementMaxLengthValidation(int maxLength)
 
     public int MaxLength { get; }
 
-    public void Validate(DicomElement dicomElement)
+    public void Validate(DicomElement dicomElement, bool withLeniency = false)
     {
         string value = dicomElement.GetFirstValueOrDefault<string>();
+        if (withLeniency)
+        {
+            value = value.TrimEnd('\0');
+        }
         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)
+    public void Validate(DicomElement dicomElement, bool withLeniency = false)
     {
         EnsureArg.IsNotNull(dicomElement, nameof(dicomElement));
         DicomVR vr = dicomElement.ValueRepresentation;
@@ -43,7 +43,7 @@ public void Validate(DicomElement dicomElement)
         }
         if (Validations.TryGetValue(vr, out IElementValidation validationRule))
         {
-            validationRule.Validate(dicomElement);
+            validationRule.Validate(dicomElement, withLeniency);
         }
         else
         {

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

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