Epic/92/OpenAI compatible response api #132
Conversation
extract handler to different provider and fix API dos
…on-streaming feat: Add OpenAI Responses API implementation with routes, handlers, validation, and models
…i-compatible-response-api
…pletion-response-api feat/94 Add non-stream response flow with conversation integration
…onses-create Feat/95/ support streaming response with stateful conversation
There was a problem hiding this comment.
@locnguyen1986 Out of curiosity, this file is manually crafted or generated?
There was a problem hiding this comment.
Yup :) It's generated file from our route files
|
|
||
| // IsEmpty checks if the error is empty (no error) | ||
| func (e *Error) IsEmpty() bool { | ||
| return e == nil || e.Code == "" |
apps/jan-api-gateway/application/app/infrastructure/database/dbschema/conversation.go
Outdated
Show resolved
Hide resolved
| // Error represents a standardized error with code and message | ||
| type Error struct { | ||
| Code string `json:"code"` | ||
| Message string `json:"message"` |
There was a problem hiding this comment.
We should wrap this with an error instance.
There was a problem hiding this comment.
My original suggestion is: if returning 3 args domain object, error, code from functions is redundant.
consider creating a wrapper struct like:
type Result[T any] struct {
Data T
Err error
Code *string
}
Messages are not mandatory in the service, as the route or other clients will respond with different messages.
apps/jan-api-gateway/application/app/domain/conversation/conversation_service.go
Outdated
Show resolved
Hide resolved
| ResponseFormat interface{} | ||
| Tools interface{} | ||
| ToolChoice interface{} | ||
| Metadata map[string]interface{} |
| publicID, err := idgen.GenerateSecureID("msg", 42) | ||
| if err != nil { | ||
| return false, common.NewError("u7v8w9x0-y1z2-3456-uvwx-789012345678", "Failed to generate item ID") | ||
| } | ||
|
|
||
| // Convert role | ||
| var role conversation.ItemRole | ||
| switch msg.Role { | ||
| case openai.ChatMessageRoleSystem: | ||
| role = conversation.ItemRoleSystem | ||
| case openai.ChatMessageRoleUser: | ||
| role = conversation.ItemRoleUser | ||
| case openai.ChatMessageRoleAssistant: | ||
| role = conversation.ItemRoleAssistant | ||
| default: | ||
| role = conversation.ItemRoleUser | ||
| } | ||
|
|
||
| // Convert content | ||
| content := make([]conversation.Content, 0, len(msg.MultiContent)) | ||
| for _, contentPart := range msg.MultiContent { | ||
| if contentPart.Type == openai.ChatMessagePartTypeText { | ||
| content = append(content, conversation.Content{ | ||
| Type: "text", | ||
| Text: &conversation.Text{ | ||
| Value: contentPart.Text, | ||
| }, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // If no multi-content, use simple text content | ||
| if len(content) == 0 && msg.Content != "" { | ||
| content = append(content, conversation.Content{ | ||
| Type: "text", | ||
| Text: &conversation.Text{ | ||
| Value: msg.Content, | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| item := &conversation.Item{ | ||
| PublicID: publicID, | ||
| Type: conversation.ItemType("message"), | ||
| Role: &role, | ||
| Content: content, | ||
| ConversationID: conv.ID, | ||
| ResponseID: responseID, | ||
| CreatedAt: time.Now(), | ||
| } |
There was a problem hiding this comment.
Should it be moved to a simple factory?
|
|
||
| // Call domain service (pure business logic) | ||
| result, err := responseRoute.responseModelService.CreateResponse(ctx, userID, domainRequest) | ||
| if !err.IsEmpty() { |
| func (s *ResponseService) ConvertDomainResponseToAPIResponse(responseEntity *Response) responsetypes.Response { | ||
| apiResponse := responsetypes.Response{ | ||
| ID: responseEntity.PublicID, | ||
| Object: "response", | ||
| Created: responseEntity.CreatedAt.Unix(), | ||
| Model: responseEntity.Model, | ||
| Status: responsetypes.ResponseStatus(responseEntity.Status), | ||
| Input: responseEntity.Input, | ||
| } | ||
|
|
||
| // Add conversation if exists | ||
| if responseEntity.ConversationID != nil { | ||
| apiResponse.Conversation = &responsetypes.ConversationInfo{ | ||
| ID: fmt.Sprintf("conv_%d", *responseEntity.ConversationID), | ||
| } | ||
| } | ||
|
|
||
| // Add timestamps | ||
| if responseEntity.CompletedAt != nil { | ||
| apiResponse.CompletedAt = ptr.ToInt64(responseEntity.CompletedAt.Unix()) | ||
| } | ||
| if responseEntity.CancelledAt != nil { | ||
| apiResponse.CancelledAt = ptr.ToInt64(responseEntity.CancelledAt.Unix()) | ||
| } | ||
| if responseEntity.FailedAt != nil { | ||
| apiResponse.FailedAt = ptr.ToInt64(responseEntity.FailedAt.Unix()) | ||
| } | ||
|
|
||
| // Parse output if exists | ||
| if responseEntity.Output != nil { | ||
| var output interface{} | ||
| if err := json.Unmarshal([]byte(*responseEntity.Output), &output); err == nil { | ||
| apiResponse.Output = output | ||
| } | ||
| } | ||
|
|
||
| // Parse usage if exists | ||
| if responseEntity.Usage != nil { | ||
| var usage responsetypes.DetailedUsage | ||
| if err := json.Unmarshal([]byte(*responseEntity.Usage), &usage); err == nil { | ||
| apiResponse.Usage = &usage | ||
| } | ||
| } | ||
|
|
||
| // Parse error if exists | ||
| if responseEntity.Error != nil { | ||
| var errorData responsetypes.ResponseError | ||
| if err := json.Unmarshal([]byte(*responseEntity.Error), &errorData); err == nil { | ||
| apiResponse.Error = &errorData | ||
| } | ||
| } | ||
|
|
||
| return apiResponse | ||
| } |
There was a problem hiding this comment.
Let's move it to the http response folder. The HTTP response includes the domain, not the other way around.
| } | ||
|
|
||
| // ConvertConversationItemToInputItem converts a conversation item to input item format | ||
| func (s *ResponseService) ConvertConversationItemToInputItem(item *conversation.Item) responsetypes.InputItem { |
There was a problem hiding this comment.
Let's move it to the http response folder. The HTTP response includes the domain, not the other way around.
…nversation-revisit
| } | ||
| if exists == nil { | ||
| exists, err = googleAuthAPI.authService.RegisterUser(reqCtx.Request.Context(), &user.User{ | ||
| exists, err = googleAuthAPI.userService.RegisterUser(reqCtx.Request.Context(), &user.User{ |
There was a problem hiding this comment.
Please use the authService, it will automatically create organization.
|
|
||
| accessTokenExp := time.Now().Add(15 * time.Minute) | ||
| // Use handleGoogleToken to calculate expiration with buffer | ||
| // Instead of hardcoded 15 minutes, the access token now uses the actual expiration time from Google's token response |
There was a problem hiding this comment.
I agree that we should move accessTokenExp := time.Now().Add(15 * time.Minute) to a single location.
However, we should not use the expiration time from the third-party library because the access token is signed by our service, It should have the same behavior as guest login.
| if err := reqCtx.ShouldBindJSON(&request); err != nil { | ||
| reqCtx.AbortWithStatusJSON(http.StatusBadRequest, responses.ErrorResponse{ | ||
| Code: "e5c96a9e-7ff9-4408-9514-9d206ca85b33", | ||
| ErrorInstance: err, |
There was a problem hiding this comment.
ErrorInstance: err, please keep it
| reqCtx.AbortWithStatusJSON(http.StatusBadRequest, responses.ErrorResponse{ | ||
| Code: "0e5b8426-b1d2-4114-ac81-d3982dc497cf", | ||
| Code: err.GetCode(), | ||
| Error: err.Error(), |
| reqCtx.AbortWithStatusJSON(http.StatusInternalServerError, responses.ErrorResponse{ | ||
| Code: "8fc529d7-f384-40f2-ac15-cd1f1e109316", | ||
| ErrorInstance: err, | ||
| }) | ||
| return |
There was a problem hiding this comment.
We should return a response in the route, handling errors.
|
|
||
| // doGetConversation performs the business logic for getting a conversation | ||
| func (api *ConversationAPI) GetConversation(conv *conversation.Conversation) (*ConversationResponse, *common.Error) { | ||
| return domainToConversationResponse(conv), nil |
There was a problem hiding this comment.
Redundant wrap func:
- If it serves as the converter, why do we have to return
*common.Error? return domainToConversationResponse(conv), nilNil is unnecessary here.- Why don't we just use domainToConversationResponse instead
| } | ||
|
|
||
| // doUpdateConversation performs the business logic for updating a conversation | ||
| func (api *ConversationAPI) UpdateConversation(ctx context.Context, conv *conversation.Conversation, request UpdateConversationRequest) (*ConversationResponse, *common.Error) { |
There was a problem hiding this comment.
- It seems like it cannot be reused.
- It did not consolidate the reusable business logic just add an extra layer of complexity.
|
|
||
| reqCtx.JSON(http.StatusOK, openai.ListResponse[*ConversationItemResponse]{ | ||
| Object: openai.ObjectTypeListList, | ||
| return &ListResponse[*ConversationItemResponse]{ |
There was a problem hiding this comment.
openai.ListResponse[*ConversationItemResponse] is reuseable
| func (api *ConversationAPI) DeleteItem(ctx context.Context, conv *conversation.Conversation, item *conversation.Item) (*ConversationItemResponse, *common.Error) { | ||
| // Use efficient deletion with item public ID instead of loading all items | ||
| itemDeleted, err := api.conversationService.DeleteItemWithConversation(ctx, conv, item) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return domainToConversationItemResponse(itemDeleted), nil | ||
| } |
There was a problem hiding this comment.
It's not business logic.
The business logic is in api.conversationService.DeleteItemWithConversation(ctx, conv, item).
There was a problem hiding this comment.
Btw, the response doesn't fit the response of openai's..
https://platform.openai.com/docs/api-reference/conversations/delete-item
| reqCtx.JSON(http.StatusOK, domainToConversationItemResponse(itemDeleted)) | ||
|
|
||
| reqCtx.JSON(http.StatusOK, result) |
There was a problem hiding this comment.
My proposal:
reqCtx.JSON(http.StatusOK, domainToConversationItemResponse(itemDeleted))
is better than
reqCtx.JSON(http.StatusOK, result)
While I was still pondering whether the delete operation would return an object like:
{
"id": "item_123",
"object": "item.deleted",
"deleted": true
}
I need to check our codebase:
reqCtx.JSON(http.StatusOK, result) -> and go to api.DeleteItem(ctx, conv, item) -> then find domainToConversationItemResponse(itemDeleted)
It would be better if I could see every possible response in just one route function.
3 participants
Changes: