From 28ea97ee9a4421eb962825807c1ba4e583eb491a Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Wed, 3 Apr 2019 11:42:21 -0400 Subject: [PATCH 1/5] reproduced failure reported by FMCNA --- .../ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java | 4 +++ .../ca/uhn/fhir/jpa/util/ExpungeOptions.java | 3 +- .../ResourceProviderExpungeDstu3Test.java | 36 +++++++++++++++++++ 3 files changed, 42 insertions(+), 1 deletion(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java index 4af1b5ee0f8..3e68d4761a4 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java @@ -224,6 +224,10 @@ public abstract class BaseHapiFhirDao implements IDao, throw new MethodNotAllowedException("$expunge is not enabled on this server"); } + if (theExpungeOptions.getLimit() < 1) { + throw new InvalidRequestException("Expunge limit may not be less than 1. Received expunge limit "+theExpungeOptions.getLimit() + "."); + } + AtomicInteger remainingCount = new AtomicInteger(theExpungeOptions.getLimit()); if (theResourceName == null && theResourceId == null && theVersion == null) { diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/ExpungeOptions.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/ExpungeOptions.java index efc3142acce..e6a07f95cd8 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/ExpungeOptions.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/ExpungeOptions.java @@ -49,8 +49,9 @@ public class ExpungeOptions { /** * The maximum number of resource versions to expunge */ - public void setLimit(int theLimit) { + public ExpungeOptions setLimit(int theLimit) { myLimit = theLimit; + return this; } public boolean isExpungeEverything() { diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderExpungeDstu3Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderExpungeDstu3Test.java index 11ac4744bc6..68c89f73335 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderExpungeDstu3Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderExpungeDstu3Test.java @@ -4,6 +4,7 @@ import ca.uhn.fhir.jpa.dao.DaoConfig; import ca.uhn.fhir.jpa.dao.IFhirResourceDao; import ca.uhn.fhir.jpa.util.ExpungeOptions; import ca.uhn.fhir.jpa.util.JpaConstants; +import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import ca.uhn.fhir.rest.server.exceptions.PreconditionFailedException; import ca.uhn.fhir.rest.server.exceptions.ResourceGoneException; import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException; @@ -290,6 +291,41 @@ public class ResourceProviderExpungeDstu3Test extends BaseResourceProviderDstu3T assertGone(myDeletedObservationId); } + @Test + public void testExpungeLimitZero() { + try { + myPatientDao.expunge(new ExpungeOptions() + .setExpungeDeletedResources(true) + .setExpungeOldVersions(true) + .setLimit(0)); + fail(); + } catch (InvalidRequestException e) { + assertEquals("Expunge limit may not be less than 1. Received expunge limit 0.", e.getMessage()); + } + } + + @Test + public void testExpungeInstanceOldVersionsAndDeletedBoundaryLimit() { + myPatientDao.delete(myTwoVersionPatientId); + + myPatientDao.expunge(myTwoVersionPatientId.toUnqualifiedVersionless(), new ExpungeOptions() + .setExpungeDeletedResources(true) + .setExpungeOldVersions(true) + .setLimit(2)); + + // Patients + assertStillThere(myOneVersionPatientId); + assertExpunged(myTwoVersionPatientId.withVersion("1")); + assertExpunged(myTwoVersionPatientId.withVersion("2")); + assertGone(myDeletedPatientId); + + // No observations deleted + assertStillThere(myOneVersionObservationId); + assertStillThere(myTwoVersionObservationId.withVersion("1")); + assertStillThere(myTwoVersionObservationId.withVersion("2")); + assertGone(myDeletedObservationId); + } + @Test public void testParameters() { Parameters p = new Parameters(); From de4cc89568ce873a2056ebbee902f93b9826eda4 Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Wed, 3 Apr 2019 12:20:49 -0400 Subject: [PATCH 2/5] bug fixed --- .../java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java index 3e68d4761a4..250866bd3ca 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java @@ -279,12 +279,12 @@ public abstract class BaseHapiFhirDao implements IDao, for (Long next : resourceIds) { txTemplate.execute(t -> { expungeHistoricalVersionsOfId(next, remainingCount); - if (remainingCount.get() <= 0) { - ourLog.debug("Expunge limit has been hit - Stopping operation"); - return toExpungeOutcome(theExpungeOptions, remainingCount); - } return null; }); + if (remainingCount.get() <= 0) { + ourLog.debug("Expunge limit has been hit - Stopping operation"); + return toExpungeOutcome(theExpungeOptions, remainingCount); + } } /* @@ -295,6 +295,10 @@ public abstract class BaseHapiFhirDao implements IDao, expungeCurrentVersionOfResource(next, remainingCount); return null; }); + if (remainingCount.get() <= 0) { + ourLog.debug("Expunge limit has been hit - Stopping operation"); + return toExpungeOutcome(theExpungeOptions, remainingCount); + } } } From 1d5f648070dea5a4828d33e09fb6b390491d8b9f Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Wed, 3 Apr 2019 13:32:10 -0400 Subject: [PATCH 3/5] fixed second expunge bug reported by FMCNA --- .../ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java | 8 +++++-- .../dao/data/IResourceHistoryTableDao.java | 17 +++++++++----- .../ResourceProviderExpungeDstu3Test.java | 23 ++++++++++++++++++- 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java index 250866bd3ca..60635b0f763 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java @@ -310,8 +310,12 @@ public abstract class BaseHapiFhirDao implements IDao, */ Pageable page = PageRequest.of(0, remainingCount.get()); Slice historicalIds = txTemplate.execute(t -> { - if (theResourceId != null && theVersion != null) { - return toSlice(myResourceHistoryTableDao.findForIdAndVersion(theResourceId, theVersion)); + if (theResourceId != null) { + if (theVersion != null) { + return toSlice(myResourceHistoryTableDao.findForIdAndVersion(theResourceId, theVersion)); + } else { + return myResourceHistoryTableDao.findIdsOfPreviousVersionsOfResourceId(page, theResourceId); + } } else { if (theResourceName != null) { return myResourceHistoryTableDao.findIdsOfPreviousVersionsOfResources(page, theResourceName); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IResourceHistoryTableDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IResourceHistoryTableDao.java index 47183ac7d8a..df5f6a84f73 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IResourceHistoryTableDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IResourceHistoryTableDao.java @@ -1,10 +1,6 @@ package ca.uhn.fhir.jpa.dao.data; -import java.util.Collection; -import java.util.Date; - -import javax.persistence.TemporalType; - +import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable; import org.springframework.data.domain.Pageable; import org.springframework.data.domain.Slice; import org.springframework.data.jpa.repository.JpaRepository; @@ -13,7 +9,9 @@ import org.springframework.data.jpa.repository.Query; import org.springframework.data.jpa.repository.Temporal; import org.springframework.data.repository.query.Param; -import ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable; +import javax.persistence.TemporalType; +import java.util.Collection; +import java.util.Date; /* * #%L @@ -74,6 +72,13 @@ public interface IResourceHistoryTableDao extends JpaRepository findForResourceId(Pageable thePage, @Param("resId") Long theId, @Param("dontWantVersion") Long theDontWantVersion); + @Query("" + + "SELECT v.myId FROM ResourceHistoryTable v " + + "LEFT OUTER JOIN ResourceTable t ON (v.myResourceId = t.myId) " + + "WHERE v.myResourceVersion != t.myVersion AND " + + "t.myId = :resId") + Slice findIdsOfPreviousVersionsOfResourceId(Pageable thePage, @Param("resId") Long theResourceId); + @Query("" + "SELECT v.myId FROM ResourceHistoryTable v " + "LEFT OUTER JOIN ResourceTable t ON (v.myResourceId = t.myId) " + diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderExpungeDstu3Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderExpungeDstu3Test.java index 68c89f73335..05648fbaa19 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderExpungeDstu3Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderExpungeDstu3Test.java @@ -18,7 +18,8 @@ import org.junit.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; public class ResourceProviderExpungeDstu3Test extends BaseResourceProviderDstu3Test { @@ -326,6 +327,26 @@ public class ResourceProviderExpungeDstu3Test extends BaseResourceProviderDstu3T assertGone(myDeletedObservationId); } + @Test + public void testExpungeNothing() { + + myPatientDao.expunge(myOneVersionPatientId.toUnqualifiedVersionless(), new ExpungeOptions() + .setExpungeDeletedResources(true) + .setExpungeOldVersions(true)); + + // Patients + assertStillThere(myOneVersionPatientId); + assertStillThere(myTwoVersionPatientId.withVersion("1")); + assertStillThere(myTwoVersionPatientId.withVersion("2")); + assertGone(myDeletedPatientId); + + // No observations deleted + assertStillThere(myOneVersionObservationId); + assertStillThere(myTwoVersionObservationId.withVersion("1")); + assertStillThere(myTwoVersionObservationId.withVersion("2")); + assertGone(myDeletedObservationId); + } + @Test public void testParameters() { Parameters p = new Parameters(); From 081ae89a4643c2063d1cc28e67d6985ca9bd146a Mon Sep 17 00:00:00 2001 From: Ken Stevens Date: Wed, 3 Apr 2019 13:42:24 -0400 Subject: [PATCH 4/5] changelog --- src/changes/changes.xml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 7c53068a282..94fa7bedd80 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -125,6 +125,13 @@ in the built WAR, in order to prevent duplicates and conflicts in implementing projects. + + Two expunge bug fixes: + The first bug is that the expunge operation wasn't bailing once it hit its limit. This resulted in a + "Page size must not be less than one!" error. + The second bug is that one case wasn't properly handled: when a resourceId with no version is provided. + This executed the case where only resource type is provided. + From 54e3c50ad75e6787ec9a839dea756a963c2db391 Mon Sep 17 00:00:00 2001 From: jamesagnew Date: Wed, 3 Apr 2019 20:15:07 -0400 Subject: [PATCH 5/5] FIx a build failure --- .../src/test/java/ca/uhn/fhir/jpa/dao/r4/BaseJpaR4Test.java | 4 ++-- .../jpa/provider/r4/ResourceProviderInterceptorR4Test.java | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/BaseJpaR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/BaseJpaR4Test.java index f6bf6eced40..e15b11279c2 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/BaseJpaR4Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/BaseJpaR4Test.java @@ -299,6 +299,8 @@ public abstract class BaseJpaR4Test extends BaseJpaTest { myDaoConfig.setAllowContainsSearches(new DaoConfig().isAllowContainsSearches()); myInterceptorRegistry.clearAnonymousHookForUnitTest(); + + myInterceptorRegistry.unregisterInterceptor(myPerformanceTracingLoggingInterceptor); } @After @@ -351,8 +353,6 @@ public abstract class BaseJpaR4Test extends BaseJpaTest { myDaoConfig.setHardTagListLimit(1000); myDaoConfig.setIncludeLimit(2000); myFhirCtx.setParserErrorHandler(new StrictErrorHandler()); - - myInterceptorRegistry.unregisterInterceptor(myPerformanceTracingLoggingInterceptor); } @Override diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderInterceptorR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderInterceptorR4Test.java index 3fe539a7f89..1b3000f799a 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderInterceptorR4Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderInterceptorR4Test.java @@ -119,6 +119,7 @@ public class ResourceProviderInterceptorR4Test extends BaseResourceProviderR4Tes for (int i = 0; i < 30; i++) { Patient p = new Patient(); p.addName().setFamily("FAM"+i); + ourLog.info("About to create patient"); myPatientDao.create(p); } @@ -130,6 +131,8 @@ public class ResourceProviderInterceptorR4Test extends BaseResourceProviderR4Tes myInterceptorRegistry.registerAnonymousInterceptor(Pointcut.PERFTRACE_SEARCH_SELECT_COMPLETE, interceptor); myInterceptors.add(interceptor); + ourLog.info("About to perform search..."); + Bundle results = ourClient.search().forResource(Patient.class).returnBundle(Bundle.class).execute(); verify(interceptor, times(1)).invoke(eq(Pointcut.PERFTRACE_SEARCH_FIRST_RESULT_LOADED), myParamsCaptor.capture()); verify(interceptor, times(1)).invoke(eq(Pointcut.PERFTRACE_SEARCH_SELECT_COMPLETE), myParamsCaptor.capture());