From 669cf05b308708784a7f300f4917ceaa5b4c10b8 Mon Sep 17 00:00:00 2001 From: Tadgh Date: Thu, 12 Aug 2021 14:25:12 -0400 Subject: [PATCH 1/2] Modify SearchParameterMap toNormalizedQueryString to include value of _contained parameter --- .../rest/api/SearchContainedModeEnum.java | 2 +- ...tained-ignored-for-cache-key-purposes.yaml | 6 ++++++ .../dao/r4/SearchCoordinatorSvcImplTest.java | 10 ++++------ .../jpa/provider/SearchParameterMapTest.java | 20 +++++++++++++++++++ ...ResourceProviderR4SearchContainedTest.java | 6 +++++- .../jpa/searchparam/SearchParameterMap.java | 8 ++++++++ 6 files changed, 44 insertions(+), 8 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_5_0/2887-contained-ignored-for-cache-key-purposes.yaml diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/api/SearchContainedModeEnum.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/api/SearchContainedModeEnum.java index e90b201b6e4..bc0f320ed5e 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/api/SearchContainedModeEnum.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/api/SearchContainedModeEnum.java @@ -51,7 +51,7 @@ public enum SearchContainedModeEnum { myCode = theCode; } - private String getCode() { + public String getCode() { return myCode; } diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_5_0/2887-contained-ignored-for-cache-key-purposes.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_5_0/2887-contained-ignored-for-cache-key-purposes.yaml new file mode 100644 index 00000000000..bf2b4b334ba --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_5_0/2887-contained-ignored-for-cache-key-purposes.yaml @@ -0,0 +1,6 @@ +--- +type: fix +issue: 2887 +jira: SMILE-2896 +title: "Fixed a bug where the search results cache was ignoring the value of `_contained` parameter when assigning a cache key. + This was causing queries run in a short period of time to return wrong cached results if one query used `_contained=true` and the other did not." diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/SearchCoordinatorSvcImplTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/SearchCoordinatorSvcImplTest.java index 4eb8f636121..5f983a7ce1f 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/SearchCoordinatorSvcImplTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/SearchCoordinatorSvcImplTest.java @@ -10,10 +10,8 @@ import ca.uhn.fhir.jpa.entity.SearchTypeEnum; import ca.uhn.fhir.jpa.model.search.SearchStatusEnum; import ca.uhn.fhir.jpa.search.cache.DatabaseSearchCacheSvcImpl; import ca.uhn.fhir.jpa.search.cache.ISearchCacheSvc; -import ca.uhn.fhir.util.TestUtil; import org.apache.commons.lang3.time.DateUtils; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; @@ -40,7 +38,7 @@ public class SearchCoordinatorSvcImplTest extends BaseJpaR4Test { private ISearchCoordinatorSvc mySearchCoordinator; @Autowired - private ISearchCacheSvc myDataaseCacheSvc; + private ISearchCacheSvc myDatabaseCacheSvc; @AfterEach public void after() { @@ -92,7 +90,7 @@ public class SearchCoordinatorSvcImplTest extends BaseJpaR4Test { assertEquals(30, mySearchResultDao.count()); }); - myDataaseCacheSvc.pollForStaleSearchesAndDeleteThem(); + myDatabaseCacheSvc.pollForStaleSearchesAndDeleteThem(); runInTransaction(()->{ // We should delete up to 10, but 3 don't get deleted since they have too many results to delete in one pass assertEquals(13, mySearchDao.count()); @@ -101,7 +99,7 @@ public class SearchCoordinatorSvcImplTest extends BaseJpaR4Test { assertEquals(15, mySearchResultDao.count()); }); - myDataaseCacheSvc.pollForStaleSearchesAndDeleteThem(); + myDatabaseCacheSvc.pollForStaleSearchesAndDeleteThem(); runInTransaction(()->{ // Once again we attempt to delete 10, but the first 3 don't get deleted and still remain // (total is 6 because 3 weren't deleted, and they blocked another 3 that might have been) @@ -110,7 +108,7 @@ public class SearchCoordinatorSvcImplTest extends BaseJpaR4Test { assertEquals(0, mySearchResultDao.count()); }); - myDataaseCacheSvc.pollForStaleSearchesAndDeleteThem(); + myDatabaseCacheSvc.pollForStaleSearchesAndDeleteThem(); runInTransaction(()->{ assertEquals(0, mySearchDao.count()); assertEquals(0, mySearchDao.countDeleted()); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/SearchParameterMapTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/SearchParameterMapTest.java index d5210186f4e..39f995742a4 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/SearchParameterMapTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/SearchParameterMapTest.java @@ -1,8 +1,10 @@ package ca.uhn.fhir.jpa.provider; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import ca.uhn.fhir.context.FhirVersionEnum; +import ca.uhn.fhir.rest.api.SearchContainedModeEnum; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.Test; @@ -41,6 +43,24 @@ public class SearchParameterMapTest { assertEquals("?birthdate=ge2001&birthdate=lt2002&name=bouvier,simpson&name=homer,jay&name:exact=ZZZ?", UrlUtil.unescape(queryString)); } + @Test + public void testContainedParameterIsIncludedInNormalizedString() { + SearchParameterMap map = new SearchParameterMap(); + map.add("name", new StringParam("Smith")); + map.setSearchContainedMode(SearchContainedModeEnum.TRUE); + String containedQueryString = map.toNormalizedQueryString(ourCtx); + + SearchParameterMap uncontainedMap = new SearchParameterMap(); + uncontainedMap.add("name", new StringParam("Smith")); + uncontainedMap.setSearchContainedMode(SearchContainedModeEnum.FALSE); + String uncontainedQueryString = uncontainedMap.toNormalizedQueryString(ourCtx); + + ourLog.info(containedQueryString); + ourLog.info(uncontainedQueryString); + assertNotEquals(containedQueryString, uncontainedQueryString); + + } + @Test public void testToQueryStringEmpty() { SearchParameterMap map = new SearchParameterMap(); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4SearchContainedTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4SearchContainedTest.java index af91f8da02a..0c6d74eb7bc 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4SearchContainedTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4SearchContainedTest.java @@ -284,7 +284,6 @@ public class ResourceProviderR4SearchContainedTest extends BaseResourceProviderR assertEquals(0L, oids.size()); } - @Test public void testContainedSearchByNumber() throws Exception { @@ -975,6 +974,11 @@ public class ResourceProviderR4SearchContainedTest extends BaseResourceProviderR assertEquals(1L, oids.size()); assertThat(oids, contains(oid1.getValue())); + } + + //See https://github.com/hapifhir/hapi-fhir/issues/2887 + @Test + public void testContainedResourceParameterIsUsedInCache() { } diff --git a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java index 92c7d29a1aa..778df7b3882 100644 --- a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java +++ b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java @@ -511,6 +511,14 @@ public class SearchParameterMap implements Serializable { b.append(getSearchTotalMode().getCode()); } + //Contained mode + if (getSearchContainedMode() != null) { + addUrlParamSeparator(b); + b.append(Constants.PARAM_CONTAINED); + b.append("="); + b.append(getSearchContainedMode().getCode()); + } + if (b.length() == 0) { b.append('?'); } From 3fd5f7d6d4961b0bf0ca501666647a8174205611 Mon Sep 17 00:00:00 2001 From: Tadgh Date: Thu, 12 Aug 2021 19:12:09 -0400 Subject: [PATCH 2/2] Omit if set to false --- .../java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java index 778df7b3882..4ad522cf3be 100644 --- a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java +++ b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java @@ -512,7 +512,8 @@ public class SearchParameterMap implements Serializable { } //Contained mode - if (getSearchContainedMode() != null) { + //For some reason, instead of null here, we default to false. That said, ommitting it is identical to setting it to false. + if (getSearchContainedMode() != SearchContainedModeEnum.FALSE) { addUrlParamSeparator(b); b.append(Constants.PARAM_CONTAINED); b.append("=");