Fix NPE in Storage with Mass Ingestion and Compartment Interceptor

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6599-fix-NPE-in-storage-module.yaml

@@ -0,0 +1,7 @@
+---
+type: fix
+issue: 6599
+jira: SMILE-9237
+title: "Previously, attempting to restart the Storage module would result in a `NullPointerException` when partition 
+selection mode was set to `PATIENT_ID`, mass ingestion was enabled, and at least one Search Parameter was disabled. This 
+has now been fixed."

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

@@ -29,6 +29,7 @@
 import ca.uhn.fhir.interceptor.api.HookParams;
 import ca.uhn.fhir.interceptor.api.IInterceptorBroadcaster;
 import ca.uhn.fhir.interceptor.api.Pointcut;
+import ca.uhn.fhir.interceptor.executor.InterceptorService;
 import ca.uhn.fhir.interceptor.model.RequestPartitionId;
 import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
 import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
@@ -47,6 +48,7 @@
 import ca.uhn.fhir.jpa.dao.data.IResourceHistoryProvenanceDao;
 import ca.uhn.fhir.jpa.dao.tx.HapiTransactionService;
 import ca.uhn.fhir.jpa.delete.DeleteConflictUtil;
+import ca.uhn.fhir.jpa.interceptor.PatientCompartmentEnforcingInterceptor;
 import ca.uhn.fhir.jpa.model.cross.IBasePersistedResource;
 import ca.uhn.fhir.jpa.model.dao.JpaPid;
 import ca.uhn.fhir.jpa.model.entity.BaseHasResource;
@@ -72,6 +74,7 @@
 import ca.uhn.fhir.jpa.searchparam.MatchUrlService;
 import ca.uhn.fhir.jpa.searchparam.ResourceSearch;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
+import ca.uhn.fhir.jpa.update.UpdateParameters;
 import ca.uhn.fhir.jpa.util.MemoryCacheService;
 import ca.uhn.fhir.jpa.util.QueryChunker;
 import ca.uhn.fhir.model.api.IQueryParameterType;
@@ -2492,16 +2495,19 @@ && getStorageSettings().getResourceServerIdStrategy()
 
 		// Start
 		if (outcome == null) {
-			outcome = doUpdateForUpdateOrPatch(
-					theRequest,
-					resourceId,
-					theMatchUrl,
-					thePerformIndexing,
-					theForceUpdateVersion,
-					theResource,
-					entity,
-					update,
-					theTransactionDetails);
+			UpdateParameters updateParameters = new UpdateParameters<>()
+					.setRequestDetails(theRequest)
+					.setResourceIdToUpdate(resourceId)
+					.setMatchUrl(theMatchUrl)
+					.setShouldPerformIndexing(thePerformIndexing)
+					.setShouldForceUpdateVersion(theForceUpdateVersion)
+					.setResource(theResource)
+					.setEntity(entity)
+					.setOperationType(update)
+					.setTransactionDetails(theTransactionDetails)
+					.setShouldForcePopulateOldResourceForProcessing(false);
+
+			outcome = doUpdateForUpdateOrPatch(updateParameters);
 		}
 
 		postUpdateTransaction(theTransactionDetails);
@@ -2523,23 +2529,14 @@ protected void postUpdateTransaction(TransactionDetails theTransactionDetails) {
 	}
 
 	@Override
-	protected DaoMethodOutcome doUpdateForUpdateOrPatch(
-			RequestDetails theRequest,
-			IIdType theResourceId,
-			String theMatchUrl,
-			boolean thePerformIndexing,
-			boolean theForceUpdateVersion,
-			T theResource,
-			IBasePersistedResource theEntity,
-			RestOperationTypeEnum theOperationType,
-			TransactionDetails theTransactionDetails) {
+	protected DaoMethodOutcome doUpdateForUpdateOrPatch(UpdateParameters<T> theUpdateParameters) {
 
 		/*
 		 * We stored a resource searchUrl at creation time to prevent resource duplication.
 		 * We'll clear any currently existing urls from the db, otherwise we could hit
 		 * duplicate index violations if we try to add another (after this create/update)
 		 */
-		ResourceTable entity = (ResourceTable) theEntity;
+		ResourceTable entity = (ResourceTable) theUpdateParameters.getEntity();
 
 		/*
 		 * If we read back the entity from hibernate, and it's already saying
@@ -2555,7 +2552,7 @@ protected DaoMethodOutcome doUpdateForUpdateOrPatch(
 
 		if (entity.isSearchUrlPresent()) {
 			JpaPid persistentId = entity.getResourceId();
-			theTransactionDetails.addUpdatedResourceId(persistentId);
+			theUpdateParameters.getTransactionDetails().addUpdatedResourceId(persistentId);
 
 			entity.setSearchUrlPresent(false); // it will be removed at the end
 		}
@@ -2572,16 +2569,11 @@ protected DaoMethodOutcome doUpdateForUpdateOrPatch(
 					null);
 		}
 
-		return super.doUpdateForUpdateOrPatch(
-				theRequest,
-				theResourceId,
-				theMatchUrl,
-				thePerformIndexing,
-				theForceUpdateVersion,
-				theResource,
-				theEntity,
-				theOperationType,
-				theTransactionDetails);
+		boolean shouldForcePopulateOldResourceForProcessing = myInterceptorBroadcaster instanceof InterceptorService
+				&& ((InterceptorService) myInterceptorBroadcaster)
+						.hasRegisteredInterceptor(PatientCompartmentEnforcingInterceptor.class);
+		theUpdateParameters.setShouldForcePopulateOldResourceForProcessing(shouldForcePopulateOldResourceForProcessing);
+		return super.doUpdateForUpdateOrPatch(theUpdateParameters);
 	}
 
 	/**

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

@@ -1,5 +1,9 @@
 package ca.uhn.fhir.jpa.interceptor;
 
+import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
+
+import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
+
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import ca.uhn.fhir.jpa.model.config.PartitionSettings;
 import ca.uhn.fhir.jpa.provider.BaseResourceProviderR4Test;
@@ -11,10 +15,10 @@
 import org.hl7.fhir.r4.model.Patient;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.springframework.beans.factory.annotation.Autowired;
 
-import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
 public class PatientCompartmentEnforcingInterceptorTest extends BaseResourceProviderR4Test {
@@ -56,11 +60,14 @@ public void after() throws Exception {
 		myPartitionSettings.setUnnamedPartitionMode(defaultPartitionSettings.isUnnamedPartitionMode());
 		myPartitionSettings.setDefaultPartitionId(defaultPartitionSettings.getDefaultPartitionId());
 		myPartitionSettings.setAllowReferencesAcrossPartitions(defaultPartitionSettings.getAllowReferencesAcrossPartitions());
-	}
 
+		myStorageSettings.setMassIngestionMode(false);
+	}
 
-	@Test
-	public void testUpdateResource_whenCrossingPatientCompartment_throws() {
+	@ParameterizedTest
+	@ValueSource(booleans = {true, false})
+	public void testUpdateResource_whenCrossingPatientCompartment_throws(boolean theMassIngestionEnabled) {
+		myStorageSettings.setMassIngestionMode(theMassIngestionEnabled);
 		myPartitionSettings.setAllowReferencesAcrossPartitions(PartitionSettings.CrossPartitionReferenceMode.ALLOWED_UNQUALIFIED);
 		createPatientA();
 		createPatientB();
@@ -76,8 +83,10 @@ public void testUpdateResource_whenCrossingPatientCompartment_throws() {
 		assertEquals("HAPI-2476: Resource compartment changed. Was a referenced Patient changed?", thrown.getMessage());
 	}
 
-	@Test
-	public void testUpdateResource_whenNotCrossingPatientCompartment_allows() {
+	@ParameterizedTest
+	@ValueSource(booleans = {true, false})
+	public void testUpdateResource_whenNotCrossingPatientCompartment_allows(boolean theMassIngestionEnabled) {
+		myStorageSettings.setMassIngestionMode(theMassIngestionEnabled);
 		createPatientA();
 
 		Observation obs = new Observation();
@@ -87,7 +96,8 @@ public void testUpdateResource_whenNotCrossingPatientCompartment_allows() {
 		obs.getNote().add(new Annotation().setText("some text"));
 		obs.setStatus(Observation.ObservationStatus.CORRECTED);
 
-		myObservationDao.update(obs, new SystemRequestDetails());
+		DaoMethodOutcome outcome = myObservationDao.update(obs, new SystemRequestDetails());
+		assertEquals("Patient/A", ((Observation) outcome.getResource()).getSubject().getReference());
 	}
 
 	private void createPatientA() {

hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseStorageResourceDao.java

@@ -29,6 +29,7 @@
 import ca.uhn.fhir.jpa.patch.FhirPatch;
 import ca.uhn.fhir.jpa.patch.JsonPatchUtils;
 import ca.uhn.fhir.jpa.patch.XmlPatchUtils;
+import ca.uhn.fhir.jpa.update.UpdateParameters;
 import ca.uhn.fhir.parser.StrictErrorHandler;
 import ca.uhn.fhir.rest.api.DeleteCascadeModeEnum;
 import ca.uhn.fhir.rest.api.PatchTypeEnum;
@@ -188,16 +189,19 @@ public DaoMethodOutcome patchInTransaction(
 
 		preProcessResourceForStorage(destinationCasted, theRequestDetails, theTransactionDetails, true);
 
-		return doUpdateForUpdateOrPatch(
-				theRequestDetails,
-				resourceId,
-				theConditionalUrl,
-				thePerformIndexing,
-				false,
-				destinationCasted,
-				entityToUpdate,
-				RestOperationTypeEnum.PATCH,
-				theTransactionDetails);
+		UpdateParameters updateParameters = new UpdateParameters<>()
+				.setRequestDetails(theRequestDetails)
+				.setResourceIdToUpdate(resourceId)
+				.setMatchUrl(theConditionalUrl)
+				.setShouldPerformIndexing(thePerformIndexing)
+				.setShouldForceUpdateVersion(false)
+				.setResource(destinationCasted)
+				.setEntity(entityToUpdate)
+				.setOperationType(RestOperationTypeEnum.PATCH)
+				.setTransactionDetails(theTransactionDetails)
+				.setShouldForcePopulateOldResourceForProcessing(false);
+
+		return doUpdateForUpdateOrPatch(updateParameters);
 	}
 
 	@Override
@@ -216,33 +220,32 @@ protected abstract IBasePersistedResource readEntityLatestVersion(
 	protected abstract IBasePersistedResource readEntityLatestVersion(
 			IIdType theId, RequestDetails theRequestDetails, TransactionDetails theTransactionDetails);
 
-	protected DaoMethodOutcome doUpdateForUpdateOrPatch(
-			RequestDetails theRequest,
-			IIdType theResourceId,
-			String theMatchUrl,
-			boolean thePerformIndexing,
-			boolean theForceUpdateVersion,
-			T theResource,
-			IBasePersistedResource theEntity,
-			RestOperationTypeEnum theOperationType,
-			TransactionDetails theTransactionDetails) {
-		if (theResourceId.hasVersionIdPart()
-				&& Long.parseLong(theResourceId.getVersionIdPart()) != theEntity.getVersion()) {
-			throw new ResourceVersionConflictException(
-					Msg.code(989) + "Trying to update " + theResourceId + " but this is not the current version");
+	protected DaoMethodOutcome doUpdateForUpdateOrPatch(UpdateParameters<T> theUpdateParameters) {
+
+		if (theUpdateParameters.getResourceIdToUpdate().hasVersionIdPart()
+				&& Long.parseLong(theUpdateParameters.getResourceIdToUpdate().getVersionIdPart())
+						!= theUpdateParameters.getEntity().getVersion()) {
+			throw new ResourceVersionConflictException(Msg.code(989) + "Trying to update "
+					+ theUpdateParameters.getResourceIdToUpdate() + " but this is not the current version");
 		}
 
-		if (theResourceId.hasResourceType() && !theResourceId.getResourceType().equals(getResourceName())) {
+		if (theUpdateParameters.getResourceIdToUpdate().hasResourceType()
+				&& !theUpdateParameters
+						.getResourceIdToUpdate()
+						.getResourceType()
+						.equals(getResourceName())) {
 			throw new UnprocessableEntityException(Msg.code(990) + "Invalid resource ID["
-					+ theEntity.getIdDt().toUnqualifiedVersionless() + "] of type[" + theEntity.getResourceType()
+					+ theUpdateParameters.getEntity().getIdDt().toUnqualifiedVersionless() + "] of type["
+					+ theUpdateParameters.getEntity().getResourceType()
 					+ "] - Does not match expected [" + getResourceName() + "]");
 		}
 
 		IBaseResource oldResource;
-		if (getStorageSettings().isMassIngestionMode()) {
+		if (getStorageSettings().isMassIngestionMode()
+				&& !theUpdateParameters.shouldForcePopulateOldResourceForProcessing()) {
 			oldResource = null;
 		} else {
-			oldResource = getStorageResourceParser().toResource(theEntity, false);
+			oldResource = getStorageResourceParser().toResource(theUpdateParameters.getEntity(), false);
 		}
 
 		/*
@@ -256,19 +259,25 @@ protected DaoMethodOutcome doUpdateForUpdateOrPatch(
 		 * See SystemProviderR4Test#testTransactionReSavesPreviouslyDeletedResources
 		 * for a test that needs this.
 		 */
-		boolean wasDeleted = theEntity.isDeleted();
-		theEntity.setNotDeleted();
+		boolean wasDeleted = theUpdateParameters.getEntity().isDeleted();
+		theUpdateParameters.getEntity().setNotDeleted();
 
 		/*
 		 * If we aren't indexing, that means we're doing this inside a transaction.
 		 * The transaction will do the actual storage to the database a bit later on,
 		 * after placeholder IDs have been replaced, by calling {@link #updateInternal}
 		 * directly. So we just bail now.
 		 */
-		if (!thePerformIndexing) {
-			theResource.setId(theEntity.getIdDt().getValue());
+		if (!theUpdateParameters.shouldPerformIndexing()) {
+			theUpdateParameters
+					.getResource()
+					.setId(theUpdateParameters.getEntity().getIdDt().getValue());
 			DaoMethodOutcome outcome = toMethodOutcome(
-							theRequest, theEntity, theResource, theMatchUrl, theOperationType)
+							theUpdateParameters.getRequest(),
+							theUpdateParameters.getEntity(),
+							theUpdateParameters.getResource(),
+							theUpdateParameters.getMatchUrl(),
+							theUpdateParameters.getOperationType())
 					.setCreated(wasDeleted);
 			outcome.setPreviousResource(oldResource);
 			if (!outcome.isNop()) {
@@ -284,16 +293,16 @@ protected DaoMethodOutcome doUpdateForUpdateOrPatch(
 		 * Otherwise, we're not in a transaction
 		 */
 		return updateInternal(
-				theRequest,
-				theResource,
-				theMatchUrl,
-				thePerformIndexing,
-				theForceUpdateVersion,
-				theEntity,
-				theResourceId,
+				theUpdateParameters.getRequest(),
+				theUpdateParameters.getResource(),
+				theUpdateParameters.getMatchUrl(),
+				theUpdateParameters.shouldPerformIndexing(),
+				theUpdateParameters.shouldForceUpdateVersion(),
+				theUpdateParameters.getEntity(),
+				theUpdateParameters.getResourceIdToUpdate(),
 				oldResource,
-				theOperationType,
-				theTransactionDetails);
+				theUpdateParameters.getOperationType(),
+				theUpdateParameters.getTransactionDetails());
 	}
 
 	public static void validateResourceType(IBasePersistedResource<?> theEntity, String theResourceName) {