Handle cascading deletes correctly with circular references (#1435)

* Handle cascading deletes correctly with circular references

* A bit of cleanup

* Address review comments

* FIx some javadocs

* Fix an incorrect message

hapi-fhir-base/src/main/resources/ca/uhn/fhir/i18n/hapi-messages.properties

@@ -113,7 +113,7 @@ ca.uhn.fhir.jpa.dao.r4.FhirResourceDaoConceptMapR4.noMatchesFound=No matches fou
 ca.uhn.fhir.jpa.dao.r4.FhirResourceDaoSearchParameterR4.invalidSearchParamExpression=The expression "{0}" can not be evaluated and may be invalid: {1}
 
 ca.uhn.fhir.jpa.interceptor.CascadingDeleteInterceptor.successMsg=Cascaded delete to {0} resources: {1}
-ca.uhn.fhir.jpa.interceptor.CascadingDeleteInterceptor.noParam=Note that cascading deletes are not active for this request. You can enable cascading deletes by using the "_cascade=true" URL parameter.
+ca.uhn.fhir.jpa.interceptor.CascadingDeleteInterceptor.noParam=Note that cascading deletes are not active for this request. You can enable cascading deletes by using the "_cascade=delete" URL parameter.
 
 ca.uhn.fhir.jpa.provider.BaseJpaProvider.cantCombintAtAndSince=Unable to combine _at and _since parameters for history operation
 ca.uhn.fhir.jpa.binstore.BinaryAccessProvider.noAttachmentDataPresent=The resource with ID {0} has no data at path: {1}

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

@@ -220,6 +220,7 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 		StopWatch w = new StopWatch();
 
 		T resourceToDelete = toResource(myResourceType, entity, null, false);
+		theDeleteConflicts.setResourceIdMarkedForDeletion(theId);
 
 		// Notify IServerOperationInterceptors about pre-action call
 		HookParams hook = new HookParams()
@@ -268,6 +269,7 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 	@Override
 	public DaoMethodOutcome delete(IIdType theId, RequestDetails theRequestDetails) {
 		DeleteConflictList deleteConflicts = new DeleteConflictList();
+		deleteConflicts.setResourceIdMarkedForDeletion(theId);
 		StopWatch w = new StopWatch();
 
 		DaoMethodOutcome retVal = delete(theId, deleteConflicts, theRequestDetails);

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

@@ -21,14 +21,43 @@ package ca.uhn.fhir.jpa.delete;
  */
 
 import ca.uhn.fhir.jpa.util.DeleteConflict;
+import org.apache.commons.lang3.Validate;
+import org.hl7.fhir.instance.model.api.IIdType;
 
-import java.util.ArrayList;
-import java.util.Iterator;
-import java.util.List;
+import java.util.*;
 import java.util.function.Predicate;
 
 public class DeleteConflictList implements Iterable<DeleteConflict> {
 	private final List<DeleteConflict> myList = new ArrayList<>();
+	private final Set<String> myResourceIdsMarkedForDeletion;
+
+	/**
+	 * Constructor
+	 */
+	public DeleteConflictList() {
+		myResourceIdsMarkedForDeletion = new HashSet<>();
+	}
+
+	/**
+	 * Constructor that shares (i.e. uses the same list, as opposed to cloning it)
+	 * of {@link #isResourceIdMarkedForDeletion(IIdType) resources marked for deletion}
+	 */
+	public DeleteConflictList(DeleteConflictList theParentList) {
+		myResourceIdsMarkedForDeletion = theParentList.myResourceIdsMarkedForDeletion;
+	}
+
+
+	public boolean isResourceIdMarkedForDeletion(IIdType theIdType) {
+		Validate.notNull(theIdType);
+		Validate.notBlank(theIdType.toUnqualifiedVersionless().getValue());
+		return myResourceIdsMarkedForDeletion.contains(theIdType.toUnqualifiedVersionless().getValue());
+	}
+
+	public void setResourceIdMarkedForDeletion(IIdType theIdType) {
+		Validate.notNull(theIdType);
+		Validate.notBlank(theIdType.toUnqualifiedVersionless().getValue());
+		myResourceIdsMarkedForDeletion.add(theIdType.toUnqualifiedVersionless().getValue());
+	}
 
 	public void add(DeleteConflict theDeleteConflict) {
 		myList.add(theDeleteConflict);

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

@@ -42,11 +42,8 @@ import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Service;
 
-import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
-import java.util.Set;
-import java.util.stream.Collectors;
 
 @Service
 public class DeleteConflictService {
@@ -67,7 +64,11 @@ public class DeleteConflictService {
 	protected IInterceptorBroadcaster myInterceptorBroadcaster;
 
 	public int validateOkToDelete(DeleteConflictList theDeleteConflicts, ResourceTable theEntity, boolean theForValidate, RequestDetails theRequest) {
-		DeleteConflictList newConflicts = new DeleteConflictList();
+
+		// We want the list of resources that are marked to be the same list even as we
+		// drill into conflict resolution stacks.. this allows us to not get caught by
+		// circular references
+		DeleteConflictList newConflicts = new DeleteConflictList(theDeleteConflicts);
 
 		// In most cases, there will be no hooks, and so we only need to check if there is at least FIRST_QUERY_RESULT_COUNT conflict and populate that.
 		// Only in the case where there is a hook do we need to go back and collect larger batches of conflicts for processing.
@@ -104,6 +105,10 @@ public class DeleteConflictService {
 
 		addConflictsToList(theDeleteConflicts, theEntity, theResultList);
 
+		if (theDeleteConflicts.isEmpty()) {
+			return new DeleteConflictOutcome();
+		}
+
 		// Notify Interceptors about pre-action call
 		HookParams hooks = new HookParams()
 			.add(DeleteConflictList.class, theDeleteConflicts)
@@ -117,6 +122,12 @@ public class DeleteConflictService {
 			IdDt targetId = theEntity.getIdDt();
 			IdDt sourceId = link.getSourceResource().getIdDt();
 			String sourcePath = link.getSourcePath();
+			if (theDeleteConflicts.isResourceIdMarkedForDeletion(sourceId)) {
+				if (theDeleteConflicts.isResourceIdMarkedForDeletion(targetId)) {
+					continue;
+				}
+			}
+
 			theDeleteConflicts.add(new DeleteConflict(sourceId, sourcePath, targetId));
 		}
 	}

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/interceptor/CascadingDeleteInterceptor.java

@@ -58,8 +58,8 @@ import static org.apache.commons.lang3.StringUtils.isNotBlank;
  * </p>
  * <p>
  * When using this interceptor, client requests must include the parameter
- * <code>_cascade=true</code> on the DELETE URL in order to activate
- * cascading delete, or include the request header <code>X-Cascade-Delete: true</code>
+ * <code>_cascade=delete</code> on the DELETE URL in order to activate
+ * cascading delete, or include the request header <code>X-Cascade-Delete: delete</code>
  * </p>
  */
 @Interceptor
@@ -113,7 +113,7 @@ public class CascadingDeleteInterceptor {
 
 				// Actually perform the delete
 				ourLog.info("Have delete conflict {} - Cascading delete", next);
-				dao.delete(nextSource, theRequest);
+				dao.delete(nextSource, theConflictList, theRequest);
 
 				cascadedDeletes.add(nextSourceId);
 

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/config/TestR4Config.java

@@ -23,6 +23,7 @@ import org.springframework.transaction.annotation.EnableTransactionManagement;
 import javax.sql.DataSource;
 import java.sql.Connection;
 import java.util.Properties;
+import java.util.concurrent.TimeUnit;
 
 import static org.junit.Assert.fail;
 
@@ -110,8 +111,8 @@ public class TestR4Config extends BaseJavaConfigR4 {
 		SLF4JLogLevel level = SLF4JLogLevel.INFO;
 		DataSource dataSource = ProxyDataSourceBuilder
 			.create(retVal)
-			.logQueryBySlf4j(level, "SQL")
-//			.logSlowQueryBySlf4j(10, TimeUnit.SECONDS)
+//			.logQueryBySlf4j(level, "SQL")
+			.logSlowQueryBySlf4j(10, TimeUnit.SECONDS)
 //			.countQuery(new ThreadQueryCountHolder())
 			.beforeQuery(new BlockLargeNumbersOfParamsListener())
 			.afterQuery(captureQueriesListener())

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

@@ -136,6 +136,48 @@ public class CascadingDeleteInterceptorR4Test extends BaseResourceProviderR4Test
 		}
 	}
 
+	@Test
+	public void testDeleteCascadingWithCircularReference() throws IOException {
+
+		Organization o0 = new Organization();
+		o0.setName("O0");
+		IIdType o0id = myOrganizationDao.create(o0).getId().toUnqualifiedVersionless();
+
+		Organization o1 = new Organization();
+		o1.setName("O1");
+		o1.getPartOf().setReference(o0id.getValue());
+		IIdType o1id = myOrganizationDao.create(o1).getId().toUnqualifiedVersionless();
+
+		o0.getPartOf().setReference(o1id.getValue());
+		myOrganizationDao.update(o0);
+
+		ourRestServer.getInterceptorService().registerInterceptor(myDeleteInterceptor);
+
+		HttpDelete delete = new HttpDelete(ourServerBase + "/Organization/" + o0id.getIdPart() + "?" + Constants.PARAMETER_CASCADE_DELETE + "=" + Constants.CASCADE_DELETE + "&_pretty=true");
+		delete.addHeader(Constants.HEADER_ACCEPT, Constants.CT_FHIR_JSON_NEW);
+		try (CloseableHttpResponse response = ourHttpClient.execute(delete)) {
+			assertEquals(200, response.getStatusLine().getStatusCode());
+			String deleteResponse = IOUtils.toString(response.getEntity().getContent(), Charsets.UTF_8);
+			ourLog.info("Response: {}", deleteResponse);
+			assertThat(deleteResponse, containsString("Cascaded delete to "));
+		}
+
+		try {
+			ourLog.info("Reading {}", o0id);
+			ourClient.read().resource(Organization.class).withId(o0id).execute();
+			fail();
+		} catch (ResourceGoneException e) {
+			// good
+		}
+		try {
+			ourLog.info("Reading {}", o1id);
+			ourClient.read().resource(Organization.class).withId(o1id).execute();
+			fail();
+		} catch (ResourceGoneException e) {
+			// good
+		}
+	}
+
 	@Test
 	public void testDeleteCascadingByHeader() throws IOException {
 		createResources();

src/changes/changes.xml

@@ -48,6 +48,14 @@
 				When using the VersionedApiConverterInterceptor, GraphQL responses failed with an HTTP
 				500 error.
 			</action>
+			<action type="fix">
+				Cascading deletes now correctly handle circular references. Previously this failed with
+				an HTTP 500 error.
+			</action>
+			<action type="fix">
+				The informational message returned in an OperationOutcome when a delete failed due to cascades not being enabled
+				contained an incorrect example. This has been corrected.
+			</action>
 		</release>
 		<release version="4.0.0" date="2019-08-14" description="Igloo">
 			<action type="add">