fixing patient everything operator (#4845)

* fixing patient everything operator

* review fix

---------

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

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4844-patient-everything-with-type-filter-fix.yaml

@@ -0,0 +1,11 @@
+---
+type: fix
+issue: 4844
+title: "/Patient/{patientid}/$everything?_type={resource types}
+   would omit resources that were not directly related to the Patient
+   resource (even if those resources were specified in the _type list).
+   This was in conflict with /Patient/{patientid}/$everything operation,
+   which did return said resources.
+   This has been fixed so both return all related resources, even if
+   those resources are not directly related to the Patient resource.
+   "

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

@@ -79,6 +79,7 @@ import ca.uhn.fhir.rest.api.SortSpec;
 import ca.uhn.fhir.rest.api.server.IPreResourceAccessDetails;
 import ca.uhn.fhir.rest.api.server.RequestDetails;
 import ca.uhn.fhir.rest.param.DateRangeParam;
+import ca.uhn.fhir.rest.param.ParameterUtil;
 import ca.uhn.fhir.rest.param.ReferenceParam;
 import ca.uhn.fhir.rest.param.StringParam;
 import ca.uhn.fhir.rest.param.TokenParam;
@@ -126,7 +127,6 @@ import java.util.Set;
 import java.util.stream.Collectors;
 
 import static ca.uhn.fhir.jpa.search.builder.QueryStack.LOCATION_POSITION;
-import static org.apache.commons.lang3.StringUtils.countMatches;
 import static org.apache.commons.lang3.StringUtils.defaultString;
 import static org.apache.commons.lang3.StringUtils.isBlank;
 import static org.apache.commons.lang3.StringUtils.isNotBlank;
@@ -1166,16 +1166,28 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 					sqlBuilder.append(" FROM ResourceLink r WHERE ");
 
 					sqlBuilder.append("r.");
-					sqlBuilder.append(searchPidFieldName);
+					sqlBuilder.append(searchPidFieldName); // (rev mode) target_resource_id | source_resource_id
 					sqlBuilder.append(" IN (:target_pids)");
 
-					// Technically if the request is a qualified star (e.g. _include=Observation:*) we
-					// should always be checking the source resource type on the resource link. We don't
-					// actually index that column though by default, so in order to try and be efficient
-					// we don't actually include it for includes (but we do for revincludes). This is
-					// because for an include it doesn't really make sense to include a different
-					// resource type than the one you are searching on.
-					if (wantResourceType != null && theReverseMode) {
+					/*
+					 * We need to set the resource type in 2 cases only:
+					 * 1) we are in $everything mode
+					 * 		(where we only want to fetch specific resource types, regardless of what is
+					 * 		available to fetch)
+					 * 2) we are doing revincludes
+					 *
+					 *	Technically if the request is a qualified star (e.g. _include=Observation:*) we
+					 * should always be checking the source resource type on the resource link. We don't
+					 * actually index that column though by default, so in order to try and be efficient
+					 * we don't actually include it for includes (but we do for revincludes). This is
+					 * because for an include, it doesn't really make sense to include a different
+					 * resource type than the one you are searching on.
+					 */
+					if (wantResourceType != null
+						&& (theReverseMode || (myParams != null && myParams.getEverythingMode() != null))) {
+						// because mySourceResourceType is not part of the HFJ_RES_LINK
+						// index, this might not be the most optimal performance.
+						// but it is for an $everything operation (and maybe we should update the index)
 						sqlBuilder.append(" AND r.mySourceResourceType = :want_resource_type");
 					} else {
 						wantResourceType = null;
@@ -1220,7 +1232,6 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 						}
 					}
 				} else {
-
 					List<String> paths;
 
 					// Start replace
@@ -1557,6 +1568,9 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 		return myResourceName;
 	}
 
