From e952c9cbf85521692b651f8a496caa49353776ea Mon Sep 17 00:00:00 2001 From: John Stairs Date: Wed, 1 May 2019 08:10:25 -0700 Subject: [PATCH] Atomic delete operation --- .../Persistence/ResourceHandlerTests.cs | 38 ++----------------- .../Resources/Delete/DeleteResourceHandler.cs | 29 ++++++-------- .../Features/Storage/CosmosFhirDataStore.cs | 5 +++ .../Upsert/upsertWithHistory.js | 7 +++- .../Persistence/FhirStorageTests.cs | 28 ++++++++++++++ 5 files changed, 54 insertions(+), 53 deletions(-) diff --git a/src/Microsoft.Health.Fhir.Core.UnitTests/Persistence/ResourceHandlerTests.cs b/src/Microsoft.Health.Fhir.Core.UnitTests/Persistence/ResourceHandlerTests.cs index 871be8fa32..fd83d1bc14 100644 --- 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(x => x.Id == "id1"), Arg.Any()) - .Returns(CreateResourceWrapper(observation, true)); - - ResourceKey resultKey = (await _mediator.DeleteResourceAsync(new ResourceKey("id1"), false)).ResourceKey; + _fhirDataStore.UpsertAsync(Arg.Any(), null, true, true, Arg.Any()).Returns(default(UpsertOutcome)); - await _fhirDataStore.DidNotReceive().UpsertAsync(Arg.Any(), Arg.Any(), true, true, Arg.Any()); + var resourceKey = new ResourceKey("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(x => x.Id == "id1"), Arg.Any()).Returns((ResourceWrapper)null); - - ResourceKey resultKey = (await _mediator.DeleteResourceAsync(new ResourceKey("id1"), false)).ResourceKey; - - await _fhirDataStore.DidNotReceive().UpsertAsync(Arg.Any(), Arg.Any(), true, true, Arg.Any()); - - 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 index 464fe67a9a..bafba6611c 100644 --- 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 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 index 7c213e216d..c81ba1aae7 100644 --- 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 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/src/Microsoft.Health.Fhir.CosmosDb/Features/Storage/StoredProcedures/Upsert/upsertWithHistory.js b/src/Microsoft.Health.Fhir.CosmosDb/Features/Storage/StoredProcedures/Upsert/upsertWithHistory.js index 3adb3079cd..f519acff11 100644 --- a/src/Microsoft.Health.Fhir.CosmosDb/Features/Storage/StoredProcedures/Upsert/upsertWithHistory.js +++ b/src/Microsoft.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 index 736a0a7aae..96ce9b52f6 100644 --- 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( () => Mediator.GetResourceAsync(new ResourceKey(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( + () => Mediator.GetResourceAsync(new ResourceKey(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( + () => Mediator.GetResourceAsync(new ResourceKey(saveResult.Resource.Id))); + } + [Fact] public async Task WhenHardDeletingAResource_ThenWeGetResourceNotFound() {