From f2087d2ccc4f272c70669c234667723561d46436 Mon Sep 17 00:00:00 2001 From: TynerGjs <132295567+TynerGjs@users.noreply.github.com> Date: Mon, 14 Aug 2023 10:04:07 -0400 Subject: [PATCH] =?UTF-8?q?added=20validateExpungeEnabled()=20call=20to=20?= =?UTF-8?q?type-level=20expunge,=20moved=20two=20=E2=80=A6=20(#5197)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- ...nge_operation_enabled-is-set-to-false.yaml | 4 ++ .../fhir/jpa/dao/BaseHapiFhirResourceDao.java | 28 +++++------ .../r4/FhirResourceDaoR4QueryCountTest.java | 2 + .../fhir/jpa/provider/r4/ExpungeR4Test.java | 50 +++++++++++++++++++ 4 files changed, 70 insertions(+), 14 deletions(-) create mode 100644 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 diff --git a/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 b/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 new file mode 100644 index 00000000000..9d146d88d51 --- /dev/null +++ b/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." 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 8cfc96ae7cb..f5cf6a6b17c 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 @@ -1156,12 +1156,6 @@ public abstract class BaseHapiFhirResourceDao 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 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 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() { diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java index 573e755fc70..13511df2607 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QueryCountTest.java +++ b/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()))); diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ExpungeR4Test.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ExpungeR4Test.java index 75e289f18bd..c4cdd4da4e1 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/ExpungeR4Test.java +++ b/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 createPatientsWithForcedIds(int theNumPatients) { RequestDetails requestDetails = new SystemRequestDetails(); List createdPatients = new ArrayList<>();