Dicom bulk update improvements (#3175)

* Dicom bulk update improvements * Fix merge conflict * Fix comment * fix Tostring() * Fix comment * Update doc * Fix logging metric * Fix comment * Add IsExternal
microsoft · Nov 10, 2023 · 97c3784 · 97c3784
1 parent 77a4c79
commit 97c3784
Show file tree

Hide file tree

Showing 11 changed files with 414 additions and 1,080 deletions.
diff --git a/docs/concepts/bulk-update.md b/docs/concepts/bulk-update.md
@@ -63,14 +63,21 @@ POST ...v1/partitions/{PartitionName}/studies/$bulkUpdate
 
 #### Request Body
 
+Below `UpdateSpecification` is passed as the request body. The `UpdateSpecification` needs both `studyInstanceUids` and `changeDataset` to be specified. 
+
 ```json
 {
-    "studyInstanceUids": ["12.3.4.5"], 
-    "changeDataset": {
-        "00100010": {
-            "vr": "LO",
-            "Value": ["New patient name"]
-        }
+   "studyInstanceUids": ["1.113654.3.13.1026"],
+    "changeDataset": { 
+        "00100010": { 
+            "vr": "PN", 
+            "Value": 
+            [
+                { 
+                    "Alphabetic": "New Patient Name 1" 
+                }
+            ] 
+        } 
     }
 }
 ```
@@ -89,7 +96,7 @@ Content-Type: application/json
 
 | Name              | Type                                        | Description                                                  |
 | ----------------- | ------------------------------------------- | ------------------------------------------------------------ |
-| 202 (Accepted)    | [Operation Reference](#operation-reference) | Extended query tag(s) have been added, and a long-running operation has been started to re-index existing DICOM instances |
+| 202 (Accepted)    | [Operation Reference](#operation-reference) | A long-running operation has been started to update DICOM attributes |
 | 400 (Bad Request) |                                             | Request body has invalid data                                |
 
 ### Operation Status
@@ -107,6 +114,8 @@ GET .../operations/{operationId}
 
 #### Responses
 
+**Successful response**
+
 ```json
 {
     "operationId": "1323c079a1b64efcb8943ef7707b5438",
@@ -117,12 +126,34 @@ GET .../operations/{operationId}
     "percentComplete": 100,
     "results": {
         "studyUpdated": 1,
-        "instanceUpdated": 16,
-        // Errors will go here
+        "instanceUpdated": 16
+    }
+}
+```
+
+**Failure respose**
+```
+{
+    "operationId": "1323c079a1b64efcb8943ef7707b5438",
+    "type": "update",
+    "createdTime": "2023-05-08T05:01:30.1441374Z",
+    "lastUpdatedTime": "2023-05-08T05:01:42.9067335Z",
+    "status": "failed",
+    "percentComplete": 100,
+    "results": {
+        "studyUpdated": 0,
+        "studyFailed": 1,
+        "instanceUpdated": 0,
+        "errors": [
+            "Failed to update instances for study 1.113654.3.13.1026"
+        ]
     }
 }
 ```
 
+If there are any instance specific exception, it will be added to the `errors` list. It will include all the UIDs of the instance like
+`Instance UIDs - PartitionKey: 1, StudyInstanceUID: 1.113654.3.13.1026, SeriesInstanceUID: 1.113654.3.13.1035, SOPInstanceUID: 1.113654.3.13.1510`
+
 | Name            | Type                    | Description                                  |
 | --------------- | ----------------------- | -------------------------------------------- |
 | 200 (OK)        | [Operation](#operation) | The operation with the specified ID has completed |
@@ -178,4 +209,6 @@ 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.
+> There is no way to delete only the latest version or revert back to original version.
+
+> QIDO doesn't support querying extended query tag after it is updated. 
diff --git a/src/Microsoft.Health.Dicom.Core/Features/Model/VersionedInstanceIdentifier.cs b/src/Microsoft.Health.Dicom.Core/Features/Model/VersionedInstanceIdentifier.cs
@@ -36,5 +36,5 @@ public override int GetHashCode()
         => base.GetHashCode() ^ Version.GetHashCode();
 
     public override string ToString()
-        => base.ToString() + $"Version: {Version}";
+        => base.ToString() + $", Version: {Version}";
 }
diff --git a/src/Microsoft.Health.Dicom.Core/Features/Update/UpdateRequestValidator.cs b/src/Microsoft.Health.Dicom.Core/Features/Update/UpdateRequestValidator.cs
@@ -81,7 +81,4 @@ public static DicomDataset ValidateDicomDataset(DicomDataset dataset)
         }
         return failedSop;
     }
-
-    private static string GetCommaSeparatedTags(List<string> tags)
-        => string.Join(", ", tags.Select(x => $"'{x}'"));
 }
diff --git a/src/Microsoft.Health.Dicom.Functions.App/host.json b/src/Microsoft.Health.Dicom.Functions.App/host.json
@@ -89,7 +89,7 @@
     "Frequency": "0 0 * * *",
     "MinimumAgeDays": 7,
     "Statuses": [ "Completed" ],
