Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for include to delete requests #4811

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add support for include to delete requests #4811

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
524e340
Allow for delete with include
Feb 3, 2025
f0c7566
Fully support include in deletion service.
Feb 3, 2025
0a5c5d1
Add tests
Feb 3, 2025
a27bef0
Add more match filters
Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions ...Health.Fhir.Core.UnitTests/Features/Operations/BulkDelete/BulkDeleteProcessingJobTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,11 @@ public async Task GivenProcessingJob_WhenJobIsRun_ThenResourcesAreDeleted()
Definition = JsonConvert.SerializeObject(definition),
};

var substituteResults = new Dictionary<string, long>();
substituteResults.Add("Patient", 3);

_deleter.DeleteMultipleAsync(Arg.Any<ConditionalDeleteResourceRequest>(), Arg.Any<CancellationToken>())
.Returns(args => 3);
.Returns(args => substituteResults);

var result = JsonConvert.DeserializeObject<BulkDeleteResult>(await _processingJob.ExecuteAsync(jobInfo, CancellationToken.None));
Assert.Single(result.ResourcesDeleted);
Expand All @@ -80,8 +83,11 @@ public async Task GivenProcessingJob_WhenJobIsRunWithMultipleResourceTypes_ThenF
Definition = JsonConvert.SerializeObject(definition),
};

var substituteResults = new Dictionary<string, long>();
substituteResults.Add("Patient", 3);

_deleter.DeleteMultipleAsync(Arg.Any<ConditionalDeleteResourceRequest>(), Arg.Any<CancellationToken>())
.Returns(args => 3);
.Returns(args => substituteResults);

var result = JsonConvert.DeserializeObject<BulkDeleteResult>(await _processingJob.ExecuteAsync(jobInfo, CancellationToken.None));
Assert.Single(result.ResourcesDeleted);
Expand Down
11 changes: 8 additions & 3 deletions src/Microsoft.Health.Fhir.Core/Extensions/SearchServiceExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ public static class SearchServiceExtensions
KnownQueryParameterNames.Summary,
KnownQueryParameterNames.Total,
KnownQueryParameterNames.ContinuationToken,
"_include",
"_revinclude",
};

/// <summary>
Expand Down Expand Up @@ -70,6 +68,7 @@ public static class SearchServiceExtensions
}

var matchedResults = new List<SearchResultEntry>();
var includeResults = new List<SearchResultEntry>();
string lastContinuationToken = continuationToken;
LongRunningOperationStatistics statistics = new LongRunningOperationStatistics(operationName: "conditionalSearchAsync");
try
Expand Down Expand Up @@ -102,6 +101,11 @@ public static class SearchServiceExtensions
results?.Results
.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));
Expand All @@ -123,7 +127,8 @@ public static class SearchServiceExtensions
}
}

return (matchedResults, lastContinuationToken);
var resultsToReturn = matchedResults.Concat(includeResults).ToList();
return (resultsToReturn, lastContinuationToken);
}
}
}
18 changes: 12 additions & 6 deletions src/Microsoft.Health.Fhir.Core/Features/Operations/BulkDelete/BulkDeleteProcessingJob.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,14 @@

_contextAccessor.RequestContext = fhirRequestContext;
var result = new BulkDeleteResult();
long numDeleted;
IDictionary<string, long> resourcesDeleted = new Dictionary<string, long>();

Check warning

Code scanning / CodeQL

Useless assignment to local variable Warning

This assignment to
resourcesDeleted
Loading
is useless, since its value is never read.
using IScoped<IDeletionService> deleter = _deleterFactory.Invoke();
Exception exception = null;
List<string> types = definition.Type.SplitByOrSeparator().ToList();

