Skip to content

Conversation

@jkiddo
Copy link
Contributor

@jkiddo jkiddo commented Oct 14, 2025

This PR introduces a safety check when installing implementation guides that contains already existing embedded terminology as listed on https://hapifhir.io/hapi-fhir/docs/validation/validation_support_modules.html#commoncodesystemsterminologyservice as shadowing existing embedded terminology may lead to unintended behaviour. For instance, when repackaging implementation guides it can easily go wrong if e.g. bcp is referenced and included as a CodeSystem with not-present (which would be default from https://terminology.hl7.org/CodeSystem-v3-ietf3066.html )

@robogary
Copy link
Contributor

Formatting check succeeded!

@codecov
Copy link

codecov bot commented Oct 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.79%. Comparing base (112cec3) to head (1a03d41).
⚠️ Report is 164 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #7298      +/-   ##
============================================
+ Coverage     83.41%   83.79%   +0.37%     
- Complexity    29193    30128     +935     
============================================
  Files          1843     1833      -10     
  Lines        113422   116456    +3034     
  Branches      14251    14762     +511     
============================================
+ Hits          94608    97580    +2972     
+ Misses        12730    12585     -145     
- Partials       6084     6291     +207     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment on lines 588 to 614
private final List<String> embeddedCodeSystems = List.of(
CommonCodeSystemsTerminologyService.LANGUAGES_CODESYSTEM_URL,
CommonCodeSystemsTerminologyService.MIMETYPES_CODESYSTEM_URL,
CommonCodeSystemsTerminologyService.CURRENCIES_CODESYSTEM_URL,
CommonCodeSystemsTerminologyService.COUNTRIES_CODESYSTEM_URL,
CommonCodeSystemsTerminologyService.UCUM_CODESYSTEM_URL,
CommonCodeSystemsTerminologyService.USPS_CODESYSTEM_URL);

private final List<String> embeddedValueSets = List.of(
CommonCodeSystemsTerminologyService.LANGUAGES_VALUESET_URL,
CommonCodeSystemsTerminologyService.MIMETYPES_VALUESET_URL,
CommonCodeSystemsTerminologyService.CURRENCIES_VALUESET_URL,
CommonCodeSystemsTerminologyService.UCUM_VALUESET_URL,
CommonCodeSystemsTerminologyService.ALL_LANGUAGES_VALUESET_URL,
CommonCodeSystemsTerminologyService.USPS_VALUESET_URL);

private boolean isEmbeddedValueSet(IBaseResource theResource) {
org.hl7.fhir.r4.model.ValueSet valueSet = myVersionCanonicalizer.valueSetToCanonical(theResource);
if (!valueSet.hasUrl()) return false;
return embeddedValueSets.contains(valueSet.getUrl());
}

private boolean isEmbeddedCodeSystem(IBaseResource theResource) {
org.hl7.fhir.r4.model.CodeSystem codeSystem = myVersionCanonicalizer.codeSystemToCanonical(theResource);
if (!codeSystem.hasUrl()) return false;
return embeddedCodeSystems.contains(codeSystem.getUrl());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

CommonCodeSystemsTerminologyService already contains the methods isCodeSystemSupported(theValidationSupportContext, theSystem), isValueSetSupported(theValidationSupportContext, theValueSetUrl), getValueSetUrl(theFhirContext, theValueSet) and getCodeSystemUrl(theFhirContext, theCodeSystem); they could be used to avoid duplicating code and risking desynchronizing embeddedCodeSystems and embeddedValueSets with CommonCodeSystemsTerminologyService

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch ... I'll rework it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed with 3b878ff

@tadgh tadgh enabled auto-merge (squash) October 29, 2025 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants