Atomic delete operation

microsoft · May 1, 2019 · e952c9c · e952c9c
1 parent e2ae1a2
commit e952c9c
Show file tree

Hide file tree

Showing 5 changed files with 54 additions and 53 deletions.
diff --git a/src/Microsoft.Health.Fhir.Core.UnitTests/Persistence/ResourceHandlerTests.cs b/src/Microsoft.Health.Fhir.Core.UnitTests/Persistence/ResourceHandlerTests.cs
@@ -219,44 +219,14 @@ public async Task GivenAFhirMediator_WhenDeletingAResourceByVersionId_ThenMethod
         [Fact]
         public async Task GivenAFhirMediator_WhenDeletingAResourceThatIsAlreadyDeleted_ThenDoNothing()
         {
-            var observation = Samples.GetDefaultObservation();
-            observation.Id = "id1";
-            observation.Meta = new Meta
-            {
-                VersionId = "version1",
-            };
-
-            _fhirDataStore.GetAsync(Arg.Is<ResourceKey>(x => x.Id == "id1"), Arg.Any<CancellationToken>())
-                .Returns(CreateResourceWrapper(observation, true));
-
-            ResourceKey resultKey = (await _mediator.DeleteResourceAsync(new ResourceKey<Observation>("id1"), false)).ResourceKey;
+            _fhirDataStore.UpsertAsync(Arg.Any<ResourceWrapper>(), null, true, true, Arg.Any<CancellationToken>()).Returns(default(UpsertOutcome));
 
-            await _fhirDataStore.DidNotReceive().UpsertAsync(Arg.Any<ResourceWrapper>(), Arg.Any<WeakETag>(), true, true, Arg.Any<CancellationToken>());
+            var resourceKey = new ResourceKey<Observation>("id1");
+            ResourceKey resultKey = (await _mediator.DeleteResourceAsync(resourceKey, false)).ResourceKey;
 
-            Assert.Equal(observation.Id, resultKey.Id);
-            Assert.Equal(observation.Meta.VersionId, resultKey.VersionId);
+            Assert.Equal(resourceKey.Id, resultKey.Id);
             Assert.Equal("Observation", resultKey.ResourceType);
-        }
-
-        [Fact]
-        public async Task GivenAFhirMediator_WhenDeletingAResourceThatDoesNotExist_ThenDoNothing()
-        {
-            var observation = Samples.GetDefaultObservation();
-            observation.Id = "id1";
-            observation.Meta = new Meta
-            {
-                VersionId = "version1",
-            };
-
-            _fhirDataStore.GetAsync(Arg.Is<ResourceKey>(x => x.Id == "id1"), Arg.Any<CancellationToken>()).Returns((ResourceWrapper)null);
-
-            ResourceKey resultKey = (await _mediator.DeleteResourceAsync(new ResourceKey<Observation>("id1"), false)).ResourceKey;
-
-            await _fhirDataStore.DidNotReceive().UpsertAsync(Arg.Any<ResourceWrapper>(), Arg.Any<WeakETag>(), true, true, Arg.Any<CancellationToken>());
-
-            Assert.Equal(observation.Id, resultKey.Id);
             Assert.Null(resultKey.VersionId);
-            Assert.Equal("Observation", resultKey.ResourceType);
         }
 
         [Fact]

diff --git a/src/Microsoft.Health.Fhir.Core/Features/Resources/Delete/DeleteResourceHandler.cs b/src/Microsoft.Health.Fhir.Core/Features/Resources/Delete/DeleteResourceHandler.cs
@@ -45,28 +45,21 @@ public async Task<DeleteResourceResponse> Handle(DeleteResourceRequest message,
             }
             else
             {
-                ResourceWrapper existing = await FhirDataStore.GetAsync(key, cancellationToken);
+                var emptyInstance = (Resource)Activator.CreateInstance(ModelInfo.GetTypeForFhirType(message.ResourceKey.ResourceType));
+                emptyInstance.Id = message.ResourceKey.Id;
 
-                version = existing?.Version;
+                ResourceWrapper deletedWrapper = CreateResourceWrapper(emptyInstance, deleted: true);
 
-                if (existing?.IsDeleted == false)
-                {
-                    var emptyInstance = (Resource)Activator.CreateInstance(ModelInfo.GetTypeForFhirType(existing.ResourceTypeName));
-                    emptyInstance.Id = existing.ResourceId;
+                bool keepHistory = await ConformanceProvider.Value.CanKeepHistory(key.ResourceType, cancellationToken);
 
-                    ResourceWrapper deletedWrapper = CreateResourceWrapper(emptyInstance, deleted: true);
+                UpsertOutcome result = await FhirDataStore.UpsertAsync(
+                    deletedWrapper,
+                    weakETag: null,
+                    allowCreate: true,
+                    keepHistory: keepHistory,
+                    cancellationToken: cancellationToken);
 
-                    bool keepHistory = await ConformanceProvider.Value.CanKeepHistory(key.ResourceType, cancellationToken);
-
-                    UpsertOutcome result = await FhirDataStore.UpsertAsync(
-                        deletedWrapper,
-                        WeakETag.FromVersionId(existing.Version),
-                        allowCreate: true,
-                        keepHistory: keepHistory,
-                        cancellationToken: cancellationToken);
-
-                    version = result.Wrapper.Version;
-                }
+                version = result?.Wrapper.Version;
             }
 
             if (string.IsNullOrWhiteSpace(version))

