Fix $validate fatal 5003 on Bundles with resource-less entries - #5791
Conversation
529a6d3 to
5768e2b
Compare
Ignixa.Extensions.FirelySdk5 0.0.163 declared a floor of Hl7.Fhir.Base 5.13.1. The version-specific *.Api projects have no direct PackageReference to Hl7.Fhir.Base, so Central Package Management has nothing to pin there and transitive highest-wins floated Base up to 5.13.1 for those projects and the *.Web artifacts built from them - defeating the Hl7FhirVersion=5.11.4 pin in Directory.Packages.props. Base 5.11.5 and later contain an unguarded .First() in ScopedNode.BundledResources() that throws on a Bundle entry with no 'resource' child (e.g. a transaction DELETE entry). The legacy validator surfaces that as a fatal internal OperationOutcome issue inside an HTTP 200 $validate response, so it is invisible to status-code and exception telemetry. Ignixa 0.6.74 lowers its declared floor to 5.11.4, so the existing pin becomes effective again. The 0.6.x line renames a few serialization members, so IgnixaImportResourceParser is updated to match: Meta.LastUpdated -> Meta.LastUpdatedOffset (Meta auto-materializes) ResourceJsonNode.MutableNode -> ToSourceNavigator().Meta<JsonNode>() ReferenceJsonNode -> read "reference" from the raw JsonObject Verified that Microsoft.Health.Fhir.R4.Web and the R4 unit test project both resolve Hl7.Fhir.Base 5.11.4, and that the import parser parity suite still passes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd62eba7-d2c6-46de-8a39-85defbbb4b5a
Guards against Hl7.Fhir.Base regressing to a version whose ScopedNode.BundledResources() calls .First()/.Single() on an entry's 'resource' child without guarding entries that have none, such as a transaction DELETE entry that carries only a request. Because $validate returns HTTP 200 with the fatal outcome in the response body, this failure is invisible to status-code and exception telemetry, so a regression test is the only practical guard. The test asserts only the absence of that catastrophic issue; ordinary validation warnings and errors remain allowed so the test does not become brittle. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd62eba7-d2c6-46de-8a39-85defbbb4b5a
5768e2b to
21700bc
Compare
| // Asser | ||
| Assert.DoesNotContain("cid-0", internalValidator.Settings.ConstraintsToIgnore ?? []); | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we add a test that covers the issue the customer ran into? A bundle being validated with resource entries that contained references shouldn't report validation errors.
There was a problem hiding this comment.
I believe it was this { "request": { "method": "DELETE", "url": "Observation/no-such-id" } } hitting the unguarded entry.Children("resource").First().ToScopedNode() on https://github.com/FirelyTeam/firely-net-sdk/pull/3099/changes#diff-872a636c2e24971aa1d68c2aac5ba903d2b34b3f82f738ce74e534bbd8a86ec5R238-R239
There was a problem hiding this comment.
I'll add the breaking payload to the bundle e2e tests too
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5791 +/- ##
==========================================
- Coverage 78.93% 78.92% -0.02%
==========================================
Files 1016 1016
Lines 36940 36910 -30
Branches 5619 5616 -3
==========================================
- Hits 29158 29130 -28
+ Misses 6403 6398 -5
- Partials 1379 1382 +3 🚀 New features to boost your workflow:
|
Complements the ProfileValidator unit test by exercising the full HTTP path. The defect surfaces as a fatal issue inside an HTTP 200 response body, so an end-to-end assertion is what actually reflects what a caller observes. Posts a transaction Bundle to Bundle/$validate containing a Patient with a managingOrganization reference, the referenced Organization, and a DELETE entry that carries only a request and no resource. Asserts that the returned OperationOutcome contains no "Internal logic failure" issue and no fatal issue with details code 5003; ordinary validation warnings and errors remain allowed. Verified that this exact payload produces "[Fatal/Exception] Internal logic failure: Sequence contains no elements" when run against Hl7.Fhir.Base 5.13.1, and passes at 5.11.4. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd62eba7-d2c6-46de-8a39-85defbbb4b5a
Extracts the breaking payload into a shared sample file, TestFiles/Normative/Bundle-TransactionWithResourcelessEntry.json, so it is reusable and follows the existing bundle-sample convention rather than being inlined as an escaped string in a test method. The Normative folder is the fallback for every FHIR version, so the sample serves STU3, R4, R4B and R5. Relocates the test from ValidateTests to BundleEdgeCaseTests, which is where bundle-shape edge cases live and which carries the Bundle category trait. Verified by loading the sample through Samples.GetJson and running it against the real ProfileValidator: it reproduces "[Fatal/Exception] Internal logic failure: Sequence contains no elements" on Hl7.Fhir.Base 5.13.1, and passes on 5.11.4. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd62eba7-d2c6-46de-8a39-85defbbb4b5a
BundleEdgeCaseTests is declared with Format.All, so moving the test into that class also generated an Xml variant. The test posts the sample as a raw JSON string, so the Xml argument set sent an Xml content type and failed with "System.FormatException : Invalid Xml encountered. Details: Data at the root level is invalid. Line 1, position 1." HttpIntegrationFixtureArgumentSetsAttribute supports a method-level override, already used elsewhere (for example in BulkDeleteTests and BulkUpdateTests), so this narrows only this test rather than the whole class. Verified via test discovery that this test now yields only the (CosmosDb, Json) and (SqlServer, Json) variants, while neighbouring tests in the class still generate all four Json and Xml combinations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: bd62eba7-d2c6-46de-8a39-85defbbb4b5a
Description
$validatesilently fails for any Bundle containing an entry with noresourcechild — e.g. a transactionDELETEentry that carries only arequest— when the Bundle also contains aReference. It returns HTTP 200 with a fatal issue in the body, so the failure is invisible to status-code and exception telemetry. Observed live: service5.0.22→All OK;5.0.53→ fatal5003.Root cause.
Hl7.Fhir.BasechangedScopedNode.BundledResources()between 5.11.4 and 5.11.5:The
.First()is unguarded, so resolving a reference forces the bundle cache to build and throwsInvalidOperationException: Sequence contains no elements. The legacy validator (Hl7.Fhir.Validation.Legacy) converts that into a fatalOperationOutcomeissue with details code5003,"Internal logic failure: Sequence contains no elements".Why the pin didn't protect us.
Directory.Packages.propsalready setHl7FhirVersion=5.11.4, but that was only partially effective.Microsoft.Health.Fhir.Coreand the sharedMicrosoft.Health.Fhir.Apicarry a directPackageReferencetoHl7.Fhir.Base, so CPM pinned them. The version-specificR4/R4B/R5/Stu3.Apiprojects do not — they onlyProjectReferenceMicrosoft.Health.Fhir.Ignixa, which pulledIgnixa.Extensions.FirelySdk5→Hl7.Fhir.Base >= 5.13.1. With no direct reference there is nothing for CPM to pin, so transitive highest-wins applied and a singleHl7.Fhir.Base.dllat 5.13.1 landed in the output,*.Webshipping artifacts included.(A direct
PackageReferencepins per project, not transitively — adding it only to*.Apiwould still leave each*.Webleaf on 5.13.1. Verified empirically.)The fix.
Ignixa.Extensions.FirelySdk50.6.74 lowers its declared floor to 5.11.4, so the existing pin becomes effective again — noNU1605suppression, no downgrade conflict. The0.6.xline renames three serialization members, soIgnixaImportResourceParseris updated to match:5 files changed:
Directory.Packages.propsIgnixaPackageVersion0.0.163→0.6.74IgnixaImportResourceParser.csProfileValidatorTests.csBundle-TransactionWithResourcelessEntry.jsonBundleEdgeCaseTests.csRelated issues
Addresses AB#206224.
Testing
Two regression tests, at both layers.
Unit —
ProfileValidatorTests.GivenABundleWithAnEntryThatHasNoResource_WhenValidating_ThenNoInternalLogicFailureIsReported, driving the existing public seamProfileValidator.TryValidate(ITypedElement, string).E2E —
BundleEdgeCaseTests.GivenABundleWithAnEntryThatHasNoResource_WhenValidated_ThenNoInternalLogicFailureIsReturned, POSTing the Bundle toBundle/$validatethrough the real HTTP pipeline. This layer matters specifically because the defect surfaces inside a 200 response body — an E2E assertion is what actually reflects what a caller observes. It lives with the other bundle-shape edge cases and carries theBundlecategory trait.The breaking payload is checked in as a reusable sample,
TestFiles/Normative/Bundle-TransactionWithResourcelessEntry.json, following the existing bundle-sample convention rather than being inlined as an escaped string.Normativeis the fallback folder for every FHIR version, so one file serves STU3, R4, R4B and R5.Both tests use the same shape: a
Patientcarrying amanagingOrganizationreference (which forces the bundle cache to build), the referencedOrganization, and an entry with only arequest(DELETE) and noresource.Both assert the absence of the crash, not the absence of all issues — ordinary validation warnings and errors remain allowed, so neither test becomes brittle. Both live in shared test projects and compile for STU3, R4, R4B and R5.
Proven to fail on the defect and pass on the fix:
Hl7.Fhir.Base/5.13.1): the unit test fails on all four FHIR versions —Assert.DoesNotContain() Failure: Filter matched in collection. The E2E sample file was verified by loading it throughSamples.GetJsonand running it against the realProfileValidatorpinned to 5.13.1, producing exactly:Microsoft.Health.Fhir.R4.Weband the R4 test project both resolveHl7.Fhir.Base/5.11.4, output dll5.11.4+3605c91f, and:ImportResourceParserParityTestsMicrosoft.Health.Fhir.R4.Core.UnitTests(full)Microsoft.Health.Fhir.R4B.Api.UnitTests(full)The E2E test itself requires a deployed server (or a data store for the in-proc fixture), so it runs in CI rather than locally — but its sample file and assertion were verified end-to-end against the validator in both the defective and fixed configurations, as shown above.
The parser migration is covered by the existing parity suite, which asserts equivalence with the Firely parser — including the missing-
metacase (GivenMissingMetaAndInvalidVersion_WhenParsedOnIncrementalLoad_ThenBothProvidersResetVersion), which is what makes dropping theMetaJsonNodeinitialisation safe.FHIR Team Checklist
Semver Change (docs)
Patch
Notes
.First()/.Single()is still present upstream inFirelyTeam/firely-net-sdkdevelop, so upgrading forward does not help — the test guards against drifting back onto an affected version.Hl7.Fhir.Base≤ 5.11.4 is safe.v5.11.5,v5.11.7,v5.12.0andv5.13.xall contain the unguarded call.0.6.xadoption in Implement incremental Ignixa SDK adoption and FHIRPath seam #5776; this PR is the minimal subset needed to ship the fix tomainnow, and makes the same functional edits toIgnixaImportResourceParseras that branch.