mdm multidelete golden resource final resource pair throws (#6001)

* fixed a bug with mdm and multidelete

hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/api/Pointcut.java

@@ -1576,11 +1576,11 @@ public enum Pointcut implements IPointcut {
 
 	/**
 	 * <b>Storage Hook:</b>
-	 * Invoked before a resource will be created, immediately before the resource
+	 * Invoked before a resource will be deleted, immediately before the resource
-	 * is persisted to the database.
+	 * is removed from the database.
 	 * <p>
-	 * Hooks will have access to the contents of the resource being created
+	 * Hooks will have access to the contents of the resource being deleted
-	 * and may choose to make modifications to it. These changes will be
+	 * and may choose to make modifications related to it. These changes will be
 	 * reflected in permanent storage.
 	 * </p>
 	 * Hooks may accept the following parameters:

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_4_0/6000-mdm-multidelete-golden-and-final-source-resource-together-throws.yaml

@@ -0,0 +1,8 @@
+---
+type: fix
+issue: 6000
+title: "In an MDM enabled system with multi-delete enabled, deleting
+        both the final source resource and it's linked golden resource
+        at the same time results in an error being thrown.
+        This has been fixed.
+"

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

@@ -908,7 +908,8 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 			RequestDetails theRequestDetails,
 			TransactionDetails theTransactionDetails) {
 		StopWatch w = new StopWatch();
-		TransactionDetails transactionDetails = new TransactionDetails();
+		TransactionDetails transactionDetails =
+				theTransactionDetails != null ? theTransactionDetails : new TransactionDetails();
 		List<ResourceTable> deletedResources = new ArrayList<>();
 
 		List<IResourcePersistentId<?>> resolvedIds =
@@ -924,6 +925,8 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 
 			T resourceToDelete = myJpaStorageResourceParser.toResource(myResourceType, entity, null, false);
 
+			transactionDetails.addDeletedResourceId(pid);
+
 			// Notify IServerOperationInterceptors about pre-action call
 			HookParams hooks = new HookParams()
 					.add(IBaseResource.class, resourceToDelete)
@@ -988,8 +991,6 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 				deletedResources.size(),
 				w.getMillis());
 
-		theTransactionDetails.addDeletedResourceIds(theResourceIds);
-
 		DeleteMethodOutcome retVal = new DeleteMethodOutcome();
 		retVal.setDeletedEntities(deletedResources);
 		retVal.setOperationOutcome(oo);

hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/helper/MdmHelperR4.java

@@ -6,6 +6,7 @@ import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
 import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao;
 import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
 import ca.uhn.fhir.mdm.model.mdmevents.MdmLinkEvent;
+import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
 import ca.uhn.fhir.rest.server.TransactionLogMessages;
 import ca.uhn.fhir.rest.server.messaging.ResourceOperationMessage;
 import ca.uhn.test.concurrency.PointcutLatch;
@@ -51,7 +52,7 @@ public class MdmHelperR4 extends BaseMdmHelper {
 		String resourceType = myFhirContext.getResourceType(theResource);
 
 		IFhirResourceDao<IBaseResource> dao = myDaoRegistry.getResourceDao(resourceType);
-		return isExternalHttpRequest ? dao.create(theResource, myMockSrd): dao.create(theResource);
+		return isExternalHttpRequest ? dao.create(theResource, myMockSrd): dao.create(theResource, new SystemRequestDetails());
 	}
 
 	public DaoMethodOutcome doUpdateResource(IBaseResource theResource, boolean isExternalHttpRequest) {

hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/interceptor/MdmStorageInterceptorIT.java

@@ -2,6 +2,7 @@ package ca.uhn.fhir.jpa.mdm.interceptor;
 
 import ca.uhn.fhir.interceptor.model.RequestPartitionId;
 import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
+import ca.uhn.fhir.jpa.api.model.DeleteMethodOutcome;
 import ca.uhn.fhir.jpa.api.svc.IIdHelperService;
 import ca.uhn.fhir.jpa.entity.MdmLink;
 import ca.uhn.fhir.jpa.mdm.BaseMdmR4Test;
@@ -16,6 +17,7 @@ import ca.uhn.fhir.mdm.model.CanonicalEID;
 import ca.uhn.fhir.mdm.model.MdmCreateOrUpdateParams;
 import ca.uhn.fhir.mdm.model.MdmTransactionContext;
 import ca.uhn.fhir.mdm.rules.config.MdmSettings;
+import ca.uhn.fhir.model.api.TemporalPrecisionEnum;
 import ca.uhn.fhir.rest.api.Constants;
 import ca.uhn.fhir.rest.api.server.IBundleProvider;
 import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
@@ -27,6 +29,8 @@ import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException;
 import org.hl7.fhir.instance.model.api.IAnyResource;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.instance.model.api.IIdType;
+import org.hl7.fhir.r4.model.ContactPoint;
+import org.hl7.fhir.r4.model.DateType;
 import org.hl7.fhir.r4.model.Enumerations;
 import org.hl7.fhir.r4.model.Medication;
 import org.hl7.fhir.r4.model.Organization;
@@ -34,11 +38,14 @@ import org.hl7.fhir.r4.model.Patient;
 import org.hl7.fhir.r4.model.SearchParameter;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.slf4j.Logger;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.data.domain.Example;
 import org.springframework.test.context.ContextConfiguration;
 
+import java.util.Collections;
 import java.util.Date;
 import java.util.List;
 
@@ -50,6 +57,7 @@ import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 import static org.slf4j.LoggerFactory.getLogger;
 
@@ -101,6 +109,70 @@ public class MdmStorageInterceptorIT extends BaseMdmR4Test {
 		assertLinkCount(0);
 	}
 
+	@ParameterizedTest
+	@ValueSource(booleans = { true, false })
+	public void deleteResourcesByUrl_withMultipleDeleteCatchingSourceAndGoldenResource_deletesWithoutThrowing(boolean theIncludeOtherResources) throws InterruptedException {
+		// setup
+		boolean allowMultipleDelete = myStorageSettings.isAllowMultipleDelete();
+		myStorageSettings.setAllowMultipleDelete(true);
+
+		int linkCount = 0;
+		int resourceCount = 0;
+		myMdmHelper.createWithLatch(buildJanePatient());
+		resourceCount += 2; // patient + golden
+		linkCount++;
+
+		// add some other resources to make it more complex
+		if (theIncludeOtherResources) {
+			Date birthday = new Date();
+			Patient patient = new Patient();
+			patient.getNameFirstRep().addGiven("yui");
+			patient.setBirthDate(birthday);
+			patient.setTelecom(Collections.singletonList(new ContactPoint()
+				.setSystem(ContactPoint.ContactPointSystem.PHONE)
+				.setValue("555-567-5555")));
+			DateType dateType = new DateType(birthday);
+			patient.addIdentifier().setSystem(TEST_ID_SYSTEM).setValue("ID.YUI.123");
+			dateType.setPrecision(TemporalPrecisionEnum.DAY);
+			patient.setBirthDateElement(dateType);
+			patient.setActive(true);
+			for (int i = 0; i < 2; i++) {
+				String familyName = i == 0 ? "hirasawa" : "kotegawa";
+				patient.getNameFirstRep().setFamily(familyName);
+				myMdmHelper.createWithLatch(patient);
+				resourceCount++;
+				linkCount++; // every resource creation creates 1 link
+			}
+			resourceCount++; // for the Golden Resource
+
+			// verify we have at least this many resources
+			SearchParameterMap map = new SearchParameterMap();
+			map.setLoadSynchronous(true);
+			IBundleProvider provider = myPatientDao.search(map, new SystemRequestDetails());
+			assertEquals(resourceCount, provider.size());
+
+			// verify we have the links
+			assertEquals(linkCount, myMdmLinkDao.count());
+		}
+
+		try {
+			// test
+			// filter will delete everything
+			DeleteMethodOutcome outcome = myPatientDao.deleteByUrl("Patient?_lastUpdated=ge2024-01-01", new SystemRequestDetails());
+
+			// validation
+			assertNotNull(outcome);
+			List<MdmLink> links = myMdmLinkDao.findAll();
+			assertTrue(links.isEmpty());
+			SearchParameterMap map = new SearchParameterMap();
+			map.setLoadSynchronous(true);
+			IBundleProvider provider = myPatientDao.search(map, new SystemRequestDetails());
+			assertTrue(provider.getAllResources().isEmpty());
+		} finally {
+			myStorageSettings.setAllowMultipleDelete(allowMultipleDelete);
+		}
+	}
+
 	@Test
 	public void testGoldenResourceDeleted_whenOnlyMatchedResourceDeleted() throws InterruptedException {
 		// Given

hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/interceptor/MdmStorageInterceptor.java

@@ -61,8 +61,10 @@ import org.springframework.stereotype.Service;
 
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.stream.Collectors;
 
@@ -70,15 +72,21 @@ import static ca.uhn.fhir.mdm.api.MdmMatchResultEnum.MATCH;
 import static ca.uhn.fhir.mdm.api.MdmMatchResultEnum.NO_MATCH;
 import static ca.uhn.fhir.mdm.api.MdmMatchResultEnum.POSSIBLE_MATCH;
 
+@SuppressWarnings("rawtypes")
 @Service
 public class MdmStorageInterceptor implements IMdmStorageInterceptor {
 
+	private static final String GOLDEN_RESOURCES_TO_DELETE = "GR_TO_DELETE";
+
 	private static final Logger ourLog = LoggerFactory.getLogger(MdmStorageInterceptor.class);
 
 	// Used to bypass trying to remove mdm links associated to a resource when running mdm-clear batch job, which
 	// deletes all links beforehand, and impacts performance for no action
 	private static final ThreadLocal<Boolean> ourLinksDeletedBeforehand = ThreadLocal.withInitial(() -> Boolean.FALSE);
 
+	@Autowired
+	private IMdmClearHelperSvc<? extends IResourcePersistentId<?>> myIMdmClearHelperSvc;
+
 	@Autowired
 	private IExpungeEverythingService myExpungeEverythingService;
 
@@ -126,7 +134,7 @@ public class MdmStorageInterceptor implements IMdmStorageInterceptor {
 
 		// If running in single EID mode, forbid multiple eids.
 		if (myMdmSettings.isPreventMultipleEids()) {
-			ourLog.debug("Forbidding multiple EIDs on ", theBaseResource);
+			ourLog.debug("Forbidding multiple EIDs on {}", theBaseResource);
 			forbidIfHasMultipleEids(theBaseResource);
 		}
 
@@ -159,7 +167,7 @@ public class MdmStorageInterceptor implements IMdmStorageInterceptor {
 
 		// If running in single EID mode, forbid multiple eids.
 		if (myMdmSettings.isPreventMultipleEids()) {
-			ourLog.debug("Forbidding multiple EIDs on ", theUpdatedResource);
+			ourLog.debug("Forbidding multiple EIDs on {}", theUpdatedResource);
 			forbidIfHasMultipleEids(theUpdatedResource);
 		}
 
@@ -188,17 +196,41 @@ public class MdmStorageInterceptor implements IMdmStorageInterceptor {
 		}
 	}
 
-	@Autowired
+	@Hook(Pointcut.STORAGE_PRECOMMIT_RESOURCE_DELETED)
-	private IMdmClearHelperSvc<? extends IResourcePersistentId<?>> myIMdmClearHelperSvc;
+	public void deletePostCommit(
+			RequestDetails theRequest, IBaseResource theResource, TransactionDetails theTransactionDetails) {
+		Set<IResourcePersistentId> goldenResourceIds = theTransactionDetails.getUserData(GOLDEN_RESOURCES_TO_DELETE);
 
+		if (goldenResourceIds != null) {
+			for (IResourcePersistentId goldenPid : goldenResourceIds) {
+				if (!theTransactionDetails.getDeletedResourceIds().contains(goldenPid)) {
+					IFhirResourceDao<?> dao = myDaoRegistry.getResourceDao(theResource);
+					deleteGoldenResource(goldenPid, dao, theRequest);
+					/*
+					 * We will add the removed id to the deleted list so that
+					 * the deletedResourceId list is accurte for what has been
+					 * deleted.
+					 *
+					 * This benefits other interceptor writers who might want
+					 * to do their own resource deletion on this same pre-commit
+					 * hook (and wouldn't be aware if we did this deletion already).
+					 */
+					theTransactionDetails.addDeletedResourceId(goldenPid);
+				}
+			}
+			theTransactionDetails.putUserData(GOLDEN_RESOURCES_TO_DELETE, null);
+		}
+	}
+
+	@SuppressWarnings("unchecked")
 	@Hook(Pointcut.STORAGE_PRESTORAGE_RESOURCE_DELETED)
