Non-partitionable resource searches could be executed against a partition that is not the default partition

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/8_6_0/7217-non-partitionable-resource-is-searched-on-non-default-partition.yaml

@@ -0,0 +1,5 @@
+---
+type: fix
+issue: 7217
+title: "Previously, searches for non-partitionable resources (e.g. a Library) could be executed against a partition that
+is not the default partition. This has been corrected."

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/BasePartitioningR4Test.java

@@ -153,7 +153,6 @@ public void afterPurgeDatabase() {
 
 	protected void createUniqueComboSp() {
 		addNextTargetPartitionForCreateWithIdDefaultPartition();
-		addNextTargetPartitionForReadDefaultPartition(); // one for search param validation
 		SearchParameter sp = new SearchParameter();
 		sp.setId("SearchParameter/patient-gender");
 		sp.setType(Enumerations.SearchParamType.TOKEN);
@@ -164,7 +163,6 @@ protected void createUniqueComboSp() {
 		mySearchParameterDao.update(sp, mySrd);
 
 		addNextTargetPartitionForCreateWithIdDefaultPartition();
-		addNextTargetPartitionForReadDefaultPartition(); // one for search param validation
 		sp = new SearchParameter();
 		sp.setId("SearchParameter/patient-family");
 		sp.setType(Enumerations.SearchParamType.STRING);
@@ -198,7 +196,6 @@ protected void createUniqueComboSp() {
 
 	protected void createNonUniqueComboSp() {
 		addNextTargetPartitionForCreateWithIdDefaultPartition();
-		addNextTargetPartitionForReadDefaultPartition(); // one for search param validation
 		SearchParameter sp = new SearchParameter();
 		sp.setId("SearchParameter/patient-family");
 		sp.setType(Enumerations.SearchParamType.STRING);
@@ -209,7 +206,6 @@ protected void createNonUniqueComboSp() {
 		mySearchParameterDao.update(sp, mySrd);
 
 		addNextTargetPartitionForCreateWithIdDefaultPartition();
-		addNextTargetPartitionForReadDefaultPartition(); // one for search param validation
 		sp = new SearchParameter();
 		sp.setId("SearchParameter/patient-managingorg");
 		sp.setType(Enumerations.SearchParamType.REFERENCE);
@@ -346,6 +342,11 @@ protected void addNextTargetPartitionForUpdate(int thePartitionId) {
 		addNextTargetPartitionForUpdate(fromPartitionId(thePartitionId));
 	}
 
+	protected RequestPartitionId withPartitionNames(String... thePartitionNames){
+		Validate.notNull(thePartitionNames);
+		Validate.isTrue(thePartitionNames.length > 0);
+		return fromPartitionNames(thePartitionNames);
+	}
 
 	protected void addNextTargetPartitionsForRead(Integer... thePartitionId) {
 		Validate.notNull(thePartitionId);
@@ -395,22 +396,9 @@ protected ICreationArgument withCreatePartition(Integer thePartitionId) {
 		};
 	}
 
-	protected ICreationArgument withReadWritePartitions(Integer thePartitionId) {
-		return t -> {
-			if (thePartitionId != null) {
-				addNextTargetPartitionsForRead(thePartitionId);
-				addNextTargetPartitionForCreate(thePartitionId, null);
-			} else {
-				addNextTargetPartitionForReadDefaultPartition();
-				addNextTargetPartitionForCreateDefaultPartition();
-			}
-		};
-	}
-
 	@Interceptor
 	public static class MyReadWriteInterceptor extends MyWriteInterceptor {
 
-
 		private final List<RequestPartitionId> myReadRequestPartitionIds = new ArrayList<>();
 
 		public void addNextIterceptorReadResult(RequestPartitionId theRequestPartitionId) {

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningNonNullDefaultPartitionR4Test.java

@@ -243,9 +243,7 @@ private Encounter createEncounter(IParser jsonParser) {
 	@Test
 	public void testCreateAndSearch_NonPartitionable() {
 		addNextTargetPartitionForCreateDefaultPartition();
-		// we need two read partition accesses for when the creation of the SP triggers a reindex of Patient
-		addNextTargetPartitionForReadDefaultPartition(); // one for search param validation
-		addNextTargetPartitionForReadDefaultPartition(); // and one for the reindex job
+
 		SearchParameter sp = new SearchParameter();
 		sp.addBase("Patient");
 		sp.setStatus(Enumerations.PublicationStatus.ACTIVE);
@@ -261,12 +259,10 @@ public void testCreateAndSearch_NonPartitionable() {
 		});
 
 		// Search on Token
-		addNextTargetPartitionForReadDefaultPartition();
 		List<String> outcome = toUnqualifiedVersionlessIdValues(mySearchParameterDao.search(SearchParameterMap.newSynchronous().add("code", new TokenParam("extpatorg")), mySrd));
 		assertThat(outcome).containsExactly("SearchParameter/" + id);
 
 		// Search on All Resources
-		addNextTargetPartitionForReadDefaultPartition();
 		outcome = toUnqualifiedVersionlessIdValues(mySearchParameterDao.search(SearchParameterMap.newSynchronous(), mySrd));
 		assertThat(outcome).containsExactly("SearchParameter/" + id);
 
@@ -275,8 +271,6 @@ public void testCreateAndSearch_NonPartitionable() {
 	@Test
 	public void testCreateAndSearch_NonPartitionable_ForcedId() {
 		addNextTargetPartitionForCreateWithIdDefaultPartition();
-		// we need two read partition accesses for when the creation of the SP triggers a reindex of Patient
-		addNextTargetPartitionForReadDefaultPartition(); // one for search param validation
 		addNextTargetPartitionForReadDefaultPartition(); // and one for the reindex job
 		SearchParameter sp = new SearchParameter();
 		sp.setId("SearchParameter/A");
@@ -294,12 +288,10 @@ public void testCreateAndSearch_NonPartitionable_ForcedId() {
 		});
 
 		// Search on Token
-		addNextTargetPartitionForReadDefaultPartition();
 		List<String> outcome = toUnqualifiedVersionlessIdValues(mySearchParameterDao.search(SearchParameterMap.newSynchronous().add("code", new TokenParam("extpatorg")), mySrd));
 		assertThat(outcome).containsExactly("SearchParameter/A");
 
 		// Search on All Resources
-		addNextTargetPartitionForReadDefaultPartition();
 		outcome = toUnqualifiedVersionlessIdValues(mySearchParameterDao.search(SearchParameterMap.newSynchronous(), mySrd));
 		assertThat(outcome).containsExactly("SearchParameter/A");
 

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/PartitioningSqlR4Test.java

@@ -137,9 +137,8 @@ public void afterEach() {
 
 	@Test
 	public void testCreateSearchParameter_DefaultPartition() {
+		addNextInterceptorCreateResult(defaultPartition());
 
-		addNextTargetPartitionForCreateDefaultPartition();
-		addNextTargetPartitionForReadDefaultPartition(); // one for search param validation
 		SearchParameter sp = new SearchParameter();
 		sp.addBase("Patient");
 		sp.setStatus(Enumerations.PublicationStatus.ACTIVE);
@@ -314,8 +313,7 @@ public void testCreate_SamePartitionReference_DefaultPartition_ByForcedId() {
 
 	@Test
 	public void testCreateSearchParameter_DefaultPartitionWithDate() {
-		addNextTargetPartitionForCreateDefaultPartition(myPartitionDate);
-		addNextTargetPartitionForReadDefaultPartition(); // one for search param validation
+		addNextInterceptorCreateResult(fromPartitionId(null, myPartitionDate));
 
 		SearchParameter sp = new SearchParameter();
 		sp.addBase("Patient");
@@ -339,7 +337,7 @@ public void testCreateSearchParameter_DefaultPartitionWithDate() {
 
 	@Test
 	public void testCreateSearchParameter_NonDefaultPartition() {
-		addNextTargetPartitionForCreate(myPartitionId, myPartitionDate);
+		addNextInterceptorCreateResult(fromPartitionId(myPartitionId, myPartitionDate));
 
 		SearchParameter sp = new SearchParameter();
 		sp.addBase("Patient");
@@ -963,7 +961,7 @@ public void testRead_PidId_MultiplePartitionNames() {
 		{
 			myCaptureQueriesListener.clear();
 			assertNoRemainingPartitionIds();
-			myPartitionInterceptor.addNextIterceptorReadResult(RequestPartitionId.fromPartitionNames(PARTITION_1, PARTITION_2));
+			addNextInterceptorReadResult(withPartitionNames(PARTITION_1, PARTITION_2));
 			IdType gotId1 = myPatientDao.read(patientId1, mySrd).getIdElement().toUnqualifiedVersionless();
 			assertEquals(patientId1, gotId1);
 
@@ -977,7 +975,8 @@ public void testRead_PidId_MultiplePartitionNames() {
 		// Two partitions including default - Found
 		{
 			myCaptureQueriesListener.clear();
-			myPartitionInterceptor.addNextIterceptorReadResult(RequestPartitionId.fromPartitionNames(PARTITION_1, JpaConstants.DEFAULT_PARTITION_NAME));
+			addNextInterceptorReadResult(withPartitionNames(PARTITION_1, JpaConstants.DEFAULT_PARTITION_NAME));
+
 			IdType gotId1;
 			try {
 				gotId1 = myPatientDao.read(patientIdNull, mySrd).getIdElement().toUnqualifiedVersionless();
@@ -996,15 +995,15 @@ public void testRead_PidId_MultiplePartitionNames() {
 
 		// Two partitions - Not Found
 		{
-			myPartitionInterceptor.addNextIterceptorReadResult(RequestPartitionId.fromPartitionNames(PARTITION_1, PARTITION_2));
+			addNextInterceptorReadResult(withPartitionNames(PARTITION_1, PARTITION_2));
 			try {
 				myPatientDao.read(patientId3, mySrd);
 				fail();
 			} catch (ResourceNotFoundException e) {
 				// good
 			}
 
-			myPartitionInterceptor.addNextIterceptorReadResult(RequestPartitionId.fromPartitionNames(PARTITION_1, PARTITION_2));
+			addNextInterceptorReadResult(withPartitionNames(PARTITION_1, PARTITION_2));
 			try {
 				myPatientDao.read(patientIdNull, mySrd);
 				fail();

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/partition/RequestPartitionHelperSvcTest.java

@@ -5,8 +5,10 @@
 import ca.uhn.fhir.jpa.entity.PartitionEntity;
 import ca.uhn.fhir.jpa.model.config.PartitionSettings;
 import ca.uhn.fhir.jpa.test.BaseJpaR4Test;
+import ca.uhn.fhir.rest.api.server.RequestDetails;
 import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
 import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException;
+import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails;
 import org.hl7.fhir.r4.model.IdType;
 import org.hl7.fhir.r4.model.Patient;
 import org.junit.jupiter.api.AfterEach;
@@ -40,6 +42,8 @@ class RequestPartitionHelperSvcTest extends BaseJpaR4Test {
 	static final int UNKNOWN_PARTITION_ID = 1_000_000;
 	static final String UNKNOWN_PARTITION_NAME = "UNKNOWN";
 
+	static final Integer DEFAULT_PARTITION_ID = null;
+
 	@Autowired
 	IPartitionDao myPartitionDao;
 	@Autowired
@@ -79,6 +83,34 @@ public void testDetermineReadPartitionForSystemRequest_withPartitionIdOnly_retur
 		assertEquals(PARTITION_NAME_1, result.getFirstPartitionNameOrNull());
 	}
 
+
+	@Test
+	public void testDetermineReadPartitionForServletRequest_whenResourceIsNonPartitionable() {
+		PartitionEntity partition1 = createPartition1();
+		ServletRequestDetails servletRequestDetails = new ServletRequestDetails();
+		servletRequestDetails.setTenantId(partition1.getName());
+
+		testDetermineReadPartitionRequest_whenResourceIsNonPartitionable_returnsDefaultPartition(servletRequestDetails);
+	}
+
+	@Test
+	public void testDetermineReadPartitionForSystemRequest_whenResourceIsNonPartitionable() {
+		PartitionEntity partitionEntity = createPartition1();
+		SystemRequestDetails srd = new SystemRequestDetails();
+		srd.setTenantId(partitionEntity.getName());
+		srd.setRequestPartitionId(RequestPartitionId.fromPartitionId(partitionEntity.getId()));
+
+		testDetermineReadPartitionRequest_whenResourceIsNonPartitionable_returnsDefaultPartition(srd);
+	}
+
+	public void testDetermineReadPartitionRequest_whenResourceIsNonPartitionable_returnsDefaultPartition(RequestDetails theDetails) {
+		// execute
+		RequestPartitionId result = mySvc.determineReadPartitionForRequestForSearchType(theDetails, "Library");
+
+		// verify
+		assertThat(result.hasDefaultPartitionId(DEFAULT_PARTITION_ID)).isTrue();
+	}
+
 	@Test
 	public void testDetermineCreatePartitionForRequest_withPartitionIdOnly_returnsCorrectPartition() {
 		// setup

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/MultitenantBatchOperationR4Test.java

@@ -18,7 +18,6 @@
 import org.hl7.fhir.instance.model.api.IIdType;
 import org.hl7.fhir.r4.hapi.rest.server.helper.BatchHelperR4;
 import org.hl7.fhir.r4.model.Bundle;
-import org.hl7.fhir.r4.model.IdType;
 import org.hl7.fhir.r4.model.Observation;
 import org.hl7.fhir.r4.model.Parameters;
 import org.hl7.fhir.r4.model.StringType;

hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/partition/BaseRequestPartitionHelperSvc.java

@@ -120,20 +120,19 @@ public RequestPartitionId determineReadPartitionForRequest(
 		boolean nonPartitionableResource = isResourceNonPartitionable(resourceType);
 
 		RequestPartitionId requestPartitionId = null;
-		// Handle system requests
-		if (requestDetails instanceof SystemRequestDetails
-				&& systemRequestHasExplicitPartition((SystemRequestDetails) requestDetails)
-				&& !nonPartitionableResource) {
-			requestPartitionId = getSystemRequestPartitionId((SystemRequestDetails) requestDetails, false);
-			logSystemRequestDetailsResolution((SystemRequestDetails) requestDetails);
-
-		} else if ((requestDetails instanceof SystemRequestDetails) && nonPartitionableResource) {
+
+		// FIXME: EHP -> this block is the 'good' block but test for it are failing. see the next fixme.
+		if (nonPartitionableResource) {
 			requestPartitionId = myPartitionSettings.getDefaultRequestPartitionId();
-			logSystemRequestDetailsResolution((SystemRequestDetails) requestDetails);
+			logRequestDetailsResolution(requestDetails);
 			logNonPartitionableType(resourceType);
+		} else if (requestDetails instanceof SystemRequestDetails systemRequestDetails
+				&& systemRequestHasExplicitPartition(systemRequestDetails)) {
+			// !nonPartitionableResource
+			requestPartitionId = getSystemRequestPartitionId(systemRequestDetails, false);
+			logRequestDetailsResolution(systemRequestDetails);
+
 		} else {
-			// TODO mb: why is this path different than create?
-			//  Here, a non-partitionable resource is still delivered to the pointcuts.
 			IInterceptorBroadcaster compositeBroadcaster =
 					CompositeInterceptorBroadcaster.newCompositeBroadcaster(myInterceptorBroadcaster, requestDetails);
 			if (compositeBroadcaster.hasHooks(Pointcut.STORAGE_PARTITION_IDENTIFY_ANY)) {
@@ -143,6 +142,29 @@ && systemRequestHasExplicitPartition((SystemRequestDetails) requestDetails)
 			}
 		}
 
+		// FIXME: EHP -> the next block from the if to the end of the else should be removed.  it is there only to be added and
+		// commented out in troubleshooting efforts
+//		if (requestDetails instanceof SystemRequestDetails
+//			&& systemRequestHasExplicitPartition((SystemRequestDetails) requestDetails)
+//			&& !nonPartitionableResource) {
+//			requestPartitionId = getSystemRequestPartitionId((SystemRequestDetails) requestDetails, false);
+//			logRequestDetailsResolution((SystemRequestDetails) requestDetails);
+//
+//		} else if ((requestDetails instanceof SystemRequestDetails) && nonPartitionableResource) {
+//			requestPartitionId = myPartitionSettings.getDefaultRequestPartitionId();
+//			logRequestDetailsResolution((SystemRequestDetails) requestDetails);
+//			logNonPartitionableType(resourceType);
+//		} else {
+//			// TODO mb: why is this path different than create?
+//			//  Here, a non-partitionable resource is still delivered to the pointcuts.
+//			IInterceptorBroadcaster compositeBroadcaster =
+//				CompositeInterceptorBroadcaster.newCompositeBroadcaster(myInterceptorBroadcaster, requestDetails);
+//			if (compositeBroadcaster.hasHooks(Pointcut.STORAGE_PARTITION_IDENTIFY_ANY)) {
+//				requestPartitionId = callAnyPointcut(compositeBroadcaster, requestDetails);
+//			} else if (compositeBroadcaster.hasHooks(Pointcut.STORAGE_PARTITION_IDENTIFY_READ)) {
+//				requestPartitionId = callReadPointcut(compositeBroadcaster, requestDetails, theDetails);
+//			}
+//		}
 		validateRequestPartitionNotNull(
 				requestPartitionId, Pointcut.STORAGE_PARTITION_IDENTIFY_ANY, Pointcut.STORAGE_PARTITION_IDENTIFY_READ);
 
@@ -213,10 +235,10 @@ public RequestPartitionId determineGenericPartitionForRequest(RequestDetails the
 			return RequestPartitionId.allPartitions();
 		}
 
-		if (theRequestDetails instanceof SystemRequestDetails
-				&& systemRequestHasExplicitPartition((SystemRequestDetails) theRequestDetails)) {
-			requestPartitionId = getSystemRequestPartitionId((SystemRequestDetails) theRequestDetails);
-			logSystemRequestDetailsResolution((SystemRequestDetails) theRequestDetails);
+		if (theRequestDetails instanceof SystemRequestDetails systemRequestDetails
+				&& systemRequestHasExplicitPartition(systemRequestDetails)) {
+			requestPartitionId = getSystemRequestPartitionId(systemRequestDetails);
+			logRequestDetailsResolution(systemRequestDetails);
 		} else {
 			IInterceptorBroadcaster compositeBroadcaster = CompositeInterceptorBroadcaster.newCompositeBroadcaster(
 					myInterceptorBroadcaster, theRequestDetails);
@@ -303,12 +325,12 @@ public RequestPartitionId determineCreatePartitionForRequest(
 		}
 
 		RequestPartitionId requestPartitionId = null;
-		if (theRequest instanceof SystemRequestDetails
-				&& systemRequestHasExplicitPartition((SystemRequestDetails) theRequest)) {
-			requestPartitionId =
-					getSystemRequestPartitionId((SystemRequestDetails) theRequest, nonPartitionableResource);
 
-			logSystemRequestDetailsResolution((SystemRequestDetails) theRequest);
+		if (theRequest instanceof SystemRequestDetails systemRequestDetails
+				&& systemRequestHasExplicitPartition(systemRequestDetails)) {
+			requestPartitionId = getSystemRequestPartitionId(systemRequestDetails, nonPartitionableResource);
+
+			logRequestDetailsResolution(systemRequestDetails);
 		} else {
 			IInterceptorBroadcaster compositeBroadcaster =
 					CompositeInterceptorBroadcaster.newCompositeBroadcaster(myInterceptorBroadcaster, requestDetails);
@@ -492,10 +514,19 @@ private static void logTroubleshootingResult(
 				theResult);
 	}
 
-	private void logSystemRequestDetailsResolution(SystemRequestDetails theRequest) {
-		ourLog.trace(
-				"Partitioning: request is a SystemRequestDetails, with RequestPartitionId={}.",
-				theRequest.getRequestPartitionId());
+	private void logRequestDetailsResolution(RequestDetails theRequest) {
+		String logString = "Partitioning: request is a {}, with tenantId {}";
+
+		String requestClassName = theRequest.getClass().getSimpleName();
+		String tenantId = theRequest.getTenantId();
+		RequestPartitionId requestPartitionId = null;
+
+		if (theRequest instanceof SystemRequestDetails systemRequestDetails) {
+			requestPartitionId = systemRequestDetails.getRequestPartitionId();
+			logString = logString + ", with request partitionId {}";
+		}
+
+		ourLog.trace(logString, requestClassName, tenantId, requestPartitionId);
 	}
 
 	private static void logSubstitutingDefaultSystemRequestDetails() {