From 056756dbabdd4f3771fc5b3b9eec4c794f0c46c9 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 7 Nov 2019 18:17:09 -0500 Subject: [PATCH] Fix paging issue #1300 (#1584) * Fix paging issue * Add changelog with credit * Add a test --- .../ca/uhn/fhir/jpa/dao/SearchBuilder.java | 1 + .../jpa/provider/BinaryAccessProvider.java | 31 ------ .../jpa/search/SearchCoordinatorSvcImpl.java | 3 +- ...sourceProviderCustomSearchParamR4Test.java | 102 ++++++++++++++++++ .../src/test/resources/r4/impact-sp.json | 29 +++++ src/changes/changes.xml | 7 ++ 6 files changed, 141 insertions(+), 32 deletions(-) delete mode 100644 hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/provider/BinaryAccessProvider.java create mode 100644 hapi-fhir-jpaserver-base/src/test/resources/r4/impact-sp.json diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/SearchBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/SearchBuilder.java index 6c6d8d4a4d2..4103a757587 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/SearchBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/SearchBuilder.java @@ -2071,6 +2071,7 @@ public class SearchBuilder implements ISearchBuilder { outerQuery.multiselect(myBuilder.countDistinct(myResourceTableRoot)); } else { outerQuery.multiselect(myResourceTableRoot.get("myId").as(Long.class)); + outerQuery.distinct(true); } } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/provider/BinaryAccessProvider.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/provider/BinaryAccessProvider.java deleted file mode 100644 index a64e6b877e5..00000000000 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/provider/BinaryAccessProvider.java +++ /dev/null @@ -1,31 +0,0 @@ -package ca.uhn.fhir.jpa.provider; - -/*- - * #%L - * HAPI FHIR JPA Server - * %% - * Copyright (C) 2014 - 2019 University Health Network - * %% - * 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. - * See the License for the specific language governing permissions and - * limitations under the License. - * #L% - */ - -/** - * @deprecated Use ca.uhn.fhir.jpa.binstore.BinaryAccessProvider instead - */ -@Deprecated -public class BinaryAccessProvider extends ca.uhn.fhir.jpa.binstore.BinaryAccessProvider { - - // FIXME: JA delete before 4.0.0 - -} 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 50ba12ea791..1552acfbb51 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 @@ -233,6 +233,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { ourLog.trace("Going to try to start next search"); Optional newSearch = mySearchCacheSvc.tryToMarkSearchAsInProgress(search); if (newSearch.isPresent()) { + ourLog.trace("Launching new search"); search = newSearch.get(); String resourceType = search.getResourceType(); SearchParameterMap params = search.getSearchParameterMap().orElseThrow(() -> new IllegalStateException("No map in PASSCOMPLET search")); @@ -1113,7 +1114,7 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { throw newResourceGoneException(getSearch().getUuid()); } - ourLog.debug("Have {} previously added IDs in search: {}", previouslyAddedResourcePids.size(), getSearch().getUuid()); + ourLog.trace("Have {} previously added IDs in search: {}", previouslyAddedResourcePids.size(), getSearch().getUuid()); setPreviouslyAddedResourcePids(previouslyAddedResourcePids); return null; }); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderCustomSearchParamR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderCustomSearchParamR4Test.java index bb4f7f5fe73..3ee8cb01336 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderCustomSearchParamR4Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderCustomSearchParamR4Test.java @@ -1,15 +1,19 @@ package ca.uhn.fhir.jpa.provider.r4; import ca.uhn.fhir.jpa.dao.BaseHapiFhirDao; +import ca.uhn.fhir.jpa.dao.DaoConfig; import ca.uhn.fhir.jpa.entity.ResourceReindexJobEntity; +import ca.uhn.fhir.jpa.entity.Search; import ca.uhn.fhir.jpa.model.entity.ModelConfig; import ca.uhn.fhir.jpa.model.entity.ResourceTable; +import ca.uhn.fhir.jpa.model.search.SearchStatusEnum; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; import ca.uhn.fhir.rest.api.Constants; import ca.uhn.fhir.rest.api.server.IBundleProvider; import ca.uhn.fhir.rest.gclient.ReferenceClientParam; import ca.uhn.fhir.rest.gclient.TokenClientParam; import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException; +import ca.uhn.fhir.util.BundleUtil; import ca.uhn.fhir.util.TestUtil; import org.apache.commons.io.IOUtils; import org.apache.http.client.methods.CloseableHttpResponse; @@ -49,6 +53,8 @@ public class ResourceProviderCustomSearchParamR4Test extends BaseResourceProvide super.after(); myModelConfig.setDefaultSearchParamsCanBeOverridden(new ModelConfig().isDefaultSearchParamsCanBeOverridden()); + myDaoConfig.setAllowContainsSearches(new DaoConfig().isAllowContainsSearches()); + } @Override @@ -431,6 +437,102 @@ public class ResourceProviderCustomSearchParamR4Test extends BaseResourceProvide } + /** + * See #1300 + */ + @Test + public void testCustomParameterMatchingManyValues() { + myDaoConfig.setAllowContainsSearches(true); + + // Add a custom search parameter + SearchParameter fooSp = new SearchParameter(); + fooSp.addBase("Questionnaire"); + fooSp.setCode("item-text"); + fooSp.setName("item-text"); + fooSp.setType(Enumerations.SearchParamType.STRING); + fooSp.setTitle("FOO SP"); + fooSp.setExpression("Questionnaire.item.text | Questionnaire.item.item.text | Questionnaire.item.item.item.text"); + fooSp.setXpathUsage(org.hl7.fhir.r4.model.SearchParameter.XPathUsageType.NORMAL); + fooSp.setStatus(org.hl7.fhir.r4.model.Enumerations.PublicationStatus.ACTIVE); + mySearchParameterDao.create(fooSp, mySrd); + mySearchParamRegistry.forceRefresh(); + + int textIndex = 0; + for (int i = 0; i < 200; i++) { + //Lots and lots of matches + Questionnaire q = new Questionnaire(); + q + .addItem() + .setText("Section " + (textIndex++)) + .addItem() + .setText("Section " + (textIndex++)) + .addItem() + .setText("Section " + (textIndex++)); + q + .addItem() + .setText("Section " + (textIndex++)) + .addItem() + .setText("Section " + (textIndex++)) + .addItem() + .setText("Section " + (textIndex++)); + q + .addItem() + .setText("Section " + (textIndex++)) + .addItem() + .setText("Section " + (textIndex++)) + .addItem() + .setText("Section " + (textIndex++)); + q + .addItem() + .setText("Section " + (textIndex++)) + .addItem() + .setText("Section " + (textIndex++)) + .addItem() + .setText("Section " + (textIndex++)); + q + .addItem() + .setText("Section " + (textIndex++)) + .addItem() + .setText("Section " + (textIndex++)) + .addItem() + .setText("Section " + (textIndex++)); + myQuestionnaireDao.create(q); + } + + int foundCount = 0; + Bundle bundle = null; + do { + + if (bundle == null) { + bundle = ourClient + .search() + .byUrl(ourServerBase + "/Questionnaire?item-text:contains=Section") + .returnBundle(Bundle.class) + .execute(); + } else { + bundle = ourClient + .loadPage() + .next(bundle) + .execute(); + } + foundCount += BundleUtil.toListOfResources(myFhirCtx, bundle).size(); + + } while (bundle.getLink("next") != null); + + runInTransaction(()->{ + List searches = mySearchEntityDao.findAll(); + assertEquals(1, searches.size()); + Search search = searches.get(0); + assertEquals(search.toString(), 200, search.getNumFound()); + assertEquals(search.toString(), 200, search.getTotalCount().intValue()); + assertEquals(search.toString(), SearchStatusEnum.FINISHED, search.getStatus()); + }); + + assertEquals(200, foundCount); + + } + + @AfterClass public static void afterClassClearContext() { TestUtil.clearAllStaticFieldsForUnitTest(); diff --git a/hapi-fhir-jpaserver-base/src/test/resources/r4/impact-sp.json b/hapi-fhir-jpaserver-base/src/test/resources/r4/impact-sp.json new file mode 100644 index 00000000000..ceb00aaa887 --- /dev/null +++ b/hapi-fhir-jpaserver-base/src/test/resources/r4/impact-sp.json @@ -0,0 +1,29 @@ +{ + "resourceType": "SearchParameter", + "id": "54805", + "meta": { + "versionId": "1", + "lastUpdated": "2019-11-04T18:33:47.918+00:00", + "source": "#pt4ERkOO6LGm5ZoA" + }, + "text": { + "status": "generated", + "div": "
Search for a questionnaire by item.text nested field - up to 3 levels.
" + }, + "url": "https://impact-fhir.mitre.org/r4/SearchParameter/QuestionnaireItemText", + "version": "0.0.1", + "name": "QuestionnaireItemText", + "status": "active", + "date": "2019-10-25T15:38:45-04:00", + "description": "Search for a questionnaire by item.text nested field - up to 3 levels.", + "code": "item-text", + "base": [ + "Questionnaire" + ], + "type": "string", + "expression": "Questionnaire.item.text | Questionnaire.item.item.text | Questionnaire.item.item.item.text", + "xpathUsage": "normal", + "modifier": [ + "contains" + ] +} diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 8cf1cb1a86f..3c3cc16bd39 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -506,6 +506,13 @@ filter could bring in search results of the wrong type. Thanks to Anthony Sute for the Pull Request and Jens Villadsen for reporting! + + In some cases where where a single search parameter matches the same resource many times with + different distinct values (e.g. a search by Patient:name where there are hundreds of patients having + hundreds of distinct names each) the Search Coordinator would end up in an infinite loop and never + return all of the possible results. Thanks to @imranmoezkhan for reporting, and to + Tim Shaffer for providing a reproducible test case! +