From 290894557d2fb3e09980c5651d9d8f2cfe0e3f8a Mon Sep 17 00:00:00 2001 From: James Agnew Date: Fri, 13 Oct 2017 06:31:37 -0400 Subject: [PATCH] Optionally require JPA server to collect count information even for large searches --- .../java/ca/uhn/fhir/jpa/dao/DaoConfig.java | 65 +++++++++++++++-- .../jpa/search/SearchCoordinatorSvcImpl.java | 14 +++- .../provider/r4/ResourceProviderR4Test.java | 71 +++++++++++++++++++ .../uhn/fhirtest/config/TdlDstu2Config.java | 1 + .../uhn/fhirtest/config/TdlDstu3Config.java | 1 + .../ca/uhn/fhirtest/config/TestR4Config.java | 2 + src/changes/changes.xml | 13 ++++ 7 files changed, 158 insertions(+), 9 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/DaoConfig.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/DaoConfig.java index 0d25bd2dfee..e981581192b 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/DaoConfig.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/DaoConfig.java @@ -108,6 +108,7 @@ public class DaoConfig { private Set myTreatReferencesAsLogical = new HashSet(DEFAULT_LOGICAL_BASE_URLS); private boolean myAutoCreatePlaceholderReferenceTargets; private Integer myCacheControlNoStoreMaxResultsUpperLimit = 1000; + private Integer myCountSearchResultsUpTo = null; /** * Constructor @@ -152,6 +153,56 @@ public class DaoConfig { myCacheControlNoStoreMaxResultsUpperLimit = theCacheControlNoStoreMaxResults; } + /** + * When searching, if set to a non-null value (default is null) the + * search coordinator will attempt to find at least this many results + * before returning a response to the client. This parameter mainly affects + * whether a "total count" is included in the response bundle for searches that + * return large amounts of data. + *

+ * For a search that returns 10000 results, if this value is set to + * 10000 the search coordinator will find all 10000 results + * prior to returning, so the initial response bundle will have the + * total set to 10000. If this value is null (or less than 10000) + * the response bundle will likely return slightly faster, but will + * not include the total. Subsequent page requests will likely + * include the total however, if they are performed after the + * search coordinator has found all results. + *

+ *

+ * Set this value to 0 to always load all + * results before returning. + *

+ */ + public Integer getCountSearchResultsUpTo() { + return myCountSearchResultsUpTo; + } + + /** + * When searching, if set to a non-null value (default is null) the + * search coordinator will attempt to find at least this many results + * before returning a response to the client. This parameter mainly affects + * whether a "total count" is included in the response bundle for searches that + * return large amounts of data. + *

+ * For a search that returns 10000 results, if this value is set to + * 10000 the search coordinator will find all 10000 results + * prior to returning, so the initial response bundle will have the + * total set to 10000. If this value is null (or less than 10000) + * the response bundle will likely return slightly faster, but will + * not include the total. Subsequent page requests will likely + * include the total however, if they are performed after the + * search coordinator has found all results. + *

+ *

+ * Set this value to 0 to always load all + * results before returning. + *

+ */ + public void setCountSearchResultsUpTo(Integer theCountSearchResultsUpTo) { + myCountSearchResultsUpTo = theCountSearchResultsUpTo; + } + /** * When a code system is added that contains more than this number of codes, * the code system will be indexed later in an incremental process in order to @@ -357,11 +408,8 @@ public class DaoConfig { /** * This may be used to optionally register server interceptors directly against the DAOs. */ - public void setInterceptors(IServerInterceptor... theInterceptor) { - setInterceptors(new ArrayList()); - if (theInterceptor != null && theInterceptor.length != 0) { - getInterceptors().addAll(Arrays.asList(theInterceptor)); - } + public void setInterceptors(List theInterceptors) { + myInterceptors = theInterceptors; } /** @@ -959,8 +1007,11 @@ public class DaoConfig { /** * This may be used to optionally register server interceptors directly against the DAOs. */ - public void setInterceptors(List theInterceptors) { - myInterceptors = theInterceptors; + public void setInterceptors(IServerInterceptor... theInterceptor) { + setInterceptors(new ArrayList()); + if (theInterceptor != null && theInterceptor.length != 0) { + getInterceptors().addAll(Arrays.asList(theInterceptor)); + } } /** 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 2160c9fec2a..8ca7b7dab30 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 @@ -509,6 +509,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { } myIdToSearchTask.remove(mySearch.getUuid()); + myInitialCollectionLatch.countDown(); myCompletionLatch.countDown(); return null; } @@ -574,7 +575,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { } } - ArrayList retVal = new ArrayList(); + ArrayList retVal = new ArrayList<>(); synchronized (mySyncedPids) { verifySearchHasntFailedOrThrowInternalErrorException(mySearch); @@ -645,7 +646,16 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { } }); - myInitialCollectionLatch.countDown(); + int numSynced; + synchronized (mySyncedPids) { + numSynced = mySyncedPids.size(); + } + + if (myDaoConfig.getCountSearchResultsUpTo() == null || + myDaoConfig.getCountSearchResultsUpTo() <= 0 || + myDaoConfig.getCountSearchResultsUpTo() <= numSynced) { + myInitialCollectionLatch.countDown(); + } } } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java index 652784b5b17..ecf24feb853 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4Test.java @@ -3194,6 +3194,77 @@ public class ResourceProviderR4Test extends BaseResourceProviderR4Test { assertEquals(SearchEntryMode.INCLUDE, found.getEntry().get(1).getSearch().getMode()); } + @Test + public void testSearchWithCountNotSet() throws Exception { + mySearchCoordinatorSvcRaw.setSyncSizeForUnitTests(1); + mySearchCoordinatorSvcRaw.setLoadingThrottleForUnitTests(100); + + for (int i =0; i < 10; i++) { + Patient pat = new Patient(); + pat.addIdentifier().setSystem("urn:system:rpdstu2").setValue("test" + i); + ourClient.create().resource(pat).execute(); + } + + Bundle found = ourClient + .search() + .forResource(Patient.class) + .returnBundle(Bundle.class) + .count(1) + .execute(); + + // If this fails under load, try increasing the throttle above + assertEquals(null, found.getTotalElement().getValue()); + assertEquals(1, found.getEntry().size()); + } + + @Test + public void testSearchWithCountSearchResultsUpTo5() throws Exception { + mySearchCoordinatorSvcRaw.setSyncSizeForUnitTests(1); + mySearchCoordinatorSvcRaw.setLoadingThrottleForUnitTests(100); + myDaoConfig.setCountSearchResultsUpTo(5); + + for (int i =0; i < 10; i++) { + Patient pat = new Patient(); + pat.addIdentifier().setSystem("urn:system:rpdstu2").setValue("test" + i); + ourClient.create().resource(pat).execute(); + } + + Bundle found = ourClient + .search() + .forResource(Patient.class) + .returnBundle(Bundle.class) + .count(1) + .execute(); + + // If this fails under load, try increasing the throttle above + assertEquals(null, found.getTotalElement().getValue()); + assertEquals(1, found.getEntry().size()); + } + + @Test + public void testSearchWithCountSearchResultsUpTo20() throws Exception { + mySearchCoordinatorSvcRaw.setSyncSizeForUnitTests(1); + mySearchCoordinatorSvcRaw.setLoadingThrottleForUnitTests(100); + myDaoConfig.setCountSearchResultsUpTo(20); + + for (int i =0; i < 10; i++) { + Patient pat = new Patient(); + pat.addIdentifier().setSystem("urn:system:rpdstu2").setValue("test" + i); + ourClient.create().resource(pat).execute(); + } + + Bundle found = ourClient + .search() + .forResource(Patient.class) + .returnBundle(Bundle.class) + .count(1) + .execute(); + + // If this fails under load, try increasing the throttle above + assertEquals(10, found.getTotalElement().getValue().intValue()); + assertEquals(1, found.getEntry().size()); + } + @Test() public void testSearchWithInvalidNumberPrefix() throws Exception { try { diff --git a/hapi-fhir-jpaserver-uhnfhirtest/src/main/java/ca/uhn/fhirtest/config/TdlDstu2Config.java b/hapi-fhir-jpaserver-uhnfhirtest/src/main/java/ca/uhn/fhirtest/config/TdlDstu2Config.java index 47380b48473..bf568c7d1c9 100644 --- a/hapi-fhir-jpaserver-uhnfhirtest/src/main/java/ca/uhn/fhirtest/config/TdlDstu2Config.java +++ b/hapi-fhir-jpaserver-uhnfhirtest/src/main/java/ca/uhn/fhirtest/config/TdlDstu2Config.java @@ -67,6 +67,7 @@ public class TdlDstu2Config extends BaseJavaConfigDstu2 { retVal.getTreatBaseUrlsAsLocal().add("http://fhirtest.uhn.ca/testDataLibraryDstu2"); retVal.getTreatBaseUrlsAsLocal().add("https://fhirtest.uhn.ca/testDataLibraryDstu2"); retVal.setIndexMissingFields(DaoConfig.IndexEnabledEnum.ENABLED); + retVal.setCountSearchResultsUpTo(TestR4Config.COUNT_SEARCH_RESULTS_UP_TO); return retVal; } diff --git a/hapi-fhir-jpaserver-uhnfhirtest/src/main/java/ca/uhn/fhirtest/config/TdlDstu3Config.java b/hapi-fhir-jpaserver-uhnfhirtest/src/main/java/ca/uhn/fhirtest/config/TdlDstu3Config.java index 052d974092d..45be4c9e38b 100644 --- a/hapi-fhir-jpaserver-uhnfhirtest/src/main/java/ca/uhn/fhirtest/config/TdlDstu3Config.java +++ b/hapi-fhir-jpaserver-uhnfhirtest/src/main/java/ca/uhn/fhirtest/config/TdlDstu3Config.java @@ -57,6 +57,7 @@ public class TdlDstu3Config extends BaseJavaConfigDstu3 { retVal.getTreatBaseUrlsAsLocal().add("http://fhirtest.uhn.ca/testDataLibraryStu3"); retVal.getTreatBaseUrlsAsLocal().add("https://fhirtest.uhn.ca/testDataLibraryStu3"); retVal.setIndexMissingFields(DaoConfig.IndexEnabledEnum.ENABLED); + retVal.setCountSearchResultsUpTo(TestR4Config.COUNT_SEARCH_RESULTS_UP_TO); return retVal; } diff --git a/hapi-fhir-jpaserver-uhnfhirtest/src/main/java/ca/uhn/fhirtest/config/TestR4Config.java b/hapi-fhir-jpaserver-uhnfhirtest/src/main/java/ca/uhn/fhirtest/config/TestR4Config.java index 609b537f500..c9462d836c2 100644 --- a/hapi-fhir-jpaserver-uhnfhirtest/src/main/java/ca/uhn/fhirtest/config/TestR4Config.java +++ b/hapi-fhir-jpaserver-uhnfhirtest/src/main/java/ca/uhn/fhirtest/config/TestR4Config.java @@ -34,6 +34,7 @@ public class TestR4Config extends BaseJavaConfigR4 { public static final String FHIR_DB_USERNAME = "${fhir.db.username}"; public static final String FHIR_DB_PASSWORD = "${fhir.db.password}"; public static final String FHIR_LUCENE_LOCATION_R4 = "${fhir.lucene.location.r4}"; + public static final Integer COUNT_SEARCH_RESULTS_UP_TO = 20000; @Value(TestR4Config.FHIR_DB_USERNAME) private String myDbUsername; @@ -56,6 +57,7 @@ public class TestR4Config extends BaseJavaConfigR4 { retVal.getTreatBaseUrlsAsLocal().add("http://fhirtest.uhn.ca/baseR4"); retVal.getTreatBaseUrlsAsLocal().add("https://fhirtest.uhn.ca/baseR4"); retVal.setIndexMissingFields(DaoConfig.IndexEnabledEnum.ENABLED); + retVal.setCountSearchResultsUpTo(TestR4Config.COUNT_SEARCH_RESULTS_UP_TO); return retVal; } diff --git a/src/changes/changes.xml b/src/changes/changes.xml index ead24aa43cb..7ee8aade32c 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -69,6 +69,19 @@ specify a charset. This log line often showed up any time a server was not supplying a response, making client logs quite noisy + + A new configuration item has been added to the JPA server DaoConfig + called + getCountSearchResultsUpTo()]]>. + This setting governs how many search results the search + coordinator should try to find before returning an initial + search response to the user, which has an effect on whether + the + Bundle.total]]> + field is always populated in search responses. This has now + been set to 20000 on out public server (fhirtest.uhn.ca) + so most search results should now include a total. +