+	/**
+	 * IncludesIterator, used to recursively fetch resources from the provided list of PIDs
+	 */
 	public class IncludesIterator extends BaseIterator<JpaPid> implements Iterator<JpaPid> {
 
 		private final RequestDetails myRequest;
@@ -1574,7 +1588,23 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 			while (myNext == null) {
 
 				if (myCurrentIterator == null) {
-					Set<Include> includes = Collections.singleton(new Include("*", true));
+					Set<Include> includes = new HashSet<>();
+					if (myParams.containsKey(Constants.PARAM_TYPE)) {
+						for (List<IQueryParameterType> typeList : myParams.get(Constants.PARAM_TYPE)) {
+							for (IQueryParameterType type : typeList) {
+								String queryString = ParameterUtil.unescape(type.getValueAsQueryToken(myContext));
+								for (String resourceType : queryString.split(",")) {
+									String rt = resourceType.trim();
+									if (isNotBlank(rt)) {
+										includes.add(new Include(rt + ":*", true));
+									}
+								}
+							}
+						}
+					}
+					if (includes.isEmpty()) {
+						includes.add(new Include("*", true));
+					}
 					Set<JpaPid> newPids = loadIncludes(myContext, myEntityManager, myCurrentPids, includes, false, getParams().getLastUpdated(), mySearchUuid, myRequest, null);
 					myCurrentIterator = newPids.iterator();
 				}
@@ -1604,6 +1634,9 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 
 	}
 
+	/**
+	 * Basic Query iterator, used to fetch the results of a query.
+	 */
 	private final class QueryIterator extends BaseIterator<JpaPid> implements IResultIterator<JpaPid> {
 
 		private final SearchRuntimeDetails mySearchRuntimeDetails;
@@ -1627,8 +1660,8 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 			myOffset = myParams.getOffset();
 			myRequest = theRequest;
 
-			// Includes are processed inline for $everything query when we don't have a '_type' specified
-			if (myParams.getEverythingMode() != null && !myParams.containsKey(Constants.PARAM_TYPE)) {
+			// everything requires fetching recursively all related resources
+			if (myParams.getEverythingMode() != null) {
 				myFetchIncludesForEverythingOperation = true;
 			}
 
@@ -1638,7 +1671,6 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 		}
 
 		private void fetchNext() {
-
 			try {
 				if (myHaveRawSqlHooks) {
 					CurrentThreadCaptureQueriesListener.startCapturing();
@@ -1656,6 +1688,7 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 						}
 					}
 
+					// assigns the results iterator
 					initializeIteratorQuery(myOffset, myMaxResultsToFetch);
 
 					if (myAlsoIncludePids == null) {
@@ -1663,9 +1696,8 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 					}
 				}
 
+
 				if (myNext == null) {
-
-
 					for (Iterator<JpaPid> myPreResultsIterator = myAlsoIncludePids.iterator(); myPreResultsIterator.hasNext(); ) {
 						JpaPid next = myPreResultsIterator.next();
 						if (next != null)
@@ -1724,6 +1756,8 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 					}
 
 					if (myNext == null) {
+						// if we got here, it means the current PjaPid has already been processed
+						// and we will decide (here) if we need to fetch related resources recursively
 						if (myFetchIncludesForEverythingOperation) {
 							myIncludesIterator = new IncludesIterator(myPidSet, myRequest);
 							myFetchIncludesForEverythingOperation = false;
@@ -1750,6 +1784,7 @@ public class SearchBuilder implements ISearchBuilder<JpaPid> {
 				mySearchRuntimeDetails.setFoundMatchesCount(myPidSet.size());
 
 			} finally {
+				// search finished - fire hooks
 				if (myHaveRawSqlHooks) {
 					SqlQueryList capturedQueries = CurrentThreadCaptureQueriesListener.getCurrentQueueAndStopCapturing();
 					HookParams params = new HookParams()

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryExecutor.java

@@ -40,6 +40,7 @@ import javax.persistence.PersistenceContextType;
 import javax.persistence.Query;
 import java.sql.Connection;
 import java.sql.PreparedStatement;
+import java.util.Arrays;
 
 public class SearchQueryExecutor implements ISearchQueryExecutor {
 
@@ -119,7 +120,7 @@ public class SearchQueryExecutor implements ISearchQueryExecutor {
 						hibernateQuery.setParameter(i, args[i - 1]);
 					}
 
-					ourLog.trace("About to execute SQL: {}", sql);
+					ourLog.trace("About to execute SQL: {}. Parameters: {}", sql, Arrays.toString(args));
 
 					/*
 					 * These settings help to ensure that we use a search cursor

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorJpaR4Test.java

@@ -1121,7 +1121,6 @@ public class AuthorizationInterceptorJpaR4Test extends BaseResourceProviderR4Tes
 		ourLog.debug(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(resp));
 	}
 
-
 	@Test
 	public void testOperationEverything_SomeIncludedResourcesNotAuthorized() {
 		Patient pt1 = new Patient();

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/JpaPatientEverythingTest.java

@@ -1,12 +1,15 @@
 package ca.uhn.fhir.jpa.provider.r4;
 
+import ca.uhn.fhir.jpa.model.util.JpaConstants;
 import ca.uhn.fhir.jpa.provider.BaseResourceProviderR4Test;
+import ca.uhn.fhir.parser.IParser;
 import ca.uhn.fhir.rest.api.Constants;
 import ca.uhn.fhir.rest.api.EncodingEnum;
 import com.google.common.base.Charsets;
 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.IIdType;
 import org.hl7.fhir.r4.model.Account;
 import org.hl7.fhir.r4.model.AdverseEvent;
 import org.hl7.fhir.r4.model.AllergyIntolerance;
@@ -45,6 +48,7 @@ import org.hl7.fhir.r4.model.FamilyMemberHistory;
 import org.hl7.fhir.r4.model.Flag;
 import org.hl7.fhir.r4.model.Goal;
 import org.hl7.fhir.r4.model.Group;
+import org.hl7.fhir.r4.model.IdType;
 import org.hl7.fhir.r4.model.ImagingStudy;
 import org.hl7.fhir.r4.model.Immunization;
 import org.hl7.fhir.r4.model.ImmunizationEvaluation;
@@ -62,6 +66,7 @@ import org.hl7.fhir.r4.model.MolecularSequence;
 import org.hl7.fhir.r4.model.NutritionOrder;
 import org.hl7.fhir.r4.model.Observation;
 import org.hl7.fhir.r4.model.Organization;
+import org.hl7.fhir.r4.model.Parameters;
 import org.hl7.fhir.r4.model.Patient;
 import org.hl7.fhir.r4.model.Person;
 import org.hl7.fhir.r4.model.Practitioner;
@@ -81,6 +86,7 @@ import org.hl7.fhir.r4.model.VisionPrescription;
 import org.junit.jupiter.api.Test;
 
 import java.io.IOException;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 import java.util.TreeSet;
@@ -88,7 +94,9 @@ import java.util.TreeSet;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.hasItem;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class JpaPatientEverythingTest extends BaseResourceProviderR4Test {
 
@@ -1626,6 +1634,168 @@ public class JpaPatientEverythingTest extends BaseResourceProviderR4Test {
 		assertThat(actual, hasItem(medicationAdministrationId));
 	}
 
+	@Test
+	public void everything_typeFilterWithRecursivelyRelatedResources_shouldReturnSameAsNonTypeFilteredEverything() {
+		String testBundle;
+		{
+			testBundle = """
+							{
+				       "resourceType": "Bundle",
+				       "type": "transaction",
+				       "entry": [
+				           {
+				               "fullUrl": "https://interop.providence.org:8000/Patient/385235",
+				               "resource": {
+				                   "resourceType": "Patient",
+				                   "id": "385235",
+				                   "active": true,
+				                   "name": [
+				                       {
+				                           "family": "TESTING",
+				                           "given": [
+				                               "TESTER",
+				                               "T"
+				                           ]
+				                       }
+				                   ],
+				                   "gender": "female"
+				               },
+				               "request": {
+				                   "method": "POST"
+				               }
+				           },
+				           {
+				               "fullUrl": "https://interop.providence.org:8000/Encounter/385236",
+				               "resource": {
+				                   "resourceType": "Encounter",
+				                   "id": "385236",
+				                   "subject": {
+				                       "reference": "Patient/385235"
+				                   }
+				               },
+				               "request": {
+				                   "method": "POST"
+				               }
+				           },
+				           {
+				               "fullUrl": "https://interop.providence.org:8000/Observation/385237",
+				               "resource": {
+				                   "resourceType": "Observation",
+				                   "id": "385237",
+				                   "subject": {
+				                       "reference": "Patient/385235"
+				                   },
+				                   "encounter": {
+				                       "reference": "Encounter/385236"
+				                   },
+				                   "performer": [
+				                       {
+				                           "reference": "Practitioner/79070"
+				                       },
+				                       {
+				                           "reference": "Practitioner/8454"
+				                       }
+				                   ],
+				                   "valueQuantity": {
+				                       "value": 100.9,
+				                       "unit": "%",
+				                       "system": "http://unitsofmeasure.org",
+				                       "code": "%"
+				                   }
+				               },
+				               "request": {
+				                   "method": "POST"
+				               }
+				           },
+				           {
+				               "fullUrl": "https://interop.providence.org:8000/Practitioner/8454",
+				               "resource": {
+				                   "resourceType": "Practitioner",
+				                   "id": "8454"
+				               },
+				               "request": {
+				                   "method": "POST"
+				               }
+				           },
+				           {
+				               "fullUrl": "https://interop.providence.org:8000/Practitioner/79070",
+				               "resource": {
+				                   "resourceType": "Practitioner",
+				                   "id": "79070",
+				                   "active": true
+				               },
+				               "request": {
+				                   "method": "POST"
+				               }
+				           }
+				       ]
+				   }
+				""";
+		}
+
+		IParser parser = myFhirContext.newJsonParser();
+		Bundle inputBundle = parser.parseResource(Bundle.class, testBundle);
+
+		int resourceCount = inputBundle.getEntry().size();
+		HashSet<String> resourceTypes = new HashSet<>();
+		for (Bundle.BundleEntryComponent entry : inputBundle.getEntry()) {
+			resourceTypes.add(entry.getResource().getResourceType().name());
+		}
+		// there are 2 practitioners in the bundle
+		assertEquals(4, resourceTypes.size());
+
+		// pre-seed the resources
+		Bundle responseBundle = myClient.transaction()
+			.withBundle(inputBundle)
+			.execute();
+		assertNotNull(responseBundle);
+		assertEquals(resourceCount, responseBundle.getEntry().size());
+
+		IIdType patientId = null;
+		for (Bundle.BundleEntryComponent entry : responseBundle.getEntry()) {
+			assertEquals("201 Created", entry.getResponse().getStatus());
+			if (entry.getResponse().getLocation().contains("Patient")) {
+				patientId = new IdType(entry.getResponse().getLocation());
+			}
+		}
+		assertNotNull(patientId);
+		assertNotNull(patientId.getIdPart());
+
+		ourLog.debug("------ EVERYTHING");
+		// test without types filter
+		{
+			Bundle response = myClient.operation()
+				.onInstance(String.format("Patient/%s", patientId.getIdPart()))
+				.named(JpaConstants.OPERATION_EVERYTHING)
+				.withNoParameters(Parameters.class)
+				.returnResourceType(Bundle.class)
+				.execute();
+			assertNotNull(response);
+			assertEquals(resourceCount, response.getEntry().size());
+			for (Bundle.BundleEntryComponent entry : response.getEntry()) {
+				assertTrue(resourceTypes.contains(entry.getResource().getResourceType().name()));
+			}
+		}
+
+		ourLog.debug("------- EVERYTHING WITH TYPES");
+		// test with types filter
+		{
+			Parameters parameters = new Parameters();
+			parameters.addParameter(Constants.PARAM_TYPE, String.join(",", resourceTypes));
+			Bundle response = myClient.operation()
+				.onInstance(String.format("Patient/%s", patientId.getIdPart()))
+				.named(JpaConstants.OPERATION_EVERYTHING)
+				.withParameters(parameters)
+				.returnResourceType(Bundle.class)
+				.execute();
+			assertNotNull(response);
+			assertEquals(resourceCount, response.getEntry().size());
+			for (Bundle.BundleEntryComponent entry : response.getEntry()) {
+				assertTrue(resourceTypes.contains(entry.getResource().getResourceType().name()));
+			}
+		}
+	}
+
 	private Set<String> getActualEverythingResultIds(String patientId) throws IOException {
 		Bundle bundle;
 		HttpGet get = new HttpGet(myClient.getServerBase() + "/" + patientId + "/$everything?_format=json");