diff --git a/src/Microsoft.Health.Dicom.Core.UnitTests/Features/Store/StoreDatasetValidatorTestsV2.cs b/src/Microsoft.Health.Dicom.Core.UnitTests/Features/Store/StoreDatasetValidatorTestsV2.cs index c28c548e9d..417871d9de 100644 --- a/src/Microsoft.Health.Dicom.Core.UnitTests/Features/Store/StoreDatasetValidatorTestsV2.cs +++ b/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()); + _minimumValidator.DidNotReceive().Validate(Arg.Any(), 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()); } [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()); } [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()); } [Fact] diff --git a/src/Microsoft.Health.Dicom.Core.UnitTests/Features/Validation/ElementMinimumValidatorTests.cs b/src/Microsoft.Health.Dicom.Core.UnitTests/Features/Validation/ElementMinimumValidatorTests.cs index 7197914413..29baeacdf5 100644 --- a/src/Microsoft.Health.Dicom.Core.UnitTests/Features/Validation/ElementMinimumValidatorTests.cs +++ b/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 SupportedDicomElements() diff --git a/src/Microsoft.Health.Dicom.Core/Extensions/DicomDatasetExtensions.cs b/src/Microsoft.Health.Dicom.Core/Extensions/DicomDatasetExtensions.cs index 628332d72b..722e0278da 100644 --- a/src/Microsoft.Health.Dicom.Core/Extensions/DicomDatasetExtensions.cs +++ b/src/Microsoft.Health.Dicom.Core/Extensions/DicomDatasetExtensions.cs @@ -395,13 +395,28 @@ public static ValidationWarnings ValidateQueryTag(this DicomDataset dataset, Que { EnsureArg.IsNotNull(dataset, nameof(dataset)); EnsureArg.IsNotNull(queryTag, nameof(queryTag)); + + return dataset.ValidateDicomTag(queryTag.Tag, minimumValidator); + } + + /// + /// Validate dicom tag in Dicom dataset. + /// + /// The dicom dataset. + /// The dicom tag being validated. + /// The minimum validator. + /// Whether or not to validate with additional leniency + 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(queryTag.Tag); + DicomElement dicomElement = dataset.GetDicomItem(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; @@ -409,7 +424,7 @@ public static ValidationWarnings ValidateQueryTag(this DicomDataset dataset, Que 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) @@ -417,7 +432,7 @@ public static ValidationWarnings ValidateQueryTag(this DicomDataset dataset, Que warning |= ValidationWarnings.IndexedDicomTagHasMultipleValues; } - minimumValidator.Validate(dicomElement); + minimumValidator.Validate(dicomElement, withLeniency); } return warning; } diff --git a/src/Microsoft.Health.Dicom.Core/Features/Store/StoreDatasetValidator.cs b/src/Microsoft.Health.Dicom.Core/Features/Store/StoreDatasetValidator.cs index 55bb7357cc..ffb5ef260b 100644 --- a/src/Microsoft.Health.Dicom.Core/Features/Store/StoreDatasetValidator.cs +++ b/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 RequiredCoreTags = new HashSet() + + public static readonly HashSet RequiredCoreTags = new HashSet() { DicomTag.StudyInstanceUID, DicomTag.SeriesInstanceUID, @@ -203,10 +204,10 @@ private static void ValidateAllItems( } /// - /// 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. /// /// Dataset to validate - /// Result builder to keep validation results in as validation runs + /// Result builder to keep errors and warnings in as validation runs /// /// 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 queryTags = await _queryTagService.GetQueryTagsAsync(); + + GenerateErrors(dicomDataset, validationResultBuilder, queryTags); + + GenerateValidationWarnings(dicomDataset, validationResultBuilder, queryTags); + } + + /// + /// Generate errors on result by validating each core tag in dataset + /// + /// + /// 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. + /// + /// + /// + /// + private void GenerateErrors(DicomDataset dicomDataset, StoreValidationResultBuilder validationResultBuilder, + IReadOnlyCollection 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))); + } + } + } + + /// + /// Generate warnings on the results by validating each item in dataset + /// + /// + /// 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. + /// + private void GenerateValidationWarnings(DicomDataset dicomDataset, StoreValidationResultBuilder validationResultBuilder, + IReadOnlyCollection queryTags) + { var stack = new Stack(); 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,7 +304,7 @@ 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))); } } @@ -258,7 +312,7 @@ private async Task ValidateAllItemsWithLeniencyAsync( } } - private void ValidateItemWithLeniency(string value, DicomElement de, IReadOnlyCollection queryTags) + private void ValidateStringItemWithLeniency(string value, DicomElement de, IReadOnlyCollection queryTags) { if (value != null && value.EndsWith('\0')) { @@ -275,6 +329,11 @@ private static bool IsIndexableTag(IReadOnlyCollection queryTags, Dico return queryTags.Any(x => x.Tag == de.Tag); } + private static bool IsIndexableTag(IReadOnlyCollection queryTags, DicomTag tag) + { + return queryTags.Any(x => x.Tag == tag); + } + private void ValidateWithoutNullPadding(string value, DicomElement de, IReadOnlyCollection queryTags) { de.ValueRepresentation.ValidateString(value.TrimEnd('\0')); @@ -284,11 +343,14 @@ private void ValidateWithoutNullPadding(string value, DicomElement de, IReadOnly } private static KeyValuePair[] TelemetryDimension(DicomItem item, bool isIndexableTag) => + TelemetryDimension(item.Tag, isIndexableTag); + + private static KeyValuePair[] TelemetryDimension(DicomTag tag, bool isIndexableTag) => new[] { - new KeyValuePair("TagKeyword", item.Tag.DictionaryEntry.Keyword), - new KeyValuePair("VR", item.ValueRepresentation.ToString()), - new KeyValuePair("Tag", item.Tag.ToString()), + new KeyValuePair("TagKeyword", tag.DictionaryEntry.Keyword), + new KeyValuePair("VR", tag.GetDefaultVR()), + new KeyValuePair("Tag", tag.ToString()), new KeyValuePair("IsIndexable", isIndexableTag.ToString()) }; diff --git a/src/Microsoft.Health.Dicom.Core/Features/Store/StoreService.cs b/src/Microsoft.Health.Dicom.Core/Features/Store/StoreService.cs index 4eb12a94cf..75c0d016ae 100644 --- a/src/Microsoft.Health.Dicom.Core/Features/Store/StoreService.cs +++ b/src/Microsoft.Health.Dicom.Core/Features/Store/StoreService.cs @@ -167,7 +167,7 @@ public async Task 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); + } } } diff --git a/src/Microsoft.Health.Dicom.Core/Features/Store/StoreValidationResult.cs b/src/Microsoft.Health.Dicom.Core/Features/Store/StoreValidationResult.cs index 2a46c77f41..4f9e43c719 100644 --- a/src/Microsoft.Health.Dicom.Core/Features/Store/StoreValidationResult.cs +++ b/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 InvalidTagErrors { get; } + + public bool InvalidCoreTagErrorsPresent + { + get => InvalidTagErrors.Any(invalidTagError => invalidTagError.Value.IsRequiredCoreTag); + } } diff --git a/src/Microsoft.Health.Dicom.Core/Features/Validation/DateValidation.cs b/src/Microsoft.Health.Dicom.Core/Features/Validation/DateValidation.cs index 4da397056d..cf7cfa70da 100644 --- a/src/Microsoft.Health.Dicom.Core/Features/Validation/DateValidation.cs +++ b/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 name = dicomElement.Tag.GetFriendlyName(); + if (withLeniency) + { + value = value.TrimEnd('\0'); + } if (string.IsNullOrEmpty(value)) { return; diff --git a/src/Microsoft.Health.Dicom.Core/Features/Validation/ElementMaxLengthValidation.cs b/src/Microsoft.Health.Dicom.Core/Features/Validation/ElementMaxLengthValidation.cs index cd0626d449..b840ae7779 100644 --- a/src/Microsoft.Health.Dicom.Core/Features/Validation/ElementMaxLengthValidation.cs +++ b/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(); + if (withLeniency) + { + value = value.TrimEnd('\0'); + } Validate(value, MaxLength, dicomElement.Tag.GetFriendlyName(), dicomElement.ValueRepresentation); } diff --git a/src/Microsoft.Health.Dicom.Core/Features/Validation/ElementMinimumValidator.cs b/src/Microsoft.Health.Dicom.Core/Features/Validation/ElementMinimumValidator.cs index e76d3d7511..cce1df403d 100644 --- a/src/Microsoft.Health.Dicom.Core/Features/Validation/ElementMinimumValidator.cs +++ b/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 { diff --git a/src/Microsoft.Health.Dicom.Core/Features/Validation/ElementRequiredLengthValidation.cs b/src/Microsoft.Health.Dicom.Core/Features/Validation/ElementRequiredLengthValidation.cs index c824bfb15e..9d7aa74e21 100644 --- a/src/Microsoft.Health.Dicom.Core/Features/Validation/ElementRequiredLengthValidation.cs +++ b/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 diff --git a/src/Microsoft.Health.Dicom.Core/Features/Validation/EncodedStringElementValidation.cs b/src/Microsoft.Health.Dicom.Core/Features/Validation/EncodedStringElementValidation.cs index 262e559951..c817c53de3 100644 --- a/src/Microsoft.Health.Dicom.Core/Features/Validation/EncodedStringElementValidation.cs +++ b/src/Microsoft.Health.Dicom.Core/Features/Validation/EncodedStringElementValidation.cs @@ -12,29 +12,34 @@ namespace Microsoft.Health.Dicom.Core.Features.Validation; internal class EncodedStringElementValidation : IElementValidation { - public void Validate(DicomElement dicomElement) + public void Validate(DicomElement dicomElement, bool withLeniency = false) { DicomVR vr = dicomElement.ValueRepresentation; switch (vr.Code) { case DicomVRCode.DT: - Validate(dicomElement, DicomValidation.ValidateDT, ValidationErrorCode.DateTimeIsInvalid); + Validate(dicomElement, DicomValidation.ValidateDT, ValidationErrorCode.DateTimeIsInvalid, withLeniency); break; case DicomVRCode.IS: - Validate(dicomElement, DicomValidation.ValidateIS, ValidationErrorCode.IntegerStringIsInvalid); + Validate(dicomElement, DicomValidation.ValidateIS, ValidationErrorCode.IntegerStringIsInvalid, withLeniency); break; case DicomVRCode.TM: - Validate(dicomElement, DicomValidation.ValidateTM, ValidationErrorCode.TimeIsInvalid); + Validate(dicomElement, DicomValidation.ValidateTM, ValidationErrorCode.TimeIsInvalid, withLeniency); break; default: throw new ArgumentOutOfRangeException(nameof(dicomElement)); }; } - private static void Validate(DicomElement element, Action validate, ValidationErrorCode errorCode) + private static void Validate(DicomElement element, Action validate, ValidationErrorCode errorCode, bool withLeniency) { string value = element.GetFirstValueOrDefault(); + if (withLeniency) + { + value = value.TrimEnd('\0'); + } + if (string.IsNullOrEmpty(value)) { return; diff --git a/src/Microsoft.Health.Dicom.Core/Features/Validation/IElementMinimumValidator.cs b/src/Microsoft.Health.Dicom.Core/Features/Validation/IElementMinimumValidator.cs index bc12c37735..573d9fc273 100644 --- a/src/Microsoft.Health.Dicom.Core/Features/Validation/IElementMinimumValidator.cs +++ b/src/Microsoft.Health.Dicom.Core/Features/Validation/IElementMinimumValidator.cs @@ -17,6 +17,7 @@ public interface IElementMinimumValidator /// Validate Dicom Element. /// /// The Dicom Element + /// Whether or not to validate with additional leniency /// - void Validate(DicomElement dicomElement); + void Validate(DicomElement dicomElement, bool withLeniency = false); } diff --git a/src/Microsoft.Health.Dicom.Core/Features/Validation/IElementValidation.cs b/src/Microsoft.Health.Dicom.Core/Features/Validation/IElementValidation.cs index 66ad55949c..d7ef92968e 100644 --- a/src/Microsoft.Health.Dicom.Core/Features/Validation/IElementValidation.cs +++ b/src/Microsoft.Health.Dicom.Core/Features/Validation/IElementValidation.cs @@ -17,6 +17,7 @@ internal interface IElementValidation /// Validate DicomElement /// /// The dicom element + /// Validate with additional leniency applied /// - void Validate(DicomElement dicomElement); + void Validate(DicomElement dicomElement, bool withLeniency = false); } diff --git a/src/Microsoft.Health.Dicom.Core/Features/Validation/LongStringValidation.cs b/src/Microsoft.Health.Dicom.Core/Features/Validation/LongStringValidation.cs index 164f473c24..42443def95 100644 --- a/src/Microsoft.Health.Dicom.Core/Features/Validation/LongStringValidation.cs +++ b/src/Microsoft.Health.Dicom.Core/Features/Validation/LongStringValidation.cs @@ -15,9 +15,13 @@ namespace Microsoft.Health.Dicom.Core.Features.Validation; /// internal class LongStringValidation : IElementValidation { - public void Validate(DicomElement dicomElement) + public void Validate(DicomElement dicomElement, bool withLeniency = false) { string value = dicomElement.GetFirstValueOrDefault(); + if (withLeniency) + { + value = value.TrimEnd('\0'); + } string name = dicomElement.Tag.GetFriendlyName(); Validate(value, name); } diff --git a/src/Microsoft.Health.Dicom.Core/Features/Validation/PartitionNameValidator.cs b/src/Microsoft.Health.Dicom.Core/Features/Validation/PartitionNameValidator.cs index 48757d65ca..0dce7945b1 100644 --- a/src/Microsoft.Health.Dicom.Core/Features/Validation/PartitionNameValidator.cs +++ b/src/Microsoft.Health.Dicom.Core/Features/Validation/PartitionNameValidator.cs @@ -12,7 +12,7 @@ public static class PartitionNameValidator { private static readonly Regex ValidIdentifierCharactersFormat = new Regex("^[A-Za-z0-9_.-]*$", RegexOptions.Compiled); - public static void Validate(string value) + public static void Validate(string value, bool withLeniency = false) { if (string.IsNullOrWhiteSpace(value)) { diff --git a/src/Microsoft.Health.Dicom.Core/Features/Validation/PersonNameValidation.cs b/src/Microsoft.Health.Dicom.Core/Features/Validation/PersonNameValidation.cs index a21f143857..152eeba324 100644 --- a/src/Microsoft.Health.Dicom.Core/Features/Validation/PersonNameValidation.cs +++ b/src/Microsoft.Health.Dicom.Core/Features/Validation/PersonNameValidation.cs @@ -12,11 +12,15 @@ namespace Microsoft.Health.Dicom.Core.Features.Validation; internal class PersonNameValidation : IElementValidation { - public void Validate(DicomElement dicomElement) + public void Validate(DicomElement dicomElement, bool withLeniency = false) { string value = dicomElement.GetFirstValueOrDefault(); string name = dicomElement.Tag.GetFriendlyName(); DicomVR vr = dicomElement.ValueRepresentation; + if (withLeniency) + { + value = value.TrimEnd('\0'); + } if (string.IsNullOrEmpty(value)) { // empty values allowed diff --git a/src/Microsoft.Health.Dicom.Core/Features/Validation/UidValidation.cs b/src/Microsoft.Health.Dicom.Core/Features/Validation/UidValidation.cs index fbbbc7a32b..40b9c46072 100644 --- a/src/Microsoft.Health.Dicom.Core/Features/Validation/UidValidation.cs +++ b/src/Microsoft.Health.Dicom.Core/Features/Validation/UidValidation.cs @@ -14,9 +14,13 @@ internal class UidValidation : IElementValidation { private static readonly Regex ValidIdentifierCharactersFormat = new Regex("^[0-9\\.]*[0-9]$", RegexOptions.Compiled); - public void Validate(DicomElement dicomElement) + public void Validate(DicomElement dicomElement, bool withLeniency = false) { string value = dicomElement.GetFirstValueOrDefault(); + if (withLeniency) + { + value = value.TrimEnd('\0'); + } string name = dicomElement.Tag.GetFriendlyName(); Validate(value, name, allowEmpty: true); } diff --git a/test/Microsoft.Health.Dicom.Web.Tests.E2E/Common/DicomInstanceId.cs b/test/Microsoft.Health.Dicom.Web.Tests.E2E/Common/DicomInstanceId.cs index f1bd5e5d78..6965665185 100644 --- a/test/Microsoft.Health.Dicom.Web.Tests.E2E/Common/DicomInstanceId.cs +++ b/test/Microsoft.Health.Dicom.Web.Tests.E2E/Common/DicomInstanceId.cs @@ -29,11 +29,11 @@ public DicomInstanceId(string studyInstanceUid, string seriesInstanceUid, string public string SopInstanceUid { get; } public Partition Partition { get; } - public static DicomInstanceId FromDicomFile(DicomFile dicomFile, Partition partition = null) + public static DicomInstanceId FromDicomFile(DicomFile dicomFile, Partition partition = null, string studyInstanceUid = null) { partition = partition ?? Partition.Default; InstanceIdentifier instanceIdentifier = dicomFile.Dataset.ToInstanceIdentifier(CorePartition.Partition.Default); - return new DicomInstanceId(instanceIdentifier.StudyInstanceUid, instanceIdentifier.SeriesInstanceUid, instanceIdentifier.SopInstanceUid, partition); + return new DicomInstanceId(studyInstanceUid ?? instanceIdentifier.StudyInstanceUid, instanceIdentifier.SeriesInstanceUid, instanceIdentifier.SopInstanceUid, partition); } public override bool Equals(object obj) diff --git a/test/Microsoft.Health.Dicom.Web.Tests.E2E/Common/DicomInstancesManager.cs b/test/Microsoft.Health.Dicom.Web.Tests.E2E/Common/DicomInstancesManager.cs index 1fa56a007b..546311036d 100644 --- a/test/Microsoft.Health.Dicom.Web.Tests.E2E/Common/DicomInstancesManager.cs +++ b/test/Microsoft.Health.Dicom.Web.Tests.E2E/Common/DicomInstancesManager.cs @@ -59,7 +59,7 @@ private string GetPartition(Partition partition) public async Task> StoreAsync(DicomFile dicomFile, string studyInstanceUid = default, Partition partition = null, CancellationToken cancellationToken = default) { EnsureArg.IsNotNull(dicomFile, nameof(dicomFile)); - _instanceIds.Add(DicomInstanceId.FromDicomFile(dicomFile, partition)); + _instanceIds.Add(DicomInstanceId.FromDicomFile(dicomFile, partition, studyInstanceUid)); return await _dicomWebClient.StoreAsync(dicomFile, studyInstanceUid, partition?.Name, cancellationToken); } @@ -79,7 +79,7 @@ public async Task> StoreAsync(IEnumerable response = await _instancesManager.StoreAsync(new[] { dicomFile1 }, studyInstanceUid: validUID); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + + // assert on response + DicomDataset responseDataset = await response.GetValueAsync(); + DicomSequence refSopSequence = responseDataset.GetSequence(DicomTag.ReferencedSOPSequence); + Assert.Single(refSopSequence); + + DicomDataset firstInstance = refSopSequence.Items[0]; + + // expect a comment sequence to be empty + DicomSequence failedAttributesSequence = firstInstance.GetSequence(DicomTag.FailedAttributesSequence); + Assert.Empty(failedAttributesSequence); + } + + [Fact] + public async Task GivenInstanceWithPatientIdValidWithNullPadding_WhenStoreInstanceWithPartialValidation_ThenExpectOkAndNoWarnings() + { + var validPatientIdWithNullPadding = "123\0"; + DicomFile dicomFile1 = new DicomFile( + Samples.CreateRandomInstanceDataset(patientId: validPatientIdWithNullPadding, validateItems: false)); DicomWebResponse response = await _instancesManager.StoreAsync(new[] { dicomFile1 }); @@ -209,6 +234,23 @@ public async Task GivenInstanceWithCoreTagWithNullPadding_WhenStoreInstanceWithP Assert.Empty(failedAttributesSequence); } + [Fact] + public async Task GivenInstanceWithPatientIdInvalidWithNullPadding_WhenStoreInstanceWithPartialValidation_ThenExpectConflict() + { + DicomFile dicomFile1 = new DicomFile( + Samples.CreateRandomInstanceDataset(patientId: "\0123\0", validateItems: false)); + + DicomWebException exception = await Assert.ThrowsAsync(() => _instancesManager.StoreAsync(new[] { dicomFile1 })); + + Assert.Equal(HttpStatusCode.Conflict, exception.StatusCode); + + Assert.False(exception.ResponseDataset.TryGetSequence(DicomTag.ReferencedSOPSequence, out DicomSequence _)); + + ValidationHelpers.ValidateFailedSopSequence( + exception.ResponseDataset, + ResponseHelper.ConvertToFailedSopSequenceEntry(dicomFile1.Dataset, ValidationHelpers.ValidationFailedFailureCode)); + } + [Fact] public async Task GivenDatasetWithInvalidVrValue_WhenStoring_TheServerShouldReturnAccepted() {