Reconcile cross-operation spec inconsistencies (#358–#363) by smunini · Pull Request #365 · HL7/sql-on-fhir

smunini · 2026-06-09T18:41:27Z

Purpose

Fixes the six spec-inconsistency issues I filed against the four sibling data operations ($viewdefinition-run, $sqlquery-run, $viewdefinition-export, $sqlquery-export): #358, #359, #360, #361, #362, #363.

Every issue's recommendation asked for the resolution to live once in a shared section the operations reference, rather than being restated (and contradicted) per operation. This PR does exactly that.

Approach

New page operations-common.md — "Common Operation Behavior" (added under the Operations menu) defines the shared rules once. The four operation pages now reference it and drop their divergent wording.

Issue-by-issue

Issue	Resolution
#358 `Binary` vs raw payload	The `return` `Binary` denotes a raw binary stream in the format's native media type — not a serialized `Binary` JSON envelope — exactly as a FHIR `Binary` read under `Accept: application/octet-stream`. The envelope form is returned only when a FHIR media type is requested. Worked examples are normative for the default case.
#359 Bundle unwrap	A `Bundle` passed as a `resource` value to `$viewdefinition-run` is unwrapped: the view runs against each `Bundle.entry[*].resource`. Added the rule and a worked example equivalent to the discrete-resource example.
#360 format set / return type	One shared `_format` enumeration (`csv`/`json`/`ndjson`/`parquet`/`fhir`) and one return-shape matrix across all four operations. `fhir` promoted to a first-class optional format on all four, including async semantics for exports (newline-delimited `Parameters` rows, `application/fhir+ndjson`). `$viewdefinition-run` `return` widened to `Resource` (`Binary`\|`Parameters`); the redundant `SQLQueryRunOutputFormatCodes` value set is removed in favour of `OutputFormatCodes` (which already contained all five codes).
#361 streaming	`Transfer-Encoding: chunked` is stated as transport framing usable with any format, explicitly distinct from incremental result production; the language now applies to all four operations, not just `$viewdefinition-run`.
#362 `Accept` semantics	The raw-payload vs `Binary`-envelope axis is defined as separate from the `_format`-vs-`Accept` precedence axis, with per-format guidance (servers MAY decline the envelope form for parquet/ndjson and return `406`).
#363 async completion	Both export operations are aligned with the cited FHIR Asynchronous Interaction Request Pattern: completion is `200 OK` with the manifest in the status-poll body. The `303 See Other` redirect and separate result resource are removed. Flow diagrams (ASCII + mermaid), operation-flow steps, status-code tables, header-scope notes, and worked examples updated accordingly.

Decisions made (open to committee revision)

Two issues offered a genuine fork; the directions taken here:

Spec Inconsistency: Supported _format set and return type diverge across the four sibling operations #360 → promote fhir everywhere (optional) rather than removing it from $sqlquery-run.
Spec Inconsistency: Export operations specify 303 See Other on completion but cite a pattern that uses 200 OK #363 → conform to the async pattern (200 OK + inline manifest) rather than documenting the 303 as a deviation.

Validation

sushi . → 0 errors (value-set count drops 4→3 with the redundant set removed). The one warning (duplicate OutputFormatCodes/ExportStatusCodes names) is pre-existing.
Full IG Publisher QA should run in CI.

Closes #358, closes #359, closes #360, closes #361, closes #362, closes #363.

Add a shared "Common Operation Behavior" page (operations-common.md) that defines, once, the behaviour the four data operations ($viewdefinition-run, $sqlquery-run, $viewdefinition-export, $sqlquery-export) had specified divergently or left undefined. Each operation page now references the shared section instead of restating (or contradicting) the rules. - #358 Binary vs raw payload: clarify the `return` Binary denotes a raw binary stream in the format's native media type, not a serialized Binary JSON envelope (as with a FHIR Binary read). Worked examples marked normative for the default case. - #359 Bundle unwrap: a Bundle passed as a `resource` value to $viewdefinition-run is unwrapped — the view runs against each Bundle.entry[*].resource. Added rule + worked example. - #360 format set / return type: define one `_format` enumeration (csv/json/ndjson/parquet/fhir) and return-shape matrix for all four operations; `fhir` promoted to an optional shared format incl. async semantics (newline-delimited Parameters rows). $viewdefinition-run return widened to Resource (Binary|Parameters); redundant SQLQueryRunOutputFormatCodes value set removed in favour of OutputFormatCodes. - #361 streaming: state chunked transfer encoding is transport framing usable with ANY format, distinct from incremental result production; apply the language to all four operations, not just $viewdefinition-run. - #362 Accept semantics: define the raw-payload vs Binary-envelope axis as distinct from the _format-vs-Accept precedence axis. - #363 async completion: align both export operations with the cited FHIR Asynchronous Interaction Request Pattern — completion is 200 OK with the manifest in the status-poll body; remove the 303 See Other redirect and the separate result resource. Diagrams, flows, examples, status tables, and header-scope notes updated accordingly. Validated with SUSHI (0 errors).

…on behavior) Implements the reconciled cross-operation rules from spec PR #365 (issues #358-#363), assuming it merges as proposed: - $viewdefinition-export completion (#363): the status poll now returns 200 OK with the manifest Parameters in the body (Expires header moved onto the poll); failed jobs return 500 + OperationOutcome directly on the poll. The 303 See Other redirect and the separate /export/{job-id}/result route/handler are removed. - FHIR content negotiation on run operations (#358/#362): new shared module crates/sof/src/fhir_format.rs used by both HFS REST and sof-server. Accept: application/fhir+json with a flat _format wraps the payload in a serialized Binary envelope (base64 data, bare native media type); application/fhir+xml is rejected with 406 + OperationOutcome (new ServerError::NotAcceptable). $sqlquery-run now shares the same helpers and gained the fhir+xml 406 check. - _format=fhir on $viewdefinition-run (#360): both servers render a typed Parameters resource (one row per result row, value[x] driven by the declared column.type, NULL parts omitted, collections repeated as parts, empty results omit parameter). Accept: application/fhir+json with no _format selects this format. - Bundle unwrapping (#359): the shared lib extractor (crates/sof/src/params.rs) now unwraps a Bundle supplied as a `resource` value one level into its entries — previously only sof-server's own extractor did this, so the HFS REST path ran the view against zero resources. Spec worked Example 5 added as a regression test. - Parquet media type: responses use application/vnd.apache.parquet (run, sqlquery, export downloads); application/parquet and application/octet-stream remain accepted as request aliases. The HFS REST CompressionLayer now skips parquet/zip responses (both parquet identifiers), matching the sof-server predicate added in 58b15f8. - Capability declarations: sof-server /metadata and $sql-on-fhir-capabilities list fhir and the new parquet media type. Deferred (out of scope): $sqlquery-export; _format=fhir on exports.

