3987 group bulk export does not apply typefilter searches to members of the group (#3988)

* Create failing test

* Implemented the fix, fixed existing unit tests

* added changelog

* added test case for no filter, exclude 1 patient

Co-authored-by: Steven Li <steven@smilecdr.com>

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/3987-group-bulk-export-does-not-apply-typefilter-searches-to-members-of-the-group.yaml

@@ -0,0 +1,6 @@
+---
+type: fix
+issue: 3987
+jira: SMILE-4921
+title: "Previously, bulk export for Group type with _typeFilter did not apply the filter if it was for the patients, and returned all members of the group.
+This has now been fixed, and the filter will apply."

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/bulk/export/svc/JpaBulkExportProcessor.java

@@ -61,6 +61,7 @@ import org.hl7.fhir.instance.model.api.IBaseReference;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.instance.model.api.IIdType;
 import org.hl7.fhir.instance.model.api.IPrimitiveType;
+import org.hl7.fhir.r4.model.Patient;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -76,6 +77,8 @@ import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Collectors;
 
+import static ca.uhn.fhir.rest.api.Constants.PARAM_HAS;
+
 public class JpaBulkExportProcessor implements IBulkExportProcessor {
 	private static final Logger ourLog = LoggerFactory.getLogger(JpaBulkExportProcessor.class);
 
@@ -150,8 +153,7 @@ public class JpaBulkExportProcessor implements IBulkExportProcessor {
 					pids.add(resultIterator.next());
 				}
 			}
-		}
+		} else if (theParams.getExportStyle() == BulkDataExportOptions.ExportStyle.GROUP) {
-		else if (theParams.getExportStyle() == BulkDataExportOptions.ExportStyle.GROUP) {
 			// Group
 			if (resourceType.equalsIgnoreCase("Patient")) {
 				return getExpandedPatientIterator(theParams);
@@ -168,8 +170,7 @@ public class JpaBulkExportProcessor implements IBulkExportProcessor {
 			queryChunker.chunk(new ArrayList<>(expandedMemberResourceIds), QUERY_CHUNK_SIZE, (idChunk) -> {
 				queryResourceTypeWithReferencesToPatients(pids, idChunk, theParams, def);
 			});
-		}
+		} else {
-		else {
 			// System
 			List<SearchParameterMap> maps = myBulkExportHelperSvc.createSearchParameterMapsForResourceType(def, theParams);
 			ISearchBuilder searchBuilder = getSearchBuilderForLocalResourceType(theParams);
@@ -256,7 +257,7 @@ public class JpaBulkExportProcessor implements IBulkExportProcessor {
 	 * possibly expanded by MDM, and don't have to go and fetch other resource DAOs.
 	 */
 	private Iterator<ResourcePersistentId> getExpandedPatientIterator(ExportPIDIteratorParameters theParameters) {
-		List<String> members = getMembers(theParameters.getGroupId());
+		List<String> members = getMembersFromGroupWithFilter(theParameters);
 		List<IIdType> ids = members.stream().map(member -> new IdDt("Patient/" + member)).collect(Collectors.toList());
 
 		// Are bulk exports partition aware or care about partition at all? This does
@@ -278,15 +279,36 @@ public class JpaBulkExportProcessor implements IBulkExportProcessor {
 	}
 
 	/**
-	 * Given the local myGroupId, read this group, and find all members' patient references.
+	 * Given the parameters, find all members' patient references in the group with the typeFilter applied.
 	 *
 	 * @return A list of strings representing the Patient IDs of the members (e.g. ["P1", "P2", "P3"]
 	 */
-	private List<String> getMembers(String theGroupId) {
+	private List<String> getMembersFromGroupWithFilter(ExportPIDIteratorParameters theParameters) {
-		SystemRequestDetails requestDetails = SystemRequestDetails.newSystemRequestAllPartitions();
+		RuntimeResourceDefinition def = myContext.getResourceDefinition(theParameters.getResourceType());
-		IBaseResource group = myDaoRegistry.getResourceDao("Group").read(new IdDt(theGroupId), requestDetails);
+		List<String> pids = new ArrayList<>();
-		List<IPrimitiveType> evaluate = myContext.newFhirPath().evaluate(group, "member.entity.reference", IPrimitiveType.class);
+
-		return evaluate.stream().map(IPrimitiveType::getValueAsString).collect(Collectors.toList());
+		List<SearchParameterMap> maps = myBulkExportHelperSvc.createSearchParameterMapsForResourceType(def, theParameters);
+
+		for (SearchParameterMap map : maps) {
+			//Ensure users did not monkey with the patient compartment search parameter.
+			validateSearchParametersForPatient(map, theParameters);
+
+			ISearchBuilder searchBuilder = getSearchBuilderForLocalResourceType(theParameters);
+
+			// Now, further filter the query with the group id.
+			HasOrListParam hasOrListParam = new HasOrListParam();
+			hasOrListParam.addOr(new HasParam("Group", "member", "_id", theParameters.getGroupId()));
+			map.add(PARAM_HAS, hasOrListParam);
+
+			IResultIterator resultIterator = searchBuilder.createQuery(map,
+				new SearchRuntimeDetails(null, theParameters.getJobId()),
+				null,
+				RequestPartitionId.allPartitions());
+			while (resultIterator.hasNext()) {
+				pids.add(resultIterator.next().toString());
+			}
+		}
+		return pids;
 	}
 
 	/**
@@ -435,7 +457,7 @@ public class JpaBulkExportProcessor implements IBulkExportProcessor {
 
 		//Now manually add the members of the group (its possible even with mdm expansion that some members dont have MDM matches,
 		//so would be otherwise skipped
-		expandedIds.addAll(getMembers(theParams.getGroupId()));
+		expandedIds.addAll(getMembersFromGroupWithFilter(theParams));
 
 		return expandedIds;
 	}

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/bulk/export/svc/JpaBulkExportProcessorTest.java

@@ -2,7 +2,6 @@ package ca.uhn.fhir.jpa.bulk.export.svc;
 
 import ca.uhn.fhir.context.FhirContext;
 import ca.uhn.fhir.context.RuntimeResourceDefinition;
-import ca.uhn.fhir.fhirpath.IFhirPath;
 import ca.uhn.fhir.interceptor.model.RequestPartitionId;
 import ca.uhn.fhir.jpa.api.config.DaoConfig;
 import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
@@ -13,7 +12,6 @@ import ca.uhn.fhir.jpa.bulk.export.model.ExportPIDIteratorParameters;
 import ca.uhn.fhir.jpa.dao.IResultIterator;
 import ca.uhn.fhir.jpa.dao.ISearchBuilder;
 import ca.uhn.fhir.jpa.dao.SearchBuilderFactory;
-import ca.uhn.fhir.jpa.dao.index.IJpaIdHelperService;
 import ca.uhn.fhir.jpa.dao.mdm.MdmExpansionCacheSvc;
 import ca.uhn.fhir.jpa.model.search.SearchRuntimeDetails;
 import ca.uhn.fhir.jpa.partition.SystemRequestDetails;
@@ -24,6 +22,9 @@ import ca.uhn.fhir.mdm.model.MdmPidTuple;
 import ca.uhn.fhir.model.primitive.IdDt;
 import ca.uhn.fhir.rest.api.server.bulk.BulkDataExportOptions;
 import ca.uhn.fhir.rest.api.server.storage.ResourcePersistentId;
+import ca.uhn.fhir.rest.param.HasOrListParam;
+import ca.uhn.fhir.rest.param.HasParam;
+import ch.qos.logback.classic.spi.ILoggingEvent;
 import org.hl7.fhir.instance.model.api.IIdType;
 import org.hl7.fhir.instance.model.api.IPrimitiveType;
 import org.hl7.fhir.r4.model.Group;
@@ -33,6 +34,7 @@ import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ValueSource;
+import org.mockito.ArgumentMatcher;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.Spy;
@@ -50,15 +52,17 @@ import java.util.List;
 import java.util.Optional;
 import java.util.Set;
 
+import static ca.uhn.fhir.rest.api.Constants.PARAM_HAS;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
+import static org.mockito.AdditionalMatchers.not;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyList;
-import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.argThat;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
@@ -242,9 +246,7 @@ public class JpaBulkExportProcessorTest {
 	}
 
 	@ParameterizedTest
-	@ValueSource(
+	@ValueSource(booleans = {true, false})
-		booleans = { true, false }
-	)
 	public void getResourcePidIterator_groupExportStyleWithPatientResource_returnsIterator(boolean theMdm) {
 		// setup
 		ExportPIDIteratorParameters parameters = createExportParameters(BulkDataExportOptions.ExportStyle.GROUP);
@@ -265,23 +267,37 @@ public class JpaBulkExportProcessorTest {
 		MdmPidTuple tuple = createTuple(groupId.getIdAsLong(), groupGoldenPid);
 
 		IFhirResourceDao<Group> groupDao = mock(IFhirResourceDao.class);
-		IFhirPath path = mock(IFhirPath.class);
 		parameters.setExpandMdm(theMdm); // set mdm expansion
 
+		// extra mocks
+		IFhirResourceDao<?> mockDao = mock(IFhirResourceDao.class);
+		ISearchBuilder searchBuilder = mock(ISearchBuilder.class);
+
 		// when
-		// getMembers
+		when(myIdHelperService.getPidsOrThrowException(any(), anyList()))
+			.thenReturn(pids);
+		// from getMembersFromGroupWithFilter
+		when(myBulkExportHelperService.createSearchParameterMapsForResourceType(any(RuntimeResourceDefinition.class), eq(parameters)))
+			.thenReturn(Collections.singletonList(new SearchParameterMap()));
+		// from getSearchBuilderForLocalResourceType
+		when(myDaoRegistry.getResourceDao(not(eq("Group"))))
+			.thenReturn(mockDao);
+		when(mySearchBuilderFactory.newSearchBuilder(eq(mockDao), eq(parameters.getResourceType()), any()))
+			.thenReturn(searchBuilder);
+		// ret
+		when(searchBuilder.createQuery(
+			any(SearchParameterMap.class),
+			any(SearchRuntimeDetails.class),
+			any(),
+			eq(RequestPartitionId.allPartitions())))
+			.thenReturn(new ListResultIterator(pids));
+
+		// mdm expansion stuff
+		if (theMdm) {
 			when(myDaoRegistry.getResourceDao(eq("Group")))
 				.thenReturn(groupDao);
 			when(groupDao.read(eq(new IdDt(parameters.getGroupId())), any(SystemRequestDetails.class)))
 				.thenReturn(groupResource);
-		when(myFhirContext.newFhirPath())
-			.thenReturn(path);
-		when(path.evaluate(eq(groupResource), anyString(), any(Class.class)))
-			.thenReturn(patientTypes);
-		when(myIdHelperService.getPidsOrThrowException(any(), anyList()))
-			.thenReturn(pids);
-		// mdm expansion stuff
-		if (theMdm) {
 			when(myIdHelperService.translatePidsToForcedIds(any(Set.class)))
 				.thenAnswer(params -> {
 					Set<ResourcePersistentId> uniqPids = params.getArgument(0);
@@ -322,7 +338,7 @@ public class JpaBulkExportProcessorTest {
 	}
 
 	@ParameterizedTest
-	@ValueSource(booleans = { false, true })
+	@ValueSource(booleans = {false, true})
 	public void getResourcePidIterator_groupExportNonPatient_returnsIterator(boolean theMdm) {
 		// setup
 		ExportPIDIteratorParameters parameters = createExportParameters(BulkDataExportOptions.ExportStyle.GROUP);
@@ -345,17 +361,33 @@ public class JpaBulkExportProcessorTest {
 		IFhirResourceDao<Group> groupDao = mock(IFhirResourceDao.class);
 		IFhirResourceDao<Observation> observationDao = mock(IFhirResourceDao.class);
 		parameters.setExpandMdm(theMdm); // set mdm expansion
-		IFhirPath path = mock(IFhirPath.class);
+
+		// extra mocks
+		IFhirResourceDao<?> mockDao = mock(IFhirResourceDao.class);
 		ISearchBuilder searchBuilder = mock(ISearchBuilder.class);
 
 		// when
-		// getMembers
 		when(myDaoRegistry.getResourceDao(eq("Group")))
 			.thenReturn(groupDao);
 		when(groupDao.read(any(IIdType.class), any(SystemRequestDetails.class)))
 			.thenReturn(groupResource);
 		when(myIdHelperService.getPidOrNull(any(), eq(groupResource)))
 			.thenReturn(groupId);
+		when(myBulkExportHelperService.createSearchParameterMapsForResourceType(any(RuntimeResourceDefinition.class), eq(parameters)))
+			.thenReturn(Collections.singletonList(new SearchParameterMap()));
+		when(myDaoRegistry.getResourceDao(not(eq("Group"))))
+			.thenReturn(mockDao);
+		when(mySearchBuilderFactory.newSearchBuilder(eq(mockDao), not(eq("Group")), any()))
+			.thenReturn(searchBuilder);
+
+		// ret
+		when(searchBuilder.createQuery(
+			any(SearchParameterMap.class),
+			any(SearchRuntimeDetails.class),
+			any(),
+			eq(RequestPartitionId.allPartitions())))
+			.thenReturn(new ListResultIterator(Collections.singletonList(pid)))
+			.thenReturn(resultIterator);
 
 		if (theMdm) {
 			when(myMdmLinkDao.expandPidsFromGroupPidGivenMatchResult(any(ResourcePersistentId.class), eq(MdmMatchResultEnum.MATCH)))
@@ -370,21 +402,6 @@ public class JpaBulkExportProcessorTest {
 					return new PersistentIdToForcedIdMap(answer);
 				});
 		}
-		when(myFhirContext.newFhirPath())
-			.thenReturn(path);
-		when(path.evaluate(eq(groupResource), anyString(), any(Class.class)))
-			.thenReturn(patientTypes);
-		when(myBulkExportHelperService.createSearchParameterMapsForResourceType(any(RuntimeResourceDefinition.class), any(ExportPIDIteratorParameters.class)))
-			.thenReturn(Collections.singletonList(new SearchParameterMap()));
-		when(myDaoRegistry.getResourceDao(eq("Observation")))
-			.thenReturn(observationDao);
-		when(mySearchBuilderFactory.newSearchBuilder(any(), eq("Observation"), any()))
-			.thenReturn(searchBuilder);
-		when(searchBuilder.createQuery(any(SearchParameterMap.class),
-			any(SearchRuntimeDetails.class),
-			any(),
-			eq(RequestPartitionId.allPartitions())))
-			.thenReturn(resultIterator);
 
 		// test
 		Iterator<ResourcePersistentId> pidIterator = myProcessor.getResourcePidIterator(parameters);

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/bulk/BulkDataExportTest.java

@@ -0,0 +1,136 @@
+package ca.uhn.fhir.jpa.bulk;
+
+import ca.uhn.fhir.jpa.api.model.BulkExportJobResults;
+import ca.uhn.fhir.jpa.api.svc.IBatch2JobRunner;
+import ca.uhn.fhir.jpa.batch.models.Batch2JobStartResponse;
+import ca.uhn.fhir.jpa.provider.r4.BaseResourceProviderR4Test;
+import ca.uhn.fhir.jpa.util.BulkExportUtils;
+import ca.uhn.fhir.rest.api.Constants;
+import ca.uhn.fhir.rest.api.server.bulk.BulkDataExportOptions;
+import ca.uhn.fhir.util.JsonUtil;
+import com.google.common.collect.Sets;
+import org.hamcrest.Matchers;
+import org.hl7.fhir.r4.model.Binary;
+import org.hl7.fhir.r4.model.Enumerations;
+import org.hl7.fhir.r4.model.Group;
+import org.hl7.fhir.r4.model.IdType;
+import org.hl7.fhir.r4.model.Patient;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.beans.factory.annotation.Autowired;
+
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+
+import static org.awaitility.Awaitility.await;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.not;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+public class BulkDataExportTest extends BaseResourceProviderR4Test {
+	private static final Logger ourLog = LoggerFactory.getLogger(BulkDataExportTest.class);
+
+	@Autowired
+	private IBatch2JobRunner myJobRunner;
+
+	@Test
+	public void testGroupBulkExportWithTypeFilter() {
+		// Create some resources
+		Patient patient = new Patient();
+		patient.setId("PF");
+		patient.setGender(Enumerations.AdministrativeGender.FEMALE);
+		patient.setActive(true);
+		myClient.update().resource(patient).execute();
+
+		patient = new Patient();
+		patient.setId("PM");
+		patient.setGender(Enumerations.AdministrativeGender.MALE);
+		patient.setActive(true);
+		myClient.update().resource(patient).execute();
+
+		Group group = new Group();
+		group.setId("Group/G");
+		group.setActive(true);
+		group.addMember().getEntity().setReference("Patient/PF");
+		group.addMember().getEntity().setReference("Patient/PM");
+		myClient.update().resource(group).execute();
+
+		varifyBulkExportResults("G", Sets.newHashSet("Patient?gender=female"), Collections.singletonList("\"PF\""), Collections.singletonList("\"PM\""));
+	}
+
+	@Test
+	public void testGroupBulkExportNotInGroup_DoeNotShowUp() {
+		// Create some resources
+		Patient patient = new Patient();
+		patient.setId("PING1");
+		patient.setGender(Enumerations.AdministrativeGender.FEMALE);
+		patient.setActive(true);
+		myClient.update().resource(patient).execute();
+
+		patient = new Patient();
+		patient.setId("PING2");
+		patient.setGender(Enumerations.AdministrativeGender.MALE);
+		patient.setActive(true);
+		myClient.update().resource(patient).execute();
+
+		patient = new Patient();
+		patient.setId("PNING3");
+		patient.setGender(Enumerations.AdministrativeGender.MALE);
+		patient.setActive(true);
+		myClient.update().resource(patient).execute();
+
+		Group group = new Group();
+		group.setId("Group/G2");
+		group.setActive(true);
+		group.addMember().getEntity().setReference("Patient/PING1");
+		group.addMember().getEntity().setReference("Patient/PING2");
+		myClient.update().resource(group).execute();
+
+		varifyBulkExportResults("G2", new HashSet<>(), List.of("\"PING1\"", "\"PING2\""), Collections.singletonList("\"PNING3\""));
+	}
+
+
+	private void varifyBulkExportResults(String theGroupId, HashSet<String> theFilters, List<String> theContainedList, List<String> theExcludedList) {
+		BulkDataExportOptions options = new BulkDataExportOptions();
+		options.setResourceTypes(Sets.newHashSet("Patient"));
+		options.setGroupId(new IdType("Group", theGroupId));
+		options.setFilters(theFilters);
+		options.setExportStyle(BulkDataExportOptions.ExportStyle.GROUP);
+		options.setOutputFormat(Constants.CT_FHIR_NDJSON);
+
+		Batch2JobStartResponse startResponse = myJobRunner.startNewJob(BulkExportUtils.createBulkExportJobParametersFromExportOptions(options));
+
+		assertNotNull(startResponse);
+
+		// Run a scheduled pass to build the export
+		myBatch2JobHelper.awaitJobCompletion(startResponse.getJobId());
+
+		await().until(() -> myJobRunner.getJobInfo(startResponse.getJobId()).getReport() != null);
+
+		// Iterate over the files
+		String report = myJobRunner.getJobInfo(startResponse.getJobId()).getReport();
+		BulkExportJobResults results = JsonUtil.deserialize(report, BulkExportJobResults.class);
+		for (Map.Entry<String, List<String>> file : results.getResourceTypeToBinaryIds().entrySet()) {
+			List<String> binaryIds = file.getValue();
+			assertEquals(1, binaryIds.size());
+			for (String binaryId : binaryIds) {
+				Binary binary = myBinaryDao.read(new IdType(binaryId));
+				assertEquals(Constants.CT_FHIR_NDJSON, binary.getContentType());
+				String contents = new String(binary.getContent(), Constants.CHARSET_UTF8);
+				ourLog.info("Next contents for type {} :\n{}", binary.getResourceType(), contents);
+				for (String containedString : theContainedList) {
+					assertThat(contents, Matchers.containsString(containedString));
+
+				}
+				for (String excludedString : theExcludedList) {
+					assertThat(contents, not(Matchers.containsString(excludedString)));
+				}
+			}
+		}
+	}
+
+}