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 cac6a1725a6..fd23e3160fb 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 @@ -1576,11 +1576,11 @@ public enum Pointcut implements IPointcut { /** * Storage Hook: - * Invoked before a resource will be created, immediately before the resource - * is persisted to the database. + * Invoked before a resource will be deleted, immediately before the resource + * is removed from the database. *

- * Hooks will have access to the contents of the resource being created - * and may choose to make modifications to it. These changes will be + * Hooks will have access to the contents of the resource being deleted + * and may choose to make modifications related to it. These changes will be * reflected in permanent storage. *

* Hooks may accept the following parameters: diff --git a/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 b/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 new file mode 100644 index 00000000000..6dc27a0aae2 --- /dev/null +++ b/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. +" diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java index e6e80b214ea..38100cc0519 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java @@ -908,7 +908,8 @@ public abstract class BaseHapiFhirResourceDao extends B RequestDetails theRequestDetails, TransactionDetails theTransactionDetails) { StopWatch w = new StopWatch(); - TransactionDetails transactionDetails = new TransactionDetails(); + TransactionDetails transactionDetails = + theTransactionDetails != null ? theTransactionDetails : new TransactionDetails(); List deletedResources = new ArrayList<>(); List> resolvedIds = @@ -924,6 +925,8 @@ public abstract class BaseHapiFhirResourceDao 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 extends B deletedResources.size(), w.getMillis()); - theTransactionDetails.addDeletedResourceIds(theResourceIds); - DeleteMethodOutcome retVal = new DeleteMethodOutcome(); retVal.setDeletedEntities(deletedResources); retVal.setOperationOutcome(oo); diff --git a/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/helper/MdmHelperR4.java b/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/helper/MdmHelperR4.java index ec9bab6755b..d6d397988d5 100644 --- a/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/helper/MdmHelperR4.java +++ b/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 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) { diff --git a/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/interceptor/MdmStorageInterceptorIT.java b/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/interceptor/MdmStorageInterceptorIT.java index 98064590372..3224104cf35 100644 --- a/hapi-fhir-jpaserver-mdm/src/test/java/ca/uhn/fhir/jpa/mdm/interceptor/MdmStorageInterceptorIT.java +++ b/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 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 diff --git a/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/interceptor/MdmStorageInterceptor.java b/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/interceptor/MdmStorageInterceptor.java index c7819736162..1851e12d7b7 100644 --- a/hapi-fhir-server-mdm/src/main/java/ca/uhn/fhir/mdm/interceptor/MdmStorageInterceptor.java +++ b/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 ourLinksDeletedBeforehand = ThreadLocal.withInitial(() -> Boolean.FALSE); + @Autowired + private IMdmClearHelperSvc> 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 - private IMdmClearHelperSvc> myIMdmClearHelperSvc; + @Hook(Pointcut.STORAGE_PRECOMMIT_RESOURCE_DELETED) + public void deletePostCommit( + RequestDetails theRequest, IBaseResource theResource, TransactionDetails theTransactionDetails) { + Set 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)) { - // 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 - IFhirResourceDao dao = myDaoRegistry.getResourceDao(theResource); + if (isDeletingLastMatchedSourceResource(sourcePid, matches)) { + /* + * We are attempting to delete the only source resource left linked to the golden resource. + * 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); - myMdmLinkDeleteSvc.deleteWithAnyReferenceTo(goldenResource); + IAnyResource goldenResource = (IAnyResource) dao.readByPid(goldenPid); + 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 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, - IIdType theSourceId, - IFhirResourceDao theDao, - RequestDetails theRequest) { + IResourcePersistentId goldenPid, 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 theMatches) { + private boolean isDeletingLastMatchedSourceResource(IResourcePersistentId theSourcePid, List 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);