Make server operation interceptor not perform superfluous calls for

conditional and no-op operations

hapi-fhir-client/src/main/java/ca/uhn/fhir/rest/client/impl/GenericClient.java

@@ -130,19 +130,6 @@ public class GenericClient extends BaseClient implements IGenericClient {
 
 	}
 
-	// public IResource read(UriDt url) {
-	// return read(inferResourceClass(url), url);
-	// }
-	//
-	// @SuppressWarnings("unchecked")
-	// public <T extends IResource> T read(final Class<T> theType, UriDt url) {
-	// return (T) invoke(theType, url, new ResourceResponseHandler<T>(theType));
-	// }
-	//
-	// public Bundle search(UriDt url) {
-	// return search(inferResourceClass(url), url);
-	// }
-
 	@Override
 	public IFetchConformanceUntyped fetchConformance() {
 		return new FetchConformanceInternal();
@@ -169,11 +156,6 @@ public class GenericClient extends BaseClient implements IGenericClient {
 		return theResource.getIdElement().getIdPart();
 	}
 
-	// @Override
-	// public <T extends IBaseResource> T read(final Class<T> theType, IdDt theId) {
-	// return doReadOrVRead(theType, theId, false, null, null);
-	// }
-
 	@Override
 	public IHistory history() {
 		return new HistoryInternal();
@@ -2105,14 +2087,14 @@ public class GenericClient extends BaseClient implements IGenericClient {
 
 		@Override
 		public IUpdateTyped resource(IBaseResource theResource) {
-			//Validate.notNull(theResource, "Resource can not be null");
+			Validate.notNull(theResource, "Resource can not be null");
 			myResource = theResource;
 			return this;
 		}
 
 		@Override
 		public IUpdateTyped resource(String theResourceBody) {
-			//Validate.notBlank(theResourceBody, "Body can not be null or blank");
+			Validate.notBlank(theResourceBody, "Body can not be null or blank");
 			myResourceBody = theResourceBody;
 			return this;
 		}

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

@@ -431,7 +431,7 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 		}
 
 		// Perform actual DB update
-		updateEntity(theResource, entity, null, thePerformIndexing, thePerformIndexing, theUpdateTime, false, thePerformIndexing);
+		ResourceTable updatedEntity = updateEntity(theResource, entity, null, thePerformIndexing, thePerformIndexing, theUpdateTime, false, thePerformIndexing);
 		theResource.setId(entity.getIdDt());
 
 
@@ -445,12 +445,14 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 		}
 
 		// Notify JPA interceptors
-		if (theRequestDetails != null) {
+		if (!updatedEntity.isUnchangedInCurrentOperation()) {
-			theRequestDetails.getRequestOperationCallback().resourceCreated(theResource);
+			if (theRequestDetails != null) {
-		}
+				theRequestDetails.getRequestOperationCallback().resourceCreated(theResource);
-		for (IServerInterceptor next : getConfig().getInterceptors()) {
+			}
-			if (next instanceof IServerOperationInterceptor) {
+			for (IServerInterceptor next : getConfig().getInterceptors()) {
-				((IServerOperationInterceptor) next).resourceCreated(theRequestDetails, theResource);
+				if (next instanceof IServerOperationInterceptor) {
+					((IServerOperationInterceptor) next).resourceCreated(theRequestDetails, theResource);
+				}
 			}
 		}
 
@@ -1262,14 +1264,16 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 		}
 
 		// Notify interceptors
-		if (theRequestDetails != null) {
+		if (!savedEntity.isUnchangedInCurrentOperation()) {
-			theRequestDetails.getRequestOperationCallback().resourceUpdated(theResource);
+			if (theRequestDetails != null) {
-			theRequestDetails.getRequestOperationCallback().resourceUpdated(oldResource, theResource);
+				theRequestDetails.getRequestOperationCallback().resourceUpdated(theResource);
-		}
+				theRequestDetails.getRequestOperationCallback().resourceUpdated(oldResource, theResource);
-		for (IServerInterceptor next : getConfig().getInterceptors()) {
+			}
-			if (next instanceof IServerOperationInterceptor) {
+			for (IServerInterceptor next : getConfig().getInterceptors()) {
-				((IServerOperationInterceptor) next).resourceUpdated(theRequestDetails, theResource);
+				if (next instanceof IServerOperationInterceptor) {
-				((IServerOperationInterceptor) next).resourceUpdated(theRequestDetails, oldResource, theResource);
+					((IServerOperationInterceptor) next).resourceUpdated(theRequestDetails, theResource);
+					((IServerOperationInterceptor) next).resourceUpdated(theRequestDetails, oldResource, theResource);
+				}
 			}
 		}
 

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

@@ -8,6 +8,7 @@ import ca.uhn.fhir.rest.api.server.ResponseDetails;
 import ca.uhn.fhir.rest.server.exceptions.BaseServerResponseException;
 import ca.uhn.fhir.rest.server.interceptor.IServerInterceptor;
 import ca.uhn.fhir.rest.server.interceptor.IServerInterceptor.ActionRequestDetails;
+import ca.uhn.fhir.rest.server.interceptor.IServerOperationInterceptor;
 import ca.uhn.fhir.rest.server.interceptor.InterceptorAdapter;
 import ca.uhn.fhir.util.TestUtil;
 import org.apache.commons.io.IOUtils;
@@ -43,9 +44,9 @@ import static org.mockito.Mockito.*;
 public class ResourceProviderInterceptorR4Test extends BaseResourceProviderR4Test {
 
 	private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(ResourceProviderInterceptorR4Test.class);
-	private IServerInterceptor myDaoInterceptor;
+	private IServerOperationInterceptor myDaoInterceptor;
 
-	private IServerInterceptor myServerInterceptor;
+	private IServerOperationInterceptor myServerInterceptor;
 
 	@Override
 	@After
@@ -60,8 +61,8 @@ public class ResourceProviderInterceptorR4Test extends BaseResourceProviderR4Tes
 	public void before() throws Exception {
 		super.before();
 
-		myServerInterceptor = mock(IServerInterceptor.class);
+		myServerInterceptor = mock(IServerOperationInterceptor.class);
-		myDaoInterceptor = mock(IServerInterceptor.class);
+		myDaoInterceptor = mock(IServerOperationInterceptor.class);
 
 		resetServerInterceptor();
 
@@ -88,6 +89,57 @@ public class ResourceProviderInterceptorR4Test extends BaseResourceProviderR4Tes
 		when(myServerInterceptor.outgoingResponse(any(RequestDetails.class), any(ResponseDetails.class), any(HttpServletRequest.class), any(HttpServletResponse.class))).thenReturn(true);
 	}
 
+	@Test
+	public void testCreateConditionalNoOpResourceInTransaction() throws Exception {
+		String methodName = "foo";
+
+		Patient pt = new Patient();
+		pt.addName().setFamily(methodName);
+
+		Bundle bundle = new Bundle();
+		bundle.setType(BundleType.TRANSACTION);
+		BundleEntryComponent entry = bundle.addEntry();
+		entry.setFullUrl("Patient");
+		entry.setResource(pt);
+		entry.getRequest().setMethod(HTTPVerb.POST);
+		entry.getRequest().setUrl("Patient");
+
+		// Transaction time!
+		transaction(bundle);
+
+		// Do it again but with a conditional create that shouldn't actually create
+		resetServerInterceptor();
+		entry.getRequest().setIfNoneExist("Patient?name=" + methodName);
+		transaction(bundle);
+
+		/*
+		 * Server Interceptor
+		 */
+
+		ArgumentCaptor<ActionRequestDetails> ardCaptor = ArgumentCaptor.forClass(ActionRequestDetails.class);
+		ArgumentCaptor<RestOperationTypeEnum> opTypeCaptor = ArgumentCaptor.forClass(RestOperationTypeEnum.class);
+		verify(myServerInterceptor, times(1)).incomingRequestPreHandled(opTypeCaptor.capture(), ardCaptor.capture());
+		assertEquals(RestOperationTypeEnum.TRANSACTION, opTypeCaptor.getAllValues().get(0));
+
+		verify(myServerInterceptor, times(1)).incomingRequestPostProcessed(any(RequestDetails.class), any(HttpServletRequest.class), any(HttpServletResponse.class));
+		verify(myServerInterceptor, times(0)).resourceCreated(any(RequestDetails.class), any(IBaseResource.class));
+		verify(myServerInterceptor, times(0)).resourceUpdated(any(RequestDetails.class), any(IBaseResource.class), any(IBaseResource.class));
+
+		/*
+		 * DAO Interceptor
+		 */
+
+		ardCaptor = ArgumentCaptor.forClass(ActionRequestDetails.class);
+		opTypeCaptor = ArgumentCaptor.forClass(RestOperationTypeEnum.class);
+		verify(myDaoInterceptor, times(1)).incomingRequestPreHandled(opTypeCaptor.capture(), ardCaptor.capture());
+		assertEquals(RestOperationTypeEnum.TRANSACTION, opTypeCaptor.getAllValues().get(0));
+
+		verify(myDaoInterceptor, times(0)).incomingRequestPostProcessed(any(RequestDetails.class), any(HttpServletRequest.class), any(HttpServletResponse.class));
+		verify(myDaoInterceptor, times(0)).resourceCreated(any(RequestDetails.class), any(IBaseResource.class));
+		verify(myDaoInterceptor, times(0)).resourceUpdated(any(RequestDetails.class), any(IBaseResource.class), any(IBaseResource.class));
+
+	}
+
 	@Test
 	public void testCreateResource() throws IOException {
 		String methodName = "testCreateResource";
@@ -123,11 +175,11 @@ public class ResourceProviderInterceptorR4Test extends BaseResourceProviderR4Tes
 	}
 
 	/*
-    * This is a weird way of verifying, but because this class
+	 * This is a weird way of verifying, but because this class
-    * is a child of a superclass that has other test children
+	 * is a child of a superclass that has other test children
-    * it's possible that other tests are hitting the server
+	 * it's possible that other tests are hitting the server
-    * at the same time
+	 * at the same time
-    */
+	 */
 	@Test
 	public void testCreateResourceInTransaction() throws IOException {
 		String methodName = "testCreateResourceInTransaction";
@@ -143,16 +195,7 @@ public class ResourceProviderInterceptorR4Test extends BaseResourceProviderR4Tes
 		entry.getRequest().setMethod(HTTPVerb.POST);
 		entry.getRequest().setUrl("Patient");
 
-		String resource = myFhirCtx.newXmlParser().encodeResourceToString(bundle);
+		transaction(bundle);
-
-		HttpPost post = new HttpPost(ourServerBase + "/");
-		post.setEntity(new StringEntity(resource, ContentType.create(Constants.CT_FHIR_XML, "UTF-8")));
-		CloseableHttpResponse response = ourHttpClient.execute(post);
-		try {
-			assertEquals(200, response.getStatusLine().getStatusCode());
-		} finally {
-			response.close();
-		}
 
 		/*
 		 * Server Interceptor
@@ -246,6 +289,72 @@ public class ResourceProviderInterceptorR4Test extends BaseResourceProviderR4Tes
 
 	}
 
+	@Test
+	public void testUpdateNoOpResourceInTransaction() throws Exception {
+		String methodName = "foo";
+
+		Patient pt = new Patient();
+		pt.addName().setFamily(methodName);
+		String ptId = myPatientDao.create(pt).getId().toUnqualifiedVersionless().getValue();
+
+		Bundle bundle = new Bundle();
+		bundle.setType(BundleType.TRANSACTION);
+		BundleEntryComponent entry = bundle.addEntry();
+		entry.setFullUrl(ptId);
+		entry.setResource(pt);
+		entry.getRequest().setMethod(HTTPVerb.PUT);
+		entry.getRequest().setUrl(ptId);
+
+		// Transaction time!
+		transaction(bundle);
+
+		// Do it again but with an update that shouldn't actually create
+		resetServerInterceptor();
+		entry.getRequest().setIfNoneExist("Patient?name=" + methodName);
+		transaction(bundle);
+
+		/*
+		 * Server Interceptor
+		 */
+
+		ArgumentCaptor<ActionRequestDetails> ardCaptor = ArgumentCaptor.forClass(ActionRequestDetails.class);
+		ArgumentCaptor<RestOperationTypeEnum> opTypeCaptor = ArgumentCaptor.forClass(RestOperationTypeEnum.class);
+		verify(myServerInterceptor, times(2)).incomingRequestPreHandled(opTypeCaptor.capture(), ardCaptor.capture());
+		assertEquals(RestOperationTypeEnum.TRANSACTION, opTypeCaptor.getAllValues().get(0));
+		assertEquals(RestOperationTypeEnum.UPDATE, opTypeCaptor.getAllValues().get(1));
+
+		verify(myServerInterceptor, times(1)).incomingRequestPostProcessed(any(RequestDetails.class), any(HttpServletRequest.class), any(HttpServletResponse.class));
+		verify(myServerInterceptor, times(0)).resourceCreated(any(RequestDetails.class), any(IBaseResource.class));
+		verify(myServerInterceptor, times(0)).resourceUpdated(any(RequestDetails.class), any(IBaseResource.class), any(IBaseResource.class));
+
+		/*
+		 * DAO Interceptor
+		 */
+
+		ardCaptor = ArgumentCaptor.forClass(ActionRequestDetails.class);
+		opTypeCaptor = ArgumentCaptor.forClass(RestOperationTypeEnum.class);
+		verify(myDaoInterceptor, times(2)).incomingRequestPreHandled(opTypeCaptor.capture(), ardCaptor.capture());
+		assertEquals(RestOperationTypeEnum.TRANSACTION, opTypeCaptor.getAllValues().get(0));
+		assertEquals(RestOperationTypeEnum.UPDATE, opTypeCaptor.getAllValues().get(1));
+
+		verify(myDaoInterceptor, times(0)).incomingRequestPostProcessed(any(RequestDetails.class), any(HttpServletRequest.class), any(HttpServletResponse.class));
+		verify(myDaoInterceptor, times(0)).resourceCreated(any(RequestDetails.class), any(IBaseResource.class));
+		verify(myDaoInterceptor, times(0)).resourceUpdated(any(RequestDetails.class), any(IBaseResource.class), any(IBaseResource.class));
+
+	}
+
+	private void transaction(Bundle theBundle) throws IOException {
+		String resource = myFhirCtx.newXmlParser().encodeResourceToString(theBundle);
+		HttpPost post = new HttpPost(ourServerBase + "/");
+		post.setEntity(new StringEntity(resource, ContentType.create(Constants.CT_FHIR_XML, "UTF-8")));
+		CloseableHttpResponse response = ourHttpClient.execute(post);
+		try {
+			assertEquals(200, response.getStatusLine().getStatusCode());
+		} finally {
+			response.close();
+		}
+	}
+
 	@AfterClass
 	public static void afterClassClearContext() {
 		TestUtil.clearAllStaticFieldsForUnitTest();

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/subscription/r4/RestHookTestR4Test.java

@@ -135,6 +135,41 @@ public class RestHookTestR4Test extends BaseResourceProviderR4Test {
 		waitForSize(0, ourCreatedObservations);
 		waitForSize(1, ourUpdatedObservations);
 		assertEquals(Constants.CT_FHIR_JSON_NEW, ourContentTypes.get(0));
+	}
+
+	@Test
+	public void testRestHookSubscriptionNoopUpdateDoesntTriggerNewDelivery() throws Exception {
+		String payload = "application/fhir+json";
+
+		String code = "1000000050";
+		String criteria1 = "Observation?code=SNOMED-CT|" + code + "&_format=xml";
+		String criteria2 = "Observation?code=SNOMED-CT|" + code + "111&_format=xml";
+
+		createSubscription(criteria1, payload, ourListenerServerBase);
+		createSubscription(criteria2, payload, ourListenerServerBase);
+
+		Observation obs = sendObservation(code, "SNOMED-CT");
+
+		// Should see 1 subscription notification
+		waitForQueueToDrain();
+		waitForSize(0, ourCreatedObservations);
+		waitForSize(1, ourUpdatedObservations);
+		assertEquals(Constants.CT_FHIR_JSON_NEW, ourContentTypes.get(0));
+
+		// Send an update with no changes
+		obs.setId(obs.getIdElement().toUnqualifiedVersionless());
+		myClient.update().resource(obs).execute();
+
+		// Should be no further deliveries
+		Thread.sleep(1000);
+		waitForQueueToDrain();
+		waitForSize(0, ourCreatedObservations);
+		waitForSize(1, ourUpdatedObservations);
+
+
+
+
+
 	}
 
 	@Test

hapi-fhir-jpaserver-elasticsearch/src/main/java/ca/uhn/fhir/jpa/search/ElasticsearchMappingProvider.java

@@ -1,5 +1,25 @@
 package ca.uhn.fhir.jpa.search;
 
+/*-
+ * #%L
+ * HAPI FHIR JPA Server - ElasticSearch Integration
+ * %%
+ * Copyright (C) 2014 - 2018 University Health Network
+ * %%
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * 
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ * #L%
+ */
+
 import org.hibernate.search.elasticsearch.analyzer.definition.ElasticsearchAnalysisDefinitionRegistryBuilder;
 import org.hibernate.search.elasticsearch.analyzer.definition.spi.ElasticsearchAnalysisDefinitionProvider;
 

src/changes/changes.xml

@@ -240,6 +240,12 @@
 				is supported according to the FHIR specification. Thanks
 				to Jeff Chung for the pull request!
 			</action>
+			<action type="fix">
+				JPA Server Operation Interceptor create/update methods will now no
+				longer be fired if the create/update operation being performed
+				is a no-op (e.g. a conditional create that did not need to perform
+				any action, or an update where the contents didn't actually change)
+			</action>
 		</release>
 		<release version="3.2.0" date="2018-01-13">
 			<action type="add">