fix(api_v2): REST API v2 defects found by the OpenFn adaptor (#554) - #555
Open
gonzalesedwin1123 wants to merge 38 commits into
Open
gonzalesedwin1123 wants to merge 38 commits into
gonzalesedwin1123 wants to merge 38 commits into
Conversation
…ip addressing (#554) Covers item A (GET/PUT resolve to an arbitrary program's membership, PUT re-parents or reassigns the membership from the body, POST Location is not followable) and item B (If-Match rejects the resource's own ETag).
…554) A beneficiary enrolled in several programs made GET/PUT /ProgramMembership/{identifier} act on whichever membership came first, and PUT wrote the body's program and beneficiary onto it, moving the membership to another program or registrant. - optional ?program= selects the membership; several memberships without it return 409, after the consent check so enrollment is not revealed - PUT refuses (422) a body naming another program or beneficiary - POST Location is URL-encoded and carries ?program= - If-Match compares against the same microsecond versionId as the ETag
An unknown group, gender, membership role or member made its parser return an empty domain, so the filter dropped out and the search returned the whole registry. Malformed values were dropped the same way. - malformed filters raise InvalidSearchParam, answered as 400 - well-formed filters naming nothing match nothing - group, membership-role, identifier and member conditions use 'any' so they hold on the same related row; ?member= excludes ended memberships
…ollowed (#554) Covers item F: Group members, $add-member/$remove-member responses, membership history and Individual groupMembership build references from the vocabulary namespace instead of the identifier type's code URI.
…URI (#554) Group members, $add-member/$remove-member responses, membership history and Individual groupMembership built references from the vocabulary namespace (urn:openspp:vocab:id-type|...), which no lookup matches, so following them failed. Use id_type_id.uri, as identifier[].system does.
…mmediate (#554) Covers item G: $remove-member, merge and split end memberships with a microsecond datetime.now(), so the stored is_ended/status stay active until the repair cron runs.
…iate (#554) $remove-member without endedDate, and the member moves in merge and split, wrote ended_date with datetime.now(). Its microseconds put the end a fraction of a second after the second-precision fields.Datetime.now() the is_ended/status computes use, so the row was stored as active until the repair cron ran. Use fields.Datetime.now(), also in GET /Group's member filter so both agree.
…ers (#554) Covers item I: POST /Individual and /Group accept an identifier already live on another registrant, and every lookup then silently picks one of them (PATCH deactivated the wrong record). Soft-removed IDs still resolve. Adds the registrant_resolver module skeleton the tests import.
The registry lets two registrants hold the same ID type and value (the ID-document deduplication manager exists to find them), but every API lookup took the first match, so reads and writes could hit either one. - POST /Individual and /Group refuse an identifier already live on another registrant (409) - a shared resolver (services/registrant_resolver) never picks one of several matches; routers answer 409, or the 'not found' 403 with jitter for a consent-requiring client lacking consent for any match - covers reads, updates, member operations, merge/split, search filters, bulk export, batch bundles and ProgramMembership beneficiaries - soft-removed (invalid) IDs no longer resolve; references, membership identifiers and Location use a live ID
Batch group create orphaned by an ambiguous member, $split creating a duplicate identifier, existence oracles (unknown beneficiary 404, search filter 403 vs empty 200), membership count disclosure and missing PUT consent check, consent predicate mismatch, soft-removed IDs still listed, transaction bundles answering 422, individual/group kind collisions. Also: positive controls for two filter tests, and the consent-denied ambiguity test now fails on the old pick-newest behaviour.
- group create runs in a savepoint so an ambiguous member cannot leave the group behind in a batch bundle; $split refuses an identifier in use - anti-enumeration: unknown beneficiary is the jittered 403 for consent clients on ProgramMembership GET/PUT; ambiguous search filters give an empty page, not 403, to a client that may not know; the membership count is no longer disclosed and PUT checks consent before 409/404 - the 409-vs-403 decision uses the read path's consent rule (filter_response), shared with ProgramMembership - soft-removed IDs are no longer listed in identifier[] or matched by identifier=; lookups resolve by kind (individual/group) - transaction bundles answer 409 for identifier conflicts; group create audit-logs the 409; HISTORY describes the actual behaviour Two existing not-found tests now use a legal-basis client for the 404 (Edwin-approved), as test_read_individual_not_found does; consent clients get 403, covered by a new test.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #555 +/- ##
==========================================
+ Coverage 76.91% 77.28% +0.37%
==========================================
Files 704 732 +28
Lines 45774 46794 +1020
==========================================
+ Hits 35205 36163 +958
- Misses 10569 10631 +62
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
gonzalesedwin1123
marked this pull request as ready for review
September 25, 2026 04:49
This was referenced Sep 25, 2026
Open
search_groups never read _offset, so every page and next link returned the first page, and consent over-fetch refilled pages from the start of the results (empty or repeated pages).
…enrollment is 409 without DB internals (#554 L, O)
…nrollment is 409 without DB internals (#554 L, O)
…total oracle, A2 links, A4, A7, A9, A11)
…earch totals; encode membership links; bound _offset (#554 review S1, A2, A11)
…straint; shared row cap; role message (#554 review A4, A8, A10)
…channel is closed (#554 review R1)
This was referenced Sep 28, 2026
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.
Fixes #554. It covers the REST API v2 defects found while building the OpenFn
@openfn/language-opensppv4 adaptor, which uses/api/v2/spp. Item C (RFC 9457 error bodies) is a follow-up, not part of this PR.Each item is its own pair of commits: a failing test, then the fix. You can review it one item at a time.
Items
spp_api_v2_programsGET/PUT /ProgramMembership/{id}no longer act on an arbitrary program's membership. A new optional?program=Program/{system}|{value}selects the membership. Without it, a beneficiary with several memberships returns 409.PUTcan no longer move a membership to another program or beneficiary (422).POSTLocationis URL-encoded and carries?program=.spp_api_v2_programsIf-MatchonPUT /ProgramMembershipcompares against the same microsecondversionIdas theETag. It used to reject every request.spp_api_v2identifier/group/gender/birthdate/_lastUpdated/membervalues return 400. Unknown groups, roles, genders or members match nothing, instead of returning the whole registry. Multi-condition filters (group=,membership-role=,identifier=,member=) apply to one related row (any).spp_api_v2Group.member[],$add-member/$remove-memberresponses, membership history,Individual.groupMembership) use the ID type's code URI (…#code), so following them works.spp_api_v2$remove-memberwithoutendedDate, and member moves in merge/split, end memberships on the ORM clock (fields.Datetime.now()). The storedis_ended/statusare therefore correct immediately, not after the repair cron runs.POST /Individual,/Group,$split, bundle creates) refuses an identifier already live on another registrant (409). Lookups never pick one of several matches (409). Soft-removed IDs no longer resolve and are no longer listed. Lookups resolve by kind (individual/group).spp_api_v2GET /Groupapplies_offset. The group search ignored it, so every page and everynextlink returned the first page again. When consent filtering skipped records, the page was refilled from the start of the results, which returned an empty page or repeated groups. Found after the issue was filed, while reviewing search for the OpenFn adaptor.spp_api_v2GET /Individual?group=…&membership-role=…requires the role on the membership of that group. Someone who held the role in another group was also returned (getGroupMembers(G, {role})).spp_api_v2_programsGET /ProgramMembershipfilters fail closed: a malformedbeneficiary=/program=returns 400 instead of every membership.spp_api_v2(+ programs)/Individual,/Groupand/ProgramMembership:nextno longer skips rows fetched but not examined;_count> 50 no longer stops at the per-query cap; a page cut short by the 3x over-fetch limit keeps itsnextlink while rows remain (clients follownextuntil null; a page can be short or empty).spp_api_v2PATCH /Individualgender works (it returned 422: the vocabulary lookup ran as the public user without sudo). Unknown codes, and codes from a vocabulary other than ISO 5218, return 422 on create and PATCH.spp_api_v2_programsPOST /ProgramMembershipreturns 409 "already a member" (pre-check, with the UNIQUE constraint as the race backstop). It returned 422 with PostgreSQL text including internal record ids. Unexpected create/PUT/search errors return a generic message and are only logged.spp_api_v2$add-memberand the member PATCH reject an unknown role code with 422 naming it (it was silently dropped); other validation errors on these endpoints now return their message.spp_api_v2_programsGET /Programwithout scope returns 403 (it returned 500: thestatusquery parameter shadowed FastAPI'sstatus).spp_api_v2(+ programs)meta.totalis the page size on every page (page_total_and_next). It was hidden only when the page met a hidden record, so?identifier=…&_offset=1returned the raw count, an existence oracle bypassing item I's 403. J had made this reachable on/Group. Also: ProgramMembership links URL-encoded;_offsetbounded (422).K–Q were found by a coverage check of the adaptor's calls against this branch and confirmed with HTTP probes; S1 and the rest came from the adversarial staff review of J–Q. Items A–J were already in this PR.
Versions:
spp_api_v219.0.2.1.1 → 19.0.2.2.0,spp_api_v2_programs19.0.1.0.0 → 19.0.1.1.0. HISTORY fragments list every client-visible change. There is no schema change, so no migration.Design decisions
spp.deduplication.manager.id_dedupexists to find them, and aUNIQUE(id_type_id, value)index would fail to build on databases that already have duplicates. The API refuses to create a clash, and refuses to guess on lookup. Caveat: the create check is check-then-insert, so two concurrent POSTs can still create a duplicate.docs/principles/api-error-responses.md):ConsentService.filter_response).access_deniedin bulk export.invalid) IDs: they neither resolve nor block reuse. They are hidden fromidentifier[]and fromidentifier=searches. References andLocationuse a live ID.fields.Datetime.now()at four sites.spp_registry._is_ended_as_ofis unchanged, because it is shared with SQL legs and cron domains.Tests
test_consent_paging(K–M, S1: unit tests forfetch_with_consent/page_total_and_next+ HTTP), K/L/N/O/P/Q tests intest_search_filters_fail_closed,test_patch_api,test_group_api,test_individual_api,test_program_membership_api(incl.TestProgramMembershipPagingAPI),test_program_api,test_scope_enforcement_program;test_search_groups_offset,test_search_offset_pages_through_results,test_search_page_filled_past_consent_denied_groups(J),test_search_filters_fail_closed,test_references_resolvable,test_membership_end_now,test_identifier_ambiguity,test_identifier_ambiguity_paths(spp_api_v2);test_program_membership_identity(spp_api_v2_programs).spp_api_v2739/739,spp_api_v2_programs134/134,spp_studio_api_v2163/163. The last one extends the changed services.test_parse_identifier_paramnow asserts the stricter single-rowanydomain.test_read_program_membership_not_foundandtest_update_program_membership_not_found_returns_404now use a legal-basis client for the 404, astest_read_individual_not_foundalready does. Consent clients get 403, which a new test covers.test_search_with_invalid_beneficiary_format/test_search_with_invalid_program_format(programs) now expect 400 instead of 200: item L's intended contract change.Behaviour changes for API clients
GET /Group?member=lists only current groups.GET /Individual/{id}no longer returns a group.identifier[].?program=, with 403 for unknown beneficiaries to consent clients.meta.totalis the page size; follownextuntil null (a page may be short or empty).beneficiary=/program=onGET /ProgramMembership; 409 for duplicate enrollment; 422 for unknown roles, unknown/foreign gender codes and out-of-range_offset; 403 (not 500) onGET /Programwithout scope.Each change is listed in HISTORY.
Follow-ups (not in this PR)
PUT /ProgramMembership).If-Matchoptional althoughapi-design.mdrequires it.#in Individual/Group/$splitLocationheaders.GET /Group?type=accepted but never applied._create_memberslogs identifier values.Known remaining issues affecting the adaptor (not in this PR)
removeFromGroupthenaddToGroup→ 422 "Duplication of Member"); spp_registry counts ended memberships. Needs a design decision.nextoffsets are database positions, so a consent-filtered client can still count hidden rows (the D10 trade-off); fix is an opaque cursor.exitReasondropped, staleexitDate, no workflow guard) and GroupgroupTypenot stored./_searchpaging,prevlinks, Program cursor,odoo.httperror-log noise, PII in logs,change_requestleaks).POST /Individual|Group/_searchhas no scope check and the old total rule; today it returns nothing because it runs without sudo, so the scope check must land before or with any sudo fix.End-to-end check
The adaptor's QA job (
packages/openspp/tmp/qa-openspp.js) against a local stack on this branch: 50 passed, 0 failed, 0 warnings (on released 19.0: 49 passed, 2 warnings for G and I). Three tests first failed because the QA job adds a member with rolemember, which isn't a vocabulary code; item P now rejects it instead of silently dropping it. Fixed on the adaptor side.