fix(search): _contained remainder — contained _id rows, _sort, Postgres modifiers, composites, compartment by smunini · Pull Request #1423 · HeliosSoftware/hfs

smunini · 2026-09-21T18:05:48Z

Summary

This PR does the remaining _contained search work from #1407. That work is what #1398 left after fixing the correctness items of #1383. There are nine commits, one per item, in priority order.

What changed

1. An ID-only contained resource is now indexed on PostgreSQL.

The writer dropped contained _id rows.
A contained resource with no other indexed value was therefore invisible to _contained search, including the no-criteria form.

2. _sort under _contained is refused where it used to be ignored.

On SQLite, PostgreSQL and MongoDB, contained matches came back ordered by container type, ID and local ID, whatever _sort asked for.
- _contained=both appended them to the sorted top-level page.
- The response was a 200 with results in the wrong order.
The response is now a 400 naming _sort, like every other criterion the contained path cannot apply.
Elasticsearch already applies _sort.
- It sorts on the contained resource's own values.
- The container's lastUpdated stands in for the contained resource's.
- That behaviour is now documented and pinned by a test.

3. The PostgreSQL value predicates are split from the id IN (…) wrapper. This is a pure refactor.

Six builders now produce a bare per-row value predicate plus a shared wrapper: build_string_condition, build_token_condition, build_of_type_condition, build_reference_condition, build_reference_identifier_condition and build_uri_condition.
The SQL for non-contained search is unchanged.
- The existing unit tests that pin exact SQL and placeholder numbering were not edited, and they pass.

4. Modifiers, and the reference and uri forms, work under _contained on PostgreSQL.

This builds on the refactor in item 3.
These modifiers are now applied, not refused: :exact, :contains, :text, :of-type, :identifier and the structural :above/:below.
Contained reference and uri predicates gain their bare-ID and URL forms.
:missing stays a 400, because existing tests pin that.

5. Composite parameters work under _contained on SQLite.

6. Composite parameters work under _contained on PostgreSQL.