-	public void deleteMdmLinks(RequestDetails theRequest, IBaseResource theResource) {
+	public void deleteMdmLinks(
+			RequestDetails theRequest, IBaseResource theResource, TransactionDetails theTransactionDetails) {
 		if (ourLinksDeletedBeforehand.get()) {
 			return;
 		}
 
 		if (myMdmSettings.isSupportedMdmType(myFhirContext.getResourceType(theResource))) {
-
 			IIdType sourceId = theResource.getIdElement().toVersionless();
 			IResourcePersistentId sourcePid =
 					myIdHelperSvc.getPidOrThrowException(RequestPartitionId.allPartitions(), sourceId);
@@ -213,34 +245,49 @@ public class MdmStorageInterceptor implements IMdmStorageInterceptor {
 					? linksByMatchResult.get(POSSIBLE_MATCH)
 					: new ArrayList<>();
 
-			if (isDeletingLastMatchedSourceResouce(sourcePid, matches)) {
+			if (isDeletingLastMatchedSourceResource(sourcePid, matches)) {
-				// We are attempting to delete the only source resource left linked to the golden resource
+				/*
-				// In this case, we should automatically delete the golden resource to prevent orphaning
+				 * We are attempting to delete the only source resource left linked to the golden resource.
-				IFhirResourceDao<?> dao = myDaoRegistry.getResourceDao(theResource);
+				 * In this case, we'll clean up remaining links and mark the orphaned
+				 * golden resource for deletion, which we'll do in STORAGE_PRECOMMIT_RESOURCE_DELETED
+				 */
 				IResourcePersistentId goldenPid = extractGoldenPid(theResource, matches.get(0));
+				if (!theTransactionDetails.getDeletedResourceIds().contains(goldenPid)) {
+					IFhirResourceDao<?> dao = myDaoRegistry.getResourceDao(theResource);
 
-				cleanUpPossibleMatches(possibleMatches, dao, goldenPid, theRequest);
+					cleanUpPossibleMatches(possibleMatches, dao, goldenPid, theRequest);
 
-				IAnyResource goldenResource = (IAnyResource) dao.readByPid(goldenPid);
+					IAnyResource goldenResource = (IAnyResource) dao.readByPid(goldenPid);
-				myMdmLinkDeleteSvc.deleteWithAnyReferenceTo(goldenResource);
+					myMdmLinkDeleteSvc.deleteWithAnyReferenceTo(goldenResource);
 
-				deleteGoldenResource(goldenPid, sourceId, dao, theRequest);
+					/*
+					 * Mark the golden resource for deletion.
+					 * We won't do it yet, because there might be additional deletes coming
+					 * that include this exact golden resource
+					 * (eg, if delete is done by a filter and multiple delete is enabled)
+					 */
+					Set<IResourcePersistentId> goldenIdsToDelete =
+							theTransactionDetails.getUserData(GOLDEN_RESOURCES_TO_DELETE);
+					if (goldenIdsToDelete == null) {
+						goldenIdsToDelete = new HashSet<>();
+					}
+					goldenIdsToDelete.add(goldenPid);
+					theTransactionDetails.putUserData(GOLDEN_RESOURCES_TO_DELETE, goldenIdsToDelete);
+				}
 			}
 			myMdmLinkDeleteSvc.deleteWithAnyReferenceTo(theResource);
 		}
 	}
 
+	@SuppressWarnings("rawtypes")
 	private void deleteGoldenResource(
-			IResourcePersistentId goldenPid,
+			IResourcePersistentId goldenPid, IFhirResourceDao<?> theDao, RequestDetails theRequest) {
-			IIdType theSourceId,
-			IFhirResourceDao<?> theDao,
-			RequestDetails theRequest) {
 		setLinksDeletedBeforehand();
 
 		if (myMdmSettings.isAutoExpungeGoldenResources()) {
 			int numDeleted = deleteExpungeGoldenResource(goldenPid);
 			if (numDeleted > 0) {
-				ourLog.info("Removed {} golden resource(s) with references to {}", numDeleted, theSourceId);
+				ourLog.info("Removed {} golden resource(s).", numDeleted);
 			}
 		} else {
 			String url = theRequest == null ? "" : theRequest.getCompleteUrl();
@@ -289,7 +336,7 @@ public class MdmStorageInterceptor implements IMdmStorageInterceptor {
 		return goldenPid;
 	}
 
-	private boolean isDeletingLastMatchedSourceResouce(IResourcePersistentId theSourcePid, List<IMdmLink> theMatches) {
+	private boolean isDeletingLastMatchedSourceResource(IResourcePersistentId theSourcePid, List<IMdmLink> theMatches) {
 		return theMatches.size() == 1
 				&& theMatches.get(0).getSourcePersistenceId().equals(theSourcePid);
 	}
@@ -302,6 +349,7 @@ public class MdmStorageInterceptor implements IMdmStorageInterceptor {
 		return retVal;
 	}
 
+	@SuppressWarnings("unchecked")
 	private int deleteExpungeGoldenResource(IResourcePersistentId theGoldenPid) {
 		IDeleteExpungeSvc deleteExpungeSvc = myIMdmClearHelperSvc.getDeleteExpungeSvc();
 		return deleteExpungeSvc.deleteExpunge(new ArrayList<>(Collections.singleton(theGoldenPid)), false, null);