Eng#1: Backend sync reconciliation, metrics, OpenAPI#3
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
| "/api/sync/hygraph": { | ||
| "post": { | ||
| "tags": [ | ||
| "sync" | ||
| ], | ||
| "summary": "Hygraph Webhook", | ||
| "description": "Webhook receiver:\n- HMAC validated (dependency)\n- Single size guard (2MB) already enforced by dependency; body/raw set on request.state\n- DB dedup via SyncEvent(event_id, body_sha256 unique)\n- 202 fast-ack with background processing (pull_all)\n- Structured JSON log line and Prometheus counters", | ||
| "operationId": "hygraph_webhook_api_sync_hygraph_post", | ||
| "responses": { | ||
| "202": { | ||
| "description": "Successful Response", | ||
| "content": { | ||
| "application/json": { |
There was a problem hiding this comment.
Hygraph webhook advertises 202 while implementation returns 200
The specification for POST /api/sync/hygraph describes a 202 success response and “fast-ack with background processing”. The actual handler (backend/api/routes_sync.py) simply returns a dict without setting status_code=202, so FastAPI responds with 200. This mismatch will cause clients following the spec to treat the 200 responses as unexpected and breaks monitoring dashboards that rely on a 202 acknowledgement. Either change the code to return 202 or update the spec to document the 200 behaviour.
Useful? React with 👍 / 👎.
| "/api/sync/hygraph/pull": { | ||
| "post": { | ||
| "tags": [ | ||
| "sync" | ||
| ], | ||
| "summary": "Hygraph Pull", | ||
| "description": "Admin pull:\n- Auth via Bearer token (constant-time compare)\n- Accepts \"type\" or \"sync_type\" + optional \"page_size\"\n- Validates positive page_size and caps inside service (\u2264200)", | ||
| "operationId": "hygraph_pull_api_sync_hygraph_pull_post", | ||
| "requestBody": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "additionalProperties": true, | ||
| "type": "object", | ||
| "title": "Body" | ||
| } | ||
| } | ||
| }, | ||
| "required": true | ||
| }, | ||
| "responses": { | ||
| "200": { | ||
| "description": "Successful Response", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "additionalProperties": true, | ||
| "type": "object", | ||
| "title": "Response Hygraph Pull Api Sync Hygraph Pull Post" | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
OpenAPI lists
/api/sync/hygraph/pull although no such route exists
The spec introduces a secured POST /api/sync/hygraph/pull endpoint, but there is no implementation anywhere in the backend (rg "hygraph/pull" backend yields nothing). Publishing a non-existent admin sync endpoint will lead operators to 404s and erodes trust in the spec. Either add the pull route or remove it from the OpenAPI document.
Useful? React with 👍 / 👎.
| "/metrics": { | ||
| "get": { | ||
| "summary": "Metrics", | ||
| "operationId": "metrics_metrics_get", | ||
| "responses": { | ||
| "200": { | ||
| "description": "Successful Response", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": {} | ||
| } |
There was a problem hiding this comment.
/metrics path documented without an implemented Prometheus endpoint
The new OpenAPI document exposes a GET /metrics path returning JSON, but the backend codebase contains no route or instrumentation for /metrics (rg "metrics" backend finds nothing). Requests to this path will return 404 and monitoring tooling configured from the spec will fail. Either instrument the app with a metrics endpoint or remove the path from the spec.
Useful? React with 👍 / 👎.
Eng#1 — Backend sync & operability
PR checklist
Notes
Summary by CodeRabbit