Fix faulty invariant FHIRPath expressions (+ regression fixtures)#4122
Open
jmandel wants to merge 1 commit into
Open
Fix faulty invariant FHIRPath expressions (+ regression fixtures)#4122jmandel wants to merge 1 commit into
jmandel wants to merge 1 commit into
Conversation
Corrects invariant expressions whose FHIRPath does not match the rule's stated
intent under the validator's evaluation semantics (a constraint passes only if
its expression converts to boolean true; false, empty {}, and runtime errors
all fail). Each behavioral fix ships a new invariant-test fixture that flips
verdict between the old and new expression.
Fixes, by class:
- Single-value operators (=, !=, in) on repeating elements: bdl-14, bdl-16,
csd-3, csd-6, pld-3
- count()/exists() aggregated across all repeats instead of per item:
isl-7, isl-8, isl-10, isl-11
- xor used for "mutually exclusive" (wrongly forbids "neither"): mrp-3, mrp-4
- Unanchored matches() (partial match): exp-2, que-16, qrs-3; eld-12 (missing ':')
- Tree-walk that skips a level: csd-1, qrs-2
- .exists() where a value is required (extension-only primitive): eld-21
- Representation pitfalls: cnt-3 (5.0 toString), bdl-7 (delimiterless key)
- Inverted/over-strict logic: drt-1
- Clause referencing a non-existent element: pld-6, adf-2
- Obsolete element names (reasonCode/reasonReference -> reason): inv-2
- Wrong literal token ('uri' -> 'url'): que-5, que-5b
- Antecedent guards the wrong element (series vs series.instance): ist-4
- Human text corrected, expression already correct: bdl-3b, docRef-2
Deferred (identified, not included): dgr-1 (needs resolve()-context
verification), cmp-3 (needs structural re-anchor from .section to the
resource), tst-1/tst-2/tst-3 (3-way chained xor; authored in a binary
spreadsheet, not editable here).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Fix faulty invariant FHIRPath expressions (+ regression fixtures)
What this does
Corrects invariant (
constraint) FHIRPath expressions whose evaluation does not match therule's stated
humantext, and adds a new invariant-test fixture for each so the behavior islocked in.
Key evaluation fact this relies on: the validator passes a constraint only when its FHIRPath
converts to boolean
true;false, an empty result{}, and a thrown runtime error allcount as fail. Several of the fixes (and several non-fixes, see below) turn on this.
How each fix was verified
Every change was checked against the real validator (
org.hl7.fhir.core6.9.8, R6) and/or aconvertToBooleanwrapper that reproduces the validator's pass/fail mapping. Each new fixture'sinstance was evaluated under both the old and the new expression and the verdict flips in
the intended direction (a
.failfixture that the old expression wrongly passed; a.passfixture the old expression wrongly failed). Abbreviations below: FP = false-positive (rejects
valid data), FN = false-negative (accepts invalid data).
Fixes included (≈29 invariants, 15 files) + new fixtures
Single-value operators (
=/!=/in) on repeating elements=/!=on different-length collections returnfalse;inthrows on a multi-item operand.bdl-14entry.request.all(method != 'PATCH')bdl-14.f2.fail.jsonbdl-16issues.issue.all(severity = …)bdl-16.p2.pass.jsoncsd-3property.where(code='parent' or code='child').exists()csd-3.f3.fail.jsoncsd-6…where(code='alternateCode' and value=%sc).exists()csd-6.p2.pass.jsonpld-3…goalId.all($this in %context.goal.id)pld-3.p1.pass.jsoncount()/exists()aggregated across all repeats instead of per itemisl-7/isl-8/isl-11instance.imageRegion2D.all(coordinate.count() mod 2 = 0)(andmod 3, waveform)isl-7.f2,isl-8.f2,isl-11.f2.fail.jsonisl-10instance.all(...)isl-10.p1.pass.jsonxorused for "mutually exclusive"mrp-3/mrp-4countnorcountQuantityis rejected →(count.exists() and countQuantity.exists()).not()mrp-3.p1,mrp-4.p1.pass.jsonUnanchored
matches()(partial match)que-16/qrs-3"double space"passes → anchor with^…$que-16.f1,qrs-3.f1.fail.jsonexp-2Expression.namelike"9-bad name"passes → anchorexp-2.f1.fail.jsoneld-12startsWith('https')(no colon) accepts'httpsfoo'→startsWith('https:')eld-12.f1.fail.jsonTree-walk that skips a level
csd-1concept.code.combine(concept.combine(concept.descendants()).concept.code).isDistinct()csd-1.f2.fail.jsonqrs-2repeat()excludes the seed) → also check the item's own childrenqrs-2.f1.fail.jsonOther
eld-21expression.exists()is true for an extension-only primitive →expression.hasValue()eld-21.f1.fail.jsoncnt-35.0is rejected (toString()keeps the.) →value = value.round()cnt-3.p1.pass.jsonbdl-7fullUrl & versionIdkey collides → insert'|'bdl-7.p1.pass.jsondrt-1code→value) and a code-only Duration is rejected →(value.exists() implies code.exists()) and (system.exists() implies system = %ucum)drt-1.f1.fail+drt-1.p1.pass.jsonpld-6/adf-2artifactelement; bothresource+resourceReference(allowed) is rejected →… implies (resource.exists() or resourceReference.exists())pld-6.p1,adf-2.p1.pass.jsonque-5/que-5b'uri'but the item-type code is'url'→ use'url'que-5.p1,que-5b.p1.pass.jsonist-4series.exists()instead ofseries.instance.exists()→ a summary-only study is rejectedist-4.p1.pass.jsonText-only (the FHIRPath was already correct — only the
humantext was wrong; no fixture)bdl-3b(Bundle): text said "request or response" but the spec/expression require both → text fixed to "and".docRef-2(DocumentReference): text said "context is not present" but the expression (matching its siblingdocRef-1and its own pass fixture) means "context is not an encounter" → text fixed.Element-rename only (no fixture possible)
inv-2(Event pattern): referenced the obsoletereasonCode/reasonReference(R6 merged these intoreason) so it could never fire → updated toreason.exists().not(). It's defined only on the abstract Event pattern and isn't applied to concrete resources, so there is no instance that can exercise it.Left out, and why
Earlier-suspected, but verified CORRECT (no change) — ~16 invariants
An earlier reasoning pass (before validating) wrongly assumed an empty FHIRPath result meant
"pass." The validator treats empty as fail, so the following are actually correct and were
not touched:
*" family:eld-3,md-1,opd-9,smp-3(a non-numericmax→toInteger()empty → fails, which is right).que-10,que-13,que-17,sdf-1,vsd-9,opd-2,opd-3,sdf-21,sdf-30(the empty result lands on the violation, so it fails correctly; valid edgecases produce a real
true).obd-0(the Java engine supports.valueon a primitive),eld-11(special-cased in Java),apd-1(fires correctly for resolvableformOf; only the universalresolve()limitationremains).
Confirmed issues deferred (need work this branch can't safely do)
dgr-1(DiagnosticReport): only walks top-levelsection.entry, missing nested sub-sectionentries. The traversal fix (
repeat(section)) is verified in isolation, but the full invariantdepends on
resolve()/%resource/hasMembermembership, which can't be evaluated outside fullvalidation — so I couldn't construct a fixture whose end-to-end verdict provably flips. Reverted
here pending validation-context confirmation.
cmp-3(Composition): the rule is anchored onComposition.section, so it can't fail whenreached and never reaches its real violation (attester + no text + no section). The fix is to
re-anchor the constraint to the resource root (a structural move, not just an expression
edit) — left for a deliberate change.
tst-1/tst-2/tst-3(TestScript): 3-way chainedxoris true for an odd count, so"all three present" passes;
tst-3'sor-of-empties only catches all-three. These are authoredin a binary
.xlsxspreadsheet that isn't editable in this workflow; they need fixing in theTestScript spreadsheet source.
Notes
isl-7/8/10/11,pld-6,adf-2,cnt-3,drt-1,eld-12,eld-21) arenewer than, or are datatype invariants not surfaced by, the validator's loaded core package, so
they were verified via the validator's FHIRPath engine + the
convertToBooleanrule ratherthan a full package validation. Datatype-invariant fixtures are hosted on a carrier resource
(
cnt-3/drt-1underParameters,exp-2underPlanDefinition,eld-*underStructureDefinition), following the existing convention (e.g.qty-4underObservation).csd-1's gap is also masked at full validation by a separate built-in duplicate-code check, butthe invariant's own FHIRPath is corrected regardless.
🤖 Generated with Claude Code