Make binary storage work for bulk export (#1836)

* Make binary storage work for bulk export

* Add changelog

* Build fixes

* Test fix

* Test fix

* Test fix

hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/api/Pointcut.java

@@ -1056,6 +1056,9 @@ public enum Pointcut {
 	 * pulled out of the servlet request. This parameter is identical to the RequestDetails parameter above but will
 	 * only be populated when operating in a RestfulServer implementation. It is provided as a convenience.
 	 * </li>
+	 * <li>
+	 * ca.uhn.fhir.rest.api.server.storage.TransactionDetails - The outer transaction details object (since 5.0.0)
+	 * </li>
 	 * </ul>
 	 * <p>
 	 * Hooks should return <code>void</code>.
@@ -1064,7 +1067,8 @@ public enum Pointcut {
 	STORAGE_PRESTORAGE_RESOURCE_CREATED(void.class,
 		"org.hl7.fhir.instance.model.api.IBaseResource",
 		"ca.uhn.fhir.rest.api.server.RequestDetails",
-		"ca.uhn.fhir.rest.server.servlet.ServletRequestDetails"
+		"ca.uhn.fhir.rest.server.servlet.ServletRequestDetails",
+		"ca.uhn.fhir.rest.api.server.storage.TransactionDetails"
 	),
 
 	/**
@@ -1094,6 +1098,9 @@ public enum Pointcut {
 	 * pulled out of the servlet request. This parameter is identical to the RequestDetails parameter above but will
 	 * only be populated when operating in a RestfulServer implementation. It is provided as a convenience.
 	 * </li>
+	 * <li>
+	 * ca.uhn.fhir.rest.api.server.storage.TransactionDetails - The outer transaction details object (since 5.0.0)
+	 * </li>
 	 * </ul>
 	 * <p>
 	 * Hooks should return <code>void</code>.
@@ -1103,7 +1110,8 @@ public enum Pointcut {
 		"org.hl7.fhir.instance.model.api.IBaseResource",
 		"org.hl7.fhir.instance.model.api.IBaseResource",
 		"ca.uhn.fhir.rest.api.server.RequestDetails",
-		"ca.uhn.fhir.rest.server.servlet.ServletRequestDetails"
+		"ca.uhn.fhir.rest.server.servlet.ServletRequestDetails",
+		"ca.uhn.fhir.rest.api.server.storage.TransactionDetails"
 	),
 
 	/**
@@ -1131,6 +1139,9 @@ public enum Pointcut {
 	 * pulled out of the servlet request. This parameter is identical to the RequestDetails parameter above but will
 	 * only be populated when operating in a RestfulServer implementation. It is provided as a convenience.
 	 * </li>
+	 * <li>
+	 * ca.uhn.fhir.rest.api.server.storage.TransactionDetails - The outer transaction details object (since 5.0.0)
+	 * </li>
 	 * </ul>
 	 * <p>
 	 * Hooks should return <code>void</code>.
@@ -1139,7 +1150,8 @@ public enum Pointcut {
 	STORAGE_PRESTORAGE_RESOURCE_DELETED(void.class,
 		"org.hl7.fhir.instance.model.api.IBaseResource",
 		"ca.uhn.fhir.rest.api.server.RequestDetails",
-		"ca.uhn.fhir.rest.server.servlet.ServletRequestDetails"
+		"ca.uhn.fhir.rest.server.servlet.ServletRequestDetails",
+		"ca.uhn.fhir.rest.api.server.storage.TransactionDetails"
 	),
 
 
@@ -1170,6 +1182,9 @@ public enum Pointcut {
 	 * pulled out of the servlet request. This parameter is identical to the RequestDetails parameter above but will
 	 * only be populated when operating in a RestfulServer implementation. It is provided as a convenience.
 	 * </li>
+	 * <li>
+	 * ca.uhn.fhir.rest.api.server.storage.TransactionDetails - The outer transaction details object (since 5.0.0)
+	 * </li>
 	 * </ul>
 	 * <p>
 	 * Hooks should return <code>void</code>.
@@ -1178,7 +1193,8 @@ public enum Pointcut {
 	STORAGE_PRECOMMIT_RESOURCE_CREATED(void.class,
 		"org.hl7.fhir.instance.model.api.IBaseResource",
 		"ca.uhn.fhir.rest.api.server.RequestDetails",
-		"ca.uhn.fhir.rest.server.servlet.ServletRequestDetails"
+		"ca.uhn.fhir.rest.server.servlet.ServletRequestDetails",
+		"ca.uhn.fhir.rest.api.server.storage.TransactionDetails"
 	),
 
 	/**
@@ -1209,6 +1225,9 @@ public enum Pointcut {
 	 * pulled out of the servlet request. This parameter is identical to the RequestDetails parameter above but will
 	 * only be populated when operating in a RestfulServer implementation. It is provided as a convenience.
 	 * </li>
+	 * <li>
+	 * ca.uhn.fhir.rest.api.server.storage.TransactionDetails - The outer transaction details object (since 5.0.0)
+	 * </li>
 	 * </ul>
 	 * <p>
 	 * Hooks should return <code>void</code>.
@@ -1218,7 +1237,8 @@ public enum Pointcut {
 		"org.hl7.fhir.instance.model.api.IBaseResource",
 		"org.hl7.fhir.instance.model.api.IBaseResource",
 		"ca.uhn.fhir.rest.api.server.RequestDetails",
-		"ca.uhn.fhir.rest.server.servlet.ServletRequestDetails"
+		"ca.uhn.fhir.rest.server.servlet.ServletRequestDetails",
+		"ca.uhn.fhir.rest.api.server.storage.TransactionDetails"
 	),
 
 
@@ -1245,6 +1265,9 @@ public enum Pointcut {
 	 * pulled out of the servlet request. This parameter is identical to the RequestDetails parameter above but will
 	 * only be populated when operating in a RestfulServer implementation. It is provided as a convenience.
 	 * </li>
+	 * <li>
+	 * ca.uhn.fhir.rest.api.server.storage.TransactionDetails - The outer transaction details object (since 5.0.0)
+	 * </li>
 	 * </ul>
 	 * <p>
 	 * Hooks should return <code>void</code>.
@@ -1253,7 +1276,8 @@ public enum Pointcut {
 	STORAGE_PRECOMMIT_RESOURCE_DELETED(void.class,
 		"org.hl7.fhir.instance.model.api.IBaseResource",
 		"ca.uhn.fhir.rest.api.server.RequestDetails",
-		"ca.uhn.fhir.rest.server.servlet.ServletRequestDetails"
+		"ca.uhn.fhir.rest.server.servlet.ServletRequestDetails",
+		"ca.uhn.fhir.rest.api.server.storage.TransactionDetails"
 	),
 
 	/**
@@ -1279,7 +1303,7 @@ public enum Pointcut {
 	 * only be populated when operating in a RestfulServer implementation. It is provided as a convenience.
 	 * </li>
 	 * <li>
-	 * ca.uhn.fhir.jpa.model.util.TransactionDetails - The outer transaction details object
+	 * ca.uhn.fhir.rest.api.server.storage.TransactionDetails - The outer transaction details object (since 5.0.0)
 	 * </li>
 	 * </ul>
 	 * <p>
@@ -1295,7 +1319,7 @@ public enum Pointcut {
 		"ca.uhn.fhir.jpa.api.model.DeleteConflictList",
 		"ca.uhn.fhir.rest.api.server.RequestDetails",
 		"ca.uhn.fhir.rest.server.servlet.ServletRequestDetails",
-		"ca.uhn.fhir.jpa.model.util.TransactionDetails"
+		"ca.uhn.fhir.rest.api.server.storage.TransactionDetails"
 	),
 
 	/**

hapi-fhir-base/src/test/java/ca/uhn/fhir/interceptor/executor/InterceptorServiceTest.java

@@ -365,11 +365,12 @@ public void testValidateParamTypesExtraParam() {
 		params.add(String.class, "C");
 		params.add(String.class, "D");
 		params.add(String.class, "E");
+		params.add(String.class, "F");
 		try {
 			svc.haveAppropriateParams(Pointcut.STORAGE_PRECOMMIT_RESOURCE_UPDATED, params);
 			fail();
 		} catch (IllegalArgumentException e) {
-			assertEquals("Wrong number of params for pointcut " + Pointcut.STORAGE_PRECOMMIT_RESOURCE_UPDATED + " - Wanted ca.uhn.fhir.rest.api.server.RequestDetails,ca.uhn.fhir.rest.server.servlet.ServletRequestDetails,org.hl7.fhir.instance.model.api.IBaseResource,org.hl7.fhir.instance.model.api.IBaseResource but found [String, String, String, String, String]", e.getMessage());
+			assertEquals("Wrong number of params for pointcut STORAGE_PRECOMMIT_RESOURCE_UPDATED - Wanted ca.uhn.fhir.rest.api.server.RequestDetails,ca.uhn.fhir.rest.api.server.storage.TransactionDetails,ca.uhn.fhir.rest.server.servlet.ServletRequestDetails,org.hl7.fhir.instance.model.api.IBaseResource,org.hl7.fhir.instance.model.api.IBaseResource but found [String, String, String, String, String, String]", e.getMessage());
 		}
 	}
 
@@ -383,6 +384,7 @@ public void testValidateParamTypesWrongParam() {
 		params.add((Class) String.class, 2);
 		params.add((Class) String.class, 3);
 		params.add((Class) String.class, 4);
+		params.add((Class) String.class, 5);
 		try {
 			svc.haveAppropriateParams(Pointcut.STORAGE_PRECOMMIT_RESOURCE_UPDATED, params);
 			fail();

hapi-fhir-docs/src/main/java/ca/uhn/hapi/fhir/docs/AuthorizationInterceptors.java

@@ -23,6 +23,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.rest.api.server.storage.TransactionDetails;
 import ca.uhn.fhir.rest.annotation.ConditionalUrlParam;
 import ca.uhn.fhir.rest.annotation.IdParam;
 import ca.uhn.fhir.rest.annotation.ResourceParam;
@@ -140,6 +141,10 @@ public MethodOutcome update(
       // let's pretend there is some storage code here.
       theResource.setId(theId.withVersion("2"));
 
+      // One TransactionDetails object should be created for each FHIR operation. Interceptors
+		// may use it for getting/setting details about the running transaction.
+		TransactionDetails transactionDetails = new TransactionDetails();
+
       // Notify the interceptor framework when we're about to perform an update. This is
 		// useful as the authorization interceptor will pick this event up and use it
 		// to factor into a decision about whether the operation should be allowed to proceed.
@@ -149,7 +154,8 @@ public MethodOutcome update(
 			.add(IBaseResource.class, previousContents)
 			.add(IBaseResource.class, newContents)
 			.add(RequestDetails.class, theRequestDetails)
-			.add(ServletRequestDetails.class, theRequestDetails);
+			.add(ServletRequestDetails.class, theRequestDetails)
+			.add(TransactionDetails.class, transactionDetails);
 		theInterceptorBroadcaster.callHooks(Pointcut.STORAGE_PRESTORAGE_RESOURCE_UPDATED, params);
 
       MethodOutcome retVal = new MethodOutcome();

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/4_3_0/1836-binary-storage-for-bulk-export.yaml

@@ -0,0 +1,6 @@
+---
+type: add
+issue: 1836
+title: "In HAPI FHIR 4.2.0, when performing a Bulk Export on a server with Binary Storage enabled, the bulk export files
+   were not able to take advantage of the externalized binary stroage and would be stored in the relational DB. This has
+   now been enhanced to allow bulk export files to store externally."

hapi-fhir-jpaserver-api/src/main/java/ca/uhn/fhir/jpa/api/dao/IFhirResourceDao.java

@@ -26,11 +26,11 @@
 import ca.uhn.fhir.jpa.api.model.DeleteMethodOutcome;
 import ca.uhn.fhir.jpa.api.model.ExpungeOptions;
 import ca.uhn.fhir.jpa.api.model.ExpungeOutcome;
-import ca.uhn.fhir.jpa.model.cross.ResourcePersistentId;
+import ca.uhn.fhir.rest.api.server.storage.ResourcePersistentId;
 import ca.uhn.fhir.jpa.model.entity.BaseHasResource;
 import ca.uhn.fhir.jpa.model.entity.ResourceTable;
 import ca.uhn.fhir.jpa.model.entity.TagTypeEnum;
-import ca.uhn.fhir.jpa.model.util.TransactionDetails;
+import ca.uhn.fhir.rest.api.server.storage.TransactionDetails;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.model.api.IQueryParameterType;
 import ca.uhn.fhir.rest.api.EncodingEnum;

hapi-fhir-jpaserver-api/src/main/java/ca/uhn/fhir/jpa/api/dao/IJpaDao.java

@@ -20,7 +20,7 @@
  * #L%
  */
 
-import ca.uhn.fhir.jpa.model.util.TransactionDetails;
+import ca.uhn.fhir.rest.api.server.storage.TransactionDetails;
 import ca.uhn.fhir.jpa.model.cross.IBasePersistedResource;
 import ca.uhn.fhir.rest.api.server.RequestDetails;
 import org.hl7.fhir.instance.model.api.IBaseResource;

hapi-fhir-jpaserver-api/src/main/java/ca/uhn/fhir/jpa/api/svc/ISearchCoordinatorSvc.java

@@ -21,7 +21,7 @@
  */
 
 import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao;
-import ca.uhn.fhir.jpa.model.cross.ResourcePersistentId;
+import ca.uhn.fhir.rest.api.server.storage.ResourcePersistentId;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.rest.api.CacheControlDirective;
 import ca.uhn.fhir.rest.api.server.IBundleProvider;

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/binstore/BinaryStorageInterceptor.java

@@ -28,12 +28,16 @@
 import ca.uhn.fhir.interceptor.api.Pointcut;
 import ca.uhn.fhir.jpa.dao.BaseHapiFhirDao;
 import ca.uhn.fhir.jpa.model.util.JpaConstants;
+import ca.uhn.fhir.rest.api.server.storage.TransactionDetails;
 import ca.uhn.fhir.rest.api.server.IPreResourceShowDetails;
 import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
-import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails;
 import ca.uhn.fhir.util.IModelVisitor2;
 import org.apache.commons.io.FileUtils;
-import org.hl7.fhir.instance.model.api.*;
+import org.hl7.fhir.instance.model.api.IBase;
+import org.hl7.fhir.instance.model.api.IBaseHasExtensions;
+import org.hl7.fhir.instance.model.api.IBaseResource;
+import org.hl7.fhir.instance.model.api.IIdType;
+import org.hl7.fhir.instance.model.api.IPrimitiveType;
 import org.hl7.fhir.r4.model.IdType;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -44,7 +48,11 @@
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStream;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.stream.Collectors;
 
@@ -84,7 +92,9 @@ public void setAutoDeExternalizeMaximumBytes(long theAutoDeExternalizeMaximumByt
 	@SuppressWarnings("unchecked")
 	@PostConstruct
 	public void start() {
-		myBinaryType = (Class<? extends IPrimitiveType<byte[]>>) myCtx.getElementDefinition("base64Binary").getImplementingClass();
+		BaseRuntimeElementDefinition<?> base64Binary = myCtx.getElementDefinition("base64Binary");
+		assert base64Binary != null;
+		myBinaryType = (Class<? extends IPrimitiveType<byte[]>>) base64Binary.getImplementingClass();
 		myDeferredListKey = getClass().getName() + "_" + hashCode() + "_DEFERRED_LIST";
 	}
 
@@ -97,7 +107,7 @@ public void expungeResource(AtomicInteger theCounter, IBaseResource theResource)
 			.stream()
 			.flatMap(t -> ((IBaseHasExtensions) t).getExtension().stream())
 			.filter(t -> JpaConstants.EXT_EXTERNALIZED_BINARY_ID.equals(t.getUrl()))
-			.map(t -> ((IPrimitiveType) t.getValue()).getValueAsString())
+			.map(t -> ((IPrimitiveType<?>) t.getValue()).getValueAsString())
 			.collect(Collectors.toList());
 
 		for (String next : attachmentIds) {
@@ -110,14 +120,14 @@ public void expungeResource(AtomicInteger theCounter, IBaseResource theResource)
 	}
 
 	@Hook(Pointcut.STORAGE_PRESTORAGE_RESOURCE_CREATED)
-	public void extractLargeBinariesBeforeCreate(ServletRequestDetails theRequestDetails, IBaseResource theResource, Pointcut thePointcut) throws IOException {
-		extractLargeBinaries(theRequestDetails, theResource, thePointcut);
+	public void extractLargeBinariesBeforeCreate(TransactionDetails theTransactionDetails, IBaseResource theResource, Pointcut thePointcut) throws IOException {
+		extractLargeBinaries(theTransactionDetails, theResource, thePointcut);
 	}
 
 	@Hook(Pointcut.STORAGE_PRESTORAGE_RESOURCE_UPDATED)
-	public void extractLargeBinariesBeforeUpdate(ServletRequestDetails theRequestDetails, IBaseResource thePreviousResource, IBaseResource theResource, Pointcut thePointcut) throws IOException {
+	public void extractLargeBinariesBeforeUpdate(TransactionDetails theTransactionDetails, IBaseResource thePreviousResource, IBaseResource theResource, Pointcut thePointcut) throws IOException {
 		blockIllegalExternalBinaryIds(thePreviousResource, theResource);
-		extractLargeBinaries(theRequestDetails, theResource, thePointcut);
+		extractLargeBinaries(theTransactionDetails, theResource, thePointcut);
 	}
 
 	/**
@@ -136,7 +146,7 @@ private void blockIllegalExternalBinaryIds(IBaseResource thePreviousResource, IB
 						.stream()
 						.filter(t -> t.getUserData(JpaConstants.EXTENSION_EXT_SYSTEMDEFINED) == null)
 						.filter(t -> EXT_EXTERNALIZED_BINARY_ID.equals(t.getUrl()))
-						.map(t -> (IPrimitiveType) t.getValue())
+						.map(t -> (IPrimitiveType<?>) t.getValue())
 						.map(t -> t.getValueAsString())
 						.filter(t -> isNotBlank(t))
 						.forEach(t -> existingBinaryIds.add(t));
@@ -152,12 +162,11 @@ private void blockIllegalExternalBinaryIds(IBaseResource thePreviousResource, IB
 					.stream()
 					.filter(t -> t.getUserData(JpaConstants.EXTENSION_EXT_SYSTEMDEFINED) == null)
 					.filter(t -> t.getUrl().equals(EXT_EXTERNALIZED_BINARY_ID))
-					.map(t->(IPrimitiveType) t.getValue())
-					.map(t->t.getValueAsString())
-					.filter(t->isNotBlank(t))
-					.filter(t->{
-						return !existingBinaryIds.contains(t);
-					}).findFirst();
+					.map(t -> (IPrimitiveType<?>) t.getValue())
+					.map(t -> t.getValueAsString())
+					.filter(t -> isNotBlank(t))
+					.filter(t -> !existingBinaryIds.contains(t))
+					.findFirst();
 
 				if (hasExternalizedBinaryReference.isPresent()) {
 					String msg = myCtx.getLocalizer().getMessage(BaseHapiFhirDao.class, "externalizedBinaryStorageExtensionFoundInRequestBody", EXT_EXTERNALIZED_BINARY_ID, hasExternalizedBinaryReference.get());
@@ -168,11 +177,8 @@ private void blockIllegalExternalBinaryIds(IBaseResource thePreviousResource, IB
 
 	}
 
-	private void extractLargeBinaries(ServletRequestDetails theRequestDetails, IBaseResource theResource, Pointcut thePointcut) throws IOException {
-		if (theRequestDetails == null) {
-			// RequestDetails will only be null for internal HAPI events.  If externalization is required for them it will need to be done in a different way.
-			return;
-		}
+	private void extractLargeBinaries(TransactionDetails theTransactionDetails, IBaseResource theResource, Pointcut thePointcut) throws IOException {
+
 		IIdType resourceId = theResource.getIdElement();
 		if (!resourceId.hasResourceType() && resourceId.hasIdPart()) {
 			String resourceType = myCtx.getResourceDefinition(theResource).getName();
@@ -197,7 +203,7 @@ private void extractLargeBinaries(ServletRequestDetails theRequestDetails, IBase
 					} else {
 						assert thePointcut == Pointcut.STORAGE_PRESTORAGE_RESOURCE_CREATED : thePointcut.name();
 						newBlobId = myBinaryStorageSvc.newBlobId();
-						List<DeferredBinaryTarget> deferredBinaryTargets = getOrCreateDeferredBinaryStorageMap(theRequestDetails);
+						List<DeferredBinaryTarget> deferredBinaryTargets = getOrCreateDeferredBinaryStorageMap(theTransactionDetails);
 						DeferredBinaryTarget newDeferredBinaryTarget = new DeferredBinaryTarget(newBlobId, nextTarget, data);
 						deferredBinaryTargets.add(newDeferredBinaryTarget);
 					}
@@ -207,26 +213,20 @@ private void extractLargeBinaries(ServletRequestDetails theRequestDetails, IBase
 			}
 
 		}
+
 	}
 
 	@Nonnull
-	@SuppressWarnings("unchecked")
-	private List<DeferredBinaryTarget> getOrCreateDeferredBinaryStorageMap(ServletRequestDetails theRequestDetails) {
-		List<DeferredBinaryTarget> deferredBinaryTargets = (List<DeferredBinaryTarget>) theRequestDetails.getUserData().get(getDeferredListKey());
-		if (deferredBinaryTargets == null) {
-			deferredBinaryTargets = new ArrayList<>();
-			theRequestDetails.getUserData().put(getDeferredListKey(), deferredBinaryTargets);
-		}
-		return deferredBinaryTargets;
+	private List<DeferredBinaryTarget> getOrCreateDeferredBinaryStorageMap(TransactionDetails theTransactionDetails) {
+		return theTransactionDetails.getOrCreateUserData(getDeferredListKey(), () -> new ArrayList<>());
 	}
 
-	@SuppressWarnings("unchecked")
 	@Hook(Pointcut.STORAGE_PRECOMMIT_RESOURCE_CREATED)
-	public void storeLargeBinariesBeforeCreatePersistence(ServletRequestDetails theRequestDetails, IBaseResource theResource, Pointcut thePoincut) throws IOException {
-		if (theRequestDetails == null) {
+	public void storeLargeBinariesBeforeCreatePersistence(TransactionDetails theTransactionDetails, IBaseResource theResource, Pointcut thePoincut) throws IOException {
+		if (theTransactionDetails == null) {
 			return;
 		}
-		List<DeferredBinaryTarget> deferredBinaryTargets = (List<DeferredBinaryTarget>) theRequestDetails.getUserData().get(getDeferredListKey());
+		List<DeferredBinaryTarget> deferredBinaryTargets = theTransactionDetails.getUserData(getDeferredListKey());
 		if (deferredBinaryTargets != null) {
 			IIdType resourceId = theResource.getIdElement();
 			for (DeferredBinaryTarget next : deferredBinaryTargets) {
@@ -284,9 +284,7 @@ public boolean acceptElement(IBase theElement, List<IBase> theContainingElementP
 				if (theElement.getClass().equals(myBinaryType)) {
 					IBase parent = theContainingElementPath.get(theContainingElementPath.size() - 2);
 					Optional<IBinaryTarget> binaryTarget = myBinaryAccessProvider.toBinaryTarget(parent);
-					if (binaryTarget.isPresent()) {
-						binaryTargets.add(binaryTarget.get());
-					}
+					binaryTarget.ifPresent(binaryTargets::add);
 				}
 				return true;
 			}