Merge pull request #1863 from jamesagnew/im_20200520_cascade_delete

Im 20200520 cascade delete

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

@@ -1284,7 +1284,7 @@ public enum Pointcut {
 
 	/**
 	 * <b>Storage Hook:</b>
-	 * Invoked before a resource will be created
+	 * Invoked before a resource will be deleted
 	 * <p>
 	 * Hooks will have access to the contents of the resource being deleted
 	 * but should not make any changes as storage has already occurred
@@ -1322,7 +1322,7 @@ public enum Pointcut {
 
 	/**
 	 * <b>Storage Hook:</b>
-	 * Invoked when a resource delete operation is about to fail due to referential integrity hcts.
+	 * Invoked when a resource delete operation is about to fail due to referential integrity checks. Intended for use with {@literal ca.uhn.fhir.jpa.interceptor.CascadingDeleteInterceptor}.
 	 * <p>
 	 * Hooks will have access to the list of resources that have references to the resource being deleted.
 	 * </p>

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_1_0/1863-enable-configuring-max-delete-conflicts-handled.yaml

@@ -0,0 +1,6 @@
+---
+type: fix
+issue: 1863
+title: "Cascade deletes were failing in cases where the resource being deleted had more than 600 conflicts due to a hard-coded limit
+   on the number of conflicts that the CascadingDeleteInterceptor was allowed to process at a time. This hard-coded limit has been
+   replaced with an optional configuration parameter."

hapi-fhir-jpaserver-api/src/main/java/ca/uhn/fhir/jpa/api/config/DaoConfig.java

@@ -83,6 +83,7 @@ public class DaoConfig {
 	private static final Logger ourLog = LoggerFactory.getLogger(DaoConfig.class);
 	private static final int DEFAULT_EXPUNGE_BATCH_SIZE = 800;
 	private IndexEnabledEnum myIndexMissingFieldsEnabled = IndexEnabledEnum.DISABLED;
+	private static final int DEFAULT_MAXIMUM_DELETE_CONFLICT_COUNT = 60;
 
 	/**
 	 * Child Configurations
@@ -153,6 +154,10 @@ public class DaoConfig {
 	private ClientIdStrategyEnum myResourceClientIdStrategy = ClientIdStrategyEnum.ALPHANUMERIC;
 	private boolean myFilterParameterEnabled = false;
 	private StoreMetaSourceInformationEnum myStoreMetaSourceInformation = StoreMetaSourceInformationEnum.SOURCE_URI_AND_REQUEST_ID;
+	/**
+	 * update setter javadoc if default changes
+	 */
+	private Integer myMaximumDeleteConflictQueryCount = DEFAULT_MAXIMUM_DELETE_CONFLICT_COUNT;
 	/**
 	 * Do not change default of {@code true}!
 	 *
@@ -2021,4 +2026,33 @@ public class DaoConfig {
 		 */
 		ANY
 	}
+
+	/**
+	 * <p>
+	 * This determines the maximum number of conflicts that should be fetched and handled while retrying a delete of a resource.
+	 * </p>
+	 * <p>
+	 * The default value for this setting is {@code 60}.
+	 * </p>
+	 *
+	 * @since 5.1.0
+	 */
+	public Integer getMaximumDeleteConflictQueryCount() {
+		return myMaximumDeleteConflictQueryCount;
+	}
+
+	/**
+	 * <p>
+	 * This determines the maximum number of conflicts that should be fetched and handled while retrying a delete of a resource.
+	 * </p>
+	 * <p>
+	 * The default value for this setting is {@code 60}.
+	 * </p>
+	 *
+	 * @since 5.1.0
+	 */
+	public void setMaximumDeleteConflictQueryCount(Integer theMaximumDeleteConflictQueryCount) {
+		myMaximumDeleteConflictQueryCount = theMaximumDeleteConflictQueryCount;
+	}
+
 }

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/delete/DeleteConflictService.java

@@ -38,21 +38,21 @@ import ca.uhn.fhir.rest.api.server.storage.TransactionDetails;
 import ca.uhn.fhir.rest.server.exceptions.ResourceVersionConflictException;
 import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails;
 import ca.uhn.fhir.util.OperationOutcomeUtil;
+import com.google.common.annotations.VisibleForTesting;
 import org.hl7.fhir.instance.model.api.IBaseOperationOutcome;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Service;
 