fix(search): _contained with no criterion, compartment, _has/_list and counts on all backends (#1383) #1398's design note recorded this as blocked on the writer ("stores contained composites with no group key"). Half of that was wrong.
- IndexRow::from_extracted keeps the extractor's composite_group on every contained row.
- The pairing can therefore be done at read time.
No writer change, no schema change and no reindex.
A value with the wrong number of components, or an unparseable value, fails closed.
The pairing case is verified on a real PostgreSQL.
- The search is component-code-value-quantity=A$gt5.
- It must not match a resource whose components are A = 1 mg and B = 9 mg.

7. Compartment searches resolve _list, _has and chains.

handlers/compartment.rs never resolved these, with or without _contained.

8. The Elasticsearch truncation of _contained results is documented.

The result list is bounded by max_result_window.
Beyond that bound, _total is a lower bound only.

9. A style fix.

A useless format! in the _sort refusal is removed.

Not done

MongoDB still refuses composites under _contained. The refusal is by name, with a 400.
MongoDB _contained=both de-duplication is per page.
- _total can over-count when a container of the searched type is itself a top-level match.
- This is item 6 of _contained search: remaining work after #1398 (Postgres modifiers, composites, _sort, contained _id rows) #1407.

I left Fixes off so that #1407 stays open for those two.

Behaviour changes clients can observe

On PostgreSQL, a contained resource that has only an ID is now found. This applies to newly written resources; existing ones need a reindex to gain the row.
_contained with _sort on SQLite, PostgreSQL and MongoDB was a 200 in the wrong order. It is now a 400 naming _sort.
On PostgreSQL under _contained, the listed modifiers and the bare-ID and URL reference forms now work. They were a 400.
On SQLite and PostgreSQL under _contained, composite parameters now work. They were a 400.
Compartment searches now honour _list, _has and chained parameters.

Verification

Run after merging current main. The backend suites used real containers. Every new case is in the shared contained_suite.rs, so it runs on all four backends.

Run	Result
`cargo test -p helios-persistence --lib`, all five backend features	1895 passed
`sqlite_tests`, `search_suite` and three composite suites	264 passed
`postgres_tests` (full)	269 passed
`mongodb_tests` (full)	206 passed
`elasticsearch_tests` (full)	141 passed
`cargo test -p helios-rest`	1717 passed
`cargo +1.98.1 clippy` with the CI flags: `helios-persistence` with all four FHIR versions and all five backends, `--all-targets`; `helios-rest` and `helios-hfs`, `--all-targets`	clean
`cargo fmt --check`	clean

Not run on this branch alone: the full --workspace --all-features compile. That runs on the combined tree of the open PRs.

Refs #1407

The PostgreSQL writer drops the `_id` and `_lastUpdated` values of a contained resource: `build_contained` answers `_id` from the `contained_local_id` column every contained row carries, so the `_id` row is a restatement with no reader (1.5% of the crud suite's index write). But `build_contained` finds contained resources by grouping their rows, so a contained resource that yields no other indexed value - a bare `{"resourceType": "Location", "id": "x"}` - had no row at all and was invisible to every `_contained` search: the no-criteria form, `_id` and `:not` included. SQLite, MongoDB and Elasticsearch keep the `_id` value and were not affected. Keep the `_id` row exactly when the contained resource would otherwise have no row (a presence row). Resources with any other indexed value still save it, and the write path cannot fail on it. Existing data: an id-only contained resource written before this change has no row and stays invisible until its container is re-indexed. Refs #1407

On SQLite, PostgreSQL and MongoDB a `_contained=true|both` search listed its contained matches by container type, id and local id whatever `_sort` asked for (and `both` appended them to the sorted top-level page) - a 200 in the wrong order. The contained path only groups the contained index rows; it never read `query.sort`. Refuse it with a 400 naming `_sort`, like every other criterion the contained path cannot apply (#1363). Elasticsearch already applies it, to the contained resource's own values across both halves of `both`, with the container's `lastUpdated` standing in for the contained resource's; that is now documented and pinned. New shared scenario `contained_suite::sort_and_id_only_contained` runs on all four backends: `_sort` is applied in the right order or refused by name, and an id-only contained resource is found (previous commit). Refs #1407

…IN wrapper `build_string_condition`, `build_token_condition`, `build_of_type_condition`, `build_reference_condition`, `build_reference_identifier_condition` and `build_uri_condition` each inlined, per value, both the bare `search_index` row predicate and the `id IN (SELECT resource_id FROM search_index WHERE ... AND <predicate>)` membership test around it. `build_contained` groups the contained rows itself and needs only the row predicate, so it could reuse none of them and refuses every modifier (#1407 items 1-2). Split each into a `*_value_predicate` function (one value, carrying the placeholder counter) plus the shared `index_membership` wrapper; the `build_*_condition` functions are now loops over the two. `:identifier` keeps its own aliased wrapper and shares the filter and the `value_reference IN (...)` predicate. No behaviour change: the emitted SQL and bind parameters are byte-identical. Checked with the existing unit tests (which pin the SQL, the placeholder numbering and `single_index_predicate` extraction) and with a throwaway differential dump of 4 types x 14 modifiers x 10 value sets x 2 offsets, solo and in a conjunction, before and after. Refs #1407

… on PostgreSQL PostgreSQL refused every modifier but `:not` under `_contained=true|both`, and matched contained reference and uri values by bare equality only, so `Observation?_contained=true&subject=pt1` (the bare-id form, the primary one in FHIR) returned nothing while `subject=Patient/pt1` matched. Both came from `build_contained` borrowing the modifier-less composite component predicates because the modifier-aware builders could not be used without their `id IN (...)` wrapper. With the value predicates split out (previous commit), `build_contained` now uses them bare: - string: `:exact`, `:contains`, `:text`, and the default starts-with now in its accent-folded, wildcard-escaped range form (it was a raw `value_string ILIKE 'v%'`, so `%` in a value was a wildcard); - token: `:text`, `:code-text`, `:of-type` (a malformed `:of-type` value fails closed instead of being dropped); - reference: bare id, `:Type`, absolute URL, and `:identifier`, whose target lookup excludes contained identifier rows (they are stored under the container and would make it a target); - uri: `:contains`, `:below`, `:above`. `contained_unsupported_reason` shrinks to the list SQLite uses. `:missing` stays a 400 (pinned by the REST and PostgreSQL suites), as do `:in`, `:not-in` and token `:above`/`:below`, which are not predicates on one index row. Non-contained SQL is unchanged (same differential dump as the refactor commit). SQLite: `reference:identifier` let a contained resource's identifier make its *container* a target, for top-level and contained searches alike; one conjunct (`si2.is_contained = 0`) in the reference handler. Found by the new shared scenario `contained_suite::reference_identifier_resolves_the_target`. Refs #1407

SQLite refused composites under `_contained=true|both`. The contained composite rows are there, each carrying the `composite_group` of its contained resource's extraction; only the read-side pairing was missing. `build_contained` now narrows the contained entities, like `_id` does, with `(resource_type, resource_id, contained_local_id) IN (SELECT ... GROUP BY resource_type, resource_id, contained_local_id, composite_group HAVING <every component>)`, reusing `CompositeHandler::build_component_fragments` exactly as the top-level `build_composite_parameter_condition` does. A composite with a modifier or without resolved components is still refused by name. The shared suite gains the discriminating case: a contained Observation with components A = 1 mg and B = 9 mg must match `component-code-value-quantity=A$lt5` and not `A$gt5`. PostgreSQL and MongoDB still refuse composites under `_contained`: PostgreSQL stores contained composites unfolded with no group key (needs a writer change and a reindex), MongoDB needs a grouped pair check over `search_index_contained`. Refs #1407

#1398's design note recorded this as blocked on the writer ("stores contained composites unfolded, one row per component, with no group key"). Half of that is wrong: `build_contained_rows` does bypass `fold_composites`, but `IndexRow::from_extracted` keeps the extractor's `composite_group` on every row, so contained composites are stored in exactly the one-row-per-component form `build_composite_condition_legacy` reads - on every database, whatever its top-level layout. No writer change, no schema change and no reindex are needed. `build_contained` now narrows the contained entities with the legacy pairing keyed on the contained entity: `(resource_type, resource_id, contained_local_id) IN (SELECT ... AND (<prefilter>) GROUP BY resource_type, resource_id, contained_local_id, composite_group HAVING bool_or(<component>) AND ...)`, every component on the base (slot 1) columns. A value with the wrong number of components or an unparseable one fails closed. A composite with a modifier or without resolved components is still refused by name. Verified on a real PostgreSQL by the shared suite, including the pairing case (`component-code-value-quantity=A$gt5` must not match components A = 1 mg, B = 9 mg). MongoDB still refuses composites under `_contained`. Refs #1407

`handlers/compartment.rs` built the query and handed it straight to the backend's `search()`. The type search handler resolves `_list`, `_has` and chained parameters into an `_id` filter first, because the backends do not read `query.list` / `query.reverse_chains`; the compartment handler never did, so `Patient/p1/Observation?_list=l1` returned the whole compartment (reproduced with the new REST test: both Observations instead of the listed one). It also never checked `supports_contained_search`, so `_contained` on a backend without contained indexing was not a 501. One helper, called by the single-type and the all-types handler before `search()`: the `_contained` capability gate (501), the `_contained` + `_list`/`_has`/chain refusal (400, same text as the type search, before any resolution so the ids are not misread as local ids), the functional `$current-*` list refusal (501), then `resolve_list` and `resolve_chains`. No terminology expander is passed, unlike the type search: a terminology-backed modifier on a chain terminal is refused by the resolver instead of expanded. Refs #1407

…s truncated The `_contained` result list on Elasticsearch is de-duplicated from one request's hits, so it - and `_total`, `search_count` and any page beyond it - is bounded by `max_result_window`. `hits.total` counts documents, not containers, so the total cannot be repaired from it. Log a warning when the window is reached instead of reporting a short total silently, and record why in the code. The bound itself is not lifted here. Refs #1407

clippy 1.98 (useless_format): the message has no arguments. Refs #1407

…arch-remainder

…omic-versioned-writes #1419 and this PR each add a backend-agnostic suite (modifier_parity_suite there, versioned_write_race_suite here) and wire it into the same spots of tests/mongodb_tests.rs and tests/postgres_tests.rs, so whichever merges second conflicts. Resolved here, making the order #1419 then this PR; every test kept unchanged. The resulting tree is byte-identical to the corresponding step of a scratch merge of main with all four open PRs (#1419, #1421, #1422, #1423), which compiled under `cargo test --workspace --all-features --no-run` and passed the full SQLite, PostgreSQL, MongoDB, Elasticsearch and REST suites.

…nto feat/1406-shared-conditional-patch #1419, #1421 and this PR each add a backend-agnostic suite and wire it into the same spots of tests/mongodb_tests.rs, tests/postgres_tests.rs and tests/sqlite_tests.rs, so whichever merges later conflicts. Resolved here, making the order #1419, #1421, then this PR; every test kept unchanged. The resulting tree is byte-identical to the corresponding step of a scratch merge of main with all four open PRs (#1419, #1421, #1422, #1423), which compiled under `cargo test --workspace --all-features --no-run` and passed the full SQLite, PostgreSQL, MongoDB, Elasticsearch and REST suites.

codecov · 2026-09-21T20:13:59Z

Codecov Report

❌ Patch coverage is 93.19938% with 44 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...ence/src/backends/postgres/search/query_builder.rs	95.50%	22 Missing ⚠️
crates/rest/src/handlers/compartment.rs	58.82%	21 Missing ⚠️
...stence/src/backends/sqlite/search/query_builder.rs	98.59%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

smunini added 10 commits September 21, 2026 08:49

style(persistence): drop a useless format! in the _sort refusal

77dd78f

clippy 1.98 (useless_format): the message has no arguments. Refs #1407

Merge remote-tracking branch 'origin/main' into fix/1407-contained-se…

2404c34

…arch-remainder

smunini mentioned this pull request Sep 21, 2026

fix(rest): refuse system-level search with 501 and stop advertising search-system (#1338) #1425

Open

Navigation Menu

fix(search): _contained remainder — contained _id rows, _sort, Postgres modifiers, composites, compartment - #1423

fix(search): _contained remainder — contained _id rows, _sort, Postgres modifiers, composites, compartment#1423
smunini wants to merge 10 commits into
mainfrom
fix/1407-contained-search-remainder

smunini commented Sep 21, 2026

Uh oh!

codecov Bot commented Sep 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

smunini commented Sep 21, 2026

Summary

What changed

Not done

Behaviour changes clients can observe

Verification

Uh oh!

codecov Bot commented Sep 21, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant