Duplicate uploads of binary content for externalized attachments (#7184)

* Duplicate uploads of binary content for externalized attachments

Author: volodymyr-korzh

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

@@ -27,10 +27,16 @@
 import ca.uhn.fhir.model.primitive.CodeDt;
 import org.apache.commons.lang3.Validate;
 import org.hl7.fhir.instance.model.api.IBase;
+import org.hl7.fhir.instance.model.api.IBaseExtension;
+import org.hl7.fhir.instance.model.api.IBaseHasExtensions;
 import org.hl7.fhir.instance.model.api.ICompositeType;
 import org.hl7.fhir.instance.model.api.IPrimitiveType;
 
 import java.util.List;
+import java.util.Optional;
+import java.util.function.Predicate;
+
+import static org.apache.commons.lang3.StringUtils.isNotBlank;
 
 public class AttachmentUtil {
 
@@ -93,6 +99,19 @@ public static void setSize(FhirContext theContext, ICompositeType theAttachment,
 		}
 	}
 
+	@SuppressWarnings("unchecked")
+	public static Optional<String> getFirstExtension(
+			IBaseHasExtensions theAttachment, String theExtension, Predicate<? super IBaseExtension<?, ?>> theFilter) {
+		return theAttachment.getExtension().stream()
+				.filter(theFilter)
+				.filter(t -> theExtension.equals(t.getUrl()))
+				.filter(t -> t.getValue() instanceof IPrimitiveType)
+				.map(t -> (IPrimitiveType<String>) t.getValue())
+				.map(t -> t.getValue())
+				.filter(t -> isNotBlank(t))
+				.findFirst();
+	}
+
 	/**
 	 * This is internal API- Use with caution as it may change
 	 */

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

@@ -81,6 +81,12 @@ public class HapiExtensions {
 	public static final String EXT_EXTERNALIZED_BINARY_ID =
 			"http://hapifhir.io/fhir/StructureDefinition/externalized-binary-id";
 
+	/**
+	 * Extension ID for the SHA-256 hash of external binary content
+	 */
+	public static final String EXT_EXTERNALIZED_BINARY_HASH_SHA_256 =
+			"http://hapifhir.io/fhir/StructureDefinition/externalized-binary-hash-sha256";
+
 	/**
 	 * For subscription, deliver a bundle containing a search result instead of just a single resource
 	 */

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/8_4_0/7183-duplicate-uploads-of-binary-content-for-externalized-binary-storage.yaml

@@ -0,0 +1,7 @@
+---
+type: fix
+issue: 7183
+jira: SMILE-10474
+title: "Previously, updating the binary content of a DocumentReference or Binary resource multiple times with 
+the same data via the $binary-access-write operation or a resource update would cause a new resource version 
+to be created and the binary data to be re-uploaded to external binary storage. This has been fixed."

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/packages/PackageInstallerSvcR4Test.java

@@ -374,7 +374,7 @@ public void testInstallR4PackageWithExternalizedBinaries() throws Exception {
 		runInTransaction(() -> {
 			SearchParameterMap map = SearchParameterMap.newSynchronous();
 			map.add(StructureDefinition.SP_URL, new UriParam("http://hl7.org/fhir/uv/shorthand/CodeSystem/shorthand-code-system"));
-			IBundleProvider result = myCodeSystemDao.search(map);
+			IBundleProvider result = myCodeSystemDao.search(map, mySrd);
 			assertEquals(1, result.sizeOrThrowNpe());
 			IBaseResource resource = result.getResources(0, 1).get(0);
 			assertEquals("CodeSystem/shorthand-code-system/_history/1", resource.getIdElement().toString());

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

@@ -19,13 +19,13 @@
  */
 package ca.uhn.fhir.jpa.binary.api;
 
+import ca.uhn.fhir.util.AttachmentUtil;
 import ca.uhn.fhir.util.HapiExtensions;
+import org.hl7.fhir.instance.model.api.IBaseExtension;
 import org.hl7.fhir.instance.model.api.IBaseHasExtensions;
-import org.hl7.fhir.instance.model.api.IPrimitiveType;
 
 import java.util.Optional;
-
-import static org.apache.commons.lang3.StringUtils.isNotBlank;
+import java.util.function.Predicate;
 
 /**
  * Wraps an Attachment datatype or Binary resource, since they both
@@ -45,14 +45,21 @@ public interface IBinaryTarget {
 
 	IBaseHasExtensions getTarget();
 
-	@SuppressWarnings("unchecked")
 	default Optional<String> getAttachmentId() {
-		return getTarget().getExtension().stream()
-				.filter(t -> HapiExtensions.EXT_EXTERNALIZED_BINARY_ID.equals(t.getUrl()))
-				.filter(t -> t.getValue() instanceof IPrimitiveType)
-				.map(t -> (IPrimitiveType<String>) t.getValue())
-				.map(t -> t.getValue())
-				.filter(t -> isNotBlank(t))
-				.findFirst();
+		return AttachmentUtil.getFirstExtension(getTarget(), HapiExtensions.EXT_EXTERNALIZED_BINARY_ID, e -> true);
+	}
+
+	default Optional<String> getAttachmentIdFiltered(Predicate<? super IBaseExtension<?, ?>> theFilter) {
+		return AttachmentUtil.getFirstExtension(getTarget(), HapiExtensions.EXT_EXTERNALIZED_BINARY_ID, theFilter);
+	}
+
+	default Optional<String> getHashExtension() {
+		return AttachmentUtil.getFirstExtension(
+				getTarget(), HapiExtensions.EXT_EXTERNALIZED_BINARY_HASH_SHA_256, e -> true);
+	}
+
+	default Optional<String> getHashExtensionFiltered(Predicate<? super IBaseExtension<?, ?>> theFilter) {
+		return AttachmentUtil.getFirstExtension(
+				getTarget(), HapiExtensions.EXT_EXTERNALIZED_BINARY_HASH_SHA_256, theFilter);
 	}
 }

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

@@ -45,8 +45,8 @@
 import ca.uhn.fhir.util.IModelVisitor2;
 import jakarta.annotation.Nonnull;
 import org.apache.commons.io.FileUtils;
-import org.apache.commons.lang3.StringUtils;
 import org.hl7.fhir.instance.model.api.IBase;
+import org.hl7.fhir.instance.model.api.IBaseExtension;
 import org.hl7.fhir.instance.model.api.IBaseHasExtensions;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.instance.model.api.IIdType;
@@ -60,20 +60,41 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.ArrayList;
-import java.util.HashSet;
+import java.util.Collection;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
+import static ca.uhn.fhir.util.HapiExtensions.EXT_EXTERNALIZED_BINARY_HASH_SHA_256;
 import static ca.uhn.fhir.util.HapiExtensions.EXT_EXTERNALIZED_BINARY_ID;
 import static org.apache.commons.lang3.StringUtils.isNotBlank;
 
 @Interceptor
 public class BinaryStorageInterceptor<T extends IPrimitiveType<byte[]>> {
 
 	private static final Logger ourLog = LoggerFactory.getLogger(BinaryStorageInterceptor.class);
+	/**
+	 * UserData key that can be set in {@link RequestDetails#getUserData()} to override
+	 * the {@link #isAllowAutoInflateBinaries()} setting for a single request.
+	 * <p>
+	 * Possible values:
+	 * <ul>
+	 *   <li>{@code Boolean.TRUE}  – force binary inflation even if globally disabled</li>
+	 *   <li>{@code Boolean.FALSE} – skip binary inflation even if globally enabled</li>
+	 *   <li>Absent                – use the global {@code isAllowAutoInflateBinaries()} setting</li>
+	 * </ul>
+	 */
+	public static final String AUTO_INFLATE_BINARY_CONTENT_KEY =
+			BinaryStorageInterceptor.class.getName() + "_AUTO_INFLATE_BINARY_CONTENT";
+
+	private static final Predicate<IBaseExtension<?, ?>> EXTENSION_FILTER_PREDICATE =
+			ext -> ext.getUserData(JpaConstants.EXTENSION_EXT_SYSTEMDEFINED) == null;
 
 	@Autowired
 	private IBinaryStorageSvc myBinaryStorageSvc;
@@ -145,7 +166,7 @@ public void extractLargeBinariesBeforeCreate(
 			IBaseResource theResource,
 			Pointcut thePointcut)
 			throws IOException {
-		extractLargeBinaries(theRequestDetails, theTransactionDetails, theResource, thePointcut);
+		extractLargeBinaries(theRequestDetails, theTransactionDetails, theResource, null, thePointcut);
 	}
 
 	@Hook(Pointcut.STORAGE_PRESTORAGE_RESOURCE_UPDATED)
@@ -156,64 +177,60 @@ public void extractLargeBinariesBeforeUpdate(
 			IBaseResource theResource,
 			Pointcut thePointcut)
 			throws IOException {
-		blockIllegalExternalBinaryIds(thePreviousResource, theResource);
-		extractLargeBinaries(theRequestDetails, theTransactionDetails, theResource, thePointcut);
+		blockIllegalExternalExtensions(thePreviousResource, theResource);
+		extractLargeBinaries(theRequestDetails, theTransactionDetails, theResource, thePreviousResource, thePointcut);
 	}
 
 	/**
-	 * Don't allow clients to submit resources with binary storage attachments declared unless the ID was already in the
-	 * resource. In other words, only HAPI itself may add a binary storage ID extension to a resource unless that
-	 * extension was already present.
+	 * Prevents clients from submitting resources with binary storage ID or binary storage hash extensions,
+	 * unless those extensions were already present in the existing resource. In other words, only HAPI itself
+	 * may add these extensions to a resource.
 	 */
-	private void blockIllegalExternalBinaryIds(IBaseResource thePreviousResource, IBaseResource theResource) {
-		Set<String> existingBinaryIds = new HashSet<>();
-		if (thePreviousResource != null) {
-			List<T> base64fields =
-					myCtx.newTerser().getAllPopulatedChildElementsOfType(thePreviousResource, myBinaryType);
-			for (IPrimitiveType<byte[]> nextBase64 : base64fields) {
-				if (nextBase64 instanceof IBaseHasExtensions) {
-					((IBaseHasExtensions) nextBase64)
-							.getExtension().stream()
-									.filter(t -> t.getUserData(JpaConstants.EXTENSION_EXT_SYSTEMDEFINED) == null)
-									.filter(t -> EXT_EXTERNALIZED_BINARY_ID.equals(t.getUrl()))
-									.map(t -> (IPrimitiveType<?>) t.getValue())
-									.map(IPrimitiveType::getValueAsString)
-									.filter(StringUtils::isNotBlank)
-									.forEach(existingBinaryIds::add);
-				}
+	private void blockIllegalExternalExtensions(IBaseResource thePreviousResource, IBaseResource theResource) {
+		Map<String, String> existingAttachmentIdToHashMap = buildHashAttachmentIdMap(thePreviousResource, true);
+		Set<String> existingBinaryIds = existingAttachmentIdToHashMap.keySet();
+		Collection<String> existingHashes = existingAttachmentIdToHashMap.values();
+
+		recursivelyScanResourceForBinaryData(theResource).forEach(target -> {
+			String id =
+					target.getAttachmentIdFiltered(EXTENSION_FILTER_PREDICATE).orElse(null);
+			String hash =
+					target.getHashExtensionFiltered(EXTENSION_FILTER_PREDICATE).orElse(null);
+
+			// binaryId check
+			if (id != null && !existingBinaryIds.contains(id)) {
+				throwIllegalExtension(EXT_EXTERNALIZED_BINARY_ID, id);
+			}
+			// hash check
+			if (hash != null && !existingHashes.contains(hash)) {
+				throwIllegalExtension(EXT_EXTERNALIZED_BINARY_HASH_SHA_256, hash);
 			}
-		}
 
-		List<T> base64fields = myCtx.newTerser().getAllPopulatedChildElementsOfType(theResource, myBinaryType);
-		for (IPrimitiveType<byte[]> nextBase64 : base64fields) {
-			if (nextBase64 instanceof IBaseHasExtensions) {
-				Optional<String> hasExternalizedBinaryReference = ((IBaseHasExtensions) nextBase64)
-						.getExtension().stream()
-								.filter(t -> t.getUserData(JpaConstants.EXTENSION_EXT_SYSTEMDEFINED) == null)
-								.filter(t -> t.getUrl().equals(EXT_EXTERNALIZED_BINARY_ID))
-								.map(t -> (IPrimitiveType<?>) t.getValue())
-								.map(IPrimitiveType::getValueAsString)
-								.filter(StringUtils::isNotBlank)
-								.filter(t -> !existingBinaryIds.contains(t))
-								.findFirst();
-
-				if (hasExternalizedBinaryReference.isPresent()) {
-					String msg = myCtx.getLocalizer()
-							.getMessage(
-									BinaryStorageInterceptor.class,
-									"externalizedBinaryStorageExtensionFoundInRequestBody",
-									EXT_EXTERNALIZED_BINARY_ID,
-									hasExternalizedBinaryReference.get());
-					throw new InvalidRequestException(Msg.code(1329) + msg);
+			// binaryId - hash pair consistency check
+			if (id != null && hash != null) {
+				String expected = existingAttachmentIdToHashMap.get(id);
+				if (!Objects.equals(expected, hash)) {
+					throwIllegalExtension(EXT_EXTERNALIZED_BINARY_HASH_SHA_256, hash);
 				}
 			}
-		}
+		});
+	}
+
+	private void throwIllegalExtension(String theExtensionUrl, String theValue) {
+		String msg = myCtx.getLocalizer()
+				.getMessage(
+						BinaryStorageInterceptor.class,
+						"externalizedBinaryStorageExtensionFoundInRequestBody",
+						theExtensionUrl,
+						theValue);
+		throw new InvalidRequestException(Msg.code(1329) + msg);
 	}
 
 	private void extractLargeBinaries(
 			RequestDetails theRequestDetails,
 			TransactionDetails theTransactionDetails,
 			IBaseResource theResource,
+			IBaseResource thePreviousResource,
 			Pointcut thePointcut)
 			throws IOException {
 
@@ -223,6 +240,7 @@ private void extractLargeBinaries(
 			resourceId = new IdType(resourceType + "/" + resourceId.getIdPart());
 		}
 
+		Map<String, String> existingHashToAttachmentId = buildHashAttachmentIdMap(thePreviousResource, false);
 		List<IBinaryTarget> attachments = recursivelyScanResourceForBinaryData(theResource);
 		for (IBinaryTarget nextTarget : attachments) {
 			byte[] data = nextTarget.getData();
@@ -233,13 +251,16 @@ private void extractLargeBinaries(
 				boolean shouldStoreBlob =
 						myBinaryStorageSvc.shouldStoreBinaryContent(nextPayloadLength, resourceId, nextContentType);
 				if (shouldStoreBlob) {
-
+					String binaryContentHash = myBinaryAccessProvider.getBinaryContentHash(data);
 					String newBinaryContentId;
 					if (thePointcut == Pointcut.STORAGE_PRESTORAGE_RESOURCE_UPDATED) {
-						ByteArrayInputStream inputStream = new ByteArrayInputStream(data);
-						StoredDetails storedDetails = myBinaryStorageSvc.storeBinaryContent(
-								resourceId, null, nextContentType, inputStream, theRequestDetails);
-						newBinaryContentId = storedDetails.getBinaryContentId();
+						newBinaryContentId = storeBinaryContentIfRequired(
+								theRequestDetails,
+								existingHashToAttachmentId,
+								binaryContentHash,
+								data,
+								resourceId,
+								nextContentType);
 					} else {
 						assert thePointcut == Pointcut.STORAGE_PRESTORAGE_RESOURCE_CREATED : thePointcut.name();
 						newBinaryContentId = myBinaryStorageSvc.newBinaryContentId();
@@ -264,11 +285,63 @@ private void extractLargeBinaries(
 					}
 
 					myBinaryAccessProvider.replaceDataWithExtension(nextTarget, newBinaryContentId);
+					myBinaryAccessProvider.addHashExtension(nextTarget, binaryContentHash);
 				}
 			}
 		}
 	}
 
+	/**
+	 * Builds a map of SHA-256 hashes to corresponding attachment IDs from the given FHIR resource.
+	 * @return A {@link Map} with keys as SHA-256 binary content hashes and values as attachment IDs.
+	 */
+	private Map<String, String> buildHashAttachmentIdMap(
+			IBaseResource thePreviousResource, boolean isAttachmentIdToHash) {
+		Map<String, String> result = new HashMap<>();
+
+		if (thePreviousResource == null) {
+			return result;
+		}
+
+		List<IBinaryTarget> previousAttachments = recursivelyScanResourceForBinaryData(thePreviousResource);
+		for (IBinaryTarget attachment : previousAttachments) {
+			Optional<String> hashOpt = attachment.getHashExtensionFiltered(EXTENSION_FILTER_PREDICATE);
+			Optional<String> idOpt = attachment.getAttachmentIdFiltered(EXTENSION_FILTER_PREDICATE);
+
+			if (isAttachmentIdToHash) {
+				idOpt.ifPresent(id -> result.put(id, hashOpt.orElse(null)));
+			} else {
+				hashOpt.ifPresent(hash -> result.put(hash, idOpt.orElse(null)));
+			}
+		}
+		return result;
+	}
+
+	/**
+	 * This method checks if the given binary content (based on its SHA-256 hash) is already stored in previous
+	 * resource version. If it is, it reuses the existing attachment ID to avoid saving the same content again.
+	 * If it's not found, it stores the new content and returns the newly generated attachment ID.
+	 */
+	private String storeBinaryContentIfRequired(
+			RequestDetails theRequestDetails,
+			Map<String, String> existingHashToAttachmentId,
+			String binaryContentHash,
+			byte[] data,
+			IIdType resourceId,
+			String nextContentType)
+			throws IOException {
+		if (existingHashToAttachmentId.get(binaryContentHash) != null) {
+			// input binary content is the same as existing binary content, reuse existing binaryId
+			return existingHashToAttachmentId.get(binaryContentHash);
+		} else {
+			// there is no existing binary content or content is different, store new content in binary storage
+			ByteArrayInputStream inputStream = new ByteArrayInputStream(data);
+			StoredDetails storedDetails = myBinaryStorageSvc.storeBinaryContent(
+					resourceId, null, nextContentType, inputStream, theRequestDetails);
+			return storedDetails.getBinaryContentId();
+		}
+	}
+
 	/**
 	 * This invokes the {@link Pointcut#STORAGE_BINARY_ASSIGN_BLOB_ID_PREFIX} hook and returns the prefix to use for the blob ID, or null if there are no implementers.
 	 * @return A string, which will be used to prefix the blob ID. May be null.
@@ -350,8 +423,14 @@ public String getDeferredListKey() {
 	}
 
 	@Hook(Pointcut.STORAGE_PRESHOW_RESOURCES)
-	public void preShow(IPreResourceShowDetails theDetails) throws IOException {
-		if (!isAllowAutoInflateBinaries()) {
+	public void preShow(IPreResourceShowDetails theDetails, RequestDetails theRequestDetails) throws IOException {
+		boolean isAllowAutoInflateBinaries = isAllowAutoInflateBinaries();
+		// Override isAllowAutoInflateBinaries setting if AUTO_INFLATE_BINARY_CONTENT flag is present in userData
+		if (theRequestDetails != null && theRequestDetails.getUserData().containsKey(AUTO_INFLATE_BINARY_CONTENT_KEY)) {
+			isAllowAutoInflateBinaries =
+					Boolean.TRUE.equals(theRequestDetails.getUserData().get(AUTO_INFLATE_BINARY_CONTENT_KEY));
+		}
+		if (!isAllowAutoInflateBinaries) {
 			return;
 		}