Add test and implementation to fix potential NPE in pre-show resources (#4388)

* Add test and implementation to fix potential NPE in pre-show resources

* add test

* WIP getting identical test scenario

* More robust solution

* Finalize Code

* Add changelog, move a bunch of changelogs

* Remove not needed test

* Minor refactor and reporting

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_4_0/4388-npe-in-binary-storage.yaml

@@ -0,0 +1,5 @@
+---
+type: fix
+issue: 4388
+jira: SMILE-5834
+title: "Fixed an edge case during a Read operation where hooks could be invoked with a null resource. This could cause a NullPointerException in some cases."

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

@@ -1173,36 +1173,42 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 				throw createResourceGoneException(entity);
 			}
 		}
-
-		// Interceptor broadcast: STORAGE_PREACCESS_RESOURCES
-		{
-			SimplePreResourceAccessDetails accessDetails = new SimplePreResourceAccessDetails(retVal);
-			HookParams params = new HookParams()
-				.add(IPreResourceAccessDetails.class, accessDetails)
-				.add(RequestDetails.class, theRequest)
-				.addIfMatchesType(ServletRequestDetails.class, theRequest);
-			CompositeInterceptorBroadcaster.doCallHooks(myInterceptorBroadcaster, theRequest, Pointcut.STORAGE_PREACCESS_RESOURCES, params);
-			if (accessDetails.isDontReturnResourceAtIndex(0)) {
-				throw new ResourceNotFoundException(Msg.code(1995) + "Resource " + theId + " is not known");
-			}
-		}
-
-		// Interceptor broadcast: STORAGE_PRESHOW_RESOURCES
-		{
-			SimplePreResourceShowDetails showDetails = new SimplePreResourceShowDetails(retVal);
-			HookParams params = new HookParams()
-				.add(IPreResourceShowDetails.class, showDetails)
-				.add(RequestDetails.class, theRequest)
-				.addIfMatchesType(ServletRequestDetails.class, theRequest);
-			CompositeInterceptorBroadcaster.doCallHooks(myInterceptorBroadcaster, theRequest, Pointcut.STORAGE_PRESHOW_RESOURCES, params);
-			//noinspection unchecked
-			retVal = (T) showDetails.getResource(0);
+		//If the resolved fhir model is null, we don't need to run pre-access over or pre-show over it.
+		if (retVal != null) {
+			invokeStoragePreaccessResources(theId, theRequest, retVal);
+			retVal = invokeStoragePreShowResources(theRequest, retVal);
 		}
 
 		ourLog.debug("Processed read on {} in {}ms", theId.getValue(), w.getMillisAndRestart());
 		return retVal;
 	}
 
