Server v3.0.0 - #2635
Open
johngrimes wants to merge 176 commits into
Open
Conversation
Add viewReference support, Reference-typed patient/group with spec cardinality, Bundle unwrapping in the resource parameter, and 422 for semantically invalid ViewDefinitions on the run operation. Across the run, export, and sqlquery-run operations, reject an explicitly unsupported _format and the unsupported source parameter rather than silently ignoring them. Add a creation-time start timestamp to the async job for later export-manifest timing.
Support view.viewReference resolution to a stored ViewDefinition with per-part exclusivity and presence checks. Rewrite the completion manifest to the SQL on FHIR shape (exportId, status, echoed clientTrackingId and _format, export timing fields, and one output per view with name and location parts), dropping the Bulk Data-style fields. Return a Parameters kick-off acknowledgement for SQL on FHIR async operations while leaving the Bulk Data kick-off body unchanged.
…ery 422 Declare the SQL on FHIR spec canonical OperationDefinition URLs for the viewdefinition-run, viewdefinition-export, and sqlquery-run operations in the CapabilityStatement, and stop serving the Pathling-authored OperationDefinitions for them. Add a regression test pinning the existing 422 response for an unmappable FHIR column type under _format=fhir on sqlquery-run, and integration coverage for the source and _format rejections across all levels and transports.
Update the admin UI manifest parser to read the spec-shaped output location parts (one or more per output) instead of a single url part, emitting one download entry per file so a view that produced several files lists every file. Add an end-to-end check that an invalid custom ViewDefinition surfaces the 422 OperationOutcome message.
Update the run, export, and sqlquery-run operation docs for the new viewReference support, Reference-typed patient/group, Bundle unwrapping, the spec-shaped export manifest and kick-off acknowledgement, the source and unsupported-format rejections, the 400/404/422 status semantics, and the spec conformance canonicals. Credit the additional author on the significantly changed sqlquery providers.
Align the admin UI export end-to-end mocks with the new completion manifest: output download files are carried in location parts, and the non-spec transactionTime and requiresAccessToken fields are removed, so the card-listing tests exercise the location-reading parser.
Strip media-type parameters (e.g. text/csv;charset=utf-8) before matching an explicit _format value across the run, export, and sqlquery-run strict parsers, so a supported media type with parameters is treated as that format rather than rejected with 400, matching the spec's content-negotiation edge case.
The strict SQL validator's expression allow-list omitted the named and positional parameter placeholder expressions, so any parameterised query (using ":name") failed static validation. This blocked runtime parameter binding for $sqlquery-run even though the binding machinery was already present. Add NamedParameter and PosParameter to the allow-list. They are leaf, unevaluable value placeholders bound at execution time, so they carry no code-execution risk.
Implement the SQL on FHIR $sqlquery-export operation, the asynchronous counterpart to $sqlquery-run, at the system, type, and instance levels. Clients can run one or more SQL queries against materialised ViewDefinition tables in the background and download each result as files, following the FHIR Asynchronous Request Pattern. Each query produces one output. Table sources can be supplied at request time, inline or by reference, in addition to being read from server storage. Supported formats are NDJSON (default), CSV, and Parquet, with patient, group, and _since filtering. A failed query fails the whole export. To avoid duplication, extract the shared asynchronous-export machinery into a new operations/export package (a generic completion-manifest builder, a file writer, and a filtered data-source builder) reused by both export operations, and refactor $sqlquery-run onto a shared SqlQueryPipeline that the export also uses. The Admin UI gains an export affordance on the SQL query result card, backed by a reusable export-job card shared with the ViewDefinition export. The operation is declared in the CapabilityStatement against the spec canonical and gated by a new sqlQueryExportEnabled configuration flag. Resolves #2633.
Replace the boolean redirectOnComplete marker on the server's asynchronous machinery with an AsyncPattern enum (STANDARD_ASYNC_PATTERN, BULK_DATA), so the @AsyncSupported annotation reads as a deliberate choice between two named patterns rather than the presence or absence of one behaviour. Correct the Javadoc that misattributed the redirect-based completion to a "SQL on FHIR unify-async" specification, citing the HL7 Asynchronous Interaction Request Pattern instead. Behaviour is unchanged; the existing async tests pass with only their identifiers updated.
isAnyExportEnabled now also covers the SQL on FHIR asynchronous export operations, so the all-exports-disabled case must also disable the viewdefinition-export and sqlquery-export flags, both of which default to enabled.
The SQL on FHIR export integration tests added to the server module push the full build-and-test suite past the previous 30-minute job limit, causing the run to be cancelled before completion. Raise the timeout on the test, pre-release, and release server workflows so the suite has room to finish.
The disabled-operation assertion called doesNotContain on its own, which passes vacuously if the operation list is empty. Assert the still-enabled sqlquery-run operation is present first, so the test proves the list is populated and only sqlquery-export was dropped.
johngrimes
added
release
Resolves twelve Netty advisories reported against 4.1.133.Final, spanning HTTP/2 denial-of-service, TLS hostname verification bypass, and DNS resolver issues. All are fixed within the 4.1.x line at 4.1.135.Final, so no move to the 4.2.x series is required. The Netty BOM remains imported ahead of Spring Boot's so the pinned version continues to win.
Addresses CVE-2026-55471, an XML external entity vulnerability in XsltUtilities.saxonTransform via an unhardened Saxon TransformerFactory. HAPI FHIR 8.10.0 still bundles 6.9.4.1, so the existing override of org.hl7.fhir.r4 and org.hl7.fhir.utilities is raised from 6.9.7 to the patched 6.9.10.
Resolves a batch of react-router security advisories by moving to the latest 7.x release. None of the vulnerable code paths (RSC, SSR, data routers, the __manifest endpoint) are reachable from the admin UI, which uses declarative routing only, but upgrading is a low-risk minor bump that clears the findings and lets the react-router suppressions be removed from the Trivy ignore list.
Restore the server image's ability to run as either the FHIR server or a Spark Kubernetes executor, selected by the container's first argument. Spark on Kubernetes launches executor pods with the single argument "executor" and no command override, so the role selection lives in the image entrypoint. A Bash dry-run test covers the role-selection logic without Docker, a network, or a cluster.
Set the dispatching entrypoint as the image's entry behaviour in both the docker and dockerPreRelease profiles, copy it into the image via Jib extraDirectories with executable permissions, and remove the now-dead jvmFlags (Jib ignores them once an explicit entrypoint is set). Run the dispatch test during the test phase of these profiles, gated by skipTests.
The citation pointed to the Spark 4.0.0 entrypoint; the runtime is Spark 4.0.2, whose executor branch is identical.
Restore the dual-role server image for Spark on Kubernetes
The Varnish frontend cache previously inherited Varnish's default 60s first-byte timeout, which caused long-running synchronous Pathling queries to fail with a 503 before the server could respond. Expose the timeout as a PATHLING_FIRST_BYTE_TIMEOUT environment variable, substituted into the backend definition, and surface it through the Helm chart as pathlingCache.firstByteTimeout (defaulting to the previous 60s).
Remove the SQL query row cap and wall-clock timeout
The asynchronous export operations accept a narrower set of output formats than the SQL on FHIR canonical they declare, so a client had no way to discover the constraint other than by receiving a 400. Rather than implement the missing formats, state the supported set in the documentation of the two operation entries. This keeps the spec canonicals in place as the declared definition. Resolves #2686
The module documents five quality checks but CI ran only three of them. Unit tests ran without coverage, so the 80% thresholds were never applied, and the duplication check did not run at all. A change could therefore erode coverage or raise duplication past its threshold while still showing a green build. Both gates were confirmed to fail on a deliberate breach before being switched on. The thresholds themselves are unchanged.
Job failures carry a FHIR OperationOutcome whose issues each have their own severity, which the shared callout could not express. Adds a second, mutually exclusive props form that renders one badged row per issue, and a plain derivation module so that turning a failure into displayable lines can be tested without mounting a component. Nothing consumes the new form yet.
A failed view run read "View run failed: View run failed: 500 - Query failed", because the card prefixed a message that already named the operation. Resolves #2688.
Two end-to-end tests established that no job list request is issued by sleeping for eleven seconds each, to outlast the idle refresh interval. They were by a wide margin the two slowest tests in the suite. Both now drive Playwright's clock instead, advancing thirty seconds of fake time and so proving strictly more than the sleeps did, in under a second. Because both assert a negative, a positive control accompanies them: it would fail if the fake clock ever stopped driving the refetch, which would otherwise let the two assertions decay into tautologies. The control was confirmed to fail with its advance removed. No application code changes; the refresh interval keeps no test-only seam.
A DELETE on an asynchronous job removed the job's output directory immediately, while the thread doing the work kept running and kept writing into it. Output therefore continued to appear after the deletion and was never cleaned up. Ownership of the removal now goes to whichever party is last: the request if the work has already terminated, the job's own thread as it exits if it has not. A single-use claim on the job, guarded by its own monitor, ensures the directory is removed exactly once and never left behind. An absent directory is treated as the normal outcome it is, rather than being logged as a failure. Refs #2687
With the two existing sleeps replaced, a lint rule stops the practice re-establishing itself. The rule is set to error rather than warning because `eslint .` exits successfully in the presence of warnings, so a warning would report the problem without gating it, and it is scoped to the end-to-end tests so that application source and unit tests are unaffected. Only the one rule is configured. The plugin's recommended preset makes this rule a warning, so adopting the preset would enforce nothing while adding findings unrelated to the practice being blocked.
Nothing on the delete path touched Spark, so a cancellation only reached it when the server next observed a stage boundary for that job. A job inside a single long write stage therefore carried on until that stage finished on its own. Because the output directory is now removed by the job's own thread as it exits, slow cancellation had become slow clean-up as well. The delete path cancels the job group directly; the stage-event checks stay as a backstop. Refs #2687
The notification raised when an output file could not be downloaded was titled by the page that owned the callback, so a failed download from a view card announced itself as "View execution failed". Each of the three download sites now raises its own notification, naming the download. This also removes the export page's failure callback, which existed only to carry that notification.
A file removal that failed while handling a job deletion produced a 500, which misdescribed what happened: by that point the job had been cancelled and removed from the registry, so the client's request had succeeded and could not usefully be retried. The response stays a 202 and carries an additional warning issue saying that the stored files could not be removed and may need manual attention. The failure reaches the operator through the server log and error reporting, on the request path and the job-thread path alike. Refs #2687
Every long-running operation reported its failure twice: persistently in its own card, and again as a transient notification saying the same thing. A user who learns that notifications repeat what is already on screen stops reading them, including the one case where a notification is the only signal there is. A failure a surface displays is no longer announced, on all six surfaces that double-reported rather than only the three the issue names. The failure-reporting callback is gone from the four pages and six components that carried it, since the only thing left travelling down that chain was the download notification, which each download site now raises for itself. Resolves #2689.
Removing a job that was registered directly rather than through the tag-based factory raised a server error, because the absence of a tag entry was treated as an inconsistency. Every bulk submit manifest job is registered that way, so deleting one failed with a 500. Having no tag entry is the normal state for such a job, so it is now recorded at debug level rather than raised.
Records that a delete is acknowledged immediately, that the work is cancelled as part of the request, that the job's output is removed once the work has stopped rather than at the moment of the request, and that a removal that fails is reported as a warning on the accepted response. Refs #2687
The convention that end-to-end tests wait on conditions rather than sleeping, and drive Playwright's clock where the point is that recurring work does not happen, is now enforced by linting and so belongs in the contributing guidelines. Note also that continuous integration applies the documented quality checks, since until now two of them were advisory.
Three presentations were in use for one concept: the shared red callout, a badge-and-text list outside any callout, and a bare red line of text. Only the callout carried an alert role, so six of the seven job surfaces announced nothing to a screen reader. Every displayed failure now renders through the shared callout, and the superseded component is gone. The derivation of what to display is shared too, which removes a local extraction helper from the SQL query card and takes the better of the two fallback chains the pair had between them.
Three of the export and import form tests timed out intermittently, roughly one run in three, once coverage instrumentation was in play. They drive long typing sequences through userEvent and take a few seconds each even when they pass, so the 5s default left almost no headroom.
The contributing guide described the shared callout as taking only a message, and stated the opposite of the rule this work establishes: that a transient operation failure is reported with a notification.
Downloading an output file that fails raised two notifications: the one the download site owns, and a generic "An unexpected error occurred" from the global unhandled-rejection handler, because the download hook rethrew after reporting. The rethrow was there to reach global 401 handling, but the handler it reaches only shows a toast; a 401 has never been treated specially on this path. The rejection now escapes only when no callback was given, so the global handler remains the backstop for a failure nothing else would report.
The ViewDefinition export wrapper had no assertion that a failed export goes to the card without a notification, and the assertion the SQL query export wrapper did have was weaker than it looked: the mocked hook ignored its options, so rewiring a notification through them would have passed. Both now capture the options the wrapper gives the hook, assert no failure callback is wired in, and drive whatever callback is there.
A delete request can land in the narrow window between the job's task entering its body and its first statement resolving the job, leaving the task with nothing to work from. That request cannot have taken responsibility for removing the output, because the work had not terminated when it asked, so the job's own thread still owns it. Skipping the removal in that case orphaned the directory permanently, which is the defect this design exists to close. Also asserts that a bulk submit manifest job is registered as already terminated, so that call site is covered by a regression test. Refs #2687
The per-surface "no notification is raised" assertions could not fail under the regression they guard: each mocked data hook discarded its options, so a card that rewired a notification through the hook's own failure callback would still have passed. Each card test now captures those options, asserts no failure callback is wired in, and drives whatever callback is there. Verified by reinstating the notification in each of the four cards and confirming each test fails.
The test that checks the session expiry dialog reports its failures had to wait out the ten second idle refresh interval in real time, since raising the dialog depends on a second job list request failing. That made it the slowest test in the Admin UI end-to-end suite by a wide margin. Installing Playwright's clock and advancing it past the interval drives that request instead, which brings the test into line with the convention the suite now follows. The assertions are unchanged.
Drive the session expiry poll rather than waiting for it
Stop job deletion racing the work it cancels
Unify job error reporting in the admin UI
Enforce the Admin UI quality gates and remove the end-to-end sleeps
Matches the version now bundled into library-runtime by the core libraries. The two must stay in step, because bulk-export is shaded into the library runtime uber jar unrelocated, so a mismatch would put two copies of the same classes on the runtime classpath.
fhir-bulk-java 1.1.0 validates the manifest as it enters the library, so a poisoned resource type is now refused before any download is attempted, rather than by the executor's check that a downloaded file resolves inside the download directory. The attack is still blocked and the file is still unreachable; only the reported cause has moved upstream. The executor's check remains as a second line of defence.
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
1 participant
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.
No description provided.