Fix an issue with binary storage not updating correctly if a retry (#2014)

interceptor is present

hapi-fhir-base/src/main/java/ca/uhn/fhir/util/FhirTerser.java

@@ -155,7 +155,17 @@ public class FhirTerser {
 
 		if (theSource instanceof IPrimitiveType<?>) {
 			if (theTarget instanceof IPrimitiveType<?>) {
-				((IPrimitiveType<?>) theTarget).setValueAsString(((IPrimitiveType<?>) theSource).getValueAsString());
+				String valueAsString = ((IPrimitiveType<?>) theSource).getValueAsString();
+				if (isNotBlank(valueAsString)) {
+					((IPrimitiveType<?>) theTarget).setValueAsString(valueAsString);
+				}
+				if (theSource instanceof IBaseHasExtensions && theTarget instanceof IBaseHasExtensions) {
+					List<? extends IBaseExtension<?, ?>> extensions = ((IBaseHasExtensions) theSource).getExtension();
+					for (IBaseExtension<?, ?> nextSource : extensions) {
+						IBaseExtension<?, ?> nextTarget = ((IBaseHasExtensions) theTarget).addExtension();
+						cloneInto(nextSource, nextTarget, theIgnoreMissingFields);
+					}
+				}
 				return theSource;
 			}
 			if (theIgnoreMissingFields) {

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

@@ -1360,17 +1360,22 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 		}
 		assert theResource.getIdElement().hasIdPart() || isNotBlank(theMatchUrl);
 
-		return myTransactionService.execute(theRequest, tx -> doUpdate(theResource, theMatchUrl, thePerformIndexing, theForceUpdateVersion, theRequest, theTransactionDetails));
+		/*
+		 * Resource updates will modify/update the version of the resource with the new version. This is generally helpful,
+		 * but leads to issues if the transaction is rolled back and retried. So if we do a rollback, we reset the resource
+		 * version to what it was.
+		 */
+		String id = theResource.getIdElement().getValue();
+		Runnable onRollback = () -> theResource.getIdElement().setValue(id);
+
+		// Execute the update in a retriable transasction
+		return myTransactionService.execute(theRequest, tx -> doUpdate(theResource, theMatchUrl, thePerformIndexing, theForceUpdateVersion, theRequest, theTransactionDetails), onRollback);
 	}
 
 	private DaoMethodOutcome doUpdate(T theResource, String theMatchUrl, boolean thePerformIndexing, boolean theForceUpdateVersion, RequestDetails theRequest, TransactionDetails theTransactionDetails) {
 		StopWatch w = new StopWatch();
 
 		T resource = theResource;
-		if (JpaInterceptorBroadcaster.hasHooks(Pointcut.STORAGE_VERSION_CONFLICT, myInterceptorBroadcaster, theRequest)) {
-			resource = (T) getContext().getResourceDefinition(theResource).newInstance();
-			getContext().newTerser().cloneInto(theResource, resource, false);
-		}
 
 		preProcessResourceForStorage(resource);
 

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

@@ -53,6 +53,10 @@ public class HapiTransactionService {
 	}
 
 	public <T> T execute(RequestDetails theRequestDetails, TransactionCallback<T> theCallback) {
+		return execute(theRequestDetails, theCallback, null);
+	}
+
+	public <T> T execute(RequestDetails theRequestDetails, TransactionCallback<T> theCallback, Runnable theOnRollback) {
 
 		for (int i = 0; ; i++) {
 			try {
@@ -70,6 +74,10 @@ public class HapiTransactionService {
 			} catch (ResourceVersionConflictException e) {
 				ourLog.debug("Version conflict detected: {}", e.toString());
 
+				if (theOnRollback != null) {
+					theOnRollback.run();
+				}
+
 				HookParams params = new HookParams()
 					.add(RequestDetails.class, theRequestDetails)
 					.addIfMatchesType(ServletRequestDetails.class, theRequestDetails);

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

@@ -6,6 +6,7 @@ import ca.uhn.fhir.interceptor.api.Pointcut;
 import ca.uhn.fhir.jpa.api.config.DaoConfig;
 import ca.uhn.fhir.jpa.binstore.IBinaryStorageSvc;
 import ca.uhn.fhir.jpa.binstore.MemoryBinaryStorageSvcImpl;
+import ca.uhn.fhir.jpa.interceptor.UserRequestRetryVersionConflictsInterceptor;
 import ca.uhn.fhir.jpa.model.util.JpaConstants;
 import ca.uhn.fhir.rest.api.Constants;
 import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
@@ -32,6 +33,7 @@ import java.io.IOException;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.in;
 import static org.hamcrest.Matchers.matchesPattern;
 import static org.junit.jupiter.api.Assertions.*;
 import static org.mockito.Mockito.*;
@@ -546,4 +548,39 @@ public class BinaryAccessProviderR4Test extends BaseResourceProviderR4Test {
 	}
 
 
+
+	@Test
+	public void testWriteWithConflictInterceptor() throws IOException {
+		UserRequestRetryVersionConflictsInterceptor interceptor = new UserRequestRetryVersionConflictsInterceptor();
+		ourRestServer.registerInterceptor(interceptor);
+		try {
+
+			IIdType id = createDocumentReference(false);
+
+			String path = ourServerBase +
+				"/DocumentReference/" + id.getIdPart() + "/" +
+				JpaConstants.OPERATION_BINARY_ACCESS_WRITE +
+				"?path=DocumentReference.content.attachment";
+			HttpPost post = new HttpPost(path);
+			post.setEntity(new ByteArrayEntity(SOME_BYTES, ContentType.IMAGE_JPEG));
+			post.addHeader("Accept", "application/fhir+json; _pretty=true");
+			String attachmentId;
+			try (CloseableHttpResponse resp = ourHttpClient.execute(post)) {
+				String response = IOUtils.toString(resp.getEntity().getContent(), Constants.CHARSET_UTF8);
+				ourLog.info("Response: {}\n{}", resp, response);
+
+				assertEquals(200, resp.getStatusLine().getStatusCode());
+				assertThat(resp.getEntity().getContentType().getValue(), containsString("application/fhir+json"));
+				DocumentReference ref = myFhirCtx.newJsonParser().parseResource(DocumentReference.class, response);
+				Attachment attachment = ref.getContentFirstRep().getAttachment();
+				attachmentId = attachment.getDataElement().getExtensionString(HapiExtensions.EXT_EXTERNALIZED_BINARY_ID);
+				assertThat(attachmentId, matchesPattern("[a-zA-Z0-9]{100}"));
+			}
+
+		} finally {
+			ourRestServer.unregisterInterceptor(interceptor);
+		}
+	}
+
+
 }

hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/util/FhirTerserR4Test.java

@@ -13,6 +13,7 @@ import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.instance.model.api.IPrimitiveType;
 import org.hl7.fhir.r4.model.BooleanType;
 import org.hl7.fhir.r4.model.Bundle;
+import org.hl7.fhir.r4.model.DocumentReference;
 import org.hl7.fhir.r4.model.Element;
 import org.hl7.fhir.r4.model.Enumeration;
 import org.hl7.fhir.r4.model.Enumerations;
@@ -181,6 +182,20 @@ public class FhirTerserR4Test {
 		assertEquals("FOO", ((StringType) exts.get(0).getValue()).getValue());
 	}
 
+	@Test
+	public void testCloneIntoExtensionOnPrimitive() {
+		DocumentReference source = new DocumentReference();
+		source.addContent().getAttachment().getDataElement().addExtension("http://foo", new StringType("bar"));
+
+		DocumentReference target = new DocumentReference();
+
+		ourCtx.newTerser().cloneInto(source, target, true);
+
+		assertEquals(1, target.getContentFirstRep().getAttachment().getDataElement().getExtension().size());
+		assertEquals("http://foo", target.getContentFirstRep().getAttachment().getDataElement().getExtension().get(0).getUrl());
+		assertEquals("bar", target.getContentFirstRep().getAttachment().getDataElement().getExtension().get(0).getValueAsPrimitive().getValueAsString());
+	}
+
 	@Test
 	public void testCloneIntoExtensionWithChildExtension() {
 		Patient patient = new Patient();