feat[Go]: implement chat write and session-create APIs#15667
feat[Go]: implement chat write and session-create APIs#15667hunnyboy1217 wants to merge 1 commit into
Conversation
Ports the following endpoints from Python (chat_api.py) to the Go layer, closing issue infiniflow#15665: POST /api/v1/chats (CreateChat) PATCH /api/v1/chats/:chat_id (PatchChat) DELETE /api/v1/chats/:chat_id (DeleteChatByID) POST /api/v1/chats/:chat_id/sessions (CreateChatSession) Changed files: internal/dao/chat.go – NameConflictExists helper (case-insensitive duplicate-name check with optional excludeID) internal/service/chat.go – CreateChat, PatchChat, DeleteChatByID, CreateChatSession; helpers: validateName, validateDatasetIDs, applyPromptDefaults, validatePromptParams, buildChatResponseMap, buildSessionResponseMap, ensureOwnedChat internal/handler/chat.go – CreateChat, PatchChat, DeleteChatByID, CreateChatSession handlers internal/router/router.go – route registration for all four endpoints Functional parity with Python: - tenant_id must not be supplied on create - name required on create, optional (but non-empty) on patch - dataset_ids validated: existence, caller ownership, chunk_num > 0, same embedding model across all datasets - prompt_config defaults applied on create (system prompt, prologue, parameters, quote, tts, refine_multiturn) - prompt_config and llm_setting merged (not replaced) on PATCH - duplicate name check (case-insensitive) on create and patch - ownership guard (_ensure_owned_chat) on patch, delete, session-create - delete is soft (status → "0") - session create prefills first message with chat's prologue; response renames dialog_id → chat_id and message → messages
dosubot
Bot
added
the
size:XL
📝 WalkthroughWalkthroughThis PR implements complete chat lifecycle management by adding chat creation, update, deletion, and session-creation endpoints with centralized validation, authorization checks, name-conflict detection, dataset ownership verification, prompt-configuration handling, and consistent JSON response formatting across DAO, service, handler, and routing layers. ChangesChat CRUD Operations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![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
🧹 Nitpick comments (1)
internal/service/chat.go (1)
1072-1079: ⚡ Quick winRedundant database fetch - reuse chat from
ensureOwnedChat.
ensureOwnedChatalready fetches and returns the chat entity, but the result is discarded (_) and then the same chat is fetched again on line 1076. Reusing the returned value avoids an unnecessary database round-trip.♻️ Proposed fix
func (s *ChatService) CreateChatSession(userID, chatID, name string) (map[string]interface{}, error) { - if _, err := s.ensureOwnedChat(userID, chatID); err != nil { - return nil, err - } - chat, err := s.chatDAO.GetByID(chatID) - if err != nil { - return nil, fmt.Errorf("Chat not found!") - } + chat, err := s.ensureOwnedChat(userID, chatID) + if err != nil { + return nil, err + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/chat.go` around lines 1072 - 1079, The CreateChatSession function currently discards the chat returned by ensureOwnedChat and then calls s.chatDAO.GetByID again; capture the returned chat from ensureOwnedChat (e.g., chat, err := s.ensureOwnedChat(userID, chatID)) and use that chat value directly instead of calling s.chatDAO.GetByID(chatID) to avoid the redundant DB fetch; ensure you preserve the same error handling/return behavior if ensureOwnedChat returns an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/handler/chat.go`:
- Around line 512-518: Do not ignore the binding error from c.ShouldBindJSON:
capture the returned error when binding into body and reject malformed JSON
(e.g., *json.SyntaxError or unexpected EOF) with a 400 response instead of
silently defaulting name; only treat an empty body (io.EOF / no content) as
acceptable and fall back to the default session name. In practice, change the
current anonymous bind call to check err := c.ShouldBindJSON(&body), return
c.JSON/AbortWithStatusJSON(400, ...) for JSON syntax/unmarshal errors (use
errors.As or check error types) and only allow the empty-body case to continue
to the name handling where name := body.Name and default to "New session".
In `@internal/service/chat.go`:
- Around line 756-761: The current code silently ignores errors from
s.userTenantDAO.GetTenantIDsByUserID, which can lead to incorrect authorization;
change the call to capture and handle the error from GetTenantIDsByUserID (check
the returned error variable), and on non-nil error either return/propagate the
error up from the enclosing function or log it and fail authorization explicitly
instead of continuing with an empty userTenantIDs; ensure the rest of the logic
that builds the authorised map (using userTenantIDs and userID) only runs when
the DAO call succeeded.
- Around line 800-814: RemoveChats currently permits deleting chats if
chat.TenantID is in the set returned by
userTenantDAO.GetTenantIDsByUserID(userID), which is more permissive than
ensureOwnedChat; update RemoveChats to apply the same strict owner check as
ensureOwnedChat (i.e., require chat.TenantID == userID and valid status) by
invoking or replicating ensureOwnedChat for each chat being removed (or
explicitly replace the tenant membership check in RemoveChats with the same
tenant equality and status checks used in ensureOwnedChat) so bulk deletion
matches Python bulk_delete_chats/_ensure_owned_chat behavior.
---
Nitpick comments:
In `@internal/service/chat.go`:
- Around line 1072-1079: The CreateChatSession function currently discards the
chat returned by ensureOwnedChat and then calls s.chatDAO.GetByID again; capture
the returned chat from ensureOwnedChat (e.g., chat, err :=
s.ensureOwnedChat(userID, chatID)) and use that chat value directly instead of
calling s.chatDAO.GetByID(chatID) to avoid the redundant DB fetch; ensure you
preserve the same error handling/return behavior if ensureOwnedChat returns an
error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7f38101-a3b9-4f4f-93ee-6201f4c441a0
📒 Files selected for processing (4)
internal/dao/chat.gointernal/handler/chat.gointernal/router/router.gointernal/service/chat.go
| // body is optional; ignore bind error and fall back to default name | ||
| _ = c.ShouldBindJSON(&body) | ||
|
|
||
| name := body.Name | ||
| if name == "" { | ||
| name = "New session" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For github.com/gin-gonic/gin v1.9.1, what error does Context.ShouldBindJSON return for (1) an empty request body and (2) malformed JSON?
💡 Result:
In gin-gonic/gin v1.9.1, the Context.ShouldBindJSON method utilizes the standard library's encoding/json package to decode the request body [1][2]. Consequently, it returns the following errors for the specified scenarios: (1) Empty Request Body: When the request body is empty, ShouldBindJSON returns an io.EOF error [3][4]. This occurs because the underlying json.Unmarshal process attempts to read from the request body stream, which is empty [4]. (2) Malformed JSON: When the JSON is malformed (e.g., missing brackets, trailing commas, or invalid syntax), ShouldBindJSON returns a *json.SyntaxError [1][2]. In some cases involving incomplete or unparseable payloads, it may also return an io.EOF or an "unexpected EOF" error [5]. It is important to note that because ShouldBindJSON reads the request body stream, subsequent attempts to read or bind the body will result in an io.EOF error [4][1][6]. If you need to bind the body multiple times, you should use Context.ShouldBindBodyWithJSON instead, which caches the body in the context [3][6].
Citations:
- 1: https://jsonic.io/guides/json-in-gin
- 2: https://www.gofaq.org/en/how-to-validate-request-data-in-gin/
- 3: [Bug] EOF when using ShouldBindJSON gin-gonic/gin#4035
- 4: ctx.shouldBindJSON crashes when called twice gin-gonic/gin#4110
- 5: feat(binding/json): check validity when bind json gin-gonic/gin#3982
- 6: https://stackoverflow.com/questions/76462803/getting-eof-for-a-valid-json-when-doing-a-gin-context-shouldbind
Reject malformed JSON payloads instead of silently defaulting the session name.
ShouldBindJSON in gin v1.9.1 returns io.EOF for an empty body, but malformed JSON returns *json.SyntaxError (and in some cases an unexpected-EOF-style error). Since the code currently discards the bind error, malformed payloads also fall through and create a session with the default "New session".
💡 Suggested change
import (
+ "errors"
+ "io"
"net/http"
"ragflow/internal/common"
"strconv"
@@
var body struct {
Name string `json:"name"`
}
- // body is optional; ignore bind error and fall back to default name
- _ = c.ShouldBindJSON(&body)
+ // body is optional; malformed JSON should still fail fast
+ if err := c.ShouldBindJSON(&body); err != nil && !errors.Is(err, io.EOF) {
+ c.JSON(http.StatusOK, gin.H{"code": common.CodeDataError, "data": false, "message": err.Error()})
+ return
+ }
name := body.Name
if name == "" {
name = "New session"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // body is optional; ignore bind error and fall back to default name | |
| _ = c.ShouldBindJSON(&body) | |
| name := body.Name | |
| if name == "" { | |
| name = "New session" | |
| } | |
| // body is optional; malformed JSON should still fail fast | |
| if err := c.ShouldBindJSON(&body); err != nil && !errors.Is(err, io.EOF) { | |
| c.JSON(http.StatusOK, gin.H{"code": common.CodeDataError, "data": false, "message": err.Error()}) | |
| return | |
| } | |
| name := body.Name | |
| if name == "" { | |
| name = "New session" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/handler/chat.go` around lines 512 - 518, Do not ignore the binding
error from c.ShouldBindJSON: capture the returned error when binding into body
and reject malformed JSON (e.g., *json.SyntaxError or unexpected EOF) with a 400
response instead of silently defaulting name; only treat an empty body (io.EOF /
no content) as acceptable and fall back to the default session name. In
practice, change the current anonymous bind call to check err :=
c.ShouldBindJSON(&body), return c.JSON/AbortWithStatusJSON(400, ...) for JSON
syntax/unmarshal errors (use errors.As or check error types) and only allow the
empty-body case to continue to the name handling where name := body.Name and
default to "New session".
| userTenantIDs, _ := s.userTenantDAO.GetTenantIDsByUserID(userID) | ||
| authorised := make(map[string]struct{}, len(userTenantIDs)+1) | ||
| for _, tid := range userTenantIDs { | ||
| authorised[tid] = struct{}{} | ||
| } | ||
| authorised[userID] = struct{}{} |
There was a problem hiding this comment.
Silently ignoring DAO error can mask authorization failures.
If GetTenantIDsByUserID fails (e.g., database connectivity issue), the error is discarded and userTenantIDs will be empty. This causes the authorised set to contain only userID, potentially denying access to datasets owned by the user's other tenants or masking underlying infrastructure problems.
🐛 Proposed fix
- userTenantIDs, _ := s.userTenantDAO.GetTenantIDsByUserID(userID)
+ userTenantIDs, err := s.userTenantDAO.GetTenantIDsByUserID(userID)
+ if err != nil {
+ return nil, fmt.Errorf("failed to resolve user tenants: %w", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| userTenantIDs, _ := s.userTenantDAO.GetTenantIDsByUserID(userID) | |
| authorised := make(map[string]struct{}, len(userTenantIDs)+1) | |
| for _, tid := range userTenantIDs { | |
| authorised[tid] = struct{}{} | |
| } | |
| authorised[userID] = struct{}{} | |
| userTenantIDs, err := s.userTenantDAO.GetTenantIDsByUserID(userID) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to resolve user tenants: %w", err) | |
| } | |
| authorised := make(map[string]struct{}, len(userTenantIDs)+1) | |
| for _, tid := range userTenantIDs { | |
| authorised[tid] = struct{}{} | |
| } | |
| authorised[userID] = struct{}{} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/chat.go` around lines 756 - 761, The current code silently
ignores errors from s.userTenantDAO.GetTenantIDsByUserID, which can lead to
incorrect authorization; change the call to capture and handle the error from
GetTenantIDsByUserID (check the returned error variable), and on non-nil error
either return/propagate the error up from the enclosing function or log it and
fail authorization explicitly instead of continuing with an empty userTenantIDs;
ensure the rest of the logic that builds the authorised map (using userTenantIDs
and userID) only runs when the DAO call succeeded.
| // ensureOwnedChat checks that userID is the owner of a valid chat. | ||
| // Mirrors Python _ensure_owned_chat. | ||
| func (s *ChatService) ensureOwnedChat(userID, chatID string) (*entity.Chat, error) { | ||
| chat, err := s.chatDAO.GetByID(chatID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("Chat not found!") | ||
| } | ||
| if chat.Status == nil || *chat.Status != "1" { | ||
| return nil, fmt.Errorf("Chat not found!") | ||
| } | ||
| if chat.TenantID != userID { | ||
| return nil, fmt.Errorf("No authorization.") | ||
| } | ||
| return chat, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Python implementation for ownership check pattern
rg -n -A5 '_ensure_owned_chat|ensure_owned_chat' --type=pyRepository: infiniflow/ragflow
Length of output: 6875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/service/chat.go"
# Show ensureOwnedChat implementation and a few nearby call sites/usages
sed -n '780,860p' "$FILE" | cat -n
# Show RemoveChats implementation region (as referenced by original comment)
sed -n '540,680p' "$FILE" | cat -n
# Find ensureOwnedChat call sites to see what is passed as the first argument
rg -n "ensureOwnedChat\(" "$FILE" -n | head -n 50Repository: infiniflow/ragflow
Length of output: 8664
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/service/chat.go"
# Show the blocks around ensureOwnedChat call sites
sed -n '920,1020p' "$FILE" | cat -n
sed -n '1020,1105p' "$FILE" | cat -n
# Also show function definitions immediately around those line ranges if needed
# (already included by sed, but keep for context)Repository: infiniflow/ragflow
Length of output: 6853
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "PatchChat\(" internal | head -n 50
rg -n "DeleteChatByID\(" internal | head -n 50
rg -n "RemoveChats\(" internal | head -n 50Repository: infiniflow/ragflow
Length of output: 942
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/handler/chat.go"
sed -n '200,320p' "$FILE" | cat -n
sed -n '380,520p' "$FILE" | cat -nRepository: infiniflow/ragflow
Length of output: 9631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find GetUser implementation
rg -n "func GetUser\\(" internal | head -n 50
# Show GetUser code
# (we'll re-run sed with the exact path after locating it, but try to print likely file if found)Repository: infiniflow/ragflow
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' internal/handler/common.go | cat -nRepository: infiniflow/ragflow
Length of output: 1393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find entity.User and entity.Chat definitions
rg -n "type User struct" internal/entity | head -n 50
rg -n "type Chat struct" internal/entity | head -n 50
# Inspect ChatService.CreateChat: find where chat entity is constructed
FILE="internal/service/chat.go"
rg -n "func \\(s \\*ChatService\\) CreateChat\\(" "$FILE"
# Print a larger window around CreateChat creation (find chatDAO.Create call too)
rg -n "chatDAO\\.Create\\(" "$FILE"Repository: infiniflow/ragflow
Length of output: 369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect user entity
sed -n '1,120p' internal/entity/user.go | cat -n
# Inspect chat entity
sed -n '1,140p' internal/entity/chat.go | cat -n
# Inspect CreateChat where chat is constructed and chatDAO.Create(chat) is called
sed -n '780,980p' internal/service/chat.go | cat -nRepository: infiniflow/ragflow
Length of output: 14777
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any Chat construction that sets TenantID explicitly
rg -n "entity\\.Chat\\{[\\s\\S]*TenantID:" internal | head -n 50
# Also search for any assignment to ".TenantID =" related to chat/dialog
rg -n "\\.TenantID\\s*=\\s*" internal | head -n 50
# Search for other chatDAO.Create usages besides internal/service/chat.go
rg -n "chatDAO\\.Create\\(" internal | head -n 50Repository: infiniflow/ragflow
Length of output: 44
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If earlier rg found nothing, widen search
rg -n "TenantID:" internal | head -n 50 || true
rg -n "\\.TenantID\\b" internal | head -n 50 || true
# Search for direct tenant_id column usage in dialog context
rg -n "tenant_id" internal | head -n 80 || true
# Search for dialog DAO/service creation/update patterns
rg -n "chatDAO\\.Create\\(|dialogDAO\\.|UpdateManyByID\\(|UpdateByID\\(|QueryByTenantIDAndID\\(" internal/service internal/dao internal/handler | head -n 100 || trueRepository: infiniflow/ragflow
Length of output: 19275
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect other chat creation path around the second chatDAO.Create (line ~926)
sed -n '880,980p' internal/service/chat.go | cat -n
# 2) Find Python "remove"/"delete" dialogs and check auth logic used there
PY="api/apps/restful_apis/chat_api.py"
rg -n "RemoveChats|remove|dialog.*rm|delete.*chat|/chats|dialog.*status" "$PY" | head -n 80
# Specifically search for usages of _ensure_owned_chat around delete endpoints, and any tenant membership query
rg -n "_ensure_owned_chat|user_tenant|tenant.*IDs|GetTenant" "$PY" | head -n 80Repository: infiniflow/ragflow
Length of output: 7000
🏁 Script executed:
#!/bin/bash
set -euo pipefail
PY="api/apps/restful_apis/chat_api.py"
# Show _ensure_owned_chat definition
sed -n '140,190p' "$PY" | cat -n
# Show bulk_delete_chats implementation (route /chats methods=["DELETE"])
sed -n '680,790p' "$PY" | cat -nRepository: infiniflow/ragflow
Length of output: 7055
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "GetTenantIDsByUserID" internal | head -n 50
# Show the implementation file for GetTenantIDsByUserID
FILE=$(rg -n "GetTenantIDsByUserID" internal -S --files-with-matches | head -n 1)
echo "First match file: $FILE"
sed -n '1,220p' "$FILE" | cat -nRepository: infiniflow/ragflow
Length of output: 8336
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' internal/dao/user_tenant.go | cat -nRepository: infiniflow/ragflow
Length of output: 6082
Align RemoveChats authorization with ensureOwnedChat (and Python _ensure_owned_chat)
ensureOwnedChat only allows when chat.TenantID == userID (and status is valid). In contrast, RemoveChats allows when chat.TenantID is in userTenantDAO.GetTenantIDsByUserID(userID) (plus userID), which can permit deleting chats belonging to other tenant IDs the user is a member of. Python’s bulk chat delete (bulk_delete_chats) also uses _ensure_owned_chat, which checks tenant_id=current_user.id, i.e., the stricter behavior.
Update RemoveChats to use the same ownership check as ensureOwnedChat (or explicitly justify/implement the intended member-vs-owner model consistently across single and bulk endpoints).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/chat.go` around lines 800 - 814, RemoveChats currently
permits deleting chats if chat.TenantID is in the set returned by
userTenantDAO.GetTenantIDsByUserID(userID), which is more permissive than
ensureOwnedChat; update RemoveChats to apply the same strict owner check as
ensureOwnedChat (i.e., require chat.TenantID == userID and valid status) by
invoking or replicating ensureOwnedChat for each chat being removed (or
explicitly replace the tenant membership check in RemoveChats with the same
tenant equality and status checks used in ensureOwnedChat) so bulk deletion
matches Python bulk_delete_chats/_ensure_owned_chat behavior.
1 participant
What problem does this PR solve?
Closes #15665 — ports the remaining Chat write endpoints and session-create from Python (
api/apps/restful_apis/chat_api.py) to the Go API layer.GET /api/v1/chatsandGET /api/v1/chats/:chat_idwere already done. This PR adds the four missing ones:/api/v1/chatsCreateChat/api/v1/chats/:chat_idPatchChat/api/v1/chats/:chat_idDeleteChatByID/api/v1/chats/:chat_id/sessionsCreateChatSessionType of change
Implementation notes
Files changed:
Functional parity with Python:
tenant_idguardtenant_idon createchunk_num > 0, same embedding model across all datasetsapplyPromptDefaultsmirrors_apply_prompt_defaults: system prompt, prologue, parameters, quote, tts, refine_multiturnNameConflictExists; on PATCH excludes the current chat IDprompt_configandllm_settingmerged with existing values (not replaced), matching Python'sdeepcopy + updateensureOwnedChatmirrors_ensure_owned_chat:chat.TenantID == userID && status == "1"status → "0"(same as PythonStatusEnum.INVALID.value)prompt_config.prologue; response renamesdialog_id → chat_idandmessage → messagesmatching_build_session_responsebuildChatResponseMapmirrors_build_chat_response: resolveskb_ids→dataset_ids+kb_names