Interceptor improvements (#4125)

* Interceptor improvements

* Add changelogs

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

@@ -42,18 +42,34 @@ public interface IBaseInterceptorService<POINTCUT extends IPointcut> extends IBa
 	 * {@link #unregisterAllInterceptors()} do not affect thread local interceptors
 	 * as they are kept in a separate list.
 	 * </p>
+	 * <p>
+	 *    ThreadLocal interceptors are now disabled by default as of HAPI FHIR 6.2.0 and must be manually
+	 *    enabled by calling {@link ca.uhn.fhir.interceptor.executor.BaseInterceptorService#setThreadlocalInvokersEnabled(boolean)}.
+	 *    They are now deprecated. Registering a threadlocal interceptor without enabling this feature will
+	 *    result in a {@link IllegalArgumentException}.
+	 * </p>
 	 *
 	 * @param theInterceptor The interceptor
 	 * @return Returns <code>true</code> if at least one valid hook method was found on this interceptor
+	 * @deprecated Threadlocal interceptors have been deprecated as of HAPI FHIR 6.2.0 and will be removed in a future release due to lack of use. If you feel that this is a bad decision, please speak up on the HAPI FHIR mailing list.
 	 */
+	@Deprecated
 	boolean registerThreadLocalInterceptor(Object theInterceptor);
 
 	/**
 	 * Unregisters a ThreadLocal interceptor
+	 * <p>
+	 *    ThreadLocal interceptors are now disabled by default as of HAPI FHIR 6.2.0 and must be manually
+	 *    enabled by calling {@link ca.uhn.fhir.interceptor.executor.BaseInterceptorService#setThreadlocalInvokersEnabled(boolean)}.
+	 *    They are now deprecated. Registering a threadlocal interceptor without enabling this feature will
+	 *    result in a {@link IllegalArgumentException}.
+	 * </p>
 	 *
 	 * @param theInterceptor The interceptor
 	 * @see #registerThreadLocalInterceptor(Object)
+	 * @deprecated Threadlocal interceptors have been deprecated as of HAPI FHIR 6.2.0 and will be removed in a future release due to lack of use. If you feel that this is a bad decision, please speak up on the HAPI FHIR mailing list.
 	 */
+	@Deprecated
 	void unregisterThreadLocalInterceptor(Object theInterceptor);
 
 	/**

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

@@ -1618,6 +1618,42 @@ public enum Pointcut implements IPointcut {
 		"ca.uhn.fhir.rest.api.InterceptorInvocationTimingEnum"
 	),
 
+	/**
+	 * <b>Storage Hook:</b>
+	 * Invoked when a FHIR transaction bundle is about to begin processing. Hooks may choose to
+	 * modify the bundle, and may affect processing by doing so.
+	 * <p>
+	 * Hooks will have access to the original bundle, as well as all the deferred interceptor broadcasts related to the
+	 * processing of the transaction bundle
+	 * </p>
+	 * Hooks may accept the following parameters:
+	 * <ul>
+	 * <li>org.hl7.fhir.instance.model.api.IBaseResource - The resource being deleted</li>
+	 * <li>
+	 * ca.uhn.fhir.rest.api.server.RequestDetails - A bean containing details about the request that is about to be processed, including details such as the
+	 * resource type and logical ID (if any) and other FHIR-specific aspects of the request which have been
+	 * pulled out of the servlet request.
+	 * </li>
+	 * <li>
+	 * ca.uhn.fhir.rest.server.servlet.ServletRequestDetails - A bean containing details about the request that is about to be processed, including details such as the
+	 * resource type and logical ID (if any) and other FHIR-specific aspects of the request which have been
+	 * 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>
+	 * </ul>
+	 * <p>
+	 * Hooks should return <code>void</code>.
+	 * </p>
+	 *
+	 * @see #STORAGE_TRANSACTION_PROCESSED
+	 * @since 6.2.0
+	 */
+	STORAGE_TRANSACTION_PROCESSING(void.class,
+		"org.hl7.fhir.instance.model.api.IBaseBundle",
+		"ca.uhn.fhir.rest.api.server.RequestDetails",
+		"ca.uhn.fhir.rest.server.servlet.ServletRequestDetails"
+	),
+
 	/**
 	 * <b>Storage Hook:</b>
 	 * Invoked after all entries in a transaction bundle have been executed
@@ -1631,9 +1667,7 @@ public enum Pointcut implements IPointcut {
 	 * <li>
 	 * ca.uhn.fhir.rest.api.server.RequestDetails - A bean containing details about the request that is about to be processed, including details such as the
 	 * resource type and logical ID (if any) and other FHIR-specific aspects of the request which have been
-	 * pulled out of the servlet request. Note that the bean
+	 * pulled out of the servlet request.
-	 * properties are not all guaranteed to be populated, depending on how early during processing the
-	 * exception occurred.
 	 * </li>
 	 * <li>
 	 * ca.uhn.fhir.rest.server.servlet.ServletRequestDetails - A bean containing details about the request that is about to be processed, including details such as the
@@ -1651,6 +1685,8 @@ public enum Pointcut implements IPointcut {
 	 * <p>
 	 * Hooks should return <code>void</code>.
 	 * </p>
+	 *
+	 * @see #STORAGE_TRANSACTION_PROCESSING
 	 */
 	STORAGE_TRANSACTION_PROCESSED(void.class,
 		"org.hl7.fhir.instance.model.api.IBaseBundle",

hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/executor/BaseInterceptorService.java

@@ -68,7 +68,7 @@ public abstract class BaseInterceptorService<POINTCUT extends IPointcut> impleme
 	private final Object myRegistryMutex = new Object();
 	private final ThreadLocal<ListMultimap<POINTCUT, BaseInvoker>> myThreadlocalInvokers = new ThreadLocal<>();
 	private String myName;
-	private boolean myThreadlocalInvokersEnabled = true;
+	private boolean myThreadlocalInvokersEnabled = false;
 	private boolean myWarnOnInterceptorWithNoHooks = true;
 
 	/**
@@ -96,15 +96,18 @@ public abstract class BaseInterceptorService<POINTCUT extends IPointcut> impleme
 	}
 
 	/**
-	 * Are threadlocal interceptors enabled on this registry (defaults to true)
+	 * Are threadlocal interceptors enabled on this registry (defaults to false)
 	 */
 	public boolean isThreadlocalInvokersEnabled() {
 		return myThreadlocalInvokersEnabled;
 	}
 
 	/**
-	 * Are threadlocal interceptors enabled on this registry (defaults to true)
+	 * Are threadlocal interceptors enabled on this registry (defaults to false)
+	 *
+	 * @deprecated ThreadLocal interceptors are deprecated as of HAPI FHIR 6.2.0 and will be removed in a future release.
 	 */
+	@Deprecated
 	public void setThreadlocalInvokersEnabled(boolean theThreadlocalInvokersEnabled) {
 		myThreadlocalInvokersEnabled = theThreadlocalInvokersEnabled;
 	}
@@ -184,9 +187,7 @@ public abstract class BaseInterceptorService<POINTCUT extends IPointcut> impleme
 
 	@Override
 	public boolean registerThreadLocalInterceptor(Object theInterceptor) {
-		if (!myThreadlocalInvokersEnabled) {
+		Validate.isTrue (myThreadlocalInvokersEnabled, "Thread local interceptors are not enabled on this server");
-			return false;
-		}
 		ListMultimap<POINTCUT, BaseInvoker> invokers = getThreadLocalInvokerMultimap();
 		scanInterceptorAndAddToInvokerMultimap(theInterceptor, invokers);
 		return !invokers.isEmpty();
@@ -195,12 +196,11 @@ public abstract class BaseInterceptorService<POINTCUT extends IPointcut> impleme
 
 	@Override
 	public void unregisterThreadLocalInterceptor(Object theInterceptor) {
-		if (myThreadlocalInvokersEnabled) {
+		Validate.isTrue (myThreadlocalInvokersEnabled, "Thread local interceptors are not enabled on this server");
-			ListMultimap<POINTCUT, BaseInvoker> invokers = getThreadLocalInvokerMultimap();
+		ListMultimap<POINTCUT, BaseInvoker> invokers = getThreadLocalInvokerMultimap();
-			invokers.entries().removeIf(t -> t.getValue().getInterceptor() == theInterceptor);
+		invokers.entries().removeIf(t -> t.getValue().getInterceptor() == theInterceptor);
-			if (invokers.isEmpty()) {
+		if (invokers.isEmpty()) {
-				myThreadlocalInvokers.remove();
+			myThreadlocalInvokers.remove();
-			}
 		}
 	}
 

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/4125-add-transaction-modifying-interceptor.yaml

@@ -0,0 +1,6 @@
+---
+type: add
+issue: 4125
+title: "A new interceptor pointcut `STORAGE_TRANSACTION_PROCESSING` has been added. Hooks for this 
+   pointcut can examine and modify FHIR transaction bundles being processed by the JPA server before
+   processing starts."

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/4125-deprecate-threadlocal-interceptors.yaml

@@ -0,0 +1,6 @@
+---
+type: remove
+issue: 4125
+title: "The interceptor system has now deprecated the concept of ThreadLocal interceptors. This feature was
+  added for an anticipated use case, but has never seen any real use that we are aware of and removing it
+  should provide a minor performance improvement to the interceptor registry."

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

@@ -59,6 +59,7 @@ import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.servlet.ServletContextHandler;
 import org.eclipse.jetty.servlet.ServletHolder;
 import org.hl7.fhir.common.hapi.validation.validator.FhirInstanceValidator;
+import org.hl7.fhir.instance.model.api.IBaseBundle;
 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;
@@ -577,6 +578,48 @@ public class SystemProviderR4Test extends BaseJpaR4Test {
 	}
 
 
+	@Test
+	public void testTransactionWithModifyingInterceptor() {
+
+		BundleBuilder bb = new BundleBuilder(myFhirContext);
+		Patient pt = new Patient();
+		pt.setId("A");
+		pt.setActive(true);
+		bb.addTransactionUpdateEntry(pt);
+		Bundle input = (Bundle) bb.getBundle();
+
+		IAnonymousInterceptor interceptor = new IAnonymousInterceptor() {
+			@Override
+			public void invoke(IPointcut thePointcut, HookParams theArgs) {
+				Bundle transactionBundle = (Bundle) theArgs.get(IBaseBundle.class);
+
+				Patient pt = new Patient();
+				pt.setId("B");
+				pt.setActive(false);
+				transactionBundle
+					.addEntry()
+					.setResource(pt)
+					.getRequest()
+					.setMethod(HTTPVerb.PUT)
+					.setUrl("Patient/B");
+
+			}
+		};
+
+		Bundle output;
+		try {
+			myInterceptorRegistry.registerAnonymousInterceptor(Pointcut.STORAGE_TRANSACTION_PROCESSING, interceptor);
+			output = mySystemDao.transaction(mySrd, input);
+		}finally {
+			myInterceptorRegistry.unregisterInterceptor(interceptor);
+		}
+
+		assertEquals(2, output.getEntry().size());
+		assertEquals("A", new IdType(output.getEntry().get(0).getResponse().getLocation()).getIdPart());
+		assertEquals("B", new IdType(output.getEntry().get(1).getResponse().getLocation()).getIdPart());
+	}
+
+
 	@Test
 	public void testTransactionFromBundle() throws Exception {
 		InputStream bundleRes = SystemProviderR4Test.class.getResourceAsStream("/transaction_link_patient_eve.xml");

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

@@ -325,6 +325,16 @@ public abstract class BaseTransactionProcessor {
 	private IBaseBundle processTransactionAsSubRequest(RequestDetails theRequestDetails, IBaseBundle theRequest, String theActionName, boolean theNestedMode) {
 		BaseStorageDao.markRequestAsProcessingSubRequest(theRequestDetails);
 		try {
+
+			// Interceptor call: STORAGE_TRANSACTION_PROCESSING
+			if (CompositeInterceptorBroadcaster.hasHooks(Pointcut.STORAGE_TRANSACTION_PROCESSING, myInterceptorBroadcaster, theRequestDetails)) {
+				HookParams params = new HookParams()
+					.add(RequestDetails.class, theRequestDetails)
+					.addIfMatchesType(ServletRequestDetails.class, theRequest)
+					.add(IBaseBundle.class, theRequest);
+				CompositeInterceptorBroadcaster.doCallHooks(myInterceptorBroadcaster, theRequestDetails, Pointcut.STORAGE_TRANSACTION_PROCESSING, params);
+			}
+
 			return processTransaction(theRequestDetails, theRequest, theActionName, theNestedMode);
 		} finally {
 			BaseStorageDao.clearRequestAsProcessingSubRequest(theRequestDetails);