fix(fhirpath): remove the default terminology server (#217)#262
Draft
mauripunzueta wants to merge 2 commits into
Draft
fix(fhirpath): remove the default terminology server (#217)#262mauripunzueta wants to merge 2 commits into
mauripunzueta wants to merge 2 commits into
Conversation
The FHIRPath evaluator fell back to the public HL7 test server tx.fhir.org whenever no terminology server was configured. Terminology operations carry codes taken from the resource under evaluation -- `%terminologies.validateVS( ..., $this.gender)` sends a field of a Patient -- so an unconfigured HFS deployment silently shipped patient-derived codes and display strings to a third party. An eprintln! to stderr was the only control. There is now no default. Resolution is the explicitly configured URL, then FHIRPATH_TERMINOLOGY_SERVER, then nothing: %terminologies.* and memberOf() fail with an error naming the variable to set, rather than calling a host the operator never chose. This matches the policy the REST layer already applies, where an unconfigured :in/:not-in search modifier returns 501 Not Implemented. Pointing the default at hts.heliossoftware.com was considered and rejected: it would relocate the egress rather than remove it, and HTS selects its FHIR version from cfg! at compile time, so a single root URL cannot serve R4, R4B, R5 and R6 clients correctly. Also: - Drop the per-version URL match. A terminology base URL is opaque, so no /r4 or /r5 segment is appended (HTS serves operations at the root). This also retires the indefensible "R6 may use R5 server for now" arm. - Stop building and discarding a TerminologyClient when %terminologies is merely named; only invoking an operation needs a server. - Give the terminology HTTP client a 30s timeout. Client::new() applies none, so an unresponsive server hung the evaluating thread indefinitely. - Serve the R5 suite's mode="tx" tests from an in-process stub. txTest01 and txTest02 were asserted, not skipped (the txTest02 guard tests for `translate(`, which its validateVS expression does not contain), so `cargo test` made live calls to tx.fhir.org and went red whenever that server was down. FHIRPATH_TERMINOLOGY_SERVER still overrides the stub, which is how the expectations get re-validated against a real server.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…rver (#217) Removing the default terminology server silently dropped three conformance tests, because both harnesses relied on the evaluator falling back to tx.fhir.org. The Windows "Test FHIRPath" job runs the official FHIR R5 test cases against fhirpath-server, which is started with no terminology server configured. With no default, txTest01-03 now error, and the runner reports an error as Assert.Inconclusive -> SKIPPED. The job stayed green while quietly losing the only end-to-end coverage of %terminologies. Point that server at our own HTS (per #183) rather than the public tx.fhir.org. Because the runner downgrades errors to skips, an HTS outage cannot turn CI red. Un-skip txTest03 in the Rust R5 suite. It was skipped because tx.fhir.org answered ConceptMap/cm-address-use-v2 incorrectly. HTS answers it correctly -- verified against the live server, which returns concept.code = "H", exactly what the test expects -- so the stub now serves $translate with HTS's actual response shape and the test asserts again. The expectation is re-derived against the source we now point people at, which is what #217 asked for.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #217.
The decision
Issue #217 asked us to choose between three options for the FHIRPath evaluator's hardcoded default terminology server (
evaluator.rs:636-661). This PR implements "configurable with no network default", and explicitly rejects migrating the default tohts.heliossoftware.com.Two reasons the HTS option does not work:
crates/hts/src/operations/metadata.rs:334picksfhirVersionfromcfg!(feature = ...)at compile time, andtranslate.rs/expand.rsswitch response shapes (relationshipvsequivalence) the same way. One root URL serves exactly one FHIR version per build, so it cannot be a correct default for R4/R4B/R5/R6 clients simultaneously.Why this is a privacy fix, not a config change
Terminology operations send codes drawn from the resource under evaluation. The shipped R5 conformance test proves the shape:
txTest02evaluates%terminologies.validateVS(..., $this.gender)over a Patient.validate_vstransmitssystem,codeanddisplay. In production the same path carries diagnosis and observation codes. An unconfigured HFS deployment silently sent all of that to a public test server with no BAA and no SLA — and aneprintln!to stderr was the only control, which in a container is interleaved with request logs, fires no alert, and appears in no metric. The egress also bypasseshelios-auditentirely.This PR makes the unconfigured path fail closed, consistent with policy the REST layer already applies:
crates/rest/src/handlers/search.rs:216already returns501 Not Implementedfor an unconfigured:in/:not-insearch modifier, with a comment rejecting a silent fallthrough "which returns misleading results."Changes
get_terminology_server_url()returnsOption<String>. Explicit setting →FHIRPATH_TERMINOLOGY_SERVER→None. The per-FHIR-versionmatchis deleted, so no/r4or/r5path segment is appended to a configured base URL (HTS serves operations at the root). This also retires the"R5" | "R6" => .../r5/arm.TerminologyFunctions::new()is now fallible. It is the single choke point for every terminology op, so one error covers%terminologies.*andmemberOf(). The message names the variable to set (FHIRPATH_TERMINOLOGY_SERVER,--terminology-server,HFS_TERMINOLOGY_SERVER,SOF_TERMINOLOGY_SERVER).%terminologiesno longer requires a server. A deadlet _terminology = TerminologyFunctions::new(context);built and discarded a client merely for referencing the variable; only invoking an operation resolves the server now.Client::new()applies none, so an unresponsive terminology server hung the evaluating thread indefinitely.crates/fhirpath/README.md,crates/hts/README.md, the CLI help and the server'slong_about. Note the HTS README claimed these features "fall back to empty results orfalse" without a server — that was already untrue for search (501) and is now accurate for both.Tests
test_r5_test_suitewas making live network calls to tx.fhir.org on every run.txTest01(expand) andtxTest02(validateVS) aremode="tx"tests that were asserted, not skipped — thetxTest02guard checksexpression.contains("translate("), which itsvalidateVSexpression does not contain, so it never fired. Withassert_eq!(failed_tests, 0), CI went red whenever that public server was down or rate-limiting.Those tests now run against an in-process
wiremockstub, so no test reachable fromcargo test --workspace --all-featuresmakes an outbound call.FHIRPATH_TERMINOLOGY_SERVERstill overrides the stub, which is how the expectations get re-validated against a real server. The expected values (4 gender codes,result = true) are fixed by the FHIR spec, not by any one server.New
crates/fhirpath/tests/terminology_config_tests.rspins both halves of the contract: unconfigured fails with an actionable error (andmemberOf()errors rather than answering a misleadingfalse), referencing%terminologieswithout calling it needs no server, and a configured server is the one actually called.Behavior change
%terminologies.*andmemberOf()now error instead of silently contactingtx.fhir.org. Anyone relying on the old default sets one environment variable to restore it exactly:export FHIRPATH_TERMINOLOGY_SERVER=https://tx.fhir.org/r4/get_terminology_server_url()is public API and its return type changed (String→Option<String>); the crate is pre-1.0 and both in-repo callers are updated.Not addressed
txTest03($translate) remains skipped — it was already skipped before this change for unrelated reasons, and I did not want to invent a ConceptMap fixture without a way to validate it.TerminologyClientstill constructs a freshreqwest::Clientper call (no connection reuse). Pre-existing; worth a follow-up.memberOf()should return spec-style Empty instead of erroring when unconfigured is a defensible alternative. I chose to error because it matches what already happens when the server is unreachable, and a silentfalsemislabels a valid code as a non-member.Follow-up to #183 / #184.