fix: DCI envelope support for disability registry search (#217)#303
fix: DCI envelope support for disability registry search (#217)#303Tarekchehahde wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the sync search endpoints for the Disability, Civil, and Farmer registries to accept and return DCIEnvelope wrappers, introducing a helper function _build_stub_search_envelope to generate unimplemented stub responses. It also updates the corresponding tests and replaces deprecated utcnow() calls with timezone-aware now(UTC). Feedback includes a recommendation to use the existing build_signed_envelope helper to reduce boilerplate and ensure proper signing, as well as a correction for an incorrect path in the disability_sync_notify docstring.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| our_sender_id = get_sender_id(env) or "openspp" | ||
| total_count = len(response_items) | ||
|
|
||
| callback_header = DCICallbackHeader( | ||
| version=request_envelope.header.version, | ||
| message_id=str(uuid.uuid4()), | ||
| message_ts=datetime.now(UTC), | ||
| action=f"on-{request_envelope.header.action}", | ||
| sender_id=our_sender_id, | ||
| receiver_id=request_envelope.header.sender_id, | ||
| total_count=total_count, | ||
| status="rjct", | ||
| status_reason_code=MsgHeaderStatusReasonCode.ACTION_NOT_SUPPORTED.value, | ||
| status_reason_message=not_implemented_message, | ||
| completed_count=0, | ||
| ) | ||
|
|
||
| return DCIEnvelope( | ||
| signature="", | ||
| header=callback_header, | ||
| message=search_response.model_dump(mode="json", exclude_none=True), | ||
| ) |
There was a problem hiding this comment.
Instead of manually constructing the DCICallbackHeader and returning an unsigned DCIEnvelope (with signature=""), you can leverage the existing build_signed_envelope helper from spp_dci.services.response_helpers. This reduces boilerplate, ensures consistency, and automatically signs the envelope if a signing key is configured in the environment.
from odoo.addons.spp_dci.services.response_helpers import build_signed_envelope
return build_signed_envelope(
env=env,
original_header=request_envelope.header,
response_message=search_response.model_dump(mode="json", exclude_none=True),
status_code="rjct",
status_reason_code=MsgHeaderStatusReasonCode.ACTION_NOT_SUPPORTED.value,
status_reason_message=not_implemented_message,
completed_count=0,
)| SPDCI-compliant Disability Registry notification endpoint. | ||
|
|
||
| Path: /disability/registry/sync/notify | ||
| Path: /disability/registry/sync/search |
There was a problem hiding this comment.
The path in the docstring was incorrectly changed to /disability/registry/sync/search. Since this is the notification endpoint (disability_sync_notify), it should remain /disability/registry/sync/notify.
| Path: /disability/registry/sync/search | |
| Path: /disability/registry/sync/notify |
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Thanks for the contribution — aligning the stub aliases with the DCIEnvelope schema used by the real search endpoints is a legitimate improvement, and we verified it works as described: we applied the diff to current 19.0 on a local instance, replayed the exact two-item payload from #217 against /disability/registry/sync/search, and got a well-formed on-search envelope with per-item rjct and preserved reference_id correlation. The full spp_dci_server suite also passes with the patch applied (332 tests, 0 failures).
That said, requesting changes for the points below.
1. This PR does not resolve #217 — please change Fixes #217 to Related to #217
This is the important one. The 500 reported in #217 was observed on the demo server (openspp-dci-demo-dr), which runs a disability search implementation that is not the stub this PR modifies (see the triage in #217 (comment) — the stub in this repo returns a deterministic 422/501 for those payloads and cannot 500). Merging with Fixes #217 would auto-close the issue while the actual bug persists on the deployment. The issue needs to stay open, so the PR description should link it as Related to #217.
2. Docstring regression in disability_sync_notify
The docstring path was changed from /disability/registry/sync/notify to /disability/registry/sync/search — this is the notify endpoint, so the original path was correct. (The Gemini review flagged this too.)
3. Reuse build_signed_envelope instead of hand-rolling the envelope
spp_dci.services.response_helpers.build_signed_envelope already constructs exactly this callback envelope (header, on-<action>, reason codes) and signs it with the server's active key. The new _build_stub_search_envelope duplicates that logic and always returns signature="". Signed responses matter here — the #217 reporter is specifically trying to verify response signatures. Please build the response via the existing helper.
4. Repo policy: version bump + changelog + README
Every code change to a module needs, in the same PR:
- a patch bump of
versioninspp_dci_server/__manifest__.py(5-segment19.0.x.y.z); - a
readme/HISTORY.mdfragment (OCA style,### <version>newest-first); - regenerated
README.rst/static/description/index.html(let CI's pre-commit job print the expected diff and apply it verbatim rather than regenerating locally).
CI will fail without these.
5. Document the deliberate contract changes in the PR description
The change flips two behaviors for all three registries (disability, crvs, farmer), and neither is mentioned in the description:
- HTTP status for the stub response changes 501 → 200 (with the rejection expressed DCI-style in the envelope/per-item statuses) — we think this is the right call, but it should be stated as a deliberate decision;
- bare
SearchRequestpayloads that previously received 501 now fail validation with 422. Blast radius is likely nil (integrating against a not-implemented stub is unlikely), but it's still a request-contract change and should be called out.
Happy to re-review once these are addressed — the core of the change is verified and welcome.
|
Thanks @gonzalesedwin1123 — addressed all five review points in 8b696d9:
Ready for re-review. |
Disability/crvs/farmer sync/search stubs now accept DCIEnvelope requests and return signed per-item on-search envelopes via build_signed_envelope. Bump to 19.0.2.0.4 with HISTORY/README updates. Related to OpenSPP#217.
8b696d9 to
36fc397
Compare
|
@gonzalesedwin1123 Addressed all five review points:
Ready for re-review. |
2 participants
Summary
/sync/searchstub endpoints to acceptDCIEnvelope(matching client integrations and/registry/sync/search).on-searchenvelope viabuild_signed_envelope()with onesearch_responseitem per request item (multi-item batch no longer fails validation).Contract changes (deliberate)
rjctstatuses.SearchRequestpayloads (without aDCIEnvelopewrapper) now fail FastAPI validation with 422 instead of reaching the old 501 stub.Test plan
./spp test spp_dci_server—TestRegistryAliasStubs.test_search_stubs_return_per_item_rjct/disability/registry/sync/searchreturns 200 + signed per-itemrjct(until disability module ships)Related to #217
Merge order (vs security PR #325)
Per the cross-PR interaction analysis (2026-07-24): this PR and #325 both bump
spp_dci_serverto the identical19.0.2.0.4— that manifest line auto-merges with no conflict marker. Either merge order works (code is fully disjoint: this PR is router-level, #325 is middleware that runs before any router), but whichever merges second must deliberately re-bump to19.0.2.0.5and move its HISTORY bullet under the new heading.