diff --git a/hapi-fhir-client/src/main/java/ca/uhn/fhir/rest/client/impl/GenericClient.java b/hapi-fhir-client/src/main/java/ca/uhn/fhir/rest/client/impl/GenericClient.java index b3b0acbce9b..809f7aebf73 100644 --- a/hapi-fhir-client/src/main/java/ca/uhn/fhir/rest/client/impl/GenericClient.java +++ b/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 read(final Class theType, UriDt url) { - // return (T) invoke(theType, url, new ResourceResponseHandler(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 read(final Class 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; } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java index 8fabeeb5e4b..30dd60c30c2 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java @@ -431,7 +431,7 @@ public abstract class BaseHapiFhirResourceDao 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 extends B } // Notify JPA interceptors - if (theRequestDetails != null) { - theRequestDetails.getRequestOperationCallback().resourceCreated(theResource); - } - for (IServerInterceptor next : getConfig().getInterceptors()) { - if (next instanceof IServerOperationInterceptor) { - ((IServerOperationInterceptor) next).resourceCreated(theRequestDetails, theResource); + if (!updatedEntity.isUnchangedInCurrentOperation()) { + if (theRequestDetails != null) { + theRequestDetails.getRequestOperationCallback().resourceCreated(theResource); + } + for (IServerInterceptor next : getConfig().getInterceptors()) { + if (next instanceof IServerOperationInterceptor) { + ((IServerOperationInterceptor) next).resourceCreated(theRequestDetails, theResource); + } } } @@ -1262,14 +1264,16 @@ public abstract class BaseHapiFhirResourceDao extends B } // Notify interceptors - if (theRequestDetails != null) { - theRequestDetails.getRequestOperationCallback().resourceUpdated(theResource); - theRequestDetails.getRequestOperationCallback().resourceUpdated(oldResource, theResource); - } - for (IServerInterceptor next : getConfig().getInterceptors()) { - if (next instanceof IServerOperationInterceptor) { - ((IServerOperationInterceptor) next).resourceUpdated(theRequestDetails, theResource); - ((IServerOperationInterceptor) next).resourceUpdated(theRequestDetails, oldResource, theResource); + if (!savedEntity.isUnchangedInCurrentOperation()) { + if (theRequestDetails != null) { + theRequestDetails.getRequestOperationCallback().resourceUpdated(theResource); + theRequestDetails.getRequestOperationCallback().resourceUpdated(oldResource, theResource); + } + for (IServerInterceptor next : getConfig().getInterceptors()) { + if (next instanceof IServerOperationInterceptor) { + ((IServerOperationInterceptor) next).resourceUpdated(theRequestDetails, theResource); + ((IServerOperationInterceptor) next).resourceUpdated(theRequestDetails, oldResource, theResource); + } } } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderInterceptorR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderInterceptorR4Test.java index 1ec71b6358f..f1ae45d2b73 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderInterceptorR4Test.java +++ b/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); - myDaoInterceptor = mock(IServerInterceptor.class); + myServerInterceptor = mock(IServerOperationInterceptor.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 ardCaptor = ArgumentCaptor.forClass(ActionRequestDetails.class); + ArgumentCaptor 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 - * is a child of a superclass that has other test children - * it's possible that other tests are hitting the server - * at the same time - */ + * This is a weird way of verifying, but because this class + * is a child of a superclass that has other test children + * it's possible that other tests are hitting the server + * 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); - - 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(); - } + transaction(bundle); /* * 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 ardCaptor = ArgumentCaptor.forClass(ActionRequestDetails.class); + ArgumentCaptor 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(); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/subscription/r4/RestHookTestR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/subscription/r4/RestHookTestR4Test.java index a8a5d3a702d..5a9c817e71e 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/subscription/r4/RestHookTestR4Test.java +++ b/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 diff --git a/hapi-fhir-jpaserver-elasticsearch/src/main/java/ca/uhn/fhir/jpa/search/ElasticsearchMappingProvider.java b/hapi-fhir-jpaserver-elasticsearch/src/main/java/ca/uhn/fhir/jpa/search/ElasticsearchMappingProvider.java index 12f25874116..2fe8b5824aa 100644 --- a/hapi-fhir-jpaserver-elasticsearch/src/main/java/ca/uhn/fhir/jpa/search/ElasticsearchMappingProvider.java +++ b/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; diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 8869cddb77e..2ab3593d359 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -240,6 +240,12 @@ is supported according to the FHIR specification. Thanks to Jeff Chung for the pull request! + + 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) +