diff --git a/src/Microsoft.Health.Fhir.CosmosDb/Features/Storage/CosmosFhirDataStore.cs b/src/Microsoft.Health.Fhir.CosmosDb/Features/Storage/CosmosFhirDataStore.cs
@@ -125,6 +125,11 @@ public async Task<UpsertOutcome> UpsertAsync(
                 }
                 else if (dce.Error?.Message?.Contains(GetValue(HttpStatusCode.NotFound), StringComparison.Ordinal) == true)
                 {
+                    if (cosmosWrapper.IsDeleted)
+                    {
+                        return null;
+                    }
+
                     if (weakETag != null)
                     {
                         throw new ResourceConflictException(weakETag);

diff --git a/...rosoft.Health.Fhir.CosmosDb/Features/Storage/StoredProcedures/Upsert/upsertWithHistory.js b/...rosoft.Health.Fhir.CosmosDb/Features/Storage/StoredProcedures/Upsert/upsertWithHistory.js
@@ -36,7 +36,7 @@ function upsertWithHistory(doc, matchVersionId, allowCreate, keepHistory) {
         throw new Error(errorMessages.InputWasArray);
     }
 
-    if (!stringIsNullOrEmpty(matchVersionId) || !allowCreate) {
+    if (!stringIsNullOrEmpty(matchVersionId) || !allowCreate || doc.isDeleted) {
         tryReplace(doc, replacePrimaryCallback, matchVersionId);
     } else {
         tryCreate(doc, createPrimaryCallback);
@@ -92,6 +92,11 @@ function upsertWithHistory(doc, matchVersionId, allowCreate, keepHistory) {
 
                 let document = documents[0];
 
+                if (doc.isDeleted && document.isDeleted) {
+                    // don't create another version if already deleted
+                    throw new Error(errorMessages.DocumentNotFound);
+                }
+
                 let documentVersion = document.version;
 
                 // If a match version was passed in, check it matches the primary record

diff --git a/test/Microsoft.Health.Fhir.Tests.Integration/Persistence/FhirStorageTests.cs b/test/Microsoft.Health.Fhir.Tests.Integration/Persistence/FhirStorageTests.cs
@@ -322,6 +322,34 @@ await Assert.ThrowsAsync<ResourceGoneException>(
                 () => Mediator.GetResourceAsync(new ResourceKey<Observation>(saveResult.Resource.Id)));
         }
 
+        [Fact]
+        public async Task WhenDeletingAResourceThatNeverExisted_ThenReadingTheResourceReturnsNotFound()
+        {
+            string id = "missingid";
+
+            var deletedResourceKey = await Mediator.DeleteResourceAsync(new ResourceKey("Observation", id), false);
+
+            Assert.Null(deletedResourceKey.ResourceKey.VersionId);
+
+            await Assert.ThrowsAsync<ResourceNotFoundException>(
+                () => Mediator.GetResourceAsync(new ResourceKey<Observation>(id)));
+        }
+
+        [Fact]
+        public async Task WhenDeletingAResourceForASecondTime_ThenWeDoNotGetANewVersion()
+        {
+            var saveResult = await Mediator.UpsertResourceAsync(Samples.GetJsonSample("Weight"));
+
+            await Mediator.DeleteResourceAsync(new ResourceKey("Observation", saveResult.Resource.Id), false);
+
+            var deletedResourceKey2 = await Mediator.DeleteResourceAsync(new ResourceKey("Observation", saveResult.Resource.Id), false);
+
+            Assert.Null(deletedResourceKey2.ResourceKey.VersionId);
+
+            await Assert.ThrowsAsync<ResourceGoneException>(
+                () => Mediator.GetResourceAsync(new ResourceKey<Observation>(saveResult.Resource.Id)));
+        }
+
         [Fact]
         public async Task WhenHardDeletingAResource_ThenWeGetResourceNotFound()
         {