diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_0/6440-fix-hooks-not-called-for-precheck-for-cached-search.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_0/6440-fix-hooks-not-called-for-precheck-for-cached-search.yaml new file mode 100644 index 000000000000..86f16018012d --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_6_0/6440-fix-hooks-not-called-for-precheck-for-cached-search.yaml @@ -0,0 +1,8 @@ +--- +type: fix +issue: 6440 +title: "Previously, if an `IInterceptorBroadcaster` was set in a `RequestDetails` object, +`STORAGE_PRECHECK_FOR_CACHED_SEARCH` hooks that were registered to that `IInterceptorBroadcaster` were not +called. Also, if an `IInterceptorBroadcaster` was set in the `RequestDetails` object, the boolean return value of the hooks +registered to that `IInterceptorBroadcaster` were not taken into account. This second issue existed for all pointcuts +that returned a boolean type, not just for `STORAGE_PRECHECK_FOR_CACHED_SEARCH`. These issues have now been fixed." diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java index f73b69a3c2d0..2f5c54af2dd7 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java @@ -600,12 +600,12 @@ private PersistedJpaBundleProvider findCachedQuery( .add(SearchParameterMap.class, theParams) .add(RequestDetails.class, theRequestDetails) .addIfMatchesType(ServletRequestDetails.class, theRequestDetails); - Object outcome = CompositeInterceptorBroadcaster.doCallHooksAndReturnObject( + boolean canUseCache = CompositeInterceptorBroadcaster.doCallHooks( myInterceptorBroadcaster, theRequestDetails, Pointcut.STORAGE_PRECHECK_FOR_CACHED_SEARCH, params); - if (Boolean.FALSE.equals(outcome)) { + if (!canUseCache) { return null; } diff --git a/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/helper/BaseMdmHelper.java b/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/helper/BaseMdmHelper.java index 33a288ee811a..ee0e08ef7eae 100644 --- a/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/helper/BaseMdmHelper.java +++ b/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/helper/BaseMdmHelper.java @@ -1,6 +1,7 @@ package ca.uhn.fhir.jpa.mdm.helper; import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.interceptor.api.HookParams; import ca.uhn.fhir.interceptor.api.IInterceptorBroadcaster; import ca.uhn.fhir.interceptor.api.IInterceptorService; import ca.uhn.fhir.interceptor.api.Pointcut; @@ -23,6 +24,7 @@ import java.util.function.Supplier; import static org.awaitility.Awaitility.await; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.when; /** @@ -78,6 +80,7 @@ public void beforeEach(ExtensionContext context) throws Exception { //they are coming from an external HTTP Request. MockitoAnnotations.initMocks(this); when(myMockSrd.getInterceptorBroadcaster()).thenReturn(myMockInterceptorBroadcaster); + when(myMockInterceptorBroadcaster.callHooks(any(Pointcut.class), any(HookParams.class))).thenReturn(true); when(myMockSrd.getServletRequest()).thenReturn(myMockServletRequest); when(myMockSrd.getServer()).thenReturn(myMockRestfulServer); when(myMockSrd.getRequestId()).thenReturn("MOCK_REQUEST"); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/BaseComboParamsR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/BaseComboParamsR4Test.java index f73670fbddc8..2eb8a60aa2e8 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/BaseComboParamsR4Test.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/BaseComboParamsR4Test.java @@ -62,6 +62,9 @@ public void before() throws Exception { myMessages.add("REUSING CACHED SEARCH"); return null; }); + + // allow searches to use cached results + when(myInterceptorBroadcaster.callHooks(eq(Pointcut.STORAGE_PRECHECK_FOR_CACHED_SEARCH), ArgumentMatchers.any(HookParams.class))).thenReturn(true); } @AfterEach diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/util/CompositeInterceptorBroadcaster.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/util/CompositeInterceptorBroadcaster.java index 39d75793f28d..6670ab15a4c1 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/util/CompositeInterceptorBroadcaster.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/util/CompositeInterceptorBroadcaster.java @@ -81,7 +81,7 @@ public boolean callHooks(Pointcut thePointcut, HookParams theParams) { } if (theRequestDetails != null && theRequestDetails.getInterceptorBroadcaster() != null && retVal) { IInterceptorBroadcaster interceptorBroadcaster = theRequestDetails.getInterceptorBroadcaster(); - interceptorBroadcaster.callHooks(thePointcut, theParams); + retVal = interceptorBroadcaster.callHooks(thePointcut, theParams); } return retVal; } diff --git a/hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/util/CompositeInterceptorBroadcasterTest.java b/hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/util/CompositeInterceptorBroadcasterTest.java new file mode 100644 index 000000000000..2a670d158113 --- /dev/null +++ b/hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/util/CompositeInterceptorBroadcasterTest.java @@ -0,0 +1,161 @@ +package ca.uhn.fhir.rest.server.util; + +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.RequestDetails; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class CompositeInterceptorBroadcasterTest { + + @Mock + private IInterceptorBroadcaster myModuleBroadcasterMock; + @Mock + private IInterceptorBroadcaster myReqDetailsBroadcasterMock; + @Mock + private Pointcut myPointcutMock; + @Mock + private HookParams myHookParamsMock; + @Mock + private RequestDetails myRequestDetailsMock; + + + @Test + void doCallHooks_WhenModuleBroadcasterReturnsTrue_And_RequestDetailsBroadcasterReturnsTrue_ThenReturnsTrue() { + when(myRequestDetailsMock.getInterceptorBroadcaster()).thenReturn(myReqDetailsBroadcasterMock); + + when(myModuleBroadcasterMock.callHooks(myPointcutMock, myHookParamsMock)).thenReturn(true); + when(myReqDetailsBroadcasterMock.callHooks(myPointcutMock, myHookParamsMock)).thenReturn(true); + + boolean retVal = CompositeInterceptorBroadcaster.doCallHooks(myModuleBroadcasterMock, myRequestDetailsMock, + myPointcutMock, myHookParamsMock); + + assertThat(retVal).isTrue(); + + verify(myModuleBroadcasterMock).callHooks(myPointcutMock, myHookParamsMock); + verify(myReqDetailsBroadcasterMock).callHooks(myPointcutMock, myHookParamsMock); + } + + @Test + void doCallHooks_WhenModuleBroadcasterReturnsTrue_And_RequestDetailsBroadcasterReturnsFalse_ThenReturnsFalse() { + when(myRequestDetailsMock.getInterceptorBroadcaster()).thenReturn(myReqDetailsBroadcasterMock); + + when(myModuleBroadcasterMock.callHooks(myPointcutMock, myHookParamsMock)).thenReturn(true); + when(myReqDetailsBroadcasterMock.callHooks(myPointcutMock, myHookParamsMock)).thenReturn(false); + + boolean retVal = CompositeInterceptorBroadcaster.doCallHooks(myModuleBroadcasterMock, myRequestDetailsMock, + myPointcutMock, myHookParamsMock); + + assertThat(retVal).isFalse(); + + verify(myModuleBroadcasterMock).callHooks(myPointcutMock, myHookParamsMock); + verify(myReqDetailsBroadcasterMock).callHooks(myPointcutMock, myHookParamsMock); + } + + @Test + void doCallHooks_WhenModuleBroadcasterReturnsFalse_ThenSkipsBroadcasterInRequestDetails_And_ReturnsFalse() { + when(myRequestDetailsMock.getInterceptorBroadcaster()).thenReturn(myReqDetailsBroadcasterMock); + + when(myModuleBroadcasterMock.callHooks(myPointcutMock, myHookParamsMock)).thenReturn(false); + + boolean retVal = CompositeInterceptorBroadcaster.doCallHooks(myModuleBroadcasterMock, myRequestDetailsMock, + myPointcutMock, myHookParamsMock); + + assertThat(retVal).isFalse(); + + verify(myModuleBroadcasterMock).callHooks(myPointcutMock, myHookParamsMock); + verify(myReqDetailsBroadcasterMock, never()).callHooks(myPointcutMock, myHookParamsMock); + } + + @Test + void doCallHooks_WhenModuleBroadcasterReturnsTrue_And_NullRequestDetailsBroadcaster_ThenReturnsTrue() { + + when(myModuleBroadcasterMock.callHooks(myPointcutMock, myHookParamsMock)).thenReturn(true); + when(myRequestDetailsMock.getInterceptorBroadcaster()).thenReturn(null); + + boolean retVal = CompositeInterceptorBroadcaster.doCallHooks(myModuleBroadcasterMock, myRequestDetailsMock, myPointcutMock, + myHookParamsMock); + + assertThat(retVal).isTrue(); + + verify(myModuleBroadcasterMock).callHooks(myPointcutMock, myHookParamsMock); + } + + @Test + void doCallHooks_WhenModuleBroadcasterReturnsFalse_And_NullRequestDetailsBroadcaster_ThenReturnsFalse() { + + when(myModuleBroadcasterMock.callHooks(myPointcutMock, myHookParamsMock)).thenReturn(false); + when(myRequestDetailsMock.getInterceptorBroadcaster()).thenReturn(null); + + boolean retVal = CompositeInterceptorBroadcaster.doCallHooks(myModuleBroadcasterMock, myRequestDetailsMock, myPointcutMock, + myHookParamsMock); + + assertThat(retVal).isFalse(); + + verify(myModuleBroadcasterMock).callHooks(myPointcutMock, myHookParamsMock); + } + + @Test + void doCallHooks_WhenModuleBroadcasterReturnsTrue_And_NullRequestDetails_ThenReturnsTrue() { + + when(myModuleBroadcasterMock.callHooks(myPointcutMock, myHookParamsMock)).thenReturn(true); + + boolean retVal = CompositeInterceptorBroadcaster.doCallHooks(myModuleBroadcasterMock, null, myPointcutMock, myHookParamsMock); + + assertThat(retVal).isTrue(); + + verify(myModuleBroadcasterMock).callHooks(myPointcutMock, myHookParamsMock); + } + + @Test + void doCallHooks_WhenModuleBroadcasterReturnsFalse_And_NullRequestDetails_ThenReturnsFalse() { + + when(myModuleBroadcasterMock.callHooks(myPointcutMock, myHookParamsMock)).thenReturn(false); + + boolean retVal = CompositeInterceptorBroadcaster.doCallHooks(myModuleBroadcasterMock, null, myPointcutMock, myHookParamsMock); + + assertThat(retVal).isFalse(); + + verify(myModuleBroadcasterMock).callHooks(myPointcutMock, myHookParamsMock); + } + + @Test + void doCallHooks_WhenNullModuleBroadcaster_And_NullRequestDetails_ThenReturnsTrue() { + + boolean retVal = CompositeInterceptorBroadcaster.doCallHooks(null, null, myPointcutMock, myHookParamsMock); + + assertThat(retVal).isTrue(); + } + + @Test + void doCallHooks_WhenNullModuleBroadcaster_And_RequestDetailsBroadcasterReturnsTrue_ThenReturnsTrue() { + when(myRequestDetailsMock.getInterceptorBroadcaster()).thenReturn(myReqDetailsBroadcasterMock); + when(myReqDetailsBroadcasterMock.callHooks(myPointcutMock, myHookParamsMock)).thenReturn(true); + + boolean retVal = CompositeInterceptorBroadcaster.doCallHooks(null, myRequestDetailsMock, myPointcutMock, myHookParamsMock); + + assertThat(retVal).isTrue(); + verify(myReqDetailsBroadcasterMock).callHooks(myPointcutMock, myHookParamsMock); + } + + + @Test + void doCallHooks_WhenNullModuleBroadcaster_And_RequestDetailsBroadcasterReturnsFalse_ThenReturnsFalse() { + when(myRequestDetailsMock.getInterceptorBroadcaster()).thenReturn(myReqDetailsBroadcasterMock); + when(myReqDetailsBroadcasterMock.callHooks(myPointcutMock, myHookParamsMock)).thenReturn(false); + + boolean retVal = CompositeInterceptorBroadcaster.doCallHooks(null, myRequestDetailsMock, myPointcutMock, myHookParamsMock); + + assertThat(retVal).isFalse(); + verify(myReqDetailsBroadcasterMock).callHooks(myPointcutMock, myHookParamsMock); + } +}