From d4717a08d61ff2a39224d27034eb600377149a89 Mon Sep 17 00:00:00 2001
From: James Agnew <jamesagnew@gmail.com>
Date: Tue, 12 Sep 2023 16:40:48 -0400
Subject: [PATCH] Improve docs for NO-OP update pointcuts (#5294)

---
 .../ca/uhn/fhir/interceptor/api/Pointcut.java | 13 +++
 .../dao/r4/FhirResourceDaoR4UpdateTest.java   | 92 +++++++++++++++++++
 2 files changed, 105 insertions(+)

diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/api/Pointcut.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/api/Pointcut.java
index e481f4cc2bb..994ea9cd1cf 100644
--- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/api/Pointcut.java
+++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/api/Pointcut.java
@@ -1484,6 +1484,15 @@ public enum Pointcut implements IPointcut {
 	 * to the new contents of the resource. These changes will be reflected in
 	 * permanent storage.
 	 * </p>
+	 * <p>
+	 * <b>NO-OPS:</b> If the client has submitted an update that does not actually make any changes
+	 * (i.e. the resource they include in the PUT body is identical to the content that
+	 * was already stored) the server may choose to ignore the update and perform
+	 * a "NO-OP". In this case, this pointcut is still invoked, but {@link #STORAGE_PRECOMMIT_RESOURCE_UPDATED}
+	 * will not be. Hook methods for this pointcut may make changes to the new contents of the
+	 * resource being updated, and in this case the NO-OP will be cancelled and
+	 * {@link #STORAGE_PRECOMMIT_RESOURCE_UPDATED} will also be invoked.
+	 * </p>
 	 * Hooks may accept the following parameters:
 	 * <ul>
 	 * <li>org.hl7.fhir.instance.model.api.IBaseResource - The previous contents of the resource being updated</li>
@@ -1617,6 +1626,10 @@ public enum Pointcut implements IPointcut {
 	 * changes as storage has already occurred. Changes will not be reflected
 	 * in storage, but may be reflected in the HTTP response.
 	 * </p>
+	 * <p>
+	 * NO-OP note: See {@link #STORAGE_PRESTORAGE_RESOURCE_UPDATED} for a note on
+	 * no-op updates when no changes are detected.
+	 * </p>
 	 * Hooks may accept the following parameters:
 	 * <ul>
 	 * <li>org.hl7.fhir.instance.model.api.IBaseResource - The previous contents of the resource</li>
diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4UpdateTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4UpdateTest.java
index 48a9de4e516..a1e846387d5 100644
--- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4UpdateTest.java
+++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4UpdateTest.java
@@ -1,6 +1,10 @@
 package ca.uhn.fhir.jpa.dao.r4;
 
 import ca.uhn.fhir.i18n.Msg;
+import ca.uhn.fhir.interceptor.api.HookParams;
+import ca.uhn.fhir.interceptor.api.IAnonymousInterceptor;
+import ca.uhn.fhir.interceptor.api.IPointcut;
+import ca.uhn.fhir.interceptor.api.Pointcut;
 import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
 import ca.uhn.fhir.jpa.model.dao.JpaPid;
 import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable;
@@ -16,11 +20,13 @@ import ca.uhn.fhir.rest.api.MethodOutcome;
 import ca.uhn.fhir.rest.api.server.IBundleProvider;
 import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
 import ca.uhn.fhir.rest.param.StringParam;
+import ca.uhn.fhir.rest.server.exceptions.InternalErrorException;
 import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
 import ca.uhn.fhir.rest.server.exceptions.ResourceGoneException;
 import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException;
 import ca.uhn.fhir.rest.server.exceptions.ResourceVersionConflictException;
 import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException;
+import ca.uhn.fhir.util.BundleBuilder;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.instance.model.api.IIdType;
 import org.hl7.fhir.r4.model.CanonicalType;
@@ -37,6 +43,7 @@ import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
+import javax.persistence.Id;
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.HashSet;
@@ -71,6 +78,7 @@ public class FhirResourceDaoR4UpdateTest extends BaseJpaR4Test {
 		myStorageSettings.setResourceServerIdStrategy(new JpaStorageSettings().getResourceServerIdStrategy());
 		myStorageSettings.setResourceClientIdStrategy(new JpaStorageSettings().getResourceClientIdStrategy());
 		myStorageSettings.setResourceServerIdStrategy(JpaStorageSettings.IdStrategyEnum.SEQUENTIAL_NUMERIC);
+		myInterceptorRegistry.unregisterAllAnonymousInterceptors();
 	}
 
 
@@ -1172,4 +1180,88 @@ public class FhirResourceDaoR4UpdateTest extends BaseJpaR4Test {
 		}
 	}
 
+	@Test
+	public void testUpdateNoChange_ChangeForcedInPreStorageInterceptor() {
+		// Add interceptor which forces a change
+		myInterceptorRegistry.registerAnonymousInterceptor(Pointcut.STORAGE_PRESTORAGE_RESOURCE_UPDATED, new IAnonymousInterceptor() {
+			@Override
+			public void invoke(IPointcut thePointcut, HookParams theArgs) {
+				Patient newResource = (Patient) theArgs.get(IBaseResource.class, 1);
+				newResource.addIdentifier().setValue("foo");
+			}
+		});
+
+		Patient p = new Patient();
+		p.setId("Patient/A");
+		p.setActive(true);
+		assertEquals("1", myPatientDao.update(p, mySrd).getId().getVersionIdPart());
+
+		p = new Patient();
+		p.setId("Patient/A");
+		p.setActive(true);
+		assertEquals("2", myPatientDao.update(p, mySrd).getId().getVersionIdPart());
+
+		p = myPatientDao.read(new IdType("Patient/A"), mySrd);
+		assertTrue(p.getActive());
+		assertEquals("foo", p.getIdentifierFirstRep().getValue());
+	}
+
+	/**
+	 * The precommit interceptor should not actually be invoked since this is a NO-OP
+	 * (only prestorage is)
+	 */
+	@Test
+	public void testUpdateNoChange_ChangeForcedInPreCommitInterceptor() {
+		// Add interceptor which forces a change
+		myInterceptorRegistry.registerAnonymousInterceptor(Pointcut.STORAGE_PRECOMMIT_RESOURCE_UPDATED, new IAnonymousInterceptor() {
+			@Override
+			public void invoke(IPointcut thePointcut, HookParams theArgs) {
+				throw new InternalErrorException("failed intentionally");
+			}
+		});
+
+		Patient p = new Patient();
+		p.setId("Patient/A");
+		p.setActive(true);
+		assertEquals("1", myPatientDao.update(p, mySrd).getId().getVersionIdPart());
+
+		p = new Patient();
+		p.setId("Patient/A");
+		p.setActive(true);
+		assertEquals("1", myPatientDao.update(p, mySrd).getId().getVersionIdPart());
+
+		p = myPatientDao.read(new IdType("Patient/A"), mySrd);
+		assertTrue(p.getActive());
+		assertEquals(0, p.getIdentifier().size());
+	}
+
+	@Test
+	public void testUpdateNoChange_ChangeForcedInPreStorageInterceptor_InTransaction() {
+		// Add interceptor which forces a change
+		myInterceptorRegistry.registerAnonymousInterceptor(Pointcut.STORAGE_PRESTORAGE_RESOURCE_UPDATED, new IAnonymousInterceptor() {
+			@Override
+			public void invoke(IPointcut thePointcut, HookParams theArgs) {
+				Patient newResource = (Patient) theArgs.get(IBaseResource.class, 1);
+				newResource.addIdentifier().setValue("foo");
+			}
+		});
+
+		Patient p = new Patient();
+		p.setId("Patient/A");
+		p.setActive(true);
+		BundleBuilder bb = new BundleBuilder(myFhirContext);
+		bb.addTransactionUpdateEntry(p);
+		assertThat(mySystemDao.transaction(mySrd, bb.getBundleTyped()).getEntryFirstRep().getResponse().getLocation(), endsWith("/_history/1"));
+
+		p = new Patient();
+		p.setId("Patient/A");
+		p.setActive(true);
+		bb = new BundleBuilder(myFhirContext);
+		bb.addTransactionUpdateEntry(p);
+		assertThat(mySystemDao.transaction(mySrd, bb.getBundleTyped()).getEntryFirstRep().getResponse().getLocation(), endsWith("/_history/2"));
+
+		p = myPatientDao.read(new IdType("Patient/A"), mySrd);
+		assertTrue(p.getActive());
+		assertEquals("foo", p.getIdentifierFirstRep().getValue());
+	}
 }