-import java.util.Iterator;
 import java.util.List;
 
 @Service
 public class DeleteConflictService {
 	private static final Logger ourLog = LoggerFactory.getLogger(DeleteConflictService.class);
 	public static final int FIRST_QUERY_RESULT_COUNT = 1;
-	public static final int RETRY_QUERY_RESULT_COUNT = 60;
+	public static int MAX_RETRY_ATTEMPTS = 10;
-	public static final int MAX_RETRY_ATTEMPTS = 10;
+	public static String MAX_RETRY_ATTEMPTS_EXCEEDED_MSG = "Requested delete operation stopped before all conflicts were handled. May need to increase the configured Maximum Delete Conflict Query Count.";
 
 	@Autowired
 	DeleteConflictFinderService myDeleteConflictFinderService;
@@ -81,11 +81,16 @@ public class DeleteConflictService {
 		while (outcome != null) {
 			int shouldRetryCount = Math.min(outcome.getShouldRetryCount(), MAX_RETRY_ATTEMPTS);
 			if (!(retryCount < shouldRetryCount)) break;
-			newConflicts = new DeleteConflictList();
+			newConflicts = new DeleteConflictList(newConflicts);
-			outcome = findAndHandleConflicts(theRequest, newConflicts, theEntity, theForValidate, RETRY_QUERY_RESULT_COUNT, theTransactionDetails);
+			outcome = findAndHandleConflicts(theRequest, newConflicts, theEntity, theForValidate, myDaoConfig.getMaximumDeleteConflictQueryCount(), theTransactionDetails);
 			++retryCount;
 		}
 		theDeleteConflicts.addAll(newConflicts);
+		if(retryCount >= MAX_RETRY_ATTEMPTS && !theDeleteConflicts.isEmpty()) {
+			IBaseOperationOutcome oo = OperationOutcomeUtil.newInstance(myFhirContext);
+			OperationOutcomeUtil.addIssue(myFhirContext, oo, BaseHapiFhirDao.OO_SEVERITY_ERROR, MAX_RETRY_ATTEMPTS_EXCEEDED_MSG,null, "processing");
+			throw new ResourceVersionConflictException(MAX_RETRY_ATTEMPTS_EXCEEDED_MSG, oo);
+		}
 		return retryCount;
 	}
 
@@ -143,17 +148,13 @@ public class DeleteConflictService {
 		IBaseOperationOutcome oo = OperationOutcomeUtil.newInstance(theFhirContext);
 		String firstMsg = null;
 
-		Iterator<DeleteConflict> iterator = theDeleteConflicts.iterator();
+		for (DeleteConflict next : theDeleteConflicts) {
-		while (iterator.hasNext()) {
+			String msg = "Unable to delete " +
-			DeleteConflict next = iterator.next();
+				next.getTargetId().toUnqualifiedVersionless().getValue() +
-			StringBuilder b = new StringBuilder();
+				" because at least one resource has a reference to this resource. First reference found was resource " +
-			b.append("Unable to delete ");
+				next.getSourceId().toUnqualifiedVersionless().getValue() +
-			b.append(next.getTargetId().toUnqualifiedVersionless().getValue());
+				" in path " +
-			b.append(" because at least one resource has a reference to this resource. First reference found was resource ");
+				next.getSourcePath();
-			b.append(next.getSourceId().toUnqualifiedVersionless().getValue());
-			b.append(" in path ");
-			b.append(next.getSourcePath());
-			String msg = b.toString();
 			if (firstMsg == null) {
 				firstMsg = msg;
 			}
@@ -162,4 +163,9 @@ public class DeleteConflictService {
 
 		throw new ResourceVersionConflictException(firstMsg, oo);
 	}
+
+	@VisibleForTesting
+	static void setMaxRetryAttempts(Integer theMaxRetryAttempts) {
+		MAX_RETRY_ATTEMPTS = theMaxRetryAttempts;
+	}
 }

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/delete/DeleteConflictServiceR4Test.java

@@ -2,6 +2,7 @@ package ca.uhn.fhir.jpa.delete;
 
 import ca.uhn.fhir.interceptor.api.Hook;
 import ca.uhn.fhir.interceptor.api.Pointcut;
+import ca.uhn.fhir.jpa.api.config.DaoConfig;
 import ca.uhn.fhir.jpa.api.model.DeleteConflict;
 import ca.uhn.fhir.jpa.api.model.DeleteConflictList;
 import ca.uhn.fhir.jpa.dao.r4.BaseJpaR4Test;
@@ -17,9 +18,9 @@ import org.junit.Before;
 import org.junit.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.springframework.beans.factory.annotation.Autowired;
 
 import java.util.ArrayList;
-import java.util.Iterator;
 import java.util.List;
 import java.util.function.Function;
 
@@ -30,7 +31,10 @@ import static org.junit.Assert.fail;
 public class DeleteConflictServiceR4Test extends BaseJpaR4Test {
 	private static final Logger ourLog = LoggerFactory.getLogger(DeleteConflictServiceR4Test.class);
 
-	private DeleteConflictInterceptor myDeleteInterceptor = new DeleteConflictInterceptor();
+	@Autowired
+	DaoConfig myDaoConfig;
+
+	private final DeleteConflictInterceptor myDeleteInterceptor = new DeleteConflictInterceptor();
 	private int myInterceptorDeleteCount;
 
 	@Before
@@ -135,6 +139,48 @@ public class DeleteConflictServiceR4Test extends BaseJpaR4Test {
 		assertEquals(3, myInterceptorDeleteCount);
 	}
 
+	@Test
+	public void testDeleteHookDeletesLargeNumberOfConflicts() {
+
+		Organization organization = new Organization();
+		organization.setName("FOO");
+		IIdType organizationId = myOrganizationDao.create(organization).getId().toUnqualifiedVersionless();
+
+		// Create 12 conflicts.
+		for (int j=0; j < 12 ; j++) {
+			Patient patient = new Patient();
+			patient.setManagingOrganization(new Reference(organizationId));
+			myPatientDao.create(patient).getId().toUnqualifiedVersionless();
+		}
+
+		DeleteConflictService.setMaxRetryAttempts(3);
+		myDaoConfig.setMaximumDeleteConflictQueryCount(5);
+		myDeleteInterceptor.deleteConflictFunction = this::deleteConflictsFixedRetryCount;
+		try {
+			myOrganizationDao.delete(organizationId);
+			// Needs a fourth and final pass to ensure that all conflicts are now gone.
+			fail();
+		} catch (ResourceVersionConflictException e) {
+			assertEquals(DeleteConflictService.MAX_RETRY_ATTEMPTS_EXCEEDED_MSG, e.getMessage());
+		}
+
+		// Try again with Maximum conflict count set to 6.
+		myDeleteInterceptor.myCallCount=0;
+		myInterceptorDeleteCount = 0;
+		myDaoConfig.setMaximumDeleteConflictQueryCount(6);
+
+		try {
+			myOrganizationDao.delete(organizationId);
+		} catch (ResourceVersionConflictException e) {
+			fail();
+		}
+
+		assertNotNull(myDeleteInterceptor.myDeleteConflictList);
+		assertEquals(3, myDeleteInterceptor.myCallCount);
+		assertEquals(12, myInterceptorDeleteCount);
+
+	}
+
 	@Test
 	public void testBadInterceptorNoInfiniteLoop() {
 		Organization organization = new Organization();
@@ -143,7 +189,7 @@ public class DeleteConflictServiceR4Test extends BaseJpaR4Test {
 
 		Patient patient = new Patient();
 		patient.setManagingOrganization(new Reference(organizationId));
-		IIdType patientId = myPatientDao.create(patient).getId().toUnqualifiedVersionless();
+		myPatientDao.create(patient).getId().toUnqualifiedVersionless();
 
 		// Always returning true is bad behaviour.  Our infinite loop checker should halt it
 		myDeleteInterceptor.deleteConflictFunction = t -> new DeleteConflictOutcome().setShouldRetryCount(Integer.MAX_VALUE);
@@ -152,7 +198,7 @@ public class DeleteConflictServiceR4Test extends BaseJpaR4Test {
 			myOrganizationDao.delete(organizationId);
 			fail();
 		} catch (ResourceVersionConflictException e) {
-			assertEquals("Unable to delete Organization/" + organizationId.getIdPart() + " because at least one resource has a reference to this resource. First reference found was resource Patient/" + patientId.getIdPart() + " in path Patient.managingOrganization", e.getMessage());
+			assertEquals(DeleteConflictService.MAX_RETRY_ATTEMPTS_EXCEEDED_MSG, e.getMessage());
 		}
 		assertEquals(1 + DeleteConflictService.MAX_RETRY_ATTEMPTS, myDeleteInterceptor.myCallCount);
 	}
@@ -187,9 +233,7 @@ public class DeleteConflictServiceR4Test extends BaseJpaR4Test {
 	}
 
 	private DeleteConflictOutcome deleteConflicts(DeleteConflictList theList) {
-		Iterator<DeleteConflict> iterator = theList.iterator();
+		for (DeleteConflict next : theList) {
-		while (iterator.hasNext()) {
-			DeleteConflict next = iterator.next();
 			IdDt source = next.getSourceId();
 			if ("Patient".equals(source.getResourceType())) {
 				ourLog.info("Deleting {}", source);
@@ -200,6 +244,18 @@ public class DeleteConflictServiceR4Test extends BaseJpaR4Test {
 		return new DeleteConflictOutcome().setShouldRetryCount(myInterceptorDeleteCount);
 	}
 
+	private DeleteConflictOutcome deleteConflictsFixedRetryCount(DeleteConflictList theList) {
+		for (DeleteConflict next : theList) {
+			IdDt source = next.getSourceId();
+			if ("Patient".equals(source.getResourceType())) {
+				ourLog.info("Deleting {}", source);
+				myPatientDao.delete(source, theList, null, null);
+				++myInterceptorDeleteCount;
+			}
+		}
+		return new DeleteConflictOutcome().setShouldRetryCount(DeleteConflictService.MAX_RETRY_ATTEMPTS);
+	}
+
 	private static class DeleteConflictInterceptor {
 		int myCallCount;
 		DeleteConflictList myDeleteConflictList;