Refactor CNC export endpoint to new exports router#519
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
WalkthroughThe CNC export endpoint is restructured and secured. The API path changes from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes follow a consistent refactoring pattern across multiple files (path updates and authentication additions), but require verification that all test coverage aligns with the new endpoint structure and security model. The exports.py file introduces some logic changes (service naming, error handling) that warrant separate attention. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @shayancoin. * #519 (comment) The following files were modified: * `backend/api/exports.py` * `backend/tests/test_error_envelopes.py` * `backend/tests/test_routes_quote_cnc.py` * `tests/perf/k6-quote-cnc.js`
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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 "@codex address that feedback".
| exportCNC(configuration_id: string, modules: { module_id: string; quantity: number }[]) { | ||
| return this.request('/api/cnc/export', 'POST', { configuration_id, modules }); | ||
| return this.request('/api/v1/exports/cnc', 'POST', { configuration_id, modules }); |
There was a problem hiding this comment.
The backend router now enforces require_write_token for /api/v1/exports/cnc, which validates requests against the static API_WRITE_TOKEN (see exports.py). ApiClient.exportCNC still forwards only the Supabase session token via getSessionToken() and provides no way to supply the write token. Any frontend invocation of this method will therefore receive a 403 regardless of user authentication. Either the client needs to inject the write token, or the endpoint must accept Supabase tokens instead.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
backend/api/exports.py(5 hunks)backend/api/main.py(2 hunks)backend/tests/test_error_envelopes.py(1 hunks)backend/tests/test_routes_quote_cnc.py(2 hunks)docs/API_SPEC.md(1 hunks)docs/ARCHITECTURE.md(1 hunks)docs/plan/paform-fastapi-extension-v0-1-plan.md(1 hunks)frontend/src/api/client.ts(1 hunks)tests/perf/k6-quote-cnc.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
backend/tests/test_routes_quote_cnc.py (1)
tests/perf/k6-quote-cnc.js (2)
headers(18-21)headers(100-103)
backend/tests/test_error_envelopes.py (1)
tests/perf/k6-quote-cnc.js (2)
headers(18-21)headers(100-103)
backend/api/exports.py (7)
backend/api/security.py (1)
require_write_token(75-100)backend/api/storage_dependencies.py (1)
get_storage_client(11-17)backend/services/storage.py (1)
StorageClient(22-31)backend/services/cnc_service.py (2)
CNCService(52-190)export(98-141)backend/api/schemas.py (3)
CNCExportResponse(127-130)CNCExportRequest(122-124)Module(80-87)backend/api/db.py (1)
get_db(62-68)frontend/src/api/client.ts (1)
request(36-58)
🔇 Additional comments (13)
docs/plan/paform-fastapi-extension-v0-1-plan.md (1)
115-115: Documentation updated correctly.The endpoint path change is properly reflected in the planning document.
docs/API_SPEC.md (1)
754-760: OpenAPI spec properly updated.The path, tag, and operationId are all correctly updated to reflect the new exports router structure.
tests/perf/k6-quote-cnc.js (1)
112-112: Endpoint path correctly updated.The CNC export now targets the new authenticated
/api/v1/exports/cncendpoint.docs/ARCHITECTURE.md (1)
22-22: Architecture documentation synchronized.The endpoint path is correctly updated to match the new exports router structure.
backend/api/main.py (1)
31-31: Router replacement executed correctly.The import and wiring of the new
exports_routerproperly replaces the oldcnc_router, enabling the new/api/v1/exportsprefix and authentication requirements.Also applies to: 133-133
backend/tests/test_routes_quote_cnc.py (2)
9-9: Good practice using a module-level constant for auth headers.Defining
AUTH_HEADERSat the module level promotes reusability and maintainability across test functions.
115-122: CNC export test properly updated for authenticated endpoint.The test now targets
/api/v1/exports/cncwith the required Authorization header, correctly validating the new endpoint behavior.backend/api/exports.py (6)
1-1: LGTM: Imports properly organized for exports router.All new imports are necessary and correctly organized for the refactored exports functionality.
Also applies to: 12-13, 24-24, 27-27
30-34: Excellent: Router-level write token authentication.The router configuration properly enforces authentication for all exports endpoints via the
dependenciesparameter. This is the correct approach for securing the entire router.
35-35: LGTM: Clean service instance and endpoint path.The module-level
_cnc_serviceinstance is appropriate since CNCService is stateless. The underscore prefix correctly signals internal usage, and the new endpoint path/cnccombines with the router prefix to form/api/v1/exports/cnc.Also applies to: 38-38, 45-45
52-52: LGTM: Improved variable naming and service call.The change from
mtomodulein the dict comprehension improves readability, and the service call correctly references the renamed_cnc_serviceinstance.Also applies to: 55-57, 59-59
121-125: LGTM: Proper error handling with exception chaining.The error handling correctly re-raises HTTPException instances while catching and wrapping unexpected exceptions in HTTP 500 responses. The logging and exception chaining provide good observability.
1-125: Verify that tests pass before merging.The PR objectives note a test failure due to "ImportError initializing SQLAlchemy models in the test environment." Before merging, please confirm that:
- The ImportError has been resolved
- Both test files pass:
backend/tests/test_routes_quote_cnc.pyandbackend/tests/test_error_envelopes.py- The new endpoint
/api/v1/exports/cncworks correctly with the write token authenticationRun the following to verify the tests pass:
| def test_cnc_invalid_payload_envelope() -> None: | ||
| response = client.post( | ||
| '/api/cnc/export', | ||
| '/api/v1/exports/cnc', | ||
| content='not-json', | ||
| headers={'Content-Type': 'application/json'}, | ||
| headers={ | ||
| 'Content-Type': 'application/json', | ||
| 'Authorization': 'Bearer test-write-token', | ||
| }, | ||
| ) | ||
| assert response.status_code in (400, 422) | ||
| assert _is_error_shape(response.json()) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test updated correctly for new authenticated endpoint.
The test now targets /api/v1/exports/cnc with the required Authorization header. The changes properly validate that error envelopes work with the new endpoint.
For consistency with backend/tests/test_routes_quote_cnc.py (which defines AUTH_HEADERS at the module level), consider extracting the headers to a module-level constant:
AUTH_HEADERS = {
'Content-Type': 'application/json',
'Authorization': 'Bearer test-write-token',
}🤖 Prompt for AI Agents
In backend/tests/test_error_envelopes.py around lines 30 to 40, extract the
inline headers dict into a module-level constant named AUTH_HEADERS and replace
the inline headers=... usage in the test with headers=AUTH_HEADERS; ensure
AUTH_HEADERS contains the same keys and values ('Content-Type':
'application/json', 'Authorization': 'Bearer test-write-token') and is defined
at the top of the file for consistency with other tests.
| exportCNC(configuration_id: string, modules: { module_id: string; quantity: number }[]) { | ||
| return this.request('/api/cnc/export', 'POST', { configuration_id, modules }); | ||
| return this.request('/api/v1/exports/cnc', 'POST', { configuration_id, modules }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify authentication token compatibility.
The endpoint path is correctly updated to /api/v1/exports/cnc. However, the backend now requires a write token (via require_write_token dependency), while the frontend client uses getSessionToken() at lines 40-43.
Ensure that the session token returned by getSessionToken() is accepted by the require_write_token dependency on the backend. If they are different token types (e.g., user session token vs. API write token), this could cause 401/403 errors.
Run the following script to verify how require_write_token is implemented and whether it accepts session tokens:
🏁 Script executed:
#!/bin/bash
# Search for the require_write_token dependency implementation
rg -A 10 "def require_write_token"
# Search for getSessionToken implementation in frontend
rg -A 10 "getSessionToken" frontend/src/lib/Length of output: 2941
🏁 Script executed:
#!/bin/bash
# View the request method and lines 40-43 in client.ts to see token usage
sed -n '30,50p' frontend/src/api/client.ts
# Get complete require_write_token implementation
rg -A 30 "async def require_write_token" backend/api/security.py
# Search for how the Authorization header is set in request method
rg -B 5 -A 15 "Authorization.*Bearer\|headers.*authorization" frontend/src/api/client.tsLength of output: 1759
🏁 Script executed:
#!/bin/bash
# Find _load_expected_write_token implementation
rg -B 3 -A 10 "_load_expected_write_token"Length of output: 6060
🏁 Script executed:
#!/bin/bash
# Find the exportCNC endpoint route definition
rg -B 5 -A 10 "exports/cnc" backend/Length of output: 2077
🏁 Script executed:
#!/bin/bash
# Find the route handler definition for POST /api/v1/exports/cnc
rg -B 5 "@.*post\|@router\.post" backend/ | grep -A 15 "exports/cnc"
# Also search more broadly for the endpoint definition
rg -B 3 "def.*cnc\|def.*export" backend/api/ | head -60Length of output: 0
🏁 Script executed:
#!/bin/bash
# List Python files in backend/api to understand structure
fd "\.py$" backend/api/ | head -20
# Search for "exports" or "cnc" in function definitions
rg "def .*(export|cnc)" backend/api/ -A 5
# Search for "require_write_token" usage as dependency injection
rg "require_write_token" backend/ -B 5 -A 2Length of output: 3563
Frontend client must use API write token, not session token.
The /api/v1/exports/cnc endpoint enforces require_write_token at the router level (backend/api/exports.py), which validates the Authorization header against the configured API_WRITE_TOKEN. However, the frontend client constructs the header with getSessionToken(), which returns the user session token from the auth provider—a fundamentally different token type.
The backend will reject session tokens with 403 Forbidden. Either:
- Update the frontend to use a configured write token instead of session tokens, or
- Change the backend endpoint to accept session tokens if that is the intended authentication model
The current code will fail at runtime.
| const headers = { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: 'Bearer ' + (__ENV.API_WRITE_TOKEN || 'test-write-token'), | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting the auth headers to reduce duplication.
The same headers object is defined twice (lines 18-21 and 100-103). While this works correctly, extracting it to a constant at the module level would improve maintainability.
+const AUTH_HEADERS = {
+ 'Content-Type': 'application/json',
+ Authorization: 'Bearer ' + (__ENV.API_WRITE_TOKEN || 'test-write-token'),
+};
+
function createMaterialAndModule() {
const suffix = `${Date.now()}-${Math.floor(Math.random() * 1e6)}`;
- const headers = {
- 'Content-Type': 'application/json',
- Authorization: 'Bearer ' + (__ENV.API_WRITE_TOKEN || 'test-write-token'),
- };
const materialRes = http.post(
`${baseUrl}/api/materials/`,
JSON.stringify({
name: `Walnut-${suffix}`,
texture_url: null,
cost_per_sq_ft: 12.5,
}),
- { headers }
+ { headers: AUTH_HEADERS }
);Also applies to: 100-103
🤖 Prompt for AI Agents
In tests/perf/k6-quote-cnc.js around lines 18-21 and 100-103, the
Authorization/Content-Type headers object is duplicated; extract that object to
a single module-level constant (e.g., const AUTH_HEADERS = { 'Content-Type':
'application/json', Authorization: 'Bearer ' + (__ENV.API_WRITE_TOKEN ||
'test-write-token') };) and replace the two inline definitions with references
to this constant so both request uses share the same source of truth.
) Docstrings generation was requested by @shayancoin. * #519 (comment) The following files were modified: * `backend/api/exports.py` * `backend/tests/test_error_envelopes.py` * `backend/tests/test_routes_quote_cnc.py` * `tests/perf/k6-quote-cnc.js` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68f72bf91d588330ab1f5a19a21732c6
Summary by CodeRabbit
New Features
Documentation
/api/cnc/exportto/api/v1/exports/cnc.Refactor