From 524e3406cdeff2f0b07d814168b20401aa1d4ec5 Mon Sep 17 00:00:00 2001 From: Robert Johnson Date: Mon, 3 Feb 2025 07:44:15 -0800 Subject: [PATCH 01/15] Allow for delete with include --- .../Extensions/SearchServiceExtensions.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/Microsoft.Health.Fhir.Core/Extensions/SearchServiceExtensions.cs b/src/Microsoft.Health.Fhir.Core/Extensions/SearchServiceExtensions.cs index fdd3d0ea33..7a02e8babd 100644 --- a/src/Microsoft.Health.Fhir.Core/Extensions/SearchServiceExtensions.cs +++ b/src/Microsoft.Health.Fhir.Core/Extensions/SearchServiceExtensions.cs @@ -26,8 +26,6 @@ public static class SearchServiceExtensions KnownQueryParameterNames.Summary, KnownQueryParameterNames.Total, KnownQueryParameterNames.ContinuationToken, - "_include", - "_revinclude", }; /// @@ -100,7 +98,7 @@ public static class SearchServiceExtensions { matchedResults.AddRange( results?.Results - .Where(x => x.SearchEntryMode == ValueSets.SearchEntryMode.Match) + .Where(x => x.SearchEntryMode != ValueSets.SearchEntryMode.Outcome) .Take(Math.Max(count.HasValue ? 0 : results.Results.Count(), count.GetValueOrDefault() - matchedResults.Count))); } } From f0c756614d883d15db9911f79dbc0d03911dc2b3 Mon Sep 17 00:00:00 2001 From: Robert Johnson Date: Mon, 3 Feb 2025 13:42:37 -0800 Subject: [PATCH 02/15] Fully support include in deletion service. --- .../BulkDeleteProcessingJobTests.cs | 10 ++- .../Extensions/SearchServiceExtensions.cs | 11 ++- .../BulkDelete/BulkDeleteProcessingJob.cs | 18 ++-- .../Features/Persistence/IDeletionService.cs | 2 +- .../Resources.Designer.cs | 9 ++ src/Microsoft.Health.Fhir.Core/Resources.resx | 4 + .../Resources/ResourceHandlerTests.cs | 6 +- .../Resources/ConditionalResourceHandler.cs | 12 +-- .../ConditionalDeleteResourceHandler.cs | 50 +++++++---- .../Resources/Delete/DeletionService.cs | 85 ++++++++++++------- 10 files changed, 145 insertions(+), 62 deletions(-) diff --git a/src/Microsoft.Health.Fhir.Core.UnitTests/Features/Operations/BulkDelete/BulkDeleteProcessingJobTests.cs b/src/Microsoft.Health.Fhir.Core.UnitTests/Features/Operations/BulkDelete/BulkDeleteProcessingJobTests.cs index e87579db2a..f69d0520b2 100644 --- a/src/Microsoft.Health.Fhir.Core.UnitTests/Features/Operations/BulkDelete/BulkDeleteProcessingJobTests.cs +++ b/src/Microsoft.Health.Fhir.Core.UnitTests/Features/Operations/BulkDelete/BulkDeleteProcessingJobTests.cs @@ -58,8 +58,11 @@ public async Task GivenProcessingJob_WhenJobIsRun_ThenResourcesAreDeleted() Definition = JsonConvert.SerializeObject(definition), }; + var substituteResults = new Dictionary(); + substituteResults.Add("Patient", 3); + _deleter.DeleteMultipleAsync(Arg.Any(), Arg.Any()) - .Returns(args => 3); + .Returns(args => substituteResults); var result = JsonConvert.DeserializeObject(await _processingJob.ExecuteAsync(jobInfo, CancellationToken.None)); Assert.Single(result.ResourcesDeleted); @@ -80,8 +83,11 @@ public async Task GivenProcessingJob_WhenJobIsRunWithMultipleResourceTypes_ThenF Definition = JsonConvert.SerializeObject(definition), }; + var substituteResults = new Dictionary(); + substituteResults.Add("Patient", 3); + _deleter.DeleteMultipleAsync(Arg.Any(), Arg.Any()) - .Returns(args => 3); + .Returns(args => substituteResults); var result = JsonConvert.DeserializeObject(await _processingJob.ExecuteAsync(jobInfo, CancellationToken.None)); Assert.Single(result.ResourcesDeleted); diff --git a/src/Microsoft.Health.Fhir.Core/Extensions/SearchServiceExtensions.cs b/src/Microsoft.Health.Fhir.Core/Extensions/SearchServiceExtensions.cs index 7a02e8babd..796f26655b 100644 --- a/src/Microsoft.Health.Fhir.Core/Extensions/SearchServiceExtensions.cs +++ b/src/Microsoft.Health.Fhir.Core/Extensions/SearchServiceExtensions.cs @@ -68,6 +68,7 @@ public static class SearchServiceExtensions } var matchedResults = new List(); + var includeResults = new List(); string lastContinuationToken = continuationToken; LongRunningOperationStatistics statistics = new LongRunningOperationStatistics(operationName: "conditionalSearchAsync"); try @@ -98,8 +99,13 @@ public static class SearchServiceExtensions { matchedResults.AddRange( results?.Results - .Where(x => x.SearchEntryMode != ValueSets.SearchEntryMode.Outcome) + .Where(x => x.SearchEntryMode == ValueSets.SearchEntryMode.Match) .Take(Math.Max(count.HasValue ? 0 : results.Results.Count(), count.GetValueOrDefault() - matchedResults.Count))); + + // This will get include results and outcome results. Outcome results are needed to check for too many includes warning. + includeResults.AddRange( + results?.Results + .Where(x => x.SearchEntryMode != ValueSets.SearchEntryMode.Match)); } } while (count.HasValue && matchedResults.Count < count && !string.IsNullOrEmpty(lastContinuationToken)); @@ -121,7 +127,8 @@ public static class SearchServiceExtensions } } - return (matchedResults, lastContinuationToken); + var resultsToReturn = matchedResults.Concat(includeResults).ToList(); + return (resultsToReturn, lastContinuationToken); } } } diff --git a/src/Microsoft.Health.Fhir.Core/Features/Operations/BulkDelete/BulkDeleteProcessingJob.cs b/src/Microsoft.Health.Fhir.Core/Features/Operations/BulkDelete/BulkDeleteProcessingJob.cs index a92c531d70..af9e7c0306 100644 --- a/src/Microsoft.Health.Fhir.Core/Features/Operations/BulkDelete/BulkDeleteProcessingJob.cs +++ b/src/Microsoft.Health.Fhir.Core/Features/Operations/BulkDelete/BulkDeleteProcessingJob.cs @@ -73,14 +73,14 @@ public async Task ExecuteAsync(JobInfo jobInfo, CancellationToken cancel _contextAccessor.RequestContext = fhirRequestContext; var result = new BulkDeleteResult(); - long numDeleted; + IDictionary resourcesDeleted = new Dictionary(); using IScoped deleter = _deleterFactory.Invoke(); Exception exception = null; List types = definition.Type.SplitByOrSeparator().ToList(); try { - numDeleted = await deleter.Value.DeleteMultipleAsync( + resourcesDeleted = await deleter.Value.DeleteMultipleAsync( new ConditionalDeleteResourceRequest( types[0], (IReadOnlyList>)definition.SearchParameters, @@ -91,16 +91,22 @@ public async Task ExecuteAsync(JobInfo jobInfo, CancellationToken cancel allowPartialSuccess: false), // Explicitly setting to call out that this can be changed in the future if we want to. Bulk delete offers the possibility of automatically rerunning the operation until it succeeds, fully automating the process. cancellationToken); } - catch (IncompleteOperationException ex) + catch (IncompleteOperationException> ex) { - numDeleted = ex.PartialResults; + resourcesDeleted = ex.PartialResults; result.Issues.Add(ex.Message); exception = ex; } - result.ResourcesDeleted.Add(types[0], numDeleted); + foreach (var (key, value) in resourcesDeleted) + { + if (!result.ResourcesDeleted.TryAdd(key, value)) + { + result.ResourcesDeleted[key] += value; + } + } - await _mediator.Publish(new BulkDeleteMetricsNotification(jobInfo.Id, numDeleted), cancellationToken); + await _mediator.Publish(new BulkDeleteMetricsNotification(jobInfo.Id, resourcesDeleted.Sum(resource => resource.Value)), cancellationToken); if (exception != null) { diff --git a/src/Microsoft.Health.Fhir.Core/Features/Persistence/IDeletionService.cs b/src/Microsoft.Health.Fhir.Core/Features/Persistence/IDeletionService.cs index 38d60ac85b..4ba2bf9af8 100644 --- a/src/Microsoft.Health.Fhir.Core/Features/Persistence/IDeletionService.cs +++ b/src/Microsoft.Health.Fhir.Core/Features/Persistence/IDeletionService.cs @@ -14,6 +14,6 @@ public interface IDeletionService { public Task DeleteAsync(DeleteResourceRequest request, CancellationToken cancellationToken); - public Task DeleteMultipleAsync(ConditionalDeleteResourceRequest request, CancellationToken cancellationToken); + public Task> DeleteMultipleAsync(ConditionalDeleteResourceRequest request, CancellationToken cancellationToken); } } diff --git a/src/Microsoft.Health.Fhir.Core/Resources.Designer.cs b/src/Microsoft.Health.Fhir.Core/Resources.Designer.cs index a7532e80a9..46b620490f 100644 --- a/src/Microsoft.Health.Fhir.Core/Resources.Designer.cs +++ b/src/Microsoft.Health.Fhir.Core/Resources.Designer.cs @@ -1591,6 +1591,15 @@ internal static string SqlQueryProcessorRanOutOfInternalResourcesException { } } + /// + /// Looks up a localized string similar to The number of included results exceeded the limit of {0}. + /// + internal static string TooManyIncludeResults { + get { + return ResourceManager.GetString("TooManyIncludeResults", resourceCulture); + } + } + /// /// Looks up a localized string similar to There was resource contention with another process in the datastore. Please retry this transaction.. /// diff --git a/src/Microsoft.Health.Fhir.Core/Resources.resx b/src/Microsoft.Health.Fhir.Core/Resources.resx index 2408c458e3..16d3e81376 100644 --- a/src/Microsoft.Health.Fhir.Core/Resources.resx +++ b/src/Microsoft.Health.Fhir.Core/Resources.resx @@ -757,4 +757,8 @@ Error occurred during an operation that is dependent on the customer-managed key. Use https://go.microsoft.com/fwlink/?linkid=2300268 to troubleshoot the issue. + + The number of included results exceeded the limit of {0} + {0} is the limit of how many include results the service will return. + \ No newline at end of file diff --git a/src/Microsoft.Health.Fhir.Shared.Core.UnitTests/Features/Resources/ResourceHandlerTests.cs b/src/Microsoft.Health.Fhir.Shared.Core.UnitTests/Features/Resources/ResourceHandlerTests.cs index d790fbf676..5b256dd37a 100644 --- a/src/Microsoft.Health.Fhir.Shared.Core.UnitTests/Features/Resources/ResourceHandlerTests.cs +++ b/src/Microsoft.Health.Fhir.Shared.Core.UnitTests/Features/Resources/ResourceHandlerTests.cs @@ -14,9 +14,11 @@ using MediatR; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; using Microsoft.Extensions.Primitives; using Microsoft.Health.Core.Features.Security.Authorization; using Microsoft.Health.Extensions.DependencyInjection; +using Microsoft.Health.Fhir.Core.Configs; using Microsoft.Health.Fhir.Core.Exceptions; using Microsoft.Health.Fhir.Core.Extensions; using Microsoft.Health.Fhir.Core.Features.Audit; @@ -115,6 +117,8 @@ public ResourceHandlerTests() var referenceResolver = new ResourceReferenceResolver(_searchService, new TestQueryStringParser(), Substitute.For>()); _resourceIdProvider = new ResourceIdProvider(); + var coreFeatureConfiguration = new CoreFeatureConfiguration(); + var auditLogger = Substitute.For(); var logger = Substitute.For>(); @@ -129,7 +133,7 @@ public ResourceHandlerTests() collection.Add(x => new UpsertResourceHandler(_fhirDataStore, lazyConformanceProvider, _resourceWrapperFactory, _resourceIdProvider, referenceResolver, _authorizationService, ModelInfoProvider.Instance)).Singleton().AsSelf().AsImplementedInterfaces(); collection.Add(x => new ConditionalCreateResourceHandler(_fhirDataStore, lazyConformanceProvider, _resourceWrapperFactory, _searchService, x.GetService(), _resourceIdProvider, _authorizationService, conditionalCreateLogger)).Singleton().AsSelf().AsImplementedInterfaces(); collection.Add(x => new ConditionalUpsertResourceHandler(_fhirDataStore, lazyConformanceProvider, _resourceWrapperFactory, _searchService, x.GetService(), _resourceIdProvider, _authorizationService, conditionalUpsertLogger)).Singleton().AsSelf().AsImplementedInterfaces(); - collection.Add(x => new ConditionalDeleteResourceHandler(_fhirDataStore, lazyConformanceProvider, _resourceWrapperFactory, _searchService, x.GetService(), _resourceIdProvider, _authorizationService, deleter, contextAccessor, conditionalDeleteLogger)).Singleton().AsSelf().AsImplementedInterfaces(); + collection.Add(x => new ConditionalDeleteResourceHandler(_fhirDataStore, lazyConformanceProvider, _resourceWrapperFactory, _searchService, x.GetService(), _resourceIdProvider, _authorizationService, deleter, contextAccessor, new OptionsWrapper(coreFeatureConfiguration), conditionalDeleteLogger)).Singleton().AsSelf().AsImplementedInterfaces(); collection.Add(x => new GetResourceHandler(_fhirDataStore, lazyConformanceProvider, _resourceWrapperFactory, _resourceIdProvider, _dataResourceFilter, _authorizationService, contextAccessor, _searchService)).Singleton().AsSelf().AsImplementedInterfaces(); collection.Add(x => new DeleteResourceHandler(_fhirDataStore, lazyConformanceProvider, _resourceWrapperFactory, _resourceIdProvider, _authorizationService, deleter)).Singleton().AsSelf().AsImplementedInterfaces(); diff --git a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/ConditionalResourceHandler.cs b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/ConditionalResourceHandler.cs index a8cba0c33d..afbfa1ba5a 100644 --- a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/ConditionalResourceHandler.cs +++ b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/ConditionalResourceHandler.cs @@ -54,27 +54,27 @@ public async Task Handle(TRequest request, CancellationToken cancella throw new UnauthorizedFhirActionException(); } - var matchedResults = await _searchService.ConditionalSearchAsync( + var results = await _searchService.ConditionalSearchAsync( request.ResourceType, request.ConditionalParameters, cancellationToken, logger: _logger); - int count = matchedResults.Results.Count; - if (count == 0) + int matchCount = results.Results.Where(result => result.SearchEntryMode == ValueSets.SearchEntryMode.Match).Count(); + if (matchCount == 0) { _logger.LogInformation("Conditional handler: Not Match. ResourceType={ResourceType}", request.ResourceType); return await HandleNoMatch(request, cancellationToken); } - else if (count == 1) + else if (matchCount == 1) { _logger.LogInformation("Conditional handler: One Match Found. ResourceType={ResourceType}", request.ResourceType); - return await HandleSingleMatch(request, matchedResults.Results.First(), cancellationToken); + return await HandleSingleMatch(request, results.Results.First(), cancellationToken); } else { // Multiple matches: The server returns a 412 Precondition Failed error indicating the client's criteria were not selective enough - _logger.LogInformation("PreconditionFailed: Conditional handler: Multiple Matches Found. ResourceType={ResourceType}, NumberOfMatches={NumberOfMatches}", request.ResourceType, count); + _logger.LogInformation("PreconditionFailed: Conditional handler: Multiple Matches Found. ResourceType={ResourceType}, NumberOfMatches={NumberOfMatches}", request.ResourceType, matchCount); throw new PreconditionFailedException(string.Format(CultureInfo.InvariantCulture, Core.Resources.ConditionalOperationNotSelectiveEnough, request.ResourceType)); } } diff --git a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/ConditionalDeleteResourceHandler.cs b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/ConditionalDeleteResourceHandler.cs index f177cc59f0..1200f99191 100644 --- a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/ConditionalDeleteResourceHandler.cs +++ b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/ConditionalDeleteResourceHandler.cs @@ -13,8 +13,10 @@ using MediatR; using Microsoft.Build.Framework; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; using Microsoft.Health.Core.Features.Context; using Microsoft.Health.Core.Features.Security.Authorization; +using Microsoft.Health.Fhir.Core.Configs; using Microsoft.Health.Fhir.Core.Exceptions; using Microsoft.Health.Fhir.Core.Extensions; using Microsoft.Health.Fhir.Core.Features.Conformance; @@ -31,6 +33,7 @@ public class ConditionalDeleteResourceHandler : BaseResourceHandler, IRequestHan private readonly ISearchService _searchService; private readonly IDeletionService _deleter; private readonly RequestContextAccessor _fhirContext; + private readonly CoreFeatureConfiguration _configuration; private readonly ILogger _logger; public ConditionalDeleteResourceHandler( @@ -43,17 +46,20 @@ public ConditionalDeleteResourceHandler( IAuthorizationService authorizationService, IDeletionService deleter, RequestContextAccessor fhirContext, + IOptions configuration, ILogger logger) : base(fhirDataStore, conformanceProvider, resourceWrapperFactory, resourceIdProvider, authorizationService) { EnsureArg.IsNotNull(mediator, nameof(mediator)); EnsureArg.IsNotNull(searchService, nameof(searchService)); EnsureArg.IsNotNull(deleter, nameof(deleter)); + EnsureArg.IsNotNull(configuration.Value, nameof(configuration)); EnsureArg.IsNotNull(logger, nameof(logger)); _searchService = searchService; _deleter = deleter; _fhirContext = fhirContext; + _configuration = configuration.Value; _logger = logger; } @@ -86,33 +92,47 @@ public async Task Handle(ConditionalDeleteResourceReques private async Task DeleteSingleAsync(ConditionalDeleteResourceRequest request, CancellationToken cancellationToken) { - var matchedResults = await _searchService.ConditionalSearchAsync( + var results = await _searchService.ConditionalSearchAsync( request.ResourceType, request.ConditionalParameters, cancellationToken, logger: _logger); - int count = matchedResults.Results.Count; + int count = results.Results.Where(result => result.SearchEntryMode == ValueSets.SearchEntryMode.Match).Count(); + bool tooManyIncludeResults = _fhirContext.RequestContext.BundleIssues.Any(x => string.Equals(x.Diagnostics, Core.Resources.TruncatedIncludeMessage, StringComparison.OrdinalIgnoreCase)); + if (count == 0) { return new DeleteResourceResponse(0); } - else if (count == 1) + else if (count == 1 && !tooManyIncludeResults) { - var result = await _deleter.DeleteAsync( - new DeleteResourceRequest( - request.ResourceType, - matchedResults.Results.First().Resource.ResourceId, - request.DeleteOperation, - bundleResourceContext: request.BundleResourceContext), - cancellationToken); + if (results.Results.Count == 1) + { + var result = await _deleter.DeleteAsync( + new DeleteResourceRequest( + request.ResourceType, + results.Results.First().Resource.ResourceId, + request.DeleteOperation, + bundleResourceContext: request.BundleResourceContext), + cancellationToken); - if (string.IsNullOrWhiteSpace(result.VersionId)) + if (string.IsNullOrWhiteSpace(result.VersionId)) + { + return new DeleteResourceResponse(result); + } + + return new DeleteResourceResponse(result, weakETag: WeakETag.FromVersionId(result.VersionId)); + } + else { - return new DeleteResourceResponse(result); + // Include results were present, use delete multiple to handle them. + return await DeleteMultipleAsync(request, cancellationToken); } - - return new DeleteResourceResponse(result, weakETag: WeakETag.FromVersionId(result.VersionId)); + } + else if (count == 1 && tooManyIncludeResults) + { + throw new BadRequestException(string.Format(CultureInfo.InvariantCulture, Core.Resources.TooManyIncludeResults, _configuration.DefaultIncludeCountPerSearch)); } else { @@ -124,7 +144,7 @@ private async Task DeleteSingleAsync(ConditionalDeleteRe private async Task DeleteMultipleAsync(ConditionalDeleteResourceRequest request, CancellationToken cancellationToken) { - long numDeleted = await _deleter.DeleteMultipleAsync(request, cancellationToken); + long numDeleted = (await _deleter.DeleteMultipleAsync(request, cancellationToken)).Sum(result => result.Value); return new DeleteResourceResponse((int)numDeleted); } } diff --git a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs index 88f43c7320..6672a8a49d 100644 --- a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs +++ b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs @@ -111,17 +111,17 @@ public async Task DeleteAsync(DeleteResourceRequest request, Cancel return new ResourceKey(key.ResourceType, key.Id, version); } - public async Task DeleteMultipleAsync(ConditionalDeleteResourceRequest request, CancellationToken cancellationToken) + public async Task> DeleteMultipleAsync(ConditionalDeleteResourceRequest request, CancellationToken cancellationToken) { EnsureArg.IsNotNull(request, nameof(request)); var searchCount = 1000; - IReadOnlyCollection matchedResults; + IReadOnlyCollection results; string ct; using (var searchService = _searchServiceFactory.Invoke()) { - (matchedResults, ct) = await searchService.Value.ConditionalSearchAsync( + (results, ct) = await searchService.Value.ConditionalSearchAsync( request.ResourceType, request.ConditionalParameters, cancellationToken, @@ -131,19 +131,19 @@ public async Task DeleteMultipleAsync(ConditionalDeleteResourceRequest req logger: _logger); } - long numDeleted = 0; + Dictionary resourceTypesDeleted = new Dictionary(); long numQueuedForDeletion = 0; - var deleteTasks = new List>(); + var deleteTasks = new List>>(); using var cancellationTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); // Delete the matched results... try { - while (matchedResults.Any() || !string.IsNullOrEmpty(ct)) + while (results.Any() || !string.IsNullOrEmpty(ct)) { IReadOnlyCollection resultsToDelete = - request.DeleteAll ? matchedResults : matchedResults.Take(Math.Max((int)(request.MaxDeleteCount.GetValueOrDefault() - numQueuedForDeletion), 0)).ToArray(); + request.DeleteAll ? results : results.Take(Math.Max((int)(request.MaxDeleteCount.GetValueOrDefault() - numQueuedForDeletion), 0)).ToArray(); numQueuedForDeletion += resultsToDelete.Count; @@ -161,7 +161,8 @@ public async Task DeleteMultipleAsync(ConditionalDeleteResourceRequest req break; } - numDeleted += deleteTasks.Where(x => x.IsCompletedSuccessfully).Sum(x => x.Result); + resourceTypesDeleted = AppendDeleteResults(resourceTypesDeleted, deleteTasks.Where(x => x.IsCompletedSuccessfully).Select(task => task.Result)); + deleteTasks = deleteTasks.Where(task => !task.IsCompletedSuccessfully).ToList(); if (deleteTasks.Count >= MaxParallelThreads) @@ -173,7 +174,7 @@ public async Task DeleteMultipleAsync(ConditionalDeleteResourceRequest req { using (var searchService = _searchServiceFactory.Invoke()) { - (matchedResults, ct) = await searchService.Value.ConditionalSearchAsync( + (results, ct) = await searchService.Value.ConditionalSearchAsync( request.ResourceType, request.ConditionalParameters, cancellationToken, @@ -214,17 +215,24 @@ public async Task DeleteMultipleAsync(ConditionalDeleteResourceRequest req _logger.LogError(ex, "Error deleting"); } - numDeleted += deleteTasks.Where(x => x.IsCompletedSuccessfully).Sum(x => x.Result); + resourceTypesDeleted = AppendDeleteResults(resourceTypesDeleted, deleteTasks.Where(x => x.IsCompletedSuccessfully).Select(task => task.Result)); if (deleteTasks.Any((task) => task.IsFaulted || task.IsCanceled)) { var exceptions = new List(); - deleteTasks.Where((task) => task.IsFaulted || task.IsCanceled).ToList().ForEach((Task result) => + deleteTasks.Where((task) => task.IsFaulted || task.IsCanceled).ToList().ForEach((Task> result) => { if (result.Exception != null) { // Count the number of resources deleted before the exception was thrown. Update the total. - result.Exception.InnerExceptions.Where((ex) => ex is IncompleteOperationException).ToList().ForEach((ex) => numDeleted += ((IncompleteOperationException)ex).PartialResults); + if (result.Exception.InnerExceptions.Any(ex => ex is IncompleteOperationException>)) + { + AppendDeleteResults( + resourceTypesDeleted, + result.Exception.InnerExceptions.Where((ex) => ex is IncompleteOperationException>) + .Select(ex => ((IncompleteOperationException>)ex).PartialResults)); + } + if (result.IsFaulted) { // Filter out noise from the cancellation exceptions caused by the core exception. @@ -233,15 +241,15 @@ public async Task DeleteMultipleAsync(ConditionalDeleteResourceRequest req } }); var aggregateException = new AggregateException(exceptions); - throw new IncompleteOperationException(aggregateException, numDeleted); + throw new IncompleteOperationException>(aggregateException, resourceTypesDeleted); } - return numDeleted; + return resourceTypesDeleted; } - private async Task SoftDeleteResourcePage(ConditionalDeleteResourceRequest request, IReadOnlyCollection resourcesToDelete, CancellationToken cancellationToken) + private async Task> SoftDeleteResourcePage(ConditionalDeleteResourceRequest request, IReadOnlyCollection resourcesToDelete, CancellationToken cancellationToken) { - await CreateAuditLog(request.ResourceType, request.DeleteOperation, false, resourcesToDelete.Select((item) => item.Resource.ResourceId)); + await CreateAuditLog(request.ResourceType, request.DeleteOperation, false, resourcesToDelete.Select((item) => (item.Resource.ResourceTypeName, item.Resource.ResourceId))); bool keepHistory = await _conformanceProvider.Value.CanKeepHistory(request.ResourceType, cancellationToken); ResourceWrapperOperation[] softDeletes = resourcesToDelete.Select(item => @@ -259,22 +267,24 @@ private async Task SoftDeleteResourcePage(ConditionalDeleteResourceRequest { _logger.LogError(ex.InnerException, "Error soft deleting"); - var ids = ex.PartialResults.Select(item => item.Key.Id); + var ids = ex.PartialResults.Select(item => (item.Key.ResourceType, item.Key.Id)); await CreateAuditLog(request.ResourceType, request.DeleteOperation, true, ids); - throw; + throw new IncompleteOperationException>( + ex.InnerException, + ids.GroupBy(pair => pair.ResourceType).ToDictionary(group => group.Key, group => (long)group.Count())); } - await CreateAuditLog(request.ResourceType, request.DeleteOperation, true, resourcesToDelete.Select((item) => item.Resource.ResourceId)); + await CreateAuditLog(request.ResourceType, request.DeleteOperation, true, resourcesToDelete.Select((item) => (item.Resource.ResourceTypeName, item.Resource.ResourceId))); - return resourcesToDelete.Count; + return resourcesToDelete.GroupBy(x => x.Resource.ResourceTypeName).ToDictionary(x => x.Key, x => (long)x.Count()); } - private async Task HardDeleteResourcePage(ConditionalDeleteResourceRequest request, IReadOnlyCollection resourcesToDelete, CancellationToken cancellationToken) + private async Task> HardDeleteResourcePage(ConditionalDeleteResourceRequest request, IReadOnlyCollection resourcesToDelete, CancellationToken cancellationToken) { - await CreateAuditLog(request.ResourceType, request.DeleteOperation, false, resourcesToDelete.Select((item) => item.Resource.ResourceId)); + await CreateAuditLog(request.ResourceType, request.DeleteOperation, false, resourcesToDelete.Select((item) => (item.Resource.ResourceTypeName, item.Resource.ResourceId))); - var parallelBag = new ConcurrentBag(); + var parallelBag = new ConcurrentBag<(string, string)>(); try { using var fhirDataStore = _fhirDataStoreFactory.Invoke(); @@ -283,18 +293,18 @@ private async Task HardDeleteResourcePage(ConditionalDeleteResourceRequest await Parallel.ForEachAsync(resourcesToDelete, cancellationToken, async (item, innerCt) => { await _retryPolicy.ExecuteAsync(async () => await fhirDataStore.Value.HardDeleteAsync(new ResourceKey(item.Resource.ResourceTypeName, item.Resource.ResourceId), request.DeleteOperation == DeleteOperation.PurgeHistory, request.AllowPartialSuccess, innerCt)); - parallelBag.Add(item.Resource.ResourceId); + parallelBag.Add((item.Resource.ResourceTypeName, item.Resource.ResourceId)); }); } catch (Exception ex) { await CreateAuditLog(request.ResourceType, request.DeleteOperation, true, parallelBag); - throw new IncompleteOperationException(ex, parallelBag.Count); + throw new IncompleteOperationException>(ex, parallelBag.GroupBy(x => x.Item1).ToDictionary(x => x.Key, x => (long)x.Count())); } await CreateAuditLog(request.ResourceType, request.DeleteOperation, true, parallelBag); - return parallelBag.Count; + return parallelBag.GroupBy(x => x.Item1).ToDictionary(x => x.Key, x => (long)x.Count()); } private ResourceWrapper CreateSoftDeletedWrapper(string resourceType, string resourceId) @@ -314,7 +324,7 @@ private ResourceWrapper CreateSoftDeletedWrapper(string resourceType, string res return deletedWrapper; } - private System.Threading.Tasks.Task CreateAuditLog(string resourceType, DeleteOperation operation, bool complete, IEnumerable items, HttpStatusCode statusCode = HttpStatusCode.OK) + private System.Threading.Tasks.Task CreateAuditLog(string primaryResourceType, DeleteOperation operation, bool complete, IEnumerable<(string resourceType, string resourceId)> items, HttpStatusCode statusCode = HttpStatusCode.OK) { var auditTask = System.Threading.Tasks.Task.Run(() => { @@ -322,16 +332,17 @@ private System.Threading.Tasks.Task CreateAuditLog(string resourceType, DeleteOp var context = _contextAccessor.RequestContext; var deleteAdditionalProperties = new Dictionary(); deleteAdditionalProperties["Affected Items"] = items.Aggregate( + string.Empty, (aggregate, item) => { - aggregate += ", " + item; + aggregate += ", " + (string.Equals(item.resourceType, primaryResourceType, StringComparison.OrdinalIgnoreCase) ? string.Empty : "[Include] ") + item.resourceType + "/" + item.resourceId; return aggregate; }); _auditLogger.LogAudit( auditAction: action, operation: operation.ToString(), - resourceType: resourceType, + resourceType: primaryResourceType, requestUri: context.Uri, statusCode: statusCode, correlationId: context.CorrelationId, @@ -345,5 +356,21 @@ private System.Threading.Tasks.Task CreateAuditLog(string resourceType, DeleteOp return auditTask; } + + private static Dictionary AppendDeleteResults(Dictionary results, IEnumerable> newResults) + { + foreach (var newResult in newResults) + { + foreach (var (key, value) in newResult) + { + if (!results.TryAdd(key, value)) + { + results[key] += value; + } + } + } + + return results; + } } } From 0a5c5d13dbe5c6ac42b2ef92744635723b33d158 Mon Sep 17 00:00:00 2001 From: Robert Johnson Date: Mon, 3 Feb 2025 14:52:27 -0800 Subject: [PATCH 03/15] Add tests --- .../Resources/ResourceReferenceResolver.cs | 4 +- .../Rest/BulkDeleteTests.cs | 94 ++++++++++++++++++- 2 files changed, 94 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/ResourceReferenceResolver.cs b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/ResourceReferenceResolver.cs index 1f639735ca..d71dd7e21f 100644 --- a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/ResourceReferenceResolver.cs +++ b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/ResourceReferenceResolver.cs @@ -70,12 +70,12 @@ public async Task ResolveReferencesAsync(Resource resource, IDictionary result.SearchEntryMode == ValueSets.SearchEntryMode.Match).Count() != 1) { throw new RequestNotValidException(string.Format(Core.Resources.InvalidConditionalReference, reference.Reference)); } - string resourceId = results.First().Resource.ResourceId; + string resourceId = results.First(result => result.SearchEntryMode == ValueSets.SearchEntryMode.Match).Resource.ResourceId; referenceIdDictionary.Add(reference.Reference, (resourceId, resourceType)); diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs index 6e62dc59d5..401c74cc88 100644 --- a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs +++ b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs @@ -8,7 +8,9 @@ using System.Linq; using System.Net; using System.Net.Http; +using System.Threading.Tasks; using Hl7.Fhir.Model; +using IdentityServer4.Models; using Microsoft.AspNetCore.WebUtilities; using Microsoft.Health.Fhir.Client; using Microsoft.Health.Fhir.Core.Features; @@ -38,7 +40,7 @@ public BulkDeleteTests(HttpIntegrationTestFixture fixture) _fhirClient = fixture.TestFhirClient; } - [Fact(Skip = "Fails transiently. It will be re-enabled after adding delay")] + [SkippableFact] public async Task GivenVariousResourcesOfDifferentTypes_WhenBulkDeleted_ThenAllAreDeleted() { CheckBulkDeleteEnabled(); @@ -84,7 +86,7 @@ public async Task GivenBulkDeleteRequestWithInvalidSearchParameters_WhenRequeste Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); } - [Fact(Skip = "Fails transiently. It will be re-enabled after adding delay")] + [SkippableFact] public async Task GivenSoftBulkDeleteRequest_WhenCompleted_ThenHistoricalRecordsExist() { CheckBulkDeleteEnabled(); @@ -177,6 +179,92 @@ public async Task GivenPurgeBulkDeleteRequest_WhenCompleted_ThenHistoricalRecord Assert.Equal(resource.VersionId, current.Resource.VersionId); } + [SkippableFact] + public async Task GivenBulkDeleteJobWithIncludeSearch_WhenCompleted_ThenIncludedResourcesAreDeleted() + { + CheckBulkDeleteEnabled(); + + var resourceTypes = new Dictionary() + { + { "Patient", 1 }, + { "Observation", 1 }, + }; + + string tag = Guid.NewGuid().ToString(); + var patient = (await _fhirClient.CreateResourcesAsync(1, tag)).FirstOrDefault(); + + var observation = Activator.CreateInstance(); + observation.Meta = new Meta() + { + Tag = new List + { + new Coding("testTag", tag), + }, + }; + observation.Subject = new ResourceReference("Patient/" + patient.Id); + observation.Status = ObservationStatus.Final; + observation.Code = new CodeableConcept("test", "test"); + + await _fhirClient.CreateAsync(observation); + + await Task.Delay(5000); // Add delay to ensure resources are created before bulk delete + + using HttpRequestMessage request = GenerateBulkDeleteRequest( + tag, + "Observation/$bulk-delete", + queryParams: new Dictionary + { + { "_include", "Observation:subject" }, + }); + + using HttpResponseMessage response = await _httpClient.SendAsync(request); + Assert.Equal(HttpStatusCode.Accepted, response.StatusCode); + await MonitorBulkDeleteJob(response.Content.Headers.ContentLocation, resourceTypes); + } + + [SkippableFact] + public async Task GivenBulkDeleteJobWithRevincludeSearch_WhenCompleted_ThenIncludedResourcesAreDeleted() + { + CheckBulkDeleteEnabled(); + + var resourceTypes = new Dictionary() + { + { "Patient", 1 }, + { "Observation", 1 }, + }; + + string tag = Guid.NewGuid().ToString(); + var patient = (await _fhirClient.CreateResourcesAsync(1, tag)).FirstOrDefault(); + + var observation = Activator.CreateInstance(); + observation.Meta = new Meta() + { + Tag = new List + { + new Coding("testTag", tag), + }, + }; + observation.Subject = new ResourceReference("Patient/" + patient.Id); + observation.Status = ObservationStatus.Final; + observation.Code = new CodeableConcept("test", "test"); + + await _fhirClient.CreateAsync(observation); + + await Task.Delay(5000); // Add delay to ensure resources are created before bulk delete + + using HttpRequestMessage request = GenerateBulkDeleteRequest( + tag, + "Patient/$bulk-delete", + queryParams: new Dictionary + { + { "_revinclude", "Observation:subject" }, + }); + + using HttpResponseMessage response = await _httpClient.SendAsync(request); + Assert.Equal(HttpStatusCode.Accepted, response.StatusCode); + await MonitorBulkDeleteJob(response.Content.Headers.ContentLocation, resourceTypes); + } + private async Task RunBulkDeleteRequest( Dictionary expectedResults, bool addUndeletedResource = false, @@ -194,6 +282,8 @@ private async Task RunBulkDeleteRequest( await _fhirClient.CreateResourcesAsync(ModelInfoProvider.GetTypeForFhirType(key), (int)expectedResults[key], tag); } + await Task.Delay(5000); // Add delay to ensure resources are created before bulk delete + using HttpRequestMessage request = GenerateBulkDeleteRequest(tag, path, queryParams); using HttpResponseMessage response = await _httpClient.SendAsync(request); From a27bef0336467b362f827780639496f8ca644995 Mon Sep 17 00:00:00 2001 From: Robert Johnson Date: Tue, 4 Feb 2025 07:47:31 -0800 Subject: [PATCH 04/15] Add more match filters --- .../Features/Resources/ConditionalResourceHandler.cs | 5 +++-- .../Features/Resources/ResourceReferenceResolver.cs | 7 ++++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/ConditionalResourceHandler.cs b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/ConditionalResourceHandler.cs index afbfa1ba5a..6708110997 100644 --- a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/ConditionalResourceHandler.cs +++ b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/ConditionalResourceHandler.cs @@ -60,7 +60,8 @@ public async Task Handle(TRequest request, CancellationToken cancella cancellationToken, logger: _logger); - int matchCount = results.Results.Where(result => result.SearchEntryMode == ValueSets.SearchEntryMode.Match).Count(); + var matches = results.Results.Where(result => result.SearchEntryMode == ValueSets.SearchEntryMode.Match).ToList(); + int matchCount = matches.Count; if (matchCount == 0) { _logger.LogInformation("Conditional handler: Not Match. ResourceType={ResourceType}", request.ResourceType); @@ -69,7 +70,7 @@ public async Task Handle(TRequest request, CancellationToken cancella else if (matchCount == 1) { _logger.LogInformation("Conditional handler: One Match Found. ResourceType={ResourceType}", request.ResourceType); - return await HandleSingleMatch(request, results.Results.First(), cancellationToken); + return await HandleSingleMatch(request, matches.First(), cancellationToken); } else { diff --git a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/ResourceReferenceResolver.cs b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/ResourceReferenceResolver.cs index d71dd7e21f..0b9d083163 100644 --- a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/ResourceReferenceResolver.cs +++ b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/ResourceReferenceResolver.cs @@ -96,7 +96,12 @@ public async Task> GetExistingResourceId( var searchResourceRequest = new SearchResourceRequest(resourceType, conditionalParameters); - return (await _searchService.ConditionalSearchAsync(searchResourceRequest.ResourceType, searchResourceRequest.Queries, cancellationToken, logger: _logger)).Results; + var matches = (await _searchService.ConditionalSearchAsync(searchResourceRequest.ResourceType, searchResourceRequest.Queries, cancellationToken, logger: _logger)) + .Results + .Where(result => result.SearchEntryMode == ValueSets.SearchEntryMode.Match) + .ToList(); + + return matches; } } } From a779ae6492f999ffa49eeb42745d2e308a3a18c8 Mon Sep 17 00:00:00 2001 From: Robert Johnson Date: Thu, 6 Feb 2025 12:22:49 -0800 Subject: [PATCH 05/15] Add too many includes check in multi delete --- .../Resources/Delete/DeletionService.cs | 36 ++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs index 6672a8a49d..59a6589605 100644 --- a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs +++ b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs @@ -7,6 +7,7 @@ using System.Collections.Concurrent; using System.Collections.Generic; using System.Diagnostics; +using System.Globalization; using System.Linq; using System.Net; using System.Security.Cryptography; @@ -18,9 +19,12 @@ using Hl7.Fhir.Serialization; using Microsoft.Build.Framework; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; using Microsoft.Health.Abstractions.Exceptions; using Microsoft.Health.Core.Features.Audit; +using Microsoft.Health.Core.Features.Context; using Microsoft.Health.Extensions.DependencyInjection; +using Microsoft.Health.Fhir.Core.Configs; using Microsoft.Health.Fhir.Core.Exceptions; using Microsoft.Health.Fhir.Core.Extensions; using Microsoft.Health.Fhir.Core.Features.Audit; @@ -46,6 +50,8 @@ public class DeletionService : IDeletionService private readonly AsyncRetryPolicy _retryPolicy; private readonly FhirRequestContextAccessor _contextAccessor; private readonly IAuditLogger _auditLogger; + private readonly RequestContextAccessor _fhirContext; + private readonly CoreFeatureConfiguration _configuration; private readonly ILogger _logger; internal const string DefaultCallerAgent = "Microsoft.Health.Fhir.Server"; @@ -59,6 +65,8 @@ public DeletionService( ResourceIdProvider resourceIdProvider, FhirRequestContextAccessor contextAccessor, IAuditLogger auditLogger, + RequestContextAccessor fhirContext, + IOptions configuration, ILogger logger) { _resourceWrapperFactory = EnsureArg.IsNotNull(resourceWrapperFactory, nameof(resourceWrapperFactory)); @@ -69,6 +77,8 @@ public DeletionService( _contextAccessor = EnsureArg.IsNotNull(contextAccessor, nameof(contextAccessor)); _auditLogger = EnsureArg.IsNotNull(auditLogger, nameof(auditLogger)); _logger = EnsureArg.IsNotNull(logger, nameof(logger)); + _fhirContext = EnsureArg.IsNotNull(fhirContext, nameof(fhirContext)); + _configuration = EnsureArg.IsNotNull(configuration.Value, nameof(configuration)); _retryPolicy = Policy .Handle() @@ -116,6 +126,7 @@ public async Task> DeleteMultipleAsync(ConditionalDele EnsureArg.IsNotNull(request, nameof(request)); var searchCount = 1000; + bool tooManyIncludeResults = false; IReadOnlyCollection results; string ct; @@ -137,6 +148,12 @@ public async Task> DeleteMultipleAsync(ConditionalDele var deleteTasks = new List>>(); using var cancellationTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + if (CheckFhirContextIssues()) + { + var innerException = new BadRequestException(string.Format(CultureInfo.InvariantCulture, Core.Resources.TooManyIncludeResults, _configuration.DefaultIncludeCountPerSearch)); + throw new IncompleteOperationException>(innerException, resourceTypesDeleted); + } + // Delete the matched results... try { @@ -184,6 +201,12 @@ public async Task> DeleteMultipleAsync(ConditionalDele onlyIds: true, logger: _logger); } + + if (CheckFhirContextIssues()) + { + tooManyIncludeResults = true; + break; + } } else { @@ -217,9 +240,15 @@ public async Task> DeleteMultipleAsync(ConditionalDele resourceTypesDeleted = AppendDeleteResults(resourceTypesDeleted, deleteTasks.Where(x => x.IsCompletedSuccessfully).Select(task => task.Result)); - if (deleteTasks.Any((task) => task.IsFaulted || task.IsCanceled)) + if (deleteTasks.Any((task) => task.IsFaulted || task.IsCanceled) || tooManyIncludeResults) { var exceptions = new List(); + + if (tooManyIncludeResults) + { + exceptions.Add(new BadRequestException(string.Format(CultureInfo.InvariantCulture, Core.Resources.TooManyIncludeResults, _configuration.DefaultIncludeCountPerSearch))); + } + deleteTasks.Where((task) => task.IsFaulted || task.IsCanceled).ToList().ForEach((Task> result) => { if (result.Exception != null) @@ -357,6 +386,11 @@ private System.Threading.Tasks.Task CreateAuditLog(string primaryResourceType, D return auditTask; } + private bool CheckFhirContextIssues() + { + return _fhirContext.RequestContext.BundleIssues.Any(x => string.Equals(x.Diagnostics, Core.Resources.TruncatedIncludeMessage, StringComparison.OrdinalIgnoreCase)); + } + private static Dictionary AppendDeleteResults(Dictionary results, IEnumerable> newResults) { foreach (var newResult in newResults) From cb4f9056f6d0f2d5002e87326628f36dddfb886d Mon Sep 17 00:00:00 2001 From: Robert Johnson Date: Fri, 7 Feb 2025 07:41:24 -0800 Subject: [PATCH 06/15] Fix issues with conditional delete and add more tests. --- .../Resources/Delete/DeletionService.cs | 17 +- .../Rest/ConditionalDeleteTests.cs | 156 ++++++++++++++---- 2 files changed, 129 insertions(+), 44 deletions(-) diff --git a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs index 59a6589605..cb75717621 100644 --- a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs +++ b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs @@ -159,18 +159,15 @@ public async Task> DeleteMultipleAsync(ConditionalDele { while (results.Any() || !string.IsNullOrEmpty(ct)) { - IReadOnlyCollection resultsToDelete = - request.DeleteAll ? results : results.Take(Math.Max((int)(request.MaxDeleteCount.GetValueOrDefault() - numQueuedForDeletion), 0)).ToArray(); - - numQueuedForDeletion += resultsToDelete.Count; + numQueuedForDeletion += results.Where(result => result.SearchEntryMode == ValueSets.SearchEntryMode.Match).Count(); if (request.DeleteOperation == DeleteOperation.SoftDelete) { - deleteTasks.Add(SoftDeleteResourcePage(request, resultsToDelete, cancellationTokenSource.Token)); + deleteTasks.Add(SoftDeleteResourcePage(request, results, cancellationTokenSource.Token)); } else { - deleteTasks.Add(HardDeleteResourcePage(request, resultsToDelete, cancellationTokenSource.Token)); + deleteTasks.Add(HardDeleteResourcePage(request, results, cancellationTokenSource.Token)); } if (deleteTasks.Any((task) => task.IsFaulted || task.IsCanceled)) @@ -280,12 +277,12 @@ private async Task> SoftDeleteResourcePage(ConditionalD { await CreateAuditLog(request.ResourceType, request.DeleteOperation, false, resourcesToDelete.Select((item) => (item.Resource.ResourceTypeName, item.Resource.ResourceId))); - bool keepHistory = await _conformanceProvider.Value.CanKeepHistory(request.ResourceType, cancellationToken); - ResourceWrapperOperation[] softDeletes = resourcesToDelete.Select(item => + ResourceWrapperOperation[] softDeletes = await Task.WhenAll(resourcesToDelete.Select(async item => { - ResourceWrapper deletedWrapper = CreateSoftDeletedWrapper(request.ResourceType, item.Resource.ResourceId); + bool keepHistory = await _conformanceProvider.Value.CanKeepHistory(item.Resource.ResourceTypeName, cancellationToken); + ResourceWrapper deletedWrapper = CreateSoftDeletedWrapper(item.Resource.ResourceTypeName, item.Resource.ResourceId); return new ResourceWrapperOperation(deletedWrapper, true, keepHistory, null, false, false, bundleResourceContext: request.BundleResourceContext); - }).ToArray(); + })); try { diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/ConditionalDeleteTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/ConditionalDeleteTests.cs index 617b088b3b..fd59a502e2 100644 --- a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/ConditionalDeleteTests.cs +++ b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/ConditionalDeleteTests.cs @@ -41,7 +41,7 @@ public ConditionalDeleteTests(HttpIntegrationTestFixture fixture) [Trait(Traits.Priority, Priority.One)] public async Task GivenAnIncompleteSearchParam_WhenDeletingConditionally_TheServerRespondsWithCorrectMessage() { - FhirClientException fhirException = await Assert.ThrowsAsync(() => _client.DeleteAsync($"{_resourceType}?identifier=", CancellationToken.None)); + FhirClientException fhirException = await Assert.ThrowsAsync(() => _client.DeleteAsync($"{_resourceType}?_tag=", CancellationToken.None)); Assert.Equal(HttpStatusCode.PreconditionFailed, fhirException.StatusCode); Assert.Equal(fhirException.Response.Resource.Issue[0].Diagnostics, string.Format(Core.Resources.ConditionalOperationNotSelectiveEnough, _resourceType)); } @@ -52,10 +52,10 @@ public async Task GivenAnIncompleteSearchParam_WhenDeletingConditionally_TheServ [Trait(Traits.Priority, Priority.One)] public async Task GivenNoExistingResources_WhenDeletingConditionally_TheServerShouldReturnAccepted(int deleteCount) { - var identifier = Guid.NewGuid().ToString(); - await ValidateResults(identifier, 0); + var tag = Guid.NewGuid().ToString(); + await ValidateResults(tag, 0); - FhirResponse response = await _client.DeleteAsync($"{_resourceType}?identifier={identifier}&_count={deleteCount}", CancellationToken.None); + FhirResponse response = await _client.DeleteAsync($"{_resourceType}?_tag={tag}&_count={deleteCount}", CancellationToken.None); Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); } @@ -67,26 +67,26 @@ public async Task GivenNoExistingResources_WhenDeletingConditionally_TheServerSh [Trait(Traits.Priority, Priority.One)] public async Task GivenOneMatchingResource_WhenDeletingConditionally_TheServerShouldDeleteSuccessfully(string hardDeleteKey, bool hardDeleteValue, int deleteCount) { - var identifier = Guid.NewGuid().ToString(); - await CreateWithIdentifier(identifier); - await ValidateResults(identifier, 1); + var tag = Guid.NewGuid().ToString(); + await CreateWithTag(tag); + await ValidateResults(tag, 1); - FhirResponse response = await _client.DeleteAsync($"{_resourceType}?identifier={identifier}&{hardDeleteKey}={hardDeleteValue}&_count={deleteCount}", CancellationToken.None); + FhirResponse response = await _client.DeleteAsync($"{_resourceType}?_tag={tag}&{hardDeleteKey}={hardDeleteValue}&_count={deleteCount}", CancellationToken.None); Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); - await ValidateResults(identifier, 0); + await ValidateResults(tag, 0); } [Fact] [Trait(Traits.Priority, Priority.One)] public async Task GivenMultipleMatchingResources_WhenDeletingConditionallyInSingleMode_TheServerShouldReturnError() { - var identifier = Guid.NewGuid().ToString(); - await CreateWithIdentifier(identifier); - await CreateWithIdentifier(identifier); - await ValidateResults(identifier, 2); + var tag = Guid.NewGuid().ToString(); + await CreateWithTag(tag); + await CreateWithTag(tag); + await ValidateResults(tag, 2); - await Assert.ThrowsAsync(() => _client.DeleteAsync($"{_resourceType}?identifier={identifier}", CancellationToken.None)); + await Assert.ThrowsAsync(() => _client.DeleteAsync($"{_resourceType}?_tag={tag}", CancellationToken.None)); } [InlineData(-1)] @@ -96,8 +96,8 @@ public async Task GivenMultipleMatchingResources_WhenDeletingConditionallyInSing [Trait(Traits.Priority, Priority.One)] public async Task GivenMultipleMatchingResources_WhenDeletingConditionallyWithOutOfRangeCount_TheServerShouldReturnError(int deleteCount) { - var identifier = Guid.NewGuid().ToString(); - await Assert.ThrowsAsync(() => _client.DeleteAsync($"{_resourceType}?identifier={identifier}&_count={deleteCount}", CancellationToken.None)); + var tag = Guid.NewGuid().ToString(); + await Assert.ThrowsAsync(() => _client.DeleteAsync($"{_resourceType}?_tag={tag}&_count={deleteCount}", CancellationToken.None)); } [InlineData(true)] @@ -106,17 +106,17 @@ public async Task GivenMultipleMatchingResources_WhenDeletingConditionallyWithOu [Trait(Traits.Priority, Priority.One)] public async Task GivenMultipleMatchingResources_WhenDeletingConditionallyWithMultipleFlag_TheServerShouldDeleteSuccessfully(bool hardDelete) { - var identifier = Guid.NewGuid().ToString(); - await CreateWithIdentifier(identifier); - await CreateWithIdentifier(identifier); - await CreateWithIdentifier(identifier); - await ValidateResults(identifier, 3); + var tag = Guid.NewGuid().ToString(); + await CreateWithTag(tag); + await CreateWithTag(tag); + await CreateWithTag(tag); + await ValidateResults(tag, 3); - FhirResponse response = await _client.DeleteAsync($"{_resourceType}?identifier={identifier}&hardDelete={hardDelete}&_count=100", CancellationToken.None); + FhirResponse response = await _client.DeleteAsync($"{_resourceType}?_tag={tag}&hardDelete={hardDelete}&_count=100", CancellationToken.None); Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); Assert.Equal(3, int.Parse(response.Headers.GetValues(KnownHeaders.ItemsDeleted).First())); - await ValidateResults(identifier, 0); + await ValidateResults(tag, 0); } [InlineData(true, 50, 50, 0)] @@ -126,20 +126,66 @@ public async Task GivenMultipleMatchingResources_WhenDeletingConditionallyWithMu [Theory] public async Task GivenMatchingResources_WhenDeletingConditionallyWithMultipleFlag_TheServerShouldDeleteSuccessfully(bool hardDelete, int create, int delete, int expected) { - var identifier = Guid.NewGuid().ToString(); + var tag = Guid.NewGuid().ToString(); - await Task.WhenAll(Enumerable.Range(1, create).Select(_ => CreateWithIdentifier(identifier))); - await ValidateResults(identifier, create); + await Task.WhenAll(Enumerable.Range(1, create).Select(_ => CreateWithTag(tag))); + await ValidateResults(tag, create); - FhirResponse response = await _client.DeleteAsync($"{_resourceType}?identifier={identifier}&hardDelete={hardDelete}&_count={delete}", CancellationToken.None); + FhirResponse response = await _client.DeleteAsync($"{_resourceType}?_tag={tag}&hardDelete={hardDelete}&_count={delete}", CancellationToken.None); Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); Assert.Equal(delete, int.Parse(response.Headers.GetValues(KnownHeaders.ItemsDeleted).First())); - await ValidateResults(identifier, expected); + await ValidateResults(tag, expected); } - private async Task CreateWithIdentifier(string identifier) + [Fact] + public async Task GivenMatchingResources_WhenDeletingConditionallyWithInclude_ThenTheIncludedResourcesAreDeleted() + { + var tag = Guid.NewGuid().ToString(); + await CreateGroupWithPatients(tag, 5); + await ValidateResults(tag, 6); + FhirResponse response = await _client.DeleteAsync($"Group?_tag={tag}&_count=1&_include=Group:member", CancellationToken.None); + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + Assert.Equal(6, int.Parse(response.Headers.GetValues(KnownHeaders.ItemsDeleted).First())); // currently returning 1 for some reason + await ValidateResults(tag, 0); + } + + [Fact] + public async Task GivenMatchingResources_WhenHardDeletingConditionallyWithInclude_ThenTheIncludedResourcesAreDeleted() + { + var tag = Guid.NewGuid().ToString(); + await CreateGroupWithPatients(tag, 5); + await ValidateResults(tag, 6); + FhirResponse response = await _client.DeleteAsync($"Group?hardDelete=true&_tag={tag}&_count=1&_include=Group:member", CancellationToken.None); + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + Assert.Equal(6, int.Parse(response.Headers.GetValues(KnownHeaders.ItemsDeleted).First())); // currently returning 1 for some reason + await ValidateResults(tag, 0); + } + + [Fact] + public async Task GivenMultipleMatchingResources_WhenDeletingConditionallyWithInclude_ThenTheIncludedResourcesAreDeleted() + { + var tag = Guid.NewGuid().ToString(); + await CreateGroupWithPatients(tag, 5); + await CreateGroupWithPatients(tag, 5); + await ValidateResults(tag, 12); + FhirResponse response = await _client.DeleteAsync($"Group?_tag={tag}&_count=2&_include=Group:member", CancellationToken.None); + Assert.Equal(HttpStatusCode.NoContent, response.StatusCode); + Assert.Equal(12, int.Parse(response.Headers.GetValues(KnownHeaders.ItemsDeleted).First())); // currently returning 1 for some reason + await ValidateResults(tag, 0); + } + + [Fact] + public async Task GivenMatchingResources_WhenDeletingConditionallyWithMoreIncludedResourcesThanTheLimit_ThenBadRequestIsRetured() + { + var tag = Guid.NewGuid().ToString(); + await CreateGroupWithPatients(tag, 101); + await ValidateResults(tag, 102); + await Assert.ThrowsAsync(() => _client.DeleteAsync($"Group?_tag={tag}&_count=100&_include=Group:member", CancellationToken.None)); + } + + private async Task CreateWithTag(string tag) { await _createSemaphore.WaitAsync(TimeSpan.FromMinutes(1)); @@ -147,7 +193,8 @@ private async Task CreateWithIdentifier(string identifier) { Encounter encounter = Samples.GetJsonSample("Encounter-For-Patient-f001").ToPoco(); - encounter.Identifier.Add(new Identifier("http://e2etests", identifier)); + encounter.Meta = new Meta(); + encounter.Meta.Tag = new System.Collections.Generic.List { new Coding("http://e2etests", tag) }; using FhirResponse response = await _client.CreateAsync(encounter); Assert.Equal(HttpStatusCode.Created, response.StatusCode); @@ -158,17 +205,58 @@ private async Task CreateWithIdentifier(string identifier) } } - private async Task ValidateResults(string identifier, int expected) + private async Task CreateGroupWithPatients(string tag, int count) + { + await _createSemaphore.WaitAsync(TimeSpan.FromMinutes(1)); + try + { + Bundle createBundle = new Bundle(); + createBundle.Type = Bundle.BundleType.Transaction; + createBundle.Entry = new System.Collections.Generic.List(); + + Group group = new Group(); + group.Member = new System.Collections.Generic.List(); + group.Actual = true; + group.Type = Group.GroupType.Person; + + group.Meta = new Meta(); + group.Meta.Tag = new System.Collections.Generic.List { new Coding("http://e2etests", tag) }; + + for (int i = 0; i < count; i++) + { + var id = Guid.NewGuid(); + var patient = new Patient(); + patient.Meta = new Meta(); + patient.Meta.Tag = new System.Collections.Generic.List { new Coding("http://e2etests", tag) }; + patient.Id = id.ToString(); + + createBundle.Entry.Add(new Bundle.EntryComponent { Resource = patient, Request = new Bundle.RequestComponent { Method = Bundle.HTTPVerb.PUT, Url = $"Patient/{id}" } }); + + group.Member.Add(new Group.MemberComponent { Entity = new ResourceReference($"Patient/{id}") }); + } + + createBundle.Entry.Add(new Bundle.EntryComponent { Resource = group, Request = new Bundle.RequestComponent { Method = Bundle.HTTPVerb.POST, Url = "Group" } }); + + using FhirResponse response = await _client.PostBundleAsync(createBundle); + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + finally + { + _createSemaphore.Release(); + } + } + + private async Task ValidateResults(string tag, int expected) { - var result = await GetResourceCount(identifier); + var result = await GetResourceCount(tag); Assert.Equal(expected, result); } - private async Task GetResourceCount(string identifier) + private async Task GetResourceCount(string tag) { try { - FhirResponse result = await _client.SearchAsync(ResourceType.Encounter, $"identifier=http://e2etests|{identifier}&_summary=count"); + FhirResponse result = await _client.SearchAsync($"?_tag=http://e2etests|{tag}&_summary=count"); return result.Resource.Total; } From 8f1a5f20055011aeb1da082ab9449be7ce1f116d Mon Sep 17 00:00:00 2001 From: Robert Johnson Date: Fri, 7 Feb 2025 07:58:29 -0800 Subject: [PATCH 07/15] Add another test --- .../Rest/BulkDeleteTests.cs | 46 ++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs index 401c74cc88..1553df42b1 100644 --- a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs +++ b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs @@ -265,6 +265,50 @@ public async Task GivenBulkDeleteJobWithRevincludeSearch_WhenCompleted_ThenInclu await MonitorBulkDeleteJob(response.Content.Headers.ContentLocation, resourceTypes); } + [SkippableFact] + public async Task GivenBulkHardDeleteJobWithIncludeSearch_WhenCompleted_ThenIncludedResourcesAreDeleted() + { + CheckBulkDeleteEnabled(); + + var resourceTypes = new Dictionary() + { + { "Patient", 1 }, + { "Observation", 1 }, + }; + + string tag = Guid.NewGuid().ToString(); + var patient = (await _fhirClient.CreateResourcesAsync(1, tag)).FirstOrDefault(); + + var observation = Activator.CreateInstance(); + observation.Meta = new Meta() + { + Tag = new List + { + new Coding("testTag", tag), + }, + }; + observation.Subject = new ResourceReference("Patient/" + patient.Id); + observation.Status = ObservationStatus.Final; + observation.Code = new CodeableConcept("test", "test"); + + await _fhirClient.CreateAsync(observation); + + await Task.Delay(5000); // Add delay to ensure resources are created before bulk delete + + using HttpRequestMessage request = GenerateBulkDeleteRequest( + tag, + "Observation/$bulk-delete", + queryParams: new Dictionary + { + { "_include", "Observation:subject" }, + { KnownQueryParameterNames.BulkHardDelete, "true" }, + }); + + using HttpResponseMessage response = await _httpClient.SendAsync(request); + Assert.Equal(HttpStatusCode.Accepted, response.StatusCode); + await MonitorBulkDeleteJob(response.Content.Headers.ContentLocation, resourceTypes); + } + private async Task RunBulkDeleteRequest( Dictionary expectedResults, bool addUndeletedResource = false, @@ -282,7 +326,7 @@ private async Task RunBulkDeleteRequest( await _fhirClient.CreateResourcesAsync(ModelInfoProvider.GetTypeForFhirType(key), (int)expectedResults[key], tag); } - await Task.Delay(5000); // Add delay to ensure resources are created before bulk delete + await Task.Delay(2000); // Add delay to ensure resources are created before bulk delete using HttpRequestMessage request = GenerateBulkDeleteRequest(tag, path, queryParams); From c9b047b984072498193b17778d44447da65e7c71 Mon Sep 17 00:00:00 2001 From: Robert Johnson Date: Fri, 7 Feb 2025 08:31:29 -0800 Subject: [PATCH 08/15] Fix build --- .../Features/Resources/ResourceHandlerTests.cs | 2 +- .../Features/Resources/Delete/DeletionService.cs | 5 +---- .../Persistence/FhirStorageTestsFixture.cs | 6 +++++- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.Health.Fhir.Shared.Core.UnitTests/Features/Resources/ResourceHandlerTests.cs b/src/Microsoft.Health.Fhir.Shared.Core.UnitTests/Features/Resources/ResourceHandlerTests.cs index 5b256dd37a..ad389acf8d 100644 --- a/src/Microsoft.Health.Fhir.Shared.Core.UnitTests/Features/Resources/ResourceHandlerTests.cs +++ b/src/Microsoft.Health.Fhir.Shared.Core.UnitTests/Features/Resources/ResourceHandlerTests.cs @@ -122,7 +122,7 @@ public ResourceHandlerTests() var auditLogger = Substitute.For(); var logger = Substitute.For>(); - var deleter = new DeletionService(_resourceWrapperFactory, lazyConformanceProvider, _fhirDataStore.CreateMockScopeProvider(), _searchService.CreateMockScopeProvider(), _resourceIdProvider, contextAccessor, auditLogger, logger); + var deleter = new DeletionService(_resourceWrapperFactory, lazyConformanceProvider, _fhirDataStore.CreateMockScopeProvider(), _searchService.CreateMockScopeProvider(), _resourceIdProvider, contextAccessor, auditLogger, new OptionsWrapper(coreFeatureConfiguration), logger); var conditionalCreateLogger = Substitute.For>(); var conditionalUpsertLogger = Substitute.For>(); diff --git a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs index cb75717621..da1db78d77 100644 --- a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs +++ b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs @@ -50,7 +50,6 @@ public class DeletionService : IDeletionService private readonly AsyncRetryPolicy _retryPolicy; private readonly FhirRequestContextAccessor _contextAccessor; private readonly IAuditLogger _auditLogger; - private readonly RequestContextAccessor _fhirContext; private readonly CoreFeatureConfiguration _configuration; private readonly ILogger _logger; @@ -65,7 +64,6 @@ public DeletionService( ResourceIdProvider resourceIdProvider, FhirRequestContextAccessor contextAccessor, IAuditLogger auditLogger, - RequestContextAccessor fhirContext, IOptions configuration, ILogger logger) { @@ -77,7 +75,6 @@ public DeletionService( _contextAccessor = EnsureArg.IsNotNull(contextAccessor, nameof(contextAccessor)); _auditLogger = EnsureArg.IsNotNull(auditLogger, nameof(auditLogger)); _logger = EnsureArg.IsNotNull(logger, nameof(logger)); - _fhirContext = EnsureArg.IsNotNull(fhirContext, nameof(fhirContext)); _configuration = EnsureArg.IsNotNull(configuration.Value, nameof(configuration)); _retryPolicy = Policy @@ -385,7 +382,7 @@ private System.Threading.Tasks.Task CreateAuditLog(string primaryResourceType, D private bool CheckFhirContextIssues() { - return _fhirContext.RequestContext.BundleIssues.Any(x => string.Equals(x.Diagnostics, Core.Resources.TruncatedIncludeMessage, StringComparison.OrdinalIgnoreCase)); + return _contextAccessor.RequestContext.BundleIssues.Any(x => string.Equals(x.Diagnostics, Core.Resources.TruncatedIncludeMessage, StringComparison.OrdinalIgnoreCase)); } private static Dictionary AppendDeleteResults(Dictionary results, IEnumerable> newResults) diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/FhirStorageTestsFixture.cs b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/FhirStorageTestsFixture.cs index a0a2730b63..390c566d60 100644 --- a/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/FhirStorageTestsFixture.cs +++ b/test/Microsoft.Health.Fhir.Shared.Tests.Integration/Persistence/FhirStorageTestsFixture.cs @@ -19,10 +19,12 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; using Microsoft.Health.Abstractions.Features.Transactions; using Microsoft.Health.Core.Features.Context; using Microsoft.Health.Fhir.Api.Features.Bundle; using Microsoft.Health.Fhir.Api.Features.Routing; +using Microsoft.Health.Fhir.Core.Configs; using Microsoft.Health.Fhir.Core.Extensions; using Microsoft.Health.Fhir.Core.Features.Audit; using Microsoft.Health.Fhir.Core.Features.Conformance; @@ -188,10 +190,12 @@ public async Task InitializeAsync() GetResourceHandler = new GetResourceHandler(DataStore, new Lazy(() => ConformanceProvider), resourceWrapperFactory, _resourceIdProvider, _dataResourceFilter, DisabledFhirAuthorizationService.Instance, FhirRequestContextAccessor, SearchService); + var coreFeatureConfiguration = new CoreFeatureConfiguration(); + var auditLogger = Substitute.For(); var logger = Substitute.For>(); - var deleter = new DeletionService(resourceWrapperFactory, new Lazy(() => ConformanceProvider), DataStore.CreateMockScopeProvider(), SearchService.CreateMockScopeProvider(), _resourceIdProvider, new FhirRequestContextAccessor(), auditLogger, logger); + var deleter = new DeletionService(resourceWrapperFactory, new Lazy(() => ConformanceProvider), DataStore.CreateMockScopeProvider(), SearchService.CreateMockScopeProvider(), _resourceIdProvider, new FhirRequestContextAccessor(), auditLogger, new OptionsWrapper(coreFeatureConfiguration), logger); var collection = new ServiceCollection(); From 95605000d331d60b0014900ba36091364f8f871e Mon Sep 17 00:00:00 2001 From: Robert Johnson Date: Fri, 7 Feb 2025 11:20:35 -0800 Subject: [PATCH 09/15] Fix R5 issue --- .../Rest/ConditionalDeleteTests.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/ConditionalDeleteTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/ConditionalDeleteTests.cs index fd59a502e2..0caddafd32 100644 --- a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/ConditionalDeleteTests.cs +++ b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/ConditionalDeleteTests.cs @@ -216,7 +216,11 @@ private async Task CreateGroupWithPatients(string tag, int count) Group group = new Group(); group.Member = new System.Collections.Generic.List(); +#if !R5 group.Actual = true; +#else + group.Membership = Group.GroupMembershipBasis.Enumerated; +#endif group.Type = Group.GroupType.Person; group.Meta = new Meta(); From 0c0839337978bccb7c862093f2169086d11ef701 Mon Sep 17 00:00:00 2001 From: Robert Johnson Date: Fri, 7 Feb 2025 14:27:05 -0800 Subject: [PATCH 10/15] Address edge cases --- .../Resources/Delete/DeletionService.cs | 70 ++++++++++++++++--- 1 file changed, 59 insertions(+), 11 deletions(-) diff --git a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs index da1db78d77..248bbffa1a 100644 --- a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs +++ b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs @@ -272,25 +272,55 @@ public async Task> DeleteMultipleAsync(ConditionalDele private async Task> SoftDeleteResourcePage(ConditionalDeleteResourceRequest request, IReadOnlyCollection resourcesToDelete, CancellationToken cancellationToken) { - await CreateAuditLog(request.ResourceType, request.DeleteOperation, false, resourcesToDelete.Select((item) => (item.Resource.ResourceTypeName, item.Resource.ResourceId))); + await CreateAuditLog( + request.ResourceType, + request.DeleteOperation, + false, + resourcesToDelete.Select((item) => (item.Resource.ResourceTypeName, item.Resource.ResourceId, item.SearchEntryMode == ValueSets.SearchEntryMode.Include))); - ResourceWrapperOperation[] softDeletes = await Task.WhenAll(resourcesToDelete.Select(async item => + ResourceWrapperOperation[] softDeleteMatches = await Task.WhenAll(resourcesToDelete.Where(resource => resource.SearchEntryMode == ValueSets.SearchEntryMode.Match).Select(async item => { bool keepHistory = await _conformanceProvider.Value.CanKeepHistory(item.Resource.ResourceTypeName, cancellationToken); ResourceWrapper deletedWrapper = CreateSoftDeletedWrapper(item.Resource.ResourceTypeName, item.Resource.ResourceId); return new ResourceWrapperOperation(deletedWrapper, true, keepHistory, null, false, false, bundleResourceContext: request.BundleResourceContext); })); + ResourceWrapperOperation[] softDeleteIncludes = await Task.WhenAll(resourcesToDelete.Where(resource => resource.SearchEntryMode == ValueSets.SearchEntryMode.Include).Select(async item => + { + bool keepHistory = await _conformanceProvider.Value.CanKeepHistory(item.Resource.ResourceTypeName, cancellationToken); + ResourceWrapper deletedWrapper = CreateSoftDeletedWrapper(item.Resource.ResourceTypeName, item.Resource.ResourceId); + return new ResourceWrapperOperation(deletedWrapper, true, keepHistory, null, false, false, bundleResourceContext: request.BundleResourceContext); + })); + + var partialResults = new List<(string, string, bool)>(); try { using var fhirDataStore = _fhirDataStoreFactory.Invoke(); - await fhirDataStore.Value.MergeAsync(softDeletes, cancellationToken); + + // Delete includes first so that if there is a failure, the match resources are not deleted. This allows the job to restart. + await fhirDataStore.Value.MergeAsync(softDeleteIncludes, cancellationToken); + partialResults.AddRange(softDeleteIncludes.Select(item => ( + item.Wrapper.ResourceTypeName, + item.Wrapper.ResourceId, + resourcesToDelete + .Where(resource => resource.Resource.ResourceId == item.Wrapper.ResourceId && resource.Resource.ResourceTypeName == item.Wrapper.ResourceTypeName) + .FirstOrDefault().SearchEntryMode == ValueSets.SearchEntryMode.Include))); + + await fhirDataStore.Value.MergeAsync(softDeleteMatches, cancellationToken); } catch (IncompleteOperationException> ex) { _logger.LogError(ex.InnerException, "Error soft deleting"); - var ids = ex.PartialResults.Select(item => (item.Key.ResourceType, item.Key.Id)); + var ids = ex.PartialResults.Select(item => ( + item.Key.ResourceType, + item.Key.Id, + resourcesToDelete + .Where(resource => resource.Resource.ResourceId == item.Key.Id && resource.Resource.ResourceTypeName == item.Key.ResourceType) + .FirstOrDefault().SearchEntryMode == ValueSets.SearchEntryMode.Include)).ToList(); + + ids.AddRange(partialResults); + await CreateAuditLog(request.ResourceType, request.DeleteOperation, true, ids); throw new IncompleteOperationException>( @@ -298,25 +328,43 @@ private async Task> SoftDeleteResourcePage(ConditionalD ids.GroupBy(pair => pair.ResourceType).ToDictionary(group => group.Key, group => (long)group.Count())); } - await CreateAuditLog(request.ResourceType, request.DeleteOperation, true, resourcesToDelete.Select((item) => (item.Resource.ResourceTypeName, item.Resource.ResourceId))); + await CreateAuditLog( + request.ResourceType, + request.DeleteOperation, + true, + resourcesToDelete.Select((item) => (item.Resource.ResourceTypeName, item.Resource.ResourceId, item.SearchEntryMode == ValueSets.SearchEntryMode.Include))); return resourcesToDelete.GroupBy(x => x.Resource.ResourceTypeName).ToDictionary(x => x.Key, x => (long)x.Count()); } private async Task> HardDeleteResourcePage(ConditionalDeleteResourceRequest request, IReadOnlyCollection resourcesToDelete, CancellationToken cancellationToken) { - await CreateAuditLog(request.ResourceType, request.DeleteOperation, false, resourcesToDelete.Select((item) => (item.Resource.ResourceTypeName, item.Resource.ResourceId))); + await CreateAuditLog( + request.ResourceType, + request.DeleteOperation, + false, + resourcesToDelete.Select((item) => (item.Resource.ResourceTypeName, item.Resource.ResourceId, item.SearchEntryMode == ValueSets.SearchEntryMode.Include))); - var parallelBag = new ConcurrentBag<(string, string)>(); + var parallelBag = new ConcurrentBag<(string, string, bool)>(); try { using var fhirDataStore = _fhirDataStoreFactory.Invoke(); + var matchedResources = resourcesToDelete.Where(resource => resource.SearchEntryMode == ValueSets.SearchEntryMode.Match).ToList(); + var includedResources = resourcesToDelete.Where(resource => resource.SearchEntryMode == ValueSets.SearchEntryMode.Include).ToList(); + + // Delete includes first so that if there is a failure, the match resources are not deleted. This allows the job to restart. // This throws AggrigateExceptions - await Parallel.ForEachAsync(resourcesToDelete, cancellationToken, async (item, innerCt) => + await Parallel.ForEachAsync(includedResources, cancellationToken, async (item, innerCt) => + { + await _retryPolicy.ExecuteAsync(async () => await fhirDataStore.Value.HardDeleteAsync(new ResourceKey(item.Resource.ResourceTypeName, item.Resource.ResourceId), request.DeleteOperation == DeleteOperation.PurgeHistory, request.AllowPartialSuccess, innerCt)); + parallelBag.Add((item.Resource.ResourceTypeName, item.Resource.ResourceId, item.SearchEntryMode == ValueSets.SearchEntryMode.Include)); + }); + + await Parallel.ForEachAsync(matchedResources, cancellationToken, async (item, innerCt) => { await _retryPolicy.ExecuteAsync(async () => await fhirDataStore.Value.HardDeleteAsync(new ResourceKey(item.Resource.ResourceTypeName, item.Resource.ResourceId), request.DeleteOperation == DeleteOperation.PurgeHistory, request.AllowPartialSuccess, innerCt)); - parallelBag.Add((item.Resource.ResourceTypeName, item.Resource.ResourceId)); + parallelBag.Add((item.Resource.ResourceTypeName, item.Resource.ResourceId, item.SearchEntryMode == ValueSets.SearchEntryMode.Include)); }); } catch (Exception ex) @@ -347,7 +395,7 @@ private ResourceWrapper CreateSoftDeletedWrapper(string resourceType, string res return deletedWrapper; } - private System.Threading.Tasks.Task CreateAuditLog(string primaryResourceType, DeleteOperation operation, bool complete, IEnumerable<(string resourceType, string resourceId)> items, HttpStatusCode statusCode = HttpStatusCode.OK) + private System.Threading.Tasks.Task CreateAuditLog(string primaryResourceType, DeleteOperation operation, bool complete, IEnumerable<(string resourceType, string resourceId, bool included)> items, HttpStatusCode statusCode = HttpStatusCode.OK) { var auditTask = System.Threading.Tasks.Task.Run(() => { @@ -358,7 +406,7 @@ private System.Threading.Tasks.Task CreateAuditLog(string primaryResourceType, D string.Empty, (aggregate, item) => { - aggregate += ", " + (string.Equals(item.resourceType, primaryResourceType, StringComparison.OrdinalIgnoreCase) ? string.Empty : "[Include] ") + item.resourceType + "/" + item.resourceId; + aggregate += ", " + (item.included ? "[Include] " : string.Empty) + item.resourceType + "/" + item.resourceId; return aggregate; }); From 09cef6da31a9fcdef4a50a30b8bcd47ab7c454fd Mon Sep 17 00:00:00 2001 From: Robert Johnson Date: Fri, 7 Feb 2025 14:53:16 -0800 Subject: [PATCH 11/15] Fix test for Cosmos --- .../Rest/ConditionalDeleteTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/ConditionalDeleteTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/ConditionalDeleteTests.cs index 0caddafd32..0d616dd7b2 100644 --- a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/ConditionalDeleteTests.cs +++ b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/ConditionalDeleteTests.cs @@ -211,7 +211,7 @@ private async Task CreateGroupWithPatients(string tag, int count) try { Bundle createBundle = new Bundle(); - createBundle.Type = Bundle.BundleType.Transaction; + createBundle.Type = Bundle.BundleType.Batch; createBundle.Entry = new System.Collections.Generic.List(); Group group = new Group(); From 27875c6b7e2486dcbcbe6b0188e10088693109e0 Mon Sep 17 00:00:00 2001 From: Robert Johnson Date: Mon, 10 Feb 2025 08:02:08 -0800 Subject: [PATCH 12/15] Fix unit test --- .../Resources/Delete/DeletionService.cs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs index 248bbffa1a..aa978d4be7 100644 --- a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs +++ b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs @@ -298,13 +298,16 @@ await CreateAuditLog( using var fhirDataStore = _fhirDataStoreFactory.Invoke(); // Delete includes first so that if there is a failure, the match resources are not deleted. This allows the job to restart. - await fhirDataStore.Value.MergeAsync(softDeleteIncludes, cancellationToken); - partialResults.AddRange(softDeleteIncludes.Select(item => ( - item.Wrapper.ResourceTypeName, - item.Wrapper.ResourceId, - resourcesToDelete - .Where(resource => resource.Resource.ResourceId == item.Wrapper.ResourceId && resource.Resource.ResourceTypeName == item.Wrapper.ResourceTypeName) - .FirstOrDefault().SearchEntryMode == ValueSets.SearchEntryMode.Include))); + if (softDeleteIncludes.Any()) + { + await fhirDataStore.Value.MergeAsync(softDeleteIncludes, cancellationToken); + partialResults.AddRange(softDeleteIncludes.Select(item => ( + item.Wrapper.ResourceTypeName, + item.Wrapper.ResourceId, + resourcesToDelete + .Where(resource => resource.Resource.ResourceId == item.Wrapper.ResourceId && resource.Resource.ResourceTypeName == item.Wrapper.ResourceTypeName) + .FirstOrDefault().SearchEntryMode == ValueSets.SearchEntryMode.Include))); + } await fhirDataStore.Value.MergeAsync(softDeleteMatches, cancellationToken); } From 8e47037a06e5433453090c9175d4a5fc1003441a Mon Sep 17 00:00:00 2001 From: Robert Johnson Date: Tue, 11 Feb 2025 13:10:51 -0800 Subject: [PATCH 13/15] Address PR comments --- .../Resources/Delete/DeletionService.cs | 6 ++-- .../Rest/BulkDeleteTests.cs | 35 +++++++++++++++++-- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs index aa978d4be7..a62eed4723 100644 --- a/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs +++ b/src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/Delete/DeletionService.cs @@ -145,7 +145,7 @@ public async Task> DeleteMultipleAsync(ConditionalDele var deleteTasks = new List>>(); using var cancellationTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - if (CheckFhirContextIssues()) + if (AreIncludeResultsTruncated()) { var innerException = new BadRequestException(string.Format(CultureInfo.InvariantCulture, Core.Resources.TooManyIncludeResults, _configuration.DefaultIncludeCountPerSearch)); throw new IncompleteOperationException>(innerException, resourceTypesDeleted); @@ -196,7 +196,7 @@ public async Task> DeleteMultipleAsync(ConditionalDele logger: _logger); } - if (CheckFhirContextIssues()) + if (AreIncludeResultsTruncated()) { tooManyIncludeResults = true; break; @@ -431,7 +431,7 @@ private System.Threading.Tasks.Task CreateAuditLog(string primaryResourceType, D return auditTask; } - private bool CheckFhirContextIssues() + private bool AreIncludeResultsTruncated() { return _contextAccessor.RequestContext.BundleIssues.Any(x => string.Equals(x.Diagnostics, Core.Resources.TruncatedIncludeMessage, StringComparison.OrdinalIgnoreCase)); } diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs index 1553df42b1..d064118c62 100644 --- a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs +++ b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs @@ -20,6 +20,7 @@ using Microsoft.Health.Fhir.Tests.E2E.Common; using Microsoft.Health.Test.Utilities; using Xunit; +using static Hl7.Fhir.Model.Encounter; using Task = System.Threading.Tasks.Task; namespace Microsoft.Health.Fhir.Tests.E2E.Rest @@ -188,11 +189,25 @@ public async Task GivenBulkDeleteJobWithIncludeSearch_WhenCompleted_ThenIncluded { { "Patient", 1 }, { "Observation", 1 }, + { "Encounter", 1 }, }; string tag = Guid.NewGuid().ToString(); var patient = (await _fhirClient.CreateResourcesAsync(1, tag)).FirstOrDefault(); + var encounter = Activator.CreateInstance(); + encounter.Meta = new Meta() + { + Tag = new List + { + new Coding("testTag", tag), + }, + }; + encounter.Status = EncounterStatus.Finished; + encounter.Class = new Coding("test", "test"); + + encounter = await _fhirClient.CreateAsync(encounter); + var observation = Activator.CreateInstance(); observation.Meta = new Meta() { @@ -202,6 +217,7 @@ public async Task GivenBulkDeleteJobWithIncludeSearch_WhenCompleted_ThenIncluded }, }; observation.Subject = new ResourceReference("Patient/" + patient.Id); + observation.Encounter = new ResourceReference("Encounter/" + encounter.Id); observation.Status = ObservationStatus.Final; observation.Code = new CodeableConcept("test", "test"); @@ -214,7 +230,7 @@ public async Task GivenBulkDeleteJobWithIncludeSearch_WhenCompleted_ThenIncluded "Observation/$bulk-delete", queryParams: new Dictionary { - { "_include", "Observation:subject" }, + { "_include", "Observation:*" }, }); using HttpResponseMessage response = await _httpClient.SendAsync(request); @@ -231,11 +247,26 @@ public async Task GivenBulkDeleteJobWithRevincludeSearch_WhenCompleted_ThenInclu { { "Patient", 1 }, { "Observation", 1 }, + { "Encounter", 1 }, }; string tag = Guid.NewGuid().ToString(); var patient = (await _fhirClient.CreateResourcesAsync(1, tag)).FirstOrDefault(); + var encounter = Activator.CreateInstance(); + encounter.Meta = new Meta() + { + Tag = new List + { + new Coding("testTag", tag), + }, + }; + encounter.Subject = new ResourceReference("Patient/" + patient.Id); + encounter.Status = EncounterStatus.Finished; + encounter.Class = new Coding("test", "test"); + + await _fhirClient.CreateAsync(encounter); + var observation = Activator.CreateInstance(); observation.Meta = new Meta() { @@ -257,7 +288,7 @@ public async Task GivenBulkDeleteJobWithRevincludeSearch_WhenCompleted_ThenInclu "Patient/$bulk-delete", queryParams: new Dictionary { - { "_revinclude", "Observation:subject" }, + { "_revinclude", "*:*" }, }); using HttpResponseMessage response = await _httpClient.SendAsync(request); From 3dcbb330bface14b64cd4c8d58256bf446f73770 Mon Sep 17 00:00:00 2001 From: Robert Johnson Date: Tue, 11 Feb 2025 14:08:46 -0800 Subject: [PATCH 14/15] Fix test --- .../Rest/BulkDeleteTests.cs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs index d064118c62..dc8a3f9841 100644 --- a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs +++ b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs @@ -181,8 +181,13 @@ public async Task GivenPurgeBulkDeleteRequest_WhenCompleted_ThenHistoricalRecord } [SkippableFact] +#pragma warning disable CS1998 // Async method lacks 'await' operators and will run synchronously public async Task GivenBulkDeleteJobWithIncludeSearch_WhenCompleted_ThenIncludedResourcesAreDeleted() +#pragma warning restore CS1998 // Async method lacks 'await' operators and will run synchronously { +#if Stu3 + Skip.If(true, "Referenced used isn't present in Stu3"); +#else CheckBulkDeleteEnabled(); var resourceTypes = new Dictionary() @@ -203,8 +208,13 @@ public async Task GivenBulkDeleteJobWithIncludeSearch_WhenCompleted_ThenIncluded new Coding("testTag", tag), }, }; - encounter.Status = EncounterStatus.Finished; + encounter.Status = EncounterStatus.Planned; +#if !R5 encounter.Class = new Coding("test", "test"); +#else + encounter.Class = new List(); + encounter.Class.Add(new CodeableConcept("test", "test")); +#endif encounter = await _fhirClient.CreateAsync(encounter); @@ -236,6 +246,7 @@ public async Task GivenBulkDeleteJobWithIncludeSearch_WhenCompleted_ThenIncluded using HttpResponseMessage response = await _httpClient.SendAsync(request); Assert.Equal(HttpStatusCode.Accepted, response.StatusCode); await MonitorBulkDeleteJob(response.Content.Headers.ContentLocation, resourceTypes); +#endif } [SkippableFact] From 7aec9ab965b89ce1d554f11f20df62b89411920b Mon Sep 17 00:00:00 2001 From: Robert Johnson Date: Tue, 11 Feb 2025 14:22:02 -0800 Subject: [PATCH 15/15] Fix test --- .../Rest/BulkDeleteTests.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs index dc8a3f9841..5f6fae390f 100644 --- a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs +++ b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs @@ -273,8 +273,13 @@ public async Task GivenBulkDeleteJobWithRevincludeSearch_WhenCompleted_ThenInclu }, }; encounter.Subject = new ResourceReference("Patient/" + patient.Id); - encounter.Status = EncounterStatus.Finished; + encounter.Status = EncounterStatus.Planned; +#if !R5 encounter.Class = new Coding("test", "test"); +#else + encounter.Class = new List(); + encounter.Class.Add(new CodeableConcept("test", "test")); +#endif await _fhirClient.CreateAsync(encounter);