Fix contention aware patch in transaction (#4416)

* Fix contention aware patch in transaction

* Add changelog

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_4_0/4416-add-contention-aware-patch-in-transaction.yaml

@@ -0,0 +1,6 @@
+---
+type: add
+issue: 4416
+title: "The JPA server now supports contention aware patch operations (i.e. using `If-Match`)
+    within a FHIR Transaction. Previously the `Bundle.entry.request.ifMatch` element was incorrectly
+    treated as a conditional URL and not a version tag."

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

@@ -5,6 +5,7 @@ import ca.uhn.fhir.jpa.provider.BaseResourceProviderR4Test;
 import ca.uhn.fhir.rest.api.Constants;
 import ca.uhn.fhir.rest.api.MethodOutcome;
 import ca.uhn.fhir.rest.server.exceptions.ResourceGoneException;
+import ca.uhn.fhir.rest.server.exceptions.ResourceVersionConflictException;
 import com.google.common.base.Charsets;
 import org.apache.commons.io.IOUtils;
 import org.apache.http.client.methods.CloseableHttpResponse;
@@ -17,6 +18,7 @@ import org.hl7.fhir.r4.model.Binary;
 import org.hl7.fhir.r4.model.BooleanType;
 import org.hl7.fhir.r4.model.Bundle;
 import org.hl7.fhir.r4.model.CodeType;
+import org.hl7.fhir.r4.model.IdType;
 import org.hl7.fhir.r4.model.Media;
 import org.hl7.fhir.r4.model.Observation;
 import org.hl7.fhir.r4.model.OperationOutcome;
@@ -29,6 +31,7 @@ import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
+import java.util.Date;
 
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.MatcherAssert.assertThat;
@@ -74,6 +77,71 @@ public class PatchProviderR4Test extends BaseResourceProviderR4Test {
 		assertEquals(1, resultingResource.getIdentifier().size());
 	}
 
+	@Test
+	public void testFhirPatch_ContentionAware_Match() {
+		IIdType pid1;
+		{
+			Patient patient = new Patient();
+			patient.setActive(true);
+			pid1 = myPatientDao.create(patient, mySrd).getId().toUnqualifiedVersionless();
+		}
+
+		Parameters patch = new Parameters();
+		Parameters.ParametersParameterComponent op = patch.addParameter().setName("operation");
+		op.addPart().setName("type").setValue(new CodeType("replace"));
+		op.addPart().setName("path").setValue(new CodeType("Patient.active"));
+		op.addPart().setName("value").setValue(new BooleanType(false));
+
+		MethodOutcome outcome = myClient
+			.patch()
+			.withFhirPatch(patch)
+			.withId(pid1)
+			.withAdditionalHeader(Constants.HEADER_IF_MATCH, "W/\"1\"")
+			.execute();
+		assertEquals("2", outcome.getId().getVersionIdPart());
+
+		Patient newPt = myClient.read().resource(Patient.class).withId(pid1.getIdPart()).execute();
+		assertEquals("2", newPt.getIdElement().getVersionIdPart());
+		assertEquals(false, newPt.getActive());
+	}
+
+
+	@Test
+	public void testFhirPatch_ContentionAware_NoMatch() {
+		IIdType pid1;
+		{
+			Patient patient = new Patient();
+			patient.setActive(true);
+			pid1 = myPatientDao.create(patient, mySrd).getId().toUnqualifiedVersionless();
+
+			patient.setBirthDate(new Date());
+			myPatientDao.update(patient, mySrd);
+		}
+
+		Parameters patch = new Parameters();
+		Parameters.ParametersParameterComponent op = patch.addParameter().setName("operation");
+		op.addPart().setName("type").setValue(new CodeType("replace"));
+		op.addPart().setName("path").setValue(new CodeType("Patient.active"));
+		op.addPart().setName("value").setValue(new BooleanType(false));
+
+		try {
+			myClient
+				.patch()
+				.withFhirPatch(patch)
+				.withId(pid1)
+				.withAdditionalHeader(Constants.HEADER_IF_MATCH, "W/\"1\"")
+				.execute();
+			fail();
+		} catch (ResourceVersionConflictException e) {
+			// good
+		}
+
+		Patient newPt = myClient.read().resource(Patient.class).withId(pid1.getIdPart()).execute();
+		assertEquals("2", newPt.getIdElement().getVersionIdPart());
+		assertEquals(true, newPt.getActive());
+	}
+
+
 	@Test
 	public void testFhirPatch_Transaction() throws Exception {
 		String methodName = "testFhirPatch_Transaction";
@@ -115,6 +183,88 @@ public class PatchProviderR4Test extends BaseResourceProviderR4Test {
 		assertEquals(false, newPt.getActive());
 	}
 
+	@Test
+	public void testFhirPatch_TransactionContentionAware_Match() {
+		IIdType pid1;
+		{
+			Patient patient = new Patient();
+			patient.setActive(true);
+			pid1 = myPatientDao.create(patient, mySrd).getId().toUnqualifiedVersionless();
+		}
+
+		Parameters patch = new Parameters();
+		Parameters.ParametersParameterComponent op = patch.addParameter().setName("operation");
+		op.addPart().setName("type").setValue(new CodeType("replace"));
+		op.addPart().setName("path").setValue(new CodeType("Patient.active"));
+		op.addPart().setName("value").setValue(new BooleanType(false));
+
+		Bundle input = new Bundle();
+		input.setType(Bundle.BundleType.TRANSACTION);
+		input.addEntry()
+			.setFullUrl(pid1.getValue())
+			.setResource(patch)
+			.getRequest()
+			.setUrl(pid1.getValue())
+			.setIfMatch("W/\"1\"")
+			.setMethod(Bundle.HTTPVerb.PATCH);
+
+		Bundle outcome = myClient
+			.transaction()
+			.withBundle(input)
+			.execute();
+		ourLog.info(myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome));
+		assertEquals("2", new IdType(outcome.getEntry().get(0).getResponse().getLocation()).getVersionIdPart());
+
+		Patient newPt = myClient.read().resource(Patient.class).withId(pid1.getIdPart()).execute();
+		assertEquals("2", newPt.getIdElement().getVersionIdPart());
+		assertEquals(false, newPt.getActive());
+	}
+
+
+	@Test
+	public void testFhirPatch_TransactionContentionAware_NoMatch() {
+		IIdType pid1;
+		{
+			Patient patient = new Patient();
+			patient.setActive(true);
+			pid1 = myPatientDao.create(patient, mySrd).getId().toUnqualifiedVersionless();
+
+			patient.setBirthDate(new Date());
+			myPatientDao.update(patient, mySrd);
+		}
+
+		Parameters patch = new Parameters();
+		Parameters.ParametersParameterComponent op = patch.addParameter().setName("operation");
+		op.addPart().setName("type").setValue(new CodeType("replace"));
+		op.addPart().setName("path").setValue(new CodeType("Patient.active"));
+		op.addPart().setName("value").setValue(new BooleanType(false));
+
+		try {
+			Bundle input = new Bundle();
+			input.setType(Bundle.BundleType.TRANSACTION);
+			input.addEntry()
+				.setFullUrl(pid1.getValue())
+				.setResource(patch)
+				.getRequest()
+				.setUrl(pid1.getValue())
+				.setIfMatch("W/\"1\"")
+				.setMethod(Bundle.HTTPVerb.PATCH);
+
+			myClient
+				.transaction()
+				.withBundle(input)
+				.execute();
+			fail();
+		} catch (ResourceVersionConflictException e) {
+			// good
+		}
+
+		Patient newPt = myClient.read().resource(Patient.class).withId(pid1.getIdPart()).execute();
+		assertEquals("2", newPt.getIdElement().getVersionIdPart());
+		assertEquals(true, newPt.getActive());
+	}
+
+
 	@Test
 	public void testFhirPatch_TransactionWithSearchParameter() throws Exception {
 		String methodName = "testFhirPatch_Transaction";

hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/UpdateMethodBinding.java

@@ -33,7 +33,6 @@ import org.hl7.fhir.instance.model.api.IIdType;
 import ca.uhn.fhir.context.FhirContext;
 import ca.uhn.fhir.rest.annotation.Update;
 import ca.uhn.fhir.rest.api.Constants;
-import ca.uhn.fhir.rest.api.MethodOutcome;
 import ca.uhn.fhir.rest.api.RequestTypeEnum;
 import ca.uhn.fhir.rest.api.RestOperationTypeEnum;
 import ca.uhn.fhir.rest.api.server.RequestDetails;
@@ -41,6 +40,7 @@ import ca.uhn.fhir.rest.param.ParameterUtil;
 import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
 
 import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 
 public class UpdateMethodBinding extends BaseOutcomeReturningMethodBindingWithResourceParam {
 
@@ -60,15 +60,19 @@ public class UpdateMethodBinding extends BaseOutcomeReturningMethodBindingWithRe
 		super.addParametersForServerRequest(theRequest, theParams);
 	}
 
-	public static IIdType applyETagAsVersion(RequestDetails theRequest, IIdType id) {
+	public static IIdType applyETagAsVersion(RequestDetails theRequest, IIdType theId) {
 		String ifMatchValue = theRequest.getHeader(Constants.HEADER_IF_MATCH);
-		if (isNotBlank(ifMatchValue)) {
+		return applyETagAsVersion(ifMatchValue, theId);
-			ifMatchValue = ParameterUtil.parseETagValue(ifMatchValue);
+	}
-			if (id != null && id.hasVersionIdPart() == false) {
+
-				id = id.withVersion(ifMatchValue);
+	public static IIdType applyETagAsVersion(String theIfMatchValue, IIdType theId) {
+		if (isNotBlank(theIfMatchValue)) {
+			theIfMatchValue = ParameterUtil.parseETagValue(theIfMatchValue);
+			if (theId != null && theId.hasVersionIdPart() == false) {
+				theId = theId.withVersion(theIfMatchValue);
 			}
 		}
-		return id;
+		return theId;
 	}
 
 	@Override

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

@@ -71,6 +71,7 @@ import ca.uhn.fhir.rest.server.exceptions.PayloadTooLargeException;
 import ca.uhn.fhir.rest.server.exceptions.PreconditionFailedException;
 import ca.uhn.fhir.rest.server.method.BaseMethodBinding;
 import ca.uhn.fhir.rest.server.method.BaseResourceReturningMethodBinding;
+import ca.uhn.fhir.rest.server.method.UpdateMethodBinding;
 import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails;
 import ca.uhn.fhir.rest.server.servlet.ServletSubRequestDetails;
 import ca.uhn.fhir.rest.server.util.CompositeInterceptorBroadcaster;
@@ -108,20 +109,7 @@ import org.springframework.transaction.support.TransactionTemplate;
 
 import javax.annotation.Nonnull;
 import javax.annotation.PostConstruct;
-import java.util.ArrayList;
+import java.util.*;
-import java.util.Collection;
-import java.util.Comparator;
-import java.util.Date;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.IdentityHashMap;
-import java.util.Iterator;
-import java.util.LinkedHashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
-import java.util.Set;
-import java.util.TreeSet;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
@@ -139,8 +127,8 @@ public abstract class BaseTransactionProcessor {
 	public static final String URN_PREFIX = "urn:";
 	public static final String URN_PREFIX_ESCAPED = UrlUtil.escapeUrlParam(URN_PREFIX);
 	public static final Pattern UNQUALIFIED_MATCH_URL_START = Pattern.compile("^[a-zA-Z0-9_]+=");
-	private static final Logger ourLog = LoggerFactory.getLogger(BaseTransactionProcessor.class);
 	public static final Pattern INVALID_PLACEHOLDER_PATTERN = Pattern.compile("[a-zA-Z]+:.*");
+	private static final Logger ourLog = LoggerFactory.getLogger(BaseTransactionProcessor.class);
 	private BaseStorageDao myDao;
 	@Autowired
 	private PlatformTransactionManager myTxManager;
@@ -192,7 +180,7 @@ public abstract class BaseTransactionProcessor {
 
 	private TaskExecutor getTaskExecutor() {
 		if (myExecutor == null) {
-				myExecutor = myThreadPoolFactory.newThreadPool(myDaoConfig.getBundleBatchPoolSize(), myDaoConfig.getBundleBatchMaxPoolSize(), "bundle-batch-");
+			myExecutor = myThreadPoolFactory.newThreadPool(myDaoConfig.getBundleBatchPoolSize(), myDaoConfig.getBundleBatchMaxPoolSize(), "bundle-batch-");
 		}
 		return myExecutor;
 	}
@@ -271,7 +259,7 @@ public abstract class BaseTransactionProcessor {
 
 		populateIdToPersistedOutcomeMap(idToPersistedOutcome, newId, outcome);
 
-		if(shouldSwapBinaryToActualResource(theRes, theResourceType, nextResourceId)) {
+		if (shouldSwapBinaryToActualResource(theRes, theResourceType, nextResourceId)) {
 			theRes = idToPersistedOutcome.get(newId).getResource();
 		}
 
@@ -1097,7 +1085,16 @@ public abstract class BaseTransactionProcessor {
 						IFhirResourceDao<? extends IBaseResource> dao = toDao(parts, verb, url);
 						IIdType patchId = myContext.getVersion().newIdType().setValue(parts.getResourceId());
 
-						String conditionalUrl = isNull(patchId.getIdPart()) ? url : matchUrl;
+						String conditionalUrl;
+						if (isNull(patchId.getIdPart())) {
+							conditionalUrl = url;
+						} else {
+							conditionalUrl = matchUrl;
+							String ifMatch = myVersionAdapter.getEntryRequestIfMatch(nextReqEntry);
+							if (isNotBlank(ifMatch)) {
+								patchId = UpdateMethodBinding.applyETagAsVersion(ifMatch, patchId);
+							}
+						}
 
 						DaoMethodOutcome outcome = dao.patchInTransaction(patchId, conditionalUrl, false, patchType, patchBody, patchBodyParameters, theRequest, theTransactionDetails);
 						setConditionalUrlToBeValidatedLater(conditionalUrlToIdMap, matchUrl, outcome.getId());
@@ -1627,7 +1624,7 @@ public abstract class BaseTransactionProcessor {
 	 * Extracts the transaction url from the entry and verifies it's:
 	 * * not null or bloack
 	 * * is a relative url matching the resourceType it is about
-	 *
+	 * <p>
 	 * Returns the transaction url (or throws an InvalidRequestException if url is not valid)
 	 */
 	private String extractAndVerifyTransactionUrlForEntry(IBase theEntry, String theVerb) {
@@ -1643,7 +1640,7 @@ public abstract class BaseTransactionProcessor {
 
 	/**
 	 * Returns true if the provided url is a valid entry request.url.
-	 *
+	 * <p>
 	 * This means:
 	 * a) not an absolute url (does not start with http/https)
 	 * b) starts with either a ResourceType or /ResourceType
@@ -1705,20 +1702,21 @@ public abstract class BaseTransactionProcessor {
 
 	private String toMatchUrl(IBase theEntry) {
 		String verb = myVersionAdapter.getEntryRequestVerb(myContext, theEntry);
-		if (verb.equals("POST")) {
+		switch (defaultString(verb)) {
-			return myVersionAdapter.getEntryIfNoneExist(theEntry);
+			case "POST":
+				return myVersionAdapter.getEntryIfNoneExist(theEntry);
+			case "PUT":
+			case "DELETE":
+			case "PATCH":
+				String url = extractTransactionUrlOrThrowException(theEntry, verb);
+				UrlUtil.UrlParts parts = UrlUtil.parseUrl(url);
+				if (isBlank(parts.getResourceId())) {
+					return parts.getResourceType() + '?' + parts.getParams();
+				}
+				return null;
+			default:
+				return null;
 		}
-		if (verb.equals("PATCH")) {
-			return myVersionAdapter.getEntryRequestIfMatch(theEntry);
-		}
-		if (verb.equals("PUT") || verb.equals("DELETE")) {
-			String url = extractTransactionUrlOrThrowException(theEntry, verb);
-			UrlUtil.UrlParts parts = UrlUtil.parseUrl(url);
-			if (isBlank(parts.getResourceId())) {
-				return parts.getResourceType() + '?' + parts.getParams();
-			}
-		}
-		return null;
 	}
 
 	/**