use SearchParamater validator in package installer

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

@@ -34,6 +34,7 @@ import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao;
 import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
 import ca.uhn.fhir.jpa.dao.data.INpmPackageVersionDao;
 import ca.uhn.fhir.jpa.dao.tx.IHapiTransactionService;
+import ca.uhn.fhir.jpa.dao.validation.SearchParameterDaoValidator;
 import ca.uhn.fhir.jpa.model.config.PartitionSettings;
 import ca.uhn.fhir.jpa.model.entity.NpmPackageVersionEntity;
 import ca.uhn.fhir.jpa.packages.loader.PackageResourceParsingSvc;
@@ -47,8 +48,10 @@ import ca.uhn.fhir.rest.param.StringParam;
 import ca.uhn.fhir.rest.param.TokenParam;
 import ca.uhn.fhir.rest.param.UriParam;
 import ca.uhn.fhir.rest.server.exceptions.ResourceVersionConflictException;
+import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException;
 import ca.uhn.fhir.util.FhirTerser;
 import ca.uhn.fhir.util.SearchParameterUtil;
+import ca.uhn.hapi.converters.canonical.VersionCanonicalizer;
 import com.google.common.annotations.VisibleForTesting;
 import jakarta.annotation.PostConstruct;
 import org.apache.commons.lang3.Validate;
@@ -73,7 +76,6 @@ import java.util.Optional;
 
 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.isBlank;
 
 /**
  * @since 5.1.0
@@ -117,6 +119,12 @@ public class PackageInstallerSvcImpl implements IPackageInstallerSvc {
 	@Autowired
 	private JpaStorageSettings myStorageSettings;
 
+	@Autowired
+	private SearchParameterDaoValidator mySearchParameterDaoValidator;
+
+	@Autowired
+	private VersionCanonicalizer myVersionCanonicalizer;
+
 	/**
 	 * Constructor
 	 */
@@ -431,6 +439,23 @@ public class PackageInstallerSvcImpl implements IPackageInstallerSvc {
 		return outcome != null && !outcome.isNop();
 	}
 
