OpenAI conversation: Standardizing API response#318
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe failure response mechanism was enhanced to include additional metadata extracted from the original request. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant OpenAI
Client->>API: Send request (async or sync)
API->>API: Determine request type (async or sync)
API->>API: Extract additional metadata (exclude keys based on type)
API->>OpenAI: Forward request
OpenAI-->>API: Return error
API->>API: Generate failure response with error + metadata
API-->>Client: Return enriched failure response
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ 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! |
avirajsingh7
left a comment
avirajsingh7
left a comment
There was a problem hiding this comment.
In the current implementation of Prompt Management, I’m only including pagination details in the metadata field of the API response. However, I’ve been thinking about introducing a proper structure and validation to metadata as well. By defining it using Pydantic (via SQLModel), we can make the API responses easier to document and validate.
Adding code snippet
from sqlmodel import SQLModel, Field
class Pagination(SQLModel):
"""
Pagination metadata for API responses.
"""
total: int = Field(..., description="Total number of items", ge=0)
skip: int = Field(..., description="Number of items to skip", ge=0)
limit: int = Field(
..., description="Maximum number of items to return", ge=1, le=100
)
class Metadata(SQLModel):
"""
Metadata for API responses.
"""
pagination: Pagination | None = None
class ResponseMetadata(Metadata):
"""
Metadata for API responses.
"""
assistants_id: str | None = Field(
None, description="ID of the assistant", example="12345"
)
call_back_url: str | None = Field(
None, description="Callback URL for the response", example="https://example.com/callback"
)
The idea is to have a base Metadata class that contains common fields—like pagination—and then allow more specific metadata to be added by extending this base class.
For now you can ignore pagination thing that I will include in Prompt PR.
I will wait for @kartpop opinion also, this can be considered over engineering also but it is always good to return validated response.
kartpop
left a comment
kartpop
left a comment
There was a problem hiding this comment.
Approving this, anticipating that the assumption in my comment is correct.
| k: v | ||
| for k, v in request.items() | ||
| if k not in {"assistant_id", "callback_url", "response_id", "question"} | ||
| # Keys to exclude for async request (ResponsesAPIRequest) |
There was a problem hiding this comment.
I assume that the only reason we are hardcoding here is because:
- this route is very specific to OpenAI Responses API AND
- these keys are very specific to that API
Right? If yes, this is fine.
Yes, I think that this might be over-engineering. At least in the context of the current PR, where the additional Even in the case of |
3 participants
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
Please add here if any other information is required for the reviewer.
Summary by CodeRabbit