From 51a69f0dc9e64c13d26f489f09b97540f5e29d11 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Sat, 13 Oct 2018 12:02:19 -0400 Subject: [PATCH] Fixes to migrator and count logic --- .../ca/uhn/fhir/jpa/config/BaseConfig.java | 25 +- .../java/ca/uhn/fhir/jpa/entity/Search.java | 7 + .../fhir/jpa/entity/SearchParamPresent.java | 5 +- .../search/PersistedJpaBundleProvider.java | 45 ++- .../jpa/search/SearchCoordinatorSvcImpl.java | 109 +++--- .../ca/uhn/fhir/jpa/config/TestR4Config.java | 6 +- .../FhirResourceDaoR4SearchOptimizedTest.java | 61 ++- .../fhir/rest/server/RestfulServerUtils.java | 8 + .../server/method/SummaryEnumParameter.java | 18 +- ...amTest.java => SummaryParamDstu2Test.java} | 4 +- .../fhir/rest/server/SummaryParamR4Test.java | 362 ++++++++++++++++++ 11 files changed, 560 insertions(+), 90 deletions(-) rename hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/server/{SummaryParamTest.java => SummaryParamDstu2Test.java} (99%) create mode 100644 hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/SummaryParamR4Test.java diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/BaseConfig.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/BaseConfig.java index 706f4d1bdd9..e94be10617a 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/BaseConfig.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/config/BaseConfig.java @@ -9,9 +9,9 @@ package ca.uhn.fhir.jpa.config; * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -32,7 +32,10 @@ import ca.uhn.fhir.jpa.subscription.resthook.SubscriptionRestHookInterceptor; import ca.uhn.fhir.jpa.subscription.websocket.SubscriptionWebsocketInterceptor; import ca.uhn.fhir.jpa.util.IReindexController; import ca.uhn.fhir.jpa.util.ReindexController; +import org.hibernate.cfg.AvailableSettings; import org.hibernate.jpa.HibernatePersistenceProvider; +import org.hibernate.query.criteria.LiteralHandlingMode; +import org.hibernate.resource.jdbc.spi.PhysicalConnectionHandlingMode; import org.springframework.beans.factory.annotation.Autowire; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Bean; @@ -52,6 +55,7 @@ import org.springframework.scheduling.concurrent.ScheduledExecutorFactoryBean; import org.springframework.scheduling.config.ScheduledTaskRegistrar; import javax.annotation.Nonnull; +import java.util.Map; import java.util.concurrent.ScheduledExecutorService; @Configuration @@ -85,7 +89,22 @@ public abstract class BaseConfig implements SchedulingConfigurer { * factory with HAPI FHIR customizations */ protected LocalContainerEntityManagerFactoryBean entityManagerFactory() { - LocalContainerEntityManagerFactoryBean retVal = new LocalContainerEntityManagerFactoryBean(); + LocalContainerEntityManagerFactoryBean retVal = new LocalContainerEntityManagerFactoryBean() { + @Override + public Map getJpaPropertyMap() { + Map retVal = super.getJpaPropertyMap(); + + if (!retVal.containsKey(AvailableSettings.CRITERIA_LITERAL_HANDLING_MODE)) { + retVal.put(AvailableSettings.CRITERIA_LITERAL_HANDLING_MODE, LiteralHandlingMode.BIND); + } + + if (!retVal.containsKey(AvailableSettings.CONNECTION_HANDLING)) { + retVal.put(AvailableSettings.CONNECTION_HANDLING, PhysicalConnectionHandlingMode.DELAYED_ACQUISITION_AND_HOLD); + } + + return retVal; + } + }; configureEntityManagerFactory(retVal, fhirContext()); return retVal; } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/Search.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/Search.java index ef8846df1ab..35722280124 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/Search.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/Search.java @@ -150,6 +150,13 @@ public class Search implements Serializable { myFailureMessage = left(theFailureMessage, FAILURE_MESSAGE_LENGTH); } + // FIXME: remove this + private static final Logger ourLog = LoggerFactory.getLogger(Search.class); + @PrePersist + public void prePersist() { + ourLog.info("*** SAVING WITH VERSION {} TOTAL {}", myVersion, myTotalCount); + } + public Long getId() { return myId; } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/SearchParamPresent.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/SearchParamPresent.java index 7715dd3eff1..02dbfde2433 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/SearchParamPresent.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/SearchParamPresent.java @@ -111,8 +111,9 @@ public class SearchParamPresent implements Serializable { return b.build(); } - public static long calculateHashPresence(String theResourceType, String theParamName, boolean thePresent) { - return BaseResourceIndexedSearchParam.hash(theResourceType, theParamName, Boolean.toString(thePresent)); + public static long calculateHashPresence(String theResourceType, String theParamName, Boolean thePresent) { + String string = thePresent != null ? Boolean.toString(thePresent) : Boolean.toString(false); + return BaseResourceIndexedSearchParam.hash(theResourceType, theParamName, string); } } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProvider.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProvider.java index 20d24605f9d..1d9f51f4818 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProvider.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/PersistedJpaBundleProvider.java @@ -9,9 +9,9 @@ package ca.uhn.fhir.jpa.search; * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -24,10 +24,15 @@ import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.jpa.dao.IDao; import ca.uhn.fhir.jpa.dao.ISearchBuilder; import ca.uhn.fhir.jpa.dao.data.ISearchDao; -import ca.uhn.fhir.jpa.entity.*; +import ca.uhn.fhir.jpa.entity.BaseHasResource; +import ca.uhn.fhir.jpa.entity.ResourceHistoryTable; +import ca.uhn.fhir.jpa.entity.Search; +import ca.uhn.fhir.jpa.entity.SearchTypeEnum; import ca.uhn.fhir.model.primitive.InstantDt; import ca.uhn.fhir.rest.api.server.IBundleProvider; import org.hl7.fhir.instance.model.api.IBaseResource; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.transaction.PlatformTransactionManager; import org.springframework.transaction.TransactionDefinition; import org.springframework.transaction.TransactionStatus; @@ -46,6 +51,7 @@ import java.util.*; public class PersistedJpaBundleProvider implements IBundleProvider { + private static final Logger ourLog = LoggerFactory.getLogger(PersistedJpaBundleProvider.class); private FhirContext myContext; private IDao myDao; private EntityManager myEntityManager; @@ -149,25 +155,26 @@ public class PersistedJpaBundleProvider implements IBundleProvider { if (mySearchEntity == null) { ensureDependenciesInjected(); - TransactionTemplate template = new TransactionTemplate(myPlatformTransactionManager); - template.setPropagationBehavior(TransactionTemplate.PROPAGATION_REQUIRED); - return template.execute(new TransactionCallback() { - @Override - public Boolean doInTransaction(TransactionStatus theStatus) { - try { - setSearchEntity(mySearchDao.findByUuid(myUuid)); + TransactionTemplate txTemplate = new TransactionTemplate(myPlatformTransactionManager); + txTemplate.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRED); + txTemplate.setIsolationLevel(TransactionDefinition.ISOLATION_READ_UNCOMMITTED); + return txTemplate.execute(s -> { + try { + setSearchEntity(mySearchDao.findByUuid(myUuid)); - if (mySearchEntity == null) { - return false; - } - - // Load the includes now so that they are available outside of this transaction - mySearchEntity.getIncludes().size(); - - return true; - } catch (NoResultException e) { + if (mySearchEntity == null) { return false; } + + // FIXME: remove + ourLog.info("** Retrieved search with version {} and total {}", mySearchEntity.getVersion(), mySearchEntity.getTotalCount()); + + // Load the includes now so that they are available outside of this transaction + mySearchEntity.getIncludes().size(); + + return true; + } catch (NoResultException e) { + return false; } }); } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java index 7f9fa948bfb..ecfb1b305ac 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java @@ -9,9 +9,9 @@ package ca.uhn.fhir.jpa.search; * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -56,10 +56,13 @@ import org.springframework.transaction.TransactionStatus; import org.springframework.transaction.annotation.Propagation; import org.springframework.transaction.annotation.Transactional; import org.springframework.transaction.support.TransactionCallbackWithoutResult; +import org.springframework.transaction.support.TransactionSynchronization; +import org.springframework.transaction.support.TransactionSynchronizationManager; import org.springframework.transaction.support.TransactionTemplate; import javax.annotation.Nullable; import javax.persistence.EntityManager; +import javax.transaction.TransactionManager; import java.util.*; import java.util.concurrent.*; @@ -421,41 +424,39 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { } public abstract class BaseTask implements Callable { - protected Search getSearch() { - return getSearch; - } - - private final Search getSearch; - private final SearchParameterMap myParams; - private final IDao myCallingDao; - private final String myResourceType; - private final ArrayList mySyncedPids = new ArrayList<>(); - - protected CountDownLatch getInitialCollectionLatch() { - return myInitialCollectionLatch; - } - + private final SearchParameterMap myParams; + private final IDao myCallingDao; + private final String myResourceType; + private final ArrayList mySyncedPids = new ArrayList<>(); private final CountDownLatch myInitialCollectionLatch = new CountDownLatch(1); private final CountDownLatch myCompletionLatch; private final ArrayList myUnsyncedPids = new ArrayList<>(); + private Search mySearch; private boolean myAbortRequested; private int myCountSaved = 0; private boolean myAdditionalPrefetchThresholdsRemaining; private List myPreviouslyAddedResourcePids; private Integer myMaxResultsToFetch; private int myCountFetchedDuringThisPass; - /** * Constructor */ protected BaseTask(Search theSearch, IDao theCallingDao, SearchParameterMap theParams, String theResourceType) { - getSearch = theSearch; + mySearch = theSearch; myCallingDao = theCallingDao; myParams = theParams; myResourceType = theResourceType; myCompletionLatch = new CountDownLatch(1); } + protected Search getSearch() { + return mySearch; + } + + protected CountDownLatch getInitialCollectionLatch() { + return myInitialCollectionLatch; + } + protected void setPreviouslyAddedResourcePids(List thePreviouslyAddedResourcePids) { myPreviouslyAddedResourcePids = thePreviouslyAddedResourcePids; myCountSaved = myPreviouslyAddedResourcePids.size(); @@ -475,8 +476,8 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { boolean keepWaiting; do { synchronized (mySyncedPids) { - ourLog.trace("Search status is {}", getSearch.getStatus()); - keepWaiting = mySyncedPids.size() < theToIndex && getSearch.getStatus() == SearchStatusEnum.LOADING; + ourLog.trace("Search status is {}", mySearch.getStatus()); + keepWaiting = mySyncedPids.size() < theToIndex && mySearch.getStatus() == SearchStatusEnum.LOADING; } if (keepWaiting) { ourLog.info("Waiting, as we only have {} results", mySyncedPids.size()); @@ -492,7 +493,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { ArrayList retVal = new ArrayList<>(); synchronized (mySyncedPids) { - verifySearchHasntFailedOrThrowInternalErrorException(getSearch); + verifySearchHasntFailedOrThrowInternalErrorException(mySearch); int toIndex = theToIndex; if (mySyncedPids.size() < toIndex) { @@ -526,13 +527,13 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { txTemplate.execute(new TransactionCallbackWithoutResult() { @Override protected void doInTransactionWithoutResult(TransactionStatus theArg0) { - if (getSearch.getId() == null) { + if (mySearch.getId() == null) { doSaveSearch(); } List resultsToSave = Lists.newArrayList(); for (Long nextPid : myUnsyncedPids) { - SearchResult nextResult = new SearchResult(getSearch); + SearchResult nextResult = new SearchResult(mySearch); nextResult.setResourcePid(nextPid); nextResult.setOrder(myCountSaved++); resultsToSave.add(nextResult); @@ -547,22 +548,22 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { myUnsyncedPids.clear(); if (theResultIter.hasNext() == false) { - getSearch.setNumFound(myCountSaved); + mySearch.setNumFound(myCountSaved); if (myMaxResultsToFetch != null && myCountSaved < myMaxResultsToFetch) { - getSearch.setStatus(SearchStatusEnum.FINISHED); - getSearch.setTotalCount(myCountSaved); + mySearch.setStatus(SearchStatusEnum.FINISHED); + mySearch.setTotalCount(myCountSaved); } else if (myAdditionalPrefetchThresholdsRemaining) { ourLog.trace("Setting search status to PASSCMPLET"); - getSearch.setStatus(SearchStatusEnum.PASSCMPLET); - getSearch.setSearchParameterMap(myParams); + mySearch.setStatus(SearchStatusEnum.PASSCMPLET); + mySearch.setSearchParameterMap(myParams); } else { - getSearch.setStatus(SearchStatusEnum.FINISHED); - getSearch.setTotalCount(myCountSaved); + mySearch.setStatus(SearchStatusEnum.FINISHED); + mySearch.setTotalCount(myCountSaved); } } } - getSearch.setNumFound(myCountSaved); + mySearch.setNumFound(myCountSaved); int numSynced; synchronized (mySyncedPids) { @@ -615,6 +616,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { TransactionTemplate txTemplate = new TransactionTemplate(myManagedTxManager); txTemplate.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW); + txTemplate.setIsolationLevel(TransactionDefinition.ISOLATION_READ_UNCOMMITTED); txTemplate.execute(new TransactionCallbackWithoutResult() { @Override protected void doInTransactionWithoutResult(TransactionStatus theStatus) { @@ -622,7 +624,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { } }); - ourLog.info("Completed search for [{}] and found {} resources in {}ms", getSearch.getSearchQueryString(), mySyncedPids.size(), sw.getMillis()); + ourLog.info("Completed search for [{}{}] and found {} resources in {}ms", mySearch.getResourceType(), mySearch.getSearchQueryString(), mySyncedPids.size(), sw.getMillis()); } catch (Throwable t) { @@ -655,15 +657,15 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { failureCode = ((BaseServerResponseException) t).getStatusCode(); } - getSearch.setFailureMessage(failureMessage); - getSearch.setFailureCode(failureCode); - getSearch.setStatus(SearchStatusEnum.FAILED); + mySearch.setFailureMessage(failureMessage); + mySearch.setFailureCode(failureCode); + mySearch.setStatus(SearchStatusEnum.FAILED); saveSearch(); } finally { - myIdToSearchTask.remove(getSearch.getUuid()); + myIdToSearchTask.remove(mySearch.getUuid()); myInitialCollectionLatch.countDown(); markComplete(); @@ -672,14 +674,26 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { } private void doSaveSearch() { - if (getSearch.getId() == null) { - mySearchDao.save(getSearch); - for (SearchInclude next : getSearch.getIncludes()) { + + // FIXME: remove + Integer totalCount = mySearch.getTotalCount(); + TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() { + @Override + public void afterCommit() { + ourLog.info("Have flushed save with total {}", totalCount); + } + }); + ourLog.info("** Saving search version {} count {}", mySearch.getVersion(), totalCount); + + if (mySearch.getId() == null) { + mySearch = mySearchDao.saveAndFlush(mySearch); + for (SearchInclude next : mySearch.getIncludes()) { mySearchIncludeDao.save(next); } } else { - mySearchDao.save(getSearch); + mySearch = mySearchDao.saveAndFlush(mySearch); } + } /** @@ -698,18 +712,20 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { boolean wantCount = myParams.getSummaryMode().contains(SummaryEnum.COUNT); boolean wantOnlyCount = wantCount && myParams.getSummaryMode().size() == 1; if (wantCount) { + ourLog.info("** performing count"); ISearchBuilder sb = newSearchBuilder(); - Iterator countIterator = sb.createCountQuery(myParams, getSearch.getUuid()); + Iterator countIterator = sb.createCountQuery(myParams, mySearch.getUuid()); Long count = countIterator.next(); + ourLog.info("** got count {}", count); TransactionTemplate txTemplate = new TransactionTemplate(myManagedTxManager); - txTemplate.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRED); +// txTemplate.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW); txTemplate.execute(new TransactionCallbackWithoutResult() { @Override protected void doInTransactionWithoutResult(TransactionStatus theArg0) { - getSearch.setTotalCount(count.intValue()); + mySearch.setTotalCount(count.intValue()); if (wantOnlyCount) { - getSearch.setStatus(SearchStatusEnum.FINISHED); + mySearch.setStatus(SearchStatusEnum.FINISHED); } doSaveSearch(); } @@ -719,6 +735,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { } } + ourLog.info("** done count"); ISearchBuilder sb = newSearchBuilder(); /* @@ -727,7 +744,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { * "pre-fetch thresholds" specified in DaoConfig#getPreFetchThresholds() * as well as the value of the _count parameter. */ - int currentlyLoaded = defaultIfNull(getSearch.getNumFound(), 0); + int currentlyLoaded = defaultIfNull(mySearch.getNumFound(), 0); int minWanted = 0; if (myParams.getCount() != null) { minWanted = myParams.getCount(); @@ -774,7 +791,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { /* * Construct the SQL query we'll be sending to the database */ - Iterator theResultIterator = sb.createQuery(myParams, getSearch.getUuid()); + Iterator theResultIterator = sb.createQuery(myParams, mySearch.getUuid()); /* * The following loop actually loads the PIDs of the resources diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestR4Config.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestR4Config.java index 943fc6b0451..648c56d92a6 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestR4Config.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestR4Config.java @@ -4,10 +4,11 @@ import ca.uhn.fhir.jpa.dao.DaoConfig; import ca.uhn.fhir.rest.server.interceptor.RequestValidatingInterceptor; import ca.uhn.fhir.validation.ResultSeverityEnum; import net.ttddyy.dsproxy.listener.ThreadQueryCountHolder; -import net.ttddyy.dsproxy.listener.logging.SLF4JLogLevel; import net.ttddyy.dsproxy.support.ProxyDataSourceBuilder; import org.apache.commons.dbcp2.BasicDataSource; +import org.hibernate.cfg.AvailableSettings; import org.hibernate.query.criteria.LiteralHandlingMode; +import org.hibernate.resource.jdbc.spi.PhysicalConnectionHandlingMode; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Lazy; @@ -23,7 +24,7 @@ import java.sql.SQLException; import java.util.Properties; import java.util.concurrent.TimeUnit; -import static org.junit.Assert.*; +import static org.junit.Assert.fail; @Configuration @EnableTransactionManagement() @@ -134,7 +135,6 @@ public class TestR4Config extends BaseJavaConfigR4 { extraProperties.put("hibernate.search.default.directory_provider", "ram"); extraProperties.put("hibernate.search.lucene_version", "LUCENE_CURRENT"); extraProperties.put("hibernate.search.autoregister_listeners", "true"); - extraProperties.put("hibernate.criteria.literal_handling_mode", LiteralHandlingMode.BIND); return extraProperties; } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchOptimizedTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchOptimizedTest.java index e5a30332e5a..eb75c0fecd5 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchOptimizedTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchOptimizedTest.java @@ -3,6 +3,7 @@ package ca.uhn.fhir.jpa.dao.r4; import ca.uhn.fhir.jpa.dao.SearchParameterMap; import ca.uhn.fhir.jpa.entity.Search; import ca.uhn.fhir.jpa.entity.SearchStatusEnum; +import ca.uhn.fhir.jpa.search.SearchCoordinatorSvcImpl; import ca.uhn.fhir.rest.api.SortSpec; import ca.uhn.fhir.rest.api.SummaryEnum; import ca.uhn.fhir.rest.api.server.IBundleProvider; @@ -15,6 +16,7 @@ import org.junit.After; import org.junit.AfterClass; import org.junit.Before; import org.junit.Test; +import org.springframework.aop.framework.AopProxyUtils; import org.springframework.scheduling.concurrent.ThreadPoolExecutorFactoryBean; import java.util.ArrayList; @@ -26,17 +28,23 @@ import java.util.concurrent.Future; import static org.apache.commons.lang3.StringUtils.leftPad; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.hasSize; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertThat; +import static org.junit.Assert.*; @SuppressWarnings({"unchecked", "deprecation", "Duplicates"}) public class FhirResourceDaoR4SearchOptimizedTest extends BaseJpaR4Test { private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(FhirResourceDaoR4SearchOptimizedTest.class); + private SearchCoordinatorSvcImpl mySearchCoordinatorSvcImpl; + + @Before + public void before() { + mySearchCoordinatorSvcImpl = (SearchCoordinatorSvcImpl) AopProxyUtils.getSingletonTarget(mySearchCoordinatorSvc); + } @After public final void after() { + mySearchCoordinatorSvcImpl.setLoadingThrottleForUnitTests(null); + mySearchCoordinatorSvcImpl.setSyncSizeForUnitTests(SearchCoordinatorSvcImpl.DEFAULT_SYNC_SIZE); } @Before @@ -119,6 +127,30 @@ public class FhirResourceDaoR4SearchOptimizedTest extends BaseJpaR4Test { } + @Test + public void testFetchCountAndDataForSlowLoading() { + mySearchCoordinatorSvcImpl.setLoadingThrottleForUnitTests(25); + mySearchCoordinatorSvcImpl.setSyncSizeForUnitTests(10); + + myDaoConfig.setSearchPreFetchThresholds(Arrays.asList(1000, -1)); + + SearchParameterMap params = new SearchParameterMap(); + params.setSort(new SortSpec(Patient.SP_NAME)); + params.setCount(5); + params.setSummaryMode(Sets.newHashSet(SummaryEnum.COUNT, SummaryEnum.DATA)); + IBundleProvider results = myPatientDao.search(params); + String uuid = results.getUuid(); + ourLog.info("** Search returned UUID: {}", uuid); + +// assertEquals(200, myDatabaseBackedPagingProvider.retrieveResultList(uuid).size().intValue()); + assertEquals(200, results.size().intValue()); + List ids = toUnqualifiedVersionlessIdValues(results, 0, 5, true); + assertEquals("Patient/PT00000", ids.get(0)); + assertEquals("Patient/PT00004", ids.get(4)); + + ourLog.info("** About to make new query for search with UUID: {}", uuid); + assertEquals(200, myDatabaseBackedPagingProvider.retrieveResultList(uuid).size().intValue()); + } @Test public void testFetchCountAndData() { @@ -135,6 +167,21 @@ public class FhirResourceDaoR4SearchOptimizedTest extends BaseJpaR4Test { assertEquals("Patient/PT00000", ids.get(0)); assertEquals("Patient/PT00009", ids.get(9)); assertEquals(200, myDatabaseBackedPagingProvider.retrieveResultList(uuid).size().intValue()); + + // Try the same query again. This time the same thing should come back, but + // from the cache... + + params = new SearchParameterMap(); + params.setSort(new SortSpec(Patient.SP_NAME)); + params.setSummaryMode(Sets.newHashSet(SummaryEnum.COUNT, SummaryEnum.DATA)); + results = myPatientDao.search(params); + uuid = results.getUuid(); + assertEquals(200, results.size().intValue()); + ids = toUnqualifiedVersionlessIdValues(results, 0, 10, true); + assertEquals("Patient/PT00000", ids.get(0)); + assertEquals("Patient/PT00009", ids.get(9)); + assertEquals(200, myDatabaseBackedPagingProvider.retrieveResultList(uuid).size().intValue()); + } @Test @@ -190,7 +237,7 @@ public class FhirResourceDaoR4SearchOptimizedTest extends BaseJpaR4Test { } - @Test + @Test public void testFetchOnlySmallBatches() { myDaoConfig.setSearchPreFetchThresholds(Arrays.asList(20, 50, 190)); @@ -348,7 +395,7 @@ public class FhirResourceDaoR4SearchOptimizedTest extends BaseJpaR4Test { } - @Test + @Test public void testFetchUnlimited() { myDaoConfig.setSearchPreFetchThresholds(Arrays.asList(20, -1)); @@ -405,7 +452,7 @@ public class FhirResourceDaoR4SearchOptimizedTest extends BaseJpaR4Test { } - @Test + @Test public void testFetchSecondBatchInManyThreads() throws Throwable { myDaoConfig.setSearchPreFetchThresholds(Arrays.asList(20, -1)); @@ -500,7 +547,7 @@ public class FhirResourceDaoR4SearchOptimizedTest extends BaseJpaR4Test { } - @AfterClass + @AfterClass public static void afterClassClearContext() { TestUtil.clearAllStaticFieldsForUnitTest(); } diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/RestfulServerUtils.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/RestfulServerUtils.java index e8169ac924f..5203733b932 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/RestfulServerUtils.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/RestfulServerUtils.java @@ -731,7 +731,15 @@ public class RestfulServerUtils { if (theResource == null) { // No response is being returned } else if (encodingDomainResourceAsText && theResource instanceof IResource) { + // DSTU2 writer.append(((IResource) theResource).getText().getDiv().getValueAsString()); + } else if (encodingDomainResourceAsText && theResource instanceof IDomainResource) { + // DSTU3+ + try { + writer.append(((IDomainResource) theResource).getText().getDivAsString()); + } catch (Exception e) { + throw new InternalErrorException(e); + } } else { FhirVersionEnum forVersion = theResource.getStructureFhirVersionEnum(); IParser parser = getNewParser(theServer.getFhirContext(), forVersion, theRequestDetails); diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/SummaryEnumParameter.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/SummaryEnumParameter.java index 34091f2f0d2..f69518d3479 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/SummaryEnumParameter.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/SummaryEnumParameter.java @@ -69,20 +69,22 @@ public class SummaryEnumParameter implements IParameter { retVal = null; } else if (isBlank(summary[0])) { retVal = null; - } else if (summary.length == 1) { + } else if (summary.length == 1 && summary[0].indexOf(',') == -1) { retVal = toCollectionOrNull(SummaryEnum.fromCode(summary[0])); if (retVal == null) { retVal = toCollectionOrNull(SummaryEnum.fromCode(summary[0].toLowerCase())); } } else { retVal = new HashSet<>(); - for (String next : summary) { - SummaryEnum value = SummaryEnum.fromCode(next); - if (value == null) { - value = SummaryEnum.fromCode(next.toLowerCase()); - } - if (value != null) { - retVal.add(value); + for (String nextParamValue : summary) { + for (String nextParamValueTok : nextParamValue.split(",")) { + SummaryEnum value = SummaryEnum.fromCode(nextParamValueTok); + if (value == null) { + value = SummaryEnum.fromCode(nextParamValueTok.toLowerCase()); + } + if (value != null) { + retVal.add(value); + } } } } diff --git a/hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/server/SummaryParamTest.java b/hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/server/SummaryParamDstu2Test.java similarity index 99% rename from hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/server/SummaryParamTest.java rename to hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/server/SummaryParamDstu2Test.java index d49c9925def..ce88e6a526c 100644 --- a/hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/server/SummaryParamTest.java +++ b/hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/server/SummaryParamDstu2Test.java @@ -36,13 +36,13 @@ import ca.uhn.fhir.rest.api.SummaryEnum; import ca.uhn.fhir.util.PortUtil; import ca.uhn.fhir.util.TestUtil; -public class SummaryParamTest { +public class SummaryParamDstu2Test { private static CloseableHttpClient ourClient; private static FhirContext ourCtx = FhirContext.forDstu2(); private static SummaryEnum ourLastSummary; private static List ourLastSummaryList; - private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(SummaryParamTest.class); + private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(SummaryParamDstu2Test.class); private static int ourPort; private static Server ourServer; diff --git a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/SummaryParamR4Test.java b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/SummaryParamR4Test.java new file mode 100644 index 00000000000..e65b0537b75 --- /dev/null +++ b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/SummaryParamR4Test.java @@ -0,0 +1,362 @@ +package ca.uhn.fhir.rest.server; + +import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.rest.annotation.IdParam; +import ca.uhn.fhir.rest.annotation.Read; +import ca.uhn.fhir.rest.annotation.Search; +import ca.uhn.fhir.rest.api.Constants; +import ca.uhn.fhir.rest.api.SummaryEnum; +import ca.uhn.fhir.util.PortUtil; +import ca.uhn.fhir.util.TestUtil; +import org.apache.commons.io.IOUtils; +import org.apache.http.HttpResponse; +import org.apache.http.client.methods.HttpGet; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.impl.client.HttpClientBuilder; +import org.apache.http.impl.conn.PoolingHttpClientConnectionManager; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.servlet.ServletHandler; +import org.eclipse.jetty.servlet.ServletHolder; +import org.hl7.fhir.instance.model.api.IBaseResource; +import org.hl7.fhir.r4.model.IdType; +import org.hl7.fhir.r4.model.MedicationRequest; +import org.hl7.fhir.r4.model.Patient; +import org.hl7.fhir.r4.model.Reference; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.TimeUnit; + +import static org.hamcrest.Matchers.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; + +public class SummaryParamR4Test { + + private static CloseableHttpClient ourClient; + private static FhirContext ourCtx = FhirContext.forR4(); + private static SummaryEnum ourLastSummary; + private static List ourLastSummaryList; + private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(SummaryParamR4Test.class); + private static int ourPort; + + private static Server ourServer; + + @Before + public void before() { + ourLastSummary = null; + ourLastSummaryList = null; + } + @Test + public void testReadSummaryData() throws Exception { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/1?_summary=" + SummaryEnum.DATA.getCode()); + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent()); + IOUtils.closeQuietly(status.getEntity().getContent()); + ourLog.info(responseContent); + + assertEquals(200, status.getStatusLine().getStatusCode()); + assertEquals(Constants.CT_FHIR_XML_NEW + Constants.CHARSET_UTF8_CTSUFFIX.replace(" ", "").toLowerCase(), status.getEntity().getContentType().getValue().replace(" ", "").replace("UTF", "utf")); + assertThat(responseContent, not(containsString("THE DIV"))); + assertThat(responseContent, (containsString("family"))); + assertThat(responseContent, (containsString("maritalStatus"))); + assertEquals(SummaryEnum.DATA, ourLastSummary); + } + + + + @Test + public void testReadSummaryText() throws Exception { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/1?_summary=" + SummaryEnum.TEXT.getCode()); + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent()); + IOUtils.closeQuietly(status.getEntity().getContent()); + ourLog.info(responseContent); + + assertEquals(200, status.getStatusLine().getStatusCode()); + assertEquals(Constants.CT_HTML_WITH_UTF8.replace(" ", "").toLowerCase(), status.getEntity().getContentType().getValue().replace(" ", "").replace("UTF", "utf")); + assertThat(responseContent, not(containsString("THE DIV", responseContent); + assertThat(responseContent, not(containsString("efer"))); + assertEquals(SummaryEnum.TEXT, ourLastSummary); + } + + @Test + public void testReadSummaryTextWithMandatory() throws Exception { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/MedicationRequest/1?_summary=" + SummaryEnum.TEXT.getCode()); + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent()); + IOUtils.closeQuietly(status.getEntity().getContent()); + ourLog.info(responseContent); + + assertEquals(200, status.getStatusLine().getStatusCode()); + assertEquals(Constants.CT_HTML_WITH_UTF8.replace(" ", "").toLowerCase(), status.getEntity().getContentType().getValue().replace(" ", "").replace("UTF", "utf")); + assertThat(responseContent, not(containsString("TEXT", responseContent); + assertThat(responseContent, not(containsString("family"))); + assertThat(responseContent, not(containsString("maritalStatus"))); + } + + @Test + public void testReadSummaryTrue() throws Exception { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient/1?_summary=" + SummaryEnum.TRUE.getCode()); + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent()); + IOUtils.closeQuietly(status.getEntity().getContent()); + ourLog.info(responseContent); + + assertEquals(200, status.getStatusLine().getStatusCode()); + assertEquals(Constants.CT_FHIR_XML_NEW + Constants.CHARSET_UTF8_CTSUFFIX.replace(" ", "").toLowerCase(), status.getEntity().getContentType().getValue().replace(" ", "").replace("UTF", "utf")); + assertThat(responseContent, not(containsString("THE DIV"))); + assertThat(responseContent, (containsString("family"))); + assertThat(responseContent, not(containsString("maritalStatus"))); + assertEquals(SummaryEnum.TRUE, ourLastSummary); + } + + @Test + public void testSearchSummaryCount() throws Exception { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_pretty=true&_summary=" + SummaryEnum.COUNT.getCode()); + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent()); + IOUtils.closeQuietly(status.getEntity().getContent()); + ourLog.info(responseContent); + + assertEquals(200, status.getStatusLine().getStatusCode()); + assertThat(responseContent, (containsString(""))); + assertThat(responseContent, not(containsString("entry"))); + assertThat(responseContent, not(containsString("THE DIV"))); + assertThat(responseContent, not(containsString("family"))); + assertThat(responseContent, not(containsString("maritalStatus"))); + assertEquals(SummaryEnum.COUNT, ourLastSummary); + } + + @Test + public void testSearchSummaryCountAndData() throws Exception { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_pretty=true&_summary=" + SummaryEnum.COUNT.getCode() + "," + SummaryEnum.DATA.getCode()); + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent()); + IOUtils.closeQuietly(status.getEntity().getContent()); + ourLog.info(responseContent); + + assertEquals(200, status.getStatusLine().getStatusCode()); + assertThat(responseContent, (containsString(""))); + assertThat(responseContent, (containsString("entry"))); + assertThat(responseContent, not(containsString("THE DIV"))); + assertThat(responseContent, (containsString("family"))); + assertThat(responseContent, (containsString("maritalStatus"))); + } + + @Test + public void testSearchSummaryData() throws Exception { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_summary=" + SummaryEnum.DATA.getCode()); + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent()); + IOUtils.closeQuietly(status.getEntity().getContent()); + ourLog.info(responseContent); + + assertEquals(200, status.getStatusLine().getStatusCode()); + assertThat(responseContent, containsString(""))); + assertThat(responseContent, (containsString("entry"))); + assertThat(responseContent, (containsString("THE DIV"))); + assertThat(responseContent, not(containsString("family"))); + assertThat(responseContent, not(containsString("maritalStatus"))); + assertEquals(SummaryEnum.TEXT, ourLastSummary); + } + + @Test + public void testSearchSummaryTextWithMandatory() throws Exception { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/MedicationRequest?_summary=" + SummaryEnum.TEXT.getCode() + "&_pretty=true"); + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent()); + IOUtils.closeQuietly(status.getEntity().getContent()); + ourLog.info(responseContent); + + assertEquals(200, status.getStatusLine().getStatusCode()); + assertThat(responseContent, (containsString(""))); + assertThat(responseContent, (containsString("entry"))); + assertThat(responseContent, (containsString(">TEXT<"))); + assertThat(responseContent, (containsString("Medication/123"))); + assertThat(responseContent, not(containsStringIgnoringCase("note"))); + } + + @Test + public void testSearchSummaryTextMulti() throws Exception { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_query=multi&_summary=" + SummaryEnum.TEXT.getCode()); + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent()); + IOUtils.closeQuietly(status.getEntity().getContent()); + ourLog.info(responseContent); + + assertEquals(200, status.getStatusLine().getStatusCode()); + assertThat(responseContent, (containsString(""))); + assertThat(responseContent, (containsString("entry"))); + assertThat(responseContent, (containsString("THE DIV"))); + assertThat(responseContent, not(containsString("family"))); + assertThat(responseContent, not(containsString("maritalStatus"))); + assertThat(ourLastSummaryList, contains(SummaryEnum.TEXT)); + } + + @Test + public void testSearchSummaryTrue() throws Exception { + HttpGet httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?_summary=" + SummaryEnum.TRUE.getCode()); + HttpResponse status = ourClient.execute(httpGet); + String responseContent = IOUtils.toString(status.getEntity().getContent()); + IOUtils.closeQuietly(status.getEntity().getContent()); + ourLog.info(responseContent); + + assertEquals(200, status.getStatusLine().getStatusCode()); + assertThat(responseContent, containsString(" getResourceType() { + return MedicationRequest.class; + } + + @Read + public MedicationRequest read(@IdParam IdType theId) { + MedicationRequest retVal = new MedicationRequest(); + retVal.getText().setDivAsString("
TEXT
"); + retVal.addNote().setText("NOTE"); + retVal.setMedication(new Reference("Medication/123")); + retVal.setId(theId); + return retVal; + } + + @Search + public List read() { + return Arrays.asList(read(new IdType("999"))); + } + + } + + public static class DummyPatientResourceProvider implements IResourceProvider { + + @Override + public Class getResourceType() { + return Patient.class; + } + + @Read + public Patient read(@IdParam IdType theId, SummaryEnum theSummary) { + ourLastSummary = theSummary; + Patient patient = new Patient(); + patient.setId("Patient/1/_history/1"); + patient.getText().setDivAsString("
THE DIV
"); + patient.addName().setFamily("FAMILY"); + patient.getMaritalStatus().addCoding().setCode("D"); + return patient; + } + + @Search(queryName = "multi") + public Patient search(List theSummary) { + ourLastSummaryList = theSummary; + Patient patient = new Patient(); + patient.setId("Patient/1/_history/1"); + patient.getText().setDivAsString("
THE DIV
"); + patient.addName().setFamily("FAMILY"); + patient.getMaritalStatus().addCoding().setCode("D"); + return patient; + } + + @Search() + public Patient search(SummaryEnum theSummary) { + ourLastSummary = theSummary; + Patient patient = new Patient(); + patient.setId("Patient/1/_history/1"); + patient.getText().setDivAsString("
THE DIV
"); + patient.addName().setFamily("FAMILY"); + patient.getMaritalStatus().addCoding().setCode("D"); + return patient; + } + + } + +}