+	private T invokeStoragePreShowResources(RequestDetails theRequest, T retVal) {
+		// Interceptor broadcast: STORAGE_PRESHOW_RESOURCES
+		SimplePreResourceShowDetails showDetails = new SimplePreResourceShowDetails(retVal);
+		HookParams params = new HookParams()
+			.add(IPreResourceShowDetails.class, showDetails)
+			.add(RequestDetails.class, theRequest)
+			.addIfMatchesType(ServletRequestDetails.class, theRequest);
+		CompositeInterceptorBroadcaster.doCallHooks(myInterceptorBroadcaster, theRequest, Pointcut.STORAGE_PRESHOW_RESOURCES, params);
+		//noinspection unchecked
+		retVal = (T) showDetails.getResource(0);//TODO GGG/JA : getting resource 0 is interesting. We apparently allow null values in the list. Should we?
+		return retVal;
+	}
+
+	private void invokeStoragePreaccessResources(IIdType theId, RequestDetails theRequest, T theResource) {
+		// Interceptor broadcast: STORAGE_PREACCESS_RESOURCES
+		SimplePreResourceAccessDetails accessDetails = new SimplePreResourceAccessDetails(theResource);
+		HookParams params = new HookParams()
+			.add(IPreResourceAccessDetails.class, accessDetails)
+			.add(RequestDetails.class, theRequest)
+			.addIfMatchesType(ServletRequestDetails.class, theRequest);
+		CompositeInterceptorBroadcaster.doCallHooks(myInterceptorBroadcaster, theRequest, Pointcut.STORAGE_PREACCESS_RESOURCES, params);
+		if (accessDetails.isDontReturnResourceAtIndex(0)) {
+			throw new ResourceNotFoundException(Msg.code(1995) + "Resource " + theId + " is not known");
+		}
+	}
+
 	@Override
 	@Transactional
 	public BaseHasResource readEntity(IIdType theId, RequestDetails theRequest) {

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

@@ -31,7 +31,8 @@ import java.util.Collection;
 public interface IJpaStorageResourceParser extends IStorageResourceParser {
 
 	/**
-	 * Convert a storage entity into a FHIR resource model instance
+	 * Convert a storage entity into a FHIR resource model instance. This method may return null if the entity is not
+	 * completely flushed, including the entities history entries.
 	 */
 	<R extends IBaseResource> R toResource(Class<R> theResourceType, IBaseResourceEntity theEntity, Collection<ResourceTag> theTagList, boolean theForHistoryOperation);
 
@@ -43,7 +44,7 @@ public interface IJpaStorageResourceParser extends IStorageResourceParser {
 
 	/**
 	 * Populates a resource model object's metadata (Resource.meta.*) based on the
-	 * values from a stroage entity.
+	 * values from a storage entity.
 	 *
 	 * @param theEntitySource The source
 	 * @param theResourceTarget The target

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

@@ -48,6 +48,7 @@ import com.google.common.collect.ListMultimap;
 import com.google.common.collect.MultimapBuilder;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.Validate;
+import org.apache.commons.lang3.time.DateUtils;
 import org.hl7.fhir.instance.model.api.IAnyResource;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.instance.model.api.IIdType;

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

@@ -2,6 +2,8 @@ package ca.uhn.fhir.jpa.provider.r4;
 
 import ca.uhn.fhir.i18n.Msg;
 import ca.uhn.fhir.jpa.api.config.DaoConfig;
+import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
+import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao;
 import ca.uhn.fhir.jpa.config.JpaConfig;
 import ca.uhn.fhir.jpa.entity.Search;
 import ca.uhn.fhir.jpa.model.search.SearchStatusEnum;
@@ -21,6 +23,7 @@ import ca.uhn.fhir.rest.server.interceptor.consent.ConsentOutcome;
 import ca.uhn.fhir.rest.server.interceptor.consent.DelegatingConsentService;
 import ca.uhn.fhir.rest.server.interceptor.consent.IConsentContextServices;
 import ca.uhn.fhir.rest.server.interceptor.consent.IConsentService;
+import ca.uhn.fhir.util.BundleBuilder;
 import ca.uhn.fhir.util.BundleUtil;
 import ca.uhn.fhir.util.StopWatch;
 import ca.uhn.fhir.util.UrlUtil;
@@ -37,12 +40,14 @@ import org.apache.http.client.methods.HttpPost;
 import org.apache.http.client.methods.HttpPut;
 import org.apache.http.entity.ContentType;
 import org.apache.http.entity.StringEntity;
+import org.hl7.fhir.instance.model.api.IBaseBundle;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.instance.model.api.IIdType;
 import org.hl7.fhir.r4.model.Bundle;
 import org.hl7.fhir.r4.model.Enumerations;
 import org.hl7.fhir.r4.model.HumanName;
 import org.hl7.fhir.r4.model.IdType;
+import org.hl7.fhir.r4.model.Identifier;
 import org.hl7.fhir.r4.model.Observation;
 import org.hl7.fhir.r4.model.OperationOutcome;
 import org.hl7.fhir.r4.model.Organization;
@@ -54,6 +59,10 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.Qualifier;
+import org.springframework.transaction.PlatformTransactionManager;
+import org.springframework.transaction.TransactionDefinition;
+import org.springframework.transaction.support.TransactionSynchronizationManager;
+import org.springframework.transaction.support.TransactionTemplate;
 
 import java.io.IOException;
 import java.util.ArrayList;
@@ -65,6 +74,7 @@ import static org.apache.commons.lang3.StringUtils.leftPad;
 import static org.awaitility.Awaitility.await;
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.CoreMatchers.not;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.blankOrNullString;
@@ -109,6 +119,31 @@ public class ConsentInterceptorResourceProviderR4Test extends BaseResourceProvid
 		myServer.getRestfulServer().registerProvider(myGraphQlProvider);
 	}
 
+	@Test
+	public void testConsentServiceWhichReadsDoesNotThrowNpe() {
+		myDaoConfig.setAllowAutoInflateBinaries(true);
+		IConsentService consentService = new ReadingBackResourcesConsentSvc(myDaoRegistry);
+		myConsentInterceptor = new ConsentInterceptor(consentService, IConsentContextServices.NULL_IMPL);
+		myServer.getRestfulServer().getInterceptorService().registerInterceptor(myConsentInterceptor);
+		myInterceptorRegistry.registerInterceptor(myBinaryStorageInterceptor);
+
+		BundleBuilder builder = new BundleBuilder(myFhirContext);
+		for (int i = 0; i <10 ;i++) {
+			Observation o = new Observation();
+			o.setId("obs-" + i);
+			builder.addTransactionUpdateEntry(o);
+		}
+		for (int i = 0; i <10 ;i++) {
+			Observation o = new Observation();
+			o.setIdentifier(Lists.newArrayList(new Identifier().setSystem("http://foo").setValue("bar")));
+			builder.addTransactionCreateEntry(o);
+		}
+
+		Bundle execute = (Bundle) myClient.transaction().withBundle(builder.getBundle()).execute();
+		assertThat(execute.getEntry().size(), is(equalTo(20)));
+
+		myInterceptorRegistry.unregisterInterceptor(myBinaryStorageInterceptor);
+	}
 	@Test
 	public void testSearchAndBlockSomeWithReject() {
 		create50Observations();
@@ -807,6 +842,42 @@ public class ConsentInterceptorResourceProviderR4Test extends BaseResourceProvid
 
 	}
 
+	private static class ReadingBackResourcesConsentSvc implements  IConsentService {
+		private final DaoRegistry myDaoRegistry;
+
+		public ReadingBackResourcesConsentSvc(DaoRegistry theDaoRegistry)   {
+			myDaoRegistry = theDaoRegistry;
+		}
+		@Override
+		public ConsentOutcome startOperation(RequestDetails theRequestDetails, IConsentContextServices theContextServices) {
+			return new ConsentOutcome(ConsentOperationStatusEnum.PROCEED);
+
+		}
+
+		@Override
+		public ConsentOutcome canSeeResource(RequestDetails theRequestDetails, IBaseResource theResource, IConsentContextServices theContextServices) {
+			String fhirType = theResource.fhirType();
+			IFhirResourceDao<?> dao = myDaoRegistry.getResourceDao(fhirType);
+			String currentTransactionName = TransactionSynchronizationManager.getCurrentTransactionName();
+			dao.read(theResource.getIdElement());
+			return ConsentOutcome.PROCEED;
+		}
+
+		@Override
+		public ConsentOutcome willSeeResource(RequestDetails theRequestDetails, IBaseResource theResource, IConsentContextServices theContextServices) {
+			return ConsentOutcome.PROCEED;
+		}
+
+		@Override
+		public void completeOperationSuccess(RequestDetails theRequestDetails, IConsentContextServices theContextServices) {
+			// nothing
+		}
+
+		@Override
+		public void completeOperationFailure(RequestDetails theRequestDetails, BaseServerResponseException theException, IConsentContextServices theContextServices) {
+			// nothing
+		}
+	}
 	private static class ConsentSvcCantSeeOddNumbered implements IConsentService {
 
 		@Override

hapi-fhir-jpaserver-test-utilities/src/main/java/ca/uhn/fhir/jpa/provider/BaseResourceProviderR4Test.java

@@ -22,6 +22,7 @@ package ca.uhn.fhir.jpa.provider;
 
 import ca.uhn.fhir.batch2.jobs.expunge.DeleteExpungeProvider;
 import ca.uhn.fhir.batch2.jobs.reindex.ReindexProvider;
+import ca.uhn.fhir.jpa.api.IDaoRegistry;
 import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
 import ca.uhn.fhir.jpa.bulk.export.provider.BulkDataExportProvider;
 import ca.uhn.fhir.jpa.dao.data.IPartitionDao;

hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/api/server/SimplePreResourceShowDetails.java

@@ -28,6 +28,8 @@ import java.util.Arrays;
 import java.util.Collection;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Objects;
+import java.util.stream.Collectors;
 
 public class SimplePreResourceShowDetails implements IPreResourceShowDetails {
 

hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/api/server/SimplePreResourceShowDetailsTest.java

@@ -1,11 +1,16 @@
 package ca.uhn.fhir.rest.api.server;
 
 import org.hl7.fhir.instance.model.api.IBaseResource;
+import org.hl7.fhir.instance.model.api.IIdType;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
 
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.fail;

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

@@ -253,33 +253,46 @@ public class BinaryStorageInterceptor {
 			return;
 		}
 
-		long unmarshalledByteCount = 0;
+		long cumulativeInflatedBytes = 0;
+		int inflatedResourceCount = 0;
 
 		for (IBaseResource nextResource : theDetails) {
+			if (nextResource == null) {
+				ourLog.warn("Received a null resource during STORAGE_PRESHOW_RESOURCES. This is a bug and should be reported. Skipping resource.");
+				continue;
+			}
+			cumulativeInflatedBytes = inflateBinariesInResource(cumulativeInflatedBytes, nextResource);
+			inflatedResourceCount += 1;
+			if (cumulativeInflatedBytes >= myAutoInflateBinariesMaximumBytes) {
+				ourLog.debug("Exiting binary data inflation early.[byteCount={}, resourcesInflated={}, resourcesSkipped={}]", cumulativeInflatedBytes, inflatedResourceCount, theDetails.size() - inflatedResourceCount);
+				return;
+			}
+		}
+		ourLog.debug("Exiting binary data inflation having inflated everything.[byteCount={}, resourcesInflated={}, resourcesSkipped=0]", cumulativeInflatedBytes, inflatedResourceCount);
+	}
 
-			IIdType resourceId = nextResource.getIdElement();
-			List<IBinaryTarget> attachments = recursivelyScanResourceForBinaryData(nextResource);
 
-			for (IBinaryTarget nextTarget : attachments) {
-				Optional<String> attachmentId = nextTarget.getAttachmentId();
-				if (attachmentId.isPresent()) {
+	private long inflateBinariesInResource(long theCumulativeInflatedBytes, IBaseResource theResource) throws IOException {
+		IIdType resourceId = theResource.getIdElement();
+		List<IBinaryTarget> attachments = recursivelyScanResourceForBinaryData(theResource);
+		for (IBinaryTarget nextTarget : attachments) {
+			Optional<String> attachmentId = nextTarget.getAttachmentId();
+			if (attachmentId.isPresent()) {
 
-					StoredDetails blobDetails = myBinaryStorageSvc.fetchBlobDetails(resourceId, attachmentId.get());
-					if (blobDetails == null) {
-						String msg = myCtx.getLocalizer().getMessage(BinaryAccessProvider.class, "unknownBlobId");
-						throw new InvalidRequestException(Msg.code(1330) + msg);
-					}
+				StoredDetails blobDetails = myBinaryStorageSvc.fetchBlobDetails(resourceId, attachmentId.get());
+				if (blobDetails == null) {
+					String msg = myCtx.getLocalizer().getMessage(BinaryAccessProvider.class, "unknownBlobId");
+					throw new InvalidRequestException(Msg.code(1330) + msg);
+				}
 
-					if ((unmarshalledByteCount + blobDetails.getBytes()) < myAutoInflateBinariesMaximumBytes) {
-
-						byte[] bytes = myBinaryStorageSvc.fetchBlob(resourceId, attachmentId.get());
-						nextTarget.setData(bytes);
-						unmarshalledByteCount += blobDetails.getBytes();
-					}
+				if ((theCumulativeInflatedBytes + blobDetails.getBytes()) < myAutoInflateBinariesMaximumBytes) {
+					byte[] bytes = myBinaryStorageSvc.fetchBlob(resourceId, attachmentId.get());
+					nextTarget.setData(bytes);
+					theCumulativeInflatedBytes += blobDetails.getBytes();
 				}
 			}
-
 		}
+		return theCumulativeInflatedBytes;
 	}
 
 	@Nonnull