try
{
numDeleted = await deleter.Value.DeleteMultipleAsync(
resourcesDeleted = await deleter.Value.DeleteMultipleAsync(
new ConditionalDeleteResourceRequest(
types[0],
(IReadOnlyList<Tuple<string, string>>)definition.SearchParameters,
Expand All @@ -91,16 +91,22 @@
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<long> ex)
catch (IncompleteOperationException<IDictionary<string, long>> 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)
{
Expand Down
2 changes: 1 addition & 1 deletion src/Microsoft.Health.Fhir.Core/Features/Persistence/IDeletionService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,6 @@ public interface IDeletionService
{
public Task<ResourceKey> DeleteAsync(DeleteResourceRequest request, CancellationToken cancellationToken);

public Task<long> DeleteMultipleAsync(ConditionalDeleteResourceRequest request, CancellationToken cancellationToken);
public Task<IDictionary<string, long>> DeleteMultipleAsync(ConditionalDeleteResourceRequest request, CancellationToken cancellationToken);
}
}
9 changes: 9 additions & 0 deletions src/Microsoft.Health.Fhir.Core/Resources.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions src/Microsoft.Health.Fhir.Core/Resources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,4 +757,8 @@
<data name="OperationFailedForCustomerManagedKey" xml:space="preserve">
<value>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.</value>
</data>
<data name="TooManyIncludeResults" xml:space="preserve">
<value>The number of included results exceeded the limit of {0}</value>
<comment>{0} is the limit of how many include results the service will return.</comment>
</data>
</root>
6 changes: 5 additions & 1 deletion src/Microsoft.Health.Fhir.Shared.Core.UnitTests/Features/Resources/ResourceHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -115,6 +117,8 @@ public ResourceHandlerTests()
var referenceResolver = new ResourceReferenceResolver(_searchService, new TestQueryStringParser(), Substitute.For<ILogger<ResourceReferenceResolver>>());
_resourceIdProvider = new ResourceIdProvider();

var coreFeatureConfiguration = new CoreFeatureConfiguration();

var auditLogger = Substitute.For<IAuditLogger>();
var logger = Substitute.For<ILogger<DeletionService>>();

Expand All @@ -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<IMediator>(), _resourceIdProvider, _authorizationService, conditionalCreateLogger)).Singleton().AsSelf().AsImplementedInterfaces();
collection.Add(x => new ConditionalUpsertResourceHandler(_fhirDataStore, lazyConformanceProvider, _resourceWrapperFactory, _searchService, x.GetService<IMediator>(), _resourceIdProvider, _authorizationService, conditionalUpsertLogger)).Singleton().AsSelf().AsImplementedInterfaces();
collection.Add(x => new ConditionalDeleteResourceHandler(_fhirDataStore, lazyConformanceProvider, _resourceWrapperFactory, _searchService, x.GetService<IMediator>(), _resourceIdProvider, _authorizationService, deleter, contextAccessor, conditionalDeleteLogger)).Singleton().AsSelf().AsImplementedInterfaces();
collection.Add(x => new ConditionalDeleteResourceHandler(_fhirDataStore, lazyConformanceProvider, _resourceWrapperFactory, _searchService, x.GetService<IMediator>(), _resourceIdProvider, _authorizationService, deleter, contextAccessor, new OptionsWrapper<CoreFeatureConfiguration>(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();

Expand Down
13 changes: 7 additions & 6 deletions src/Microsoft.Health.Fhir.Shared.Core/Features/Resources/ConditionalResourceHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,27 +54,28 @@ public async Task<TResponse> 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)
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);
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, matches.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));
}
}
Expand Down
50 changes: 35 additions & 15 deletions ...oft.Health.Fhir.Shared.Core/Features/Resources/Delete/ConditionalDeleteResourceHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -31,6 +33,7 @@ public class ConditionalDeleteResourceHandler : BaseResourceHandler, IRequestHan
private readonly ISearchService _searchService;
private readonly IDeletionService _deleter;
private readonly RequestContextAccessor<IFhirRequestContext> _fhirContext;
private readonly CoreFeatureConfiguration _configuration;
private readonly ILogger<ConditionalDeleteResourceHandler> _logger;

public ConditionalDeleteResourceHandler(
Expand All @@ -43,17 +46,20 @@ public ConditionalDeleteResourceHandler(
IAuthorizationService<DataActions> authorizationService,
IDeletionService deleter,
RequestContextAccessor<IFhirRequestContext> fhirContext,
IOptions<CoreFeatureConfiguration> configuration,
ILogger<ConditionalDeleteResourceHandler> 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;
}

Expand Down Expand Up @@ -86,33 +92,47 @@ public async Task<DeleteResourceResponse> Handle(ConditionalDeleteResourceReques

private async Task<DeleteResourceResponse> DeleteSingleAsync(ConditionalDeleteResourceRequest request, CancellationToken cancellationToken)
{
var matchedResults = await _searchService.ConditionalSearchAsync(
var results = await _searchService.ConditionalSearchAsync(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we a limit on the number of included items coming back from ConditionalSearch? i.e. if we delete the primary resource and some of the included items, do some then become orphaned?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The limit is still there. On line 108 there is a check to see if we got the "include results truncated" issue. If we did it returns 408 and says there are too many included results.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to add similar behavior to bulk delete. I'm debating how best to do that so it still deletes as many results as possible. Once we have pagination for include I'll just have it page through the results.

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
{
Expand All @@ -124,7 +144,7 @@ private async Task<DeleteResourceResponse> DeleteSingleAsync(ConditionalDeleteRe

private async Task<DeleteResourceResponse> 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);
}
}
Expand Down
Loading
Loading