Add Conditional Custom eCR FhirValidator Configuration#1047
Conversation
|
Formatting check succeeded! |
Validator support chain loads US eCR package via Npm
Chris0296
force-pushed
the
cor-validate-config
branch
from
6daac71 to
a78193b
Compare
|
c-schuler
left a comment
c-schuler
left a comment
There was a problem hiding this comment.
Looking good so far. A few suggestions and we definitely need more testing. Nice work!
| } | ||
|
|
||
| } catch (IOException e) { | ||
| logger.error("Error listing Resources from package {}", npmPackage.id(), e); |
There was a problem hiding this comment.
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.
| public FhirInstanceValidator crEcrFhirInstanceValidator( | ||
| FhirContext fhirContext, DaoRegistry daoRegistry, CrSettings crSettings) { | ||
| NpmRepository npmRepository = new NpmRepository(fhirContext, crSettings.getValidatorPackages()); | ||
| PrePopulatedValidationSupport prePopulatedValidationSupport = loadPrePopulatedValidationSupport(npmRepository); |
There was a problem hiding this comment.
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.
| .withEvaluationSettings(evaluationSettings) | ||
| .withTerminologyServerClientSettings(terminologyServerClientSettings); | ||
| .withTerminologyServerClientSettings(terminologyServerClientSettings) | ||
| .withValidatorPackage(new String[] {"hl7.fhir.us.ecr", "2.1.2"}); |
There was a problem hiding this comment.
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.
2 participants
Part of: DBCG/aphl-vsm#604
Configuration of custom FhirValidator Bean which is conditionally loaded, based on the property
cr.ecr.validate.enabledValidator support chain loads prepopulated validation support via Npm, with the packages to load being determined by the CrSettings
validatorPackagesfield.This FhirValidator is marked
@Primary, so when it is conditionally created - it becomes the FhirValidator in use for FHIRs $validate operation.