…t operations Completes the four SQL-on-FHIR data operations per spec PR HL7/sql-on-fhir#365 (Common Operation Behavior). $sqlquery-export (new): - Routes at system (/$sqlquery-export), type (/Library/$sqlquery-export), and instance (/Library/{id}/$sqlquery-export) level; shares the /export/{job-id}/* status, cancel, and download flow with $viewdefinition-export, including 200 OK + manifest completion. - Kick-off resolves everything synchronously so client errors surface before the 202: query 1..* (name / queryReference / queryResource / per-query parameters bindings), Library parsing and SELECT-only validation, depends-on cap, depends-on ViewDefinition resolution (preferring inline `view` table sources matched by canonical url or label, then storage), and Library.parameter binding. Instance level scopes all input parameters to system/type per the spec, so Libraries used there must carry parameter defaults. - Execution generalizes the export controller with an ExportWork enum (Views | SqlQueries). SQL jobs materialize table sources into an in-memory SQLite engine via the controller's SofRunner (patient / group / _since filters apply to the sources), run the SQL under the same row caps and watchdog timeout as $sqlquery-run, refine output column types from the ViewDefinition schemas, and shard through the existing planner/sink. Flat formats serialize via helios_sof::format_output, byte-identical to the run operation. _format=fhir on both export operations: - Each output is a file of newline-delimited FHIR Parameters resources, one per result row, columns as typed top-level parameter entries (value[x] from declared column types, NULLs omitted, collections repeated per element), per the spec's FHIR Format section. - Files are written as shard-N.fhir.ndjson and downloaded with Content-Type: application/fhir+ndjson. - Serializers in helios_sof::fhir_format::format_view_fhir_ndjson (view rows) and helios_sof::sqlquery::format_fhir_ndjson_rows (query results, reusing the typed value[x] mapping). Discovery: $sql-on-fhir-capabilities reports supportsSqlQueryExport and HFS /metadata declares the sqlquery-export operation when an export controller is wired. Tests: 8 new integration tests in crates/rest/tests/sof_export.rs (happy path, fhir format for both operations, instance level, missing query 422, missing Prefer 400, unknown Library 404, non-SELECT SQL 400) plus unit tests for the new serializers.

Four fixes found by auditing the implementation against spec PR #365: - Unsupported _format → 400 (was 415 on the sof-server body path): SofError::UnsupportedContentType now maps to ServerError::BadRequest, per the spec's "SHALL be rejected with 400 Bad Request + OperationOutcome". The stub tests that entrenched 415 are corrected, and a new in-bin test locks the 400 against the production handler. - _format=json export shards are now downloaded with Content-Type: application/json (previously fell through to the ndjson branch); test asserts the header. - Export cancellation race: a job cancelled mid-run could be resurrected to Completed/Failed by its background task, making post-DELETE polls return 200 instead of the spec-required 404. Completion/failure/progress writebacks now go through set_status_if_running, which leaves a Cancelled job untouched. Deterministic unit test added using a blocking mock SofRunner. - HFS_EXPORT_PRESIGN_TTL_SECS default raised 3600 → 86400 so out-of-the-box S3 deployments honor the spec's >= 24h output.location validity (matching the Expires header already advertised on the completion poll).

Per SoF PR HL7/sql-on-fhir#365 (Common Operation Behavior — Asynchronous Delivery), each status poll's headers govern that poll's response, including the 200 OK completion manifest. The status handler now rejects Accept: application/fhir+xml (without fhir+json) with 406 Not Acceptable + OperationOutcome, matching the run operations' behavior, instead of silently returning JSON.

The _format parameter tables on the $viewdefinition-run, $viewdefinition-export and $sqlquery-export pages still listed the old four-format set, contradicting the shared enumeration in Common Operation Behavior and each page's own Format Parameter Clarification section. Add fhir to all three and link to the clarification section, matching the form already used on the $sqlquery-run page.

The worked example for an unsupported _format on $viewdefinition-run listed only the old four formats in its diagnostics message, omitting fhir, which the operation now supports.

The export operations cited the FHIR Asynchronous Interaction Request Pattern while linking to async-bulk.html. These are distinct R5 patterns: the Interaction pattern completes with a batch-response Bundle, whereas the Bulk Data pattern completes with 200 OK and a manifest of output file links, which is the behaviour this spec describes. Rename the pattern throughout, fix the comparison notes on the export pages, and state the Parameters manifest as the single deliberate deviation from the pattern in Common Operation Behavior.

The format table gives fhir a native media type of application/fhir+json, while Axis 2 of content negotiation says that same Accept value selects a serialized Binary envelope, making the combination ambiguous. State explicitly that Axis 2 applies only to the flat formats and that _format=fhir always yields the Parameters resource itself.

The two run operations' return parameters carry the same semantics, but only ViewDefinitionRun's documentation included the raw-stream clarification and the reference to Common Operation Behavior. Bring SQLQueryRun's into line.

The $viewdefinition-export operation defines a header parameter in its OperationDefinition, and Common Operation Behavior states the csv header rule applies to all four operations, but the Export Control table omitted it. Use the same wording as the $sqlquery-export page.

The streaming section granted chunked transfer encoding on all four operations, but the export operations' responses are the async status flow with no result payload to stream, and the file downloads are ordinary HTTP responses (possibly not even served by the FHIR server) whose framing is already governed by HTTP. State the section's scope as the two run operations and drop the normative framing statements about exports.

FHIR R5 defines the raw-vs-envelope response behaviour specifically for operations whose return is a Binary (operations.html, Operation Response), so widening the type to Resource loses that signal. Declare the return as Binary on both run operations and document the Parameters response for _format=fhir as the exception.

The fhir format exists to return result rows as typed FHIR values in the synchronous response; exports produce flat files, where a newline-delimited Parameters representation has no established media type or consumer. Restrict fhir to the run operations: bind the export operations' _format to a new ExportOutputFormatCodes value set (csv, ndjson, parquet, json), drop the fhir export semantics from Common Operation Behavior, and update the export pages' format tables and clarification sections.

A worked example's view.viewReference pointed to a Binary resource; the view parameter references ViewDefinitions.

The download step showed an ndjson file served as application/fhir+ndjson; the native media type for the ndjson format is application/x-ndjson per the shared format table.

Both lists skipped from item 5 to item 7 after the removal of the previous item 6.

johngrimes · 2026-06-13T00:10:44Z

I've made a few changes:

Brought the three remaining four-format parameter tables, the invalid-format error example, the SQLQueryRun return documentation, and the missing ViewDefinitionExport header row into line with the shared rules.
Renamed the cited async pattern to the Asynchronous Bulk Data Request Pattern, which is what async-bulk.html actually defines and what the described behaviour matches. The Parameters manifest is now noted as the one deliberate deviation.
Resolved the _format=fhir content negotiation ambiguity: the raw-versus-envelope axis now applies to the flat formats only.
Scoped chunked transfer encoding to the run operations; the export responses have no payload to stream and the file downloads are plain HTTP responses, so the spec stays silent on their framing.
Typed the run operations' return as Binary rather than Resource, since R5 defines the raw-versus-envelope behaviour specifically for Binary returns. The Parameters response for _format=fhir is documented as the exception.
Removed fhir from the export operations (run operations only now). As an export file it would rely on a newline-delimited Parameters representation with no established media type or consumer. Exports now bind to a new ExportOutputFormatCodes value set.
Cosmetic: a Binary/... viewReference in an example, the media type in the export flow diagram, em dashes, list numbering.

