From fe21dba4a69942e21aeafbd5216f072280e47bcd Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Wed, 17 Jul 2019 13:26:50 -0400 Subject: [PATCH 1/9] Added Pointcut.STORAGE_PRESTORAGE_EXPUNGE_RESOURCE. Also added a couple of tests. --- .../interceptor/api/IInterceptorService.java | 3 +- .../ca/uhn/fhir/interceptor/api/Pointcut.java | 39 ++++++++ .../executor/InterceptorService.java | 9 +- ...{ExpungeRun.java => ExpungeOperation.java} | 48 +++++++++- .../fhir/jpa/dao/expunge/ExpungeService.java | 6 +- .../dao/expunge/ResourceExpungeService.java | 4 +- .../fhir/jpa/dao/expunge/ExpungeHookTest.java | 89 +++++++++++++++++++ .../model/concurrency/FhirObjectPrinter.java | 4 +- 8 files changed, 187 insertions(+), 15 deletions(-) rename hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/{ExpungeRun.java => ExpungeOperation.java} (75%) create mode 100644 hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeHookTest.java diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/api/IInterceptorService.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/api/IInterceptorService.java index 7cae659a7d2..4797f2ef1c2 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/api/IInterceptorService.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/api/IInterceptorService.java @@ -67,8 +67,9 @@ public interface IInterceptorService extends IInterceptorBroadcaster { * Unregister an interceptor. This method has no effect if the given interceptor is not already registered. * * @param theInterceptor The interceptor to unregister + * @return Returns true if the interceptor was found and removed */ - void unregisterInterceptor(Object theInterceptor); + boolean unregisterInterceptor(Object theInterceptor); void registerAnonymousInterceptor(Pointcut thePointcut, IAnonymousInterceptor theInterceptor); 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 bd248475101..0c3e78c49bc 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 @@ -1200,6 +1200,45 @@ public enum Pointcut { "ca.uhn.fhir.rest.server.servlet.ServletRequestDetails" ), + /** + * Invoked before expunge is called on a resource. + *

+ * Hooks will be passed a reference to a counter containing the current number of records that have been deleted. + * If the hook deletes any records, the hook is expected to increment this counter by the number of records deleted. + *

+ * Hooks may accept the following parameters: + * + *

+ * Hooks should return void. + *

+ */ + + STORAGE_PRESTORAGE_EXPUNGE_RESOURCE ( + // Return type + void.class, + // Params + "java.util.concurrent.atomic.AtomicInteger", + "ca.uhn.fhir.model.primitive.IdDt", + "ca.uhn.fhir.rest.api.server.RequestDetails", + "ca.uhn.fhir.rest.server.servlet.ServletRequestDetails" + ), + /** * Note that this is a performance tracing hook. Use with caution in production * systems, since calling it may (or may not) carry a cost. diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/executor/InterceptorService.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/executor/InterceptorService.java index d6960c62006..1651249900e 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/executor/InterceptorService.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/executor/InterceptorService.java @@ -208,11 +208,12 @@ public class InterceptorService implements IInterceptorService, IInterceptorBroa } @Override - public void unregisterInterceptor(Object theInterceptor) { + public boolean unregisterInterceptor(Object theInterceptor) { synchronized (myRegistryMutex) { - myInterceptors.removeIf(t -> t == theInterceptor); - myGlobalInvokers.entries().removeIf(t -> t.getValue().getInterceptor() == theInterceptor); - myAnonymousInvokers.entries().removeIf(t -> t.getValue().getInterceptor() == theInterceptor); + boolean removed = myInterceptors.removeIf(t -> t == theInterceptor); + removed |= myGlobalInvokers.entries().removeIf(t -> t.getValue().getInterceptor() == theInterceptor); + removed |= myAnonymousInvokers.entries().removeIf(t -> t.getValue().getInterceptor() == theInterceptor); + return removed; } } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeRun.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeOperation.java similarity index 75% rename from hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeRun.java rename to hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeOperation.java index 018645b9d09..b03f381c8c8 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeRun.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeOperation.java @@ -20,9 +20,15 @@ package ca.uhn.fhir.jpa.dao.expunge; * #L% */ +import ca.uhn.fhir.interceptor.api.HookParams; +import ca.uhn.fhir.interceptor.api.IInterceptorBroadcaster; +import ca.uhn.fhir.interceptor.api.Pointcut; import ca.uhn.fhir.jpa.util.ExpungeOptions; import ca.uhn.fhir.jpa.util.ExpungeOutcome; +import ca.uhn.fhir.jpa.util.JpaInterceptorBroadcaster; +import ca.uhn.fhir.model.primitive.IdDt; import ca.uhn.fhir.rest.api.server.RequestDetails; +import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; @@ -31,20 +37,21 @@ import org.springframework.data.domain.Slice; import org.springframework.stereotype.Component; import org.springframework.transaction.PlatformTransactionManager; +import javax.persistence.Id; import java.util.concurrent.Callable; import java.util.concurrent.atomic.AtomicInteger; @Component @Scope("prototype") -public class ExpungeRun implements Callable { +public class ExpungeOperation implements Callable { private static final Logger ourLog = LoggerFactory.getLogger(ExpungeService.class); - @Autowired - private PlatformTransactionManager myPlatformTransactionManager; @Autowired private IResourceExpungeService myExpungeDaoService; @Autowired private PartitionRunner myPartitionRunner; + @Autowired + protected IInterceptorBroadcaster myInterceptorBroadcaster; private final String myResourceName; private final Long myResourceId; @@ -53,7 +60,7 @@ public class ExpungeRun implements Callable { private final RequestDetails myRequestDetails; private final AtomicInteger myRemainingCount; - public ExpungeRun(String theResourceName, Long theResourceId, Long theVersion, ExpungeOptions theExpungeOptions, RequestDetails theRequestDetails) { + public ExpungeOperation(String theResourceName, Long theResourceId, Long theVersion, ExpungeOptions theExpungeOptions, RequestDetails theRequestDetails) { myResourceName = theResourceName; myResourceId = theResourceId; myVersion = theVersion; @@ -64,6 +71,15 @@ public class ExpungeRun implements Callable { @Override public ExpungeOutcome call() { + final IdDt id; + + callHooks(); + + if (expungeLimitReached()) { + return expungeOutcome(); + } + + if (myExpungeOptions.isExpungeDeletedResources() && myVersion == null) { expungeDeletedResources(); if (expungeLimitReached()) { @@ -81,6 +97,30 @@ public class ExpungeRun implements Callable { return expungeOutcome(); } + private void callHooks() { + final AtomicInteger counter = new AtomicInteger(); + + if (myResourceId == null) { + return; + } + IdDt id; + if (myVersion == null) { + id = new IdDt(myResourceName, myResourceId); + } else { + id = new IdDt(myResourceName, myResourceId.toString(), myVersion.toString()); + } + + // Notify Interceptors about pre-action call + HookParams hooks = new HookParams() + .add(AtomicInteger.class, counter) + .add(IdDt.class, id) + .add(RequestDetails.class, myRequestDetails) + .addIfMatchesType(ServletRequestDetails.class, myRequestDetails); + JpaInterceptorBroadcaster.doCallHooks(myInterceptorBroadcaster, myRequestDetails, Pointcut.STORAGE_PRESTORAGE_EXPUNGE_RESOURCE, hooks); + + myRemainingCount.addAndGet(-1 * counter.get()); + } + private void expungeDeletedResources() { Slice resourceIds = findHistoricalVersionsOfDeletedResources(); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeService.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeService.java index 0f4ff4e7be2..238862cbd44 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeService.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeService.java @@ -44,7 +44,7 @@ public abstract class ExpungeService { private IResourceExpungeService myExpungeDaoService; @Lookup - protected abstract ExpungeRun getExpungeRun(String theResourceName, Long theResourceId, Long theVersion, ExpungeOptions theExpungeOptions, RequestDetails theRequestDetails); + protected abstract ExpungeOperation getExpungeOperation(String theResourceName, Long theResourceId, Long theVersion, ExpungeOptions theExpungeOptions, RequestDetails theRequestDetails); public ExpungeOutcome expunge(String theResourceName, Long theResourceId, Long theVersion, ExpungeOptions theExpungeOptions, RequestDetails theRequest) { ourLog.info("Expunge: ResourceName[{}] Id[{}] Version[{}] Options[{}]", theResourceName, theResourceId, theVersion, theExpungeOptions); @@ -63,8 +63,8 @@ public abstract class ExpungeService { } } - ExpungeRun expungeRun = getExpungeRun(theResourceName, theResourceId, theVersion, theExpungeOptions, theRequest); - return expungeRun.call(); + ExpungeOperation expungeOperation = getExpungeOperation(theResourceName, theResourceId, theVersion, theExpungeOptions, theRequest); + return expungeOperation.call(); } public void deleteAllSearchParams(Long theResourceId) { diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ResourceExpungeService.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ResourceExpungeService.java index 787073330c7..5b080348fff 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ResourceExpungeService.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ResourceExpungeService.java @@ -154,8 +154,8 @@ class ResourceExpungeService implements IResourceExpungeService { } } - private void expungeCurrentVersionOfResource(Long myResourceId, AtomicInteger theRemainingCount) { - ResourceTable resource = myResourceTableDao.findById(myResourceId).orElseThrow(IllegalStateException::new); + private void expungeCurrentVersionOfResource(Long theResourceId, AtomicInteger theRemainingCount) { + ResourceTable resource = myResourceTableDao.findById(theResourceId).orElseThrow(IllegalStateException::new); ResourceHistoryTable currentVersion = myResourceHistoryTableDao.findForIdAndVersion(resource.getId(), resource.getVersion()); if (currentVersion != null) { diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeHookTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeHookTest.java new file mode 100644 index 00000000000..3d5f722e291 --- /dev/null +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeHookTest.java @@ -0,0 +1,89 @@ +package ca.uhn.fhir.jpa.dao.expunge; + +import ca.uhn.fhir.interceptor.api.HookParams; +import ca.uhn.fhir.interceptor.api.IInterceptorService; +import ca.uhn.fhir.interceptor.api.Pointcut; +import ca.uhn.fhir.jpa.config.TestDstu3Config; +import ca.uhn.fhir.jpa.dao.DaoConfig; +import ca.uhn.fhir.jpa.dao.IFhirResourceDaoPatient; +import ca.uhn.fhir.jpa.model.concurrency.PointcutLatch; +import ca.uhn.fhir.jpa.util.ExpungeOptions; +import ca.uhn.fhir.model.primitive.IdDt; +import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException; +import org.hl7.fhir.dstu3.model.Patient; +import org.hl7.fhir.instance.model.api.IIdType; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.context.junit4.SpringRunner; + +import java.util.List; + +import static org.hamcrest.Matchers.containsString; +import static org.junit.Assert.*; + +@RunWith(SpringRunner.class) +@ContextConfiguration(classes = {TestDstu3Config.class}) +public class ExpungeHookTest { + @Autowired + private IFhirResourceDaoPatient myPatientDao; + @Autowired + private ExpungeService myExpungeService; + @Autowired + private IInterceptorService myInterceptorService; + @Autowired + private DaoConfig myDaoConfig; + + PointcutLatch myEverythingLatch = new PointcutLatch(Pointcut.STORAGE_PRESTORAGE_EXPUNGE_EVERYTHING); + PointcutLatch myExpungeResourceLatch = new PointcutLatch(Pointcut.STORAGE_PRESTORAGE_EXPUNGE_RESOURCE); + + @Before + public void before() { + myDaoConfig.setExpungeEnabled(true); + myInterceptorService.registerAnonymousInterceptor(Pointcut.STORAGE_PRESTORAGE_EXPUNGE_EVERYTHING, myEverythingLatch); + myInterceptorService.registerAnonymousInterceptor(Pointcut.STORAGE_PRESTORAGE_EXPUNGE_RESOURCE, myExpungeResourceLatch); + } + + @After + public void after() { + assertTrue(myInterceptorService.unregisterInterceptor(myEverythingLatch)); + assertTrue(myInterceptorService.unregisterInterceptor(myExpungeResourceLatch)); + myDaoConfig.setExpungeEnabled(new DaoConfig().isExpungeEnabled()); + } + + @Test + public void expungeEverythingHook() throws InterruptedException { + IIdType id = myPatientDao.create(new Patient()).getId(); + assertNotNull(myPatientDao.read(id)); + myEverythingLatch.setExpectedCount(1); + ExpungeOptions options = new ExpungeOptions(); + options.setExpungeEverything(true); + myExpungeService.expunge(null, null, null, options, null); + myEverythingLatch.awaitExpected(); + try { + myPatientDao.read(id); + fail(); + } catch (ResourceNotFoundException e) { + assertThat(e.getMessage(), containsString("is not known")); + } + } + + @Test + public void expungeResourceHook() throws InterruptedException { + IIdType expungeId = myPatientDao.create(new Patient()).getId(); + assertNotNull(myPatientDao.read(expungeId)); + myExpungeResourceLatch.setExpectedCount(1); + myPatientDao.delete(expungeId); + + ExpungeOptions options = new ExpungeOptions(); + options.setExpungeDeletedResources(true); + myExpungeService.expunge("Patient", expungeId.getIdPartAsLong(), expungeId.getVersionIdPartAsLong(), options, null); + HookParams hookParams = myExpungeResourceLatch.awaitExpected().get(0); + IdDt hookId = hookParams.get(IdDt.class); + assertEquals(expungeId.getValue(), hookId.getValue()); + } +} diff --git a/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/concurrency/FhirObjectPrinter.java b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/concurrency/FhirObjectPrinter.java index 468384a7fbd..2d16df27bb9 100644 --- a/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/concurrency/FhirObjectPrinter.java +++ b/hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/concurrency/FhirObjectPrinter.java @@ -30,8 +30,10 @@ public class FhirObjectPrinter implements Function { if (object instanceof IBaseResource) { IBaseResource resource = (IBaseResource) object; return resource.getClass().getSimpleName() + " { " + resource.getIdElement().getValue() + " }"; - } else { + } else if (object != null) { return object.toString(); + } else { + return "null"; } } } From d21304e771c25b05c07c902f059c5055f1b6050d Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Wed, 17 Jul 2019 13:28:17 -0400 Subject: [PATCH 2/9] cleanup --- .../java/ca/uhn/fhir/jpa/dao/expunge/ExpungeHookTest.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeHookTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeHookTest.java index 3d5f722e291..5c019aded13 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeHookTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeHookTest.java @@ -59,13 +59,19 @@ public class ExpungeHookTest { public void expungeEverythingHook() throws InterruptedException { IIdType id = myPatientDao.create(new Patient()).getId(); assertNotNull(myPatientDao.read(id)); + myEverythingLatch.setExpectedCount(1); ExpungeOptions options = new ExpungeOptions(); options.setExpungeEverything(true); myExpungeService.expunge(null, null, null, options, null); myEverythingLatch.awaitExpected(); + + assertPatientGone(id); + } + + private void assertPatientGone(IIdType theId) { try { - myPatientDao.read(id); + myPatientDao.read(theId); fail(); } catch (ResourceNotFoundException e) { assertThat(e.getMessage(), containsString("is not known")); From ffa008b2dd4c8d65031d4b39d495c36fc826a383 Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Thu, 18 Jul 2019 10:59:11 -0400 Subject: [PATCH 3/9] Fixing a build error --- .../main/java/ca/uhn/fhir/jpa/entity/TermValueSetConcept.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/TermValueSetConcept.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/TermValueSetConcept.java index 078bedb8bda..2f2293b1b41 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/TermValueSetConcept.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/TermValueSetConcept.java @@ -36,7 +36,7 @@ import static org.apache.commons.lang3.StringUtils.left; import static org.apache.commons.lang3.StringUtils.length; @Table(name = "TRM_VALUESET_CONCEPT", indexes = { - @Index(name = "IDX_VALUESET_CONCEPT_CS_CD", columnList = "SYSTEM, CODEVAL") + @Index(name = "IDX_VALUESET_CONCEPT_CS_CD", columnList = "SYSTEM_URL, CODEVAL") }) @Entity() public class TermValueSetConcept implements Serializable { From 0efe69f89197303617bd0aa6f95ccb72038ca950 Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Wed, 24 Jul 2019 10:41:37 -0400 Subject: [PATCH 4/9] Merge remote-tracking branch 'remotes/origin/master' into expunge-resource-hook # Conflicts: # hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ResourceExpungeService.java --- .../ca/uhn/fhir/interceptor/api/Pointcut.java | 39 ------------------- 1 file changed, 39 deletions(-) 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 0ebd7ae3e5f..fcac3de9935 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 @@ -1235,45 +1235,6 @@ public enum Pointcut { "ca.uhn.fhir.rest.server.servlet.ServletRequestDetails" ), - /** - * Invoked before expunge is called on a resource. - *

- * Hooks will be passed a reference to a counter containing the current number of records that have been deleted. - * If the hook deletes any records, the hook is expected to increment this counter by the number of records deleted. - *

- * Hooks may accept the following parameters: - *
    - *
  • java.util.concurrent.atomic.AtomicInteger - The counter holding the number of records deleted.
    • - *
  • ca.uhn.fhir.model.primitive.IdDt - The id of the resource to be deleted. Note that version may be null.
    • - *
  • - * ca.uhn.fhir.rest.api.server.RequestDetails - A bean containing details about the request that is about to be processed, including details such as the - * resource type and logical ID (if any) and other FHIR-specific aspects of the request which have been - * pulled out of the servlet request. Note that the bean - * properties are not all guaranteed to be populated, depending on how early during processing the - * exception occurred. - *
    • - *
  • - * ca.uhn.fhir.rest.server.servlet.ServletRequestDetails - A bean containing details about the request that is about to be processed, including details such as the - * resource type and logical ID (if any) and other FHIR-specific aspects of the request which have been - * pulled out of the servlet request. This parameter is identical to the RequestDetails parameter above but will - * only be populated when operating in a RestfulServer implementation. It is provided as a convenience. - *
    • - *
- *

- * Hooks should return void. - *

- */ - - STORAGE_PRESTORAGE_EXPUNGE_RESOURCE ( - // Return type - void.class, - // Params - "java.util.concurrent.atomic.AtomicInteger", - "ca.uhn.fhir.model.primitive.IdDt", - "ca.uhn.fhir.rest.api.server.RequestDetails", - "ca.uhn.fhir.rest.server.servlet.ServletRequestDetails" - ), - /** * Note that this is a performance tracing hook. Use with caution in production * systems, since calling it may (or may not) carry a cost. From 19c0b4fdd25fc853a2ee9b8e2ca02a9336ee113f Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Wed, 24 Jul 2019 11:30:36 -0400 Subject: [PATCH 5/9] Recovering Pointcut from a failed merge --- .../java/ca/uhn/fhir/interceptor/api/Pointcut.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) 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 fcac3de9935..d3b6843a960 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 @@ -1166,11 +1166,15 @@ public enum Pointcut { /** * Invoked before a resource is about to be expunged via the $expunge operation. *

+ * Hooks will be passed a reference to a counter containing the current number of records that have been deleted. + * If the hook deletes any records, the hook is expected to increment this counter by the number of records deleted. + *

+ *

* Hooks may accept the following parameters: *

*
    *
  • org.hl7.fhir.instance.model.api.IIdType - The ID of the resource that is about to be deleted
    • - *
  • org.hl7.fhir.instance.model.api.IBaseResource - The resource that is about to be deleted
    • + *
  • java.util.concurrent.atomic.AtomicInteger - The counter holding the number of records deleted.
    • *
  • * ca.uhn.fhir.rest.api.server.RequestDetails - A bean containing details about the request that is about to be processed, including details such as the * resource type and logical ID (if any) and other FHIR-specific aspects of the request which have been @@ -1189,12 +1193,12 @@ public enum Pointcut { * Hooks should return void. *

    */ - STORAGE_PRESTORAGE_EXPUNGE_RESOURCE( + STORAGE_PRESTORAGE_EXPUNGE_RESOURCE ( // Return type void.class, // Params + "java.util.concurrent.atomic.AtomicInteger", "org.hl7.fhir.instance.model.api.IIdType", - "org.hl7.fhir.instance.model.api.IBaseResource", "ca.uhn.fhir.rest.api.server.RequestDetails", "ca.uhn.fhir.rest.server.servlet.ServletRequestDetails" ), From 64e83fa3dfcfcf3393812a76f9d489b9984ce386 Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Wed, 24 Jul 2019 13:00:49 -0400 Subject: [PATCH 6/9] Revert "Recovering Pointcut from a failed merge" This reverts commit 19c0b4fd --- .../java/ca/uhn/fhir/interceptor/api/Pointcut.java | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) 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 d3b6843a960..fcac3de9935 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 @@ -1166,15 +1166,11 @@ public enum Pointcut { /** * Invoked before a resource is about to be expunged via the $expunge operation. *

    - * Hooks will be passed a reference to a counter containing the current number of records that have been deleted. - * If the hook deletes any records, the hook is expected to increment this counter by the number of records deleted. - *

    - *

    * Hooks may accept the following parameters: *

    *
      *
    • org.hl7.fhir.instance.model.api.IIdType - The ID of the resource that is about to be deleted
      • - *
    • java.util.concurrent.atomic.AtomicInteger - The counter holding the number of records deleted.
      • + *
    • org.hl7.fhir.instance.model.api.IBaseResource - The resource that is about to be deleted
      • *
    • * ca.uhn.fhir.rest.api.server.RequestDetails - A bean containing details about the request that is about to be processed, including details such as the * resource type and logical ID (if any) and other FHIR-specific aspects of the request which have been @@ -1193,12 +1189,12 @@ public enum Pointcut { * Hooks should return void. *

      */ - STORAGE_PRESTORAGE_EXPUNGE_RESOURCE ( + STORAGE_PRESTORAGE_EXPUNGE_RESOURCE( // Return type void.class, // Params - "java.util.concurrent.atomic.AtomicInteger", "org.hl7.fhir.instance.model.api.IIdType", + "org.hl7.fhir.instance.model.api.IBaseResource", "ca.uhn.fhir.rest.api.server.RequestDetails", "ca.uhn.fhir.rest.server.servlet.ServletRequestDetails" ), From e05387da022634dda10c9f8d051c7fb5372dedc0 Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Wed, 24 Jul 2019 13:46:20 -0400 Subject: [PATCH 7/9] Recover from failed merge again. This time with feeling! --- .../ca/uhn/fhir/interceptor/api/Pointcut.java | 6 +++ .../binstore/BinaryStorageInterceptor.java | 4 +- .../jpa/dao/expunge/ExpungeOperation.java | 26 ------------- .../dao/expunge/ResourceExpungeService.java | 37 +++++++++++-------- .../fhir/jpa/dao/expunge/ExpungeHookTest.java | 8 ++-- 5 files changed, 36 insertions(+), 45 deletions(-) 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 fcac3de9935..e31009e9a26 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 @@ -1166,9 +1166,14 @@ public enum Pointcut { /** * Invoked before a resource is about to be expunged via the $expunge operation. *

      + * Hooks will be passed a reference to a counter containing the current number of records that have been deleted. + * If the hook deletes any records, the hook is expected to increment this counter by the number of records deleted. + *

      + *

      * Hooks may accept the following parameters: *

      *
        + *
      • java.util.concurrent.atomic.AtomicInteger - The counter holding the number of records deleted.
        • *
      • org.hl7.fhir.instance.model.api.IIdType - The ID of the resource that is about to be deleted
        • *
      • org.hl7.fhir.instance.model.api.IBaseResource - The resource that is about to be deleted
        • *
      • @@ -1193,6 +1198,7 @@ public enum Pointcut { // Return type void.class, // Params + "java.util.concurrent.atomic.AtomicInteger", "org.hl7.fhir.instance.model.api.IIdType", "org.hl7.fhir.instance.model.api.IBaseResource", "ca.uhn.fhir.rest.api.server.RequestDetails", diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/binstore/BinaryStorageInterceptor.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/binstore/BinaryStorageInterceptor.java index 39367114555..9fa3ff6b5c2 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/binstore/BinaryStorageInterceptor.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/binstore/BinaryStorageInterceptor.java @@ -32,6 +32,7 @@ import org.hl7.fhir.instance.model.api.IPrimitiveType; import org.springframework.beans.factory.annotation.Autowired; import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Collectors; @Interceptor @@ -43,7 +44,7 @@ public class BinaryStorageInterceptor { private FhirContext myCtx; @Hook(Pointcut.STORAGE_PRESTORAGE_EXPUNGE_RESOURCE) - public void expungeResource(IBaseResource theResource) { + public void expungeResource(AtomicInteger theCounter, IBaseResource theResource) { Class binaryType = myCtx.getElementDefinition("base64Binary").getImplementingClass(); List binaryElements = myCtx.newTerser().getAllPopulatedChildElementsOfType(theResource, binaryType); @@ -57,6 +58,7 @@ public class BinaryStorageInterceptor { for (String next : attachmentIds) { myBinaryStorageSvc.expungeBlob(theResource.getIdElement(), next); + theCounter.incrementAndGet(); } } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeOperation.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeOperation.java index 11de6dbd855..d4db1f0e7d0 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeOperation.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeOperation.java @@ -73,8 +73,6 @@ public class ExpungeOperation implements Callable { public ExpungeOutcome call() { final IdDt id; - callHooks(); - if (expungeLimitReached()) { return expungeOutcome(); } @@ -97,30 +95,6 @@ public class ExpungeOperation implements Callable { return expungeOutcome(); } - private void callHooks() { - final AtomicInteger counter = new AtomicInteger(); - - if (myResourceId == null) { - return; - } - IdDt id; - if (myVersion == null) { - id = new IdDt(myResourceName, myResourceId); - } else { - id = new IdDt(myResourceName, myResourceId.toString(), myVersion.toString()); - } - - // Notify Interceptors about pre-action call - HookParams hooks = new HookParams() - .add(AtomicInteger.class, counter) - .add(IdDt.class, id) - .add(RequestDetails.class, myRequestDetails) - .addIfMatchesType(ServletRequestDetails.class, myRequestDetails); - JpaInterceptorBroadcaster.doCallHooks(myInterceptorBroadcaster, myRequestDetails, Pointcut.STORAGE_PRESTORAGE_EXPUNGE_RESOURCE, hooks); - - myRemainingCount.addAndGet(-1 * counter.get()); - } - private void expungeDeletedResources() { Slice resourceIds = findHistoricalVersionsOfDeletedResources(); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ResourceExpungeService.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ResourceExpungeService.java index 25dbca142b0..051e171704b 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ResourceExpungeService.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ResourceExpungeService.java @@ -139,27 +139,36 @@ class ResourceExpungeService implements IResourceExpungeService { } } - private void expungeHistoricalVersion(RequestDetails theRequestDetails, Long theNextVersionId) { + private void expungeHistoricalVersion(RequestDetails theRequestDetails, Long theNextVersionId, AtomicInteger theRemainingCount) { ResourceHistoryTable version = myResourceHistoryTableDao.findById(theNextVersionId).orElseThrow(IllegalArgumentException::new); IdDt id = version.getIdDt(); ourLog.info("Deleting resource version {}", id.getValue()); - // Interceptor call: STORAGE_PRESTORAGE_EXPUNGE_RESOURCE + callHooks(theRequestDetails, theRemainingCount, version, id); + + myResourceHistoryTagDao.deleteAll(version.getTags()); + myResourceHistoryTableDao.delete(version); + + theRemainingCount.decrementAndGet(); + } + + private void callHooks(RequestDetails theRequestDetails, AtomicInteger theRemainingCount, ResourceHistoryTable theVersion, IdDt theId) { + final AtomicInteger counter = new AtomicInteger(); if (JpaInterceptorBroadcaster.hasHooks(Pointcut.STORAGE_PRESTORAGE_EXPUNGE_RESOURCE, myInterceptorBroadcaster, theRequestDetails)) { - IFhirResourceDao resourceDao = myDaoRegistry.getResourceDao(id.getResourceType()); - IBaseResource resource = resourceDao.toResource(version, false); + IFhirResourceDao resourceDao = myDaoRegistry.getResourceDao(theId.getResourceType()); + IBaseResource resource = resourceDao.toResource(theVersion, false); HookParams params = new HookParams() - .add(IIdType.class, id) + .add(AtomicInteger.class, counter) + .add(IIdType.class, theId) .add(IBaseResource.class, resource) .add(RequestDetails.class, theRequestDetails) .addIfMatchesType(ServletRequestDetails.class, theRequestDetails); JpaInterceptorBroadcaster.doCallHooks(myInterceptorBroadcaster, theRequestDetails, Pointcut.STORAGE_PRESTORAGE_EXPUNGE_RESOURCE, params); } - - myResourceHistoryTagDao.deleteAll(version.getTags()); - myResourceHistoryTableDao.delete(version); + theRemainingCount.addAndGet(-1 * counter.get()); } + @Override @Transactional public void expungeHistoricalVersionsOfIds(RequestDetails theRequestDetails, List theResourceIds, AtomicInteger theRemainingCount) { @@ -175,8 +184,8 @@ class ResourceExpungeService implements IResourceExpungeService { @Transactional public void expungeHistoricalVersions(RequestDetails theRequestDetails, List theHistoricalIds, AtomicInteger theRemainingCount) { for (Long next : theHistoricalIds) { - expungeHistoricalVersion(theRequestDetails, next); - if (theRemainingCount.decrementAndGet() <= 0) { + expungeHistoricalVersion(theRequestDetails, next, theRemainingCount); + if (theRemainingCount.get() <= 0) { return; } } @@ -187,7 +196,7 @@ class ResourceExpungeService implements IResourceExpungeService { ResourceHistoryTable currentVersion = myResourceHistoryTableDao.findForIdAndVersion(resource.getId(), resource.getVersion()); if (currentVersion != null) { - expungeHistoricalVersion(theRequestDetails, currentVersion.getId()); + expungeHistoricalVersion(theRequestDetails, currentVersion.getId(), theRemainingCount); } ourLog.info("Expunging current version of resource {}", resource.getIdDt().getValue()); @@ -203,8 +212,6 @@ class ResourceExpungeService implements IResourceExpungeService { } myResourceTableDao.delete(resource); - - theRemainingCount.decrementAndGet(); } @Override @@ -230,8 +237,8 @@ class ResourceExpungeService implements IResourceExpungeService { Slice versionIds = myResourceHistoryTableDao.findForResourceId(page, resource.getId(), resource.getVersion()); ourLog.debug("Found {} versions of resource {} to expunge", versionIds.getNumberOfElements(), resource.getIdDt().getValue()); for (Long nextVersionId : versionIds) { - expungeHistoricalVersion(theRequestDetails, nextVersionId); - if (theRemainingCount.decrementAndGet() <= 0) { + expungeHistoricalVersion(theRequestDetails, nextVersionId, theRemainingCount); + if (theRemainingCount.get() <= 0) { return; } } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeHookTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeHookTest.java index 5c019aded13..282cc06c576 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeHookTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeHookTest.java @@ -82,14 +82,16 @@ public class ExpungeHookTest { public void expungeResourceHook() throws InterruptedException { IIdType expungeId = myPatientDao.create(new Patient()).getId(); assertNotNull(myPatientDao.read(expungeId)); - myExpungeResourceLatch.setExpectedCount(1); myPatientDao.delete(expungeId); ExpungeOptions options = new ExpungeOptions(); options.setExpungeDeletedResources(true); - myExpungeService.expunge("Patient", expungeId.getIdPartAsLong(), expungeId.getVersionIdPartAsLong(), options, null); + + myExpungeResourceLatch.setExpectedCount(2); + myExpungeService.expunge("Patient", expungeId.getIdPartAsLong(), null, options, null); HookParams hookParams = myExpungeResourceLatch.awaitExpected().get(0); - IdDt hookId = hookParams.get(IdDt.class); + + IIdType hookId = hookParams.get(IIdType.class); assertEquals(expungeId.getValue(), hookId.getValue()); } } From a20b738161b349dd8ea280fe039d4a4d2cdd017d Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Wed, 24 Jul 2019 22:00:26 -0400 Subject: [PATCH 8/9] Pre PR cleanup --- .../uhn/fhir/jpa/dao/expunge/ExpungeOperation.java | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeOperation.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeOperation.java index d4db1f0e7d0..f092b6d73e4 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeOperation.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeOperation.java @@ -20,24 +20,17 @@ package ca.uhn.fhir.jpa.dao.expunge; * #L% */ -import ca.uhn.fhir.interceptor.api.HookParams; import ca.uhn.fhir.interceptor.api.IInterceptorBroadcaster; -import ca.uhn.fhir.interceptor.api.Pointcut; import ca.uhn.fhir.jpa.util.ExpungeOptions; import ca.uhn.fhir.jpa.util.ExpungeOutcome; -import ca.uhn.fhir.jpa.util.JpaInterceptorBroadcaster; -import ca.uhn.fhir.model.primitive.IdDt; import ca.uhn.fhir.rest.api.server.RequestDetails; -import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Scope; import org.springframework.data.domain.Slice; import org.springframework.stereotype.Component; -import org.springframework.transaction.PlatformTransactionManager; -import javax.persistence.Id; import java.util.concurrent.Callable; import java.util.concurrent.atomic.AtomicInteger; @@ -71,13 +64,6 @@ public class ExpungeOperation implements Callable { @Override public ExpungeOutcome call() { - final IdDt id; - - if (expungeLimitReached()) { - return expungeOutcome(); - } - - if (myExpungeOptions.isExpungeDeletedResources() && myVersion == null) { expungeDeletedResources(); if (expungeLimitReached()) { From a7817b07ca8791ded7cd9a94d9ce4d380dafb22d Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Wed, 24 Jul 2019 22:00:34 -0400 Subject: [PATCH 9/9] Pre PR cleanup --- .../java/ca/uhn/fhir/jpa/dao/expunge/ExpungeHookTest.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeHookTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeHookTest.java index 282cc06c576..a03d5b5ace6 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeHookTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/expunge/ExpungeHookTest.java @@ -8,7 +8,6 @@ import ca.uhn.fhir.jpa.dao.DaoConfig; import ca.uhn.fhir.jpa.dao.IFhirResourceDaoPatient; import ca.uhn.fhir.jpa.model.concurrency.PointcutLatch; import ca.uhn.fhir.jpa.util.ExpungeOptions; -import ca.uhn.fhir.model.primitive.IdDt; import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException; import org.hl7.fhir.dstu3.model.Patient; import org.hl7.fhir.instance.model.api.IIdType; @@ -17,12 +16,9 @@ import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.context.junit4.SpringRunner; -import java.util.List; - import static org.hamcrest.Matchers.containsString; import static org.junit.Assert.*;