Accept null PatientID in STOW (#3235)

* STOW changes in Sql for Patiend ID to accept null

* STOW dotnet code changes to allow null PatientID, unit tests for V1 & V2

* Added E2E test for Patient Id scenario

* Fix for breaking e2e tests

* Changes from review comment

* Added comments for readability

* Updates from review comments

* Updates on v2 conformance statement

* Added a separate validator for PatientId

* Updates on v2 conformance statement

* Updated dicom cast tests to include null patient id scenarios

* Changed patientId check from IsNullOrEmpty to IsNullOrWhitespace

converter/dicom-cast/src/Microsoft.Health.DicomCast.Core.UnitTests/Features/Worker/ChangeFeedProcessorTests.cs

@@ -8,6 +8,7 @@
 using System.Text.Json;
 using System.Threading;
 using System.Threading.Tasks;
+using FellowOakDicom;
 using Microsoft.Extensions.Logging.Abstractions;
 using Microsoft.Extensions.Options;
 using Microsoft.Health.Core.Internal;
@@ -280,8 +281,39 @@ public async Task WhenThrowTimeoutRejectedException_ExceptionNotThrown()
         await _fhirTransactionPipeline.Received(1).ProcessAsync(changeFeeds1[0], DefaultCancellationToken);
     }
 
+    [Theory]
+    [InlineData(nameof(DicomTagException))]
+    [InlineData(nameof(MissingRequiredDicomTagException))]
+    public async Task WhenThrowDicomTagException_ExceptionNotThrown(string exception)
+    {
+        ChangeFeedEntry[] changeFeeds1 = new[]
+        {
+            ChangeFeedGenerator.Generate(1),
+        };
+
+        // Arrange
+        _changeFeedRetrieveService.RetrieveLatestSequenceAsync(DefaultCancellationToken).Returns(1L);
+
+        _changeFeedRetrieveService.RetrieveChangeFeedAsync(0, ChangeFeedProcessor.DefaultLimit, DefaultCancellationToken).Returns(changeFeeds1);
+        _changeFeedRetrieveService.RetrieveChangeFeedAsync(1, ChangeFeedProcessor.DefaultLimit, DefaultCancellationToken).Returns(Array.Empty<ChangeFeedEntry>());
+
+        _fhirTransactionPipeline.When(pipeline => pipeline.ProcessAsync(Arg.Any<ChangeFeedEntry>(), Arg.Any<CancellationToken>())).Do(pipeline => { ThrowDicomTagException(exception); });
+
+        // Act
+        await ExecuteProcessAsync();
+
+        // Assert
+        await _changeFeedRetrieveService.Received(2).RetrieveLatestSequenceAsync(DefaultCancellationToken);
+
+        await _changeFeedRetrieveService.ReceivedWithAnyArgs(2).RetrieveChangeFeedAsync(default, default, default);
+        await _changeFeedRetrieveService.Received(1).RetrieveChangeFeedAsync(0, ChangeFeedProcessor.DefaultLimit, DefaultCancellationToken);
+        await _changeFeedRetrieveService.Received(1).RetrieveChangeFeedAsync(1, ChangeFeedProcessor.DefaultLimit, DefaultCancellationToken);
+
+        await _fhirTransactionPipeline.Received(1).ProcessAsync(changeFeeds1[0], DefaultCancellationToken);
+    }
+
     [Fact]
-    public async Task WhenThrowDicomTagException_ExceptionNotThrown()
+    public async Task WhenMissingRequiredDicomTagException_ExceptionNotThrown()
     {
         ChangeFeedEntry[] changeFeeds1 = new[]
         {
@@ -294,7 +326,7 @@ public async Task WhenThrowDicomTagException_ExceptionNotThrown()
         _changeFeedRetrieveService.RetrieveChangeFeedAsync(0, ChangeFeedProcessor.DefaultLimit, DefaultCancellationToken).Returns(changeFeeds1);
         _changeFeedRetrieveService.RetrieveChangeFeedAsync(1, ChangeFeedProcessor.DefaultLimit, DefaultCancellationToken).Returns(Array.Empty<ChangeFeedEntry>());
 
-        _fhirTransactionPipeline.When(pipeline => pipeline.ProcessAsync(Arg.Any<ChangeFeedEntry>(), Arg.Any<CancellationToken>())).Do(pipeline => { throw new DicomTagException("exception"); });
+        _fhirTransactionPipeline.When(pipeline => pipeline.ProcessAsync(Arg.Any<ChangeFeedEntry>(), Arg.Any<CancellationToken>())).Do(pipeline => { throw new MissingRequiredDicomTagException(nameof(DicomTag.PatientID)); });
 
         // Act
         await ExecuteProcessAsync();
@@ -463,4 +495,16 @@ private async Task ExecuteProcessAsync(TimeSpan? pollIntervalDuringCatchup = nul
 
         await _changeFeedProcessor.ProcessAsync(pollIntervalDuringCatchup.Value, DefaultCancellationToken);
     }
+
+    private static void ThrowDicomTagException(string exception)
+    {
+        if (exception.Equals(nameof(DicomTagException)))
+        {
+            throw new DicomTagException("exception");
+        }
+        else if (exception.Equals(nameof(MissingRequiredDicomTagException)))
+        {
+            throw new MissingRequiredDicomTagException(nameof(DicomTag.PatientID));
+        }
+    }
 }

converter/dicom-cast/src/Microsoft.Health.DicomCast.Core.UnitTests/Features/Worker/FhirTransaction/Patient/PatientPipelineStepTests.cs

@@ -54,13 +54,24 @@ public async Task GivenNullMetadata_WhenRequestIsPrepared_ThenItShouldNotCreateE
     }
 
     [Fact]
