use SearchParamater validator in package installer (#6112)

This commit is contained in:
Emre Dincturk 2024-07-16 14:52:41 -04:00 committed by GitHub
parent 39cbec8aa4
commit 13e2d41165
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 206 additions and 75 deletions

View File

@ -0,0 +1,6 @@
---
type: fix
issue: 6111
title: "Previously, the package installer wouldn't create a composite SearchParameter resource if the SearchParameter
resource didn't have an expression element at the root level. This has now been fixed by making
SearchParameter validation in package installer consistent with the DAO level validations."

View File

@ -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)) {
String code = SearchParameterUtil.getCode(myFhirContext, theResource);
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 ("SearchParameter".equals(resourceType) && !isValidSearchParameter(theResource)) {
// this is an invalid search parameter
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":

View File

@ -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();
sp1.setCode("_id");
// Patient resource doesn't have a status element in FHIR spec
Patient resourceWithNoStatusElementInSpec = new Patient();
SearchParameter sp2 = new SearchParameter();
sp2.setCode("name");
sp2.setExpression("Patient.name");
sp2.setStatus(Enumerations.PublicationStatus.ACTIVE);
SearchParameter spWithActiveStatus = new SearchParameter();
spWithActiveStatus.setStatus(Enumerations.PublicationStatus.ACTIVE);
SearchParameter sp3 = new SearchParameter();
sp3.setCode("name");
sp3.addBase("Patient");
sp3.setStatus(Enumerations.PublicationStatus.ACTIVE);
SearchParameter spWithDraftStatus = new SearchParameter();
spWithDraftStatus.setStatus(Enumerations.PublicationStatus.DRAFT);
SearchParameter sp4 = new SearchParameter();
sp4.setCode("name");
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);
SearchParameter spWithNullStatus = new SearchParameter();
spWithNullStatus.setStatus(null);
return Stream.of(
arguments(sp1, false, false),
arguments(sp2, false, true),
arguments(sp3, false, true),
arguments(sp4, true, true),
arguments(sp5, true, false),
arguments(createSubscription(Subscription.SubscriptionStatus.REQUESTED), true, true),
arguments(createSubscription(Subscription.SubscriptionStatus.ERROR), true, false),
arguments(createSubscription(Subscription.SubscriptionStatus.ACTIVE), true, false),
arguments(createDocumentReference(Enumerations.DocumentReferenceStatus.ENTEREDINERROR), true, true),
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));
arguments(resourceWithNoStatusElementInSpec, true),
arguments(spWithActiveStatus, true),
arguments(spWithNullStatus, false),
arguments(spWithDraftStatus, false),
arguments(createSubscription(Subscription.SubscriptionStatus.REQUESTED), true),
arguments(createSubscription(Subscription.SubscriptionStatus.ERROR), false),
arguments(createSubscription(Subscription.SubscriptionStatus.ACTIVE), false),
arguments(createDocumentReference(Enumerations.DocumentReferenceStatus.ENTEREDINERROR), true),
arguments(createDocumentReference(Enumerations.DocumentReferenceStatus.NULL), false),
arguments(createDocumentReference(null), false),
arguments(createCommunication(Communication.CommunicationStatus.NOTDONE), true),
arguments(createCommunication(Communication.CommunicationStatus.NULL), false),
arguments(createCommunication(null), false));
}
@ParameterizedTest
@MethodSource(value = "parametersIsValidForUpload")
public void testValidForUpload_withResource(IBaseResource theResource,
boolean theTheMeetsOtherFilterCriteria,
boolean theMeetsStatusFilterCriteria) {
if (theTheMeetsOtherFilterCriteria) {
when(myStorageSettings.isValidateResourceStatusForPackageUpload()).thenReturn(true);
public void testValidForUpload_WhenStatusValidationSettingIsEnabled_ValidatesResourceStatus(IBaseResource theResource,
boolean theExpectedResultForStatusValidation) {
if (theResource.fhirType().equals("SearchParameter")) {
setupSearchParameterValidationMocksForSuccess();
}
assertEquals(theTheMeetsOtherFilterCriteria && theMeetsStatusFilterCriteria, mySvc.validForUpload(theResource));
when(myStorageSettings.isValidateResourceStatusForPackageUpload()).thenReturn(true);
assertEquals(theExpectedResultForStatusValidation, mySvc.validForUpload(theResource));
}
if (theTheMeetsOtherFilterCriteria) {
when(myStorageSettings.isValidateResourceStatusForPackageUpload()).thenReturn(false);
@ParameterizedTest
@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;
}
}

View File

@ -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");

View File

@ -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();