diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4844-patient-everything-with-type-filter-fix.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4844-patient-everything-with-type-filter-fix.yaml new file mode 100644 index 000000000000..5557b67e0656 --- /dev/null +++ b/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. + " diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java index 2b5600715090..a5e585cfda73 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java +++ b/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.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.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 Set loadIncludes(FhirContext theContext, EntityManager theEntityM 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 Set loadIncludes(FhirContext theContext, EntityManager theEntityM } } } else { - List paths; // Start replace @@ -1557,6 +1568,9 @@ public String getResourceName() { return myResourceName; } + /** + * IncludesIterator, used to recursively fetch resources from the provided list of PIDs + */ public class IncludesIterator extends BaseIterator implements Iterator { private final RequestDetails myRequest; @@ -1574,7 +1588,23 @@ private void fetchNext() { while (myNext == null) { if (myCurrentIterator == null) { - Set includes = Collections.singleton(new Include("*", true)); + Set includes = new HashSet<>(); + if (myParams.containsKey(Constants.PARAM_TYPE)) { + for (List 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 newPids = loadIncludes(myContext, myEntityManager, myCurrentPids, includes, false, getParams().getLastUpdated(), mySearchUuid, myRequest, null); myCurrentIterator = newPids.iterator(); } @@ -1604,6 +1634,9 @@ public JpaPid next() { } + /** + * Basic Query iterator, used to fetch the results of a query. + */ private final class QueryIterator extends BaseIterator implements IResultIterator { private final SearchRuntimeDetails mySearchRuntimeDetails; @@ -1627,8 +1660,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 +1671,6 @@ private QueryIterator(SearchRuntimeDetails theSearchRuntimeDetails, RequestDetai } private void fetchNext() { - try { if (myHaveRawSqlHooks) { CurrentThreadCaptureQueriesListener.startCapturing(); @@ -1656,6 +1688,7 @@ private void fetchNext() { } } + // assigns the results iterator initializeIteratorQuery(myOffset, myMaxResultsToFetch); if (myAlsoIncludePids == null) { @@ -1663,9 +1696,8 @@ private void fetchNext() { } } - if (myNext == null) { - + if (myNext == null) { for (Iterator myPreResultsIterator = myAlsoIncludePids.iterator(); myPreResultsIterator.hasNext(); ) { JpaPid next = myPreResultsIterator.next(); if (next != null) @@ -1724,6 +1756,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 +1784,7 @@ private void fetchNext() { mySearchRuntimeDetails.setFoundMatchesCount(myPidSet.size()); } finally { + // search finished - fire hooks if (myHaveRawSqlHooks) { SqlQueryList capturedQueries = CurrentThreadCaptureQueriesListener.getCurrentQueueAndStopCapturing(); HookParams params = new HookParams() diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryExecutor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryExecutor.java index 5a1597aec2de..951600b6d65f 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/sql/SearchQueryExecutor.java +++ b/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 diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorJpaR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorJpaR4Test.java index 85770b293a10..db4648b33a9a 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorJpaR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/AuthorizationInterceptorJpaR4Test.java @@ -1121,7 +1121,6 @@ public List buildRuleList(RequestDetails theRequestDetails) { ourLog.debug(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(resp)); } - @Test public void testOperationEverything_SomeIncludedResourcesNotAuthorized() { Patient pt1 = new Patient(); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/JpaPatientEverythingTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/JpaPatientEverythingTest.java index 527e2c5468f1..3a6285a30487 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/JpaPatientEverythingTest.java +++ b/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,6 +86,7 @@ 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 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 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 getActualEverythingResultIds(String patientId) throws IOException { Bundle bundle; HttpGet get = new HttpGet(myClient.getServerBase() + "/" + patientId + "/$everything?_format=json"); diff --git a/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/TestDaoSearch.java b/hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/TestDaoSearch.java deleted file mode 100644 index e69de29bb2d1..000000000000