Fix $bulk-delete bugs with custom search parameter #4964
Conversation
tarunmathew12
added
Bug
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs
Fixed
Show resolved
Hide resolved
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/BulkDeleteTests.cs
Outdated
Show resolved
Hide resolved
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -146,7 +143,7 @@ private async Task<IDictionary<string, long>> DeleteMultipleAsyncInternal(Condit | |||
| request.DeleteAll ? searchCount : request.MaxDeleteCount, | |||
| continuationToken, | |||
| versionType: request.VersionType, | |||
| onlyIds: true, | |||
| onlyIds: !string.Equals(request.ResourceType, KnownResourceTypes.SearchParameter, StringComparison.OrdinalIgnoreCase), | |||
There was a problem hiding this comment.
This is an edge case, but if this is an Includes request then this won't work. The resource type in the request is still the type of the primary resource, not the included resource. So if the search parameters are returned as part of an includes search then only the ids will be returned.
I don't know of any resource that references search parameters so I don't think this is an edge case worth covering, but I think calling it out in a comment would be good.
| @@ -146,7 +143,7 @@ private async Task<IDictionary<string, long>> DeleteMultipleAsyncInternal(Condit | |||
| request.DeleteAll ? searchCount : request.MaxDeleteCount, | |||
| continuationToken, | |||
| versionType: request.VersionType, | |||
| onlyIds: true, | |||
| onlyIds: !string.Equals(request.ResourceType, KnownResourceTypes.SearchParameter, StringComparison.OrdinalIgnoreCase), | |||
There was a problem hiding this comment.
This is also needed on line 239
|
Has conditional delete been tested to see how it handles deleting search parameters? |
| @@ -316,5 +351,51 @@ private async Task<ITypedElement> GetSearchParameterByUrl(string url, Cancellati | |||
|
|
|||
| return null; | |||
| } | |||
|
|
|||
| private async Task DeleteSearchParametersAsync(List<string> urls, int pageSize, CancellationToken cancellationToken) | |||
There was a problem hiding this comment.
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.
| @@ -174,6 +173,46 @@ public async Task UpdateSearchParameterStatusAsync(IReadOnlyCollection<string> s | |||
| await _mediator.Publish(new SearchParametersUpdatedNotification(updated), cancellationToken); | |||
| } | |||
|
|
|||
| internal async Task UpdateSearchParameterStatusAsync(IReadOnlyCollection<ResourceSearchParameterStatus> searchParameterStatuses, CancellationToken cancellationToken) | |||
There was a problem hiding this comment.
Why is this method needed instead of using the existing version that takes uris?
| Content = CreateNotificationContent(deletedSearchParameters), | ||
| }; | ||
|
|
||
| await _mediator.Publish(notification, cancellationToken); |
There was a problem hiding this comment.
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.
|
|
||
| // Create search parameter resources. | ||
| await CreateAsync(resourcesToCreate); | ||
|
|
There was a problem hiding this comment.
I would add a 1 second delay here. The bulk delete tests have been flaky when we don't wait a bit after resources are created.
| @@ -532,10 +863,11 @@ private HttpRequestMessage GenerateBulkDeleteRequest( | |||
| return request; | |||
| } | |||
|
|
|||
| private async Task MonitorBulkDeleteJob(Uri location, Dictionary<string, long> expectedResults) | |||
| private async Task MonitorBulkDeleteJob(Uri location, Dictionary<string, long> expectedResults, bool failOnIssues = false) | |||
There was a problem hiding this comment.
Why was this method updated? I don't see this new parameter being used anywhere.
Description
The PR will address an issue of the search parameter status going out of sync after $bulk-delete removes search parameter resources.
Related issues
Addresses [issue #121676].
Bug 121676: Hard delete custom search parameter not cleaning up all the data
Testing
Tested manually and also through adding E2E test.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)