Merge pull request #1263 from jamesagnew/expunge-testing

Expunge testing

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

@@ -224,6 +224,10 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> 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) {
@@ -275,12 +279,12 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> 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);
+				}
 			}
 
 			/*
@@ -291,6 +295,10 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> implements IDao,
 					expungeCurrentVersionOfResource(next, remainingCount);
 					return null;
 				});
+				if (remainingCount.get() <= 0) {
+					ourLog.debug("Expunge limit has been hit - Stopping operation");
+					return toExpungeOutcome(theExpungeOptions, remainingCount);
+				}
 			}
 
 		}
@@ -302,8 +310,12 @@ public abstract class BaseHapiFhirDao<T extends IBaseResource> implements IDao,
 			 */
 			Pageable page = PageRequest.of(0, remainingCount.get());
 			Slice<Long> historicalIds = txTemplate.execute(t -> {
-				if (theResourceId != null && theVersion != null) {
+				if (theResourceId != null) {
-					return toSlice(myResourceHistoryTableDao.findForIdAndVersion(theResourceId, theVersion));
+					if (theVersion != null) {
+						return toSlice(myResourceHistoryTableDao.findForIdAndVersion(theResourceId, theVersion));
+					} else {
+						return myResourceHistoryTableDao.findIdsOfPreviousVersionsOfResourceId(page, theResourceId);
+					}
 				} else {
 					if (theResourceName != null) {
 						return myResourceHistoryTableDao.findIdsOfPreviousVersionsOfResources(page, theResourceName);

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 ca.uhn.fhir.jpa.model.entity.ResourceHistoryTable;
-import java.util.Date;
-
-import javax.persistence.TemporalType;
-
 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<ResourceHistoryT
 	@Query("SELECT t.myId FROM ResourceHistoryTable t WHERE t.myResourceId = :resId AND t.myResourceVersion != :dontWantVersion")
 	Slice<Long> 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<Long> findIdsOfPreviousVersionsOfResourceId(Pageable thePage, @Param("resId") Long theResourceId);
+
 	@Query("" +
 		"SELECT v.myId FROM ResourceHistoryTable v " +
 		"LEFT OUTER JOIN ResourceTable t ON (v.myResourceId = t.myId) " +

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() {

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;
@@ -17,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 {
 
@@ -290,6 +292,61 @@ 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 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();

src/changes/changes.xml

@@ -125,6 +125,13 @@
 				in the built WAR, in order to prevent duplicates and conflicts in implementing
 				projects.
 			</action>
+			<action type="fix">
+				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.
+			</action>
 		</release>
 		<release version="3.7.0" date="2019-02-06" description="Gale">
 			<action type="add">