+	/*
+	 * This function helps preserve the resource types in the base of an existing SP when an overriding SP's base
+	 * covers only a subset of the existing base.
+	 *
+	 * For example, say for an existing SP,
+	 *  -  the current base is: [ResourceTypeA, ResourceTypeB]
+	 *   - the new base is: [ResourceTypeB]
+	 *
+	 * If we were to overwrite the existing SP's base to the new base ([ResourceTypeB]) then the
+	 * SP would stop working on ResourceTypeA, which would be a loss of functionality.
+	 *
+	 * Instead, this function updates the existing SP's base by removing the resource types that
+	 * are covered by the overriding SP.
+	 * In our example, this function updates the existing SP's base to [ResourceTypeA], so that the existing SP
+	 * still works on ResourceTypeA, and the caller then creates a new SP that covers ResourceTypeB.
+	 * https://github.com/hapifhir/hapi-fhir/issues/5366
+	 */
 	private boolean updateExistingResourceIfNecessary(
 			IFhirResourceDao theDao, IBaseResource theResource, IBaseResource theExistingResource) {
 		if (!"SearchParameter".equals(theResource.getClass().getSimpleName())) {
@@ -506,33 +531,9 @@ public class PackageInstallerSvcImpl implements IPackageInstallerSvc {
 
 	boolean validForUpload(IBaseResource theResource) {
 		String resourceType = myFhirContext.getResourceType(theResource);
-		if ("SearchParameter".equals(resourceType)) {
+		if ("SearchParameter".equals(resourceType) && !isValidSearchParameter(theResource)) {
-
+			// this is an invalid search parameter
-			String code = SearchParameterUtil.getCode(myFhirContext, theResource);
+			return false;
-			if (!isBlank(code) && code.startsWith("_")) {
-				ourLog.warn(
-						"Failed to validate resource of type {} with url {} - Error: Resource code starts with \"_\"",
-						theResource.fhirType(),
-						SearchParameterUtil.getURL(myFhirContext, theResource));
-				return false;
-			}
-
-			String expression = SearchParameterUtil.getExpression(myFhirContext, theResource);
-			if (isBlank(expression)) {
-				ourLog.warn(
-						"Failed to validate resource of type {} with url {} - Error: Resource expression is blank",
-						theResource.fhirType(),
-						SearchParameterUtil.getURL(myFhirContext, theResource));
-				return false;
-			}
-
-			if (getBaseAsStrings(myFhirContext, theResource).isEmpty()) {
-				ourLog.warn(
-						"Failed to validate resource of type {} with url {} - Error: Resource base is empty",
-						theResource.fhirType(),
-						SearchParameterUtil.getURL(myFhirContext, theResource));
-				return false;
-			}
 		}
 
 		if (!isValidResourceStatusForPackageUpload(theResource)) {
@@ -546,6 +547,21 @@ public class PackageInstallerSvcImpl implements IPackageInstallerSvc {
 		return true;
 	}
 
+	private boolean isValidSearchParameter(IBaseResource theResource) {
+		try {
+			org.hl7.fhir.r5.model.SearchParameter searchParameter =
+					myVersionCanonicalizer.searchParameterToCanonical(theResource);
+			mySearchParameterDaoValidator.validate(searchParameter);
+			return true;
+		} catch (UnprocessableEntityException unprocessableEntityException) {
+			ourLog.error(
+					"The SearchParameter with URL {} is invalid. Validation Error: {}",
+					SearchParameterUtil.getURL(myFhirContext, theResource),
+					unprocessableEntityException.getMessage());
+			return false;
+		}
+	}
+
 	/**
 	 * For resources like {@link org.hl7.fhir.r4.model.Subscription}, {@link org.hl7.fhir.r4.model.DocumentReference},
 	 * and {@link org.hl7.fhir.r4.model.Communication}, the status field doesn't necessarily need to be set to 'active'
@@ -569,9 +585,13 @@ public class PackageInstallerSvcImpl implements IPackageInstallerSvc {
 		List<IPrimitiveType> statusTypes =
 				myFhirContext.newFhirPath().evaluate(theResource, "status", IPrimitiveType.class);
 		// Resource does not have a status field
-		if (statusTypes.isEmpty()) return true;
+		if (statusTypes.isEmpty()) {
+			return true;
+		}
 		// Resource has a null status field
-		if (statusTypes.get(0).getValue() == null) return false;
+		if (statusTypes.get(0).getValue() == null) {
+			return false;
+		}
 		// Resource has a status, and we need to check based on type
 		switch (theResource.fhirType()) {
 			case "Subscription":

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

@@ -9,13 +9,20 @@ import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao;
 import ca.uhn.fhir.jpa.dao.data.INpmPackageVersionDao;
 import ca.uhn.fhir.jpa.dao.tx.IHapiTransactionService;
 import ca.uhn.fhir.jpa.dao.tx.NonTransactionalHapiTransactionService;
+import ca.uhn.fhir.jpa.dao.validation.SearchParameterDaoValidator;
 import ca.uhn.fhir.jpa.model.config.PartitionSettings;
 import ca.uhn.fhir.jpa.packages.loader.PackageResourceParsingSvc;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.jpa.searchparam.registry.ISearchParamRegistryController;
 import ca.uhn.fhir.jpa.searchparam.util.SearchParameterHelper;
+import ca.uhn.fhir.mdm.log.Logs;
 import ca.uhn.fhir.rest.api.server.RequestDetails;
 import ca.uhn.fhir.rest.server.SimpleBundleProvider;
+import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException;
+import ca.uhn.hapi.converters.canonical.VersionCanonicalizer;
+import ca.uhn.test.util.LogbackTestExtension;
+import ca.uhn.test.util.LogbackTestExtensionAssert;
+import ch.qos.logback.classic.Logger;
 import jakarta.annotation.Nonnull;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.r4.model.CodeSystem;
@@ -24,6 +31,7 @@ import org.hl7.fhir.r4.model.Communication;
 import org.hl7.fhir.r4.model.DocumentReference;
 import org.hl7.fhir.r4.model.Enumerations;
 import org.hl7.fhir.r4.model.IdType;
+import org.hl7.fhir.r4.model.Patient;
 import org.hl7.fhir.r4.model.SearchParameter;
 import org.hl7.fhir.r4.model.Subscription;
 import org.hl7.fhir.utilities.npm.NpmPackage;
@@ -31,6 +39,7 @@ 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.api.extension.RegisterExtension;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
@@ -40,6 +49,7 @@ import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.Spy;
 import org.mockito.junit.jupiter.MockitoExtension;
+import org.slf4j.LoggerFactory;
 
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
@@ -52,8 +62,14 @@ 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.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.params.provider.Arguments.arguments;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -63,6 +79,10 @@ public class PackageInstallerSvcImplTest {
 	public static final String PACKAGE_VERSION = "1.0";
 	public static final String PACKAGE_ID_1 = "package1";
 
+
+	@RegisterExtension
+	LogbackTestExtension myLogCapture = new LogbackTestExtension(LoggerFactory.getLogger(PackageInstallerSvcImpl.class));
+
 	@Mock
 	private INpmPackageVersionDao myPackageVersionDao;
 	@Mock
@@ -83,6 +103,13 @@ public class PackageInstallerSvcImplTest {
 	private SearchParameterMap mySearchParameterMap;
 	@Mock
 	private JpaStorageSettings myStorageSettings;
+
+	@Mock
+	private VersionCanonicalizer myVersionCanonicalizerMock;
+
+	@Mock
+	private SearchParameterDaoValidator mySearchParameterDaoValidatorMock;
+
 	@Spy
 	private FhirContext myCtx = FhirContext.forR4Cached();
 	@Spy
@@ -91,6 +118,8 @@ public class PackageInstallerSvcImplTest {
 	private PackageResourceParsingSvc myPackageResourceParsingSvc = new PackageResourceParsingSvc(myCtx);
 	@Spy
 	private PartitionSettings myPartitionSettings = new PartitionSettings();
+
+
 	@InjectMocks
 	private PackageInstallerSvcImpl mySvc;
 
@@ -110,66 +139,97 @@ public class PackageInstallerSvcImplTest {
 
 	@Nested
 	class ValidForUploadTest {
+
 		public static Stream<Arguments> parametersIsValidForUpload() {
-			SearchParameter sp1 = new SearchParameter();
+			// Patient resource doesn't have a status element in FHIR spec
-			sp1.setCode("_id");
+			Patient resourceWithNoStatusElementInSpec = new Patient();
 
-			SearchParameter sp2 = new SearchParameter();
+			SearchParameter spWithActiveStatus = new SearchParameter();
-			sp2.setCode("name");
+			spWithActiveStatus.setStatus(Enumerations.PublicationStatus.ACTIVE);
-			sp2.setExpression("Patient.name");
-			sp2.setStatus(Enumerations.PublicationStatus.ACTIVE);
 
-			SearchParameter sp3 = new SearchParameter();
+			SearchParameter spWithDraftStatus = new SearchParameter();
-			sp3.setCode("name");
+			spWithDraftStatus.setStatus(Enumerations.PublicationStatus.DRAFT);
-			sp3.addBase("Patient");
-			sp3.setStatus(Enumerations.PublicationStatus.ACTIVE);
 
-			SearchParameter sp4 = new SearchParameter();
+			SearchParameter spWithNullStatus = new SearchParameter();
-			sp4.setCode("name");
+			spWithNullStatus.setStatus(null);
-			sp4.addBase("Patient");
-			sp4.setExpression("Patient.name");
-			sp4.setStatus(Enumerations.PublicationStatus.ACTIVE);
-
-			SearchParameter sp5 = new SearchParameter();
-			sp5.setCode("name");
-			sp5.addBase("Patient");
-			sp5.setExpression("Patient.name");
-			sp5.setStatus(Enumerations.PublicationStatus.DRAFT);
 
 			return Stream.of(
-					arguments(sp1, false, false),
+					arguments(resourceWithNoStatusElementInSpec, true),
-					arguments(sp2, false, true),
+					arguments(spWithActiveStatus, true),
-					arguments(sp3, false, true),
+					arguments(spWithNullStatus, false),
-					arguments(sp4, true, true),
+					arguments(spWithDraftStatus, false),
-					arguments(sp5, true, false),
+					arguments(createSubscription(Subscription.SubscriptionStatus.REQUESTED), true),
-					arguments(createSubscription(Subscription.SubscriptionStatus.REQUESTED), true, true),
+					arguments(createSubscription(Subscription.SubscriptionStatus.ERROR), false),
-					arguments(createSubscription(Subscription.SubscriptionStatus.ERROR), true, false),
+					arguments(createSubscription(Subscription.SubscriptionStatus.ACTIVE), false),
-					arguments(createSubscription(Subscription.SubscriptionStatus.ACTIVE), true, false),
+					arguments(createDocumentReference(Enumerations.DocumentReferenceStatus.ENTEREDINERROR), true),
-					arguments(createDocumentReference(Enumerations.DocumentReferenceStatus.ENTEREDINERROR), true, true),
+					arguments(createDocumentReference(Enumerations.DocumentReferenceStatus.NULL), false),
-					arguments(createDocumentReference(Enumerations.DocumentReferenceStatus.NULL), true, false),
+					arguments(createDocumentReference(null), false),
-					arguments(createDocumentReference(null), true, false),
+					arguments(createCommunication(Communication.CommunicationStatus.NOTDONE), true),
-					arguments(createCommunication(Communication.CommunicationStatus.NOTDONE), true, true),
+					arguments(createCommunication(Communication.CommunicationStatus.NULL), false),
-					arguments(createCommunication(Communication.CommunicationStatus.NULL), true, false),
+					arguments(createCommunication(null), false));
-					arguments(createCommunication(null), true, false));
 		}
 
 		@ParameterizedTest
 		@MethodSource(value = "parametersIsValidForUpload")
-		public void testValidForUpload_withResource(IBaseResource theResource,
+		public void testValidForUpload_WhenStatusValidationSettingIsEnabled_ValidatesResourceStatus(IBaseResource theResource,
-																  boolean theTheMeetsOtherFilterCriteria,
+																				 		  boolean theExpectedResultForStatusValidation) {
-																  boolean theMeetsStatusFilterCriteria) {
+			if (theResource.fhirType().equals("SearchParameter")) {
-			if (theTheMeetsOtherFilterCriteria) {
+				setupSearchParameterValidationMocksForSuccess();
-				when(myStorageSettings.isValidateResourceStatusForPackageUpload()).thenReturn(true);
 			}
-			assertEquals(theTheMeetsOtherFilterCriteria && theMeetsStatusFilterCriteria, mySvc.validForUpload(theResource));
+			when(myStorageSettings.isValidateResourceStatusForPackageUpload()).thenReturn(true);
+			assertEquals(theExpectedResultForStatusValidation, mySvc.validForUpload(theResource));
+		}
 
-			if (theTheMeetsOtherFilterCriteria) {
+		@ParameterizedTest
-				when(myStorageSettings.isValidateResourceStatusForPackageUpload()).thenReturn(false);
+		@MethodSource(value = "parametersIsValidForUpload")
+		public void testValidForUpload_WhenStatusValidationSettingIsDisabled_DoesNotValidateResourceStatus(IBaseResource theResource) {
+			if (theResource.fhirType().equals("SearchParameter")) {
+				setupSearchParameterValidationMocksForSuccess();
 			}
-			assertEquals(theTheMeetsOtherFilterCriteria, mySvc.validForUpload(theResource));
+			when(myStorageSettings.isValidateResourceStatusForPackageUpload()).thenReturn(false);
+			//all resources should pass status validation in this case, so expect true always
+			assertTrue(mySvc.validForUpload(theResource));
+		}
+
+		@Test
+		public void testValidForUpload_WhenSearchParameterIsInvalid_ReturnsFalse() {
+
+			final String validationExceptionMessage = "This SP is invalid!!";
+			final String spURL = "http://myspurl.example/invalidsp";
+			SearchParameter spR4 = new SearchParameter();
+			spR4.setUrl(spURL);
+			org.hl7.fhir.r5.model.SearchParameter spR5 = new org.hl7.fhir.r5.model.SearchParameter();
+
+			when(myVersionCanonicalizerMock.searchParameterToCanonical(spR4)).thenReturn(spR5);
+			doThrow(new UnprocessableEntityException(validationExceptionMessage)).
+				when(mySearchParameterDaoValidatorMock).validate(spR5);
+
+			assertFalse(mySvc.validForUpload(spR4));
+
+			final String expectedLogMessage = String.format(
+				"The SearchParameter with URL %s is invalid. Validation Error: %s", spURL, validationExceptionMessage);
+			LogbackTestExtensionAssert.assertThat(myLogCapture).hasErrorMessage(expectedLogMessage);
+		}
+
+		@Test
+		public void testValidForUpload_WhenSearchParameterValidatorThrowsAnExceptionOtherThanUnprocessableEntityException_ThenThrows() {
+
+			SearchParameter spR4 = new SearchParameter();
+			org.hl7.fhir.r5.model.SearchParameter spR5 = new org.hl7.fhir.r5.model.SearchParameter();
+
+			RuntimeException notAnUnprocessableEntityException = new RuntimeException("should not be caught");
+			when(myVersionCanonicalizerMock.searchParameterToCanonical(spR4)).thenReturn(spR5);
+			doThrow(notAnUnprocessableEntityException).
+				when(mySearchParameterDaoValidatorMock).validate(spR5);
+
+			Exception actualExceptionThrown = assertThrows(Exception.class, () -> mySvc.validForUpload(spR4));
+			assertEquals(notAnUnprocessableEntityException, actualExceptionThrown);
 		}
 	}
 
+
+
+
 	@Test
 	public void testDontTryToInstallDuplicateCodeSystem_CodeSystemAlreadyExistsWithDifferentId() throws IOException {
 		// Setup
@@ -296,6 +356,11 @@ public class PackageInstallerSvcImplTest {
 		return pkg;
 	}
 
+	private void setupSearchParameterValidationMocksForSuccess() {
+		when(myVersionCanonicalizerMock.searchParameterToCanonical(any())).thenReturn(new org.hl7.fhir.r5.model.SearchParameter());
+		doNothing().when(mySearchParameterDaoValidatorMock).validate(any());
+	}
+
 	private static SearchParameter createSearchParameter(String theId, Collection<String> theBase) {
 		SearchParameter searchParameter = new SearchParameter();
 		if (theId != null) {
@@ -330,4 +395,5 @@ public class PackageInstallerSvcImplTest {
 		communication.setStatus(theCommunicationStatus);
 		return communication;
 	}
+
 }

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4SearchCustomSearchParamTest.java

@@ -360,7 +360,22 @@ public class FhirResourceDaoR4SearchCustomSearchParamTest extends BaseJpaR4Test
 	}
 
 	@Test
-	public void testCreateInvalidParamNoPath() {
+	public void testCreateCompositeParamNoExpressionAtRootLevel() {
+		// allow composite search parameter to have no expression element at root level on the resource
+		SearchParameter fooSp = new SearchParameter();
+		fooSp.addBase("Patient");
+		fooSp.setCode("foo");
+		fooSp.setType(Enumerations.SearchParamType.COMPOSITE);
+		fooSp.setTitle("FOO SP");
+		fooSp.setStatus(org.hl7.fhir.r4.model.Enumerations.PublicationStatus.ACTIVE);
+
+		// Ensure that no exceptions are thrown
+		mySearchParameterDao.create(fooSp, mySrd);
+		mySearchParamRegistry.forceRefresh();
+	}
+
+	@Test
+	public void testCreateInvalidParamNoExpression() {
 		SearchParameter fooSp = new SearchParameter();
 		fooSp.addBase("Patient");
 		fooSp.setCode("foo");

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/packages/PackageInstallerSvcImplCreateTest.java

@@ -9,10 +9,16 @@ import ca.uhn.fhir.jpa.entity.TermValueSet;
 import ca.uhn.fhir.jpa.searchparam.SearchParameterMap;
 import ca.uhn.fhir.jpa.test.BaseJpaR4Test;
 import ca.uhn.fhir.model.primitive.IdDt;
+import ca.uhn.fhir.rest.api.server.IBundleProvider;
 import ca.uhn.fhir.rest.api.server.SystemRequestDetails;
+import ca.uhn.fhir.rest.param.TokenParam;
+import com.github.dnault.xmlpatch.repackaged.org.jaxen.util.SingletonList;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 import org.hl7.fhir.r4.model.CodeSystem;
+import org.hl7.fhir.r4.model.CodeType;
+import org.hl7.fhir.r4.model.Enumerations;
 import org.hl7.fhir.r4.model.NamingSystem;
+import org.hl7.fhir.r4.model.SearchParameter;
 import org.hl7.fhir.r4.model.ValueSet;
 import org.hl7.fhir.utilities.npm.NpmPackage;
 import org.hl7.fhir.utilities.npm.PackageGenerator;
@@ -149,6 +155,24 @@ public class PackageInstallerSvcImplCreateTest extends BaseJpaR4Test {
 		assertEquals(copyright2, actualValueSet2.getCopyright());
 	}
 
+	@Test
+	void installCompositeSearchParameterWithNoExpressionAtRoot() throws IOException {
+		final String spCode = "my-test-composite-sp-with-no-expression";
+		SearchParameter spR4 = new SearchParameter();
+		spR4.setStatus(Enumerations.PublicationStatus.ACTIVE);
+		spR4.setType(Enumerations.SearchParamType.COMPOSITE);
+		spR4.setBase(List.of(new CodeType("Patient")));
+		spR4.setCode(spCode);
+
+		install(spR4);
+
+		// verify the SP is created
+		SearchParameterMap map = SearchParameterMap.newSynchronous()
+			.add(SearchParameter.SP_CODE, new TokenParam(spCode));
+		IBundleProvider outcome = mySearchParameterDao.search(map);
+		assertEquals(1, outcome.size());
+	}
+
 	@Nonnull
 	private List<ValueSet> getAllValueSets() {
 		final List<IBaseResource> allResources = myValueSetDao.search(SearchParameterMap.newSynchronous(), REQUEST_DETAILS).getAllResources();

hapi-fhir-jpaserver-test-utilities/src/test/java/ca/uhn/fhir/jpa/dao/validation/SearchParameterDaoValidatorTest.java

@@ -114,9 +114,9 @@ public class SearchParameterDaoValidatorTest {
     }
 
     @ParameterizedTest
-    @MethodSource("extensionProvider")
+    @MethodSource("extensionAndRootLevelExpressionProvider")
-    public void testMethodValidate_nonUniqueComboAndCompositeSearchParamWithComponentOfTypeReference_isNotAllowed(Extension theExtension) {
+    public void testMethodValidate_nonUniqueComboAndCompositeSearchParamWithComponentOfTypeReference_isNotAllowed(Extension theExtension, String theRootLevelExpression) {
-        SearchParameter sp = createSearchParameter(COMPOSITE, "SearchParameter/patient-code", "patient-code", "Observation");
+        SearchParameter sp = createSearchParameter(COMPOSITE, "SearchParameter/patient-code", "patient-code", theRootLevelExpression);
         sp.addExtension(theExtension);
 
         sp.addComponent(new SearchParameterComponentComponent().setDefinition(SP_COMPONENT_DEFINITION_OF_TYPE_TOKEN));
@@ -205,22 +205,35 @@ public class SearchParameterDaoValidatorTest {
         return retVal;
     }
 
-    static Stream<Arguments> extensionProvider() {
+	private static Extension createUniquenessExtension(boolean theIsUnique) {
-        return Stream.of(
+		return new Extension(HapiExtensions.EXT_SP_UNIQUE, new BooleanType(theIsUnique));
-                Arguments.of(
+	}
-                        new Extension(HapiExtensions.EXT_SP_UNIQUE, new BooleanType(false))), // composite SP of type combo with non-unique index
+
-                Arguments.of((Object) null) // composite SP
+	static Stream<Arguments> extensionAndRootLevelExpressionProvider() {
-        );
+		return Stream.of(
-    }
+			// composite SP of type combo with non-unique index, and expression at root level
+			Arguments.of(createUniquenessExtension(false), "Observation"),
+			// composite SP of type combo with non-unique index with null expression at root level
+			Arguments.of(createUniquenessExtension(false), null),
+			// composite SP with expression at root level
+		    Arguments.of((Object) null, "Observation"),
+			// composite SP with null expression at root level
+			Arguments.of((Object) null, null)
+
+		);
+	}
+
 
     static Stream<Arguments> comboSpProvider() {
         return Stream.of(
                 Arguments.of(createSearchParameter(Enumerations.SearchParamType.COMPOSITE, "SearchParameter/any-type", "any-type", "Observation")
-                        .addExtension(new Extension(HapiExtensions.EXT_SP_UNIQUE, new BooleanType(false)))), // composite SP of type combo with non-unique index
+                        .addExtension(createUniquenessExtension(false))), // composite SP of type combo with non-unique
+			// index
 
                 Arguments.of(createSearchParameter(Enumerations.SearchParamType.COMPOSITE, "SearchParameter/any-type", "any-type", "Observation")
-                        .addExtension(new Extension(HapiExtensions.EXT_SP_UNIQUE, new BooleanType(true)))) // composite SP of type combo with unique index
+                        .addExtension(createUniquenessExtension(true))) // composite SP of type combo with unique index
-        );
+
+		);
     }
 
     static Stream<Arguments> compositeSpProvider() {

hapi-fhir-storage/src/main/java/ca/uhn/fhir/jpa/dao/validation/SearchParameterDaoValidator.java

@@ -98,31 +98,30 @@ public class SearchParameterDaoValidator {
 			return;
 		}
 
-		// Search parameters must have a base
+		// Search parameters must have a base except for Composite ones (for which consider it optional)
-		if (isCompositeWithoutBase(searchParameter)) {
+		if (isWithoutBase(searchParameter) && !isCompositeSp(searchParameter)) {
 			throw new UnprocessableEntityException(Msg.code(1113) + "SearchParameter.base is missing");
 		}
 
-		// Do we have a valid expression
+		// Search parameters must have an expression except for the Composite ones (for which consider it optional)
-		if (isCompositeWithoutExpression(searchParameter)) {
+		if (isBlank(searchParameter.getExpression()) && !isCompositeSp(searchParameter)) {
-
-			// this is ok
-
-		} else if (isBlank(searchParameter.getExpression())) {
-
 			throw new UnprocessableEntityException(Msg.code(1114) + "SearchParameter.expression is missing");
-
-		} else {
-
-			FhirVersionEnum fhirVersion = myFhirContext.getVersion().getVersion();
-			if (fhirVersion.isOlderThan(FhirVersionEnum.DSTU3)) {
-				// omitting validation for DSTU2_HL7ORG, DSTU2_1 and DSTU2
-			} else {
-				maybeValidateCompositeSpForUniqueIndexing(searchParameter);
-				maybeValidateSearchParameterExpressionsOnSave(searchParameter);
-				maybeValidateCompositeWithComponent(searchParameter);
-			}
 		}
+
+		FhirVersionEnum fhirVersion = myFhirContext.getVersion().getVersion();
+		if (fhirVersion.isOlderThan(FhirVersionEnum.DSTU3)) {
+			// omitting validation for DSTU2_HL7ORG, DSTU2_1 and DSTU2
+			return;
+		}
+
+		maybeValidateCompositeSpForUniqueIndexing(searchParameter);
+		if (!isBlank(searchParameter.getExpression())) {
+			// above we validated the expression is not blank for a non-composite SP.
+			// So, this validation happens for all non-composite SPs and
+			// composite SPs that have an expression at root level
+			maybeValidateSearchParameterExpressionsOnSave(searchParameter);
+		}
+		maybeValidateCompositeWithComponent(searchParameter);
 	}
 
 	private boolean isCompositeSp(SearchParameter theSearchParameter) {
@@ -130,15 +129,10 @@ public class SearchParameterDaoValidator {
 				&& theSearchParameter.getType().equals(Enumerations.SearchParamType.COMPOSITE);
 	}
 
-	private boolean isCompositeWithoutBase(SearchParameter searchParameter) {
+	private boolean isWithoutBase(SearchParameter searchParameter) {
 		return ElementUtil.isEmpty(searchParameter.getBase())
 				&& ElementUtil.isEmpty(
-						searchParameter.getExtensionsByUrl(HapiExtensions.EXTENSION_SEARCHPARAM_CUSTOM_BASE_RESOURCE))
+						searchParameter.getExtensionsByUrl(HapiExtensions.EXTENSION_SEARCHPARAM_CUSTOM_BASE_RESOURCE));
-				&& !isCompositeSp(searchParameter);
-	}
-
-	private boolean isCompositeWithoutExpression(SearchParameter searchParameter) {
-		return isCompositeSp(searchParameter) && isBlank(searchParameter.getExpression());
 	}
 
 	private boolean isCompositeWithComponent(SearchParameter theSearchParameter) {