-    public async Task GivenMissingPatientId_WhenPreparingTheRequest_ThenMissingRequiredDicomTagExceptionShouldBeThrown()
+    public async Task GivenMissingPatientIdTag_WhenPreparingTheRequest_ThenMissingRequiredDicomTagExceptionShouldBeThrown()
     {
         var context = new FhirTransactionContext(ChangeFeedGenerator.Generate(metadata: new DicomDataset()));
 
         await Assert.ThrowsAsync<MissingRequiredDicomTagException>(() => _patientPipeline.PrepareRequestAsync(context, DefaultCancellationToken));
     }
 
+    [Fact]
+    public async Task GivenPatientIdTagPresentWithMissingValue_WhenPreparingTheRequest_ThenMissingRequiredDicomTagExceptionShouldBeThrown()
+    {
+        DicomDataset dicomDataset = new DicomDataset();
+        dicomDataset.AddOrUpdate(DicomTag.PatientID, new string[] { null });
+
+        var context = new FhirTransactionContext(ChangeFeedGenerator.Generate(metadata: dicomDataset));
+
+        await Assert.ThrowsAsync<MissingRequiredDicomTagException>(() => _patientPipeline.PrepareRequestAsync(context, DefaultCancellationToken));
+    }
+
     [Fact]
     public async Task GivenNoExistingPatient_WhenRequestIsPrepared_ThenCorrectEntryComponentShouldBeCreated()
     {

docs/concepts/bulk-update.md

@@ -211,3 +211,5 @@ There is no change in other APIs. All the other APIs supports only latest versio
 > Only one update operation can be performed at a time.
 
 > There is no way to delete only the latest version or revert back to original version.
+
+> We do not support updating any field from non-null to a null value.

docs/concepts/extended-query-tags.md

@@ -73,6 +73,8 @@ The following VR types are supported:
 
 > Only the first value will be indexed of a single valued data element that incorrectly has multiple values.
 
+> We do not index extended query tags if the value is null or empty.
+
 #### Responses
 
 | Name              | Type                                        | Description                                                  |

docs/resources/v2-conformance-statement.md

@@ -73,7 +73,7 @@ The following DICOM elements are required to be present in every DICOM file atte
 - SOPClassUID
 - PatientID
 
-> Note: All identifiers must be between 1 and 64 characters long, and only contain alpha numeric characters or the following special characters: `.`, `-`. PatientID is validated based on its LO VR type.
+> Note: All identifiers must be between 1 and 64 characters long, and only contain alpha numeric characters or the following special characters: `.`, `-`. PatientID continues to be a required tag and can have the value as null in the input. PatientID is validated based on its LO VR type.
 
 Each file stored must have a unique combination of StudyInstanceUID, SeriesInstanceUID and SopInstanceUID. The warning code `45070` will be returned if a file with the same identifiers already exists.
 
@@ -454,6 +454,8 @@ We support searching on below attributes and search type.
 | ManufacturerModelName | | X | X | X | X |  |
 | SOPInstanceUID |  |  | X |  | X | X |
 
+> Note: We do not support searching using empty string for any attributes. 
+
 #### Search Matching
 
 We support below matching types.
@@ -877,6 +879,8 @@ We support searching on these attributes:
 | ProcedureStepState |
 | StudyInstanceUID |
 
+> Note: We do not support searching using empty string for any attributes. 
+
 #### Search Matching
 
 We support these matching types:

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

@@ -80,8 +80,6 @@ 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>());
     }
 
     [Fact]
@@ -120,6 +118,71 @@ public async Task GivenPartialValidation_WhenPatientIDInvalid_ExpectTagValidated
             result.InvalidTagErrors[DicomTag.PatientID].Error);
     }
 
