Includes performance enhancement (#1702)

* Includes performance enhancement

* Add changelog

* Test fix

* Fix typo

* A few coverage cleanups

* Test fix

* Fix changelog

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/4_3_0/1702-includes-perf-fix.yaml

@@ -0,0 +1,4 @@
+---
+type: perf
+issue: 1702
+title: "Loading of _include and _revinclude values has been optimized to be slightly faster"

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

@@ -29,17 +29,29 @@ import ca.uhn.fhir.interceptor.api.Pointcut;
 import ca.uhn.fhir.jpa.dao.data.IResourceSearchViewDao;
 import ca.uhn.fhir.jpa.dao.data.IResourceTagDao;
 import ca.uhn.fhir.jpa.dao.index.IdHelperService;
-import ca.uhn.fhir.jpa.dao.predicate.*;
+import ca.uhn.fhir.jpa.dao.predicate.PredicateBuilder;
+import ca.uhn.fhir.jpa.dao.predicate.PredicateBuilderFactory;
+import ca.uhn.fhir.jpa.dao.predicate.QueryRoot;
+import ca.uhn.fhir.jpa.dao.predicate.SearchBuilderJoinEnum;
+import ca.uhn.fhir.jpa.dao.predicate.SearchBuilderJoinKey;
 import ca.uhn.fhir.jpa.entity.ResourceSearchView;
 import ca.uhn.fhir.jpa.interceptor.JpaPreResourceAccessDetails;
 import ca.uhn.fhir.jpa.model.cross.ResourcePersistentId;
-import ca.uhn.fhir.jpa.model.entity.*;
+import ca.uhn.fhir.jpa.model.entity.BaseResourceIndexedSearchParam;
+import ca.uhn.fhir.jpa.model.entity.ResourceIndexedCompositeStringUnique;
+import ca.uhn.fhir.jpa.model.entity.ResourceLink;
+import ca.uhn.fhir.jpa.model.entity.ResourceTable;
+import ca.uhn.fhir.jpa.model.entity.ResourceTag;
 import ca.uhn.fhir.jpa.model.search.SearchRuntimeDetails;
 import ca.uhn.fhir.jpa.model.search.StorageProcessingMessage;
 import ca.uhn.fhir.jpa.searchparam.JpaRuntimeSearchParam;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.jpa.searchparam.registry.ISearchParamRegistry;
-import ca.uhn.fhir.jpa.util.*;
+import ca.uhn.fhir.jpa.util.BaseIterator;
+import ca.uhn.fhir.jpa.util.CurrentThreadCaptureQueriesListener;
+import ca.uhn.fhir.jpa.util.JpaInterceptorBroadcaster;
+import ca.uhn.fhir.jpa.util.ScrollableResultsIterator;
+import ca.uhn.fhir.jpa.util.SqlQueryList;
 import ca.uhn.fhir.model.api.IQueryParameterType;
 import ca.uhn.fhir.model.api.IResource;
 import ca.uhn.fhir.model.api.Include;
@@ -80,10 +92,27 @@ import javax.persistence.EntityManager;
 import javax.persistence.PersistenceContext;
 import javax.persistence.PersistenceContextType;
 import javax.persistence.TypedQuery;
-import javax.persistence.criteria.*;
+import javax.persistence.criteria.CriteriaBuilder;
-import java.util.*;
+import javax.persistence.criteria.CriteriaQuery;
+import javax.persistence.criteria.From;
+import javax.persistence.criteria.Join;
+import javax.persistence.criteria.JoinType;
+import javax.persistence.criteria.Order;
+import javax.persistence.criteria.Predicate;
+import javax.persistence.criteria.Root;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
 
-import static org.apache.commons.lang3.StringUtils.*;
+import static org.apache.commons.lang3.StringUtils.defaultString;
+import static org.apache.commons.lang3.StringUtils.isBlank;
+import static org.apache.commons.lang3.StringUtils.isNotBlank;
 
 /**
  * The SearchBuilder is responsible for actually forming the SQL query that handles
@@ -103,8 +132,9 @@ public class SearchBuilder implements ISearchBuilder {
 	private static final List<ResourcePersistentId> EMPTY_LONG_LIST = Collections.unmodifiableList(new ArrayList<>());
 	private static final Logger ourLog = LoggerFactory.getLogger(SearchBuilder.class);
 	private static ResourcePersistentId NO_MORE = new ResourcePersistentId(-1L);
-	@Autowired
+	private final QueryRoot myQueryRoot = new QueryRoot();
-	private DaoConfig myDaoConfig;
+	private final String myResourceName;
+	private final Class<? extends IBaseResource> myResourceType;
 	@Autowired
 	protected IInterceptorBroadcaster myInterceptorBroadcaster;
 	@Autowired
@@ -112,6 +142,8 @@ public class SearchBuilder implements ISearchBuilder {
 	@PersistenceContext(type = PersistenceContextType.TRANSACTION)
 	protected EntityManager myEntityManager;
 	@Autowired
+	private DaoConfig myDaoConfig;
+	@Autowired
 	private IResourceSearchViewDao myResourceSearchViewDao;
 	@Autowired
 	private FhirContext myContext;
@@ -123,7 +155,6 @@ public class SearchBuilder implements ISearchBuilder {
 	private ISearchParamRegistry mySearchParamRegistry;
 	@Autowired
 	private PredicateBuilderFactory myPredicateBuilderFactory;
-
 	private List<ResourcePersistentId> myAlsoIncludePids;
 	private CriteriaBuilder myBuilder;
 	private IDao myCallingDao;
@@ -133,9 +164,6 @@ public class SearchBuilder implements ISearchBuilder {
 	private Integer myMaxResultsToFetch;
 	private Set<ResourcePersistentId> myPidSet;
 	private PredicateBuilder myPredicateBuilder;
-	private final QueryRoot myQueryRoot = new QueryRoot();
-	private final String myResourceName;
-	private final Class<? extends IBaseResource> myResourceType;
 
 	/**
 	 * Constructor
@@ -595,6 +623,7 @@ public class SearchBuilder implements ISearchBuilder {
 			return new HashSet<>();
 		}
 		String searchFieldName = theReverseMode ? "myTargetResourcePid" : "mySourceResourcePid";
+		String findFieldName = theReverseMode ? "mySourceResourcePid" : "myTargetResourcePid";
 
 		Collection<ResourcePersistentId> nextRoundMatches = theMatches;
 		HashSet<ResourcePersistentId> allAdded = new HashSet<>();
@@ -619,17 +648,17 @@ public class SearchBuilder implements ISearchBuilder {
 				boolean matchAll = "*".equals(nextInclude.getValue());
 				if (matchAll) {
 					String sql;
-					sql = "SELECT r FROM ResourceLink r WHERE r." + searchFieldName + " IN (:target_pids) ";
+					sql = "SELECT r." + findFieldName + " FROM ResourceLink r WHERE r." + searchFieldName + " IN (:target_pids) ";
 					List<Collection<ResourcePersistentId>> partitions = partition(nextRoundMatches, MAXIMUM_PAGE_SIZE);
 					for (Collection<ResourcePersistentId> nextPartition : partitions) {
-						TypedQuery<ResourceLink> q = theEntityManager.createQuery(sql, ResourceLink.class);
+						TypedQuery<Long> q = theEntityManager.createQuery(sql, Long.class);
 						q.setParameter("target_pids", ResourcePersistentId.toLongList(nextPartition));
-						List<ResourceLink> results = q.getResultList();
+						List<Long> results = q.getResultList();
-						for (ResourceLink resourceLink : results) {
+						for (Long resourceLink : results) {
 							if (theReverseMode) {
-								pidsToInclude.add(new ResourcePersistentId(resourceLink.getSourceResourcePid()));
+								pidsToInclude.add(new ResourcePersistentId(resourceLink));
 							} else {
-								pidsToInclude.add(new ResourcePersistentId(resourceLink.getTargetResourcePid()));
+								pidsToInclude.add(new ResourcePersistentId(resourceLink));
 							}
 						}
 					}
@@ -666,16 +695,16 @@ public class SearchBuilder implements ISearchBuilder {
 
 						boolean haveTargetTypesDefinedByParam = param.hasTargets();
 						if (targetResourceType != null) {
-							sql = "SELECT r FROM ResourceLink r WHERE r.mySourcePath = :src_path AND r." + searchFieldName + " IN (:target_pids) AND r.myTargetResourceType = :target_resource_type";
+							sql = "SELECT r." + findFieldName + " FROM ResourceLink r WHERE r.mySourcePath = :src_path AND r." + searchFieldName + " IN (:target_pids) AND r.myTargetResourceType = :target_resource_type";
 						} else if (haveTargetTypesDefinedByParam) {
-							sql = "SELECT r FROM ResourceLink r WHERE r.mySourcePath = :src_path AND r." + searchFieldName + " IN (:target_pids) AND r.myTargetResourceType in (:target_resource_types)";
+							sql = "SELECT r." + findFieldName + " FROM ResourceLink r WHERE r.mySourcePath = :src_path AND r." + searchFieldName + " IN (:target_pids) AND r.myTargetResourceType in (:target_resource_types)";
 						} else {
-							sql = "SELECT r FROM ResourceLink r WHERE r.mySourcePath = :src_path AND r." + searchFieldName + " IN (:target_pids)";
+							sql = "SELECT r." + findFieldName + " FROM ResourceLink r WHERE r.mySourcePath = :src_path AND r." + searchFieldName + " IN (:target_pids)";
 						}
 
 						List<Collection<ResourcePersistentId>> partitions = partition(nextRoundMatches, MAXIMUM_PAGE_SIZE);
 						for (Collection<ResourcePersistentId> nextPartition : partitions) {
-							TypedQuery<ResourceLink> q = theEntityManager.createQuery(sql, ResourceLink.class);
+							TypedQuery<Long> q = theEntityManager.createQuery(sql, Long.class);
 							q.setParameter("src_path", nextPath);
 							q.setParameter("target_pids", ResourcePersistentId.toLongList(nextPartition));
 							if (targetResourceType != null) {
@@ -683,18 +712,10 @@ public class SearchBuilder implements ISearchBuilder {
 							} else if (haveTargetTypesDefinedByParam) {
 								q.setParameter("target_resource_types", param.getTargets());
 							}
-							List<ResourceLink> results = q.getResultList();
+							List<Long> results = q.getResultList();
-							for (ResourceLink resourceLink : results) {
+							for (Long resourceLink : results) {
-								if (theReverseMode) {
+								if (resourceLink != null) {
-									Long pid = resourceLink.getSourceResourcePid();
+									pidsToInclude.add(new ResourcePersistentId(resourceLink));
-									if (pid != null) {
-										pidsToInclude.add(new ResourcePersistentId(pid));
-									}
-								} else {
-									Long pid = resourceLink.getTargetResourcePid();
-									if (pid != null) {
-										pidsToInclude.add(new ResourcePersistentId(pid));
-									}
 								}
 							}
 						}

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

@@ -39,11 +39,10 @@ public class SearchBuilderTest {
 
 		TypedQuery mockQuery = mock(TypedQuery.class);
 		when(mockEntityManager.createQuery(any(), any())).thenReturn(mockQuery);
-		List<ResourceLink> resultList = new ArrayList<>();
+		List<Long> resultList = new ArrayList<>();
-		ResourceLink link = new ResourceLink();
+		Long link = 1L;
 		ResourceTable target = new ResourceTable();
 		target.setId(1L);
-		link.setTargetResource(target);
 		resultList.add(link);
 		when(mockQuery.getResultList()).thenReturn(resultList);
 

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

@@ -162,6 +162,9 @@ public abstract class BaseJpaR4Test extends BaseJpaTest {
 	@Qualifier("myConditionDaoR4")
 	protected IFhirResourceDao<Condition> myConditionDao;
 	@Autowired
+	@Qualifier("myEpisodeOfCareDaoR4")
+	protected IFhirResourceDao<EpisodeOfCare> myEpisodeOfCareDao;
+	@Autowired
 	protected DaoConfig myDaoConfig;
 	@Autowired
 	protected ModelConfig myModelConfig;

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

@@ -4228,6 +4228,45 @@ public class FhirResourceDaoR4SearchNoFtTest extends BaseJpaR4Test {
 		}
 	}
 
+	@Test
+	public void testCircularReferencesDontBreakRevIncludes() {
+
+		Patient p = new Patient();
+		p.setActive(true);
+		IIdType patientId = myPatientDao.create(p).getId().toUnqualifiedVersionless();
+
+		Encounter enc = new Encounter();
+		enc.setStatus(Encounter.EncounterStatus.ARRIVED);
+		enc.getSubject().setReference(patientId.getValue());
+		IIdType encId = myEncounterDao.create(enc).getId().toUnqualifiedVersionless();
+
+		Condition cond = new Condition();
+		cond.addIdentifier().setSystem("http://foo").setValue("123");
+		IIdType conditionId = myConditionDao.create(cond).getId().toUnqualifiedVersionless();
+
+		EpisodeOfCare ep = new EpisodeOfCare();
+		ep.setStatus(EpisodeOfCare.EpisodeOfCareStatus.ACTIVE);
+		IIdType epId = myEpisodeOfCareDao.create(ep).getId().toUnqualifiedVersionless();
+
+		enc.getEpisodeOfCareFirstRep().setReference(ep.getId());
+		myEncounterDao.update(enc);
+		cond.getEncounter().setReference(enc.getId());
+		myConditionDao.update(cond);
+		ep.getDiagnosisFirstRep().getCondition().setReference(cond.getId());
+		myEpisodeOfCareDao.update(ep);
+
+		// Search time
+		SearchParameterMap params = new SearchParameterMap();
+		params.setLoadSynchronous(true);
+		params.addRevInclude(new Include("*").setRecurse(true));
+		IBundleProvider results = myPatientDao.search(params);
+		List<String> values = toUnqualifiedVersionlessIdValues(results);
+		assertThat(values.toString(), values, containsInAnyOrder(patientId.getValue(), encId.getValue(), conditionId.getValue(), epId.getValue()));
+
+	}
+
+
+
 	private String toStringMultiline(List<?> theResults) {
 		StringBuilder b = new StringBuilder();
 		for (Object next : theResults) {

hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/ResourceLink.java

@@ -138,10 +138,6 @@ public class ResourceLink extends BaseResourceIndex {
 		mySourceResourceType = theSourceResource.getResourceType();
 	}
 
-	public Long getSourceResourcePid() {
-		return mySourceResourcePid;
-	}
-
 	public ResourceTable getTargetResource() {
 		return myTargetResource;
 	}
@@ -157,10 +153,6 @@ public class ResourceLink extends BaseResourceIndex {
 		return myTargetResourcePid;
 	}
 
-	public String getTargetResourceUrl() {
-		return myTargetResourceUrl;
-	}
-
 	public void setTargetResourceUrl(IIdType theTargetResourceUrl) {
 		Validate.isTrue(theTargetResourceUrl.hasBaseUrl());
 		Validate.isTrue(theTargetResourceUrl.hasResourceType());

hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/ReadMethodBinding.java

@@ -160,7 +160,7 @@ public class ReadMethodBinding extends BaseResourceReturningMethodBinding {
 		IBundleProvider retVal = toResourceList(response);
 
 
-		if (retVal.size() == 1) {
+		if (Integer.valueOf(1).equals(retVal.size())) {
 			List<IBaseResource> responseResources = retVal.getResources(0, 1);
 			IBaseResource responseResource = responseResources.get(0);
 

hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/method/ReadMethodBindingTest.java

@@ -59,6 +59,11 @@ public class ReadMethodBindingTest {
 		assertTrue(binding.incomingServerRequestMatchesMethod(myRequestDetails));
 
 		// VRead
+		when(myRequestDetails.getId()).thenReturn(new IdDt("Patient/123/_history/123"));
+		assertFalse(binding.incomingServerRequestMatchesMethod(myRequestDetails));
+
+		// Type history
+		when(myRequestDetails.getId()).thenReturn(new IdDt("Patient/123"));
 		when(myRequestDetails.getResourceName()).thenReturn("Patient");
 		when(myRequestDetails.getOperation()).thenReturn("_history");
 		assertFalse(binding.incomingServerRequestMatchesMethod(myRequestDetails));