Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ CrSettings settings(
EvaluationSettings evaluationSettings, TerminologyServerClientSettings terminologyServerClientSettings) {
return new CrSettings()
.withEvaluationSettings(evaluationSettings)
.withTerminologyServerClientSettings(terminologyServerClientSettings);
.withTerminologyServerClientSettings(terminologyServerClientSettings)
.withValidatorPackage(new String[] {"hl7.fhir.us.ecr", "2.1.2"});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR says packages "are determined by the CrSettings validatorPackages field," but the field is populated with a hardcoded id/version, not from any property. The only knob is the on/off flag, not which packages.

Just flagging this for now. I understand this will likely be addressed once the PR is ready for a final review.

}

@Bean
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package org.opencds.cqf.fhir.cr.hapi.config;

import ca.uhn.fhir.validation.FhirValidator;
import org.opencds.cqf.fhir.cr.CrSettings;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.context.annotation.Condition;
import org.springframework.context.annotation.ConditionContext;
import org.springframework.core.type.AnnotatedTypeMetadata;

/**
* Condition that gates the creation of a {@link FhirValidator} configured to load NPM packages provided by {@link CrSettings}. Based on the {@code cr.ecr.validate.enabled}
* property (or the equivalent {@code CR_ECR_VALIDATE_ENABLED} environment variable).
*
* <p>Set {@code cr.ecr.validate.enabled=true} (or export {@code CR_ECR_VALIDATE_ENABLED=true}) to
* enable the creation of this {@link FhirValidator}.
*/
public class CrEcrValidateCondition implements Condition {
private static final Logger ourLog = LoggerFactory.getLogger(CrEcrValidateCondition.class);

static final String PROPERTY_NAME = "cr.ecr.validate.enabled";

@Override
public boolean matches(ConditionContext conditionContext, AnnotatedTypeMetadata annotatedTypeMetadata) {
var environment = conditionContext.getEnvironment();
var enabled = environment.getProperty(PROPERTY_NAME, Boolean.class, false);
if (!enabled) {

Check warning on line 27 in cqf-fhir-cr-hapi/src/main/java/org/opencds/cqf/fhir/cr/hapi/config/CrEcrValidateCondition.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a primitive boolean expression here.

See more on https://sonarcloud.io/project/issues?id=cqframework_clinical-reasoning&issues=AZ6o0SbEMshxyy6QgCqD&open=AZ6o0SbEMshxyy6QgCqD&pullRequest=1047
ourLog.info(
"CrEcrValidateCondition not met: '{}' is not set to true. "
+ "Custom FhirValidator will not be created.",
PROPERTY_NAME);
}
return enabled;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
package org.opencds.cqf.fhir.cr.hapi.config;

import ca.uhn.fhir.context.FhirContext;
import ca.uhn.fhir.context.support.DefaultProfileValidationSupport;
import ca.uhn.fhir.jpa.api.dao.DaoRegistry;
import ca.uhn.fhir.jpa.validation.ValidatorResourceFetcher;
import ca.uhn.fhir.parser.IParser;
import ca.uhn.fhir.validation.FhirValidator;
import java.io.IOException;
import java.io.InputStream;
import org.hl7.fhir.common.hapi.validation.support.CommonCodeSystemsTerminologyService;
import org.hl7.fhir.common.hapi.validation.support.InMemoryTerminologyServerValidationSupport;
import org.hl7.fhir.common.hapi.validation.support.PrePopulatedValidationSupport;
import org.hl7.fhir.common.hapi.validation.support.SnapshotGeneratingValidationSupport;
import org.hl7.fhir.common.hapi.validation.support.ValidationSupportChain;
import org.hl7.fhir.common.hapi.validation.validator.FhirInstanceValidator;
import org.hl7.fhir.utilities.npm.NpmPackage;
import org.opencds.cqf.fhir.cr.CrSettings;
import org.opencds.cqf.fhir.utility.repository.NpmRepository;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Conditional;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.Primary;

/**
* Configures a {@link FhirValidator} to be used by HAPI FHIR's built-in {@code $validate} operation.
*
* <p>This configuration is only activated when {@code cr.ecr.validate.enabled=true} is set (or the
* equivalent {@code CR_ECR_VALIDATE_ENABLED=true} environment variable). When disabled, the
* default HAPI FHIR {@code FhirValidator} is used.
*/
@Configuration
@Conditional(CrEcrValidateCondition.class)
public class CrEcrValidateConfig {

private static final Logger logger = LoggerFactory.getLogger(CrEcrValidateConfig.class);

/**
* Builds a {@link FhirInstanceValidator} using a {@link ValidationSupportChain} which contains
* a {@link PrePopulatedValidationSupport} that is populated with the contents of specified
* Npm Packages.
*
* <p>Marked {@link Primary} so it takes precedence when multiple {@link FhirInstanceValidator} beans
* are present in the application context.
*/
@Bean
@Primary
public FhirInstanceValidator crEcrFhirInstanceValidator(
FhirContext fhirContext, DaoRegistry daoRegistry, CrSettings crSettings) {
NpmRepository npmRepository = new NpmRepository(fhirContext, crSettings.getValidatorPackages());
PrePopulatedValidationSupport prePopulatedValidationSupport = loadPrePopulatedValidationSupport(npmRepository);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call chain: loadPrePopulatedValidationSupport then npmRepository.getLoadedPackages() at bean-construction time is problematic. FilesystemPackageCacheManager.loadPackage will attempt a network fetch from the package server when the package isn't cached, and then every StructureDefinition/ValueSet/CodeSystem in every transitive package is parsed into memory (all synchronously on the bean-init thread). This blocks application startup and can hang on a slow/unavailable package server. Worth at least documenting the pre-warm requirement, and ideally moving the load off the startup critical path.

var supportChain = new ValidationSupportChain(
new DefaultProfileValidationSupport(fhirContext),
new CommonCodeSystemsTerminologyService(fhirContext),
new InMemoryTerminologyServerValidationSupport(fhirContext),
new SnapshotGeneratingValidationSupport(fhirContext),
prePopulatedValidationSupport);
var instanceValidator = new FhirInstanceValidator(supportChain);
instanceValidator.setValidatorResourceFetcher(
new ValidatorResourceFetcher(fhirContext, supportChain, daoRegistry));
return instanceValidator;
}

/**
* This {@link FhirValidator} instance loads the above {@link FhirInstanceValidator} module.
* This instance which will conditionally be used by HAPI FHIR's built-in {@code $validate} operation.
*
* <p>Marked {@link Primary} so it takes precedence when multiple {@link FhirValidator} beans
* are present in the application context.
*/
@Bean
@Primary
public FhirValidator crEcrFhirValidator(FhirContext fhirContext, FhirInstanceValidator crEcrFhirInstanceValidator) {
var validator = fhirContext.newValidator().registerValidatorModule(crEcrFhirInstanceValidator);
validator.setValidateAgainstStandardSchema(false);
validator.setValidateAgainstStandardSchematron(false);
return validator;
}

private PrePopulatedValidationSupport loadPrePopulatedValidationSupport(NpmRepository npmRepository) {
PrePopulatedValidationSupport prePopulatedValidationSupport =
new PrePopulatedValidationSupport(npmRepository.fhirContext());
var parser = npmRepository.fhirContext().newJsonParser();
for (var npmPackage : npmRepository.getLoadedPackages()) {
try {
var files = npmPackage.listResources("StructureDefinition", "ValueSet", "CodeSystem");
for (var filename : files) {
logger.info("Loading pre-populated validation support from {}", filename);
addResource(npmPackage, filename, parser, prePopulatedValidationSupport);
}

} catch (IOException e) {
logger.error("Error listing Resources from package {}", npmPackage.id(), e);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NpmRepository.ensurePackagesLoaded() catches all exceptions and leaves loadedPackages as an empty list. Then loadPrePopulatedValidationSupport catches IOException per package and continues. Then addResource catches all exceptions per resource (line 108).

So if hl7.fhir.us.ecr#2.1.2 isn't in the local ~/.fhir cache and can't be downloaded, getLoadedPackages() returns empty, prePopulatedValidationSupport is empty. However, the @primary FhirValidator is still created and installed, replacing the default. Combined with setValidateAgainstStandardSchema(false) / setValidateAgainstStandardSchematron(false), $validate then runs with effectively no profile support and silently stops doing meaningful validation for the lifetime of the server, with only a warning in the log. For a feature whose entire purpose is validation, "fail open and quiet" is probably the wrong default. Consider failing bean creation (or at least surfacing a hard error) when the configured packages don't load.

}
}
return prePopulatedValidationSupport;
}

private void addResource(
NpmPackage npmPackage,
String filename,
IParser parser,
PrePopulatedValidationSupport prePopulatedValidationSupport) {
try (InputStream is = npmPackage.load("package", filename)) {
var resource = parser.parseResource(is);
prePopulatedValidationSupport.addResource(resource);
} catch (Exception e) {
logger.error("Error loading Resource from package {}: {}", npmPackage.id(), filename, e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import org.opencds.cqf.fhir.cr.crmi.R4ReleaseService;
import org.opencds.cqf.fhir.cr.ecr.r4.R4ERSDTransformService;
import org.opencds.cqf.fhir.cr.hapi.common.StringTimePeriodHandler;
import org.opencds.cqf.fhir.cr.hapi.config.CrEcrValidateConfig;
import org.opencds.cqf.fhir.cr.hapi.config.ProviderLoader;
import org.opencds.cqf.fhir.cr.hapi.config.ProviderSelector;
import org.opencds.cqf.fhir.cr.hapi.config.RepositoryConfig;
Expand Down Expand Up @@ -69,7 +70,8 @@
WithdrawOperationConfig.class,
ReviseOperationConfig.class,
ArtifactDiffOperationConfig.class,
CreateChangelogOperationConfig.class
CreateChangelogOperationConfig.class,
CrEcrValidateConfig.class
})
public class CrR4Config {

Expand Down
17 changes: 17 additions & 0 deletions cqf-fhir-cr/src/main/java/org/opencds/cqf/fhir/cr/CrSettings.java
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
package org.opencds.cqf.fhir.cr;

import java.util.ArrayList;
import java.util.List;
import org.opencds.cqf.fhir.cql.EvaluationSettings;
import org.opencds.cqf.fhir.utility.client.TerminologyServerClientSettings;

public class CrSettings {
private EvaluationSettings evaluationSettings;
private TerminologyServerClientSettings terminologyServerClientSettings;
private List<String[]> validatorPackages;

public static CrSettings getDefault() {
return new CrSettings();
Expand All @@ -14,6 +17,7 @@ public static CrSettings getDefault() {
public CrSettings() {
evaluationSettings = EvaluationSettings.getDefault();
terminologyServerClientSettings = TerminologyServerClientSettings.getDefault();
validatorPackages = new ArrayList<>();
}

public EvaluationSettings getEvaluationSettings() {
Expand Down Expand Up @@ -42,4 +46,17 @@ public CrSettings withTerminologyServerClientSettings(
public void setTerminologyServerClientSettings(TerminologyServerClientSettings terminologyServerClientSettings) {
this.terminologyServerClientSettings = terminologyServerClientSettings;
}

public List<String[]> getValidatorPackages() {
return validatorPackages;
}

public CrSettings withValidatorPackage(String[] validatorPackage) {
this.validatorPackages.add(validatorPackage);
return this;
}

public void setValidatorPackages(List<String[]> validatorPackages) {
this.validatorPackages = validatorPackages;
}
}
Loading