fix(sof): SQL-on-FHIR v2 spec follow-up fixes#115
Merged
Conversation
Three spec-compliance items from the v2 OperationDefinition audit (`crates/sof/docs/spec-inconsistencies.md`): 1. Absent `patient` / `group` references are now `400 Bad Request` with an `OperationOutcome`, per the spec's error table — the previous "200 OK + Warning: 199 header" path is gone. New `SofError::ReferencedResourceNotFound` carries the absence; the filter in `filter_resources_by_patient_and_group` returns Err instead of an outcome wrapper, dropping `PatientGroupFilterOutcome`. 2. `sof-server` now accepts a bare `ViewDefinition` body as an alternative to a `Parameters` wrapper (the HFS REST handler already did). Other operation parameters must come from the query string in that shape. 3. The HFS REST system-level route at `/$viewdefinition-run` was already wired (verified during audit) — capability output and docs continue to advertise it. Updates the see-also section of `spec-inconsistencies.md` to record item #1 as a resolved deviation and notes the bare-ViewDefinition shortcut as a deliberate-but-spec-silent convenience.
- Reject unsupported `_format` values at submit time with 400 + OperationOutcome (not-supported). Previously unknown formats silently downgraded to NDJSON in the serializer while the manifest echoed the bogus value. - Reject duplicate view names in a multi-view submission with 400 + OperationOutcome (invalid). The completion manifest groups files by view name, so colliding names would silently merge separate views' shards into a single `output` entry. - Capture `failed_at` once at the failure transition (added to `JobStatus::Failed`) and use it in the failed-job manifest so `exportEndTime` / `exportDuration` are stable across multiple polls of the result URL. Previously the result handler called `chrono::Utc::now()` on each poll. Tests: - `test_export_unknown_format_returns_400` - `test_export_duplicate_view_names_returns_400` - `test_export_failed_manifest_export_end_time_is_stable` - `test_export_cancel_after_completion_preserves_completed_state` (locks in the existing controller behavior that a cancel on a completed job is a no-op rather than overwriting state)
`ReferencedResourceNotFound(String)` was added to `SofError` in 66f701d (the $viewdefinition-run v2 spec alignment) but the exhaustive match in `lib_coverage_tests.rs` was not updated, breaking `cargo test --workspace` (used by the Code Coverage CI job). Local `cargo test` from the repo root misses this because pysof is excluded from default workspace members.
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.
Summary
Three-commit stacked PR on top of #66 with follow-up spec-compliance fixes for
$viewdefinition-export(and an unrelated pysof CI break).$viewdefinition-exportspec gaps (2e511d6):_formatvalues at submit time with 400 + OperationOutcome. Previously unknown formats silently downgraded to NDJSON while the manifest echoed the bogus value.outputentry.failed_at: DateTime<Utc>toJobStatus::Failedand use it in the failed-job manifest. Previously the result handler calledchrono::Utc::now()on every poll, soexportEndTime/exportDurationshifted between calls.SofError::ReferencedResourceNotFoundin the exhaustive match inlib_coverage_tests.rs. The variant was introduced in 66f701d but the pysof coverage test wasn't updated, breaking the Code Coverage CI job (cargo test --workspace).feat/sof-integration): align$viewdefinition-runwith v2 spec error & body shapes — context for the pysof breakage.Spec assessment was conducted against OperationDefinition-ViewDefinitionExport; these are the three concrete bugs surfaced by that read-through. Persistent job storage and
source-parameter support remain intentionally out of scope.Test plan
cargo test -p helios-rest --test sof_export— 34/34 pass (4 new tests included)cargo clippy -p helios-rest --all-targetswith CI flags — cleancargo fmt --all --check— cleancargo check --testsincrates/pysof/— compiles (was the CI blocker)New tests added in
crates/rest/tests/sof_export.rstest_export_unknown_format_returns_400test_export_duplicate_view_names_returns_400test_export_failed_manifest_export_end_time_is_stable(polls twice across a 1.1s sleep, asserts identical times)test_export_cancel_after_completion_preserves_completed_state(locks in the existing controller behavior that a cancel on a completed job is a no-op)