updating partition interceptor to allow multiple patient ids (#5326)

* updating partition interceptor to allow multiple patient ids

* spotless

---------

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

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5235-allow-search-on-multiple-patient-ids-in-partitioned-environment.yaml

@@ -0,0 +1,6 @@
+---
+type: add
+issue: 5235
+title: "Changes have been made to allow searching on multiple patient _ids
+        when in a patient_id partitioned environment.
+    "

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

@@ -162,7 +162,6 @@ public class IdHelperService implements IIdHelperService<JpaPid> {
 			boolean theExcludeDeleted)
 			throws ResourceNotFoundException {
 		assert myDontCheckActiveTransactionForUnitTest || TransactionSynchronizationManager.isSynchronizationActive();
-		assert theRequestPartitionId != null;
 
 		if (theResourceId.contains("/")) {
 			theResourceId = theResourceId.substring(theResourceId.indexOf("/") + 1);
@@ -510,8 +509,6 @@ public class IdHelperService implements IIdHelperService<JpaPid> {
 
 	private Map<String, List<IResourceLookup<JpaPid>>> translateForcedIdToPids(
 			@Nonnull RequestPartitionId theRequestPartitionId, Collection<IIdType> theId, boolean theExcludeDeleted) {
-		assert theRequestPartitionId != null;
-
 		theId.forEach(id -> Validate.isTrue(id.hasIdPart()));
 
 		if (theId.isEmpty()) {

hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/BaseMdmR4Test.java

@@ -183,6 +183,17 @@ abstract public class BaseMdmR4Test extends BaseJpaR4Test {
 
 	@Nonnull
 	protected Patient createPatient(Patient thePatient, boolean theMdmManaged, boolean isRedirect) {
+		return createPatientWithUpdate(
+			thePatient, theMdmManaged, isRedirect, false
+		);
+	}
+
+	protected Patient createPatientWithUpdate(
+		Patient thePatient,
+		boolean theMdmManaged,
+		boolean isRedirect,
+		boolean theUseUpdateBool
+	) {
 		if (theMdmManaged) {
 			MdmResourceUtil.setMdmManaged(thePatient);
 			if (isRedirect) {
@@ -192,9 +203,15 @@ abstract public class BaseMdmR4Test extends BaseJpaR4Test {
 			}
 		}
 
-		DaoMethodOutcome outcome = myPatientDao.create(thePatient);
+		Patient patient;
-		Patient patient = (Patient) outcome.getResource();
+		if (theUseUpdateBool) {
-		patient.setId(outcome.getId());
+			DaoMethodOutcome outcome = myPatientDao.update(thePatient);
+			patient = (Patient) outcome.getResource();
+		} else {
+			DaoMethodOutcome outcome = myPatientDao.create(thePatient);
+			patient = (Patient) outcome.getResource();
+			patient.setId(outcome.getId());
+		}
 		return patient;
 	}
 
@@ -209,8 +226,25 @@ abstract public class BaseMdmR4Test extends BaseJpaR4Test {
 		return patient;
 	}
 
+	public Patient createPatientOnPartition(
+		Patient thePatient,
+		boolean theMdmManaged,
+		boolean isRedirect,
+		RequestPartitionId theRequestPartitionId
+	) {
+		return createPatientOnPartition(
+			thePatient, theMdmManaged, isRedirect, theRequestPartitionId, false
+		);
+	}
+
 	@Nonnull
-	protected Patient createPatientOnPartition(Patient thePatient, boolean theMdmManaged, boolean isRedirect, RequestPartitionId theRequestPartitionId) {
+	protected Patient createPatientOnPartition(
+		Patient thePatient,
+		boolean theMdmManaged,
+		boolean isRedirect,
+		RequestPartitionId theRequestPartitionId,
+		boolean theDoUpdate
+	) {
 		if (theMdmManaged) {
 			MdmResourceUtil.setMdmManaged(thePatient);
 			if (isRedirect) {
@@ -222,9 +256,17 @@ abstract public class BaseMdmR4Test extends BaseJpaR4Test {
 
 		SystemRequestDetails systemRequestDetails = new SystemRequestDetails();
 		systemRequestDetails.setRequestPartitionId(theRequestPartitionId);
-		DaoMethodOutcome outcome = myPatientDao.create(thePatient, systemRequestDetails);
+
-		Patient patient = (Patient) outcome.getResource();
+		Patient patient;
-		patient.setId(outcome.getId());
+		if (theDoUpdate) {
+			DaoMethodOutcome outcome = myPatientDao.update(thePatient, systemRequestDetails);
+			patient = (Patient) outcome.getResource();
+			patient.setId(outcome.getId());
+		} else {
+			DaoMethodOutcome outcome = myPatientDao.create(thePatient, systemRequestDetails);
+			patient = (Patient) outcome.getResource();
+			patient.setId(outcome.getId());
+		}
 		patient.setUserData(Constants.RESOURCE_PARTITION_ID, theRequestPartitionId);
 		return patient;
 	}

hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/svc/MdmResourceDaoSvcTest.java

@@ -2,28 +2,42 @@ package ca.uhn.fhir.jpa.mdm.svc;
 
 import ca.uhn.fhir.interceptor.model.RequestPartitionId;
 import ca.uhn.fhir.jpa.entity.PartitionEntity;
+import ca.uhn.fhir.jpa.interceptor.PatientIdPartitionInterceptor;
 import ca.uhn.fhir.jpa.mdm.BaseMdmR4Test;
 import ca.uhn.fhir.jpa.model.config.PartitionSettings;
+import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
+import ca.uhn.fhir.jpa.searchparam.extractor.ISearchParamExtractor;
 import ca.uhn.fhir.mdm.util.MdmResourceUtil;
+import ca.uhn.fhir.rest.api.server.IBundleProvider;
 import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
+import ca.uhn.fhir.rest.param.StringOrListParam;
+import ca.uhn.fhir.rest.param.StringParam;
 import org.hl7.fhir.instance.model.api.IAnyResource;
+import org.hl7.fhir.instance.model.api.IBaseBundle;
+import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.r4.model.Patient;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
 
 import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.Optional;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
+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;
 
 public class MdmResourceDaoSvcTest extends BaseMdmR4Test {
 	private static final String TEST_EID = "TEST_EID";
 	@Autowired
 	MdmResourceDaoSvc myResourceDaoSvc;
+	@Autowired
+	private ISearchParamExtractor mySearchParamExtractor;
 
 	@Override
 	@AfterEach
@@ -77,6 +91,74 @@ public class MdmResourceDaoSvcTest extends BaseMdmR4Test {
 		assertThat(foundSourcePatient.get().getIdElement().toUnqualifiedVersionless().getValue(), is(goodSourcePatient.getIdElement().toUnqualifiedVersionless().getValue()));
 	}
 
+	@Test
+	public void testSearchForMultiplePatientsByIdInPartitionedEnvironment() {
+		// setup
+		int resourceCount = 3;
+		String[] idPrefaces = new String[] {
+			"RED", "BLUE", "GREEN"
+		};
+
+		SearchParameterMap map;
+		IBundleProvider result;
+
+		myPartitionSettings.setPartitioningEnabled(true);
+		myPartitionSettings.setUnnamedPartitionMode(true);
+		myPartitionSettings.setIncludePartitionInSearchHashes(false);
+
+		PatientIdPartitionInterceptor interceptor = new PatientIdPartitionInterceptor(myFhirContext, mySearchParamExtractor, myPartitionSettings);
+		myInterceptorRegistry.registerInterceptor(interceptor);
+
+		try {
+			StringOrListParam patientIds = new StringOrListParam();
+			for (int i = 0; i < resourceCount; i++) {
+				String idPreface = idPrefaces[i];
+				Patient patient = new Patient();
+				patient.setId("Patient/" + idPreface + i);
+				// patients must be created with a forced id for PatientId partitioning
+				Patient patientOnPartition = createPatientWithUpdate(patient,
+					true, false, true);
+				patientIds.add(new StringParam("Patient/" +
+					patientOnPartition.getIdElement().getIdPart()
+				));
+			}
+
+			// test
+			map = SearchParameterMap.newSynchronous();
+			map.add("_id", patientIds);
+			result = myPatientDao.search(map, new SystemRequestDetails());
+
+			// verify
+			assertNotNull(result);
+			assertFalse(result.isEmpty());
+			List<IBaseResource> resources = result.getAllResources();
+			assertEquals(resourceCount, resources.size());
+			int count = 0;
+			for (IBaseResource resource : resources) {
+				String id = idPrefaces[count++];
+				assertTrue(resource instanceof Patient);
+				Patient patient = (Patient) resource;
+				assertTrue(patient.getId().contains(id));
+			}
+
+			// ensure single id works too
+			StringParam firstId = patientIds.getValuesAsQueryTokens().get(0);
+			map = SearchParameterMap.newSynchronous();
+			map.add("_id", firstId);
+			result = myPatientDao.search(map, new SystemRequestDetails());
+
+			// verify 2
+			assertNotNull(result);
+			resources = result.getAllResources();
+			assertEquals(1, resources.size());
+			assertTrue(result.getAllResources().get(0) instanceof Patient);
+			Patient patient = (Patient) result.getAllResources().get(0);
+			assertTrue(patient.getId().contains(firstId.getValue()));
+		} finally {
+			myInterceptorRegistry.unregisterInterceptor(interceptor);
+		}
+	}
+
 	@Test
 	public void testSearchGoldenResourceOnDifferentPartitions() {
 		myPartitionSettings.setPartitioningEnabled(true);

hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/svc/candidate/MdmPartitionedGoldenResourceFindingTest.java

@@ -28,6 +28,7 @@ public class MdmPartitionedGoldenResourceFindingTest extends BaseMdmR4Test {
 	public void testNoMatchOnResourcesInDifferentPartition(){
 		myMdmSettings.setSearchAllPartitionForMatch(false);
 		myPartitionSettings.setPartitioningEnabled(true);
+		myPartitionSettings.setUnnamedPartitionMode(false);
 		myPartitionLookupSvc.createPartition(new PartitionEntity().setId(1).setName(PARTITION_1), null);
 		myPartitionLookupSvc.createPartition(new PartitionEntity().setId(2).setName(PARTITION_2), null);
 
@@ -51,6 +52,7 @@ public class MdmPartitionedGoldenResourceFindingTest extends BaseMdmR4Test {
 	public void testMatchOnResourcesInDifferentPartitionIfSearchAllPartition(){
 		myMdmSettings.setSearchAllPartitionForMatch(true);
 		myPartitionSettings.setPartitioningEnabled(true);
+		myPartitionSettings.setUnnamedPartitionMode(false);
 		myPartitionLookupSvc.createPartition(new PartitionEntity().setId(1).setName(PARTITION_1), null);
 		myPartitionLookupSvc.createPartition(new PartitionEntity().setId(2).setName(PARTITION_2), null);
 

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/interceptor/PatientIdPartitionInterceptorTest.java

@@ -279,7 +279,7 @@ public class PatientIdPartitionInterceptorTest extends BaseJpaR4SystemTest {
 					.add("subject", new TokenParam("http://foo", "2"))
 				, mySrd);
 		} catch (MethodNotAllowedException e) {
-			assertEquals(Msg.code(1325) + "Multiple values for parameter subject is not supported in patient compartment mode", e.getMessage());
+			assertEquals(Msg.code(1324) + "Multiple values for parameter subject is not supported in patient compartment mode", e.getMessage());
 		}
 
 		// Multiple ORs

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/packages/JpaPackageCacheTest.java

@@ -117,6 +117,7 @@ public class JpaPackageCacheTest extends BaseJpaR4Test {
 	public void testSaveAndDeletePackageUnnamedPartitionsEnabled() throws IOException {
 		myPartitionSettings.setPartitioningEnabled(true);
 		myPartitionSettings.setDefaultPartitionId(0);
+		boolean isUnnamed = myPartitionSettings.isUnnamedPartitionMode();
 		myPartitionSettings.setUnnamedPartitionMode(true);
 		PatientIdPartitionInterceptor patientIdPartitionInterceptor = new PatientIdPartitionInterceptor(myFhirContext, mySearchParamExtractor, myPartitionSettings);
 		myInterceptorService.registerInterceptor(patientIdPartitionInterceptor);
@@ -145,6 +146,7 @@ public class JpaPackageCacheTest extends BaseJpaR4Test {
 			List<String> deleteOutcomeMsgs = deleteOutcomeJson.getMessage();
 			assertEquals("Deleting package hl7.fhir.uv.shorthand#0.12.0", deleteOutcomeMsgs.get(0));
 		} finally {
+			myPartitionSettings.setUnnamedPartitionMode(isUnnamed);
 			myInterceptorService.unregisterInterceptor(patientIdPartitionInterceptor);
 			myInterceptorService.unregisterInterceptor(myRequestTenantPartitionInterceptor);
 		}

hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/interceptor/PatientIdPartitionInterceptor.java

@@ -43,6 +43,7 @@ import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.r4.model.IdType;
 import org.springframework.beans.factory.annotation.Autowired;
 
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 import java.util.stream.Collectors;
@@ -150,23 +151,25 @@ public class PatientIdPartitionInterceptor {
 				}
 				break;
 			case SEARCH_TYPE:
-				SearchParameterMap params = (SearchParameterMap) theReadDetails.getSearchParams();
+				SearchParameterMap params = theReadDetails.getSearchParams();
-				String idPart = null;
+
 				if ("Patient".equals(theReadDetails.getResourceType())) {
-					idPart = getSingleResourceIdValueOrNull(params, "_id", "Patient");
+					List<String> idParts = getResourceIdList(params, "_id", "Patient", false);
+
+					if (idParts.size() == 1) {
+						return provideCompartmentMemberInstanceResponse(theRequestDetails, idParts.get(0));
+					} else {
+						return RequestPartitionId.allPartitions();
+					}
 				} else {
 					for (RuntimeSearchParam nextCompartmentSp : compartmentSps) {
-						idPart = getSingleResourceIdValueOrNull(params, nextCompartmentSp.getName(), "Patient");
+						List<String> idParts = getResourceIdList(params, nextCompartmentSp.getName(), "Patient", true);
-						if (idPart != null) {
+						if (!idParts.isEmpty()) {
-							break;
+							return provideCompartmentMemberInstanceResponse(theRequestDetails, idParts.get(0));
 						}
 					}
 				}
 
-				if (isNotBlank(idPart)) {
-					return provideCompartmentMemberInstanceResponse(theRequestDetails, idPart);
-				}
-
 				break;
 
 			default:
@@ -191,39 +194,40 @@ public class PatientIdPartitionInterceptor {
 				.collect(Collectors.toList());
 	}
 
-	private String getSingleResourceIdValueOrNull(
+	private List<String> getResourceIdList(
-			SearchParameterMap theParams, String theParamName, String theResourceType) {
+			SearchParameterMap theParams, String theParamName, String theResourceType, boolean theExpectOnlyOneBool) {
-		String idPart = null;
+		List<String> idParts = new ArrayList<>();
 		List<List<IQueryParameterType>> idParamAndList = theParams.get(theParamName);
-		if (idParamAndList != null && idParamAndList.size() == 1) {
+		if (idParamAndList != null) {
-			List<IQueryParameterType> idParamOrList = idParamAndList.get(0);
+			for (List<IQueryParameterType> idParamOrList : idParamAndList) {
-			if (idParamOrList.size() == 1) {
+				for (IQueryParameterType idParam : idParamOrList) {
-				IQueryParameterType idParam = idParamOrList.get(0);
+					if (isNotBlank(idParam.getQueryParameterQualifier())) {
-				if (isNotBlank(idParam.getQueryParameterQualifier())) {
+						throw new MethodNotAllowedException(
-					throw new MethodNotAllowedException(Msg.code(1322) + "The parameter " + theParamName
+								Msg.code(1322) + "The parameter " + theParamName + idParam.getQueryParameterQualifier()
-							+ idParam.getQueryParameterQualifier() + " is not supported in patient compartment mode");
+										+ " is not supported in patient compartment mode");
-				}
+					}
-				if (idParam instanceof ReferenceParam) {
+					if (idParam instanceof ReferenceParam) {
-					String chain = ((ReferenceParam) idParam).getChain();
+						String chain = ((ReferenceParam) idParam).getChain();
-					if (chain != null) {
+						if (chain != null) {
-						throw new MethodNotAllowedException(Msg.code(1323) + "The parameter " + theParamName + "."
+							throw new MethodNotAllowedException(Msg.code(1323) + "The parameter " + theParamName + "."
-								+ chain + " is not supported in patient compartment mode");
+									+ chain + " is not supported in patient compartment mode");
+						}
+					}
+
+					IdType id = new IdType(idParam.getValueAsQueryToken(myFhirContext));
+					if (!id.hasResourceType() || id.getResourceType().equals(theResourceType)) {
+						idParts.add(id.getIdPart());
 					}
 				}
-
-				IdType id = new IdType(idParam.getValueAsQueryToken(myFhirContext));
-				if (!id.hasResourceType() || id.getResourceType().equals(theResourceType)) {
-					idPart = id.getIdPart();
-				}
-			} else if (idParamOrList.size() > 1) {
-				throw new MethodNotAllowedException(Msg.code(1324) + "Multiple values for parameter " + theParamName
-						+ " is not supported in patient compartment mode");
 			}
-		} else if (idParamAndList != null && idParamAndList.size() > 1) {
+		}
-			throw new MethodNotAllowedException(Msg.code(1325) + "Multiple values for parameter " + theParamName
+
+		if (theExpectOnlyOneBool && idParts.size() > 1) {
+			throw new MethodNotAllowedException(Msg.code(1324) + "Multiple values for parameter " + theParamName
 					+ " is not supported in patient compartment mode");
 		}
-		return idPart;
+
+		return idParts;
 	}
 
 	/**