-    "ExcludeFunctions": [ "MigratingFrameRangeFilesAsync", "UpdateInstancesV2Async", "UpdateInstancesV3Async" ]
+    "ExcludeFunctions": [ "MigratingFrameRangeFilesAsync", "UpdateInstancesV3Async", "UpdateInstancesV4Async" ]
   },
   "SqlServer": {
     "Retry": {

diff --git a/src/Microsoft.Health.Dicom.Functions.UnitTests/Update/UpdateDurableFunctionTests.Activity.cs b/src/Microsoft.Health.Dicom.Functions.UnitTests/Update/UpdateDurableFunctionTests.Activity.cs
@@ -3,19 +3,20 @@
 // Licensed under the MIT License (MIT). See LICENSE in the repo root for license information.
 // -------------------------------------------------------------------------------------------------
 
-using System;
 using System.Collections.Generic;
 using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
 using FellowOakDicom;
 using Microsoft.Extensions.Logging.Abstractions;
+using Microsoft.Health.Dicom.Core.Exceptions;
 using Microsoft.Health.Dicom.Core.Features.Common;
 using Microsoft.Health.Dicom.Core.Features.Model;
 using Microsoft.Health.Dicom.Core.Features.Partitioning;
 using Microsoft.Health.Dicom.Functions.Update.Models;
 using Microsoft.Health.Dicom.Tests.Common;
 using NSubstitute;
+using NSubstitute.ExceptionExtensions;
 using Xunit;
 
 namespace Microsoft.Health.Dicom.Functions.UnitTests.Update;
@@ -81,7 +82,7 @@ public async Task GivenInstanceMetadata_WhenUpdatingBlobInBatches_ThenShouldUpda
                 .Returns(DefaultFileProperties);
         }
 
-        await _updateDurableFunction.UpdateInstanceBlobsV2Async(
+        await _updateDurableFunction.UpdateInstanceBlobsV3Async(
             new UpdateInstanceBlobArgumentsV2(Partition.Default, expected, dataset),
             NullLogger.Instance);
 
@@ -95,6 +96,41 @@ await _updateInstanceService
         }
     }
 
+    [Fact]
+    public async Task GivenInstanceMetadata_WhenUpdatingWithDataStoreException_ThenShouldReturnFailureCorrectly()
+    {
+        var studyInstanceUid = TestUidGenerator.Generate();
+        var expected = GetInstanceIdentifiersList(studyInstanceUid);
+
+        var dataset = "{\"00100010\":{\"vr\":\"PN\",\"Value\":[{\"Alphabetic\":\"Patient Name\"}]}}";
+
+        foreach (var instance in expected)
+        {
+            _updateInstanceService
+                .UpdateInstanceBlobAsync(
+                    instance,
+                    Arg.Is<DicomDataset>(x => x.GetSingleValue<string>(DicomTag.PatientName) == "Patient Name"),
+                    Partition.Default,
+                    Arg.Any<CancellationToken>())
+                .ThrowsAsync(new DataStoreException("Error"));
+        }
+
+        var response = await _updateDurableFunction.UpdateInstanceBlobsV3Async(
+            new UpdateInstanceBlobArgumentsV2(Partition.Default, expected, dataset),
+            NullLogger.Instance);
+
+        foreach (var instance in expected)
+        {
+            await _updateInstanceService
+                .Received(1)
+                .UpdateInstanceBlobAsync(Arg.Is(GetPredicate(instance)), Arg.Is<DicomDataset>(x => x.GetSingleValue<string>(DicomTag.PatientName) == "Patient Name"),
+                Partition.Default,
+                Arg.Any<CancellationToken>());
+        }
+
+        Assert.Equal(expected.Count, response.Errors.Count);
+    }
+
     [Fact]
     public async Task GivenCompleteInstanceArgument_WhenCompleting_ThenShouldComplete()
     {
@@ -108,7 +144,7 @@ public async Task GivenCompleteInstanceArgument_WhenCompleting_ThenShouldComplet
             { DicomTag.PatientName, "Patient Name" }
         };
 
