3242 search content param removed not readded (#3244)

* 3242 initial changes and nonsense tests

* 3242 added changelog

* 3242 cleanup

* cleanup

* 3242 updated for review

* cleanup

* 3242 review fixes

* review fixes

* fix changelog file

* review fixes

Co-authored-by: leif stawnyczy <leifstawnyczy@leifs-MacBook-Pro.local>

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_7_0/3242-fix-bug-that-mutates-searchparam-map.yaml

@@ -0,0 +1,5 @@
+---
+issue: 3242
+type: fix
+title: "Fixed bug that caused queries that checked Elasticsearch when setting `_total=accurate` searches to return all
+      values when using _content or _text parameters"

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/FulltextSearchSvcImpl.java

@@ -100,15 +100,12 @@ public class FulltextSearchSvcImpl implements IFulltextSearchSvc {
 		return requiresHibernateSearchAccess;
 	}
 
-
-
-
 	private List<ResourcePersistentId> doSearch(String theResourceType, SearchParameterMap theParams, ResourcePersistentId theReferencingPid) {
 		// keep this in sync with supportsSomeOf();
 		SearchSession session = Search.session(myEntityManager);
 
 		List<Long> longPids = session.search(ResourceTable.class)
-			//Selects are replacements for projection and convert more cleanly than the old implementation.
+			// Selects are replacements for projection and convert more cleanly than the old implementation.
 			.select(
 				f -> f.field("myId", Long.class)
 			)
@@ -118,11 +115,20 @@ public class FulltextSearchSvcImpl implements IFulltextSearchSvc {
 
 					/*
 					 * Handle _content parameter (resource body content)
+					 *
+					 * Posterity:
+					 * We do not want the HAPI-FHIR dao's to process the
+					 * _content parameter, so we remove it from the map here
 					 */
 					List<List<IQueryParameterType>> contentAndTerms = theParams.remove(Constants.PARAM_CONTENT);
 					builder.addStringTextSearch(Constants.PARAM_CONTENT, contentAndTerms);
+
 					/*
 					 * Handle _text parameter (resource narrative content)
+					 *
+					 * Positerity:
+					 * We do not want the HAPI-FHIR dao's to process the
+					 * _text parameter, so we remove it from the map here
 					 */
 					List<List<IQueryParameterType>> textAndTerms = theParams.remove(Constants.PARAM_TEXT);
 					builder.addStringTextSearch(Constants.PARAM_TEXT, textAndTerms);

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java

@@ -89,14 +89,12 @@ 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.TransactionSynchronizationManager;
 import org.springframework.transaction.support.TransactionTemplate;
 
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import javax.annotation.PostConstruct;
 import javax.persistence.EntityManager;
-import javax.validation.constraints.NotNull;
 import java.io.IOException;
 import java.time.Instant;
 import java.time.temporal.ChronoUnit;
@@ -1059,7 +1057,6 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc {
 		 * search, and starts it.
 		 */
 		private void doSearch() {
-
 			/*
 			 * If the user has explicitly requested a _count, perform a
 			 *
@@ -1072,7 +1069,17 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc {
 			if (wantCount) {
 				ourLog.trace("Performing count");
 				ISearchBuilder sb = newSearchBuilder();
-				Iterator<Long> countIterator = sb.createCountQuery(myParams, mySearch.getUuid(), myRequest, myRequestPartitionId);
+
+				/*
+				 * createCountQuery
+				 * NB: (see createQuery below)
+				 * Because FulltextSearchSvcImpl will (internally)
+				 * mutate the myParams (searchmap),
+				 * (specifically removing the _content and _text filters)
+				 * we will have to clone those parameters here so that
+				 * the "correct" params are used in createQuery below
+				 */
+				Iterator<Long> countIterator = sb.createCountQuery(myParams.clone(), mySearch.getUuid(), myRequest, myRequestPartitionId);
 				Long count = countIterator.hasNext() ? countIterator.next() : 0L;
 				ourLog.trace("Got count {}", count);
 
@@ -1146,7 +1153,18 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc {
 			}
 
 			/*
+			 * createQuery
 			 * Construct the SQL query we'll be sending to the database
+			 *
+			 * NB: (See createCountQuery above)
+			 * We will pass the original myParams here (not a copy)
+			 * because we actually _want_ the mutation of the myParams to happen.
+			 * Specifically because SearchBuilder itself will _expect_
+			 * not to have these parameters when dumping back
+			 * to our DB.
+			 *
+			 * This is an odd implementation behaviour, but the change
+			 * for this will require a lot more handling at higher levels
 			 */
 			try (IResultIterator resultIterator = sb.createQuery(myParams, mySearchRuntimeDetails, myRequest, myRequestPartitionId)) {
 				assert (resultIterator != null);

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirSearchDaoR4Test.java

@@ -3,12 +3,15 @@ package ca.uhn.fhir.jpa.dao.r4;
 import ca.uhn.fhir.jpa.dao.IFulltextSearchSvc;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.rest.api.Constants;
+import ca.uhn.fhir.rest.api.SearchTotalModeEnum;
+import ca.uhn.fhir.rest.api.server.IBundleProvider;
 import ca.uhn.fhir.rest.api.server.storage.ResourcePersistentId;
 import ca.uhn.fhir.rest.param.StringAndListParam;
 import ca.uhn.fhir.rest.param.StringOrListParam;
 import ca.uhn.fhir.rest.param.StringParam;
 import org.hl7.fhir.r4.model.Organization;
 import org.hl7.fhir.r4.model.Patient;
+import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.dao.InvalidDataAccessApiUsageException;
@@ -37,6 +40,62 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test {
 		assert !TransactionSynchronizationManager.isActualTransactionActive();
 	}
 
+	@Test
+	public void testSearchReturnsExpectedPatientsWhenContentTypeUsed() {
+		// setup
+		String content = "yui";
+
+		Long id1;
+		{
+			Patient patient = new Patient();
+			patient.addName().addGiven(content).setFamily("hirasawa");
+			id1 = myPatientDao.create(patient, mySrd).getId().toUnqualifiedVersionless().getIdPartAsLong();
+		}
+		Long id2;
+		{
+			Patient patient = new Patient();
+			patient.addName().addGiven("mio").setFamily("akiyama");
+			id2 = myPatientDao.create(patient, mySrd).getId().toUnqualifiedVersionless().getIdPartAsLong();
+		}
+
+		SearchParameterMap params = new SearchParameterMap();
+		params.add("_content", new StringParam(content));
+
+		// test
+		List<ResourcePersistentId> ids = mySearchDao.search("Patient",
+			params);
+
+		// verify results
+		Assertions.assertEquals(1, ids.size());
+		Assertions.assertEquals(id1, ids.get(0).getIdAsLong());
+	}
+
+	@Test
+	public void testSearchesWithAccurateCountReturnOnlyExpectedResults() {
+		// create 2 patients
+		Patient patient = new Patient();
+		patient.addName().setFamily("hirasawa");
+		myPatientDao.create(patient);
+
+		Patient patient2 = new Patient();
+		patient2.addName().setFamily("akiyama");
+		myPatientDao.create(patient2);
+
+		// construct searchmap with Accurate search mode
+		SearchParameterMap map = new SearchParameterMap();
+		map.add(Constants.PARAM_CONTENT, new StringParam("hirasawa"));
+		map.setSearchTotalMode(SearchTotalModeEnum.ACCURATE);
+
+		// test
+		IBundleProvider ret = myPatientDao.search(map);
+
+		// only one should be returned
+		Assertions.assertEquals(1, ret.size());
+		Patient retPatient = (Patient) ret.getAllResources().get(0);
+		Assertions.assertEquals(patient.getName().get(0).getFamily(),
+			retPatient.getName().get(0).getFamily());
+	}
+
 	@Test
 	public void testContentSearch() {
 		Long id1;
@@ -64,7 +123,7 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test {
 			id3 = myOrganizationDao.create(org, mySrd).getId().toUnqualifiedVersionless().getIdPartAsLong();
 		}
 
-		SearchParameterMap map = new SearchParameterMap();
+		SearchParameterMap map;
 		String resourceName = "Patient";
 
 		// One term
@@ -72,6 +131,7 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test {
 			StringAndListParam content = new StringAndListParam();
 			content.addAnd(new StringOrListParam().addOr(new StringParam("AAAS")));
 
+			map = new SearchParameterMap();
 			map.add(Constants.PARAM_CONTENT, content);
 			List<ResourcePersistentId> found = mySearchDao.search(resourceName, map);
 			assertThat(ResourcePersistentId.toLongList(found), containsInAnyOrder(id1));
@@ -81,6 +141,8 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test {
 			StringAndListParam content = new StringAndListParam();
 			content.addAnd(new StringOrListParam().addOr(new StringParam("AAAS")).addOr(new StringParam("AAAB")));
 
+			map = new SearchParameterMap();
+			map.add(Constants.PARAM_CONTENT, content);
 			map.add(Constants.PARAM_CONTENT, content);
 			List<ResourcePersistentId> found = mySearchDao.search(resourceName, map);
 			assertThat(ResourcePersistentId.toLongList(found), containsInAnyOrder(id1, id2));
@@ -91,6 +153,7 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test {
 			content.addAnd(new StringOrListParam().addOr(new StringParam("AAAS")));
 			content.addAnd(new StringOrListParam().addOr(new StringParam("CCC")));
 
+			map = new SearchParameterMap();
 			map.add(Constants.PARAM_CONTENT, content);
 			List<ResourcePersistentId> found = mySearchDao.search(resourceName, map);
 			assertThat(ResourcePersistentId.toLongList(found), containsInAnyOrder(id1));
@@ -101,6 +164,7 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test {
 			content.addAnd(new StringOrListParam().addOr(new StringParam("AAAB")).addOr(new StringParam("AAAS")));
 			content.addAnd(new StringOrListParam().addOr(new StringParam("CCC")));
 
+			map = new SearchParameterMap();
 			map.add(Constants.PARAM_CONTENT, content);
 			List<ResourcePersistentId> found = mySearchDao.search(resourceName, map);
 			assertThat(ResourcePersistentId.toLongList(found), containsInAnyOrder(id1, id2));
@@ -110,6 +174,7 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test {
 			StringAndListParam content = new StringAndListParam();
 			content.addAnd(new StringOrListParam().addOr(new StringParam("CCC")).addOr(new StringParam("DDD")));
 
+			map = new SearchParameterMap();
 			map.add(Constants.PARAM_CONTENT, content);
 			List<ResourcePersistentId> found = mySearchDao.search(null, map);
 			assertThat(ResourcePersistentId.toLongList(found), containsInAnyOrder(id1, id2, id3));
@@ -137,7 +202,7 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test {
 			myPatientDao.create(patient, mySrd).getId().toUnqualifiedVersionless().getIdPartAsLong();
 		}
 
-		SearchParameterMap map = new SearchParameterMap();
+		SearchParameterMap map;
 		String resourceName = "Patient";
 
 		// One term
@@ -145,6 +210,7 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test {
 			StringAndListParam content = new StringAndListParam();
 			content.addAnd(new StringOrListParam().addOr(new StringParam("AAAS")));
 
+			map = new SearchParameterMap();
 			map.add(Constants.PARAM_TEXT, content);
 			List<ResourcePersistentId> found = mySearchDao.search(resourceName, map);
 			assertThat(ResourcePersistentId.toLongList(found), containsInAnyOrder(id1));
@@ -154,6 +220,7 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test {
 			StringAndListParam content = new StringAndListParam();
 			content.addAnd(new StringOrListParam().addOr(new StringParam("AAAS")).addOr(new StringParam("AAAB")));
 
+			map = new SearchParameterMap();
 			map.add(Constants.PARAM_TEXT, content);
 			List<ResourcePersistentId> found = mySearchDao.search(resourceName, map);
 			assertThat(ResourcePersistentId.toLongList(found), containsInAnyOrder(id1, id2));
@@ -164,6 +231,7 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test {
 			content.addAnd(new StringOrListParam().addOr(new StringParam("AAAS")));
 			content.addAnd(new StringOrListParam().addOr(new StringParam("CCC")));
 
+			map = new SearchParameterMap();
 			map.add(Constants.PARAM_TEXT, content);
 			List<ResourcePersistentId> found = mySearchDao.search(resourceName, map);
 			assertThat(ResourcePersistentId.toLongList(found), containsInAnyOrder(id1));
@@ -174,6 +242,7 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test {
 			content.addAnd(new StringOrListParam().addOr(new StringParam("AAAB")).addOr(new StringParam("AAAS")));
 			content.addAnd(new StringOrListParam().addOr(new StringParam("CCC")));
 
+			map = new SearchParameterMap();
 			map.add(Constants.PARAM_TEXT, content);
 			List<ResourcePersistentId> found = mySearchDao.search(resourceName, map);
 			assertThat(ResourcePersistentId.toLongList(found), containsInAnyOrder(id1, id2));
@@ -183,6 +252,7 @@ public class FhirSearchDaoR4Test extends BaseJpaR4Test {
 			StringAndListParam content = new StringAndListParam();
 			content.addAnd(new StringOrListParam().addOr(new StringParam("div")));
 
+			map = new SearchParameterMap();
 			map.add(Constants.PARAM_TEXT, content);
 			List<ResourcePersistentId> found = mySearchDao.search(resourceName, map);
 			assertThat(ResourcePersistentId.toLongList(found), empty());

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImplTest.java

@@ -621,7 +621,6 @@ public class SearchCoordinatorSvcImplTest {
 	 */
 	@Test
 	public void testFetchAllResultsReturnsNull() {
-
 		when(myDaoRegistry.getResourceDao(anyString())).thenReturn(myCallingDao);
 		when(myCallingDao.getContext()).thenReturn(ourCtx);
 

hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java

@@ -18,6 +18,7 @@ import ca.uhn.fhir.rest.param.QuantityParam;
 import ca.uhn.fhir.rest.param.TokenParamModifier;
 import ca.uhn.fhir.util.ObjectUtil;
 import ca.uhn.fhir.util.UrlUtil;
+import com.fasterxml.jackson.annotation.JsonIgnore;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.Validate;
 import org.apache.commons.lang3.builder.ToStringBuilder;
@@ -89,6 +90,42 @@ public class SearchParameterMap implements Serializable {
 		super();
 	}
 
+	/**
+	 * Creates and returns a copy of this map
+	 */
+	@JsonIgnore
+	@Override
+	public SearchParameterMap clone() {
+		SearchParameterMap map = new SearchParameterMap();
+		map.setSummaryMode(getSummaryMode());
+		map.setSort(getSort());
+		map.setSearchTotalMode(getSearchTotalMode());
+		map.setRevIncludes(getRevIncludes());
+		map.setIncludes(getIncludes());
+		map.setEverythingMode(getEverythingMode());
+		map.setCount(getCount());
+		map.setDeleteExpunge(isDeleteExpunge());
+		map.setLastN(isLastN());
+		map.setLastNMax(getLastNMax());
+		map.setLastUpdated(getLastUpdated());
+		map.setLoadSynchronous(isLoadSynchronous());
+		map.setNearDistanceParam(getNearDistanceParam());
+		map.setLoadSynchronousUpTo(getLoadSynchronousUpTo());
+		map.setOffset(getOffset());
+		map.setSearchContainedMode(getSearchContainedMode());
+
+		for (Map.Entry<String, List<List<IQueryParameterType>>> entry : mySearchParameterMap.entrySet()) {
+			List<List<IQueryParameterType>> params = entry.getValue();
+			for (List<IQueryParameterType> p : params) {
+				for (IQueryParameterType t : p) {
+					map.add(entry.getKey(), t);
+				}
+			}
+		}
+
+		return map;
+	}
+
 	/**
 	 * Constructor
 	 */

hapi-fhir-jpaserver-searchparam/src/test/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMapTest.java

@@ -2,11 +2,21 @@ package ca.uhn.fhir.jpa.searchparam;
 
 import ca.uhn.fhir.context.FhirContext;
 import ca.uhn.fhir.model.api.IQueryParameterType;
+import ca.uhn.fhir.model.api.Include;
+import ca.uhn.fhir.rest.api.SearchContainedModeEnum;
+import ca.uhn.fhir.rest.api.SearchTotalModeEnum;
+import ca.uhn.fhir.rest.api.SortSpec;
 import ca.uhn.fhir.rest.param.DateRangeParam;
+import ca.uhn.fhir.rest.param.QuantityParam;
+import ca.uhn.fhir.rest.param.StringParam;
 import ca.uhn.fhir.rest.param.TokenOrListParam;
 import ca.uhn.fhir.rest.param.TokenParam;
+import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 
+import java.sql.Date;
+import java.time.Instant;
+import java.util.HashSet;
 import java.util.List;
 
 import static ca.uhn.fhir.rest.param.TokenParamModifier.TEXT;
@@ -97,7 +107,77 @@ class SearchParameterMapTest {
 
 		List<List<IQueryParameterType>> unqualifiedAnds = map.remove("code");
 		assertThat(unqualifiedAnds, is(nullValue()));
+	}
 
+	@Test
+	public void clone_searchParams_copiesAllFields() {
+		HashSet<Include> includes = new HashSet<>();
+		Include i = new Include("test", true);
+		includes.add(i);
 
+		SearchParameterMap orig = new SearchParameterMap();
+		orig.setOffset(1);
+		orig.setLoadSynchronousUpTo(2);
+		orig.setLoadSynchronous(true);
+		orig.setNearDistanceParam(new QuantityParam());
+		orig.setCount(3);
+		orig.setLastNMax(4);
+		orig.setLastN(true);
+		orig.setDeleteExpunge(true);
+		orig.setIncludes(includes);
+		orig.setSearchTotalMode(SearchTotalModeEnum.ACCURATE);
+		orig.setLastUpdated(new DateRangeParam());
+		orig.setSearchContainedMode(SearchContainedModeEnum.BOTH);
+		orig.setEverythingMode(SearchParameterMap.EverythingModeEnum.ENCOUNTER_INSTANCE);
+		orig.setSort(new SortSpec());
+		orig.add("something", new StringParam("value"));
+
+		// test
+		SearchParameterMap copy = orig.clone();
+
+		// verify that they are not the same
+		Assertions.assertNotEquals(orig, copy);
+
+		// ... but that they are equal
+		Assertions.assertEquals(orig.toNormalizedQueryString(null),
+			copy.toNormalizedQueryString(null));
+		Assertions.assertEquals(orig.getOffset(), copy.getOffset());
+		Assertions.assertEquals(orig.getLoadSynchronousUpTo(), copy.getLoadSynchronousUpTo());
+		Assertions.assertEquals(orig.isLoadSynchronous(), copy.isLoadSynchronous());
+		Assertions.assertEquals(orig.getNearDistanceParam(), copy.getNearDistanceParam());
+		Assertions.assertEquals(orig.getCount(), copy.getCount());
+		Assertions.assertEquals(orig.getLastNMax(), copy.getLastNMax());
+		Assertions.assertEquals(orig.isLastN(), copy.isLastN());
+		Assertions.assertEquals(orig.isDeleteExpunge(), copy.isDeleteExpunge());
+		Assertions.assertEquals(orig.getIncludes(), copy.getIncludes());
+		Assertions.assertEquals(orig.getSearchTotalMode(), copy.getSearchTotalMode());
+		Assertions.assertEquals(orig.getLastUpdated(), copy.getLastUpdated());
+		Assertions.assertEquals(orig.getSearchContainedMode(), copy.getSearchContainedMode());
+		Assertions.assertEquals(orig.getEverythingMode(), copy.getEverythingMode());
+		Assertions.assertEquals(orig.getSort(), copy.getSort());
+		Assertions.assertEquals(orig.get("something"), copy.get("something"));
+
+		// verify changing one does not change the other
+		orig.setOffset(100);
+		Assertions.assertNotEquals(orig.toNormalizedQueryString(null),
+			copy.toNormalizedQueryString(null));
+	}
+
+	@Test
+	public void clone_searchParams_haveSameSearchParamsMap() {
+		SearchParameterMap orig = new SearchParameterMap();
+		orig.add("string", new StringParam("stringvalue"));
+		orig.add("datetime", new DateRangeParam(Date.from(Instant.now()),
+			new Date(2000, 11, 11)));
+		orig.add("int", new QuantityParam(1));
+
+		// test
+		SearchParameterMap clone = orig.clone();
+
+		// verify
+		Assertions.assertEquals(orig.size(), clone.size());
+		Assertions.assertEquals(orig.get("string"), clone.get("string"));
+		Assertions.assertEquals(orig.get("datetime"), clone.get("datetime"));
+		Assertions.assertEquals(orig.get("int"), clone.get("int"));
 	}
 }