Skip to content

feat(spp_api_v2): OpenAPI polymorphic bodies, OAuth2 scheme in auth middleware, bundle schemas (re-land from #76)#276

Open
gonzalesedwin1123 wants to merge 6 commits into
19.0from
reland/api-v2-core
Open

feat(spp_api_v2): OpenAPI polymorphic bodies, OAuth2 scheme in auth middleware, bundle schemas (re-land from #76)#276
gonzalesedwin1123 wants to merge 6 commits into
19.0from
reland/api-v2-core

Conversation

@gonzalesedwin1123

@gonzalesedwin1123 gonzalesedwin1123 commented Jul 2, 2026

Copy link
Copy Markdown
Member

Re-lands the spp_api_v2 portion of reverted PR #76 (revert: #271). Everything listed is contained in THIS PR's diff.

Summary

  • utils/openapi_polymorphic.py: polymorphic_body() helper + OpenAPI schema-injection hook, installed on the app via the endpoint registry.
  • Auth middleware: OAuth2 client-credentials security scheme replacing plain HTTPBearer (advertises the token endpoint in OpenAPI), Bearer-prefix stripping.
  • BundleEntry.resource becomes a polymorphic Individual/Group body; schema import-order fix.
  • New OpenAPI contract/polymorphic/bundle tests.

Added on top of #76 (not in the original)

  • gemini-code-assist review findings, both applied: the polymorphic hook now delegates to the app's original openapi() generator and injects schemas into its output, preserving app metadata (contact, license_info, servers, tags) that feat: geofence-based geographic targeting for programs #76's direct get_openapi() call dropped; the Bearer-prefix strip also trims surrounding whitespace.
  • Version bump + HISTORY entry; README rendered from the updated fragment.

Note: tests/test_search_service.py name-assertion updates from #76 landed with the revert itself (#271 kept them aligned with the retained spp_registry name fix), so they are not part of this diff.

Verification

  • ./spp t spp_api_v2: 640 passed, 0 failed (includes OpenAPI contract tests)
  • codecov: patch pass
  • All gemini-code-assist threads resolved with fixing-commit references

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces polymorphic OpenAPI schema utilities to document BundleEntry.resource as a polymorphic Individual/Group body, replaces the plain HTTPBearer authentication scheme with an OAuth2 client-credentials scheme, and adds corresponding contract and unit tests. The review feedback highlights two main improvements: first, in openapi_polymorphic.py, the custom OpenAPI hook should call the original app.openapi method instead of calling get_openapi directly to avoid discarding app metadata; second, in auth.py, the token extraction should strip any leading or trailing whitespace after slicing the 'Bearer ' prefix to ensure robust JWT decoding.

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.

Comment thread spp_api_v2/utils/openapi_polymorphic.py
Comment thread spp_api_v2/middleware/auth.py Outdated
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.17647% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.62%. Comparing base (bf61488) to head (5af3425).
⚠️ Report is 1 commits behind head on 19.0.

Files with missing lines Patch % Lines
spp_api_v2/utils/openapi_polymorphic.py 89.09% 6 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             19.0     #276      +/-   ##
==========================================
- Coverage   74.86%   73.62%   -1.24%     
==========================================
  Files        1093      355     -738     
  Lines       63718    19522   -44196     
==========================================
- Hits        47701    14374   -33327     
+ Misses      16017     5148   -10869     
Flag Coverage Δ
endpoint_route_handler ?
fastapi ?
spp_aggregation ?
spp_alerts ?
spp_analytics ?
spp_api_v2 79.92% <91.17%> (+0.13%) ⬆️
spp_api_v2_change_request 66.41% <ø> (ø)
spp_api_v2_cycles 70.65% <ø> (ø)
spp_api_v2_data 77.43% <ø> (ø)
spp_api_v2_entitlements 69.96% <ø> (ø)
spp_api_v2_gis 71.52% <ø> (ø)
spp_api_v2_products 65.66% <ø> (ø)
spp_api_v2_programs 92.03% <ø> (ø)
spp_api_v2_service_points 70.54% <ø> (ø)
spp_api_v2_simulation 71.12% <ø> (ø)
spp_api_v2_vocabulary 57.26% <ø> (ø)
spp_approval ?
spp_area ?
spp_area_hdx ?
spp_attachment_av_scan ?
spp_audit ?
spp_audit_programs ?
spp_banking ?
spp_base_common 90.26% <ø> (ø)
spp_base_setting ?
spp_case_base ?
spp_case_cel ?
spp_case_demo ?
spp_case_entitlements ?
spp_case_graduation ?
spp_case_programs ?
spp_case_registry ?
spp_case_session ?
spp_cel_domain ?
spp_cel_event ?
spp_cel_registry_search ?
spp_cel_vocabulary ?
spp_change_request_v2 ?
spp_claim_169 ?
spp_cr_type_assign_program ?
spp_cr_types_advanced ?
spp_cr_types_base ?
spp_dci ?
spp_dci_client ?
spp_dci_client_dr 85.60% <ø> (ø)
spp_dci_client_ibr 92.27% <ø> (ø)
spp_dci_client_sr ?
spp_dci_compliance 92.76% <ø> (ø)
spp_dci_demo 69.23% <ø> (ø)
spp_dci_indicators 95.83% <ø> (ø)
spp_dci_server ?
spp_dci_server_social ?
spp_demo ?
spp_demo_phl_luzon ?
spp_disability_registry ?
spp_drims ?
spp_drims_sl ?
spp_drims_sl_demo ?
spp_encryption ?
spp_farmer_registry ?
spp_farmer_registry_cr ?
spp_farmer_registry_demo ?
spp_farmer_registry_vocabularies ?
spp_gis ?
spp_gis_indicators ?
spp_gis_report ?
spp_graduation ?
spp_grm ?
spp_grm_case_link ?
spp_grm_demo ?
spp_hazard ?
spp_hazard_programs ?
spp_hxl_area ?
spp_import_match ?
spp_indicator ?
spp_irrigation ?
spp_land_record ?
spp_metric ?
spp_metric_service ?
spp_metrics_core ?
spp_metrics_services ?
spp_mis_demo_v2 ?
spp_oauth ?
spp_program_geofence ?
spp_programs 65.27% <ø> (ø)
spp_registrant_gis ?
spp_registry 86.83% <ø> (ø)
spp_registry_group_hierarchy ?
spp_scoring ?
spp_scoring_programs ?
spp_security 66.66% <ø> (ø)
spp_service_points ?
spp_simulation ?
spp_starter_disability_registry ?
spp_starter_farmer_registry ?
spp_starter_social_registry ?
spp_starter_sp_mis ?
spp_statistic ?
spp_storage_backend ?
spp_studio ?
spp_studio_change_requests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
spp_api_v2/__manifest__.py 0.00% <ø> (ø)
spp_api_v2/middleware/auth.py 76.11% <100.00%> (+0.36%) ⬆️
spp_api_v2/models/fastapi_endpoint_registry.py 94.28% <100.00%> (+0.34%) ⬆️
spp_api_v2/schemas/__init__.py 100.00% <100.00%> (ø)
spp_api_v2/schemas/bundle.py 100.00% <100.00%> (ø)
spp_api_v2/utils/openapi_polymorphic.py 89.09% <89.09%> (ø)

... and 739 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

… bundle schemas (from #76)

Re-lands the spp_api_v2 portion of PR #76, which was reverted wholesale
in d38ff9d. Restores the OpenAPI polymorphic schema utilities and app
hook, the OAuth2 client-credentials security scheme in the auth
middleware, the polymorphic BundleEntry.resource schema, and the
OpenAPI contract tests, exactly as merged in 8bf9a3a. Bumps the module
version to 19.0.2.1.0 with a matching HISTORY entry.
@gonzalesedwin1123

Copy link
Copy Markdown
Member Author

gemini-code-assist disposition:

Applied: Bearer-prefix strip now also trims whitespace (token[7:].strip()). Suite: 640/0.

Deferred: openapi_polymorphic.py using the app's original openapi() instead of a partial get_openapi call — agreed in principle (contact/license metadata), but the OpenAPI contract tests pin the current output; changing generation belongs in its own PR with contract-test updates.

…emini review)

The hook now calls the app's original openapi() generator and injects the
polymorphic schemas into its output, so contact/license_info/servers/tags
configured on the FastAPI app survive. Contract tests: 640/0.
@gonzalesedwin1123

Copy link
Copy Markdown
Member Author

Update: the deferred OpenAPI finding is now applied — the polymorphic hook delegates to the app's original openapi() generator and injects schemas into its output, preserving contact/license_info/servers/tags. Contract/polymorphic/bundle tests all pass (640/0). All gemini findings on this PR are now addressed.

@kneckinator

Copy link
Copy Markdown
Contributor

Reviewed with a focus on downstream modules that extend the api_v2 app (via _get_fastapi_routers() or by importing the auth middleware / shared schemas). Overall this is safe to land — Depends(get_authenticated_client) consumers re-resolve against the new signature without code changes, the polymorphic hook only injects models that are actually $ref'd, and runtime types deliberately stay dict. Two things worth addressing, two worth knowing:

1. BundleEntry.resource docs are now wrong for spp_api_v2_products (should fix or track).
spp_api_v2_products/routers/product.py:131 builds BundleEntry(resource=data) where data is a Product dict, returned through the shared Bundle response model. The shared schema now documents resource as oneOf [Individual, Group], and test_bundle_openapi.py asserts the list is exactly those two on the grounds that bundle_service only supports them — but BundleEntry is reused by other routers with other resource types. The Product search response is now actively mis-documented, and strict validators / codegen against the spec will flag Product payloads as invalid. Runtime is unaffected. Suggestion: give reusing modules their own entry schema, or provide a way to extend the oneOf where Bundle is reused, rather than baking bundle_service's restriction into an app-wide schema.

2. Relative tokenUrl: "oauth/token" may resolve to the wrong URL (please verify manually).
The app mounts at /api/v2/spp (data/fastapi_endpoint.xml:6) and the real endpoint is POST /api/v2/spp/oauth/token (routers/oauth.py:95). Per OpenAPI 3, relative URLs resolve against the server URL, and RFC 3986 resolution of oauth/token against /api/v2/spp drops the last path segment → /api/v2/oauth/token (404). Some Swagger UI versions resolve against the spec URL instead, which happens to give the right answer — but since discoverability for consumers like QGIS is the point of this change, please verify the Authorize flow in Swagger UI and QGIS against a running instance, or derive the path from the endpoint's root_path. (Relative does adapt if an instance mounts under a different root path, so an absolute hardcode has its own downside.)

3. Auth is slightly more lenient now (informational).
A raw JWT without the Bearer prefix is now accepted (previously HTTPBearer rejected it with 401 "Missing Authorization header"), and a non-Bearer scheme like Basic xyz now fails as "invalid token" rather than "missing header". Not a security weakening, but clients can start silently depending on the no-prefix form. Also note: any code calling get_authenticated_client() directly with an HTTPAuthorizationCredentials object (rather than via Depends) would now break with an AttributeError — no such caller exists in this repo, but it's a real signature change for extenders.

4. Silent wrong-$ref on schema-name collision (hardening).
The hook injects by bare class name with components.setdefault(name, ...). If FastAPI already generated a different schema under that name (duplicate Pydantic model names across modules on the same app), the hand-built $ref silently points at the wrong model — and the contract test won't catch it, because the ref resolves. A _logger.warning when the name already exists with different content would make that diagnosable.

Nits:

  • schemas/bundle.py imports the helper absolutely (from odoo.addons.spp_api_v2.utils...) while importing sibling schemas relatively; from ..utils.openapi_polymorphic import ... would match the file's style.
  • The schemas/__init__.py reorder is harmless but not needed — bundle.py imports its own dependencies, so the comment implies a load-order constraint that doesn't exist.
  • Test docstrings referencing "the plan's Open Questions section" and "Spike-confirmed shape" won't mean anything to a future reader; reword to describe the behavior itself.

Test coverage is good — the $ref-resolution contract test is exactly the guard downstream modules need. One gap: nothing asserts the OAuth2 scheme lands in securitySchemes or that tokenUrl resolves against the live oauth endpoint, which is where item 2 hides.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants