Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NPE in Storage with Mass Ingestion and Compartment Interceptor #6598

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+69 −10
Open

Fix NPE in Storage with Mass Ingestion and Compartment Interceptor #6598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4046a20
add tests and fix mass ingestion and patient compartment interceptor …
Jan 7, 2025
d79ef1a
changelogs
Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ...s/src/main/resources/ca/uhn/hapi/fhir/changelog/7_8_0/6599-fix-NPE-in-storage-module.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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."
8 changes: 7 additions & 1 deletion hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -2572,6 +2574,9 @@ protected DaoMethodOutcome doUpdateForUpdateOrPatch(
null);
}

boolean theShouldForcePopulateOldResourceForProcessing = myInterceptorBroadcaster instanceof InterceptorService
&& ((InterceptorService) myInterceptorBroadcaster)
.hasRegisteredInterceptor(PatientCompartmentEnforcingInterceptor.class);
return super.doUpdateForUpdateOrPatch(
theRequest,
theResourceId,
Expand All @@ -2581,7 +2586,8 @@ protected DaoMethodOutcome doUpdateForUpdateOrPatch(
theResource,
theEntity,
theOperationType,
theTransactionDetails);
theTransactionDetails,
theShouldForcePopulateOldResourceForProcessing);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're at 11 parameters here.

We probably should've wrapped these in a parameters object 9 parameters ago.

What would be the impact of doing so now?

}

/**
Expand Down
26 changes: 18 additions & 8 deletions ...src/test/java/ca/uhn/fhir/jpa/interceptor/PatientCompartmentEnforcingInterceptorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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 {
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand All @@ -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() {
Expand Down
26 changes: 25 additions & 1 deletion hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/BaseStorageResourceDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,30 @@ protected DaoMethodOutcome doUpdateForUpdateOrPatch(
IBasePersistedResource theEntity,
RestOperationTypeEnum theOperationType,
TransactionDetails theTransactionDetails) {
return doUpdateForUpdateOrPatch(
theRequest,
theResourceId,
theMatchUrl,
thePerformIndexing,
theForceUpdateVersion,
theResource,
theEntity,
theOperationType,
theTransactionDetails,
false);
}

protected DaoMethodOutcome doUpdateForUpdateOrPatch(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This multiple method thing is exactly why i think we should make a new parameters object and have that API be the 'main' one.

Multiple overrides and multiple methods will lead to funny bugs if people make methods with different 'combinations' of parameters.

RequestDetails theRequest,
IIdType theResourceId,
String theMatchUrl,
boolean thePerformIndexing,
boolean theForceUpdateVersion,
T theResource,
IBasePersistedResource theEntity,
RestOperationTypeEnum theOperationType,
TransactionDetails theTransactionDetails,
boolean theShouldForcePopulateOldResourceForProcessing) {
if (theResourceId.hasVersionIdPart()
&& Long.parseLong(theResourceId.getVersionIdPart()) != theEntity.getVersion()) {
throw new ResourceVersionConflictException(
Expand All @@ -239,7 +263,7 @@ protected DaoMethodOutcome doUpdateForUpdateOrPatch(
}

IBaseResource oldResource;
if (getStorageSettings().isMassIngestionMode()) {
if (getStorageSettings().isMassIngestionMode() && !theShouldForcePopulateOldResourceForProcessing) {
oldResource = null;
} else {
oldResource = getStorageResourceParser().toResource(theEntity, false);
Expand Down
5 changes: 5 additions & 0 deletions hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/util/ResourceCompartmentUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ public class ResourceCompartmentUtil {
*/
public static Optional<String> getPatientCompartmentIdentity(
IBaseResource theResource, FhirContext theFhirContext, ISearchParamExtractor theSearchParamExtractor) {
if (theResource == null) {
// The resource may be null in mass ingestion mode
return Optional.empty();
}

RuntimeResourceDefinition resourceDef = theFhirContext.getResourceDefinition(theResource);
List<RuntimeSearchParam> patientCompartmentSps =
ResourceCompartmentUtil.getPatientCompartmentSearchParams(resourceDef);
Expand Down
7 changes: 7 additions & 0 deletions hapi-fhir-storage/src/test/java/ca/uhn/fhir/jpa/util/ResourceCompartmentUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,13 @@ void whenNoPatientResource_returnsPatientCompartment() {
assertThat(result).contains("P01");
// }
}

@Test
void nullResource_shouldNotThrowNPE() {
// The input resource may be null when mass ingestion is enabled.
Optional<String> result = ResourceCompartmentUtil.getPatientCompartmentIdentity(null, myFhirContext, mySearchParamExtractor);
assertThat(result).isEmpty();
}
}

private List<RuntimeSearchParam> getMockSearchParams(boolean providePatientCompartment) {
Expand Down
Loading