-
Notifications
You must be signed in to change notification settings - Fork 541
Fix $bulk-delete bugs with custom search parameter #4964
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
Changes from all commits
aa190d4
62daa03
53acd9b
d4ae388
21b8701
179274e
6917021
4536421
d4a5874
af3484b
ff8250c
19439a2
8e982d9
06beb09
8e54cb9
942c800
95e9407
3bd595d
026b92a
c6067b9
5814c29
ce0000f
835a8af
bb6dee4
cebd7cf
6d65aab
792169b
27088a9
17dc3da
66d833e
ea18303
220dacf
b205575
6c4a4c6
228156d
98a8a1a
5c9c27f
59cb78a
2ec5767
9706ecc
373b00e
5ece706
6144df2
a771700
1feb027
13f5de0
0b0877f
48fe44f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,24 +6,30 @@ | |
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Text.Json; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using EnsureThat; | ||
using Hl7.Fhir.ElementModel; | ||
using Hl7.Fhir.Rest; | ||
using MediatR; | ||
using Microsoft.Extensions.Logging; | ||
using Microsoft.Health.Extensions.DependencyInjection; | ||
using Microsoft.Health.Fhir.Core.Exceptions; | ||
using Microsoft.Health.Fhir.Core.Extensions; | ||
using Microsoft.Health.Fhir.Core.Features.Definition; | ||
using Microsoft.Health.Fhir.Core.Features.Definition.BundleWrappers; | ||
using Microsoft.Health.Fhir.Core.Features.Operations.BulkDelete.Messages; | ||
using Microsoft.Health.Fhir.Core.Features.Persistence; | ||
using Microsoft.Health.Fhir.Core.Features.Search.Registry; | ||
using Microsoft.Health.Fhir.Core.Models; | ||
|
||
namespace Microsoft.Health.Fhir.Core.Features.Search.Parameters | ||
{ | ||
public class SearchParameterOperations : ISearchParameterOperations | ||
public class SearchParameterOperations : ISearchParameterOperations, INotificationHandler<BulkDeleteMetricsNotification> | ||
{ | ||
private const int DefaultDeleteTasksPerPage = 5; | ||
|
||
private readonly ISearchParameterDefinitionManager _searchParameterDefinitionManager; | ||
private readonly SearchParameterStatusManager _searchParameterStatusManager; | ||
private readonly IModelInfoProvider _modelInfoProvider; | ||
|
@@ -283,6 +289,35 @@ | |
cancellationToken); | ||
} | ||
|
||
public async Task Handle(BulkDeleteMetricsNotification notification, CancellationToken cancellationToken) | ||
{ | ||
var content = notification?.Content?.ToString(); | ||
if (!string.IsNullOrWhiteSpace(content)) | ||
{ | ||
try | ||
{ | ||
var urls = JsonSerializer.Deserialize<List<string>>(content); | ||
if (urls.Any(x => !string.IsNullOrWhiteSpace(x))) | ||
{ | ||
await DeleteSearchParametersAsync(urls, DefaultDeleteTasksPerPage, cancellationToken); | ||
} | ||
else | ||
{ | ||
_logger.LogInformation("Ignoring the notification as its content doesn't have any search parameter Uri."); | ||
} | ||
} | ||
catch (JsonException ex) | ||
{ | ||
_logger.LogError(ex, "Failed to deserialize the notification content."); | ||
throw; | ||
} | ||
} | ||
else | ||
{ | ||
_logger.LogInformation("Ignoring the notification as its content is empty."); | ||
} | ||
} | ||
|
||
private void DeleteSearchParameter(string url) | ||
{ | ||
try | ||
|
@@ -316,5 +351,51 @@ | |
|
||
return null; | ||
} | ||
|
||
private async Task DeleteSearchParametersAsync(List<string> urls, int pageSize, CancellationToken cancellationToken) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like how there are a DeleteSearchParameter and DeleteSearchParametersAsync methods in the same class that do different things. Could this method be DeleteSearchParametersFromDatabaseAsync? I'm not thrilled with this, but it helps show the two methods are doing different things. |
||
{ | ||
if (urls?.Any() ?? false) | ||
{ | ||
var urlMap = urls.ToHashSet(StringComparer.OrdinalIgnoreCase); | ||
var allSearchParameterStatuses = await _searchParameterStatusManager.GetAllSearchParameterStatus(cancellationToken); | ||
var searchParameterStatusesToUpdate = allSearchParameterStatuses | ||
.Where(x => urlMap.TryGetValue(x.Uri.OriginalString, out _) && (x.Status != SearchParameterStatus.PendingDelete && x.Status != SearchParameterStatus.Deleted)) | ||
.ToList(); | ||
_logger.LogInformation($"Updating the status of {searchParameterStatusesToUpdate.Count} search parameters (out of {urls.Count} urls) to PendingDelete..."); | ||
var count = 0; | ||
while (count < searchParameterStatusesToUpdate.Count) | ||
{ | ||
var statusesInPage = searchParameterStatusesToUpdate.Skip(count).Take(pageSize).ToList(); | ||
count += statusesInPage.Count; | ||
|
||
var urlsString = string.Join(Environment.NewLine, statusesInPage.Select(x => x.Uri.OriginalString)); | ||
try | ||
{ | ||
statusesInPage.ForEach( | ||
x => | ||
{ | ||
x.Status = SearchParameterStatus.PendingDelete; | ||
x.LastUpdated = Clock.UtcNow; | ||
}); | ||
|
||
_logger.LogInformation($"Updating the status of search parameters:{Environment.NewLine}{urlsString}"); | ||
await _searchParameterStatusManager.UpdateSearchParameterStatusAsync(statusesInPage, cancellationToken); | ||
|
||
statusesInPage.ForEach( | ||
x => | ||
{ | ||
_searchParameterDefinitionManager.UpdateSearchParameterStatus( | ||
x.Uri.OriginalString, | ||
SearchParameterStatus.PendingDelete); | ||
}); | ||
} | ||
catch (Exception ex) | ||
{ | ||
// Note: ignore the exception and continue updating the rest. | ||
_logger.LogError(ex, $"Failed to update the status of search parameters:{Environment.NewLine}{urlsString}"); | ||
} | ||
tarunmathew12 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern with this method of updating the search parameters is that it isn't safe if the service shuts down. If the notification isn't fully handled the DB can be left in a bad state where the Search Parameter resources are deleted, but the Search Parameter table hasn't been updated yet.