Add package installer property resource filter on status (#5470)

* Expanding a ValueSet using hierarchical CodeSystem would fail in different scenarios with a constraint violation exception when codes (term concepts) were being persisted

* Expanding a ValueSet using hierarchical CodeSystem would fail in different scenarios with a constraint violation exception when codes (term concepts) were being persisted

* Small changes after code review. Doing some variable/method renaming and correcting changelog Jira issue number.

* There is a programmatic filter enabled which skips installation for certain resources based on their status
in PackageInstallerSvcImpl. The filter can now be controlled via StorageSettings.

* Fix typo in variable name from recent pull request 5461.

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_0_0/5469-package-installer-property-filter-resource-on-status.yaml

@@ -0,0 +1,6 @@
+---
+type: add
+issue: 5469
+jira: SMILE-7594
+title: "There is a programmatic filter enabled which skips installation for certain resources based on their status 
+in PackageInstallerSvcImpl. The filter can now be controlled via StorageSettings."

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/packages/PackageInstallerSvcImpl.java

@@ -28,6 +28,7 @@ import ca.uhn.fhir.context.support.IValidationSupport;
 import ca.uhn.fhir.context.support.ValidationSupportContext;
 import ca.uhn.fhir.i18n.Msg;
 import ca.uhn.fhir.interceptor.model.RequestPartitionId;
+import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
 import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
 import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao;
 import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
@@ -72,7 +73,6 @@ import javax.annotation.PostConstruct;
 
 import static ca.uhn.fhir.jpa.packages.util.PackageUtils.DEFAULT_INSTALL_TYPES;
 import static ca.uhn.fhir.util.SearchParameterUtil.getBaseAsStrings;
-import static org.apache.commons.lang3.StringUtils.defaultString;
 import static org.apache.commons.lang3.StringUtils.isBlank;
 
 /**
@@ -114,6 +114,9 @@ public class PackageInstallerSvcImpl implements IPackageInstallerSvc {
 	@Autowired
 	private PackageResourceParsingSvc myPackageResourceParsingSvc;
 
+	@Autowired
+	private JpaStorageSettings myStorageSettings;
+
 	/**
 	 * Constructor
 	 */
@@ -506,7 +509,7 @@ public class PackageInstallerSvcImpl implements IPackageInstallerSvc {
 		if ("SearchParameter".equals(resourceType)) {
 
 			String code = SearchParameterUtil.getCode(myFhirContext, theResource);
-			if (defaultString(code).startsWith("_")) {
+			if (!isBlank(code) && code.startsWith("_")) {
 				ourLog.warn(
 						"Failed to validate resource of type {} with url {} - Error: Resource code starts with \"_\"",
 						theResource.fhirType(),
@@ -548,7 +551,7 @@ public class PackageInstallerSvcImpl implements IPackageInstallerSvc {
 	 * and {@link org.hl7.fhir.r4.model.Communication}, the status field doesn't necessarily need to be set to 'active'
 	 * for that resource to be eligible for upload via packages. For example, all {@link org.hl7.fhir.r4.model.Subscription}
 	 * have a status of {@link org.hl7.fhir.r4.model.Subscription.SubscriptionStatus#REQUESTED} when they are originally
-	 * inserted into the database, so we accept that value for {@link org.hl7.fhir.r4.model.Subscription} isntead.
+	 * inserted into the database, so we accept that value for {@link org.hl7.fhir.r4.model.Subscription} instead.
 	 * Furthermore, {@link org.hl7.fhir.r4.model.DocumentReference} and {@link org.hl7.fhir.r4.model.Communication} can
 	 * exist with a wide variety of values for status that include ones such as
 	 * {@link org.hl7.fhir.r4.model.Communication.CommunicationStatus#ENTEREDINERROR},
@@ -560,6 +563,9 @@ public class PackageInstallerSvcImpl implements IPackageInstallerSvc {
 	 * @return {@link Boolean#TRUE} if the status value of this resource is acceptable for package upload.
 	 */
 	private boolean isValidResourceStatusForPackageUpload(IBaseResource theResource) {
+		if (!myStorageSettings.isValidateResourceStatusForPackageUpload()) {
+			return true;
+		}
 		List<IPrimitiveType> statusTypes =
 				myFhirContext.newFhirPath().evaluate(theResource, "status", IPrimitiveType.class);
 		// Resource does not have a status field

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/packages/PackageInstallerSvcImplTest.java

@@ -3,6 +3,7 @@ package ca.uhn.fhir.jpa.packages;
 import ca.uhn.fhir.context.FhirContext;
 import ca.uhn.fhir.context.FhirVersionEnum;
 import ca.uhn.fhir.context.support.IValidationSupport;
+import ca.uhn.fhir.jpa.api.config.JpaStorageSettings;
 import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
 import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao;
 import ca.uhn.fhir.jpa.dao.data.INpmPackageVersionDao;
@@ -26,9 +27,11 @@ import org.hl7.fhir.r4.model.SearchParameter;
 import org.hl7.fhir.r4.model.Subscription;
 import org.hl7.fhir.utilities.npm.NpmPackage;
 import org.hl7.fhir.utilities.npm.PackageGenerator;
+import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
 import org.mockito.ArgumentCaptor;
 import org.mockito.Captor;
@@ -46,10 +49,10 @@ import java.util.Collection;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Optional;
+import java.util.stream.Stream;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.params.provider.Arguments.arguments;
-import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
@@ -78,6 +81,8 @@ public class PackageInstallerSvcImplTest {
 	private SearchParameterHelper mySearchParameterHelper;
 	@Mock
 	private SearchParameterMap mySearchParameterMap;
+	@Mock
+	private JpaStorageSettings myStorageSettings;
 	@Spy
 	private FhirContext myCtx = FhirContext.forR4Cached();
 	@Spy
@@ -103,111 +108,66 @@ public class PackageInstallerSvcImplTest {
 		mySvc.assertFhirVersionsAreCompatible("R4", "R4B");
 	}
 
-	@Test
+	@Nested
-	public void testValidForUpload_SearchParameterWithMetaParam() {
+	class ValidForUploadTest {
-		SearchParameter sp = new SearchParameter();
+		public static Stream<Arguments> parametersIsValidForUpload() {
-		sp.setCode("_id");
+			SearchParameter sp1 = new SearchParameter();
-		assertFalse(mySvc.validForUpload(sp));
+			sp1.setCode("_id");
-	}
 
-	@Test
+			SearchParameter sp2 = new SearchParameter();
-	public void testValidForUpload_SearchParameterWithNoBase() {
+			sp2.setCode("name");
-		SearchParameter sp = new SearchParameter();
+			sp2.setExpression("Patient.name");
-		sp.setCode("name");
+			sp2.setStatus(Enumerations.PublicationStatus.ACTIVE);
-		sp.setExpression("Patient.name");
-		sp.setStatus(Enumerations.PublicationStatus.ACTIVE);
-		assertFalse(mySvc.validForUpload(sp));
-	}
 
-	@Test
+			SearchParameter sp3 = new SearchParameter();
-	public void testValidForUpload_SearchParameterWithNoExpression() {
+			sp3.setCode("name");
-		SearchParameter sp = new SearchParameter();
+			sp3.addBase("Patient");
-		sp.setCode("name");
+			sp3.setStatus(Enumerations.PublicationStatus.ACTIVE);
-		sp.addBase("Patient");
-		sp.setStatus(Enumerations.PublicationStatus.ACTIVE);
-		assertFalse(mySvc.validForUpload(sp));
-	}
 
+			SearchParameter sp4 = new SearchParameter();
+			sp4.setCode("name");
+			sp4.addBase("Patient");
+			sp4.setExpression("Patient.name");
+			sp4.setStatus(Enumerations.PublicationStatus.ACTIVE);
 
-	@Test
+			SearchParameter sp5 = new SearchParameter();
-	public void testValidForUpload_GoodSearchParameter() {
+			sp5.setCode("name");
-		SearchParameter sp = new SearchParameter();
+			sp5.addBase("Patient");
-		sp.setCode("name");
+			sp5.setExpression("Patient.name");
-		sp.addBase("Patient");
+			sp5.setStatus(Enumerations.PublicationStatus.DRAFT);
-		sp.setExpression("Patient.name");
-		sp.setStatus(Enumerations.PublicationStatus.ACTIVE);
-		assertTrue(mySvc.validForUpload(sp));
-	}
 
-	@Test
+			return Stream.of(
-	public void testValidForUpload_RequestedSubscription() {
+					arguments(sp1, false, false),
-		Subscription.SubscriptionChannelComponent subscriptionChannelComponent =
+					arguments(sp2, false, true),
-			new Subscription.SubscriptionChannelComponent()
+					arguments(sp3, false, true),
-				.setType(Subscription.SubscriptionChannelType.RESTHOOK)
+					arguments(sp4, true, true),
-				.setEndpoint("https://tinyurl.com/2p95e27r");
+					arguments(sp5, true, false),
-		Subscription subscription = new Subscription();
+					arguments(createSubscription(Subscription.SubscriptionStatus.REQUESTED), true, true),
-		subscription.setCriteria("Patient?name=smith");
+					arguments(createSubscription(Subscription.SubscriptionStatus.ERROR), true, false),
-		subscription.setChannel(subscriptionChannelComponent);
+					arguments(createSubscription(Subscription.SubscriptionStatus.ACTIVE), true, false),
-		subscription.setStatus(Subscription.SubscriptionStatus.REQUESTED);
+					arguments(createDocumentReference(Enumerations.DocumentReferenceStatus.ENTEREDINERROR), true, true),
-		assertTrue(mySvc.validForUpload(subscription));
+					arguments(createDocumentReference(Enumerations.DocumentReferenceStatus.NULL), true, false),
-	}
+					arguments(createDocumentReference(null), true, false),
+					arguments(createCommunication(Communication.CommunicationStatus.NOTDONE), true, true),
+					arguments(createCommunication(Communication.CommunicationStatus.NULL), true, false),
+					arguments(createCommunication(null), true, false));
+		}
 
-	@Test
+		@ParameterizedTest
-	public void testValidForUpload_ErrorSubscription() {
+		@MethodSource(value = "parametersIsValidForUpload")
-		Subscription.SubscriptionChannelComponent subscriptionChannelComponent =
+		public void testValidForUpload_withResource(IBaseResource theResource,
-			new Subscription.SubscriptionChannelComponent()
+																  boolean theTheMeetsOtherFilterCriteria,
-				.setType(Subscription.SubscriptionChannelType.RESTHOOK)
+																  boolean theMeetsStatusFilterCriteria) {
-				.setEndpoint("https://tinyurl.com/2p95e27r");
+			if (theTheMeetsOtherFilterCriteria) {
-		Subscription subscription = new Subscription();
+				when(myStorageSettings.isValidateResourceStatusForPackageUpload()).thenReturn(true);
-		subscription.setCriteria("Patient?name=smith");
+			}
-		subscription.setChannel(subscriptionChannelComponent);
+			assertEquals(theTheMeetsOtherFilterCriteria && theMeetsStatusFilterCriteria, mySvc.validForUpload(theResource));
-		subscription.setStatus(Subscription.SubscriptionStatus.ERROR);
-		assertFalse(mySvc.validForUpload(subscription));
-	}
 
-	@Test
+			if (theTheMeetsOtherFilterCriteria) {
-	public void testValidForUpload_ActiveSubscription() {
+				when(myStorageSettings.isValidateResourceStatusForPackageUpload()).thenReturn(false);
-		Subscription.SubscriptionChannelComponent subscriptionChannelComponent =
+			}
-			new Subscription.SubscriptionChannelComponent()
+			assertEquals(theTheMeetsOtherFilterCriteria, mySvc.validForUpload(theResource));
-				.setType(Subscription.SubscriptionChannelType.RESTHOOK)
+		}
-				.setEndpoint("https://tinyurl.com/2p95e27r");
-		Subscription subscription = new Subscription();
-		subscription.setCriteria("Patient?name=smith");
-		subscription.setChannel(subscriptionChannelComponent);
-		subscription.setStatus(Subscription.SubscriptionStatus.ACTIVE);
-		assertFalse(mySvc.validForUpload(subscription));
-	}
-
-	@Test
-	public void testValidForUpload_DocumentRefStatusValuePresent() {
-		DocumentReference documentReference = new DocumentReference();
-		documentReference.setStatus(Enumerations.DocumentReferenceStatus.ENTEREDINERROR);
-		assertTrue(mySvc.validForUpload(documentReference));
-	}
-
-	@Test
-	public void testValidForUpload_DocumentRefStatusValueNull() {
-		DocumentReference documentReference = new DocumentReference();
-		documentReference.setStatus(Enumerations.DocumentReferenceStatus.NULL);
-		assertFalse(mySvc.validForUpload(documentReference));
-		documentReference.setStatus(null);
-		assertFalse(mySvc.validForUpload(documentReference));
-	}
-
-	@Test
-	public void testValidForUpload_CommunicationStatusValuePresent() {
-		Communication communication = new Communication();
-		communication.setStatus(Communication.CommunicationStatus.NOTDONE);
-		assertTrue(mySvc.validForUpload(communication));
-	}
-
-	@Test
-	public void testValidForUpload_CommunicationStatusValueNull() {
-		Communication communication = new Communication();
-		communication.setStatus(Communication.CommunicationStatus.NULL);
-		assertFalse(mySvc.validForUpload(communication));
-		communication.setStatus(null);
-		assertFalse(mySvc.validForUpload(communication));
 	}
 
 	@Test
@@ -346,4 +306,28 @@ public class PackageInstallerSvcImplTest {
 		searchParameter.setExpression("someExpression");
 		return searchParameter;
 	}
+
+	private static Subscription createSubscription(Subscription.SubscriptionStatus theSubscriptionStatus) {
+		Subscription.SubscriptionChannelComponent subscriptionChannelComponent =
+				new Subscription.SubscriptionChannelComponent()
+						.setType(Subscription.SubscriptionChannelType.RESTHOOK)
+						.setEndpoint("https://tinyurl.com/2p95e27r");
+		Subscription subscription = new Subscription();
+		subscription.setCriteria("Patient?name=smith");
+		subscription.setChannel(subscriptionChannelComponent);
+		subscription.setStatus(theSubscriptionStatus);
+		return subscription;
+	}
+
+	private static DocumentReference createDocumentReference(Enumerations.DocumentReferenceStatus theDocumentStatus) {
+		DocumentReference documentReference = new DocumentReference();
+		documentReference.setStatus(theDocumentStatus);
+		return documentReference;
+	}
+
+	private static Communication createCommunication(Communication.CommunicationStatus theCommunicationStatus) {
+		Communication communication = new Communication();
+		communication.setStatus(theCommunicationStatus);
+		return communication;
+	}
 }

hapi-fhir-jpaserver-model/src/main/java/ca/uhn/fhir/jpa/model/entity/StorageSettings.java

@@ -146,6 +146,14 @@ public class StorageSettings {
 	 */
 	private boolean myLanguageSearchParameterEnabled = false;
 
+	/**
+	 * If set to false, all resource types will be installed via package installer, regardless of their status.
+	 * Otherwise, resources will be filtered based on status according to some criteria which can be found in
+	 * <code>PackageInstallerSvcImpl#isValidResourceStatusForPackageUpload<code>
+	 * @since 7.0.0
+	 */
+	private boolean myValidateResourceStatusForPackageUpload = true;
+
 	/**
 	 * If set to true, the server will prevent the creation of Subscriptions which cannot be evaluated IN-MEMORY. This can improve
 	 * overall server performance.
@@ -1319,6 +1327,22 @@ public class StorageSettings {
 		myLanguageSearchParameterEnabled = theLanguageSearchParameterEnabled;
 	}
 
+	/**
+	 * @return true if the filter is enabled for resources installed via package installer, false otherwise
+	 * @since 7.0.0
+	 */
+	public boolean isValidateResourceStatusForPackageUpload() {
+		return myValidateResourceStatusForPackageUpload;
+	}
+
+	/**
+	 * Should resources being installed via package installer be filtered.
+	 * @since 7.0.0
+	 */
+	public void setValidateResourceStatusForPackageUpload(boolean theValidateResourceStatusForPackageUpload) {
+		myValidateResourceStatusForPackageUpload = theValidateResourceStatusForPackageUpload;
+	}
+
 	private static void validateTreatBaseUrlsAsLocal(String theUrl) {
 		Validate.notBlank(theUrl, "Base URL must not be null or empty");
 

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/term/ValueSetExpansionWithHierarchyR4Test.java

@@ -97,13 +97,13 @@ public class ValueSetExpansionWithHierarchyR4Test extends BaseTermR4Test {
 
    @ParameterizedTest
    @MethodSource(value = "parametersValueSets")
-   public void testExpandValueSet_whenUsingHierarchicalCodeSystem_willExpandSuccessfully(ValueSet theValueSet, int theExpectedConceptExpensionCount) {
+   public void testExpandValueSet_whenUsingHierarchicalCodeSystem_willExpandSuccessfully(ValueSet theValueSet, int theExpectedConceptExpansionCount) {
       myValueSetDao.create(theValueSet, mySrd);
       myTermSvc.preExpandDeferredValueSetsToTerminologyTables();
       Optional<TermValueSet> optionalTermValueSet = runInTransaction(() -> myTermValueSetDao.findTermValueSetByUrlAndNullVersion(theValueSet.getUrl()));
       assertTrue(optionalTermValueSet.isPresent());
       TermValueSet expandedTermValueSet = optionalTermValueSet.get();
       assertEquals(TermValueSetPreExpansionStatusEnum.EXPANDED, expandedTermValueSet.getExpansionStatus());
-      assertEquals(theExpectedConceptExpensionCount, expandedTermValueSet.getTotalConcepts());
+      assertEquals(theExpectedConceptExpansionCount, expandedTermValueSet.getTotalConcepts());
    }
 }