feat(spp_api_v2): OpenAPI polymorphic bodies, OAuth2 scheme in auth middleware, bundle schemas (re-land from #76)#276
Conversation
There was a problem hiding this comment.
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.
… 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.
dc9fec1 to
c534549
Compare
|
gemini-code-assist disposition: Applied: Bearer-prefix strip now also trims whitespace ( Deferred: |
…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.
|
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. |
|
Reviewed with a focus on downstream modules that extend the 1. 2. Relative 3. Auth is slightly more lenient now (informational). 4. Silent wrong- Nits:
Test coverage is good — the |
- BundleEntry stays generic (other modules reuse it for non-registrant resources, e.g. Products); registrant endpoints (batch, filter preview) now document Individual|Group via new RegistrantBundle/RegistrantBundleEntry subtypes, so the published spec is truthful for every reuser. - The advertised OAuth2 tokenUrl is absolutized against the endpoint's root_path at OpenAPI generation time; strict RFC 3986 clients previously resolved the relative form to a 404. Contract test added asserting securitySchemes and the resolved URL. - Polymorphic hook logs a warning on schema-name collisions instead of silently keeping a wrong $ref target. - HISTORY documents accepted Authorization forms (Bearer-prefixed and raw). - Nits: relative import in bundle schemas, drop misleading import-order comment, reword test docstrings to be self-contained.
|
Review disposition — all four findings addressed in
Nits all folded in (relative import, order-comment dropped, test docstrings reworded). |
emjay0921
left a comment
There was a problem hiding this comment.
Re-review — Approved ✅
Both prior (gemini) findings verified fixed in the actual code, not just acknowledged:
- OpenAPI metadata loss (
utils/openapi_polymorphic.py) — the hook now builds via the app's own generator (original_openapi()) and injects only missing$refed models, socontact/license_info/servers/tagsare preserved (10ec1d9a). - Bearer token whitespace (
middleware/auth.py) —token = token[7:].strip()(802e08d3).
Also checked:
- Auth is sound — JWT pinned to
HS256(no alg-confusion),audience+issuerboth validated, secret-strength gate, fail-closed 401s. No bypass. - OpenAPI hook composition — polymorphic hook (inner) +
absolutize_oauth_token_urls(outer) compose correctly and the tokenURL rewrite is idempotent. - Bundle refactor —
RegistrantBundle/RegistrantBundleEntrysubclass the genericBundle(kept for other modules); the polymorphicresourcekeeps itsdictruntime type. - CI green, version bumped (
19.0.2.0.1 → 19.0.2.1.0) with a HISTORY entry.
Good to have (non-blocking follow-ups)
- A regression test asserting the polymorphic hook preserves app metadata (
contact/license_info/servers) — the exact thing the first fix restored; nothing currently pins it. - A unit test exercising
get_authenticated_clientwith a"Bearer <token>"header to pin the token-strip fix. absolutize_oauth_token_urlsonly rewritestokenUrl— fine forclientCredentials, but would needauthorizationUrl/refreshUrlif other flows are added later.batch.py's route could addresponse_model_exclude_none=Trueto match thefilter.pysearch routes (avoids emitting nulls in batch responses).
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.BundleEntry.resourcebecomes a polymorphic Individual/Group body; schema import-order fix.Added on top of #76 (not in the 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 directget_openapi()call dropped; the Bearer-prefix strip also trims surrounding whitespace.Note:
tests/test_search_service.pyname-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)