-
Notifications
You must be signed in to change notification settings - Fork 0
Implement conversations API with Item-based #78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
add conversation into Jan routes fix uuid code
apps/jan-api-gateway/application/app/infrastructure/database/dbschema/conversation.go
Outdated
Show resolved
Hide resolved
func stringToStringPtr(s string) *string { | ||
if s == "" { | ||
return nil | ||
} | ||
return &s | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ptr.ToString(...)
instead.
...n-api-gateway/application/app/interfaces/http/routes/v1/conversations/conversations_route.go
Outdated
Show resolved
Hide resolved
...n-api-gateway/application/app/interfaces/http/routes/v1/conversations/conversations_route.go
Outdated
Show resolved
Hide resolved
...n-api-gateway/application/app/interfaces/http/routes/v1/conversations/conversations_route.go
Outdated
Show resolved
Hide resolved
...n-api-gateway/application/app/interfaces/http/routes/v1/conversations/conversations_route.go
Outdated
Show resolved
Hide resolved
// @Accept json | ||
// @Produce json | ||
// @Param conversation_id path string true "Conversation ID" | ||
// @Success 200 {object} conversation.Conversation "Conversation details" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return a Response object instead of a domain object.
...n-api-gateway/application/app/interfaces/http/routes/v1/conversations/conversations_route.go
Outdated
Show resolved
Hide resolved
conversationsRouter.PATCH("/:conversation_id", api.UpdateConversation) | ||
conversationsRouter.DELETE("/:conversation_id", api.DeleteConversation) | ||
conversationsRouter.POST("/:conversation_id/items", api.AddItem) | ||
conversationsRouter.GET("/:conversation_id/items/search", api.SearchItems) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there no such endpoint in the OpenAI platform API documentation?
...plication/app/infrastructure/database/repository/conversationrepo/conversation_repository.go
Outdated
Show resolved
Hide resolved
...n-api-gateway/application/app/interfaces/http/routes/v1/conversations/conversations_route.go
Outdated
Show resolved
Hide resolved
// @Failure 500 {object} responses.ErrorResponse "Internal server error" | ||
// @Router /v1/conversations [post] | ||
func (api *ConversationAPI) CreateConversation(ctx *gin.Context) { | ||
ownerID, err := api.validateAPIKey(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have concerns about this. Should we separate users into client users and API users?
Currently, they share the same user ID. Assuming the client has a "list conversations" function, the current implementation would cause conversations created using an API key to be retrieved in the client's "list conversations," which is not our intended design. We should fix this in the next iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may keep same user, The client user may register the API and still work with their history. And they should aware that. I think we should keep same user base now.
...n-api-gateway/application/app/interfaces/http/routes/v1/conversations/conversations_route.go
Outdated
Show resolved
Hide resolved
...n-api-gateway/application/app/interfaces/http/routes/v1/conversations/conversations_route.go
Outdated
Show resolved
Hide resolved
apps/jan-api-gateway/application/app/domain/conversation/conversation_service.go
Outdated
Show resolved
Hide resolved
...n-api-gateway/application/app/interfaces/http/routes/v1/conversations/conversations_route.go
Outdated
Show resolved
Hide resolved
return conversation, nil | ||
} | ||
|
||
func (s *ConversationService) GetConversation(ctx context.Context, publicID string, userID uint) (*Conversation, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current name, GetConversation, is misleading because the function not only retrieves a conversation by its public ID but also performs these additional actions:
- Checks access permissions based on userID.
- Loads associated items.
- Populates the Items field on the returned Conversation object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its name is changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code is refactored with naming function and reused if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetConversation
still takes publicID
and userID
. Also, Go doesn't support function overloading. Consider renaming it to something like GetConversationByPublicIDAndUserID
.
return conversation, nil | ||
} | ||
|
||
func (s *ConversationService) UpdateConversation(ctx context.Context, publicID string, userID uint, title *string, metadata map[string]string) (*Conversation, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming Considerations
- Why UpdateConversation Could Be Misleading: It suggests a simple update operation, but the function also handles retrieval and access control, which are significant responsibilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its name is changed
apps/jan-api-gateway/application/app/domain/conversation/conversation.go
Show resolved
Hide resolved
HasMore: false, // For now, we don't implement pagination | ||
FirstID: "", | ||
LastID: "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider following the pattern of /projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it later
if len(data) > 0 {
response.FirstID = data[0].ID
response.LastID = data[len(data)-1].ID
}
but now we don't have paging in these API so it should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we plan to add these back later, please leave a // TODO: comment.
update the model
refactoring code to support use case
func GenerateConversationID() (string, error) { | ||
return GenerateSecureID("conv", 16) | ||
} | ||
|
||
// GenerateItemID generates an item/message ID with format "msg_..." | ||
func GenerateItemID() (string, error) { | ||
return GenerateSecureID("msg", 16) | ||
} | ||
|
||
// GenerateAPIKeyID generates an API key ID with format "sk_..." | ||
func GenerateAPIKeyID() (string, error) { | ||
return GenerateSecureID("sk", 24) | ||
} | ||
|
||
// GenerateOrganizationID generates an organization ID with format "org_..." | ||
func GenerateOrganizationID() (string, error) { | ||
return GenerateSecureID("org", 16) | ||
} | ||
|
||
// GenerateProjectID generates a project ID with format "proj_..." | ||
func GenerateProjectID() (string, error) { | ||
return GenerateSecureID("proj", 16) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to the domain or service level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right, It should belong to domain services, let me create the shared service for that.
func ValidateConversationID(id string) bool { | ||
return ValidateIDFormat(id, "conv") | ||
} | ||
|
||
// ValidateItemID validates an item/message ID format | ||
func ValidateItemID(id string) bool { | ||
return ValidateIDFormat(id, "msg") | ||
} | ||
|
||
// ValidateAPIKeyID validates an API key ID format | ||
func ValidateAPIKeyID(id string) bool { | ||
return ValidateIDFormat(id, "sk") | ||
} | ||
|
||
// ValidateOrganizationID validates an organization ID format | ||
func ValidateOrganizationID(id string) bool { | ||
return ValidateIDFormat(id, "org") | ||
} | ||
|
||
// ValidateProjectID validates a project ID format | ||
func ValidateProjectID(id string) bool { | ||
return ValidateIDFormat(id, "proj") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to the domain or service level.
return conversation, nil | ||
} | ||
|
||
func (s *ConversationService) GetConversation(ctx context.Context, publicID string, userID uint) (*Conversation, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetConversation
still takes publicID
and userID
. Also, Go doesn't support function overloading. Consider renaming it to something like GetConversationByPublicIDAndUserID
.
@@ -0,0 +1,839 @@ | |||
package conversation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a good idea; the introduction of this handler leads to high coupling between /v1/jan/conversation and /v1/conversation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we can discuss this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yah, I introduce handler now as we try to keep the same feature for both Jan and OpenAI, too much duplicated code, if we plan to merge these APIs endpoint or custom handling, we may do it in different way. Now almost of them are same. And I think they will be the same of the future too.
}) | ||
return | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate length of items here: Initial items to include in the conversation context. You may add up to 20 items at a time.
func (v *ConversationValidator) ValidatePublicID(publicID string) error { | ||
if publicID == "" { | ||
return fmt.Errorf("public ID cannot be empty") | ||
} | ||
|
||
if len(publicID) < 5 || len(publicID) > 50 { | ||
return fmt.Errorf("public ID must be between 5 and 50 characters") | ||
} | ||
|
||
// Use centralized ID validation logic | ||
if strings.HasPrefix(publicID, "conv_") { | ||
if !idutils.ValidateConversationID(publicID) { | ||
return fmt.Errorf("invalid conversation ID format") | ||
} | ||
} else if strings.HasPrefix(publicID, "msg_") { | ||
if !idutils.ValidateItemID(publicID) { | ||
return fmt.Errorf("invalid item ID format") | ||
} | ||
} else { | ||
// Fallback to regex pattern for unknown prefixes | ||
if !v.publicIDPattern.MatchString(publicID) { | ||
return fmt.Errorf("public ID contains invalid characters") | ||
} | ||
} | ||
|
||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The public ID of a domain object should have a single, consistent format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we support conversation, message as public id now. It not the conversation only.
} | ||
|
||
// Convert all items at once for batch processing | ||
itemsToCreate := make([]struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the Item domain object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above, It's for batch processing only. However, I move it to global struct so it's easier for maintain
} | ||
|
||
conversationID := ctx.Param("conversation_id") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Query parameters?
after
include
limit
order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we plan to add these back later, please leave a // TODO:
comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, ignore all the business like paging or handling, I have another PRs with introduce the business layer which gather the conversation ( core business ) and will introduce the Conversation API for Jan ( will replicate with Jan Desktop API now ), We will define it later. This PR focus on structure and simple CRUDs.
@@ -0,0 +1,186 @@ | |||
package conversations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's disregard the /jan/v1/conversation interfaces for now. It's exposed to our client. We don't need to adhere to OpenAI's standard. Let's focus on OpenAI's.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have to keep that, it will be used by Jan Web as migration. Now we have to maintain both of them to make everything works first.
HasMore: false, // For now, we don't implement pagination | ||
FirstID: "", | ||
LastID: "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we plan to add these back later, please leave a // TODO: comment.
@@ -0,0 +1,136 @@ | |||
package id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This service violates SPR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments from chatgpt:
Creating a single service like IDService to handle ID generation for all domain entities is generally not a good idea due to several key design principles it violates: separation of concerns, maintainability, and domain-driven design (DDD).
Separation of Concerns
A single IDService becomes a god object, which is an anti-pattern where one object knows too much or does too much. This violates the principle of separation of concerns by coupling the low-level technical detail of ID generation with the high-level business logic of different domains. The IDService knows about conversations, organizations, users, and API keys, which are distinct business concepts. In a well-designed system, the Organization domain should be responsible for its own identity, not a shared, centralized service. This also creates a single point of failure: if a change to the ID generation logic for one domain entity (e.g., UserID) breaks something, it could potentially affect all other domains using the same service.
Dependency Hell:
All parts of the application that need an ID must depend on this single service. This tight coupling makes it hard to refactor or replace a part of the system without affecting every other part.
func (s *IDService) GenerateProjectID() (string, error) { | ||
return s.GenerateSecureID("proj", 16) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The specification belongs to the project domain.
// The service has a dependency on the repository interface. | ||
repo ProjectRepository | ||
repo ProjectRepository | ||
idService *id.IDService |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dependency Hell:
All parts of the application that need an ID must depend on this single service. This tight coupling makes it hard to refactor or replace a part of the system without affecting every other part.
Description:
This PR implements conversation-like functionality based on the OpenAI Conversations API to allow users to create and manage conversations with context retention. The implementation follows OpenAI's API specification exactly and uses API key authentication for enhanced security.
OpenAI Platform Reference: https://platform.openai.com/docs/api-reference/conversations
Available Endpoints:
V1 API (OpenAI-Compatible):
[POST] /v1/conversations
- Create conversation with initial items[GET] /v1/conversations/{conversation_id}
- Get conversation details[POST] /v1/conversations/{conversation_id}
- Update conversation metadata (OpenAI uses POST, not PATCH)[DELETE] /v1/conversations/{conversation_id}
- Delete conversation[POST] /v1/conversations/{conversation_id}/items
- Create multiple items[GET] /v1/conversations/{conversation_id}/items
- List conversation items[GET] /v1/conversations/{conversation_id}/items/{item_id}
- Retrieve a specific item[DELETE] /v1/conversations/{conversation_id}/items/{item_id}
- Delete an item (returns updated conversation)Jan API Endpoints (Identical to V1):
[POST] /jan/v1/conversations
- Create conversation with initial items[GET] /jan/v1/conversations/{conversation_id}
- Get conversation details[POST] /jan/v1/conversations/{conversation_id}
- Update conversation metadata[DELETE] /jan/v1/conversations/{conversation_id}
- Delete conversation[POST] /jan/v1/conversations/{conversation_id}/items
- Create multiple items[GET] /jan/v1/conversations/{conversation_id}/items
- List conversation items[GET] /jan/v1/conversations/{conversation_id}/items/{item_id}
- Retrieve a specific item[DELETE] /jan/v1/conversations/{conversation_id}/items/{item_id}
- Delete an item (returns updated conversation)Change-Specific Checklist
Domain Changes
This PR introduces changes to the domain models or business logic.
List of Changed Domains:
conversation
- New domain for conversation and item managementuser
- Enhanced with conversation-related functionalityDatabase Migration
This PR includes a database migration.
Table Changes:
conversations
- New table for storing conversation metadata with proper indexingitems
- New table for storing conversation items/messages with foreign key relationshipsMigration Strategy:
Endpoint Changes
This PR adds, modifies, or removes API endpoints.
✅ Swagger/API Docs have been updated for all changed endpoints.
✅ Breaking Change: No - This PR introduces new endpoints only
Key Features Implemented:
✅ OpenAI API Compliance: 100% adherence to OpenAI Conversations API specification with exact request/response structures
✅ Response Structure Compliance: All responses match OpenAI schemas
✅ Custom Error Types: Robust error handling with
errors.Is()
and specific error types for different failure scenarios✅ Dual API Support: Identical functionality available on both
/v1/
(OpenAI-compatible) and/jan/v1/
(Jan-specific) routes✅ Context Retention: Conversations maintain full context history with proper item relationships
✅ Private Mode: Conversations are private by default with proper access control validation
✅ Structured Content: Support for complex content types with proper JSON marshaling/unmarshaling