johngrimes · 2026-06-13T00:11:52Z

I'll create a separate PR to align the spec to the new Asynchronous Interaction Request Pattern page.

…xport) Align our SoF implementation to the final state of PR #365 after John Grimes' 13 follow-up commits. Behavioral change — remove _format=fhir from the export operations (commit 8c21fc4), reverting the export half of bbcaa0a: - $viewdefinition-export / $sqlquery-export now reject _format=fhir with 400; the export _format binds to the new ExportOutputFormatCodes set (csv, ndjson, parquet, json). A newline-delimited Parameters file has no established media type or consumer; fhir is a run-operation format. - Delete the now-dead export-fhir code: format_view_fhir_ndjson + FHIR_NDJSON_MIME, format_fhir_ndjson_rows + its re-export, the .fhir.ndjson extension and application/fhir+ndjson download content-type, and the column-type refinement that only fed it. - Flip the two export-fhir tests to assert 400. Doc/metadata alignment: - Async pattern rename to "Asynchronous Bulk Data Request Pattern" (2c7d3d8) in export.rs and spec-inconsistencies.md. - Document the run operations' return as Binary (raw stream) with Parameters as the _format=fhir exception (86c178b) in sqlquery.rs. - capability.rs: declare the run/export value-set split via a second formatBinding (ExportOutputFormatCodes); strengthen the capability test to assert it. - Add a Resolution summary to spec-inconsistencies.md recording how issues #358-363 settled. The run-side Accept/envelope behaviour (b8b9014) and run-only fhir support were already correct and unchanged. cargo fmt + clippy (CI flags) clean; helios-sof and helios-rest SoF test suites pass.

…lled Per the SQL-on-FHIR operations-common spec (HL7/sql-on-fhir#365), a server SHOULD clean up partial results when an export is cancelled via DELETE on the status URL. Previously, cancelling a $viewdefinition-export / $sqlquery-export job transitioned it to Cancelled but already-written output shards were never deleted and remained downloadable via GET /export/{job_id}/{filename}. - Add `delete_job(&self, job_id)` to the `ExportSink` trait and implement it for all three sinks: FilesystemSink (remove_dir_all), InMemorySink (drop matching keys), S3Sink (paginated list + delete under the job key prefix). - Call it from the cancellation path, and again from the background task when it finishes a job that was cancelled mid-run (covers the write-after-cancel race). - Gate `read_shard` on Cancelled status so a cancelled job's files 404 even while deletion is still draining. Closes HeliosSoftware#144

johngrimes self-assigned this Jun 9, 2026

smunini mentioned this pull request Jun 10, 2026

SoF export: clean up partial results when a job is cancelled HeliosSoftware/hfs#144

Closed

johngrimes added 13 commits June 13, 2026 08:57

Add fhir to invalid-format error example

e22ddae

The worked example for an unsupported _format on $viewdefinition-run listed only the old four formats in its diagnostics message, omitting fhir, which the operation now supports.

Fix viewReference resource type in export example

51f860c

A worked example's view.viewReference pointed to a Binary resource; the view parameter references ViewDefinitions.

Correct media type in export flow diagram

b0f7518

The download step showed an ndjson file served as application/fhir+ndjson; the native media type for the ndjson format is application/x-ndjson per the shared format table.

Replace em dashes with spaced hyphens on common behaviour page

1f959c8

Renumber export operation flow lists

1c041f9

Both lists skipped from item 5 to item 7 after the removal of the previous item 6.

johngrimes approved these changes Jun 13, 2026

View reviewed changes

johngrimes merged commit f0da3d2 into main Jun 13, 2026
1 check passed

johngrimes deleted the fix-spec-inconsistencies-358-363 branch June 13, 2026 00:15

johngrimes mentioned this pull request Jul 20, 2026

Bind ViewDefinition operations to their own output format ValueSets #331

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reconcile cross-operation spec inconsistencies (#358–#363) - #365

Reconcile cross-operation spec inconsistencies (#358–#363)#365
johngrimes merged 14 commits into
mainfrom
fix-spec-inconsistencies-358-363

smunini commented Jun 9, 2026

Uh oh!

johngrimes commented Jun 13, 2026

Uh oh!

johngrimes commented Jun 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

smunini commented Jun 9, 2026

Purpose

Approach

Issue-by-issue

Decisions made (open to committee revision)

Validation

Uh oh!

johngrimes commented Jun 13, 2026

Uh oh!

johngrimes commented Jun 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants