fix patientId interceptor partition selection (#6236)

* fixed

* fixed

* detect uuid references earlier in the PatientIdPartitionInterceptor

* TODO

hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/client/api/IRestfulClientFactory.java

@@ -37,7 +37,6 @@ public interface IRestfulClientFactory {
 	 */
 	public static final int DEFAULT_CONNECTION_REQUEST_TIMEOUT = 10000;
 
-
 	/**
 	 * Default value for {@link #getConnectionTimeToLive()}
 	 */
@@ -81,7 +80,6 @@ public interface IRestfulClientFactory {
 	 */
 	int getConnectTimeout();
 
-
 	/**
 	 * Gets the connection time to live, in milliseconds. This is the amount of time to keep connections alive for reuse.
 	 * <p>

hapi-fhir-base/src/main/java/org/hl7/fhir/instance/model/api/IIdType.java

@@ -120,6 +120,13 @@ public interface IIdType extends IPrimitiveType<String> {
 	 */
 	boolean isVersionIdPartValidLong();
 
+	/**
+	 * @return true if the id begins with "urn:uuid:"
+	 */
+	default boolean isUuid() {
+		return getValue() != null && getValue().startsWith("urn:uuid:");
+	}
+
 	@Override
 	IIdType setValue(String theString);
 

hapi-fhir-cli/hapi-fhir-cli-api/src/main/java/ca/uhn/fhir/cli/client/HapiFhirCliRestfulClientFactory.java

@@ -105,10 +105,11 @@ public class HapiFhirCliRestfulClientFactory extends RestfulClientFactory {
 				Registry<ConnectionSocketFactory> registry = RegistryBuilder.<ConnectionSocketFactory>create()
 						.register("https", sslConnectionSocketFactory)
 						.build();
-				connectionManager =
-						new PoolingHttpClientConnectionManager(registry, null, null, null, getConnectionTimeToLive(), TimeUnit.MILLISECONDS);
+				connectionManager = new PoolingHttpClientConnectionManager(
+						registry, null, null, null, getConnectionTimeToLive(), TimeUnit.MILLISECONDS);
 			} else {
-				connectionManager = new PoolingHttpClientConnectionManager(getConnectionTimeToLive(), TimeUnit.MILLISECONDS);
+				connectionManager =
+						new PoolingHttpClientConnectionManager(getConnectionTimeToLive(), TimeUnit.MILLISECONDS);
 			}
 
 			connectionManager.setMaxTotal(getPoolMaxTotal());

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_0/6231-patient-id-compartment.yaml

@@ -0,0 +1,14 @@
+---
+type: fix
+issue: 6231
+title: "The PatientIdPartitionInterceptor could on rare occasion select the incorrect 
+partition for a resource. This has been corrected. In order for the wrong partition
+to be selected, the following three things need to be true:
+1) there are multiple values of a patient compartment for a resource (see https://hl7.org/fhir/R4/compartmentdefinition-patient.html)
+2) a patient compartment value is a non-Patient reference 
+3) the search parameter of the incorrect value needs to come alphabetically before the search parameter of the correct
+value.
+
+For example, if a QuestionnaireResponse has subject Patient/123 and author Organization/456,
+then since 'author' appears ahead of 'subject' alphabetically it would incorrectly determine the partition.
+The fix changed the partition selection so that it now only matches on Patient references."

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/util/ResourceCompartmentUtilTest.java

@@ -0,0 +1,36 @@
+package ca.uhn.fhir.jpa.util;
+
+import ca.uhn.fhir.jpa.searchparam.extractor.ISearchParamExtractor;
+import ca.uhn.fhir.jpa.test.BaseJpaR4Test;
+import org.hl7.fhir.instance.model.api.IIdType;
+import org.hl7.fhir.r4.model.QuestionnaireResponse;
+import org.hl7.fhir.r4.model.Reference;
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
+
+import java.util.Optional;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class ResourceCompartmentUtilTest extends BaseJpaR4Test {
+
+	public static final String FAMILY = "TestFamily";
+	private static final String ORG_NAME = "Test Organization";
+	@Autowired
+	ISearchParamExtractor mySearchParamExtractor;
+
+	@Test
+	public void testMultiCompartment() {
+		IIdType pid = createPatient(withFamily(FAMILY));
+		QuestionnaireResponse qr = new QuestionnaireResponse();
+		qr.setSubject(new Reference(pid));
+		IIdType oid = createOrganization(withName(ORG_NAME));
+		qr.setAuthor(new Reference(oid));
+
+		Optional<String> result = ResourceCompartmentUtil.getPatientCompartmentIdentity(qr, myFhirContext, mySearchParamExtractor);
+		// red-green: before the bug fix, this returned the org id because "author" is alphabetically before "patient"
+		assertThat(result)
+			.isPresent()
+			.hasValue(pid.getIdPart());
+	}
+}

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

@@ -39,6 +39,7 @@ import ca.uhn.fhir.rest.server.exceptions.MethodNotAllowedException;
 import ca.uhn.fhir.rest.server.provider.ProviderConstants;
 import jakarta.annotation.Nonnull;
 import org.hl7.fhir.instance.model.api.IBaseResource;
+import org.hl7.fhir.instance.model.api.IIdType;
 import org.hl7.fhir.r4.model.IdType;
 import org.springframework.beans.factory.annotation.Autowired;
 
@@ -92,15 +93,15 @@ public class PatientIdPartitionInterceptor {
 
 		Optional<String> oCompartmentIdentity;
 		if (resourceDef.getName().equals("Patient")) {
-			oCompartmentIdentity =
-					Optional.ofNullable(theResource.getIdElement().getIdPart());
-			if (oCompartmentIdentity.isEmpty()) {
+			IIdType idElement = theResource.getIdElement();
+			oCompartmentIdentity = Optional.ofNullable(idElement.getIdPart());
+			if (idElement.isUuid() || oCompartmentIdentity.isEmpty()) {
 				throw new MethodNotAllowedException(
 						Msg.code(1321) + "Patient resource IDs must be client-assigned in patient compartment mode");
 			}
 		} else {
-			oCompartmentIdentity =
-					ResourceCompartmentUtil.getResourceCompartment(theResource, compartmentSps, mySearchParamExtractor);
+			oCompartmentIdentity = ResourceCompartmentUtil.getResourceCompartment(
+					"Patient", theResource, compartmentSps, mySearchParamExtractor);
 		}
 
 		return oCompartmentIdentity

hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/util/ResourceCompartmentUtil.java

@@ -45,8 +45,8 @@ public class ResourceCompartmentUtil {
 	/**
 	 * Extract, if exists, the patient compartment identity of the received resource.
 	 * It must be invoked in patient compartment mode.
-	 * @param theResource the resource to which extract the patient compartment identity
-	 * @param theFhirContext the active FhirContext
+	 * @param theResource             the resource to which extract the patient compartment identity
+	 * @param theFhirContext          the active FhirContext
 	 * @param theSearchParamExtractor the configured search parameter extractor
 	 * @return the optional patient compartment identifier
 	 * @throws MethodNotAllowedException if received resource is of type "Patient" and ID is not assigned.
@@ -69,20 +69,23 @@ public class ResourceCompartmentUtil {
 			return Optional.of(compartmentIdentity);
 		}
 
-		return getResourceCompartment(theResource, patientCompartmentSps, theSearchParamExtractor);
+		return getResourceCompartment("Patient", theResource, patientCompartmentSps, theSearchParamExtractor);
 	}
 
 	/**
 	 * Extracts and returns an optional compartment of the received resource
-	 * @param theResource source resource which compartment is extracted
-	 * @param theCompartmentSps the RuntimeSearchParam list involving the searched compartment
+	 * @param theCompartmentName     the name of the compartment
+	 * @param theResource            source resource which compartment is extracted
+	 * @param theCompartmentSps      the RuntimeSearchParam list involving the searched compartment
 	 * @param mySearchParamExtractor the ISearchParamExtractor to be used to extract the parameter values
 	 * @return optional compartment of the received resource
 	 */
 	public static Optional<String> getResourceCompartment(
+			String theCompartmentName,
 			IBaseResource theResource,
 			List<RuntimeSearchParam> theCompartmentSps,
 			ISearchParamExtractor mySearchParamExtractor) {
+		// TODO KHS consolidate with FhirTerser.getCompartmentOwnersForResource()
 		return theCompartmentSps.stream()
 				.flatMap(param -> Arrays.stream(BaseSearchParamExtractor.splitPathsR4(param.getPath())))
 				.filter(StringUtils::isNotBlank)
@@ -94,7 +97,10 @@ public class ResourceCompartmentUtil {
 				.filter(t -> t instanceof IBaseReference)
 				.map(t -> (IBaseReference) t)
 				.map(t -> t.getReferenceElement().getValue())
-				.map(t -> new IdType(t).getIdPart())
+				.map(IdType::new)
+				.filter(t -> theCompartmentName.equals(
+						t.getResourceType())) // assume the compartment name matches the resource type
+				.map(IdType::getIdPart)
 				.filter(StringUtils::isNotBlank)
 				.findFirst();
 	}

hapi-fhir-storage/src/test/java/ca/uhn/fhir/jpa/util/ResourceCompartmentUtilTest.java

@@ -48,7 +48,7 @@ class ResourceCompartmentUtilTest {
 		when(mySearchParamExtractor.getPathValueExtractor(myResource, "Observation.subject"))
 			.thenReturn(() -> List.of(new Reference("Patient/P01")));
 
-		Optional<String> oCompartment = ResourceCompartmentUtil.getResourceCompartment(
+		Optional<String> oCompartment = ResourceCompartmentUtil.getResourceCompartment("Patient",
 			myResource, myCompartmentSearchParams, mySearchParamExtractor);
 
 		assertThat(oCompartment).isPresent();