From 76c2c66505ee33b08bb31cf09cbec804886bc18f Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Fri, 28 Jul 2023 13:45:20 -0400 Subject: [PATCH 01/17] First commit with very rough fix and unit test. --- .../fhir/changelog/6_8_0/9999-something.yaml | 4 + .../fhir/jpa/config/Batch2SupportConfig.java | 9 +- .../fhir/jpa/reindex/Batch2DaoSvcImpl.java | 71 ++++++++--- .../jpa/reindex/Batch2DaoSvcImplTest.java | 117 ++++++++++++++++++ .../batch2/jobs/step/ResourceIdListStep.java | 15 ++- .../batch2/jobs/step/LoadIdsStepTest.java | 23 ++-- .../jobs/step/ResourceIdListStepTest.java | 2 +- 7 files changed, 205 insertions(+), 36 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/9999-something.yaml create mode 100644 hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/9999-something.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/9999-something.yaml new file mode 100644 index 000000000000..953d776ff4f6 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/9999-something.yaml @@ -0,0 +1,4 @@ +--- +type: fix +issue: 9999 +title: "Something something" diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/Batch2SupportConfig.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/Batch2SupportConfig.java index 7fbf12097034..5824fc7c0b3f 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/Batch2SupportConfig.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/Batch2SupportConfig.java @@ -19,16 +19,21 @@ */ package ca.uhn.fhir.jpa.config; +import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.jpa.api.config.JpaStorageSettings; +import ca.uhn.fhir.jpa.api.dao.DaoRegistry; import ca.uhn.fhir.jpa.api.svc.IBatch2DaoSvc; import ca.uhn.fhir.jpa.api.svc.IDeleteExpungeSvc; import ca.uhn.fhir.jpa.api.svc.IIdHelperService; import ca.uhn.fhir.jpa.dao.IFulltextSearchSvc; import ca.uhn.fhir.jpa.dao.data.IResourceLinkDao; +import ca.uhn.fhir.jpa.dao.data.IResourceTableDao; import ca.uhn.fhir.jpa.dao.expunge.ResourceTableFKProvider; +import ca.uhn.fhir.jpa.dao.tx.IHapiTransactionService; import ca.uhn.fhir.jpa.delete.batch2.DeleteExpungeSqlBuilder; import ca.uhn.fhir.jpa.delete.batch2.DeleteExpungeSvcImpl; import ca.uhn.fhir.jpa.reindex.Batch2DaoSvcImpl; +import ca.uhn.fhir.jpa.searchparam.MatchUrlService; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Bean; @@ -37,8 +42,8 @@ public class Batch2SupportConfig { @Bean - public IBatch2DaoSvc batch2DaoSvc() { - return new Batch2DaoSvcImpl(); + public IBatch2DaoSvc batch2DaoSvc(IResourceTableDao theResourceTableDao, MatchUrlService theMatchUrlService, DaoRegistry theDaoRegistry, FhirContext theFhirContext, IHapiTransactionService theTransactionService, JpaStorageSettings theJpaStorageSettings) { + return new Batch2DaoSvcImpl(theResourceTableDao, theMatchUrlService, theDaoRegistry, theFhirContext, theTransactionService, theJpaStorageSettings); } @Bean diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java index f195fc25475e..d6fcab50b147 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java @@ -22,6 +22,7 @@ import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.context.RuntimeResourceDefinition; import ca.uhn.fhir.interceptor.model.RequestPartitionId; +import ca.uhn.fhir.jpa.api.config.JpaStorageSettings; import ca.uhn.fhir.jpa.api.dao.DaoRegistry; import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao; import ca.uhn.fhir.jpa.api.pid.EmptyResourcePidList; @@ -41,10 +42,10 @@ import ca.uhn.fhir.rest.api.server.storage.IResourcePersistentId; import ca.uhn.fhir.rest.param.DateRangeParam; import ca.uhn.fhir.util.DateRangeUtil; -import org.springframework.beans.factory.annotation.Autowired; import org.springframework.data.domain.Pageable; import org.springframework.data.domain.Slice; +import java.util.ArrayList; import java.util.Date; import java.util.List; import java.util.stream.Collectors; @@ -55,26 +56,32 @@ public class Batch2DaoSvcImpl implements IBatch2DaoSvc { - @Autowired - private IResourceTableDao myResourceTableDao; + private final IResourceTableDao myResourceTableDao; - @Autowired - private MatchUrlService myMatchUrlService; + private final MatchUrlService myMatchUrlService; - @Autowired - private DaoRegistry myDaoRegistry; + private final DaoRegistry myDaoRegistry; - @Autowired - private FhirContext myFhirContext; + private final FhirContext myFhirContext; - @Autowired - private IHapiTransactionService myTransactionService; + private final IHapiTransactionService myTransactionService; + + private final JpaStorageSettings myJpaStorageSettings; @Override public boolean isAllResourceTypeSupported() { return true; } + public Batch2DaoSvcImpl(IResourceTableDao theResourceTableDao, MatchUrlService theMatchUrlService, DaoRegistry theDaoRegistry, FhirContext theFhirContext, IHapiTransactionService theTransactionService, JpaStorageSettings theJpaStorageSettings) { + myResourceTableDao = theResourceTableDao; + myMatchUrlService = theMatchUrlService; + myDaoRegistry = theDaoRegistry; + myFhirContext = theFhirContext; + myTransactionService = theTransactionService; + myJpaStorageSettings = theJpaStorageSettings; + } + @Override public IResourcePidList fetchResourceIdsPage( Date theStart, @@ -89,24 +96,52 @@ public IResourcePidList fetchResourceIdsPage( if (theUrl == null) { return fetchResourceIdsPageNoUrl(theStart, theEnd, thePageSize, theRequestPartitionId); } else { - return fetchResourceIdsPageWithUrl( - theStart, theEnd, thePageSize, theUrl, theRequestPartitionId); + // TODO: validation for garbage URL + final Integer internalSynchronousSearchSize = myJpaStorageSettings.getInternalSynchronousSearchSize(); + + if (internalSynchronousSearchSize == null || internalSynchronousSearchSize <= 0) { + // TODO: new HAPI code + throw new IllegalStateException("HAPI-99999: this should never happen: internalSynchronousSearchSize is null or less than or equal to 0"); + } + + final int searchSizeThreshold = internalSynchronousSearchSize - 1; + final List allIds = new ArrayList<>(); + List currentIds = new ArrayList<>(); + boolean init = true; + Date lastDate = null; + // 10000 > 9999 + while (init || searchSizeThreshold < currentIds.size() ) { + init = false; + // TODO: why does this method get executed multiple times? + // TODO: this is an infinite loop because I can't signal to this method that it should start at a new point + final HomogeneousResourcePidList resourcePidList = fetchResourceIdsPageWithUrl(theStart, theEnd, thePageSize, currentIds.size(), theUrl, theRequestPartitionId); + currentIds = resourcePidList.getIds(); + lastDate = resourcePidList.getLastDate(); + allIds.addAll(currentIds); + } + + final String resourceType = theUrl.substring(0, theUrl.indexOf('?')); + + return new HomogeneousResourcePidList(resourceType, allIds, lastDate, theRequestPartitionId); } }); } - private IResourcePidList fetchResourceIdsPageWithUrl( - Date theStart, Date theEnd, int thePageSize, String theUrl, RequestPartitionId theRequestPartitionId) { - + // TODO: consider just returning the IDs + private HomogeneousResourcePidList fetchResourceIdsPageWithUrl( + Date theStart, Date theEnd, int thePageSize, int theOffset, String theUrl, RequestPartitionId theRequestPartitionId) { String resourceType = theUrl.substring(0, theUrl.indexOf('?')); RuntimeResourceDefinition def = myFhirContext.getResourceDefinition(resourceType); + // TODO: consider cleaning up date stuff that's no longer needed SearchParameterMap searchParamMap = myMatchUrlService.translateMatchUrl(theUrl, def); - searchParamMap.setSort(new SortSpec(Constants.PARAM_LASTUPDATED, SortOrderEnum.ASC)); + searchParamMap.setSort(new SortSpec(Constants.PARAM_ID, SortOrderEnum.ASC)); DateRangeParam chunkDateRange = - DateRangeUtil.narrowDateRange(searchParamMap.getLastUpdated(), theStart, theEnd); + DateRangeUtil.narrowDateRange(searchParamMap.getLastUpdated(), theStart, theEnd); searchParamMap.setLastUpdated(chunkDateRange); searchParamMap.setCount(thePageSize); + // TODO: try this: + searchParamMap.setOffset(theOffset); IFhirResourceDao dao = myDaoRegistry.getResourceDao(resourceType); SystemRequestDetails request = new SystemRequestDetails(); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java new file mode 100644 index 000000000000..b8f639dc7eec --- /dev/null +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java @@ -0,0 +1,117 @@ +package ca.uhn.fhir.jpa.reindex; + +import ca.uhn.fhir.interceptor.model.RequestPartitionId; +import ca.uhn.fhir.jpa.api.config.JpaStorageSettings; +import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao; +import ca.uhn.fhir.jpa.api.pid.IResourcePidList; +import ca.uhn.fhir.jpa.api.svc.IBatch2DaoSvc; +import ca.uhn.fhir.jpa.dao.tx.IHapiTransactionService; +import ca.uhn.fhir.jpa.dao.tx.NonTransactionalHapiTransactionService; +import ca.uhn.fhir.jpa.searchparam.MatchUrlService; +import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; +import ca.uhn.fhir.jpa.test.BaseJpaR4Test; +import ca.uhn.fhir.rest.api.server.IBundleProvider; +import ca.uhn.fhir.rest.api.server.SystemRequestDetails; +import org.hl7.fhir.r4.model.Enumerations; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.Mock; +import org.springframework.beans.factory.annotation.Autowired; + +import javax.annotation.Nonnull; +import java.time.LocalDate; +import java.time.Month; +import java.time.ZoneId; +import java.util.Date; +import java.util.stream.IntStream; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class Batch2DaoSvcImplTest extends BaseJpaR4Test { + + private static final Date PREVIOUS_MILLENNIUM = toDate(LocalDate.of(1999, Month.DECEMBER, 31)); + private static final Date TOMORROW = toDate(LocalDate.now().plusDays(1)); + private static final String URL_PATIENT_EXPUNGE_TRUE = "Patient?_expunge=true"; + +// @Autowired + private IBatch2DaoSvc mySubject; + + @Autowired + private JpaStorageSettings myJpaStorageSettings; + @Autowired + private MatchUrlService myMatchUrlService; + private final IHapiTransactionService myTransactionService = new NonTransactionalHapiTransactionService(); + + @BeforeEach + void beforeEach() { + myJpaStorageSettings.setInternalSynchronousSearchSize(10); + mySubject = new Batch2DaoSvcImpl(myResourceTableDao, myMatchUrlService, myDaoRegistry, myFhirContext, myTransactionService, myJpaStorageSettings); + } + + @Test + void fetchResourcesByUrl_noResults() { + // TODO: grab value from the functional test and use them here + // TODO: try different variants of URLs and fail appropriately + final IResourcePidList resourcePidList = mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), URL_PATIENT_EXPUNGE_TRUE); + + assertTrue(resourcePidList.isEmpty()); + } + + @Test + void fetchResourcesByUrl_oneResultLessThanThreshold() { + final int expectedNumResults = 9; + + IntStream.range(0, expectedNumResults) + .forEach(num -> createPatient()); + + final IBundleProvider search = myPatientDao.search(SearchParameterMap.newSynchronous(), new SystemRequestDetails()); + // TODO: grab value from the functional test and use them here + // TODO: try different variants of URLs and fail appropriately + // TODO: today plus one day? + final IResourcePidList resourcePidList = mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), URL_PATIENT_EXPUNGE_TRUE); + + // TODO: figure out how to spy on results to figure out the number of dao calls + assertEquals(expectedNumResults, resourcePidList.size()); + } + + @Test + void fetchResourcesByUrl_resultsEqualToThreshold() { + final int expectedNumResults = 10; + + IntStream.range(0, expectedNumResults) + .forEach(num -> createPatient(withId("patient"+num))); + + final IBundleProvider search = myPatientDao.search(SearchParameterMap.newSynchronous(), new SystemRequestDetails()); + // TODO: grab value from the functional test and use them here + // TODO: try different variants of URLs and fail appropriately + // TODO: today plus one day? + final IResourcePidList resourcePidList = mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), URL_PATIENT_EXPUNGE_TRUE); + + // TODO: figure out how to spy on results to figure out the number of dao calls + assertEquals(expectedNumResults, resourcePidList.size()); + } + + // TODO: parameterized + @Test + void fetchResourcesByUrl_resultsOverThreshold() { + final int expectedNumResults = 11; + + IntStream.range(0, expectedNumResults) + .forEach(num -> createPatient()); + + final IBundleProvider search = myPatientDao.search(SearchParameterMap.newSynchronous(), new SystemRequestDetails()); + // TODO: grab value from the functional test and use them here + // TODO: try different variants of URLs and fail appropriately + // TODO: today plus one day? + final IResourcePidList resourcePidList = mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), URL_PATIENT_EXPUNGE_TRUE); + + // TODO: figure out how to spy on results to figure out the number of dao calls + assertEquals(expectedNumResults, resourcePidList.size()); + } + + @Nonnull + private static Date toDate(LocalDate theLocalDate) { + return Date.from(theLocalDate.atStartOfDay(ZoneId.systemDefault()).toInstant()); + } +} diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java index 716886f1b8c2..e45e53ea56a8 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java @@ -88,19 +88,22 @@ public RunOutcome run( // we won't go over MAX_BATCH_OF_IDS maxBatchId = Math.min(batchSize.intValue(), maxBatchId); } - while (true) { + // TODO: get rid of while(true) +// while (true) { + // TODO: get rid of the page size semantics here and deal with one huge IResourcePidList, but break it up into multiple chunks IResourcePidList nextChunk = myIdChunkProducer.fetchResourceIdsPage( nextStart, end, pageSize, requestPartitionId, theStepExecutionDetails.getData()); if (nextChunk.isEmpty()) { ourLog.info("No data returned"); - break; +// break; } + // TODO: do we need this anymore? // If we get the same last time twice in a row, we've clearly reached the end if (nextChunk.getLastDate().getTime() == previousLastTime) { ourLog.info("Matching final timestamp of {}, loading is completed", new Date(previousLastTime)); - break; +// break; } ourLog.info("Found {} IDs from {} to {}", nextChunk.size(), nextStart, nextChunk.getLastDate()); @@ -115,8 +118,8 @@ public RunOutcome run( idBuffer.add(nextId); } - previousLastTime = nextChunk.getLastDate().getTime(); - nextStart = nextChunk.getLastDate(); +// previousLastTime = nextChunk.getLastDate().getTime(); +// nextStart = nextChunk.getLastDate(); while (idBuffer.size() > maxBatchId) { List submissionIds = new ArrayList<>(); @@ -132,7 +135,7 @@ public RunOutcome run( chunkCount++; submitWorkChunk(submissionIds, nextChunk.getRequestPartitionId(), theDataSink); } - } +// } totalIdsFound += idBuffer.size(); chunkCount++; diff --git a/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/jobs/step/LoadIdsStepTest.java b/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/jobs/step/LoadIdsStepTest.java index ed333913bbed..bb4bc41ba21e 100644 --- a/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/jobs/step/LoadIdsStepTest.java +++ b/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/jobs/step/LoadIdsStepTest.java @@ -30,6 +30,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -71,26 +72,30 @@ public void testGenerateSteps() { // First Execution - when(myBatch2DaoSvc.fetchResourceIdsPage(eq(DATE_1), eq(DATE_END), eq(DEFAULT_PAGE_SIZE), isNull(), isNull())) + lenient().when(myBatch2DaoSvc.fetchResourceIdsPage(eq(DATE_1), eq(DATE_END), eq(DEFAULT_PAGE_SIZE), isNull(), isNull())) .thenReturn(createIdChunk(0L, 20000L, DATE_2)); - when(myBatch2DaoSvc.fetchResourceIdsPage(eq(DATE_2), eq(DATE_END), eq(DEFAULT_PAGE_SIZE), isNull(), isNull())) + lenient().when(myBatch2DaoSvc.fetchResourceIdsPage(eq(DATE_2), eq(DATE_END), eq(DEFAULT_PAGE_SIZE), isNull(), isNull())) .thenReturn(createIdChunk(20000L, 40000L, DATE_3)); - when(myBatch2DaoSvc.fetchResourceIdsPage(eq(DATE_3), eq(DATE_END), eq(DEFAULT_PAGE_SIZE), isNull(), isNull())) + lenient().when(myBatch2DaoSvc.fetchResourceIdsPage(eq(DATE_3), eq(DATE_END), eq(DEFAULT_PAGE_SIZE), isNull(), isNull())) .thenReturn(createIdChunk(40000L, 40040L, DATE_4)); - when(myBatch2DaoSvc.fetchResourceIdsPage(eq(DATE_4), eq(DATE_END), eq(DEFAULT_PAGE_SIZE), isNull(), isNull())) + lenient().when(myBatch2DaoSvc.fetchResourceIdsPage(eq(DATE_4), eq(DATE_END), eq(DEFAULT_PAGE_SIZE), isNull(), isNull())) .thenReturn(new EmptyResourcePidList()); mySvc.run(details, mySink); - verify(mySink, times(81)).accept(myChunkIdsCaptor.capture()); - for (int i = 0; i < 80; i++) { + // TODO: figure out the semantics of the new behaviour to make this pass: + final int expectedLoops = 40; + verify(mySink, times(40)).accept(myChunkIdsCaptor.capture()); + + final List allCapturedValues = myChunkIdsCaptor.getAllValues(); + for (int i = 0; i < expectedLoops ; i++) { String expected = createIdChunk(i * 500, (i * 500) + 500).toString(); - String actual = myChunkIdsCaptor.getAllValues().get(i).toString(); + String actual = allCapturedValues.get(i).toString(); assertEquals(expected, actual); } + // TODO: figure out what this test is about assertEquals(createIdChunk(40000, 40040).toString(), - myChunkIdsCaptor.getAllValues().get(80).toString()); - + allCapturedValues.get(expectedLoops -1).toString()); } @Nonnull diff --git a/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStepTest.java b/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStepTest.java index 222ace385670..1fb51f4cc8d6 100644 --- a/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStepTest.java +++ b/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStepTest.java @@ -56,7 +56,7 @@ void beforeEach() { } @ParameterizedTest - @ValueSource(ints = {1, 100, 500, 501, 2345}) + @ValueSource(ints = {1, 100, 500, 501, 2345, 10500}) void testResourceIdListBatchSizeLimit(int theListSize) { List idList = generateIdList(theListSize); when(myStepExecutionDetails.getData()).thenReturn(myData); From 0ed77cffceb72f02ad0360a07d3c75e35c02272e Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Fri, 28 Jul 2023 16:35:58 -0400 Subject: [PATCH 02/17] Refinements to ResourceIdListStep and Batch2DaoSvcImpl. Make LoadIdsStepTest pass. Enhance Batch2DaoSvcImplTest. --- .../fhir/jpa/reindex/Batch2DaoSvcImpl.java | 52 +++----- .../jpa/reindex/Batch2DaoSvcImplTest.java | 121 ++++++++++-------- .../batch2/jobs/step/ResourceIdListStep.java | 88 ++++++------- .../batch2/jobs/step/LoadIdsStepTest.java | 6 +- 4 files changed, 122 insertions(+), 145 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java index d6fcab50b147..961343e0065b 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java @@ -40,8 +40,7 @@ import ca.uhn.fhir.rest.api.SortSpec; import ca.uhn.fhir.rest.api.server.SystemRequestDetails; import ca.uhn.fhir.rest.api.server.storage.IResourcePersistentId; -import ca.uhn.fhir.rest.param.DateRangeParam; -import ca.uhn.fhir.util.DateRangeUtil; +import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; import org.springframework.data.domain.Pageable; import org.springframework.data.domain.Slice; @@ -55,6 +54,7 @@ import static org.apache.commons.collections4.CollectionUtils.isNotEmpty; public class Batch2DaoSvcImpl implements IBatch2DaoSvc { + private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(Batch2DaoSvcImpl.class); private final IResourceTableDao myResourceTableDao; @@ -96,65 +96,47 @@ public IResourcePidList fetchResourceIdsPage( if (theUrl == null) { return fetchResourceIdsPageNoUrl(theStart, theEnd, thePageSize, theRequestPartitionId); } else { - // TODO: validation for garbage URL + if (!theUrl.contains("?")) { + throw new InternalErrorException("HAPI-99999: this should never happen: URL is missing a '?'"); + } + final Integer internalSynchronousSearchSize = myJpaStorageSettings.getInternalSynchronousSearchSize(); if (internalSynchronousSearchSize == null || internalSynchronousSearchSize <= 0) { // TODO: new HAPI code - throw new IllegalStateException("HAPI-99999: this should never happen: internalSynchronousSearchSize is null or less than or equal to 0"); + throw new InternalErrorException("HAPI-99999: this should never happen: internalSynchronousSearchSize is null or less than or equal to 0"); } - final int searchSizeThreshold = internalSynchronousSearchSize - 1; - final List allIds = new ArrayList<>(); - List currentIds = new ArrayList<>(); - boolean init = true; - Date lastDate = null; - // 10000 > 9999 - while (init || searchSizeThreshold < currentIds.size() ) { - init = false; - // TODO: why does this method get executed multiple times? - // TODO: this is an infinite loop because I can't signal to this method that it should start at a new point - final HomogeneousResourcePidList resourcePidList = fetchResourceIdsPageWithUrl(theStart, theEnd, thePageSize, currentIds.size(), theUrl, theRequestPartitionId); - currentIds = resourcePidList.getIds(); - lastDate = resourcePidList.getLastDate(); + List currentIds = fetchResourceIdsPageWithUrl(0, theUrl, theRequestPartitionId); + ourLog.info("FIRST currentIds: {}", currentIds.size()); + final List allIds = new ArrayList<>(currentIds); + while (internalSynchronousSearchSize < currentIds.size() ) { + currentIds = fetchResourceIdsPageWithUrl(allIds.size(), theUrl, theRequestPartitionId); + ourLog.info("NEXT currentIds: {}", currentIds.size()); allIds.addAll(currentIds); } final String resourceType = theUrl.substring(0, theUrl.indexOf('?')); - return new HomogeneousResourcePidList(resourceType, allIds, lastDate, theRequestPartitionId); + return new HomogeneousResourcePidList(resourceType, allIds, theEnd, theRequestPartitionId); } }); } - // TODO: consider just returning the IDs - private HomogeneousResourcePidList fetchResourceIdsPageWithUrl( - Date theStart, Date theEnd, int thePageSize, int theOffset, String theUrl, RequestPartitionId theRequestPartitionId) { + private List fetchResourceIdsPageWithUrl(int theOffset, String theUrl, RequestPartitionId theRequestPartitionId) { String resourceType = theUrl.substring(0, theUrl.indexOf('?')); RuntimeResourceDefinition def = myFhirContext.getResourceDefinition(resourceType); - // TODO: consider cleaning up date stuff that's no longer needed SearchParameterMap searchParamMap = myMatchUrlService.translateMatchUrl(theUrl, def); searchParamMap.setSort(new SortSpec(Constants.PARAM_ID, SortOrderEnum.ASC)); - DateRangeParam chunkDateRange = - DateRangeUtil.narrowDateRange(searchParamMap.getLastUpdated(), theStart, theEnd); - searchParamMap.setLastUpdated(chunkDateRange); - searchParamMap.setCount(thePageSize); - // TODO: try this: searchParamMap.setOffset(theOffset); + searchParamMap.setLoadSynchronousUpTo(myJpaStorageSettings.getInternalSynchronousSearchSize() + 1); IFhirResourceDao dao = myDaoRegistry.getResourceDao(resourceType); SystemRequestDetails request = new SystemRequestDetails(); request.setRequestPartitionId(theRequestPartitionId); - List ids = dao.searchForIds(searchParamMap, request); - - Date lastDate = null; - if (isNotEmpty(ids)) { - IResourcePersistentId lastResourcePersistentId = ids.get(ids.size() - 1); - lastDate = dao.readByPid(lastResourcePersistentId, true).getMeta().getLastUpdated(); - } - return new HomogeneousResourcePidList(resourceType, ids, lastDate, theRequestPartitionId); + return dao.searchForIds(searchParamMap, request); } @Nonnull diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java index b8f639dc7eec..ea48590a4fb0 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java @@ -2,7 +2,7 @@ import ca.uhn.fhir.interceptor.model.RequestPartitionId; import ca.uhn.fhir.jpa.api.config.JpaStorageSettings; -import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao; +import ca.uhn.fhir.jpa.api.dao.DaoRegistry; import ca.uhn.fhir.jpa.api.pid.IResourcePidList; import ca.uhn.fhir.jpa.api.svc.IBatch2DaoSvc; import ca.uhn.fhir.jpa.dao.tx.IHapiTransactionService; @@ -10,12 +10,15 @@ import ca.uhn.fhir.jpa.searchparam.MatchUrlService; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; import ca.uhn.fhir.jpa.test.BaseJpaR4Test; +import ca.uhn.fhir.model.primitive.IdDt; import ca.uhn.fhir.rest.api.server.IBundleProvider; import ca.uhn.fhir.rest.api.server.SystemRequestDetails; -import org.hl7.fhir.r4.model.Enumerations; +import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; +import org.hl7.fhir.instance.model.api.IIdType; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.mockito.Mock; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.springframework.beans.factory.annotation.Autowired; import javax.annotation.Nonnull; @@ -23,91 +26,101 @@ import java.time.Month; import java.time.ZoneId; import java.util.Date; +import java.util.List; import java.util.stream.IntStream; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; class Batch2DaoSvcImplTest extends BaseJpaR4Test { private static final Date PREVIOUS_MILLENNIUM = toDate(LocalDate.of(1999, Month.DECEMBER, 31)); private static final Date TOMORROW = toDate(LocalDate.now().plusDays(1)); private static final String URL_PATIENT_EXPUNGE_TRUE = "Patient?_expunge=true"; + private static final String PATIENT = "Patient"; + private static final int INTERNAL_SYNCHRONOUS_SEARCH_SIZE = 10; -// @Autowired - private IBatch2DaoSvc mySubject; + private final IHapiTransactionService myTransactionService = new NonTransactionalHapiTransactionService(); @Autowired private JpaStorageSettings myJpaStorageSettings; @Autowired private MatchUrlService myMatchUrlService; - private final IHapiTransactionService myTransactionService = new NonTransactionalHapiTransactionService(); + + private DaoRegistry mySpiedDaoRegistry; + + private IBatch2DaoSvc mySubject; @BeforeEach void beforeEach() { - myJpaStorageSettings.setInternalSynchronousSearchSize(10); - mySubject = new Batch2DaoSvcImpl(myResourceTableDao, myMatchUrlService, myDaoRegistry, myFhirContext, myTransactionService, myJpaStorageSettings); - } + myJpaStorageSettings.setInternalSynchronousSearchSize(INTERNAL_SYNCHRONOUS_SEARCH_SIZE); - @Test - void fetchResourcesByUrl_noResults() { - // TODO: grab value from the functional test and use them here - // TODO: try different variants of URLs and fail appropriately - final IResourcePidList resourcePidList = mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), URL_PATIENT_EXPUNGE_TRUE); + mySpiedDaoRegistry = spy(myDaoRegistry); - assertTrue(resourcePidList.isEmpty()); + mySubject = new Batch2DaoSvcImpl(myResourceTableDao, myMatchUrlService, mySpiedDaoRegistry, myFhirContext, myTransactionService, myJpaStorageSettings); } - @Test - void fetchResourcesByUrl_oneResultLessThanThreshold() { - final int expectedNumResults = 9; + // TODO: LD this test won't work with the nonUrl variant yet: error: No existing transaction found for transaction marked with propagation 'mandatory' - IntStream.range(0, expectedNumResults) - .forEach(num -> createPatient()); - - final IBundleProvider search = myPatientDao.search(SearchParameterMap.newSynchronous(), new SystemRequestDetails()); - // TODO: grab value from the functional test and use them here - // TODO: try different variants of URLs and fail appropriately - // TODO: today plus one day? - final IResourcePidList resourcePidList = mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), URL_PATIENT_EXPUNGE_TRUE); - - // TODO: figure out how to spy on results to figure out the number of dao calls - assertEquals(expectedNumResults, resourcePidList.size()); + @Test + void fetchResourcesByUrlEmptyUrl() { + try { + mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), ""); + } catch (InternalErrorException exception) { + assertEquals("HAPI-99999: this should never happen: URL is missing a '?'", exception.getMessage()); + } catch (Exception exception) { + fail("caught wrong Exception"); + } } @Test - void fetchResourcesByUrl_resultsEqualToThreshold() { - final int expectedNumResults = 10; + void fetchResourcesByUrlSingleQuestionMark() { + try { + mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), "?"); + } catch (InternalErrorException exception) { + assertEquals("HAPI-2223: theResourceName must not be blank", exception.getMessage()); + } catch (Exception exception) { + fail("caught wrong Exception"); + } + } - IntStream.range(0, expectedNumResults) - .forEach(num -> createPatient(withId("patient"+num))); + @ParameterizedTest + @ValueSource(ints = {0, 9, 10, 11, 21, 22, 23, 45}) + void fetchResourcesByUrl(int expectedNumResults) { + final List patientIds = IntStream.range(0, expectedNumResults) + .mapToObj(num -> createPatient()) + .toList(); - final IBundleProvider search = myPatientDao.search(SearchParameterMap.newSynchronous(), new SystemRequestDetails()); - // TODO: grab value from the functional test and use them here - // TODO: try different variants of URLs and fail appropriately - // TODO: today plus one day? final IResourcePidList resourcePidList = mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), URL_PATIENT_EXPUNGE_TRUE); - // TODO: figure out how to spy on results to figure out the number of dao calls - assertEquals(expectedNumResults, resourcePidList.size()); + final List actualPatientIds = + resourcePidList.getTypedResourcePids() + .stream() + .map(typePid -> new IdDt(typePid.resourceType, (Long) typePid.id.getId())) + .toList(); + assertIdsEqual(patientIds, actualPatientIds); + + verify(mySpiedDaoRegistry, times(getExpectedNumOfInvocations(expectedNumResults))).getResourceDao(PATIENT); } - // TODO: parameterized - @Test - void fetchResourcesByUrl_resultsOverThreshold() { - final int expectedNumResults = 11; + private int getExpectedNumOfInvocations(int expectedNumResults) { + final int maxResultsPerQuery = INTERNAL_SYNCHRONOUS_SEARCH_SIZE + 1; + final int division = expectedNumResults / maxResultsPerQuery; + return division + 1; + } - IntStream.range(0, expectedNumResults) - .forEach(num -> createPatient()); + private static void assertIdsEqual(List expectedResourceIds, List actualResourceIds) { + assertEquals(expectedResourceIds.size(), actualResourceIds.size()); - final IBundleProvider search = myPatientDao.search(SearchParameterMap.newSynchronous(), new SystemRequestDetails()); - // TODO: grab value from the functional test and use them here - // TODO: try different variants of URLs and fail appropriately - // TODO: today plus one day? - final IResourcePidList resourcePidList = mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), URL_PATIENT_EXPUNGE_TRUE); + for (int index = 0; index < expectedResourceIds.size(); index++) { + final IIdType expectedIdType = expectedResourceIds.get(index); + final IIdType actualIdType = actualResourceIds.get(index); - // TODO: figure out how to spy on results to figure out the number of dao calls - assertEquals(expectedNumResults, resourcePidList.size()); + assertEquals(expectedIdType.getResourceType(), actualIdType.getResourceType()); + assertEquals(expectedIdType.getIdPartAsLong(), actualIdType.getIdPartAsLong()); + } } @Nonnull diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java index e45e53ea56a8..c2f313a86f5b 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java @@ -75,11 +75,9 @@ public RunOutcome run( ourLog.info("Beginning scan for reindex IDs in range {} to {}", start, end); - Date nextStart = start; - RequestPartitionId requestPartitionId = - theStepExecutionDetails.getParameters().getRequestPartitionId(); + RequestPartitionId requestPartitionId = + theStepExecutionDetails.getParameters().getRequestPartitionId(); Set idBuffer = new LinkedHashSet<>(); - long previousLastTime = 0L; int totalIdsFound = 0; int chunkCount = 0; @@ -88,54 +86,40 @@ public RunOutcome run( // we won't go over MAX_BATCH_OF_IDS maxBatchId = Math.min(batchSize.intValue(), maxBatchId); } - // TODO: get rid of while(true) -// while (true) { - // TODO: get rid of the page size semantics here and deal with one huge IResourcePidList, but break it up into multiple chunks - IResourcePidList nextChunk = myIdChunkProducer.fetchResourceIdsPage( - nextStart, end, pageSize, requestPartitionId, theStepExecutionDetails.getData()); - - if (nextChunk.isEmpty()) { - ourLog.info("No data returned"); -// break; - } - - // TODO: do we need this anymore? - // If we get the same last time twice in a row, we've clearly reached the end - if (nextChunk.getLastDate().getTime() == previousLastTime) { - ourLog.info("Matching final timestamp of {}, loading is completed", new Date(previousLastTime)); -// break; - } - - ourLog.info("Found {} IDs from {} to {}", nextChunk.size(), nextStart, nextChunk.getLastDate()); - if (nextChunk.size() < 10 && HapiSystemProperties.isTestModeEnabled()) { - // TODO: I've added this in order to troubleshoot MultitenantBatchOperationR4Test - // which is failing intermittently. If that stops, makes sense to remove this - ourLog.info(" * PIDS: {}", nextChunk); - } - - for (TypedResourcePid typedResourcePid : nextChunk.getTypedResourcePids()) { - TypedPidJson nextId = new TypedPidJson(typedResourcePid); - idBuffer.add(nextId); - } - -// previousLastTime = nextChunk.getLastDate().getTime(); -// nextStart = nextChunk.getLastDate(); - - while (idBuffer.size() > maxBatchId) { - List submissionIds = new ArrayList<>(); - for (Iterator iter = idBuffer.iterator(); iter.hasNext(); ) { - submissionIds.add(iter.next()); - iter.remove(); - if (submissionIds.size() == maxBatchId) { - break; - } - } - - totalIdsFound += submissionIds.size(); - chunkCount++; - submitWorkChunk(submissionIds, nextChunk.getRequestPartitionId(), theDataSink); - } -// } + + // TODO: get rid of the page size semantics here and deal with one huge IResourcePidList, but break it up into multiple chunks + IResourcePidList nextChunk = myIdChunkProducer.fetchResourceIdsPage( + start, end, pageSize, requestPartitionId, theStepExecutionDetails.getData()); + + if (nextChunk.isEmpty()) { + ourLog.info("No data returned"); + } + + ourLog.debug("Found {} IDs from {} to {}", nextChunk.size(), start, nextChunk.getLastDate()); + if (nextChunk.size() < 10 && HapiSystemProperties.isTestModeEnabled()) { + // which is failing intermittently. If that stops, makes sense to remove this + ourLog.debug(" * PIDS: {}", nextChunk); + } + + for (TypedResourcePid typedResourcePid : nextChunk.getTypedResourcePids()) { + TypedPidJson nextId = new TypedPidJson(typedResourcePid); + idBuffer.add(nextId); + } + + while (idBuffer.size() > maxBatchId) { + List submissionIds = new ArrayList<>(); + for (Iterator iter = idBuffer.iterator(); iter.hasNext(); ) { + submissionIds.add(iter.next()); + iter.remove(); + if (submissionIds.size() == maxBatchId) { + break; + } + } + + totalIdsFound += submissionIds.size(); + chunkCount++; + submitWorkChunk(submissionIds, nextChunk.getRequestPartitionId(), theDataSink); + } totalIdsFound += idBuffer.size(); chunkCount++; diff --git a/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/jobs/step/LoadIdsStepTest.java b/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/jobs/step/LoadIdsStepTest.java index bb4bc41ba21e..8b42d252da11 100644 --- a/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/jobs/step/LoadIdsStepTest.java +++ b/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/jobs/step/LoadIdsStepTest.java @@ -33,7 +33,6 @@ import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) public class LoadIdsStepTest { @@ -93,9 +92,8 @@ public void testGenerateSteps() { String actual = allCapturedValues.get(i).toString(); assertEquals(expected, actual); } - // TODO: figure out what this test is about - assertEquals(createIdChunk(40000, 40040).toString(), - allCapturedValues.get(expectedLoops -1).toString()); + final ResourceIdListWorkChunkJson expectedIdChunk = createIdChunk(19500, 20000); + assertEquals(expectedIdChunk.toString(), allCapturedValues.get(expectedLoops -1).toString()); } @Nonnull From fe106cf479adeb84067028f50b3c15d6f1d0ff3c Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Fri, 28 Jul 2023 16:39:23 -0400 Subject: [PATCH 03/17] Spotless --- .../fhir/jpa/config/Batch2SupportConfig.java | 16 ++++++- .../fhir/jpa/reindex/Batch2DaoSvcImpl.java | 29 ++++++++----- .../batch2/jobs/step/ResourceIdListStep.java | 43 ++++++++++--------- 3 files changed, 55 insertions(+), 33 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/Batch2SupportConfig.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/Batch2SupportConfig.java index 5824fc7c0b3f..0ea5cae51758 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/Batch2SupportConfig.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/Batch2SupportConfig.java @@ -42,8 +42,20 @@ public class Batch2SupportConfig { @Bean - public IBatch2DaoSvc batch2DaoSvc(IResourceTableDao theResourceTableDao, MatchUrlService theMatchUrlService, DaoRegistry theDaoRegistry, FhirContext theFhirContext, IHapiTransactionService theTransactionService, JpaStorageSettings theJpaStorageSettings) { - return new Batch2DaoSvcImpl(theResourceTableDao, theMatchUrlService, theDaoRegistry, theFhirContext, theTransactionService, theJpaStorageSettings); + public IBatch2DaoSvc batch2DaoSvc( + IResourceTableDao theResourceTableDao, + MatchUrlService theMatchUrlService, + DaoRegistry theDaoRegistry, + FhirContext theFhirContext, + IHapiTransactionService theTransactionService, + JpaStorageSettings theJpaStorageSettings) { + return new Batch2DaoSvcImpl( + theResourceTableDao, + theMatchUrlService, + theDaoRegistry, + theFhirContext, + theTransactionService, + theJpaStorageSettings); } @Bean diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java index 961343e0065b..012151033d15 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java @@ -51,8 +51,6 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; -import static org.apache.commons.collections4.CollectionUtils.isNotEmpty; - public class Batch2DaoSvcImpl implements IBatch2DaoSvc { private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(Batch2DaoSvcImpl.class); @@ -73,7 +71,13 @@ public boolean isAllResourceTypeSupported() { return true; } - public Batch2DaoSvcImpl(IResourceTableDao theResourceTableDao, MatchUrlService theMatchUrlService, DaoRegistry theDaoRegistry, FhirContext theFhirContext, IHapiTransactionService theTransactionService, JpaStorageSettings theJpaStorageSettings) { + public Batch2DaoSvcImpl( + IResourceTableDao theResourceTableDao, + MatchUrlService theMatchUrlService, + DaoRegistry theDaoRegistry, + FhirContext theFhirContext, + IHapiTransactionService theTransactionService, + JpaStorageSettings theJpaStorageSettings) { myResourceTableDao = theResourceTableDao; myMatchUrlService = theMatchUrlService; myDaoRegistry = theDaoRegistry; @@ -97,20 +101,24 @@ public IResourcePidList fetchResourceIdsPage( return fetchResourceIdsPageNoUrl(theStart, theEnd, thePageSize, theRequestPartitionId); } else { if (!theUrl.contains("?")) { - throw new InternalErrorException("HAPI-99999: this should never happen: URL is missing a '?'"); + throw new InternalErrorException( + "HAPI-99999: this should never happen: URL is missing a '?'"); } - final Integer internalSynchronousSearchSize = myJpaStorageSettings.getInternalSynchronousSearchSize(); + final Integer internalSynchronousSearchSize = + myJpaStorageSettings.getInternalSynchronousSearchSize(); - if (internalSynchronousSearchSize == null || internalSynchronousSearchSize <= 0) { + if (internalSynchronousSearchSize == null || internalSynchronousSearchSize <= 0) { // TODO: new HAPI code - throw new InternalErrorException("HAPI-99999: this should never happen: internalSynchronousSearchSize is null or less than or equal to 0"); + throw new InternalErrorException( + "HAPI-99999: this should never happen: internalSynchronousSearchSize is null or less than or equal to 0"); } - List currentIds = fetchResourceIdsPageWithUrl(0, theUrl, theRequestPartitionId); + List currentIds = + fetchResourceIdsPageWithUrl(0, theUrl, theRequestPartitionId); ourLog.info("FIRST currentIds: {}", currentIds.size()); final List allIds = new ArrayList<>(currentIds); - while (internalSynchronousSearchSize < currentIds.size() ) { + while (internalSynchronousSearchSize < currentIds.size()) { currentIds = fetchResourceIdsPageWithUrl(allIds.size(), theUrl, theRequestPartitionId); ourLog.info("NEXT currentIds: {}", currentIds.size()); allIds.addAll(currentIds); @@ -123,7 +131,8 @@ public IResourcePidList fetchResourceIdsPage( }); } - private List fetchResourceIdsPageWithUrl(int theOffset, String theUrl, RequestPartitionId theRequestPartitionId) { + private List fetchResourceIdsPageWithUrl( + int theOffset, String theUrl, RequestPartitionId theRequestPartitionId) { String resourceType = theUrl.substring(0, theUrl.indexOf('?')); RuntimeResourceDefinition def = myFhirContext.getResourceDefinition(resourceType); diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java index c2f313a86f5b..f2a82acaa39b 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java @@ -75,8 +75,8 @@ public RunOutcome run( ourLog.info("Beginning scan for reindex IDs in range {} to {}", start, end); - RequestPartitionId requestPartitionId = - theStepExecutionDetails.getParameters().getRequestPartitionId(); + RequestPartitionId requestPartitionId = + theStepExecutionDetails.getParameters().getRequestPartitionId(); Set idBuffer = new LinkedHashSet<>(); int totalIdsFound = 0; int chunkCount = 0; @@ -87,38 +87,39 @@ public RunOutcome run( maxBatchId = Math.min(batchSize.intValue(), maxBatchId); } - // TODO: get rid of the page size semantics here and deal with one huge IResourcePidList, but break it up into multiple chunks + // TODO: get rid of the page size semantics here and deal with one huge IResourcePidList, but break it up into + // multiple chunks IResourcePidList nextChunk = myIdChunkProducer.fetchResourceIdsPage( - start, end, pageSize, requestPartitionId, theStepExecutionDetails.getData()); + start, end, pageSize, requestPartitionId, theStepExecutionDetails.getData()); if (nextChunk.isEmpty()) { - ourLog.info("No data returned"); + ourLog.info("No data returned"); } ourLog.debug("Found {} IDs from {} to {}", nextChunk.size(), start, nextChunk.getLastDate()); if (nextChunk.size() < 10 && HapiSystemProperties.isTestModeEnabled()) { - // which is failing intermittently. If that stops, makes sense to remove this - ourLog.debug(" * PIDS: {}", nextChunk); + // which is failing intermittently. If that stops, makes sense to remove this + ourLog.debug(" * PIDS: {}", nextChunk); } for (TypedResourcePid typedResourcePid : nextChunk.getTypedResourcePids()) { - TypedPidJson nextId = new TypedPidJson(typedResourcePid); - idBuffer.add(nextId); + TypedPidJson nextId = new TypedPidJson(typedResourcePid); + idBuffer.add(nextId); } while (idBuffer.size() > maxBatchId) { - List submissionIds = new ArrayList<>(); - for (Iterator iter = idBuffer.iterator(); iter.hasNext(); ) { - submissionIds.add(iter.next()); - iter.remove(); - if (submissionIds.size() == maxBatchId) { - break; - } - } - - totalIdsFound += submissionIds.size(); - chunkCount++; - submitWorkChunk(submissionIds, nextChunk.getRequestPartitionId(), theDataSink); + List submissionIds = new ArrayList<>(); + for (Iterator iter = idBuffer.iterator(); iter.hasNext(); ) { + submissionIds.add(iter.next()); + iter.remove(); + if (submissionIds.size() == maxBatchId) { + break; + } + } + + totalIdsFound += submissionIds.size(); + chunkCount++; + submitWorkChunk(submissionIds, nextChunk.getRequestPartitionId(), theDataSink); } totalIdsFound += idBuffer.size(); From 078e230552adf651cedb100349170a10cc9b1780 Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Fri, 28 Jul 2023 16:53:55 -0400 Subject: [PATCH 04/17] Fix checkstyle errors. --- .../java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java index 012151033d15..659a936f6813 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java @@ -21,6 +21,7 @@ import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.context.RuntimeResourceDefinition; +import ca.uhn.fhir.i18n.Msg; import ca.uhn.fhir.interceptor.model.RequestPartitionId; import ca.uhn.fhir.jpa.api.config.JpaStorageSettings; import ca.uhn.fhir.jpa.api.dao.DaoRegistry; @@ -102,16 +103,16 @@ public IResourcePidList fetchResourceIdsPage( } else { if (!theUrl.contains("?")) { throw new InternalErrorException( - "HAPI-99999: this should never happen: URL is missing a '?'"); + Msg.code(2422) + ": this should never happen: URL is missing a '?'"); } final Integer internalSynchronousSearchSize = myJpaStorageSettings.getInternalSynchronousSearchSize(); if (internalSynchronousSearchSize == null || internalSynchronousSearchSize <= 0) { - // TODO: new HAPI code throw new InternalErrorException( - "HAPI-99999: this should never happen: internalSynchronousSearchSize is null or less than or equal to 0"); + Msg.code(2423) + + ": this should never happen: internalSynchronousSearchSize is null or less than or equal to 0"); } List currentIds = From 6ba6ea8bd740d4f34e84b1e866535e391be2ddcc Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Mon, 31 Jul 2023 12:23:48 -0400 Subject: [PATCH 05/17] Fix test failures. --- .../fhir/jpa/reindex/Batch2DaoSvcImpl.java | 2 +- .../r4/MultitenantBatchOperationR4Test.java | 4 +-- .../jpa/reindex/Batch2DaoSvcImplTest.java | 5 +--- .../reindex/ResourceReindexSvcImplTest.java | 25 ++++++++++--------- 4 files changed, 16 insertions(+), 20 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java index 659a936f6813..2f887568557d 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java @@ -103,7 +103,7 @@ public IResourcePidList fetchResourceIdsPage( } else { if (!theUrl.contains("?")) { throw new InternalErrorException( - Msg.code(2422) + ": this should never happen: URL is missing a '?'"); + Msg.code(2422) + "this should never happen: URL is missing a '?'"); } final Integer internalSynchronousSearchSize = diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/MultitenantBatchOperationR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/MultitenantBatchOperationR4Test.java index d0b6638dccee..ef1acc0bf6c7 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/MultitenantBatchOperationR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/MultitenantBatchOperationR4Test.java @@ -32,10 +32,8 @@ import java.util.stream.Collectors; import static ca.uhn.fhir.jpa.model.util.JpaConstants.DEFAULT_PARTITION_NAME; -import static org.awaitility.Awaitility.await; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.hasSize; -import static org.hamcrest.Matchers.in; import static org.hamcrest.Matchers.isA; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -108,7 +106,7 @@ public void testDeleteExpungeOperation() { String jobId = BatchHelperR4.jobIdFromBatch2Parameters(response); myBatch2JobHelper.awaitJobCompletion(jobId); - assertThat(interceptor.requestPartitionIds, hasSize(5)); + assertThat(interceptor.requestPartitionIds, hasSize(4)); RequestPartitionId partitionId = interceptor.requestPartitionIds.get(0); assertEquals(TENANT_B_ID, partitionId.getFirstPartitionIdOrNull()); assertEquals(TENANT_B, partitionId.getFirstPartitionNameOrNull()); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java index ea48590a4fb0..1bacf2d25285 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java @@ -8,11 +8,8 @@ import ca.uhn.fhir.jpa.dao.tx.IHapiTransactionService; import ca.uhn.fhir.jpa.dao.tx.NonTransactionalHapiTransactionService; import ca.uhn.fhir.jpa.searchparam.MatchUrlService; -import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; import ca.uhn.fhir.jpa.test.BaseJpaR4Test; import ca.uhn.fhir.model.primitive.IdDt; -import ca.uhn.fhir.rest.api.server.IBundleProvider; -import ca.uhn.fhir.rest.api.server.SystemRequestDetails; import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; import org.hl7.fhir.instance.model.api.IIdType; import org.junit.jupiter.api.BeforeEach; @@ -69,7 +66,7 @@ void fetchResourcesByUrlEmptyUrl() { try { mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), ""); } catch (InternalErrorException exception) { - assertEquals("HAPI-99999: this should never happen: URL is missing a '?'", exception.getMessage()); + assertEquals("HAPI-2422: this should never happen: URL is missing a '?'", exception.getMessage()); } catch (Exception exception) { fail("caught wrong Exception"); } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/ResourceReindexSvcImplTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/ResourceReindexSvcImplTest.java index 01b2afeeadfd..56c09865ea01 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/ResourceReindexSvcImplTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/ResourceReindexSvcImplTest.java @@ -3,16 +3,13 @@ import ca.uhn.fhir.jpa.api.pid.IResourcePidList; import ca.uhn.fhir.jpa.api.pid.TypedResourcePid; import ca.uhn.fhir.jpa.api.svc.IBatch2DaoSvc; -import ca.uhn.fhir.jpa.model.dao.JpaPid; import ca.uhn.fhir.jpa.test.BaseJpaR4Test; -import org.hl7.fhir.r4.model.DateType; -import org.hl7.fhir.r4.model.InstantType; +import org.hl7.fhir.instance.model.api.IIdType; import org.junit.jupiter.api.MethodOrderer; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestMethodOrder; import org.springframework.beans.factory.annotation.Autowired; -import java.util.ArrayList; import java.util.Date; import java.util.List; @@ -111,26 +108,26 @@ public void testFetchResourceIdsPage_WithUrl_WithData() { // Setup - createPatient(withActiveFalse()); + final Long patientId0 = createPatient(withActiveFalse()).getIdPartAsLong(); sleepUntilTimeChanges(); // Start of resources within range Date start = new Date(); sleepUntilTimeChanges(); - Long id0 = createPatient(withActiveFalse()).getIdPartAsLong(); + Long patientId1 = createPatient(withActiveFalse()).getIdPartAsLong(); createObservation(withObservationCode("http://foo", "bar")); createObservation(withObservationCode("http://foo", "bar")); sleepUntilTimeChanges(); Date beforeLastInRange = new Date(); sleepUntilTimeChanges(); - Long id1 = createPatient(withActiveFalse()).getIdPartAsLong(); + Long patientId2 = createPatient(withActiveFalse()).getIdPartAsLong(); sleepUntilTimeChanges(); Date end = new Date(); sleepUntilTimeChanges(); // End of resources within range createObservation(withObservationCode("http://foo", "bar")); - createPatient(withActiveFalse()); + final Long patientId3 = createPatient(withActiveFalse()).getIdPartAsLong(); sleepUntilTimeChanges(); // Execute @@ -140,13 +137,17 @@ public void testFetchResourceIdsPage_WithUrl_WithData() { // Verify - assertEquals(2, page.size()); + assertEquals(4, page.size()); List typedResourcePids = page.getTypedResourcePids(); - assertThat(page.getTypedResourcePids(), contains(new TypedResourcePid("Patient", id0), new TypedResourcePid("Patient", id1))); + assertThat(page.getTypedResourcePids(), + contains(new TypedResourcePid("Patient", patientId0), + new TypedResourcePid("Patient", patientId1), + new TypedResourcePid("Patient", patientId2), + new TypedResourcePid("Patient", patientId3))); assertTrue(page.getLastDate().after(beforeLastInRange)); - assertTrue(page.getLastDate().before(end)); + assertTrue(page.getLastDate().before(end) || page.getLastDate().equals(end)); - assertEquals(3, myCaptureQueriesListener.logSelectQueries().size()); + assertEquals(1, myCaptureQueriesListener.logSelectQueries().size()); assertEquals(0, myCaptureQueriesListener.countInsertQueries()); assertEquals(0, myCaptureQueriesListener.countUpdateQueries()); assertEquals(0, myCaptureQueriesListener.countDeleteQueries()); From fc97ee784f788ba3f2e5917436783bce88c93978 Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Mon, 31 Jul 2023 14:04:53 -0400 Subject: [PATCH 06/17] Minor refactoring. New unit test. Finalize changelist. --- ...e-over-10k-resources-deletes-only-10k.yaml | 5 ++ .../fhir/changelog/6_8_0/9999-something.yaml | 4 -- .../fhir/jpa/reindex/Batch2DaoSvcImpl.java | 63 +++++++++++-------- .../jpa/reindex/Batch2DaoSvcImplTest.java | 11 ++++ .../batch2/jobs/step/ResourceIdListStep.java | 2 - 5 files changed, 52 insertions(+), 33 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/5150--delete-expunge-over-10k-resources-deletes-only-10k.yaml delete mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/9999-something.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/5150--delete-expunge-over-10k-resources-deletes-only-10k.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/5150--delete-expunge-over-10k-resources-deletes-only-10k.yaml new file mode 100644 index 000000000000..64666cbcf693 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/5150--delete-expunge-over-10k-resources-deletes-only-10k.yaml @@ -0,0 +1,5 @@ +--- +type: fix +issue: 5150 +title: "When running a $delete-expunge with over 10,000 resources, only the first 10,000 resources were deleted. + This is now fixed." diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/9999-something.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/9999-something.yaml deleted file mode 100644 index 953d776ff4f6..000000000000 --- a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_8_0/9999-something.yaml +++ /dev/null @@ -1,4 +0,0 @@ ---- -type: fix -issue: 9999 -title: "Something something" diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java index 2f887568557d..47a24b1c4d88 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java @@ -101,37 +101,46 @@ public IResourcePidList fetchResourceIdsPage( if (theUrl == null) { return fetchResourceIdsPageNoUrl(theStart, theEnd, thePageSize, theRequestPartitionId); } else { - if (!theUrl.contains("?")) { - throw new InternalErrorException( - Msg.code(2422) + "this should never happen: URL is missing a '?'"); - } - - final Integer internalSynchronousSearchSize = - myJpaStorageSettings.getInternalSynchronousSearchSize(); - - if (internalSynchronousSearchSize == null || internalSynchronousSearchSize <= 0) { - throw new InternalErrorException( - Msg.code(2423) - + ": this should never happen: internalSynchronousSearchSize is null or less than or equal to 0"); - } - - List currentIds = - fetchResourceIdsPageWithUrl(0, theUrl, theRequestPartitionId); - ourLog.info("FIRST currentIds: {}", currentIds.size()); - final List allIds = new ArrayList<>(currentIds); - while (internalSynchronousSearchSize < currentIds.size()) { - currentIds = fetchResourceIdsPageWithUrl(allIds.size(), theUrl, theRequestPartitionId); - ourLog.info("NEXT currentIds: {}", currentIds.size()); - allIds.addAll(currentIds); - } - - final String resourceType = theUrl.substring(0, theUrl.indexOf('?')); - - return new HomogeneousResourcePidList(resourceType, allIds, theEnd, theRequestPartitionId); + return fetchResourceIdsPageWithUrl(theEnd, theUrl, theRequestPartitionId); } }); } + @Nonnull + private HomogeneousResourcePidList fetchResourceIdsPageWithUrl(Date theEnd, @Nonnull String theUrl, @Nullable RequestPartitionId theRequestPartitionId) { + if (!theUrl.contains("?")) { + throw new InternalErrorException( + Msg.code(2422) + "this should never happen: URL is missing a '?'"); + } + + final Integer internalSynchronousSearchSize = + myJpaStorageSettings.getInternalSynchronousSearchSize(); + + if (internalSynchronousSearchSize == null || internalSynchronousSearchSize <= 0) { + throw new InternalErrorException( + Msg.code(2423) + + "this should never happen: internalSynchronousSearchSize is null or less than or equal to 0"); + } + + List currentIds = + fetchResourceIdsPageWithUrl(0, theUrl, theRequestPartitionId); + ourLog.debug("FIRST currentIds: {}", currentIds.size()); + + final List allIds = new ArrayList<>(currentIds); + + while (internalSynchronousSearchSize < currentIds.size()) { + // Ensure the offset is set to the last ID in the cumulative List, otherwise, we'll be stuck in an infinite loop here: + currentIds = fetchResourceIdsPageWithUrl(allIds.size(), theUrl, theRequestPartitionId); + ourLog.debug("NEXT currentIds: {}", currentIds.size()); + + allIds.addAll(currentIds); + } + + final String resourceType = theUrl.substring(0, theUrl.indexOf('?')); + + return new HomogeneousResourcePidList(resourceType, allIds, theEnd, theRequestPartitionId); + } + private List fetchResourceIdsPageWithUrl( int theOffset, String theUrl, RequestPartitionId theRequestPartitionId) { String resourceType = theUrl.substring(0, theUrl.indexOf('?')); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java index 1bacf2d25285..6eee1041d982 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java @@ -83,6 +83,17 @@ void fetchResourcesByUrlSingleQuestionMark() { } } + @Test + void fetchResourcesByUrlNonsensicalResource() { + try { + mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), "Banana?_expunge=true"); + } catch (InternalErrorException exception) { + assertEquals("HAPI-2223: HAPI-1684: Unknown resource name \"Banana\" (this name is not known in FHIR version \"R4\")", exception.getMessage()); + } catch (Exception exception) { + fail("caught wrong Exception"); + } + } + @ParameterizedTest @ValueSource(ints = {0, 9, 10, 11, 21, 22, 23, 45}) void fetchResourcesByUrl(int expectedNumResults) { diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java index f2a82acaa39b..c677687af855 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java @@ -87,8 +87,6 @@ public RunOutcome run( maxBatchId = Math.min(batchSize.intValue(), maxBatchId); } - // TODO: get rid of the page size semantics here and deal with one huge IResourcePidList, but break it up into - // multiple chunks IResourcePidList nextChunk = myIdChunkProducer.fetchResourceIdsPage( start, end, pageSize, requestPartitionId, theStepExecutionDetails.getData()); From 910c49cfdc907ae54500cc311286f27523793edc Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Mon, 31 Jul 2023 14:07:52 -0400 Subject: [PATCH 07/17] Spotless fix. --- .../fhir/jpa/reindex/Batch2DaoSvcImpl.java | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java index 47a24b1c4d88..0c67a11a19cd 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java @@ -107,29 +107,27 @@ public IResourcePidList fetchResourceIdsPage( } @Nonnull - private HomogeneousResourcePidList fetchResourceIdsPageWithUrl(Date theEnd, @Nonnull String theUrl, @Nullable RequestPartitionId theRequestPartitionId) { + private HomogeneousResourcePidList fetchResourceIdsPageWithUrl( + Date theEnd, @Nonnull String theUrl, @Nullable RequestPartitionId theRequestPartitionId) { if (!theUrl.contains("?")) { - throw new InternalErrorException( - Msg.code(2422) + "this should never happen: URL is missing a '?'"); + throw new InternalErrorException(Msg.code(2422) + "this should never happen: URL is missing a '?'"); } - final Integer internalSynchronousSearchSize = - myJpaStorageSettings.getInternalSynchronousSearchSize(); + final Integer internalSynchronousSearchSize = myJpaStorageSettings.getInternalSynchronousSearchSize(); if (internalSynchronousSearchSize == null || internalSynchronousSearchSize <= 0) { - throw new InternalErrorException( - Msg.code(2423) - + "this should never happen: internalSynchronousSearchSize is null or less than or equal to 0"); + throw new InternalErrorException(Msg.code(2423) + + "this should never happen: internalSynchronousSearchSize is null or less than or equal to 0"); } - List currentIds = - fetchResourceIdsPageWithUrl(0, theUrl, theRequestPartitionId); + List currentIds = fetchResourceIdsPageWithUrl(0, theUrl, theRequestPartitionId); ourLog.debug("FIRST currentIds: {}", currentIds.size()); final List allIds = new ArrayList<>(currentIds); while (internalSynchronousSearchSize < currentIds.size()) { - // Ensure the offset is set to the last ID in the cumulative List, otherwise, we'll be stuck in an infinite loop here: + // Ensure the offset is set to the last ID in the cumulative List, otherwise, we'll be stuck in an infinite + // loop here: currentIds = fetchResourceIdsPageWithUrl(allIds.size(), theUrl, theRequestPartitionId); ourLog.debug("NEXT currentIds: {}", currentIds.size()); From 301315e8bb78b147fdce827216dfff4ae39ff5fe Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Mon, 31 Jul 2023 14:12:13 -0400 Subject: [PATCH 08/17] Delete now useless code from unit test. --- .../uhn/fhir/batch2/jobs/step/LoadIdsStepTest.java | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/jobs/step/LoadIdsStepTest.java b/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/jobs/step/LoadIdsStepTest.java index 8b42d252da11..ac06d4ba3ced 100644 --- a/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/jobs/step/LoadIdsStepTest.java +++ b/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/jobs/step/LoadIdsStepTest.java @@ -6,7 +6,6 @@ import ca.uhn.fhir.batch2.jobs.chunk.ResourceIdListWorkChunkJson; import ca.uhn.fhir.batch2.jobs.parameters.PartitionedUrlListJobParameters; import ca.uhn.fhir.batch2.model.JobInstance; -import ca.uhn.fhir.jpa.api.pid.EmptyResourcePidList; import ca.uhn.fhir.jpa.api.pid.HomogeneousResourcePidList; import ca.uhn.fhir.jpa.api.pid.IResourcePidList; import ca.uhn.fhir.jpa.api.svc.IBatch2DaoSvc; @@ -30,17 +29,15 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isNull; -import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) public class LoadIdsStepTest { public static final Date DATE_1 = new InstantType("2022-01-01T00:00:00Z").getValue(); public static final Date DATE_2 = new InstantType("2022-01-02T00:00:00Z").getValue(); - public static final Date DATE_3 = new InstantType("2022-01-03T00:00:00Z").getValue(); - public static final Date DATE_4 = new InstantType("2022-01-04T00:00:00Z").getValue(); public static final Date DATE_END = new InstantType("2022-02-01T00:00:00Z").getValue(); @Mock @@ -71,18 +68,11 @@ public void testGenerateSteps() { // First Execution - lenient().when(myBatch2DaoSvc.fetchResourceIdsPage(eq(DATE_1), eq(DATE_END), eq(DEFAULT_PAGE_SIZE), isNull(), isNull())) + when(myBatch2DaoSvc.fetchResourceIdsPage(eq(DATE_1), eq(DATE_END), eq(DEFAULT_PAGE_SIZE), isNull(), isNull())) .thenReturn(createIdChunk(0L, 20000L, DATE_2)); - lenient().when(myBatch2DaoSvc.fetchResourceIdsPage(eq(DATE_2), eq(DATE_END), eq(DEFAULT_PAGE_SIZE), isNull(), isNull())) - .thenReturn(createIdChunk(20000L, 40000L, DATE_3)); - lenient().when(myBatch2DaoSvc.fetchResourceIdsPage(eq(DATE_3), eq(DATE_END), eq(DEFAULT_PAGE_SIZE), isNull(), isNull())) - .thenReturn(createIdChunk(40000L, 40040L, DATE_4)); - lenient().when(myBatch2DaoSvc.fetchResourceIdsPage(eq(DATE_4), eq(DATE_END), eq(DEFAULT_PAGE_SIZE), isNull(), isNull())) - .thenReturn(new EmptyResourcePidList()); mySvc.run(details, mySink); - // TODO: figure out the semantics of the new behaviour to make this pass: final int expectedLoops = 40; verify(mySink, times(40)).accept(myChunkIdsCaptor.capture()); From fedb7c834bb0f6a95935aed1e7d2391d846fabfc Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Mon, 31 Jul 2023 14:14:20 -0400 Subject: [PATCH 09/17] Delete more useless code. --- .../java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java index c677687af855..b55888362c79 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java @@ -95,10 +95,6 @@ public RunOutcome run( } ourLog.debug("Found {} IDs from {} to {}", nextChunk.size(), start, nextChunk.getLastDate()); - if (nextChunk.size() < 10 && HapiSystemProperties.isTestModeEnabled()) { - // which is failing intermittently. If that stops, makes sense to remove this - ourLog.debug(" * PIDS: {}", nextChunk); - } for (TypedResourcePid typedResourcePid : nextChunk.getTypedResourcePids()) { TypedPidJson nextId = new TypedPidJson(typedResourcePid); From 289e49cb364aaf963763aa38afec62d4490f234c Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Mon, 31 Jul 2023 14:18:36 -0400 Subject: [PATCH 10/17] Test pre-commit hook --- .../java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java index b55888362c79..89a8e15646b3 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java @@ -91,7 +91,7 @@ public RunOutcome run( start, end, pageSize, requestPartitionId, theStepExecutionDetails.getData()); if (nextChunk.isEmpty()) { - ourLog.info("No data returned"); + ourLog.info("No data returned"); //x } ourLog.debug("Found {} IDs from {} to {}", nextChunk.size(), start, nextChunk.getLastDate()); From 6f562605188061c9ff266649432608eb9bd6c4b8 Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Mon, 31 Jul 2023 14:20:36 -0400 Subject: [PATCH 11/17] More spotless fixes. --- .../java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java index 89a8e15646b3..b6afe1940e42 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java @@ -31,7 +31,6 @@ import ca.uhn.fhir.interceptor.model.RequestPartitionId; import ca.uhn.fhir.jpa.api.pid.IResourcePidList; import ca.uhn.fhir.jpa.api.pid.TypedResourcePid; -import ca.uhn.fhir.system.HapiSystemProperties; import ca.uhn.fhir.util.Logs; import org.slf4j.Logger; @@ -91,7 +90,7 @@ public RunOutcome run( start, end, pageSize, requestPartitionId, theStepExecutionDetails.getData()); if (nextChunk.isEmpty()) { - ourLog.info("No data returned"); //x + ourLog.info("No data returned"); } ourLog.debug("Found {} IDs from {} to {}", nextChunk.size(), start, nextChunk.getLastDate()); From 32e2c8fcc0d85625796e83b722f6b170db2cf83a Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Wed, 2 Aug 2023 13:56:41 -0400 Subject: [PATCH 12/17] Address most code review feedback. --- .../jpa/reindex/Batch2DaoSvcImplTest.java | 30 +++++------------ .../batch2/jobs/step/ResourceIdListStep.java | 33 +++++++------------ 2 files changed, 20 insertions(+), 43 deletions(-) diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java index 6eee1041d982..41f52a7db8aa 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java @@ -63,35 +63,23 @@ void beforeEach() { @Test void fetchResourcesByUrlEmptyUrl() { - try { - mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), ""); - } catch (InternalErrorException exception) { - assertEquals("HAPI-2422: this should never happen: URL is missing a '?'", exception.getMessage()); - } catch (Exception exception) { - fail("caught wrong Exception"); - } + final InternalErrorException exception = assertThrows(InternalErrorException.class, () -> mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), "")); + + assertEquals("HAPI-2422: this should never happen: URL is missing a '?'", exception.getMessage()); } @Test void fetchResourcesByUrlSingleQuestionMark() { - try { - mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), "?"); - } catch (InternalErrorException exception) { - assertEquals("HAPI-2223: theResourceName must not be blank", exception.getMessage()); - } catch (Exception exception) { - fail("caught wrong Exception"); - } + final InternalErrorException exception = assertThrows(InternalErrorException.class, () -> mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), "?")); + + assertEquals("HAPI-2223: theResourceName must not be blank", exception.getMessage()); } @Test void fetchResourcesByUrlNonsensicalResource() { - try { - mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), "Banana?_expunge=true"); - } catch (InternalErrorException exception) { - assertEquals("HAPI-2223: HAPI-1684: Unknown resource name \"Banana\" (this name is not known in FHIR version \"R4\")", exception.getMessage()); - } catch (Exception exception) { - fail("caught wrong Exception"); - } + final InternalErrorException exception = assertThrows(InternalErrorException.class, () -> mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, 800, RequestPartitionId.defaultPartition(), "Banana?_expunge=true")); + + assertEquals("HAPI-2223: HAPI-1684: Unknown resource name \"Banana\" (this name is not known in FHIR version \"R4\")", exception.getMessage()); } @ParameterizedTest diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java index b6afe1940e42..c5b4f289ebcd 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/jobs/step/ResourceIdListStep.java @@ -30,17 +30,17 @@ import ca.uhn.fhir.batch2.jobs.parameters.PartitionedJobParameters; import ca.uhn.fhir.interceptor.model.RequestPartitionId; import ca.uhn.fhir.jpa.api.pid.IResourcePidList; -import ca.uhn.fhir.jpa.api.pid.TypedResourcePid; import ca.uhn.fhir.util.Logs; +import com.google.common.collect.Iterators; +import com.google.common.collect.UnmodifiableIterator; import org.slf4j.Logger; -import java.util.ArrayList; import java.util.Collection; import java.util.Date; -import java.util.Iterator; import java.util.LinkedHashSet; import java.util.List; import java.util.Set; +import java.util.stream.Collectors; import javax.annotation.Nonnull; public class ResourceIdListStep @@ -76,7 +76,6 @@ public RunOutcome run( RequestPartitionId requestPartitionId = theStepExecutionDetails.getParameters().getRequestPartitionId(); - Set idBuffer = new LinkedHashSet<>(); int totalIdsFound = 0; int chunkCount = 0; @@ -86,7 +85,7 @@ public RunOutcome run( maxBatchId = Math.min(batchSize.intValue(), maxBatchId); } - IResourcePidList nextChunk = myIdChunkProducer.fetchResourceIdsPage( + final IResourcePidList nextChunk = myIdChunkProducer.fetchResourceIdsPage( start, end, pageSize, requestPartitionId, theStepExecutionDetails.getData()); if (nextChunk.isEmpty()) { @@ -95,30 +94,20 @@ public RunOutcome run( ourLog.debug("Found {} IDs from {} to {}", nextChunk.size(), start, nextChunk.getLastDate()); - for (TypedResourcePid typedResourcePid : nextChunk.getTypedResourcePids()) { - TypedPidJson nextId = new TypedPidJson(typedResourcePid); - idBuffer.add(nextId); - } + final Set idBuffer = nextChunk.getTypedResourcePids().stream() + .map(TypedPidJson::new) + .collect(Collectors.toCollection(LinkedHashSet::new)); + + final UnmodifiableIterator> partition = Iterators.partition(idBuffer.iterator(), maxBatchId); - while (idBuffer.size() > maxBatchId) { - List submissionIds = new ArrayList<>(); - for (Iterator iter = idBuffer.iterator(); iter.hasNext(); ) { - submissionIds.add(iter.next()); - iter.remove(); - if (submissionIds.size() == maxBatchId) { - break; - } - } + while (partition.hasNext()) { + final List submissionIds = partition.next(); totalIdsFound += submissionIds.size(); chunkCount++; submitWorkChunk(submissionIds, nextChunk.getRequestPartitionId(), theDataSink); } - totalIdsFound += idBuffer.size(); - chunkCount++; - submitWorkChunk(idBuffer, requestPartitionId, theDataSink); - ourLog.info("Submitted {} chunks with {} resource IDs", chunkCount, totalIdsFound); return RunOutcome.SUCCESS; } From 6d34ccf7e699528a3c8fce71520ba3f178cdde26 Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Wed, 2 Aug 2023 14:09:13 -0400 Subject: [PATCH 13/17] Remove use of pageSize parameter and see if this breaks the pipeline. --- .../main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java | 4 +++- .../ca/uhn/fhir/jpa/mdm/svc/GoldenResourceSearchSvcImpl.java | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java index 0c67a11a19cd..932caf44878f 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java @@ -159,7 +159,9 @@ private List fetchResourceIdsPageWithUrl( @Nonnull private IResourcePidList fetchResourceIdsPageNoUrl( Date theStart, Date theEnd, int thePagesize, RequestPartitionId theRequestPartitionId) { - Pageable page = Pageable.ofSize(thePagesize); +// Pageable page = Pageable.ofSize(thePagesize); + // TODO: test this + Pageable page = Pageable.unpaged(); Slice slice; if (theRequestPartitionId == null || theRequestPartitionId.isAllPartitions()) { slice = myResourceTableDao.findIdsTypesAndUpdateTimesOfResourcesWithinUpdatedRangeOrderedFromOldest( diff --git a/hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/svc/GoldenResourceSearchSvcImpl.java b/hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/svc/GoldenResourceSearchSvcImpl.java index 51712154b23c..814452b668b9 100644 --- a/hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/svc/GoldenResourceSearchSvcImpl.java +++ b/hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/svc/GoldenResourceSearchSvcImpl.java @@ -82,7 +82,8 @@ private IResourcePidList fetchResourceIdsPageWithResourceType( DateRangeParam chunkDateRange = DateRangeUtil.narrowDateRange(searchParamMap.getLastUpdated(), theStart, theEnd); searchParamMap.setLastUpdated(chunkDateRange); - searchParamMap.setCount(thePageSize); // request this many pids + // TODO: test this +// searchParamMap.setCount(thePageSize); // request this many pids searchParamMap.add( "_tag", new TokenParam(MdmConstants.SYSTEM_GOLDEN_RECORD_STATUS, MdmConstants.CODE_GOLDEN_RECORD)); From f5438f3238aa361ca3c611255c948ee5822acf69 Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Wed, 2 Aug 2023 14:10:11 -0400 Subject: [PATCH 14/17] Remove use of pageSize parameter and see if this breaks the pipeline. --- .../src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java | 2 -- .../ca/uhn/fhir/jpa/mdm/svc/GoldenResourceSearchSvcImpl.java | 2 -- 2 files changed, 4 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java index 932caf44878f..2c1c68f488a6 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java @@ -159,8 +159,6 @@ private List fetchResourceIdsPageWithUrl( @Nonnull private IResourcePidList fetchResourceIdsPageNoUrl( Date theStart, Date theEnd, int thePagesize, RequestPartitionId theRequestPartitionId) { -// Pageable page = Pageable.ofSize(thePagesize); - // TODO: test this Pageable page = Pageable.unpaged(); Slice slice; if (theRequestPartitionId == null || theRequestPartitionId.isAllPartitions()) { diff --git a/hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/svc/GoldenResourceSearchSvcImpl.java b/hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/svc/GoldenResourceSearchSvcImpl.java index 814452b668b9..794e19946370 100644 --- a/hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/svc/GoldenResourceSearchSvcImpl.java +++ b/hapi-fhir-jpaserver-mdm/src/main/java/ca/uhn/fhir/jpa/mdm/svc/GoldenResourceSearchSvcImpl.java @@ -82,8 +82,6 @@ private IResourcePidList fetchResourceIdsPageWithResourceType( DateRangeParam chunkDateRange = DateRangeUtil.narrowDateRange(searchParamMap.getLastUpdated(), theStart, theEnd); searchParamMap.setLastUpdated(chunkDateRange); - // TODO: test this -// searchParamMap.setCount(thePageSize); // request this many pids searchParamMap.add( "_tag", new TokenParam(MdmConstants.SYSTEM_GOLDEN_RECORD_STATUS, MdmConstants.CODE_GOLDEN_RECORD)); From d0ed91f576b1ce20b79de547879938e968fce900 Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Wed, 2 Aug 2023 14:47:52 -0400 Subject: [PATCH 15/17] Fix the noUrl case by passing an unlimited Pegeable instead. Effectively stop using page size for most databases. --- .../fhir/jpa/reindex/Batch2DaoSvcImpl.java | 9 ++++-- .../jpa/reindex/Batch2DaoSvcImplTest.java | 28 +++++++++++++++---- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java index 2c1c68f488a6..4670cdebe410 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java @@ -91,6 +91,9 @@ public Batch2DaoSvcImpl( public IResourcePidList fetchResourceIdsPage( Date theStart, Date theEnd, + // TODO: LD: We must keep thePageSize parameter here for the time being even if though it's no longer used + // in this class because + // otherwise we'll break other implementors of IBatch2DaoSvc @Nonnull Integer thePageSize, @Nullable RequestPartitionId theRequestPartitionId, @Nullable String theUrl) { @@ -99,7 +102,7 @@ public IResourcePidList fetchResourceIdsPage( .withRequestPartitionId(theRequestPartitionId) .execute(() -> { if (theUrl == null) { - return fetchResourceIdsPageNoUrl(theStart, theEnd, thePageSize, theRequestPartitionId); + return fetchResourceIdsPageNoUrl(theStart, theEnd, theRequestPartitionId); } else { return fetchResourceIdsPageWithUrl(theEnd, theUrl, theRequestPartitionId); } @@ -158,8 +161,8 @@ private List fetchResourceIdsPageWithUrl( @Nonnull private IResourcePidList fetchResourceIdsPageNoUrl( - Date theStart, Date theEnd, int thePagesize, RequestPartitionId theRequestPartitionId) { - Pageable page = Pageable.unpaged(); + Date theStart, Date theEnd, RequestPartitionId theRequestPartitionId) { + final Pageable page = Pageable.unpaged(); Slice slice; if (theRequestPartitionId == null || theRequestPartitionId.isAllPartitions()) { slice = myResourceTableDao.findIdsTypesAndUpdateTimesOfResourcesWithinUpdatedRangeOrderedFromOldest( diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java index 41f52a7db8aa..d4f5dbf07d2d 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImplTest.java @@ -6,7 +6,6 @@ import ca.uhn.fhir.jpa.api.pid.IResourcePidList; import ca.uhn.fhir.jpa.api.svc.IBatch2DaoSvc; import ca.uhn.fhir.jpa.dao.tx.IHapiTransactionService; -import ca.uhn.fhir.jpa.dao.tx.NonTransactionalHapiTransactionService; import ca.uhn.fhir.jpa.searchparam.MatchUrlService; import ca.uhn.fhir.jpa.test.BaseJpaR4Test; import ca.uhn.fhir.model.primitive.IdDt; @@ -26,7 +25,8 @@ import java.util.List; import java.util.stream.IntStream; -import static org.junit.jupiter.api.Assertions.*; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -39,12 +39,12 @@ class Batch2DaoSvcImplTest extends BaseJpaR4Test { private static final String PATIENT = "Patient"; private static final int INTERNAL_SYNCHRONOUS_SEARCH_SIZE = 10; - private final IHapiTransactionService myTransactionService = new NonTransactionalHapiTransactionService(); - @Autowired private JpaStorageSettings myJpaStorageSettings; @Autowired private MatchUrlService myMatchUrlService; + @Autowired + private IHapiTransactionService myIHapiTransactionService ; private DaoRegistry mySpiedDaoRegistry; @@ -56,7 +56,7 @@ void beforeEach() { mySpiedDaoRegistry = spy(myDaoRegistry); - mySubject = new Batch2DaoSvcImpl(myResourceTableDao, myMatchUrlService, mySpiedDaoRegistry, myFhirContext, myTransactionService, myJpaStorageSettings); + mySubject = new Batch2DaoSvcImpl(myResourceTableDao, myMatchUrlService, mySpiedDaoRegistry, myFhirContext, myIHapiTransactionService, myJpaStorageSettings); } // TODO: LD this test won't work with the nonUrl variant yet: error: No existing transaction found for transaction marked with propagation 'mandatory' @@ -101,6 +101,24 @@ void fetchResourcesByUrl(int expectedNumResults) { verify(mySpiedDaoRegistry, times(getExpectedNumOfInvocations(expectedNumResults))).getResourceDao(PATIENT); } + @ParameterizedTest + @ValueSource(ints = {0, 9, 10, 11, 21, 22, 23, 45}) + void fetchResourcesNoUrl(int expectedNumResults) { + final int pageSizeWellBelowThreshold = 2; + final List patientIds = IntStream.range(0, expectedNumResults) + .mapToObj(num -> createPatient()) + .toList(); + + final IResourcePidList resourcePidList = mySubject.fetchResourceIdsPage(PREVIOUS_MILLENNIUM, TOMORROW, pageSizeWellBelowThreshold, RequestPartitionId.defaultPartition(), null); + + final List actualPatientIds = + resourcePidList.getTypedResourcePids() + .stream() + .map(typePid -> new IdDt(typePid.resourceType, (Long) typePid.id.getId())) + .toList(); + assertIdsEqual(patientIds, actualPatientIds); + } + private int getExpectedNumOfInvocations(int expectedNumResults) { final int maxResultsPerQuery = INTERNAL_SYNCHRONOUS_SEARCH_SIZE + 1; final int division = expectedNumResults / maxResultsPerQuery; From daf419f6fe39852719cd297ee199d933e945fc32 Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Wed, 2 Aug 2023 15:23:40 -0400 Subject: [PATCH 16/17] Deprecate the old method and have it call the new one by default. --- .../fhir/jpa/reindex/Batch2DaoSvcImpl.java | 9 +------ .../uhn/fhir/jpa/api/svc/IBatch2DaoSvc.java | 25 ++++++++++++++++--- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java index 4670cdebe410..1a8e3e638831 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/reindex/Batch2DaoSvcImpl.java @@ -89,14 +89,7 @@ public Batch2DaoSvcImpl( @Override public IResourcePidList fetchResourceIdsPage( - Date theStart, - Date theEnd, - // TODO: LD: We must keep thePageSize parameter here for the time being even if though it's no longer used - // in this class because - // otherwise we'll break other implementors of IBatch2DaoSvc - @Nonnull Integer thePageSize, - @Nullable RequestPartitionId theRequestPartitionId, - @Nullable String theUrl) { + Date theStart, Date theEnd, @Nullable RequestPartitionId theRequestPartitionId, @Nullable String theUrl) { return myTransactionService .withSystemRequest() .withRequestPartitionId(theRequestPartitionId) diff --git a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/api/svc/IBatch2DaoSvc.java b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/api/svc/IBatch2DaoSvc.java index 07d366fd2d1b..8a10a6e35b89 100644 --- a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/api/svc/IBatch2DaoSvc.java +++ b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/api/svc/IBatch2DaoSvc.java @@ -34,18 +34,37 @@ public interface IBatch2DaoSvc { boolean isAllResourceTypeSupported(); /** + * Fetches a page of resource IDs for all resource types. The page size is up to the discretion of the implementation. + * + * @param theStart The start of the date range, must be inclusive. + * @param theEnd The end of the date range, should be exclusive. + * @param theRequestPartitionId The request partition ID (may be null on non-partitioned systems) + * @param theUrl The search URL, or null to return IDs for all resources across all resource types. Null will only be supplied if {@link #isAllResourceTypeSupported()} returns true. + */ + default IResourcePidList fetchResourceIdsPage( + Date theStart, Date theEnd, @Nullable RequestPartitionId theRequestPartitionId, @Nullable String theUrl) { + throw new UnsupportedOperationException("Not implemented unless explicitly overridden"); + } + + // TODO: LD: eliminate this call in all other implementors + /** + * @deprecated Please call (@link {@link #fetchResourceIdsPage(Date, Date, RequestPartitionId, String)} instead. + *

* Fetches a page of resource IDs for all resource types. The page size is up to the discretion of the implementation. * * @param theStart The start of the date range, must be inclusive. * @param theEnd The end of the date range, should be exclusive. * @param thePageSize The number of records to query in each pass. - * @param theRequestPartitionId The request partition ID (may be null on nonpartitioned systems) + * @param theRequestPartitionId The request partition ID (may be null on non-partitioned systems) * @param theUrl The search URL, or null to return IDs for all resources across all resource types. Null will only be supplied if {@link #isAllResourceTypeSupported()} returns true. */ - IResourcePidList fetchResourceIdsPage( + @Deprecated + default IResourcePidList fetchResourceIdsPage( Date theStart, Date theEnd, @Nonnull Integer thePageSize, @Nullable RequestPartitionId theRequestPartitionId, - @Nullable String theUrl); + @Nullable String theUrl) { + return fetchResourceIdsPage(theStart, theEnd, theRequestPartitionId, theUrl); + } } From 610b4dfdcdf305ebc9d14799c2ad7a983ed1054a Mon Sep 17 00:00:00 2001 From: Luke deGruchy Date: Wed, 2 Aug 2023 16:33:43 -0400 Subject: [PATCH 17/17] Add Msg code to Exception. --- .../src/main/java/ca/uhn/fhir/jpa/api/svc/IBatch2DaoSvc.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/api/svc/IBatch2DaoSvc.java b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/api/svc/IBatch2DaoSvc.java index 8a10a6e35b89..d6759d53bde1 100644 --- a/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/api/svc/IBatch2DaoSvc.java +++ b/hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/api/svc/IBatch2DaoSvc.java @@ -19,6 +19,7 @@ */ package ca.uhn.fhir.jpa.api.svc; +import ca.uhn.fhir.i18n.Msg; import ca.uhn.fhir.interceptor.model.RequestPartitionId; import ca.uhn.fhir.jpa.api.pid.IResourcePidList; @@ -43,7 +44,7 @@ public interface IBatch2DaoSvc { */ default IResourcePidList fetchResourceIdsPage( Date theStart, Date theEnd, @Nullable RequestPartitionId theRequestPartitionId, @Nullable String theUrl) { - throw new UnsupportedOperationException("Not implemented unless explicitly overridden"); + throw new UnsupportedOperationException(Msg.code(2425) + "Not implemented unless explicitly overridden"); } // TODO: LD: eliminate this call in all other implementors