Skip to content

Commit e1f23a3

Browse files
smuninismunini
committed
fix(sof): align with PR HL7/sql-on-fhir#365 follow-ups (remove fhir export)
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.
1 parent 504c992 commit e1f23a3

10 files changed

Lines changed: 129 additions & 574 deletions

File tree

‎crates/rest/src/export/in_memory.rs‎

Lines changed: 31 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
//! the (pre-validated) SQL is executed, and the result rows are sharded
1212
//! into output files.
1313
14-
use std::collections::HashMap;
1514
use std::sync::Arc;
1615
use std::time::Duration;
1716

@@ -224,16 +223,12 @@ impl<Sink: ExportSink + 'static> ExportJobController for InMemoryController<Sink
224223
// Job execution
225224
// ============================================================================
226225

227-
/// File extension (without leading dot) for an output format. `fhir` export
228-
/// files carry newline-delimited `Parameters` resources
229-
/// (`application/fhir+ndjson`), distinguished from plain ndjson by the
230-
/// compound `.fhir.ndjson` extension.
226+
/// File extension (without leading dot) for an output format.
231227
fn ext_for(format: &str) -> &'static str {
232228
match format {
233229
"csv" => "csv",
234230
"parquet" => "parquet",
235231
"json" => "json",
236-
"fhir" => "fhir.ndjson",
237232
_ => "ndjson",
238233
}
239234
}
@@ -325,7 +320,7 @@ async fn run_views_job<Sink: ExportSink>(
325320
let shard_slice = &rows[range];
326321
let row_count = shard_slice.len();
327322

328-
let data = format_rows(shard_slice, &format, task.header, &named.view)
323+
let data = format_rows(shard_slice, &format, task.header)
329324
.map_err(|e| format!("view '{}': {e}", named.name))?;
330325

331326
// Shard files are numbered by a running index across the whole
@@ -406,9 +401,8 @@ async fn run_sqlquery_job<Sink: ExportSink>(
406401

407402
/// Materializes a query's table sources and executes its SQL, enforcing the
408403
/// same row caps and timeout as the synchronous `$sqlquery-run` operation.
409-
/// Output column types are refined from the table sources' ViewDefinition
410-
/// schemas (mirroring the run handler) so `_format=fhir` exports carry the
411-
/// right `value[x]` choices.
404+
/// The export operations emit flat formats only (csv/ndjson/parquet/json), so
405+
/// the result's JSON cell values feed `format_output` directly.
412406
async fn execute_sql_query(
413407
runner: &Arc<dyn SofRunner>,
414408
task: &ExportTask,
@@ -461,7 +455,7 @@ async fn execute_sql_query(
461455
tokio::task::spawn_blocking(move || engine.execute_select(&sql, &bindings, max_rows)).await;
462456
watchdog.abort();
463457

464-
let mut result = match exec_result {
458+
let result = match exec_result {
465459
Ok(Ok(r)) => r,
466460
Ok(Err(e)) if e.to_string().contains("interrupted") => {
467461
return Err(ExportError::Runner(format!(
@@ -476,22 +470,6 @@ async fn execute_sql_query(
476470
}
477471
};
478472

479-
// Refine output column types: when a result column name matches a column
480-
// materialized from a table source, prefer the VD-declared FHIR type.
481-
let mut name_to_type: HashMap<String, helios_sof::sqlquery::ColumnFhirType> = HashMap::new();
482-
for schema in &schemas {
483-
for col in &schema.columns {
484-
name_to_type
485-
.entry(col.name.clone())
486-
.or_insert_with(|| col.fhir_type.clone());
487-
}
488-
}
489-
for (i, col) in result.columns.iter().enumerate() {
490-
if let Some(t) = name_to_type.get(col) {
491-
result.column_types[i] = t.clone();
492-
}
493-
}
494-
495473
Ok(result)
496474
}
497475

@@ -500,66 +478,53 @@ async fn execute_sql_query(
500478
// ============================================================================
501479

502480
/// Serializes a shard of view-output rows (column → value JSON objects).
503-
/// `view_json` supplies the declared column types for `_format=fhir`.
504481
fn format_rows(
505482
rows: &[serde_json::Value],
506483
format: &str,
507484
include_csv_header: bool,
508-
view_json: &serde_json::Value,
509485
) -> Result<Vec<u8>, ExportError> {
510486
match format {
511487
"csv" => format_csv(rows, include_csv_header),
512488
"parquet" => format_parquet(rows),
513489
"json" => format_json_array(rows),
514-
"fhir" => helios_sof::fhir_format::format_view_fhir_ndjson(rows, view_json)
515-
.map_err(|e| ExportError::Serialization(e.to_string())),
516490
_ => format_ndjson(rows),
517491
}
518492
}
519493

520-
/// Serializes a shard of SQL query result rows. The flat formats go through
521-
/// `helios_sof::format_output` (matching the `$sqlquery-run` bytes); `fhir`
522-
/// emits newline-delimited typed `Parameters` resources.
494+
/// Serializes a shard of SQL query result rows through
495+
/// `helios_sof::format_output` (matching the `$sqlquery-run` bytes). The
496+
/// export operations support flat formats only; `fhir` is a run-operation
497+
/// format and is rejected at kick-off.
523498
fn format_query_rows(
524499
result: &QueryResult,
525500
range: std::ops::Range<usize>,
526501
format: &str,
527502
include_csv_header: bool,
528503
) -> Result<Vec<u8>, ExportError> {
529504
let rows = &result.rows[range];
530-
match format {
531-
"fhir" => helios_sof::sqlquery::format_fhir_ndjson_rows(
532-
&result.columns,
533-
&result.column_types,
534-
rows,
535-
)
536-
.map_err(|e| ExportError::Serialization(e.to_string())),
537-
_ => {
538-
let ct = match format {
539-
"csv" => {
540-
if include_csv_header {
541-
helios_sof::ContentType::CsvWithHeader
542-
} else {
543-
helios_sof::ContentType::Csv
544-
}
545-
}
546-
"json" => helios_sof::ContentType::Json,
547-
"parquet" => helios_sof::ContentType::Parquet,
548-
_ => helios_sof::ContentType::NdJson,
549-
};
550-
// Build a ProcessedResult directly so columns keep their SQL
551-
// order (mirrors the `$sqlquery-run` handler).
552-
let processed = helios_sof::ProcessedResult {
553-
columns: result.columns.clone(),
554-
rows: rows
555-
.iter()
556-
.map(|r| helios_sof::ProcessedRow { values: r.clone() })
557-
.collect(),
558-
};
559-
helios_sof::format_output(processed, ct, None)
560-
.map_err(|e| ExportError::Serialization(e.to_string()))
505+
let ct = match format {
506+
"csv" => {
507+
if include_csv_header {
508+
helios_sof::ContentType::CsvWithHeader
509+
} else {
510+
helios_sof::ContentType::Csv
511+
}
561512
}
562-
}
513+
"json" => helios_sof::ContentType::Json,
514+
"parquet" => helios_sof::ContentType::Parquet,
515+
_ => helios_sof::ContentType::NdJson,
516+
};
517+
// Build a ProcessedResult directly so columns keep their SQL order
518+
// (mirrors the `$sqlquery-run` handler).
519+
let processed = helios_sof::ProcessedResult {
520+
columns: result.columns.clone(),
521+
rows: rows
522+
.iter()
523+
.map(|r| helios_sof::ProcessedRow { values: r.clone() })
524+
.collect(),
525+
};
526+
helios_sof::format_output(processed, ct, None)
527+
.map_err(|e| ExportError::Serialization(e.to_string()))
563528
}
564529

565530
/// Serialises rows as a single JSON array (`_format=json`).

‎crates/rest/src/handlers/sof/capability.rs‎

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -87,29 +87,45 @@ where
8787
bool_param("supportsAbsoluteReference", true),
8888
];
8989

90-
// Supported output formats (G2: includes parquet; fhir is supported by
91-
// both $viewdefinition-run and $sqlquery-run).
90+
// Supported output formats (G2: includes parquet). `fhir` is offered by
91+
// the synchronous run operations only ($viewdefinition-run,
92+
// $sqlquery-run); per spec PR #365 the export operations bind `_format`
93+
// to ExportOutputFormatCodes (csv/ndjson/parquet/json) and reject `fhir`.
94+
// The capability operation's `supportedFormat` is a single flat list, so
95+
// `fhir` appears here because the server genuinely supports it.
9296
for fmt in ["ndjson", "json", "csv", "parquet", "fhir"] {
9397
params.push(json!({
9498
"name": "supportedFormat",
9599
"valueCode": fmt
96100
}));
97101
}
98102

99-
// Audit item #13: explicit declaration of the spec's
100-
// OutputFormatCodes value-set binding (extensible). The codes
101-
// accepted above (ndjson/json/csv/parquet/fhir) are exactly the
102-
// canonical CodeSystem codes; this entry lets audit tools
103-
// discover the binding without having to follow the
104-
// CapabilityStatement → OperationDefinition link.
103+
// Audit item #13: explicit declaration of the spec's value-set bindings
104+
// (extensible). The run operations bind `_format` to OutputFormatCodes
105+
// (which includes `fhir`); the export operations bind to
106+
// ExportOutputFormatCodes (csv/ndjson/parquet/json, no `fhir`) per spec
107+
// PR #365. These entries let audit tools discover the bindings without
108+
// following the CapabilityStatement → OperationDefinition link.
105109
params.push(json!({
106110
"name": "formatBinding",
107111
"part": [
108112
{
109113
"name": "valueSet",
110114
"valueUri": "https://sql-on-fhir.org/ig/ValueSet/OutputFormatCodes"
111115
},
112-
{"name": "strength", "valueCode": "extensible"}
116+
{"name": "strength", "valueCode": "extensible"},
117+
{"name": "operationScope", "valueString": "run"}
118+
]
119+
}));
120+
params.push(json!({
121+
"name": "formatBinding",
122+
"part": [
123+
{
124+
"name": "valueSet",
125+
"valueUri": "https://sql-on-fhir.org/ig/ValueSet/ExportOutputFormatCodes"
126+
},
127+
{"name": "strength", "valueCode": "extensible"},
128+
{"name": "operationScope", "valueString": "export"}
113129
]
114130
}));
115131

‎crates/rest/src/handlers/sof/export.rs‎

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,10 @@
1212
//! | `/export/{job-id}/status` | DELETE | Cancel job |
1313
//! | `/export/{job-id}/{filename}` | GET | Download output file |
1414
//!
15-
//! Both operations share the status/cancel/download flow and the output
16-
//! format set (`ndjson` default, `csv`, `json`, `parquet`, `fhir` — the
17-
//! latter producing newline-delimited `Parameters` files served as
18-
//! `application/fhir+ndjson`).
15+
//! Both operations share the status/cancel/download flow and the export
16+
//! output format set (`ndjson` default, `csv`, `json`, `parquet`). The `fhir`
17+
//! format is intentionally not offered here: per the spec's Common Operation
18+
//! Behavior it applies to the synchronous run operations only.
1919
//!
2020
//! ## Submit response (202)
2121
//!
@@ -31,7 +31,7 @@
3131
//!
3232
//! - `202 Accepted` + `X-Progress: running` while the job is running
3333
//! - `200 OK` with the completion manifest `Parameters` resource in the body
34-
//! when complete (per the FHIR Asynchronous Interaction Request Pattern —
34+
//! when complete (per the FHIR Asynchronous Bulk Data Request Pattern —
3535
//! there is no `303 See Other` redirect and no separate result URL)
3636
//! - `500 Internal Server Error` + `OperationOutcome` if the job failed
3737
//! - `404 Not Found` if the job ID is unknown or was cancelled
@@ -75,13 +75,15 @@ const ALLOWED_BODY_PARAMS: &[&str] = &[
7575
"source",
7676
];
7777

78-
/// Output formats this server can serialize. The spec binds `_format` to
79-
/// the extensible `OutputFormatCodes` value set; we reject anything outside
80-
/// this list with 400 per the spec's "reject unsupported parameters" rule
81-
/// rather than silently downgrading the output to NDJSON. `fhir` exports
82-
/// each output as a file of newline-delimited `Parameters` resources
83-
/// (`application/fhir+ndjson`) per the spec's Common Operation Behavior.
84-
const SUPPORTED_FORMATS: &[&str] = &["ndjson", "csv", "json", "parquet", "fhir"];
78+
/// Output formats this server can serialize. The spec binds the export
79+
/// `_format` to the extensible `ExportOutputFormatCodes` value set
80+
/// (`csv`, `ndjson`, `parquet`, `json`); we reject anything outside this list
81+
/// with 400 per the spec's "reject unsupported parameters" rule rather than
82+
/// silently downgrading the output to NDJSON. `fhir` is deliberately absent:
83+
/// per the spec's Common Operation Behavior it applies to the synchronous run
84+
/// operations only, since a newline-delimited `Parameters` file has no
85+
/// established media type or consumer.
86+
const SUPPORTED_FORMATS: &[&str] = &["ndjson", "csv", "json", "parquet"];
8587

8688
/// Query parameters for `$viewdefinition-export`.
8789
///
@@ -92,7 +94,8 @@ const SUPPORTED_FORMATS: &[&str] = &["ndjson", "csv", "json", "parquet", "fhir"]
9294
#[derive(Debug, Default, Deserialize)]
9395
#[serde(deny_unknown_fields)]
9496
pub struct ExportQueryParams {
95-
/// Output format: `ndjson` (default), `csv`, `json`, or `parquet`.
97+
/// Output format: `ndjson` (default), `csv`, `json`, or `parquet`
98+
/// (`ExportOutputFormatCodes`; `fhir` is a run-operation-only format).
9699
#[serde(rename = "_format")]
97100
pub format: Option<String>,
98101

@@ -1291,15 +1294,10 @@ where
12911294
.into_response()),
12921295
Some(data) => {
12931296
// Determine Content-Type from extension (G3: include Parquet).
1294-
// `.fhir.ndjson` (newline-delimited Parameters resources from
1295-
// `_format=fhir`) must be checked before the plain ndjson
1296-
// fallthrough.
12971297
let content_type = if filename.ends_with(".csv") {
12981298
"text/csv; charset=utf-8"
12991299
} else if filename.ends_with(".parquet") {
13001300
"application/vnd.apache.parquet"
1301-
} else if filename.ends_with(".fhir.ndjson") {
1302-
"application/fhir+ndjson"
13031301
} else if filename.ends_with(".json") {
13041302
// `_format=json` shards hold a single JSON array of rows.
13051303
"application/json"

‎crates/rest/src/handlers/sof/sqlquery.rs‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,16 @@
1515
//!
1616
//! ## Output shape for flat formats
1717
//!
18-
//! The spec declares the operation's `return` parameter as `Binary | Parameters`
19-
//! (1..1). By default, flat formats (csv/json/ndjson/parquet) are returned as
20-
//! raw payload bytes with the format's `Content-Type` — matching `$viewdefinition-run`
21-
//! and every other SoF reference implementation. Callers that want a strictly
22-
//! spec-shaped response can ask for the `Binary` wrapper by setting
23-
//! `Accept: application/fhir+json`; in that case the bytes are base64-encoded
24-
//! into `Binary.data` and the response is a FHIR `Binary` resource.
25-
//! `_format=fhir` always returns a `Parameters` resource as specified.
18+
//! The spec declares the operation's `return` parameter as `Binary` (1..1):
19+
//! a raw binary stream in the format's native media type, *not* a serialized
20+
//! `Binary` resource envelope (spec PR #365, commit `86c178b`; same shape as
21+
//! `$viewdefinition-run`). When `_format=fhir` is requested the response is a
22+
//! `Parameters` resource instead — the documented exception to the `Binary`
23+
//! return. By default, flat formats (csv/json/ndjson/parquet) are returned as
24+
//! raw payload bytes with the format's `Content-Type`. Callers that want the
25+
//! serialized `Binary` envelope (base64 `data`) can request it by setting
26+
//! `Accept: application/fhir+json` on a *flat* `_format`; this envelope axis
27+
//! does not apply to `_format=fhir`, which always returns `Parameters`.
2628
//!
2729
//! ## Type fidelity under `_format=fhir`
2830
//!

‎crates/rest/tests/sof_capabilities.rs‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,24 @@ mod sof_capability_tests {
194194
strength, "extensible",
195195
"binding strength must be `extensible` per spec"
196196
);
197+
198+
// Per spec PR #365, the export operations bind to a separate
199+
// ExportOutputFormatCodes value set (no `fhir`). The capability
200+
// response declares this second binding alongside the run binding.
201+
let has_export_binding = params
202+
.iter()
203+
.filter(|p| p["name"] == "formatBinding")
204+
.filter_map(|b| b["part"].as_array())
205+
.flat_map(|parts| parts.iter())
206+
.filter(|p| p["name"] == "valueSet")
207+
.any(|p| {
208+
p["valueUri"].as_str()
209+
== Some("https://sql-on-fhir.org/ig/ValueSet/ExportOutputFormatCodes")
210+
});
211+
assert!(
212+
has_export_binding,
213+
"capability must declare the ExportOutputFormatCodes binding for the export operations"
214+
);
197215
}
198216

199217
// =========================================================================

0 commit comments

Comments
 (0)