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.

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

Merged
merged 16 commits into from
Feb 13, 2025
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
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);
}
}
}
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
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
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

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
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>
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
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
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(
LTA-Thinking marked this conversation as resolved.
Show resolved Hide resolved
mikaelweave marked this conversation as resolved.
Show resolved Hide resolved
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