From 3cf8254625011bac661ffb54ade5ab197f68099c Mon Sep 17 00:00:00 2001 From: Anthony Sute Date: Tue, 5 Nov 2019 17:46:33 -0500 Subject: [PATCH 01/10] (1) Fixes to address _filter-based _id retrievals not being restricted to the specified resource type (2) Import fix for FhirServerConfig.java in hapi-fhir-jpaserver-example --- .../ca/uhn/fhir/jpa/dao/SearchBuilder.java | 13 +++++-- .../dao/r4/FhirResourceDaoR4FilterTest.java | 35 +++++++++++++++++++ .../demo/elasticsearch/FhirServerConfig.java | 2 +- 3 files changed, 46 insertions(+), 4 deletions(-) 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 916e855c181..ffcbfb9c076 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 @@ -834,13 +834,18 @@ public class SearchBuilder implements ISearchBuilder { SearchFilterParser.CompareOperation operation = defaultIfNull(theOperation, SearchFilterParser.CompareOperation.eq); assert operation == SearchFilterParser.CompareOperation.eq || operation == SearchFilterParser.CompareOperation.ne; + List codePredicates = new ArrayList<>(); switch (operation) { default: case eq: - nextPredicate = theRoot.get("myId").as(Long.class).in(allOrPids); + codePredicates.add(theRoot.get("myId").as(Long.class).in(allOrPids)); + codePredicates.add(myBuilder.equal(myResourceTableRoot.get("myResourceType"), theResourceName)); + nextPredicate = myBuilder.and(toArray(codePredicates)); break; case ne: - nextPredicate = theRoot.get("myId").as(Long.class).in(allOrPids).not(); + codePredicates.add(theRoot.get("myId").as(Long.class).in(allOrPids).not()); + codePredicates.add(myBuilder.equal(myResourceTableRoot.get("myResourceType"), theResourceName)); + nextPredicate = myBuilder.and(toArray(codePredicates)); break; } @@ -2718,7 +2723,9 @@ public class SearchBuilder implements ISearchBuilder { RuntimeSearchParam searchParam = mySearchParamRegistry.getActiveSearchParam(theResourceName, theFilter.getParamPath().getName()); - if (searchParam.getName().equals(IAnyResource.SP_RES_ID)) { + if (searchParam == null) { + throw new InvalidRequestException("Invalid search parameter specified, " + theFilter.getParamPath().getName() + ", for resource type " + theResourceName); + } else if (searchParam.getName().equals(IAnyResource.SP_RES_ID)) { if (searchParam.getParamType() == RestSearchParameterTypeEnum.TOKEN) { TokenParam param = new TokenParam(); param.setValueAsQueryToken(null, diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4FilterTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4FilterTest.java index 75778d1902d..a7d3b665bb8 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4FilterTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4FilterTest.java @@ -169,6 +169,41 @@ public class FhirResourceDaoR4FilterTest extends BaseJpaR4Test { } } + @Test + public void testRetrieveDifferentTypeEq() { + + Patient p = new Patient(); + p.addName().setFamily("Smith").addGiven("John"); + p.setActive(true); + String id1 = myPatientDao.create(p).getId().toUnqualifiedVersionless().getValue(); + String idVal = id1.split("/")[1]; + + SearchParameterMap map; + List found; + + map = new SearchParameterMap(); + map.setLoadSynchronous(true); + map.add(Constants.PARAM_FILTER, new StringParam(String.format("status eq active or _id eq %s", + idVal))); + found = toUnqualifiedVersionlessIdValues(myEncounterDao.search(map)); + assertThat(found, Matchers.empty()); + + map = new SearchParameterMap(); + map.setLoadSynchronous(true); + map.add(Constants.PARAM_FILTER, new StringParam(String.format("_id eq %s", + idVal))); + found = toUnqualifiedVersionlessIdValues(myEncounterDao.search(map)); + assertThat(found, Matchers.empty()); + + map = new SearchParameterMap(); + map.setLoadSynchronous(true); + map.add(Constants.PARAM_FILTER, new StringParam(String.format("_id eq %s", + idVal))); + found = toUnqualifiedVersionlessIdValues(myPatientDao.search(map)); + assertThat(found, containsInAnyOrder(id1)); + + } + @AfterClass public static void afterClassClearContext() { TestUtil.clearAllStaticFieldsForUnitTest(); diff --git a/hapi-fhir-jpaserver-example/src/main/java/ca/uhn/fhir/jpa/demo/elasticsearch/FhirServerConfig.java b/hapi-fhir-jpaserver-example/src/main/java/ca/uhn/fhir/jpa/demo/elasticsearch/FhirServerConfig.java index 902bd9ab74a..363f8708e0e 100644 --- a/hapi-fhir-jpaserver-example/src/main/java/ca/uhn/fhir/jpa/demo/elasticsearch/FhirServerConfig.java +++ b/hapi-fhir-jpaserver-example/src/main/java/ca/uhn/fhir/jpa/demo/elasticsearch/FhirServerConfig.java @@ -6,7 +6,7 @@ import javax.sql.DataSource; import ca.uhn.fhir.jpa.config.BaseJavaConfigDstu3; import ca.uhn.fhir.jpa.dao.DaoConfig; -import ca.uhn.fhir.jpa.search.elastic.ElasticsearchMappingProvider; +import ca.uhn.fhir.jpa.search.ElasticsearchMappingProvider; import ca.uhn.fhir.jpa.util.SubscriptionsRequireManualActivationInterceptorDstu3; import ca.uhn.fhir.rest.server.interceptor.IServerInterceptor; import ca.uhn.fhir.rest.server.interceptor.ResponseHighlighterInterceptor; From 92de268f32500a3066a7700da416dff4173a9519 Mon Sep 17 00:00:00 2001 From: Anthony Sute Date: Tue, 5 Nov 2019 19:45:17 -0500 Subject: [PATCH 02/10] Fixes for LGTM warnings --- .../src/main/java/ca/uhn/fhir/jpa/dao/SearchBuilder.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) 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 ffcbfb9c076..c787fde2995 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 @@ -2766,7 +2766,7 @@ public class SearchBuilder implements ISearchBuilder { // addPredicateTag(Collections.singletonList(Collections.singletonList(new UriParam(((SearchFilterParser.FilterParameter) theFilter).getValue()))), // searchParam.getName()); // } - else if (searchParam != null) { + else { RestSearchParameterTypeEnum typeEnum = searchParam.getParamType(); if (typeEnum == RestSearchParameterTypeEnum.URI) { return addPredicateUri(theResourceName, @@ -2814,8 +2814,6 @@ public class SearchBuilder implements ISearchBuilder { Collections.singletonList(param), theFilter.getOperation()); } - } else { - throw new InvalidRequestException("Invalid search parameter specified, " + theFilter.getParamPath().getName() + ", for resource type " + theResourceName); } return null; } From f02d80365e514e875fc72907941a336fefe48d80 Mon Sep 17 00:00:00 2001 From: jamesagnew Date: Wed, 6 Nov 2019 08:50:10 -0500 Subject: [PATCH 03/10] Credit for #1581 --- src/changes/changes.xml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 01c8111034e..8cf1cb1a86f 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -501,6 +501,11 @@ A NullPointerException in the XML Parser was fixed when serializing a resource containing an extension on a primitive datatype that was missing a URL declaration. + + When using the _filter search parameter in the JPA server with an untyped resource ID, the + filter could bring in search results of the wrong type. Thanks to Anthony Sute for the Pull + Request and Jens Villadsen for reporting! + From ebc52e6b4867a4c8bfca6bbc94a2dcc08835ac52 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 7 Nov 2019 08:49:12 -0500 Subject: [PATCH 04/10] Correctly handle below expansion with missing system --- .../ca/uhn/fhir/i18n/hapi-messages.properties | 2 + .../ca/uhn/fhir/jpa/dao/SearchBuilder.java | 15 ++++ .../r4/FhirResourceDaoR4SearchNoFtTest.java | 87 +++++++++++++++++-- 3 files changed, 97 insertions(+), 7 deletions(-) diff --git a/hapi-fhir-base/src/main/resources/ca/uhn/fhir/i18n/hapi-messages.properties b/hapi-fhir-base/src/main/resources/ca/uhn/fhir/i18n/hapi-messages.properties index 62f9b35e864..16ba7a08737 100644 --- a/hapi-fhir-base/src/main/resources/ca/uhn/fhir/i18n/hapi-messages.properties +++ b/hapi-fhir-base/src/main/resources/ca/uhn/fhir/i18n/hapi-messages.properties @@ -109,6 +109,8 @@ ca.uhn.fhir.jpa.searchparam.extractor.BaseSearchParamExtractor.failedToExtractPa ca.uhn.fhir.jpa.dao.SearchBuilder.invalidQuantityPrefix=Unable to handle quantity prefix "{0}" for value: {1} ca.uhn.fhir.jpa.dao.SearchBuilder.invalidNumberPrefix=Unable to handle number prefix "{0}" for value: {1} ca.uhn.fhir.jpa.dao.SearchBuilder.sourceParamDisabled=The _source parameter is disabled on this server +ca.uhn.fhir.jpa.dao.SearchBuilder.invalidCodeMissingSystem=Invalid token specified for parameter {0} - No system specified: {1}|{2} +ca.uhn.fhir.jpa.dao.SearchBuilder.invalidCodeMissingCode=Invalid token specified for parameter {0} - No code specified: {1}|{2} ca.uhn.fhir.jpa.dao.dstu3.FhirResourceDaoConceptMapDstu3.matchesFound=Matches found! ca.uhn.fhir.jpa.dao.dstu3.FhirResourceDaoConceptMapDstu3.noMatchesFound=No matches found! 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 c787fde2995..6c6d8d4a4d2 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 @@ -1857,9 +1857,11 @@ public class SearchBuilder implements ISearchBuilder { codes.addAll(myTerminologySvc.expandValueSet(code)); } else if (modifier == TokenParamModifier.ABOVE) { system = determineSystemIfMissing(theParamName, code, system); + validateHaveSystemAndCodeForToken(theParamName, code, system); codes.addAll(myTerminologySvc.findCodesAbove(system, code)); } else if (modifier == TokenParamModifier.BELOW) { system = determineSystemIfMissing(theParamName, code, system); + validateHaveSystemAndCodeForToken(theParamName, code, system); codes.addAll(myTerminologySvc.findCodesBelow(system, code)); } else { codes.add(new VersionIndependentConcept(system, code)); @@ -1902,6 +1904,19 @@ public class SearchBuilder implements ISearchBuilder { return retVal; } + private void validateHaveSystemAndCodeForToken(String theParamName, String theCode, String theSystem) { + String systemDesc = defaultIfBlank(theSystem, "(missing)"); + String codeDesc = defaultIfBlank(theCode, "(missing)"); + if (isBlank(theCode)) { + String msg = myContext.getLocalizer().getMessage(SearchBuilder.class, "invalidCodeMissingSystem", theParamName, systemDesc, codeDesc); + throw new InvalidRequestException(msg); + } + if (isBlank(theSystem)) { + String msg = myContext.getLocalizer().getMessage(SearchBuilder.class, "invalidCodeMissingCode", theParamName, systemDesc, codeDesc); + throw new InvalidRequestException(msg); + } + } + private Predicate addPredicateToken(String theResourceName, String theParamName, CriteriaBuilder theBuilder, From theFrom, List theTokens, TokenParamModifier theModifier, TokenModeEnum theTokenMode) { if (myDontUseHashesForSearch) { final Path systemExpression = theFrom.get("mySystem"); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java index 090c953325f..7cac003f8fa 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchNoFtTest.java @@ -2,10 +2,15 @@ package ca.uhn.fhir.jpa.dao.r4; import ca.uhn.fhir.context.RuntimeResourceDefinition; import ca.uhn.fhir.jpa.dao.DaoConfig; -import ca.uhn.fhir.jpa.dao.data.ISearchDao; -import ca.uhn.fhir.jpa.dao.data.ISearchResultDao; import ca.uhn.fhir.jpa.entity.Search; -import ca.uhn.fhir.jpa.model.entity.*; +import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamDate; +import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamNumber; +import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamQuantity; +import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamString; +import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamToken; +import ca.uhn.fhir.jpa.model.entity.ResourceIndexedSearchParamUri; +import ca.uhn.fhir.jpa.model.entity.ResourceLink; +import ca.uhn.fhir.jpa.model.entity.ResourceTable; import ca.uhn.fhir.jpa.searchparam.MatchUrlService; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap.EverythingModeEnum; @@ -34,7 +39,11 @@ import org.hl7.fhir.r4.model.Enumerations.AdministrativeGender; import org.hl7.fhir.r4.model.Observation.ObservationStatus; import org.hl7.fhir.r4.model.Subscription.SubscriptionChannelType; import org.hl7.fhir.r4.model.Subscription.SubscriptionStatus; -import org.junit.*; +import org.junit.After; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.Ignore; +import org.junit.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.transaction.TransactionStatus; import org.springframework.transaction.support.TransactionCallback; @@ -45,11 +54,29 @@ import javax.servlet.http.HttpServletRequest; import java.io.IOException; import java.math.BigDecimal; import java.nio.charset.StandardCharsets; -import java.util.*; +import java.util.Collections; +import java.util.Date; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.TreeSet; import java.util.stream.Collectors; -import static org.hamcrest.Matchers.*; -import static org.junit.Assert.*; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.endsWith; +import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.hasItems; +import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; @SuppressWarnings({"unchecked", "Duplicates"}) @@ -1978,6 +2005,52 @@ public class FhirResourceDaoR4SearchNoFtTest extends BaseJpaR4Test { } + @Test + public void testSearchOnCodesWithBelow() { + myFhirCtx.setParserErrorHandler(new StrictErrorHandler()); + + CodeSystem cs = new CodeSystem(); + cs.setUrl("http://foo"); + cs.setContent(CodeSystem.CodeSystemContentMode.COMPLETE); + cs.addConcept().setCode("111-1") + .addConcept().setCode("111-2"); + cs.addConcept().setCode("222-1") + .addConcept().setCode("222-2"); + myCodeSystemDao.create(cs); + + Observation obs1 = new Observation(); + obs1.getCode().addCoding().setSystem("http://foo").setCode("111-1"); + String id1 = myObservationDao.create(obs1).getId().toUnqualifiedVersionless().getValue(); + + Observation obs2 = new Observation(); + obs2.getCode().addCoding().setSystem("http://foo").setCode("111-2"); + String id2 = myObservationDao.create(obs2).getId().toUnqualifiedVersionless().getValue(); + + + IBundleProvider result; + + result = myObservationDao.search(new SearchParameterMap().setLoadSynchronous(true).add(Observation.SP_CODE, new TokenParam("http://foo", "111-1"))); + assertThat(toUnqualifiedVersionlessIds(result).toString(), toUnqualifiedVersionlessIdValues(result), containsInAnyOrder(id1)); + + result = myObservationDao.search(new SearchParameterMap().setLoadSynchronous(true).add(Observation.SP_CODE, new TokenParam("http://foo", "111-1").setModifier(TokenParamModifier.BELOW))); + assertThat(toUnqualifiedVersionlessIds(result).toString(), toUnqualifiedVersionlessIdValues(result), containsInAnyOrder(id1, id2)); + + try { + myObservationDao.search(new SearchParameterMap().setLoadSynchronous(true).add(Observation.SP_CODE, new TokenParam(null, "111-1").setModifier(TokenParamModifier.BELOW))); + fail(); + } catch (InvalidRequestException e) { + assertEquals("Invalid token specified for parameter code - No code specified: (missing)|111-1", e.getMessage()); + } + + try { + myObservationDao.search(new SearchParameterMap().setLoadSynchronous(true).add(Observation.SP_CODE, new TokenParam("111-1", null).setModifier(TokenParamModifier.BELOW))); + fail(); + } catch (InvalidRequestException e) { + assertEquals("Invalid token specified for parameter code - No system specified: 111-1|(missing)", e.getMessage()); + } + } + + @Test public void testSearchParamChangesType() { String name = "testSearchParamChangesType"; From 056756dbabdd4f3771fc5b3b9eec4c794f0c46c9 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 7 Nov 2019 18:17:09 -0500 Subject: [PATCH 05/10] 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! + From d4aea824607c0c8bf80a90a1ef831af922b5be38 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 7 Nov 2019 21:50:06 -0500 Subject: [PATCH 06/10] Add some test logging --- ...sourceProviderCustomSearchParamR4Test.java | 120 ++++++++++++++++++ 1 file changed, 120 insertions(+) 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..d3f2cd0028d 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,10 +1,16 @@ package ca.uhn.fhir.jpa.provider.r4; +import ca.uhn.fhir.interceptor.api.Hook; +import ca.uhn.fhir.interceptor.api.Pointcut; import ca.uhn.fhir.jpa.dao.BaseHapiFhirDao; import ca.uhn.fhir.jpa.entity.ResourceReindexJobEntity; 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.search.ISearchCoordinatorSvc; +import ca.uhn.fhir.jpa.search.SearchCoordinatorSvcImpl; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; +import ca.uhn.fhir.jpa.util.SpringObjectCaster; import ca.uhn.fhir.rest.api.Constants; import ca.uhn.fhir.rest.api.server.IBundleProvider; import ca.uhn.fhir.rest.gclient.ReferenceClientParam; @@ -14,6 +20,7 @@ import ca.uhn.fhir.util.TestUtil; import org.apache.commons.io.IOUtils; import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpGet; +import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.instance.model.api.IIdType; import org.hl7.fhir.r4.model.*; import org.hl7.fhir.r4.model.CapabilityStatement.CapabilityStatementRestComponent; @@ -26,11 +33,15 @@ import org.junit.After; import org.junit.AfterClass; import org.junit.Before; import org.junit.Test; +import org.springframework.aop.support.AopUtils; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.data.util.ProxyUtils; import org.springframework.transaction.TransactionStatus; import org.springframework.transaction.support.TransactionCallbackWithoutResult; import org.springframework.transaction.support.TransactionTemplate; import java.io.IOException; +import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -43,12 +54,15 @@ public class ResourceProviderCustomSearchParamR4Test extends BaseResourceProvide private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(ResourceProviderCustomSearchParamR4Test.class); + @Override @After public void after() throws Exception { super.after(); myModelConfig.setDefaultSearchParamsCanBeOverridden(new ModelConfig().isDefaultSearchParamsCanBeOverridden()); + myDaoConfig.setAllowContainsSearches(new DaoConfig().isAllowContainsSearches()); + } @Override @@ -431,6 +445,112 @@ 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; + List ids = new ArrayList<>(); + 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++)); + ids.add(myQuestionnaireDao.create(q).getId().getIdPartAsLong()); + } + + int foundCount = 0; + Bundle bundle = null; + List actualIds = new ArrayList<>(); + 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(); + } + List resources = BundleUtil.toListOfResources(myFhirCtx, bundle); + resources.forEach(t->actualIds.add(t.getIdElement().getIdPartAsLong())); + foundCount += resources.size(); + + } while (bundle.getLink("next") != null); + + + runInTransaction(()->{ + + List searches = mySearchEntityDao.findAll(); + assertEquals(1, searches.size()); + Search search = searches.get(0); + String message = "\nWanted: " + ids + "\n" + + "Actual: " + actualIds + "\n" + + search.toString(); + assertEquals(message, 200, search.getNumFound()); + assertEquals(message, 200, search.getTotalCount().intValue()); + assertEquals(message, SearchStatusEnum.FINISHED, search.getStatus()); + }); + + assertEquals(200, foundCount); + + } + + @AfterClass public static void afterClassClearContext() { TestUtil.clearAllStaticFieldsForUnitTest(); From 7c8342dcc1233248a9321c60d4191fe75f269afb Mon Sep 17 00:00:00 2001 From: jamesagnew Date: Fri, 8 Nov 2019 05:53:59 -0500 Subject: [PATCH 07/10] Add some test logging --- .../ca/uhn/fhir/interceptor/api/Pointcut.java | 27 ++ .../ca/uhn/fhir/jpa/dao/SearchBuilder.java | 18 +- ...sourceProviderCustomSearchParamR4Test.java | 258 ++++++++++-------- 3 files changed, 179 insertions(+), 124 deletions(-) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/api/Pointcut.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/api/Pointcut.java index 558595de056..61d6a604cb6 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/api/Pointcut.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/api/Pointcut.java @@ -1599,6 +1599,33 @@ public enum Pointcut { ), + /** + * THIS IS AN EXPERIMENTAL HOOK AND MAY BE REMOVED OR CHANGED WITHOUT WARNING. + * + * Note that this is a performance tracing hook. Use with caution in production + * systems, since calling it may (or may not) carry a cost. + *

+ * This hook is invoked when a search has found an individual ID. + *

+ * Hooks may accept the following parameters: + *
    + *
  • + * java.lang.Integer - The query ID + *
    • + *
  • + * java.lang.Object - The ID + *
    • + *
+ *

+ * Hooks should return void. + *

+ */ + JPA_PERFTRACE_SEARCH_FOUND_ID(void.class, + "java.lang.Integer", + "java.lang.Object" + ), + + /** * Note that this is a performance tracing hook. Use with caution in production * systems, since calling it may (or may not) carry a cost. 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 4103a757587..43c194050f6 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 @@ -3141,6 +3141,8 @@ public class SearchBuilder implements ISearchBuilder { private final SearchRuntimeDetails mySearchRuntimeDetails; private final RequestDetails myRequest; + private final boolean myHaveRawSqlHooks; + private final boolean myHavePerftraceFoundIdHook; private boolean myFirst = true; private IncludesIterator myIncludesIterator; private Long myNext; @@ -3159,13 +3161,16 @@ public class SearchBuilder implements ISearchBuilder { if (myParams.getEverythingMode() != null) { myStillNeedToFetchIncludes = true; } + + myHavePerftraceFoundIdHook =JpaInterceptorBroadcaster.hasHooks(Pointcut.JPA_PERFTRACE_SEARCH_FOUND_ID, myInterceptorBroadcaster, myRequest); + myHaveRawSqlHooks = JpaInterceptorBroadcaster.hasHooks(Pointcut.JPA_PERFTRACE_RAW_SQL, myInterceptorBroadcaster, myRequest); + } private void fetchNext() { - boolean haveRawSqlHooks = JpaInterceptorBroadcaster.hasHooks(Pointcut.JPA_PERFTRACE_RAW_SQL, myInterceptorBroadcaster, myRequest); try { - if (haveRawSqlHooks) { + if (myHaveRawSqlHooks) { CurrentThreadCaptureQueriesListener.startCapturing(); } @@ -3206,6 +3211,13 @@ public class SearchBuilder implements ISearchBuilder { if (myNext == null) { while (myResultsIterator.hasNext()) { Long next = myResultsIterator.next(); + if (myHavePerftraceFoundIdHook) { + HookParams params = new HookParams() + .add(Integer.class, System.identityHashCode(this)) + .add(Object.class, next); + JpaInterceptorBroadcaster.doCallHooks(myInterceptorBroadcaster, myRequest, Pointcut.JPA_PERFTRACE_SEARCH_FOUND_ID, params); + } + if (next != null) { if (myPidSet.add(next)) { myNext = next; @@ -3244,7 +3256,7 @@ public class SearchBuilder implements ISearchBuilder { mySearchRuntimeDetails.setFoundMatchesCount(myPidSet.size()); } finally { - if (haveRawSqlHooks) { + if (myHaveRawSqlHooks) { SqlQueryList capturedQueries = CurrentThreadCaptureQueriesListener.getCurrentQueueAndStopCapturing(); HookParams params = new HookParams() .add(RequestDetails.class, myRequest) 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 0d0424f074d..150873e3c5e 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,5 +1,7 @@ package ca.uhn.fhir.jpa.provider.r4; +import ca.uhn.fhir.interceptor.api.Hook; +import ca.uhn.fhir.interceptor.api.Pointcut; import ca.uhn.fhir.jpa.dao.BaseHapiFhirDao; import ca.uhn.fhir.jpa.dao.DaoConfig; import ca.uhn.fhir.jpa.entity.ResourceReindexJobEntity; @@ -35,7 +37,6 @@ import org.springframework.transaction.TransactionStatus; import org.springframework.transaction.support.TransactionCallbackWithoutResult; import org.springframework.transaction.support.TransactionTemplate; -import java.io.IOException; import java.util.ArrayList; import java.util.HashMap; import java.util.List; @@ -114,9 +115,9 @@ public class ResourceProviderCustomSearchParamR4Test extends BaseResourceProvide myModelConfig.setDefaultSearchParamsCanBeOverridden(true); CapabilityStatement conformance = ourClient - .fetchConformance() - .ofType(CapabilityStatement.class) - .execute(); + .fetchConformance() + .ofType(CapabilityStatement.class) + .execute(); Map map = extractSearchParams(conformance, "Patient"); CapabilityStatementRestResourceSearchParamComponent param = map.get("foo"); @@ -166,9 +167,9 @@ public class ResourceProviderCustomSearchParamR4Test extends BaseResourceProvide }); conformance = ourClient - .fetchConformance() - .ofType(CapabilityStatement.class) - .execute(); + .fetchConformance() + .ofType(CapabilityStatement.class) + .execute(); map = extractSearchParams(conformance, "Patient"); param = map.get("foo"); @@ -184,9 +185,9 @@ public class ResourceProviderCustomSearchParamR4Test extends BaseResourceProvide myModelConfig.setDefaultSearchParamsCanBeOverridden(false); CapabilityStatement conformance = ourClient - .fetchConformance() - .ofType(CapabilityStatement.class) - .execute(); + .fetchConformance() + .ofType(CapabilityStatement.class) + .execute(); Map map = extractSearchParams(conformance, "Patient"); CapabilityStatementRestResourceSearchParamComponent param = map.get("foo"); @@ -221,9 +222,9 @@ public class ResourceProviderCustomSearchParamR4Test extends BaseResourceProvide mySearchParamRegistry.forceRefresh(); conformance = ourClient - .fetchConformance() - .ofType(CapabilityStatement.class) - .execute(); + .fetchConformance() + .ofType(CapabilityStatement.class) + .execute(); map = extractSearchParams(conformance, "Patient"); param = map.get("foo"); @@ -260,7 +261,7 @@ public class ResourceProviderCustomSearchParamR4Test extends BaseResourceProvide fooSp.setStatus(org.hl7.fhir.r4.model.Enumerations.PublicationStatus.ACTIVE); mySearchParameterDao.create(fooSp, mySrd); - runInTransaction(()->{ + runInTransaction(() -> { List allJobs = myResourceReindexJobDao.findAll(); assertEquals(1, allJobs.size()); assertEquals("Patient", allJobs.get(0).getResourceType()); @@ -327,9 +328,9 @@ public class ResourceProviderCustomSearchParamR4Test extends BaseResourceProvide ourLog.info(myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(eyeColourSp)); ourClient - .create() - .resource(eyeColourSp) - .execute(); + .create() + .resource(eyeColourSp) + .execute(); mySearchParamRegistry.forceRefresh(); @@ -346,11 +347,11 @@ public class ResourceProviderCustomSearchParamR4Test extends BaseResourceProvide IIdType p2id = myPatientDao.create(p2).getId().toUnqualifiedVersionless(); Bundle bundle = ourClient - .search() - .forResource(Patient.class) - .where(new TokenClientParam("eyecolour").exactly().code("blue")) - .returnBundle(Bundle.class) - .execute(); + .search() + .forResource(Patient.class) + .where(new TokenClientParam("eyecolour").exactly().code("blue")) + .returnBundle(Bundle.class) + .execute(); ourLog.info(myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(bundle)); @@ -393,11 +394,11 @@ public class ResourceProviderCustomSearchParamR4Test extends BaseResourceProvide Bundle result; result = ourClient - .search() - .forResource(Observation.class) - .where(new ReferenceClientParam("foo").hasChainedProperty(Patient.GENDER.exactly().code("male"))) - .returnBundle(Bundle.class) - .execute(); + .search() + .forResource(Observation.class) + .where(new ReferenceClientParam("foo").hasChainedProperty(Patient.GENDER.exactly().code("male"))) + .returnBundle(Bundle.class) + .execute(); foundResources = toUnqualifiedVersionlessIdValues(result); assertThat(foundResources, contains(obsId1.getValue())); @@ -433,11 +434,11 @@ public class ResourceProviderCustomSearchParamR4Test extends BaseResourceProvide Bundle result; result = ourClient - .search() - .forResource(Patient.class) - .where(new TokenClientParam("foo").exactly().code("male")) - .returnBundle(Bundle.class) - .execute(); + .search() + .forResource(Patient.class) + .where(new TokenClientParam("foo").exactly().code("male")) + .returnBundle(Bundle.class) + .execute(); foundResources = toUnqualifiedVersionlessIdValues(result); assertThat(foundResources, contains(patId.getValue())); @@ -450,103 +451,118 @@ public class ResourceProviderCustomSearchParamR4Test extends BaseResourceProvide @Test public void testCustomParameterMatchingManyValues() { - myDaoConfig.setAllowContainsSearches(true); + List found = new ArrayList<>(); - // 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; - List ids = new ArrayList<>(); - 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++)); - ids.add(myQuestionnaireDao.create(q).getId().getIdPartAsLong()); - } - - int foundCount = 0; - Bundle bundle = null; - List actualIds = new ArrayList<>(); - 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(); + class Interceptor { + @Hook(Pointcut.JPA_PERFTRACE_SEARCH_FOUND_ID) + public void foundId(Integer theSearchId, Object theId) { + found.add(theSearchId + "/" + theId); } - List resources = BundleUtil.toListOfResources(myFhirCtx, bundle); - resources.forEach(t->actualIds.add(t.getIdElement().getIdPartAsLong())); - foundCount += resources.size(); + } + Interceptor interceptor = new Interceptor(); + myInterceptorRegistry.registerInterceptor(interceptor); + try { + myDaoConfig.setAllowContainsSearches(true); - } while (bundle.getLink("next") != null); + // 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; + List ids = new ArrayList<>(); + 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++)); + ids.add(myQuestionnaireDao.create(q).getId().getIdPartAsLong()); + } - runInTransaction(()->{ + int foundCount = 0; + Bundle bundle = null; + List actualIds = new ArrayList<>(); + do { - List searches = mySearchEntityDao.findAll(); - assertEquals(1, searches.size()); - Search search = searches.get(0); - String message = "\nWanted: " + ids + "\n" + - "Actual: " + actualIds + "\n" + - search.toString(); - assertEquals(message, 200, search.getNumFound()); - assertEquals(message, 200, search.getTotalCount().intValue()); - assertEquals(message, SearchStatusEnum.FINISHED, search.getStatus()); - }); + if (bundle == null) { + bundle = ourClient + .search() + .byUrl(ourServerBase + "/Questionnaire?item-text:contains=Section") + .returnBundle(Bundle.class) + .execute(); + } else { + bundle = ourClient + .loadPage() + .next(bundle) + .execute(); + } + List resources = BundleUtil.toListOfResources(myFhirCtx, bundle); + resources.forEach(t -> actualIds.add(t.getIdElement().getIdPartAsLong())); + foundCount += resources.size(); - assertEquals(200, foundCount); + } while (bundle.getLink("next") != null); + ourLog.info("Found: {}", found); + + runInTransaction(() -> { + + List searches = mySearchEntityDao.findAll(); + assertEquals(1, searches.size()); + Search search = searches.get(0); + String message = "\nWanted: " + ids + "\n" + + "Actual: " + actualIds + "\n" + + "Found : " + found + "\n" + + search.toString(); + assertEquals(message, 200, search.getNumFound()); + assertEquals(message, 200, search.getTotalCount().intValue()); + assertEquals(message, SearchStatusEnum.FINISHED, search.getStatus()); + }); + + assertEquals(200, foundCount); + } finally { + myInterceptorRegistry.unregisterInterceptor(interceptor); + } } From d19341c9a088f9dddcb72a31ce546b90205118ff Mon Sep 17 00:00:00 2001 From: jamesagnew Date: Fri, 8 Nov 2019 06:36:17 -0500 Subject: [PATCH 08/10] Update badges --- src/site/xdoc/index.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/site/xdoc/index.xml b/src/site/xdoc/index.xml index dab45f9dec5..ad31b24b712 100644 --- a/src/site/xdoc/index.xml +++ b/src/site/xdoc/index.xml @@ -17,9 +17,9 @@

- Build Status + Build Status
- Coverage Status + Coverage Status
Maven Central
From 3b1d8d1bc0cf2365fd7e047bc551b01349d94f84 Mon Sep 17 00:00:00 2001 From: jamesagnew Date: Fri, 8 Nov 2019 07:27:25 -0500 Subject: [PATCH 09/10] Add test logging sort --- .../r4/ResourceProviderCustomSearchParamR4Test.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) 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 150873e3c5e..bde1f3131d6 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 @@ -49,6 +49,7 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; +import static org.thymeleaf.util.ListUtils.sort; public class ResourceProviderCustomSearchParamR4Test extends BaseResourceProviderR4Test { @@ -550,9 +551,9 @@ public class ResourceProviderCustomSearchParamR4Test extends BaseResourceProvide List searches = mySearchEntityDao.findAll(); assertEquals(1, searches.size()); Search search = searches.get(0); - String message = "\nWanted: " + ids + "\n" + - "Actual: " + actualIds + "\n" + - "Found : " + found + "\n" + + String message = "\nWanted: " + sort(ids) + "\n" + + "Actual: " + sort(actualIds) + "\n" + + "Found : " + sort(found) + "\n" + search.toString(); assertEquals(message, 200, search.getNumFound()); assertEquals(message, 200, search.getTotalCount().intValue()); From 1a348abaf504c21d21824a242c3751a9f6b7e86b Mon Sep 17 00:00:00 2001 From: James Agnew Date: Fri, 8 Nov 2019 08:45:08 -0500 Subject: [PATCH 10/10] Try to fix 2 intermittent test failures --- .../ca/uhn/fhir/fluentpath/IFluentPath.java | 6 +++++- .../r4/FhirResourceDaoSearchParameterR4.java | 5 ++--- ...sourceProviderCustomSearchParamR4Test.java | 9 ++++---- ...minologyLoaderSvcIntegrationDstu3Test.java | 21 +++++++++++++------ .../hapi/fluentpath/FluentPathDstu3.java | 5 +++++ .../fhir/r4/hapi/fluentpath/FluentPathR4.java | 5 +++++ .../hl7/fhir/r5/hapi/fhirpath/FhirPathR5.java | 5 +++++ 7 files changed, 41 insertions(+), 15 deletions(-) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/fluentpath/IFluentPath.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/fluentpath/IFluentPath.java index b7482133676..895ec8602ae 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/fluentpath/IFluentPath.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/fluentpath/IFluentPath.java @@ -47,5 +47,9 @@ public interface IFluentPath { */ Optional evaluateFirst(IBase theInput, String thePath, Class theReturnType); - + + /** + * Parses the expression and throws an exception if it can not parse correctly + */ + void parse(String theExpression) throws Exception; } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoSearchParameterR4.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoSearchParameterR4.java index 44fb191cabb..b0da0bf4f19 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoSearchParameterR4.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoSearchParameterR4.java @@ -145,10 +145,9 @@ public class FhirResourceDaoSearchParameterR4 extends BaseHapiFhirResourceDao searches = mySearchEntityDao.findAll(); assertEquals(1, searches.size()); Search search = searches.get(0); - String message = "\nWanted: " + sort(ids) + "\n" + - "Actual: " + sort(actualIds) + "\n" + - "Found : " + sort(found) + "\n" + + String message = "\nWanted: " + (ids) + "\n" + + "Actual: " + (actualIds) + "\n" + + "Found : " + (found) + "\n" + search.toString(); assertEquals(message, 200, search.getNumFound()); assertEquals(message, 200, search.getTotalCount().intValue()); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TerminologyLoaderSvcIntegrationDstu3Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TerminologyLoaderSvcIntegrationDstu3Test.java index 6ef7afe5c59..cb5a2b9d82b 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TerminologyLoaderSvcIntegrationDstu3Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/term/TerminologyLoaderSvcIntegrationDstu3Test.java @@ -7,7 +7,13 @@ import ca.uhn.fhir.jpa.dao.dstu3.BaseJpaDstu3Test; import ca.uhn.fhir.jpa.term.api.ITermLoaderSvc; import ca.uhn.fhir.util.TestUtil; import com.google.common.collect.Lists; -import org.hl7.fhir.dstu3.model.*; +import org.hl7.fhir.dstu3.model.CodeType; +import org.hl7.fhir.dstu3.model.Coding; +import org.hl7.fhir.dstu3.model.Parameters; +import org.hl7.fhir.dstu3.model.PrimitiveType; +import org.hl7.fhir.dstu3.model.StringType; +import org.hl7.fhir.dstu3.model.Type; +import org.hl7.fhir.dstu3.model.ValueSet; import org.hl7.fhir.instance.model.api.IPrimitiveType; import org.junit.After; import org.junit.AfterClass; @@ -24,8 +30,13 @@ import java.util.Set; import java.util.stream.Collectors; import static org.awaitility.Awaitility.await; -import static org.hamcrest.Matchers.*; -import static org.junit.Assert.*; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.greaterThan; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; public class TerminologyLoaderSvcIntegrationDstu3Test extends BaseJpaDstu3Test { @@ -116,9 +127,7 @@ public class TerminologyLoaderSvcIntegrationDstu3Test extends BaseJpaDstu3Test { myTerminologyDeferredStorageSvc.saveDeferred(); - runInTransaction(() -> { - await().until(() -> myTermConceptMapDao.count(), greaterThan(0L)); - }); + await().until(() -> runInTransaction(() -> myTermConceptMapDao.count()), greaterThan(0L)); } @Test diff --git a/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/hapi/fluentpath/FluentPathDstu3.java b/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/hapi/fluentpath/FluentPathDstu3.java index 11da788d945..cfb1daeaaa5 100644 --- a/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/hapi/fluentpath/FluentPathDstu3.java +++ b/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/hapi/fluentpath/FluentPathDstu3.java @@ -49,4 +49,9 @@ public class FluentPathDstu3 implements IFluentPath { return evaluate(theInput, thePath, theReturnType).stream().findFirst(); } + @Override + public void parse(String theExpression) { + myEngine.parse(theExpression); + } + } diff --git a/hapi-fhir-structures-r4/src/main/java/org/hl7/fhir/r4/hapi/fluentpath/FluentPathR4.java b/hapi-fhir-structures-r4/src/main/java/org/hl7/fhir/r4/hapi/fluentpath/FluentPathR4.java index 45e9aa7e27f..f85d63a6472 100644 --- a/hapi-fhir-structures-r4/src/main/java/org/hl7/fhir/r4/hapi/fluentpath/FluentPathR4.java +++ b/hapi-fhir-structures-r4/src/main/java/org/hl7/fhir/r4/hapi/fluentpath/FluentPathR4.java @@ -49,5 +49,10 @@ public class FluentPathR4 implements IFluentPath { return evaluate(theInput, thePath, theReturnType).stream().findFirst(); } + @Override + public void parse(String theExpression) { + myEngine.parse(theExpression); + } + } diff --git a/hapi-fhir-structures-r5/src/main/java/org/hl7/fhir/r5/hapi/fhirpath/FhirPathR5.java b/hapi-fhir-structures-r5/src/main/java/org/hl7/fhir/r5/hapi/fhirpath/FhirPathR5.java index 7c3345cedf3..1d68df9bb84 100644 --- a/hapi-fhir-structures-r5/src/main/java/org/hl7/fhir/r5/hapi/fhirpath/FhirPathR5.java +++ b/hapi-fhir-structures-r5/src/main/java/org/hl7/fhir/r5/hapi/fhirpath/FhirPathR5.java @@ -49,5 +49,10 @@ public class FhirPathR5 implements IFluentPath { return evaluate(theInput, thePath, theReturnType).stream().findFirst(); } + @Override + public void parse(String theExpression) { + myEngine.parse(theExpression); + } + }