Skip to content

Commit 4fc99e7

Browse files
author
Luke deGruchy
authored
$delete-expunge over 10k resources will now delete all resources (#5144)
* First commit with very rough fix and unit test. * Refinements to ResourceIdListStep and Batch2DaoSvcImpl. Make LoadIdsStepTest pass. Enhance Batch2DaoSvcImplTest. * Spotless * Fix checkstyle errors. * Fix test failures. * Minor refactoring. New unit test. Finalize changelist. * Spotless fix. * Delete now useless code from unit test. * Delete more useless code. * Test pre-commit hook * More spotless fixes. * Address most code review feedback. * Remove use of pageSize parameter and see if this breaks the pipeline. * Remove use of pageSize parameter and see if this breaks the pipeline. * Fix the noUrl case by passing an unlimited Pegeable instead. Effectively stop using page size for most databases. * Deprecate the old method and have it call the new one by default.
1 parent f82da96 commit 4fc99e7

File tree

11 files changed

+308
-131
lines changed

11 files changed

+308
-131
lines changed

‎hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/5150--delete-expunge-over-10k-resources-deletes-only-10k.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
type: fix
3+
issue: 5150
4+
title: "When running a $delete-expunge with over 10,000 resources, only the first 10,000 resources were deleted.
5+
This is now fixed."

‎hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/Batch2SupportConfig.java‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,16 +19,21 @@
1919
*/
2020
package ca.uhn.fhir.jpa.config;
2121

22+
import ca.uhn.fhir.context.FhirContext;
2223
import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
24+
import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
2325
import ca.uhn.fhir.jpa.api.svc.IBatch2DaoSvc;
2426
import ca.uhn.fhir.jpa.api.svc.IDeleteExpungeSvc;
2527
import ca.uhn.fhir.jpa.api.svc.IIdHelperService;
2628
import ca.uhn.fhir.jpa.dao.IFulltextSearchSvc;
2729
import ca.uhn.fhir.jpa.dao.data.IResourceLinkDao;
30+
import ca.uhn.fhir.jpa.dao.data.IResourceTableDao;
2831
import ca.uhn.fhir.jpa.dao.expunge.ResourceTableFKProvider;
32+
import ca.uhn.fhir.jpa.dao.tx.IHapiTransactionService;
2933
import ca.uhn.fhir.jpa.delete.batch2.DeleteExpungeSqlBuilder;
3034
import ca.uhn.fhir.jpa.delete.batch2.DeleteExpungeSvcImpl;
3135
import ca.uhn.fhir.jpa.reindex.Batch2DaoSvcImpl;
36+
import ca.uhn.fhir.jpa.searchparam.MatchUrlService;
3237
import org.springframework.beans.factory.annotation.Autowired;
3338
import org.springframework.context.annotation.Bean;
3439

@@ -37,8 +42,20 @@
3742
public class Batch2SupportConfig {
3843

3944
@Bean
40-
public IBatch2DaoSvc batch2DaoSvc() {
41-
return new Batch2DaoSvcImpl();
45+
public IBatch2DaoSvc batch2DaoSvc(
46+
IResourceTableDao theResourceTableDao,
47+
MatchUrlService theMatchUrlService,
48+
DaoRegistry theDaoRegistry,
49+
FhirContext theFhirContext,
50+
IHapiTransactionService theTransactionService,
51+
JpaStorageSettings theJpaStorageSettings) {
52+
return new Batch2DaoSvcImpl(
53+
theResourceTableDao,
54+
theMatchUrlService,
55+
theDaoRegistry,
56+
theFhirContext,
57+
theTransactionService,
58+
theJpaStorageSettings);
4259
}
4360

4461
@Bean

‎hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java‎

Lines changed: 70 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,9 @@
2121

2222
import ca.uhn.fhir.context.FhirContext;
2323
import ca.uhn.fhir.context.RuntimeResourceDefinition;
24+
import ca.uhn.fhir.i18n.Msg;
2425
import ca.uhn.fhir.interceptor.model.RequestPartitionId;
26+
import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
2527
import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
2628
import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao;
2729
import ca.uhn.fhir.jpa.api.pid.EmptyResourcePidList;
@@ -39,93 +41,121 @@
3941
import ca.uhn.fhir.rest.api.SortSpec;
4042
import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
4143
import ca.uhn.fhir.rest.api.server.storage.IResourcePersistentId;
42-
import ca.uhn.fhir.rest.param.DateRangeParam;
43-
import ca.uhn.fhir.util.DateRangeUtil;
44-
import org.springframework.beans.factory.annotation.Autowired;
44+
import ca.uhn.fhir.rest.server.exceptions.InternalErrorException;
4545
import org.springframework.data.domain.Pageable;
4646
import org.springframework.data.domain.Slice;
4747

48+
import java.util.ArrayList;
4849
import java.util.Date;
4950
import java.util.List;
5051
import java.util.stream.Collectors;
5152
import javax.annotation.Nonnull;
5253
import javax.annotation.Nullable;
5354

54-
import static org.apache.commons.collections4.CollectionUtils.isNotEmpty;
55-
5655
public class Batch2DaoSvcImpl implements IBatch2DaoSvc {
56+
private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(Batch2DaoSvcImpl.class);
57+
58+
private final IResourceTableDao myResourceTableDao;
5759

58-
@Autowired
59-
private IResourceTableDao myResourceTableDao;
60+
private final MatchUrlService myMatchUrlService;
6061

61-
@Autowired
62-
private MatchUrlService myMatchUrlService;
62+
private final DaoRegistry myDaoRegistry;
6363

64-
@Autowired
65-
private DaoRegistry myDaoRegistry;
64+
private final FhirContext myFhirContext;
6665

67-
@Autowired
68-
private FhirContext myFhirContext;
66+
private final IHapiTransactionService myTransactionService;
6967

70-
@Autowired
71-
private IHapiTransactionService myTransactionService;
68+
private final JpaStorageSettings myJpaStorageSettings;
7269

7370
@Override
7471
public boolean isAllResourceTypeSupported() {
7572
return true;
7673
}
7774

75+
public Batch2DaoSvcImpl(
76+
IResourceTableDao theResourceTableDao,
77+
MatchUrlService theMatchUrlService,
78+
DaoRegistry theDaoRegistry,
79+
FhirContext theFhirContext,
80+
IHapiTransactionService theTransactionService,
81+
JpaStorageSettings theJpaStorageSettings) {
82+
myResourceTableDao = theResourceTableDao;
83+
myMatchUrlService = theMatchUrlService;
84+
myDaoRegistry = theDaoRegistry;
85+
myFhirContext = theFhirContext;
86+
myTransactionService = theTransactionService;
87+
myJpaStorageSettings = theJpaStorageSettings;
88+
}
89+
7890
@Override
7991
public IResourcePidList fetchResourceIdsPage(
80-
Date theStart,
81-
Date theEnd,
82-
@Nonnull Integer thePageSize,
83-
@Nullable RequestPartitionId theRequestPartitionId,
84-
@Nullable String theUrl) {
92+
Date theStart, Date theEnd, @Nullable RequestPartitionId theRequestPartitionId, @Nullable String theUrl) {
8593
return myTransactionService
8694
.withSystemRequest()
8795
.withRequestPartitionId(theRequestPartitionId)
8896
.execute(() -> {
8997
if (theUrl == null) {
90-
return fetchResourceIdsPageNoUrl(theStart, theEnd, thePageSize, theRequestPartitionId);
98+
return fetchResourceIdsPageNoUrl(theStart, theEnd, theRequestPartitionId);
9199
} else {
92-
return fetchResourceIdsPageWithUrl(
93-
theStart, theEnd, thePageSize, theUrl, theRequestPartitionId);
100+
return fetchResourceIdsPageWithUrl(theEnd, theUrl, theRequestPartitionId);
94101
}
95102
});
96103
}
97104

98-
private IResourcePidList fetchResourceIdsPageWithUrl(
99-
Date theStart, Date theEnd, int thePageSize, String theUrl, RequestPartitionId theRequestPartitionId) {
105+
@Nonnull
106+
private HomogeneousResourcePidList fetchResourceIdsPageWithUrl(
107+
Date theEnd, @Nonnull String theUrl, @Nullable RequestPartitionId theRequestPartitionId) {
108+
if (!theUrl.contains("?")) {
109+
throw new InternalErrorException(Msg.code(2422) + "this should never happen: URL is missing a '?'");
110+
}
111+
112+
final Integer internalSynchronousSearchSize = myJpaStorageSettings.getInternalSynchronousSearchSize();
113+
114+
if (internalSynchronousSearchSize == null || internalSynchronousSearchSize <= 0) {
115+
throw new InternalErrorException(Msg.code(2423)
116+
+ "this should never happen: internalSynchronousSearchSize is null or less than or equal to 0");
117+
}
118+
119+
List<IResourcePersistentId> currentIds = fetchResourceIdsPageWithUrl(0, theUrl, theRequestPartitionId);
120+
ourLog.debug("FIRST currentIds: {}", currentIds.size());
100121

122+
final List<IResourcePersistentId> allIds = new ArrayList<>(currentIds);
123+
124+
while (internalSynchronousSearchSize < currentIds.size()) {
125+
// Ensure the offset is set to the last ID in the cumulative List, otherwise, we'll be stuck in an infinite
126+
// loop here:
127+
currentIds = fetchResourceIdsPageWithUrl(allIds.size(), theUrl, theRequestPartitionId);
128+
ourLog.debug("NEXT currentIds: {}", currentIds.size());
129+
130+
allIds.addAll(currentIds);
131+
}
132+
133+
final String resourceType = theUrl.substring(0, theUrl.indexOf('?'));
134+
135+
return new HomogeneousResourcePidList(resourceType, allIds, theEnd, theRequestPartitionId);
136+
}
137+
138+
private List<IResourcePersistentId> fetchResourceIdsPageWithUrl(
139+
int theOffset, String theUrl, RequestPartitionId theRequestPartitionId) {
101140
String resourceType = theUrl.substring(0, theUrl.indexOf('?'));
102141
RuntimeResourceDefinition def = myFhirContext.getResourceDefinition(resourceType);
103142

104143
SearchParameterMap searchParamMap = myMatchUrlService.translateMatchUrl(theUrl, def);
105-
searchParamMap.setSort(new SortSpec(Constants.PARAM_LASTUPDATED, SortOrderEnum.ASC));
106-
DateRangeParam chunkDateRange =
107-
DateRangeUtil.narrowDateRange(searchParamMap.getLastUpdated(), theStart, theEnd);
108-
searchParamMap.setLastUpdated(chunkDateRange);
109-
searchParamMap.setCount(thePageSize);
144+
searchParamMap.setSort(new SortSpec(Constants.PARAM_ID, SortOrderEnum.ASC));
145+
searchParamMap.setOffset(theOffset);
146+
searchParamMap.setLoadSynchronousUpTo(myJpaStorageSettings.getInternalSynchronousSearchSize() + 1);
110147

111148
IFhirResourceDao<?> dao = myDaoRegistry.getResourceDao(resourceType);
112149
SystemRequestDetails request = new SystemRequestDetails();
113150
request.setRequestPartitionId(theRequestPartitionId);
114-
List<IResourcePersistentId> ids = dao.searchForIds(searchParamMap, request);
115-
116-
Date lastDate = null;
117-
if (isNotEmpty(ids)) {
118-
IResourcePersistentId lastResourcePersistentId = ids.get(ids.size() - 1);
119-
lastDate = dao.readByPid(lastResourcePersistentId, true).getMeta().getLastUpdated();
120-
}
121151

122-
return new HomogeneousResourcePidList(resourceType, ids, lastDate, theRequestPartitionId);
152+
return dao.searchForIds(searchParamMap, request);
123153
}
124154

125155
@Nonnull
126156
private IResourcePidList fetchResourceIdsPageNoUrl(
127-
Date theStart, Date theEnd, int thePagesize, RequestPartitionId theRequestPartitionId) {
128-
Pageable page = Pageable.ofSize(thePagesize);
157+
Date theStart, Date theEnd, RequestPartitionId theRequestPartitionId) {
158+
final Pageable page = Pageable.unpaged();
129159
Slice<Object[]> slice;
130160
if (theRequestPartitionId == null || theRequestPartitionId.isAllPartitions()) {
131161
slice = myResourceTableDao.findIdsTypesAndUpdateTimesOfResourcesWithinUpdatedRangeOrderedFromOldest(

‎hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/svc/GoldenResourceSearchSvcImpl.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,6 @@ private IResourcePidList fetchResourceIdsPageWithResourceType(
8282
DateRangeParam chunkDateRange =
8383
DateRangeUtil.narrowDateRange(searchParamMap.getLastUpdated(), theStart, theEnd);
8484
searchParamMap.setLastUpdated(chunkDateRange);
85-
searchParamMap.setCount(thePageSize); // request this many pids
8685
searchParamMap.add(
8786
"_tag", new TokenParam(MdmConstants.SYSTEM_GOLDEN_RECORD_STATUS, MdmConstants.CODE_GOLDEN_RECORD));
8887

‎hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/MultitenantBatchOperationR4Test.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,8 @@
3232
import java.util.stream.Collectors;
3333

3434
import static ca.uhn.fhir.jpa.model.util.JpaConstants.DEFAULT_PARTITION_NAME;
35-
import static org.awaitility.Awaitility.await;
3635
import static org.hamcrest.MatcherAssert.assertThat;
3736
import static org.hamcrest.Matchers.hasSize;
38-
import static org.hamcrest.Matchers.in;
3937
import static org.hamcrest.Matchers.isA;
4038
import static org.junit.jupiter.api.Assertions.assertEquals;
4139

@@ -108,7 +106,7 @@ public void testDeleteExpungeOperation() {
108106
String jobId = BatchHelperR4.jobIdFromBatch2Parameters(response);
109107
myBatch2JobHelper.awaitJobCompletion(jobId);
110108

111-
assertThat(interceptor.requestPartitionIds, hasSize(5));
109+
assertThat(interceptor.requestPartitionIds, hasSize(4));
112110
RequestPartitionId partitionId = interceptor.requestPartitionIds.get(0);
113111
assertEquals(TENANT_B_ID, partitionId.getFirstPartitionIdOrNull());
114112
assertEquals(TENANT_B, partitionId.getFirstPartitionNameOrNull());

‎hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
package ca.uhn.fhir.jpa.reindex;
2+
3+
import ca.uhn.fhir.interceptor.model.RequestPartitionId;
4+
import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
5+
import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
6+
import ca.uhn.fhir.jpa.api.pid.IResourcePidList;
7+
import ca.uhn.fhir.jpa.api.svc.IBatch2DaoSvc;
8+
import ca.uhn.fhir.jpa.dao.tx.IHapiTransactionService;
9+
import ca.uhn.fhir.jpa.searchparam.MatchUrlService;
10+
import ca.uhn.fhir.jpa.test.BaseJpaR4Test;
11+
import ca.uhn.fhir.model.primitive.IdDt;
12+
import ca.uhn.fhir.rest.server.exceptions.InternalErrorException;
13+
import org.hl7.fhir.instance.model.api.IIdType;
14+
import org.junit.jupiter.api.BeforeEach;
15+
import org.junit.jupiter.api.Test;
16+
import org.junit.jupiter.params.ParameterizedTest;
17+
import org.junit.jupiter.params.provider.ValueSource;
18+
import org.springframework.beans.factory.annotation.Autowired;
19+
20+
import javax.annotation.Nonnull;
21+
import java.time.LocalDate;
22+
import java.time.Month;
23+
import java.time.ZoneId;
24+
import java.util.Date;
25+
import java.util.List;
26+
import java.util.stream.IntStream;
27+
28+
import static org.junit.jupiter.api.Assertions.assertEquals;
29+
import static org.junit.jupiter.api.Assertions.assertThrows;
30+
import static org.mockito.Mockito.spy;
31+
import static org.mockito.Mockito.times;
32+
import static org.mockito.Mockito.verify;
33+
34+
class Batch2DaoSvcImplTest extends BaseJpaR4Test {
35+
36+
private static final Date PREVIOUS_MILLENNIUM = toDate(LocalDate.of(1999, Month.DECEMBER, 31));
37+
private static final Date TOMORROW = toDate(LocalDate.now().plusDays(1));
38+
private static final String URL_PATIENT_EXPUNGE_TRUE = "Patient?_expunge=true";
39+
private static final String PATIENT = "Patient";
40+
private static final int INTERNAL_SYNCHRONOUS_SEARCH_SIZE = 10;
41+
42+
@Autowired
43+
private JpaStorageSettings myJpaStorageSettings;
44+
@Autowired
45+
private MatchUrlService myMatchUrlService;
46+
@Autowired
47+
private IHapiTransactionService myIHapiTransactionService ;
48+
49+
private DaoRegistry mySpiedDaoRegistry;
50+
51+
private IBatch2DaoSvc mySubject;
52+
53+
@BeforeEach
54+
void beforeEach() {
55+
myJpaStorageSettings.setInternalSynchronousSearchSize(INTERNAL_SYNCHRONOUS_SEARCH_SIZE);
56+
57+
mySpiedDaoRegistry = spy(myDaoRegistry);
58+
59+
mySubject = new Batch2DaoSvcImpl(myResourceTableDao, myMatchUrlService, mySpiedDaoRegistry, myFhirContext, myIHapiTransactionService, myJpaStorageSettings);
60+
}
61+
62+
// TODO: LD this test won't work with the nonUrl variant yet: error: No existing transaction found for transaction marked with propagation 'mandatory'
63+
64+
@Test
65+
void fetchResourcesByUrlEmptyUrl() {
66+
final InternalErrorException exception = assertThrows(InternalErrorException.class, () -> mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), ""));
67+
68+
assertEquals("HAPI-2422: this should never happen: URL is missing a '?'", exception.getMessage());
69+
}
70+
71+
@Test
72+
void fetchResourcesByUrlSingleQuestionMark() {
73+
final InternalErrorException exception = assertThrows(InternalErrorException.class, () -> mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), "?"));
74+
75+
assertEquals("HAPI-2223: theResourceName must not be blank", exception.getMessage());
76+
}
77+
78+
@Test
79+
void fetchResourcesByUrlNonsensicalResource() {
80+
final InternalErrorException exception = assertThrows(InternalErrorException.class, () -> mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), "Banana?_expunge=true"));
81+
82+
assertEquals("HAPI-2223: HAPI-1684: Unknown resource name \"Banana\" (this name is not known in FHIR version \"R4\")", exception.getMessage());
83+
}
84+
85+
@ParameterizedTest
86+
@ValueSource(ints = {0, 9, 10, 11, 21, 22, 23, 45})
87+
void fetchResourcesByUrl(int expectedNumResults) {
88+
final List<IIdType> patientIds = IntStream.range(0, expectedNumResults)
89+
.mapToObj(num -> createPatient())
90+
.toList();
91+
92+
final IResourcePidList resourcePidList = mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), URL_PATIENT_EXPUNGE_TRUE);
93+
94+
final List<? extends IIdType> actualPatientIds =
95+
resourcePidList.getTypedResourcePids()
96+
.stream()
97+
.map(typePid -> new IdDt(typePid.resourceType, (Long) typePid.id.getId()))
98+
.toList();
99+
assertIdsEqual(patientIds, actualPatientIds);
100+
101+
verify(mySpiedDaoRegistry, times(getExpectedNumOfInvocations(expectedNumResults))).getResourceDao(PATIENT);
102+
}
103+
104+
@ParameterizedTest
105+
@ValueSource(ints = {0, 9, 10, 11, 21, 22, 23, 45})
106+
void fetchResourcesNoUrl(int expectedNumResults) {
107+
final int pageSizeWellBelowThreshold = 2;
108+
final List<IIdType> patientIds = IntStream.range(0, expectedNumResults)
109+
.mapToObj(num -> createPatient())
110+
.toList();
111+
112+
final IResourcePidList resourcePidList = mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, pageSizeWellBelowThreshold, RequestPartitionId.defaultPartition(), null);
113+
114+
final List<? extends IIdType> actualPatientIds =
115+
resourcePidList.getTypedResourcePids()
116+
.stream()
117+
.map(typePid -> new IdDt(typePid.resourceType, (Long) typePid.id.getId()))
118+
.toList();
119+
assertIdsEqual(patientIds, actualPatientIds);
120+
}
121+
122+
private int getExpectedNumOfInvocations(int expectedNumResults) {
123+
final int maxResultsPerQuery = INTERNAL_SYNCHRONOUS_SEARCH_SIZE + 1;
124+
final int division = expectedNumResults / maxResultsPerQuery;
125+
return division + 1;
126+
}
127+
128+
private static void assertIdsEqual(List<IIdType> expectedResourceIds, List<? extends IIdType> actualResourceIds) {
129+
assertEquals(expectedResourceIds.size(), actualResourceIds.size());
130+
131+
for (int index = 0; index < expectedResourceIds.size(); index++) {
132+
final IIdType expectedIdType = expectedResourceIds.get(index);
133+
final IIdType actualIdType = actualResourceIds.get(index);
134+
135+
assertEquals(expectedIdType.getResourceType(), actualIdType.getResourceType());
136+
assertEquals(expectedIdType.getIdPartAsLong(), actualIdType.getIdPartAsLong());
137+
}
138+
}
139+
140+
@Nonnull
141+
private static Date toDate(LocalDate theLocalDate) {
142+
return Date.from(theLocalDate.atStartOfDay(ZoneId.systemDefault()).toInstant());
143+
}
144+
}

0 commit comments

Comments
 (0)