+    [Theory]
+    [InlineData("")]
+    [InlineData(null)]
+    public async Task GivenPatientIdEmpty_WhenValidated_ExpectErrorProduced(string value)
+    {
+        var featureConfigurationEnableFullValidation = Substitute.For<IOptions<FeatureConfiguration>>();
+        featureConfigurationEnableFullValidation.Value.Returns(new FeatureConfiguration { });
+
+        DicomDataset dicomDataset = Samples.CreateRandomInstanceDataset(
+            validateItems: false,
+            patientId: value);
+
+        if (value == null)
+            dicomDataset.AddOrUpdate(DicomTag.PatientID, new string[] { null });
+
+        IElementMinimumValidator minimumValidator = Substitute.For<IElementMinimumValidator>();
+
+        var dicomDatasetValidator = new StoreDatasetValidator(
+            featureConfigurationEnableFullValidation,
+            minimumValidator,
+            _queryTagService,
+            _storeMeter,
+            _dicomRequestContextAccessor,
+            NullLogger<StoreDatasetValidator>.Instance);
+
+        var result = await dicomDatasetValidator.ValidateAsync(
+            dicomDataset,
+            null,
+            new CancellationToken());
+
+        Assert.Contains(
+            "DICOM100: (0010,0020) - The required tag '(0010,0020)' is missing.",
+            result.InvalidTagErrors[DicomTag.PatientID].Error);
+    }
+
+    [Fact]
+    public async Task GivenPatientIdTagNotPresent_WhenValidated_ExpectErrorProduced()
+    {
+        var featureConfigurationEnableFullValidation = Substitute.For<IOptions<FeatureConfiguration>>();
+        featureConfigurationEnableFullValidation.Value.Returns(new FeatureConfiguration { });
+
+        DicomDataset dicomDataset = Samples.CreateRandomInstanceDataset(
+            validateItems: false);
+        dicomDataset.Remove(DicomTag.PatientID);
+
+        IElementMinimumValidator minimumValidator = Substitute.For<IElementMinimumValidator>();
+
+        var dicomDatasetValidator = new StoreDatasetValidator(
+            featureConfigurationEnableFullValidation,
+            minimumValidator,
+            _queryTagService,
+            _storeMeter,
+            _dicomRequestContextAccessor,
+            NullLogger<StoreDatasetValidator>.Instance);
+
+        var result = await dicomDatasetValidator.ValidateAsync(
+            dicomDataset,
+            null,
+            new CancellationToken());
+
+        Assert.Contains(
+            "DICOM100: (0010,0020) - The required tag '(0010,0020)' is missing.",
+            result.InvalidTagErrors[DicomTag.PatientID].Error);
+    }
+
     [Fact]
     public async Task GivenDicomTagWithDifferentVR_WhenValidated_ThenShouldReturnInvalidEntries()
     {

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

@@ -189,6 +189,50 @@ public async Task GivenV2Enabled_WhenPatientIDPAddedWithComma_ExpectTagValidated
         Assert.Empty(result.InvalidTagErrors);
     }
 
+    [Theory]
+    [InlineData("")]
+    [InlineData("     ")]
+    [InlineData(null)]
+    public async Task GivenV2Enabled_WhenPatientIDTagPresentAndValueEmpty_ExpectTagValidatedAndWarningsProduced(string value)
+    {
+        DicomDataset dicomDataset = Samples.CreateRandomInstanceDataset(
+            validateItems: false,
+            patientId: value);
+
+        if (value == null)
+            dicomDataset.AddOrUpdate(DicomTag.PatientID, new string[] { null });
+
+        var result = await _dicomDatasetValidator.ValidateAsync(
+            dicomDataset,
+            null,
+            new CancellationToken());
+
+        Assert.True(result.InvalidTagErrors.Any());
+        Assert.Single(result.InvalidTagErrors);
+        Assert.False(result.HasCoreTagError);
+        Assert.False(result.InvalidTagErrors[DicomTag.PatientID].IsRequiredCoreTag);
+        Assert.Equal("DICOM100: (0010,0020) - The required tag '(0010,0020)' is missing.", result.InvalidTagErrors[DicomTag.PatientID].Error);
+    }
+
+    [Fact]
+    public async Task GivenV2Enabled_WhenPatientIDTagNotPresent_ExpectErrorProduced()
+    {
+        DicomDataset dicomDataset = Samples.CreateRandomInstanceDataset(
+            validateItems: false);
+        dicomDataset.Remove(DicomTag.PatientID);
+
+        var result = await _dicomDatasetValidator.ValidateAsync(
+            dicomDataset,
+            null,
+            new CancellationToken());
+
+        Assert.True(result.InvalidTagErrors.Any());
+        Assert.Single(result.InvalidTagErrors);
+        Assert.True(result.HasCoreTagError);
+        Assert.True(result.InvalidTagErrors[DicomTag.PatientID].IsRequiredCoreTag);
+        Assert.Equal("DICOM100: (0010,0020) - The required tag '(0010,0020)' is missing.", result.InvalidTagErrors[DicomTag.PatientID].Error);
+    }
+
     [Fact]
     public async Task GivenV2Enabled_WhenNonRequiredTagNull_ExpectTagValidatedAndNoErrorProduced()
     {