fixing patient everything operator

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/{patienid}/$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

@@ -126,7 +126,6 @@
 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;
@@ -1557,6 +1556,9 @@ public String getResourceName() {
 		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;
@@ -1604,6 +1606,9 @@ public JpaPid next() {
 
 	}
 
+	/**
+	 * 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 +1632,8 @@ private QueryIterator(SearchRuntimeDetails theSearchRuntimeDetails, RequestDetai
 			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 +1643,6 @@ private QueryIterator(SearchRuntimeDetails theSearchRuntimeDetails, RequestDetai
 		}
 
 		private void fetchNext() {
-
 			try {
 				if (myHaveRawSqlHooks) {
 					CurrentThreadCaptureQueriesListener.startCapturing();
@@ -1656,16 +1660,16 @@ private void fetchNext() {
 						}
 					}
 
+					// assigns the results iterator
 					initializeIteratorQuery(myOffset, myMaxResultsToFetch);
 
 					if (myAlsoIncludePids == null) {
 						myAlsoIncludePids = new ArrayList<>();
 					}
 				}
 
-				if (myNext == null) {
-
 
+				if (myNext == null) {
 					for (Iterator<JpaPid> myPreResultsIterator = myAlsoIncludePids.iterator(); myPreResultsIterator.hasNext(); ) {
 						JpaPid next = myPreResultsIterator.next();
 						if (next != null)
@@ -1724,6 +1728,8 @@ private void fetchNext() {
 					}
 
 					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 +1756,7 @@ private void fetchNext() {
 				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.Query;
 import java.sql.Connection;
 import java.sql.PreparedStatement;
+import java.util.Arrays;
 
 public class SearchQueryExecutor implements ISearchQueryExecutor {
 
@@ -119,7 +120,7 @@ private void fetchNext() {
 						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 List<IAuthRule> buildRuleList(RequestDetails theRequestDetails) {
 		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.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.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,14 +86,17 @@
 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;
 
 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 void patientEverything_shouldReturnMedication_whenMedicationAdministratio
 		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");