-        await _updateDurableFunction.CompleteUpdateStudyV2Async(
+        await _updateDurableFunction.CompleteUpdateStudyV3Async(
             new CompleteStudyArgumentsV2(
                 Partition.DefaultKey,
                 studyInstanceUid,
@@ -121,35 +157,6 @@ await _indexStore
             .EndUpdateInstanceAsync(Partition.DefaultKey, studyInstanceUid, Arg.Is<DicomDataset>(x => x.GetSingleValue<string>(DicomTag.PatientName) == "Patient Name"), instanceMetadataList, CancellationToken.None);
     }
 
-    [Fact]
-    [Obsolete("Obsolete")]
-    public async Task GivenInstanceUpdateFails_WhenDeleteFileV2_ThenShouldDeleteSuccessfully()
-    {
-        var studyInstanceUid = TestUidGenerator.Generate();
-        var identifiers = GetInstanceIdentifiersList(studyInstanceUid, instanceProperty: new InstanceProperties { NewVersion = 1 });
-        IReadOnlyList<InstanceFileState> expected = identifiers.Select(x =>
-            new InstanceFileState
-            {
-                Version = x.VersionedInstanceIdentifier.Version,
-                OriginalVersion = x.InstanceProperties.OriginalVersion,
-                NewVersion = x.InstanceProperties.NewVersion
-            }).Take(1).ToList();
-
-        _updateInstanceService
-            .DeleteInstanceBlobAsync(Arg.Any<long>(), Partition.Default, null, Arg.Any<CancellationToken>())
-            .Returns(Task.CompletedTask);
-
-        // Call the activity
-        await _updateDurableFunction.CleanupNewVersionBlobV2Async(
-            new CleanupBlobArguments(expected, Partition.Default),
-            NullLogger.Instance);
-
-        // Assert behavior
-        await _updateInstanceService
-            .Received(1)
-            .DeleteInstanceBlobAsync(Arg.Any<long>(), Partition.Default, null, Arg.Any<CancellationToken>());
-    }
-
     [Fact]
     public async Task GivenInstanceUpdateFails_WhenDeleteFileWithV3_ThenShouldDeleteNewVersionSuccessfullyWithoutFileProperties()
     {
@@ -194,35 +201,6 @@ await _updateInstanceService
             .DeleteInstanceBlobAsync(Arg.Any<long>(), Partition.Default, DefaultFileProperties, Arg.Any<CancellationToken>());
     }
 
-    [Fact]
-    [Obsolete("Obsolete")]
-    public async Task GivenInstanceMetadataList_WhenDeleteFileV2_ThenShouldDeleteSuccessfully()
-    {
-        var studyInstanceUid = TestUidGenerator.Generate();
-        var identifiers = GetInstanceIdentifiersList(studyInstanceUid, partition: Partition.Default);
-        IReadOnlyList<InstanceFileState> expected = identifiers.Select(x =>
-            new InstanceFileState
-            {
-                Version = x.VersionedInstanceIdentifier.Version,
-                OriginalVersion = x.InstanceProperties.OriginalVersion,
-                NewVersion = x.InstanceProperties.NewVersion
-            }).Take(1).ToList();
-
-        _updateInstanceService
-            .DeleteInstanceBlobAsync(expected[0].Version, identifiers[0].VersionedInstanceIdentifier.Partition, null, Arg.Any<CancellationToken>())
-            .Returns(Task.CompletedTask);
-
-        // Call the activity
-        await _updateDurableFunction.DeleteOldVersionBlobV2Async(
-            new CleanupBlobArguments(expected, identifiers[0].VersionedInstanceIdentifier.Partition),
-            NullLogger.Instance);
-
-        // Assert behavior
-        await _updateInstanceService
-            .Received(1)
-            .DeleteInstanceBlobAsync(expected[0].Version, identifiers[0].VersionedInstanceIdentifier.Partition, null, Arg.Any<CancellationToken>());
-    }
-
     [Fact]
     public async Task GivenInstanceMetadataList_WhenDeleteFileV3_ThenShouldDeleteSuccessfullyWithoutFileProperties()
     {
@@ -276,35 +254,6 @@ await _updateInstanceService
             .DeleteInstanceBlobAsync(identifiers[0].VersionedInstanceIdentifier.Version, identifiers[0].VersionedInstanceIdentifier.Partition, identifiers[0].InstanceProperties.FileProperties, Arg.Any<CancellationToken>());
     }
 
-    [Fact]
-    [Obsolete("Should use SetOriginalBlobToColdAccessTierV2Async instead")]
-    public async Task GivenInstanceMetadataList_WhenChangeAccessTier_ThenShoulChangeSuccessfully()
-    {
-        var studyInstanceUid = TestUidGenerator.Generate();
-        var identifiers = GetInstanceIdentifiersList(studyInstanceUid, Partition.Default, new InstanceProperties { NewVersion = 2 });
-        IReadOnlyList<InstanceFileState> expected = identifiers.Select(x =>
-            new InstanceFileState
-            {
-                Version = x.VersionedInstanceIdentifier.Version,
-                OriginalVersion = x.InstanceProperties.OriginalVersion,
-                NewVersion = x.InstanceProperties.NewVersion
-            }).Take(1).ToList();
-
-        _fileStore
-            .SetBlobToColdAccessTierAsync(Arg.Any<long>(), Partition.Default, null, Arg.Any<CancellationToken>())
-            .Returns(Task.CompletedTask);
-
-        // Call the activity
-        await _updateDurableFunction.SetOriginalBlobToColdAccessTierAsync(
-            new CleanupBlobArguments(expected, Partition.Default),
-            NullLogger.Instance);
-
-        // Assert behavior
-        await _fileStore
-            .Received(1)
-            .SetBlobToColdAccessTierAsync(Arg.Any<long>(), Partition.Default, null, Arg.Any<CancellationToken>());
-    }
-
     [Fact]
     public async Task GivenInstanceMetadataList_WhenChangeAccessTierV2_ThenShouldChangeSuccessfullyUsingFileProperties()
     {