added validateExpungeEnabled() call to type-level expunge, moved two … (#5197)

* added validateExpungeEnabled() call to type-level expunge, moved two overloading method so that they are next to each other

* added test for expunge
enabled expunge for testExpungeAllVersionsWithTagsDeletesRow in FhirResourceDaoR4QueryCountTest

* added changelog

* optimized tests and changelogs

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5196-type-level-expunge-operation-is-accepted-even-when-expunge_operation_enabled-is-set-to-false.yaml

@@ -0,0 +1,4 @@
+---
+type: fix
+issue: 5196
+title: "Previously, type-level expunge was allowed even if expunge operation was turned off. This is now fixed."

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

@@ -1156,12 +1156,6 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 		myInterceptorBroadcaster.callHooks(Pointcut.STORAGE_PRECOMMIT_RESOURCE_UPDATED, preCommitParams);
 	}
 
-	private void validateExpungeEnabled() {
-		if (!getStorageSettings().isExpungeEnabled()) {
-			throw new MethodNotAllowedException(Msg.code(968) + "$expunge is not enabled on this server");
-		}
-	}
-
 	@Override
 	@Transactional(propagation = Propagation.NEVER)
 	public ExpungeOutcome expunge(IIdType theId, ExpungeOptions theExpungeOptions, RequestDetails theRequest) {
@@ -1169,6 +1163,20 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 		return forceExpungeInExistingTransaction(theId, theExpungeOptions, theRequest);
 	}
 
+	@Override
+	@Transactional(propagation = Propagation.NEVER)
+	public ExpungeOutcome expunge(ExpungeOptions theExpungeOptions, RequestDetails theRequestDetails) {
+		ourLog.info("Beginning TYPE[{}] expunge operation", getResourceName());
+		validateExpungeEnabled();
+		return myExpungeService.expunge(getResourceName(), null, theExpungeOptions, theRequestDetails);
+	}
+
+	private void validateExpungeEnabled() {
+		if (!getStorageSettings().isExpungeEnabled()) {
+			throw new MethodNotAllowedException(Msg.code(968) + "$expunge is not enabled on this server");
+		}
+	}
+
 	@Override
 	public ExpungeOutcome forceExpungeInExistingTransaction(
 			IIdType theId, ExpungeOptions theExpungeOptions, RequestDetails theRequest) {
@@ -1202,14 +1210,6 @@ public abstract class BaseHapiFhirResourceDao<T extends IBaseResource> extends B
 				getResourceName(), JpaPid.fromId(entity.getResourceId()), theExpungeOptions, theRequest);
 	}
 
-	@Override
-	@Transactional(propagation = Propagation.NEVER)
-	public ExpungeOutcome expunge(ExpungeOptions theExpungeOptions, RequestDetails theRequestDetails) {
-		ourLog.info("Beginning TYPE[{}] expunge operation", getResourceName());
-
-		return myExpungeService.expunge(getResourceName(), null, theExpungeOptions, theRequestDetails);
-	}
-
 	@Override
 	@Nonnull
 	public String getResourceName() {

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java

@@ -162,6 +162,7 @@ public class FhirResourceDaoR4QueryCountTest extends BaseResourceProviderR4Test
 		myStorageSettings.setResourceMetaCountHardLimit(new JpaStorageSettings().getResourceMetaCountHardLimit());
 		myStorageSettings.setRespectVersionsForSearchIncludes(new JpaStorageSettings().isRespectVersionsForSearchIncludes());
 		myStorageSettings.setTagStorageMode(new JpaStorageSettings().getTagStorageMode());
+		myStorageSettings.setExpungeEnabled(false);
 
 		myFhirContext.getParserOptions().setStripVersionsFromReferences(true);
 		TermReadSvcImpl.setForceDisableHibernateSearchForUnitTest(false);
@@ -200,6 +201,7 @@ public class FhirResourceDaoR4QueryCountTest extends BaseResourceProviderR4Test
 			myPatientDao.delete(new IdType("Patient/TEST" + i));
 		}
 
+		myStorageSettings.setExpungeEnabled(true);
 
 		runInTransaction(() -> assertThat(myResourceTableDao.findAll(), not(empty())));
 		runInTransaction(() -> assertThat(myResourceHistoryTableDao.findAll(), not(empty())));

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

@@ -55,6 +55,8 @@ import org.hl7.fhir.r4.model.StringType;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -67,6 +69,7 @@ import java.util.List;
 import static ca.uhn.fhir.batch2.jobs.termcodesystem.TermCodeSystemJobConfig.TERM_CODE_SYSTEM_DELETE_JOB_NAME;
 import static org.awaitility.Awaitility.await;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.startsWith;
@@ -74,6 +77,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
@@ -921,6 +925,52 @@ public class ExpungeR4Test extends BaseResourceProviderR4Test {
 		}
 	}
 
+	@ParameterizedTest
+	@ValueSource(strings = {"instance", "type", "system"})
+	public void testExpungeNotAllowedWhenNotEnabled(String level) {
+		// setup
+		myStorageSettings.setExpungeEnabled(false);
+
+		Patient p = new Patient();
+		p.setActive(true);
+		p.addName().setFamily("FOO");
+		IIdType patientId = myPatientDao.create(p).getId();
+
+		myPatientDao.delete(patientId);
+
+		runInTransaction(() -> assertThat(myResourceTableDao.findAll(), not(empty())));
+		runInTransaction(() -> assertThat(myResourceHistoryTableDao.findAll(), not(empty())));
+
+		// execute & verify
+		MethodNotAllowedException exception = null;
+		switch (level) {
+			case "instance":
+				exception = assertThrows(MethodNotAllowedException.class, () -> {
+					myPatientDao.expunge(patientId, new ExpungeOptions()
+						.setExpungeDeletedResources(true)
+						.setExpungeOldVersions(true), null);
+				});
+				break;
+
+			case "type":
+				exception = assertThrows(MethodNotAllowedException.class, () -> {
+					myPatientDao.expunge(new ExpungeOptions()
+						.setExpungeDeletedResources(true)
+						.setExpungeOldVersions(true), null);
+				});
+				break;
+
+			case "system":
+				exception = assertThrows(MethodNotAllowedException.class, () -> {
+					mySystemDao.expunge(new ExpungeOptions()
+						.setExpungeEverything(true), null);
+				});
+		}
+
+		assertStillThere(patientId);
+		assertThat(exception.getMessage(), containsString("$expunge is not enabled on this server"));
+	}
+
 	private List<Patient> createPatientsWithForcedIds(int theNumPatients) {
 		RequestDetails requestDetails = new SystemRequestDetails();
 		List<Patient> createdPatients = new ArrayList<>();