OpenAI conversation: Standardizing API response#329
Conversation
WalkthroughRefactors response construction in backend/app/api/routes/responses.py to build custom success and error payloads that merge additional request data via get_additional_data. Updates tests to validate inclusion of callback_url in data and confirm metadata=None in specific error scenarios. No public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant API as /responses
participant U as get_additional_data
C->>API: POST /responses (payload)
API->>API: Build request_dict
API->>U: Extract additional_data
U-->>API: additional_data
alt Valid API key / success
API-->>C: 200 { data: {status, message, ...additional_data}, success:true }
else Missing/invalid API key
API-->>C: 401 { success:false, data: additional_data|null, error: message, metadata:null }
end
sequenceDiagram
participant C as Client
participant API as /responses/sync
participant U as get_additional_data
C->>API: POST /responses/sync (payload)
API->>API: Build request_dict
API->>U: Extract additional_data
U-->>API: additional_data
alt Success
API-->>C: 200 { ...APIResponse, ...additional_data }
else Error
API-->>C: 4xx/5xx { success:false, data: additional_data|null, error: message, metadata:null }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
AkhileshNegi
force-pushed
the
enhancement/consistent-client-context
branch
from
f7319d6 to
2a2f082
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
backend/app/tests/api/routes/test_responses.py (1)
110-296: Includecallback_urlin the response payloadThe
get_additional_datahelper is currently excluding"callback_url", so none of the/responsesbranches ever include it in the returneddatafield—causing the existing tests to fail. To satisfy the tests’ expectations:• In backend/app/api/routes/responses.py, locate the
get_additional_datafunction (around line 103)
• Remove"callback_url"from theasync_exclude_keysset (currently on line 106) so that it remains in the returned dict
• Verify all/responsesreturn paths (success, credential-missing, error) now spreadadditional_datacontainingcallback_urlinto their"data"objectsAfter making this change, rerun the
test_responses.pysuite to confirm thecallback_urlassertions pass.
🧹 Nitpick comments (1)
backend/app/api/routes/responses.py (1)
103-124: Consider a more robust request type detection mechanism.The current implementation determines request type by checking for the presence of
assistant_id, which could be fragile if request structures change in the future.Consider passing an explicit request type parameter or using isinstance checks:
-def get_additional_data(request: dict) -> dict: +def get_additional_data(request: dict, request_type: str = None) -> dict: """Extract additional data from request, excluding specific keys.""" # Keys to exclude for async request (ResponsesAPIRequest) async_exclude_keys = {"assistant_id", "callback_url", "response_id", "question"} # Keys to exclude for sync request (ResponsesSyncAPIRequest) sync_exclude_keys = { "model", "instructions", "vector_store_ids", "max_num_results", "temperature", "response_id", "question", } # Determine which keys to exclude based on the request structure - if "assistant_id" in request: + if request_type == "async" or (request_type is None and "assistant_id" in request): exclude_keys = async_exclude_keys else: exclude_keys = sync_exclude_keys return {k: v for k, v in request.items() if k not in exclude_keys}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/api/routes/responses.py(7 hunks)backend/app/tests/api/routes/test_responses.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/tests/api/routes/test_responses.py (1)
backend/app/tests/api/routes/test_threads.py (1)
test_process_run_variants(89-132)
🪛 GitHub Actions: AI Platform CI
backend/app/tests/api/routes/test_responses.py
[error] 1-1: Test 'test_responses_endpoint_success' failed: response data did not include 'callback_url' as expected; data contained only 'status' and 'message'.
[error] 1-1: Test 'test_responses_endpoint_without_vector_store' failed: response data did not include 'callback_url' as expected; data contained only 'status' and 'message'.
[error] 1-1: Test 'test_responses_endpoint_no_openai_credentials' failed: response data is None in error path; test attempts to check for 'callback_url' in data.
[error] 1-1: Test 'test_responses_endpoint_missing_api_key_in_credentials' failed: response data is None in error path; test attempts to check for 'callback_url' in data.
🔇 Additional comments (11)
backend/app/api/routes/responses.py (7)
244-258: LGTM: Success path correctly merges additional data.The success response construction properly includes additional request data via the spread operator, maintaining backward compatibility while extending the response structure.
268-276: Consistent error response structure with metadata standardization.The error response now uses a standardized structure with
metadata=Noneand includes additional data in thedatafield. This aligns with the PR objective of standardizing API responses.
321-327: LGTM: Error path standardization implemented correctly.The error response now properly extracts and includes additional request data in the
datafield while settingmetadata=None, consistent with the standardization objectives.
358-367: LGTM: Success path spreads additional data appropriately.The success response correctly merges additional request data into the response payload using the spread operator, maintaining the existing structure while adding extra fields.
396-403: LGTM: Sync endpoint error handling standardized.The error response structure matches the async endpoint pattern with
metadata=Noneand additional data in thedatafield.
474-489: LGTM: Sync endpoint success path includes additional data.The success response properly extracts and spreads additional request data, maintaining consistency with the async endpoint.
502-509: LGTM: OpenAI error handling follows standardized pattern.The error response structure is consistent with other error paths, properly including additional data and setting
metadata=None.backend/app/tests/api/routes/test_responses.py (4)
110-112: LGTM: Test validates additional data inclusion in success responses.The test correctly verifies that
callback_urlfrom the request is included in the responsedatafield, validating the standardization implementation.
189-191: LGTM: Consistent additional data validation across test scenarios.The test properly validates
callback_urlinclusion in the response data, maintaining consistency with other success scenario tests.
256-259: LGTM: Error scenario validation matches standardized response structure.The test correctly verifies that in error scenarios,
callback_urlis included in thedatafield andmetadatais set toNone, validating the standardized error response structure.
293-296: LGTM: Comprehensive error scenario validation.The test ensures consistent validation of the standardized error response structure across different credential-related error conditions, maintaining test coverage completeness.
1 participant
Summary
Target issue is #317
Explain the motivation for making this change. What existing problem does the pull request solve?
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Summary by CodeRabbit
New Features
Tests