feat(rest,persistence): resolve conditional URL entries inside transaction bundles - #919
Open
aacruzgon wants to merge 7 commits into
Open
feat(rest,persistence): resolve conditional URL entries inside transaction bundles#919aacruzgon wants to merge 7 commits into
aacruzgon wants to merge 7 commits into
Conversation
Side findings for review before filing: transactional instance deletes never reach composite secondaries, the spec fixture's `$lookup` entry, the test-hfs skill's missing backend commands, and `BundleError` indexes referring to the sorted entry order.
…s via ConditionalTransaction A transaction Bundle could not carry `PUT [type]?[criteria]` or `DELETE [type]?[criteria]`: the backends' `parse_url` is query-blind, and `ConditionalStorage` writes through the backend's own connection, outside the open transaction, so nothing could resolve criteria within the atomic scope. Add the shape design discussion #28 proposed. `ConditionalTransaction: Transaction` is a separate trait whose one required method, `find_matching`, is the transaction-scoped search; `create_if_none_exist`, `update_conditional` and `delete_conditional` are defaulted on top of it and reuse the existing outcome enums. `BundleEntry` gains typed `criteria`, so modifiers, chains and prefixes reach the backend as `SearchParameter`s rather than a `k=v&k=v` string. `BundleProvider` gains the required predicate `supports_conditional_in_transaction`, false when search is offloaded to a secondary (empty local index) and for S3. `core::bundle_conditionals` holds what every executor shares: resolution of every conditional entry against the transaction's starting view before any write, `412 multiple-matches` for the whole bundle on several matches, the R4 §3.1.0.11.2 overlapping-identity check (a resolved identity another entry addresses fails the bundle naming both entries), and the delete result that names what it deleted through `location`. A matched entry's `fullUrl` is seeded into the reference map before execution, so `urn:uuid` references to it resolve from any position. - SQLite and PostgreSQL implement the trait on their transaction types (PostgreSQL's transaction now carries a cloneable backend handle) and pin the pre-pass target in their PUT/DELETE arms; the string `ifNoneExist` matcher now runs through the same typed search. - MongoDB has no `Transaction` impl, so it runs the same pre-pass on its session; criteria its in-memory matcher cannot evaluate (modifier, chain, prefix, OR-list) refuse the entry with 501 rather than matching nothing. - CompositeStorage delegates the predicate and syncs a 204 carrying a `location` as a delete to secondaries. Tests: unit tests for the overlap check, URL parsing and entry results in `bundle_conditionals`; the backend suites land in the next commit.
…e, PostgreSQL and MongoDB Add `tests/transactions/conditional_url_suite.rs`, generic over `BundleProvider` and `#[path]`-included by all three backend binaries the way `if_match_suite.rs` is, so the same nine scenarios run everywhere: update the single match (200 naming the new version), create on no match, 412 with the sibling create rolled back, delete the match (204 naming the deleted version), no-match 204, several-match delete rolled back, overlap with an instance-addressed entry, two conditional entries resolving to one resource, and a matched PUT's `fullUrl` resolving a `urn:uuid` reference from an earlier entry. SQLite additionally covers the offloaded-search 501 refusal and the `ConditionalTransaction` defaults (upsert, single-match delete, empty criteria match nothing). MongoDB additionally asserts the 501 for a criterion with a modifier. Postgres and Mongo wrappers use per-scenario tenants on the shared container; Mongo probes the topology first and skips on standalone. Tests: transactions_suite 53 pass; postgres_integration 18 matching pass on testcontainers; the 11 MongoDB transaction cases pass against a replica-set container (they skip on the standalone testcontainer).
…bundles The transaction arm declined any non-GET entry whose URL carried a query string with `400 not-supported` before anything executed (#503), while the batch arm resolved the same entries. With the backends now resolving typed criteria inside the open transaction, the guard becomes admission: - `PUT/DELETE [type]?[criteria]` criteria are percent-decoded with repeated keys kept and parsed with the search parser against the tenant's registry, so an unknown parameter, a modifier the type does not define, or a result-shaping `_` parameter (`_count`, `_sort`, `_include`, `_has`, …) is a 400 for the whole bundle rather than a silent "matches nothing" — which on a conditional write is a duplicate (#865) and on a `_`-name matched everything (#866). The typed criteria travel on `BundleEntry.criteria`. - The batch arm's refusals apply: criteria on a POST, an empty query, and `ifMatch` on a conditional entry are 400. A control parameter on an instance URL (`PUT Patient/123?_format=json`) is dropped; the entry addresses the instance either way. - A backend answering `supports_conditional_in_transaction() == false` declines a bundle carrying URL criteria or `ifNoneExist` intact with 501, instead of the 400 the backend's mid-bundle refusal used to surface as. - A conditional entry's audit event names the resource the backend resolved, read from the result's `location`, since its URL has no id and a delete no body. README: the transaction bullet describes the new behavior, the snapshot-resolution rule, the overlap and several-matches outcomes, the 501 gate and MongoDB's criteria-shape limit; the limitation line is removed. Tests: `AuditTarget::from_location` unit test; helios-rest lib 580 pass; the conformance cases land in the next commit. Closes #859.
Replace the two #503 guard tests (declined intact; GET exempt) with seventeen transaction cases on the in-memory SQLite harness: PUT updates the match, creates on none, 412 with the sibling rolled back; DELETE removes the match naming it, no-match 204, 412 deleting nothing; overlap with an instance entry and two conditional entries resolving to one resource are 400 with nothing written; a `urn:uuid` reference to a matched PUT resolves; percent-encoded criteria; `family:exact` honoured and case-sensitive; `_count`/`_sort`/`_include` refused and `_id=nonexistent` deleting nothing; unknown parameter, `ifMatch` and POST-with-criteria 400; an instance URL's control parameter dropped; and the 501 gate on an offloaded-search backend for URL criteria and `ifNoneExist` alike. `create_test_server` is split so a test can hand in a configured backend. Tests: batch_conformance 89 pass.
This was referenced Sep 3, 2026
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…n reach `codecov/patch` failed the PR at 70.44% of the diff hit against an 82.08% target: 183 of the 619 measured new lines were uncovered, and two files carried 159 of them with no coverage at all. The MongoDB backend's share is structural — the coverage job's Mongo testcontainer is standalone, so every transaction test skips there (#390) — but its criteria flattener is pure, and nothing outside a live session was covered either. - `mongodb`: unit tests for `bundle_criteria_pairs`, the flattener that decides which criteria the session-scoped matcher can evaluate — plain pairs, and the refusal naming the first modifier, chain, prefix, OR-list, composite or valueless criterion. The refusal spelled out the implicit `eq` prefix (`family:exact=eqNguyen`), echoing back a criterion in a form the client never sent; it now prints only a prefix that was written. - `composite`: a recording secondary pins the delete fan-out a conditional delete's `204`+`location` triggers, that a locationless `204` syncs nothing, that a create beside it still fans out, and that a fan-out that cannot be queued is logged rather than failing the committed bundle. - `bundle_conditionals`: a fake `ConditionalTransaction` covers the pre-pass without a backend — targets pinned per entry index, a URL naming no resource type, a failing search, the 501 for a backend that cannot resolve conditionals, the `412`'s per-method operation naming, and the three `ConditionalTransaction` defaults reading their outcome from `find_matching`. - `transactions_suite`: `create_if_none_exist`'s three outcomes on a real SQLite transaction, beside the update and delete defaults already there. - `batch_conformance`: a modifier the parameter's type does not define is a 400 for the whole bundle. The ~109 lines still uncovered are the MongoDB session code that only a replica-set container reaches; making the test harness start one is its own change. Tests: persistence lib 1015, transactions_suite 53, batch_conformance 90. fmt clean; CI's clippy line clean on both crates.
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
Transaction Bundles now resolve
PUT [type]?[criteria]andDELETE [type]?[criteria]inside the open transaction. The transaction arm used to decline any non-GET entry with a query string whole (400 not-supported, #503) while the batch arm resolved the same entries (#860). The spec's own transaction example (bundle-transaction.jsonentries 3 and 6) uses both forms.The shape is the one design discussion #28 sketched: a separate
ConditionalTransaction: Transactiontrait with typed&[SearchParameter]criteria, and asupports_conditional_in_transaction()capability predicate onBundleProvider. Criteria are parsed once in the REST layer with the search parser, so the transaction path never hits #865 (modifiers dropped) or #866 (_-names matching everything). The HTTP handlers' and batch arm's string API are unchanged (#861 owns that migration).Closes #859.
Changes
Persistence
ConditionalTransaction(core/transaction.rs): one required method,find_matching, the transaction-scoped search;create_if_none_exist/update_conditional/delete_conditionaldefaulted on top of it with the existing outcome enums.BundleEntry.criteria: Option<Vec<SearchParameter>>.BundleProvider::supports_conditional_in_transaction(required, not defaulted, likesupports_atomic_transactions).core::bundle_conditionals: every conditional entry is resolved against the transaction's starting view before any write (R4 transaction processing rules) and the outcome pinned; several matches →TransactionError::MultipleMatches(412multiple-matchesfor the whole bundle); R4 §3.1.0.11.2 overlap check — a resolved identity another entry addresses (instance PUT/DELETE, or another conditional entry) fails the bundle with 400 naming both entries; a matched entry'sfullUrlis seeded into the reference map sourn:uuidreferences resolve from any position; a delete's 204 carrieslocationnaming the deleted version.ifNoneExistmatcher now runs through the same typed search.supports_conditional_in_transactionis!is_search_offloaded().name=valueonly, so a modifier, chain, prefix, or OR-list refuses the entry with 501 rather than silently matching nothing (Optimize MongoDB If-None-Exist logic #709 owns an index-backed matcher).locationas a delete to secondaries. S3 answersfalse.REST (
handlers/batch.rs)build_search_query_from_pairsagainst the tenant registry; unknown parameter, invalid modifier, or result-shaping_parameter (_count,_sort,_include,_has, …) → 400 whole bundle. Criteria on POST, empty query, andifMatchon a conditional entry → 400, as in batch. A control parameter on an instance URL (Patient/123?_format=json) is dropped.!supports_conditional_in_transaction()with URL criteria orifNoneExist→ 501 intact before anything executes (was a 400 surfacing from the backend's mid-bundle refusal).location.Docs:
crates/rest/README.mdtransaction bullet rewritten;docs/draft-issues-from-859.mdindexes the side findings, all validated and filed: #921 (transactional instance deletes never reach composite secondaries — this PR fixes only the conditional form), #922 (BundleErrorindex is the processing-order index), #868 (the fixture's$lookupentry, already tracked), and a comment on #390 (MongoDB transaction tests skip silently on the standalone testcontainer).Testing
cargo fmt --all -- --checkclean; CI's clippy line (--all-targets --all-features -D warnings+ the 8 allows) clean.cargo test -p helios-persistence: 27 binaries pass;transactions_suite53 (9 shared scenarios + offloaded 501 +ConditionalTransactiondefaults);bundle_conditionalsunit tests 8.cargo test -p helios-rest: lib 580;batch_conformance89, with 17 new transaction cases (update/create/412 rollback, delete/no-match/412, overlap ×2,urn:uuidto a matched PUT, percent-encoding,family:exacthonoured and case-sensitive,_count/_sort/_includerefused, unknown param,ifMatch, POST-with-criteria, instance control parameter, 501 gate for URL criteria andifNoneExist).postgres_integration18 matching pass (9 shared scenarios + existing conditional/transaction/ifMatch). MongoDB: the 11 transaction cases pass against a replica-set container viaHFS_TEST_MONGODB_URL; on the standalone testcontainer they take the existing skip path.Notes
BundleEntrygainscriteria(serde-defaulted,skip_serializing_ifnone);BundleProvidergains a required method — out-of-tree implementors must answer it.ConditionalTransactionis new and additive.ifNoneExistkeeps its in-order semantics. Documented in the README./metadatagating). Follow-ups filed from this work: composite: transactional DELETE [type]/[id] never reaches secondaries — deleted resources stay searchable #921, transaction: failure outcomes name the entry by processing-order index, not the client's Bundle index #922; see also batch/transaction: dispatch $operation entry URLs instead of refusing them as an unknown resource type #868 and the Test suites can pass vacuously: env-gated tests silently skip in CI (sibling of #389) #390 comment.