Repository navigation
fix(validator): treat an empty nested invariant result as a pass - #2228
Merged
Merged
Conversation
Constraints attached below the resource root were spliced into `path.all(expr)`. FHIRPath `all()` counts a per-node empty result as false, so R4/R4B `ref-1` (which has no `reference.exists()` guard) failed on every display-only or identifier-only Reference. Evaluate nested constraints in two phases instead: evaluate the relative focus path to get the nodes, then evaluate the invariant AST once per node and run each result through the same `coerce` used for root constraints. Any node failed -> Failed; else any not evaluable -> NotEvaluable; else Passed (no nodes -> Passed). This keeps the NotEvaluable warning for non-boolean nested results, which a pure FHIRPath rewrite such as `where((e) = false).empty()` would drop.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
FHIR/fhir-test-cases merged oldrobot's PRs #316-#320 on 2026-10-23 and the "Test FHIRPath" gate (which runs the upstream corpus) went red on every PR with 10 undeclared failures. Fix the engine rather than declare them: - Delimited identifiers: `none_of("`")` consumed the backslash before the escape parser ran, so Patient.`name` never resolved. - unescape('json'): decode \uXXXX (pairing UTF-16 surrogates) and \/. - Indexer: a negative index is out of range, so the result is empty instead of an error. - skip({}) / take({}): an empty count returns empty. - DateTime + / - calendar duration: accept hour- and minute-precision values with a timezone (@2015-02-04T10Z, @2015-02-04T10:00Z) and keep the input precision in the result, so `= @2015-02-04T11:00Z` compares. Vendor the new cases into the local r4/r5 corpora. testLessThan28 then exposed that extension(url) on a FHIR primitive only worked through a hard-coded fabricated birthTime extension; read the primitive's own extensions instead and drop the fabrication. The parse-all test now requires invalid="syntax" expressions to be rejected. (cherry picked from commit 7e2e6de)
mauripunzueta
marked this pull request as ready for review
October 27, 2026 13:01
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
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 #2171.
Problem
FhirPathConstraintEvaluatorwrapped constraints attached below the resource root aspath.all(expr). FHIRPathall()treats a per-node empty result asfalse, but FHIR invariant semantics treat empty as a pass (andcoercealready did that for root-attached constraints). As a result, R4/R4Bref-1(which has noreference.exists()guard) fired on every display-only or identifier-only Reference, so those resources were rejected underHFS_VALIDATION_MODE=enforce.Fix
This is option 2 from the issue: nested constraints are now evaluated in two phases, with no string splicing.
contact[0],subject, …) against the resource to get the focused nodes.evaluate(&invariant, ctx, Some(node)). This is the same callall()makes internally. Each result goes through the existingcoerce.The per-node results combine like this: if any node failed, the outcome is
Failed. Otherwise, if any node was not evaluable, it isNotEvaluable. Otherwise it isPassed, which also covers the case where the path matches no nodes. Root and nested constraints now share one coercion rule. A nested invariant that returns a non-boolean still surfaces as aNotEvaluablewarning; thewhere((e) = false).empty()rewrite would have dropped it silently. Both the focus paths and the invariants are cached as parsed ASTs.%contextstill resolves to the root, as before (doc note inlib.rsupdated).Tests
tests/fhirpath_constraint_tests.rs::nested_invariant_evaluating_to_empty_passes:ref-1onObservation.subjectfor display-only, identifier-only, literal, and local-with-contained references (no issue), and local without contained (fires). Verified failing onmainat the display-only case and passing with this change.fhirpath_effects.rs: nested empty → pass, nested false → fail, nested non-boolean →NotEvaluable, absent focus → pass, multi-nodename[0].givenrequires every node.real_pat1_invariant_fires_on_detail_less_contactandabsent_nodes_pass_invariantspass unchanged.cargo test -p helios-fhir-validator --features fhirpathpasses locally. Clippy reports nothing forhelios-fhir-validator.