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

Bulk delete enhancements #3505

Merged
merged 42 commits into from
Jan 4, 2024
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
6bd49c2
Add audit logs to bulk delete
LTA-Thinking Aug 30, 2023
606008a
Increase search result limit
LTA-Thinking Sep 1, 2023
2d6f345
Add more logging
LTA-Thinking Sep 5, 2023
37a0fea
fix logs
LTA-Thinking Sep 5, 2023
b4c04b2
Make audit async
LTA-Thinking Sep 7, 2023
39da7b6
Fix audit log
LTA-Thinking Sep 7, 2023
d0db371
Alter import tool
LTA-Thinking Sep 7, 2023
f4a7343
Add job monitor and verification to fhir client
LTA-Thinking Sep 8, 2023
71bbf05
Add wireframe
LTA-Thinking Sep 11, 2023
9a7644c
Change audit logging again
LTA-Thinking Sep 15, 2023
790b9a6
Fix search
LTA-Thinking Sep 15, 2023
9681e00
Fix tasks to actually start running
LTA-Thinking Sep 18, 2023
bcb377f
Add starting resource count
LTA-Thinking Oct 4, 2023
fbd90ff
Add resource count and better exception handling
LTA-Thinking Oct 6, 2023
47b89ab
Fix test
LTA-Thinking Oct 9, 2023
0bb2e1e
Fix test
LTA-Thinking Oct 9, 2023
9499edb
Fix tests
LTA-Thinking Oct 10, 2023
b0720d3
Fix another test
LTA-Thinking Oct 11, 2023
109b42e
Cleanup tests
LTA-Thinking Oct 12, 2023
e8712da
Add new test
LTA-Thinking Oct 18, 2023
08c6064
Fix test
LTA-Thinking Nov 14, 2023
2d86b65
Disable bulk delete tests
LTA-Thinking Nov 15, 2023
9e00e7d
Merge branch 'main' into personal/rojo/bulk-delete-logging
LTA-Thinking Nov 15, 2023
935b782
Fix resource count
LTA-Thinking Nov 15, 2023
e1a96f8
Add support for soft delete
LTA-Thinking Nov 16, 2023
7048491
Limit threads
LTA-Thinking Dec 1, 2023
f14042b
Merge branch 'main' into personal/rojo/bulk-delete-logging
LTA-Thinking Dec 1, 2023
b8c1985
Fix bugs
LTA-Thinking Dec 1, 2023
79e973b
Update unit tests and fix build errors
LTA-Thinking Dec 5, 2023
a642d64
Fix some build issues
LTA-Thinking Dec 8, 2023
dc9953e
Address PR comments
LTA-Thinking Dec 8, 2023
27ec544
Fix test and add operation definition
LTA-Thinking Dec 8, 2023
f002115
Add sample for bulk delete of soft deleted
LTA-Thinking Dec 8, 2023
abe3d5a
Fix proj file
LTA-Thinking Dec 8, 2023
e4daed9
Add more logging to tests
LTA-Thinking Dec 15, 2023
fe12ec5
Fix logs
LTA-Thinking Dec 15, 2023
897ce6d
Merge with main
LTA-Thinking Dec 28, 2023
ed182c7
Fix Net8 error
LTA-Thinking Dec 28, 2023
e2d9938
Update tests
LTA-Thinking Dec 29, 2023
9bce0ee
Revert FHIR client changes
LTA-Thinking Jan 3, 2024
48cd72a
Fix types in bulk delete tests
LTA-Thinking Jan 3, 2024
c260875
Throw exception on null definition
LTA-Thinking Jan 4, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ internal class KnownRoutes

public const string BulkDelete = "$bulk-delete";
public const string BulkDeleteResourceType = ResourceType + "/" + BulkDelete;
public const string BulkDeleteSoftDeleted = "$bulk-delete-soft-deleted";
mikaelweave marked this conversation as resolved.
Show resolved Hide resolved
mikaelweave marked this conversation as resolved.
Show resolved Hide resolved
public const string BulkDeleteSoftDeletedResourceType = ResourceType + "/" + BulkDeleteSoftDeleted;
public const string BulkDeleteJobLocation = OperationsConstants.Operations + "/" + OperationsConstants.BulkDelete + "/" + IdRouteSegment;
public const string BulkDeleteOperationDefinition = OperationDefinition + "/" + OperationsConstants.BulkDelete;
public const string ResourceTypeBulkDeleteOperationDefinition = OperationDefinition + "/" + OperationsConstants.ResourceTypeBulkDelete;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ public BulkDeleteOrchestratorJobTests()
{
_queueClient = Substitute.For<IQueueClient>();
_searchService = Substitute.For<ISearchService>();
_searchService.SearchAsync(Arg.Any<string>(), Arg.Any<IReadOnlyList<Tuple<string, string>>>(), Arg.Any<CancellationToken>()).Returns((x) =>
{
var result = new SearchResult(2, new List<Tuple<string, string>>());
return Task.FromResult(result);
});
_orchestratorJob = new BulkDeleteOrchestratorJob(_queueClient, _searchService);

_progress = new Progress<string>((result) => { });
Expand Down Expand Up @@ -62,9 +67,14 @@ public async Task GivenBulkDeleteJob_WhenNoResourceTypeIsGiven_ThenProcessingJob
await _queueClient.ReceivedWithAnyArgs(1).EnqueueAsync(Arg.Any<byte>(), Arg.Any<string[]>(), Arg.Any<long?>(), false, false, Arg.Any<CancellationToken>());
await _searchService.ReceivedWithAnyArgs(1).GetUsedResourceTypes(Arg.Any<CancellationToken>());

// Checks that two processing jobs were queued
// Checks that one processing job was queued
var calls = _queueClient.ReceivedCalls();
Assert.Equal(2, ((string[])calls.First().GetArguments()[1]).Length);
var definitions = (string[])calls.First().GetArguments()[1];
Assert.Single(definitions);

// Checks that the processing job lists both resource types
var actualDefinition = JsonConvert.DeserializeObject<BulkDeleteDefinition>(definitions[0]);
Assert.Equal(2, actualDefinition.Type.SplitByOrSeparator().Count());
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using MediatR;
Expand All @@ -14,6 +15,7 @@
using Microsoft.Health.Fhir.Core.Features.Operations;
using Microsoft.Health.Fhir.Core.Features.Operations.BulkDelete;
using Microsoft.Health.Fhir.Core.Features.Persistence;
using Microsoft.Health.Fhir.Core.Features.Search;
using Microsoft.Health.Fhir.Core.Messages.Delete;
using Microsoft.Health.Fhir.Tests.Common;
using Microsoft.Health.JobManagement;
Expand All @@ -31,13 +33,19 @@ public class BulkDeleteProcessingJobTests
private IDeletionService _deleter;
private IProgress<string> _progress;
private BulkDeleteProcessingJob _processingJob;
private ISearchService _searchService;
private IQueueClient _queueClient;

public BulkDeleteProcessingJobTests()
{
_searchService = Substitute.For<ISearchService>();
_searchService.SearchAsync(Arg.Any<string>(), Arg.Any<IReadOnlyList<Tuple<string, string>>>(), Arg.Any<CancellationToken>(), resourceVersionTypes: Arg.Any<ResourceVersionType>())
.Returns(Task.FromResult(new SearchResult(5, new List<Tuple<string, string>>())));
_queueClient = Substitute.For<IQueueClient>();
_deleter = Substitute.For<IDeletionService>();
var deleter = Substitute.For<IScoped<IDeletionService>>();
deleter.Value.Returns(_deleter);
_processingJob = new BulkDeleteProcessingJob(() => deleter, Substitute.For<RequestContextAccessor<IFhirRequestContext>>(), Substitute.For<IMediator>());
_processingJob = new BulkDeleteProcessingJob(() => deleter, Substitute.For<RequestContextAccessor<IFhirRequestContext>>(), Substitute.For<IMediator>(), _searchService, _queueClient);

_progress = new Progress<string>((result) => { });
}
Expand All @@ -55,13 +63,44 @@ public async Task GivenProcessingJob_WhenJobIsRun_ThenResourcesAreDeleted()
};

_deleter.DeleteMultipleAsync(Arg.Any<ConditionalDeleteResourceRequest>(), Arg.Any<CancellationToken>())
.Returns(args => new HashSet<string> { "1", "2", "3"});
.Returns(args => 3);

var result = JsonConvert.DeserializeObject<BulkDeleteResult>(await _processingJob.ExecuteAsync(jobInfo, _progress, CancellationToken.None));
Assert.Single(result.ResourcesDeleted);
Assert.Equal(3, result.ResourcesDeleted["Patient"]);

await _deleter.ReceivedWithAnyArgs(1).DeleteMultipleAsync(Arg.Any<ConditionalDeleteResourceRequest>(), Arg.Any<CancellationToken>());
}

[Fact]
public async Task GivenProcessingJob_WhenJobIsRunWithMultipleResourceTypes_ThenFollowupJobIsCreated()
{
_deleter.ClearReceivedCalls();

var definition = new BulkDeleteDefinition(JobType.BulkDeleteProcessing, DeleteOperation.HardDelete, "Patient,Observation,Device", new List<Tuple<string, string>>(), "https:\\\\test.com", "https:\\\\test.com", "test");
var jobInfo = new JobInfo()
{
Id = 1,
Definition = JsonConvert.SerializeObject(definition),
};

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

var result = JsonConvert.DeserializeObject<BulkDeleteResult>(await _processingJob.ExecuteAsync(jobInfo, _progress, CancellationToken.None));
Assert.Single(result.ResourcesDeleted);
Assert.Equal(3, result.ResourcesDeleted["Patient"]);

await _deleter.ReceivedWithAnyArgs(1).DeleteMultipleAsync(Arg.Any<ConditionalDeleteResourceRequest>(), Arg.Any<CancellationToken>());

// Checks that one processing job was queued
var calls = _queueClient.ReceivedCalls();
var definitions = (string[])calls.First().GetArguments()[1];
Assert.Single(definitions);

// Checks that the processing job removed the resource type that was processed and lists the remaining two resource types
var actualDefinition = JsonConvert.DeserializeObject<BulkDeleteDefinition>(definitions[0]);
Assert.Equal(2, actualDefinition.Type.SplitByOrSeparator().Count());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public async Task GivenBulkDeleteRequest_WhenJobCreationRequested_ThenJobIsCreat
};
});

var request = new CreateBulkDeleteRequest(DeleteOperation.HardDelete, KnownResourceTypes.Patient, searchParams);
var request = new CreateBulkDeleteRequest(DeleteOperation.HardDelete, KnownResourceTypes.Patient, searchParams, false);

var response = await _handler.Handle(request, CancellationToken.None);
Assert.NotNull(response);
Expand All @@ -105,7 +105,7 @@ public async Task GivenBulkDeleteRequestWithInvalidSearchParameter_WhenJobCreati
_authorizationService.CheckAccess(Arg.Any<DataActions>(), Arg.Any<CancellationToken>()).Returns(DataActions.HardDelete | DataActions.Delete);
_contextAccessor.RequestContext.BundleIssues.Add(new OperationOutcomeIssue(OperationOutcomeConstants.IssueSeverity.Warning, OperationOutcomeConstants.IssueType.Conflict));

var request = new CreateBulkDeleteRequest(DeleteOperation.HardDelete, KnownResourceTypes.Patient, searchParams);
var request = new CreateBulkDeleteRequest(DeleteOperation.HardDelete, KnownResourceTypes.Patient, searchParams, false);

await Assert.ThrowsAsync<BadRequestException>(async () => await _handler.Handle(request, CancellationToken.None));
}
Expand All @@ -118,7 +118,7 @@ public async Task GivenUnauthorizedUser_WhenJobCreationRequested_ThenUnauthorize
{
_authorizationService.CheckAccess(Arg.Any<DataActions>(), Arg.Any<CancellationToken>()).Returns(userRole);

var request = new CreateBulkDeleteRequest(deleteOperation, null, null);
var request = new CreateBulkDeleteRequest(deleteOperation, null, null, false);
await Assert.ThrowsAsync<UnauthorizedFhirActionException>(async () => await _handler.Handle(request, CancellationToken.None));
}

Expand All @@ -129,7 +129,7 @@ public async Task GivenBulkDeleteRequest_WhenJobCreationFails_ThenExceptionIsThr
_contextAccessor.RequestContext.BundleIssues.Clear();
_queueClient.EnqueueAsync((byte)QueueType.BulkDelete, Arg.Any<string[]>(), Arg.Any<long?>(), false, false, Arg.Any<CancellationToken>()).Returns(new List<JobInfo>());

var request = new CreateBulkDeleteRequest(DeleteOperation.HardDelete, null, new List<Tuple<string, string>>());
var request = new CreateBulkDeleteRequest(DeleteOperation.HardDelete, null, new List<Tuple<string, string>>(), false);
await Assert.ThrowsAsync<JobNotExistException>(async () => await _handler.Handle(request, CancellationToken.None));
}
}
Expand Down
Loading