Go: refactor and add version type - #16863
Conversation
Signed-off-by: Jin Hai <haijin.chn@gmail.com>
📝 WalkthroughWalkthroughThe PR moves version retrieval into ChangesAdmin and version changes
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant AdminClient
participant EEAdminHandler
participant AdminService
AdminClient->>EEAdminHandler: Submit enterprise admin request
EEAdminHandler->>AdminService: Validate and forward request data
AdminService-->>EEAdminHandler: Return operation result
EEAdminHandler-->>AdminClient: Send common success or error response
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
dosubot
Bot
added
☯️ refactor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/dao/user.go (1)
100-117: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFilter and sort parameters are accepted but never applied.
Listacceptsname,status,sort, andorderBybut the query body only appliesoffsetandlimit. NoWHEREclause filters bynameorstatus, and noORDER BYusessort/orderBy. Callers will receive unfiltered, unsorted results despite passing filter values, and thetotalcount won't match the filtered set.🐛 Proposed fix: apply filters and sorting
func (dao *UserDAO) List(offset, limit int, name, status, sort, orderBy string) ([]*entity.User, int64, error) { var users []*entity.User var total int64 - // Only count users with status != "0" (not deleted) - if err := DB.Model(&entity.User{}).Count(&total).Error; err != nil { + query := DB.Model(&entity.User{}).Where("status != ?", "0") + if name != "" { + query = query.Where("name LIKE ?", "%"+name+"%") + } + if status != "" { + query = query.Where("status = ?", status) + } + + if err := query.Count(&total).Error; err != nil { return nil, 0, err } - query := DB.Model(&entity.User{}) + if orderBy != "" && sort != "" { + query = query.Order(orderBy + " " + sort) + } if offset > 0 { query = query.Offset(offset) } if limit > 0 { query = query.Limit(limit) } err := query.Find(&users).Error return users, total, 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/dao/user.go` around lines 100 - 117, Update UserDAO.List to apply the name and status filters to both the total count and user query, then apply the requested sort/orderBy ordering before pagination. Preserve offset and limit behavior, and ensure total reflects the filtered result set.internal/admin/service_ee.go (1)
791-807: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ListUsersEEreturns request parameters instead of user records.
/admin/usersin Enterprise mode will return a single fake item built frompageIndex,pageSize,name, etc., so callers get bogus data instead of a user list. Query the user store likeListUsers, or fail explicitly rather than returning success with placeholder content.🤖 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/admin/service_ee.go` around lines 791 - 807, The ListUsersEE method currently returns request parameters as a fake successful user record. Replace this placeholder implementation with the Enterprise user-store query behavior used by ListUsers, preserving its filtering, pagination, sorting, and error handling; otherwise return an explicit unsupported/error result instead of fabricated data.
🧹 Nitpick comments (1)
internal/admin/handler.go (1)
160-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
ListUsersRequeststruct.ListUsersreads query params directly now, and nothing else references this type.🤖 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/admin/handler.go` around lines 160 - 168, Remove the unused ListUsersRequest struct from the admin handler, leaving the existing ListUsers query-parameter handling unchanged.Source: Coding guidelines
🤖 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/admin/handler_ee.go`:
- Around line 296-313: Update ListModelProviders so the lowercased available
query value is actually applied by passing it through the service layer and
filtering the returned providers; if the service API cannot support filtering,
remove the unused query parsing instead.
- Around line 1384-1391: Return immediately after the bad-request response in
each top-parameter parse block so invalid input cannot continue to the service
call: update ListUsersStorage (internal/admin/handler_ee.go:1384-1391),
ListUsersDocuments (internal/admin/handler_ee.go:1424-1431), ListUsersIndex
(internal/admin/handler_ee.go:1464-1471), and ListUsersQuota
(internal/admin/handler_ee.go:1516-1523). Preserve the existing 400 response and
normal flow for valid top values.
- Around line 613-614: Update the success message returned by
DeleteModelInstance to describe successful model instance deletion instead of
model provider addition, while preserving the existing common.SuccessWithData
response flow.
- Around line 1718-1726: Update GenerateUserAPIKey to validate the decoded
username after DecodeFromBase64 succeeds, returning the same bad-request
response used by the other username handlers when it is empty, before invoking
the service.
- Around line 258-268: In the request-binding error branch of the
role-default-model handler, return immediately after sending the Bad Request
response. Keep SetRoleDefaultModel execution limited to successfully bound
requests, matching the existing ResetRoleDefaultModel behavior.
- Around line 1333-1351: Read pagination inputs with c.Query("page") and
c.Query("page_size") in the pagination parsing block instead of c.Param,
preserving the existing integer validation, defaults, and bad-request responses
so caller-supplied pagination works for the affected user endpoints.
- Around line 659-664: Remove the incorrectly formatted println bind-error calls
from the JSON binding blocks at internal/admin/handler_ee.go lines 659-664,
770-775, 811-816, 832-837, 857-862, 1303-1308, 1326-1331, 1490-1495, 1638-1643,
1659-1664, and 1698-1703, while preserving common.ErrorWithCode and the existing
returns. In the Top validation blocks at internal/admin/handler_ee.go lines
1384-1391, 1424-1431, 1464-1471, and 1516-1523, return immediately after
responding with the Top must be an integer error so processing cannot continue
with the default value.
In `@internal/admin/handler.go`:
- Around line 173-232: Remove the SuccessWithData call inside the
common.OpenSourceVersion branch of the switch, keeping the unconditional
SuccessWithData call after the switch as the single success response for both
supported RAGFlow types.
In `@internal/admin/service.go`:
- Around line 143-144: Update the offset calculation in Service.ListUsers to use
a zero-based page offset, multiplying pageSize by pageIndex minus one before
passing it to userDAO.List. Preserve the existing pageSize, filtering, sorting,
and ordering arguments.
In `@internal/common/http.go`:
- Around line 109-111: Update the negative-value branch in the parameter parsing
function to avoid wrapping the nil parse error; return a standalone descriptive
error for negative parameterInt values while preserving the existing
defaultValue return and positive-or-zero validation message.
In `@internal/common/version.go`:
- Around line 42-43: Update the comment above the VERSION path logic to
reference the current internal/common package instead of internal/utility,
leaving the surrounding implementation unchanged.
In `@internal/service/system.go`:
- Around line 88-94: Fix GetVersion in SystemService so it no longer calls the
undefined common.GetRAGFlowType symbol. Either add the missing helper alongside
GetRAGFlowVersion in the common version implementation, or reuse the existing
source of truth for the type string, while preserving the VersionResponse Type
field behavior.
---
Outside diff comments:
In `@internal/admin/service_ee.go`:
- Around line 791-807: The ListUsersEE method currently returns request
parameters as a fake successful user record. Replace this placeholder
implementation with the Enterprise user-store query behavior used by ListUsers,
preserving its filtering, pagination, sorting, and error handling; otherwise
return an explicit unsupported/error result instead of fabricated data.
In `@internal/dao/user.go`:
- Around line 100-117: Update UserDAO.List to apply the name and status filters
to both the total count and user query, then apply the requested sort/orderBy
ordering before pagination. Preserve offset and limit behavior, and ensure total
reflects the filtered result set.
---
Nitpick comments:
In `@internal/admin/handler.go`:
- Around line 160-168: Remove the unused ListUsersRequest struct from the admin
handler, leaving the existing ListUsers query-parameter handling unchanged.
🪄 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: b90259ca-8e8e-408f-b22b-f20538f0211f
📒 Files selected for processing (13)
cmd/ragflow_server.gointernal/admin/handler.gointernal/admin/handler_ee.gointernal/admin/service.gointernal/admin/service_ee.gointernal/common/http.gointernal/common/version.gointernal/common/version_test.gointernal/dao/user.gointernal/handler/user.gointernal/service/admin_client.gointernal/service/system.gointernal/service/user.go
💤 Files with no reviewable changes (2)
- internal/service/user.go
- internal/handler/user.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/dao/user.go (1)
100-117: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFilter and sort parameters are accepted but never applied.
Listacceptsname,status,sort, andorderBybut the query body only appliesoffsetandlimit. NoWHEREclause filters bynameorstatus, and noORDER BYusessort/orderBy. Callers will receive unfiltered, unsorted results despite passing filter values, and thetotalcount won't match the filtered set.🐛 Proposed fix: apply filters and sorting
func (dao *UserDAO) List(offset, limit int, name, status, sort, orderBy string) ([]*entity.User, int64, error) { var users []*entity.User var total int64 - // Only count users with status != "0" (not deleted) - if err := DB.Model(&entity.User{}).Count(&total).Error; err != nil { + query := DB.Model(&entity.User{}).Where("status != ?", "0") + if name != "" { + query = query.Where("name LIKE ?", "%"+name+"%") + } + if status != "" { + query = query.Where("status = ?", status) + } + + if err := query.Count(&total).Error; err != nil { return nil, 0, err } - query := DB.Model(&entity.User{}) + if orderBy != "" && sort != "" { + query = query.Order(orderBy + " " + sort) + } if offset > 0 { query = query.Offset(offset) } if limit > 0 { query = query.Limit(limit) } err := query.Find(&users).Error return users, total, 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/dao/user.go` around lines 100 - 117, Update UserDAO.List to apply the name and status filters to both the total count and user query, then apply the requested sort/orderBy ordering before pagination. Preserve offset and limit behavior, and ensure total reflects the filtered result set.internal/admin/service_ee.go (1)
791-807: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
ListUsersEEreturns request parameters instead of user records.
/admin/usersin Enterprise mode will return a single fake item built frompageIndex,pageSize,name, etc., so callers get bogus data instead of a user list. Query the user store likeListUsers, or fail explicitly rather than returning success with placeholder content.🤖 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/admin/service_ee.go` around lines 791 - 807, The ListUsersEE method currently returns request parameters as a fake successful user record. Replace this placeholder implementation with the Enterprise user-store query behavior used by ListUsers, preserving its filtering, pagination, sorting, and error handling; otherwise return an explicit unsupported/error result instead of fabricated data.
🧹 Nitpick comments (1)
internal/admin/handler.go (1)
160-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
ListUsersRequeststruct.ListUsersreads query params directly now, and nothing else references this type.🤖 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/admin/handler.go` around lines 160 - 168, Remove the unused ListUsersRequest struct from the admin handler, leaving the existing ListUsers query-parameter handling unchanged.Source: Coding guidelines
🤖 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/admin/handler_ee.go`:
- Around line 296-313: Update ListModelProviders so the lowercased available
query value is actually applied by passing it through the service layer and
filtering the returned providers; if the service API cannot support filtering,
remove the unused query parsing instead.
- Around line 1384-1391: Return immediately after the bad-request response in
each top-parameter parse block so invalid input cannot continue to the service
call: update ListUsersStorage (internal/admin/handler_ee.go:1384-1391),
ListUsersDocuments (internal/admin/handler_ee.go:1424-1431), ListUsersIndex
(internal/admin/handler_ee.go:1464-1471), and ListUsersQuota
(internal/admin/handler_ee.go:1516-1523). Preserve the existing 400 response and
normal flow for valid top values.
- Around line 613-614: Update the success message returned by
DeleteModelInstance to describe successful model instance deletion instead of
model provider addition, while preserving the existing common.SuccessWithData
response flow.
- Around line 1718-1726: Update GenerateUserAPIKey to validate the decoded
username after DecodeFromBase64 succeeds, returning the same bad-request
response used by the other username handlers when it is empty, before invoking
the service.
- Around line 258-268: In the request-binding error branch of the
role-default-model handler, return immediately after sending the Bad Request
response. Keep SetRoleDefaultModel execution limited to successfully bound
requests, matching the existing ResetRoleDefaultModel behavior.
- Around line 1333-1351: Read pagination inputs with c.Query("page") and
c.Query("page_size") in the pagination parsing block instead of c.Param,
preserving the existing integer validation, defaults, and bad-request responses
so caller-supplied pagination works for the affected user endpoints.
- Around line 659-664: Remove the incorrectly formatted println bind-error calls
from the JSON binding blocks at internal/admin/handler_ee.go lines 659-664,
770-775, 811-816, 832-837, 857-862, 1303-1308, 1326-1331, 1490-1495, 1638-1643,
1659-1664, and 1698-1703, while preserving common.ErrorWithCode and the existing
returns. In the Top validation blocks at internal/admin/handler_ee.go lines
1384-1391, 1424-1431, 1464-1471, and 1516-1523, return immediately after
responding with the Top must be an integer error so processing cannot continue
with the default value.
In `@internal/admin/handler.go`:
- Around line 173-232: Remove the SuccessWithData call inside the
common.OpenSourceVersion branch of the switch, keeping the unconditional
SuccessWithData call after the switch as the single success response for both
supported RAGFlow types.
In `@internal/admin/service.go`:
- Around line 143-144: Update the offset calculation in Service.ListUsers to use
a zero-based page offset, multiplying pageSize by pageIndex minus one before
passing it to userDAO.List. Preserve the existing pageSize, filtering, sorting,
and ordering arguments.
In `@internal/common/http.go`:
- Around line 109-111: Update the negative-value branch in the parameter parsing
function to avoid wrapping the nil parse error; return a standalone descriptive
error for negative parameterInt values while preserving the existing
defaultValue return and positive-or-zero validation message.
In `@internal/common/version.go`:
- Around line 42-43: Update the comment above the VERSION path logic to
reference the current internal/common package instead of internal/utility,
leaving the surrounding implementation unchanged.
In `@internal/service/system.go`:
- Around line 88-94: Fix GetVersion in SystemService so it no longer calls the
undefined common.GetRAGFlowType symbol. Either add the missing helper alongside
GetRAGFlowVersion in the common version implementation, or reuse the existing
source of truth for the type string, while preserving the VersionResponse Type
field behavior.
---
Outside diff comments:
In `@internal/admin/service_ee.go`:
- Around line 791-807: The ListUsersEE method currently returns request
parameters as a fake successful user record. Replace this placeholder
implementation with the Enterprise user-store query behavior used by ListUsers,
preserving its filtering, pagination, sorting, and error handling; otherwise
return an explicit unsupported/error result instead of fabricated data.
In `@internal/dao/user.go`:
- Around line 100-117: Update UserDAO.List to apply the name and status filters
to both the total count and user query, then apply the requested sort/orderBy
ordering before pagination. Preserve offset and limit behavior, and ensure total
reflects the filtered result set.
---
Nitpick comments:
In `@internal/admin/handler.go`:
- Around line 160-168: Remove the unused ListUsersRequest struct from the admin
handler, leaving the existing ListUsers query-parameter handling unchanged.
🪄 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: b90259ca-8e8e-408f-b22b-f20538f0211f
📒 Files selected for processing (13)
cmd/ragflow_server.gointernal/admin/handler.gointernal/admin/handler_ee.gointernal/admin/service.gointernal/admin/service_ee.gointernal/common/http.gointernal/common/version.gointernal/common/version_test.gointernal/dao/user.gointernal/handler/user.gointernal/service/admin_client.gointernal/service/system.gointernal/service/user.go
💤 Files with no reviewable changes (2)
- internal/service/user.go
- internal/handler/user.go
🛑 Comments failed to post (12)
internal/admin/handler_ee.go (7)
258-268: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Missing
returnafter bind error causes execution to continue with an invalid request.On bind failure the handler writes a 400 response but falls through to
h.service.SetRoleDefaultModel(...)with an empty/partialrequest, then writes a second response. CompareResetRoleDefaultModel(line 285), which correctly returns.🐛 Proposed fix
if err := c.ShouldBindJSON(&request); err != nil { common.ResponseWithHttpCodeData(c, http.StatusBadRequest, common.CodeBadRequest, nil, "Invalid request body: "+err.Error()) + return }📝 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.var request SetRoleDefaultModelRequest if err := c.ShouldBindJSON(&request); err != nil { common.ResponseWithHttpCodeData(c, http.StatusBadRequest, common.CodeBadRequest, nil, "Invalid request body: "+err.Error()) return } result, err := h.service.SetRoleDefaultModel(roleName, request.ModelID, request.ModelType) if err != nil { common.ErrorWithCode(c, common.CodeServerError, err.Error()) return } common.SuccessWithData(c, result, "Role default model set successfully")🤖 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/admin/handler_ee.go` around lines 258 - 268, In the request-binding error branch of the role-default-model handler, return immediately after sending the Bad Request response. Keep SetRoleDefaultModel execution limited to successfully bound requests, matching the existing ResetRoleDefaultModel behavior.
296-313: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
availablefilter is parsed but never applied.
keywordsis read from theavailablequery param and lowercased, then discarded —h.service.ListModelProviders()is called with no arguments. Either wire the filter through to the service or drop the dead parsing block.🤖 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/admin/handler_ee.go` around lines 296 - 313, Update ListModelProviders so the lowercased available query value is actually applied by passing it through the service layer and filtering the returned providers; if the service API cannot support filtering, remove the unused query parsing instead.
613-614: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Wrong success message.
DeleteModelInstancereturns "Model provider added successfully". It should reflect instance deletion.✏️ Proposed fix
- common.SuccessWithData(c, result, "Model provider added successfully") + common.SuccessWithData(c, result, "Model instances deleted successfully")📝 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.common.SuccessWithData(c, result, "Model instances deleted successfully")🤖 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/admin/handler_ee.go` around lines 613 - 614, Update the success message returned by DeleteModelInstance to describe successful model instance deletion instead of model provider addition, while preserving the existing common.SuccessWithData response flow.
659-664: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the relevant function structure first. ast-grep outline internal/admin/handler_ee.go --view expanded # Read only the relevant sections around the reported line ranges. sed -n '640,690p' internal/admin/handler_ee.go sed -n '760,875p' internal/admin/handler_ee.go sed -n '1290,1345p' internal/admin/handler_ee.go sed -n '1478,1715p' internal/admin/handler_ee.go # Check whether the project uses a logger in these handlers or elsewhere in the file. rg -n "ShouldBindJSON|println\\(|ErrorWithCode\\(|log\\.|logger\\." internal/admin/handler_ee.go internal/admin -g '!**/*_test.go'Repository: infiniflow/ragflow
Length of output: 50374
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read only the sections that were truncated earlier. sed -n '1368,1528p' internal/admin/handler_ee.go # Also inspect the exact top-parsing blocks for the four affected handlers. rg -n -A14 -B6 'topStr := c\.Param\("top"\)' internal/admin/handler_ee.goRepository: infiniflow/ragflow
Length of output: 6920
Drop the debug prints and return on invalid
topvalues.
internal/admin/handler_ee.go#L659-L664,#L770-L775,#L811-L816,#L832-L837,#L857-L862,#L1303-L1308,#L1326-L1331,#L1490-L1495,#L1638-L1643,#L1659-L1664,#L1698-L1703: remove theprintlnbind-error lines; they ignore format verbs and add noisy stderr output.internal/admin/handler_ee.go#L1384-L1391,#L1424-L1431,#L1464-L1471,#L1516-L1523: addreturnafter theTop must be an integerresponse so these handlers don’t continue with the defaulttopvalue.📍 Affects 1 file
internal/admin/handler_ee.go#L659-L664(this comment)internal/admin/handler_ee.go#L770-L775internal/admin/handler_ee.go#L811-L816internal/admin/handler_ee.go#L832-L837internal/admin/handler_ee.go#L857-L862internal/admin/handler_ee.go#L1303-L1308internal/admin/handler_ee.go#L1326-L1331internal/admin/handler_ee.go#L1490-L1495internal/admin/handler_ee.go#L1638-L1643internal/admin/handler_ee.go#L1659-L1664internal/admin/handler_ee.go#L1698-L1703🤖 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/admin/handler_ee.go` around lines 659 - 664, Remove the incorrectly formatted println bind-error calls from the JSON binding blocks at internal/admin/handler_ee.go lines 659-664, 770-775, 811-816, 832-837, 857-862, 1303-1308, 1326-1331, 1490-1495, 1638-1643, 1659-1664, and 1698-1703, while preserving common.ErrorWithCode and the existing returns. In the Top validation blocks at internal/admin/handler_ee.go lines 1384-1391, 1424-1431, 1464-1471, and 1516-1523, return immediately after responding with the Top must be an integer error so processing cannot continue with the default value.Source: Coding guidelines
1333-1351: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -nP "ListUsersReports|ListUsersStorage|ListUsersQuota" internal/admin/router.go -C1Repository: infiniflow/ragflow
Length of output: 632
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== router snippets ==\n' sed -n '136,150p' internal/admin/router.go printf '\n== handler locations ==\n' rg -n "ListUsers(Reports|Storage|Documents|Index|Quota)" internal/admin/handler_ee.go -n -C 3 printf '\n== relevant handler body ==\n' sed -n '1320,1375p' internal/admin/handler_ee.go printf '\n== query param usage in these handlers ==\n' rg -n "c\.Param\\(\"page\"\\)|c\.Param\\(\"page_size\"\\)|c\.Query\\(\"page\"\\)|c\.Query\\(\"page_size\"\\)" internal/admin/handler_ee.go -n -C 2Repository: infiniflow/ragflow
Length of output: 7384
Read pagination from query params here.
c.Param("page")/c.Param("page_size")will always be empty on/users/reports,/users/storage,/users/documents,/users/index, and/users/quota; usec.Query(...)(or add matching path params) so caller-supplied pagination is honored.🤖 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/admin/handler_ee.go` around lines 1333 - 1351, Read pagination inputs with c.Query("page") and c.Query("page_size") in the pagination parsing block instead of c.Param, preserving the existing integer validation, defaults, and bad-request responses so caller-supplied pagination works for the affected user endpoints.
1384-1391: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Missing
returnafter thetopparse-error response. Unlike thepage/page_sizebranches, thetopparse-error path writes a 400 response but does not return, so execution continues into the service call and writes a second response (gin superfluous-WriteHeader). The shared root cause is the omittedreturnin eachtopblock.
internal/admin/handler_ee.go#L1384-L1391: addreturnafter the 400 inListUsersStorage'stopblock.internal/admin/handler_ee.go#L1424-L1431: addreturninListUsersDocuments.internal/admin/handler_ee.go#L1464-L1471: addreturninListUsersIndex.internal/admin/handler_ee.go#L1516-L1523: addreturninListUsersQuota.📍 Affects 1 file
internal/admin/handler_ee.go#L1384-L1391(this comment)internal/admin/handler_ee.go#L1424-L1431internal/admin/handler_ee.go#L1464-L1471internal/admin/handler_ee.go#L1516-L1523🤖 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/admin/handler_ee.go` around lines 1384 - 1391, Return immediately after the bad-request response in each top-parameter parse block so invalid input cannot continue to the service call: update ListUsersStorage (internal/admin/handler_ee.go:1384-1391), ListUsersDocuments (internal/admin/handler_ee.go:1424-1431), ListUsersIndex (internal/admin/handler_ee.go:1464-1471), and ListUsersQuota (internal/admin/handler_ee.go:1516-1523). Preserve the existing 400 response and normal flow for valid top values.
1718-1726: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Inconsistent: no empty-username guard.
Unlike every other username handler in this file,
GenerateUserAPIKeydecodes the Base64 username but does not reject an empty result before calling the service.🛡️ Proposed fix
username, err := common.DecodeFromBase64(encodedUsername) if err != nil { common.ErrorWithCode(c, common.CodeBadRequest, err.Error()) return } + if username == "" { + common.ErrorWithCode(c, common.CodeBadRequest, "Username is required") + return + }📝 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.encodedUsername := c.Param("username") username, err := common.DecodeFromBase64(encodedUsername) if err != nil { common.ErrorWithCode(c, common.CodeBadRequest, err.Error()) return } if username == "" { common.ErrorWithCode(c, common.CodeBadRequest, "Username is required") return }🤖 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/admin/handler_ee.go` around lines 1718 - 1726, Update GenerateUserAPIKey to validate the decoded username after DecodeFromBase64 succeeds, returning the same bad-request response used by the other username handlers when it is empty, before invoking the service.internal/admin/handler.go (1)
173-232: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Duplicate response write for OpenSourceVersion — response is emitted twice.
case common.OpenSourceVersioncallscommon.SuccessWithDataat line 219, then falls out of the switch into the unconditional call at line 231, writing the success response twice for every OpenSource request.🐛 Proposed fix
case common.OpenSourceVersion: users, err = h.service.ListUsers(pageInt, pageSizeInt, name, status, sort, orderBy) if err != nil { common.ErrorWithCode(c, common.CodeServerError, err.Error()) return } - - common.SuccessWithData(c, users, "List users") case common.EnterpriseEdition:📝 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.name := c.DefaultQuery("keyword", "") status := c.DefaultQuery("status", "") role := c.DefaultQuery("role", "") sort := c.DefaultQuery("sort", "") // descending or ascending orderBy := c.DefaultQuery("order", "") // order by field pageInt, err := common.ParseRequestIntPositive(c, c.Query("page"), "page", 1) if err != nil { common.ErrorWithCode(c, common.CodeBadRequest, err.Error()) return } pageSizeInt, err := common.ParseRequestIntPositive(c, c.Query("page_size"), "page_size", 10) if err != nil { common.ErrorWithCode(c, common.CodeBadRequest, err.Error()) return } plan := c.Query("plan") // plan name topInt, err := common.ParseRequestIntPositive(c, c.Query("top"), "top", 0) if err != nil { common.ErrorWithCode(c, common.CodeBadRequest, err.Error()) return } quotaInt, err := common.ParseRequestIntPositive(c, c.Query("quota"), "quota", 0) if err != nil { common.ErrorWithCode(c, common.CodeBadRequest, err.Error()) return } if quotaInt > 100 { common.ErrorWithCode(c, common.CodeBadRequest, "Quota must be less than or equal to 100") return } daysInt, err := common.ParseRequestIntPositive(c, c.Query("days"), "days", 0) if err != nil { common.ErrorWithCode(c, common.CodeBadRequest, err.Error()) return } var users []map[string]interface{} switch common.GetRAGFlowType() { case common.OpenSourceVersion: users, err = h.service.ListUsers(pageInt, pageSizeInt, name, status, sort, orderBy) if err != nil { common.ErrorWithCode(c, common.CodeServerError, err.Error()) return } case common.EnterpriseEdition: users, err = h.service.ListUsersEE(pageInt, pageSizeInt, name, status, role, sort, orderBy, plan, topInt, daysInt, quotaInt) if err != nil { common.ErrorWithCode(c, common.CodeServerError, err.Error()) return } default: common.ErrorWithCode(c, common.CodeBadRequest, "Invalid RAGFlow type") return } common.SuccessWithData(c, users, "List users") return🤖 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/admin/handler.go` around lines 173 - 232, Remove the SuccessWithData call inside the common.OpenSourceVersion branch of the switch, keeping the unconditional SuccessWithData call after the switch as the single success response for both supported RAGFlow types.internal/admin/service.go (1)
143-144: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Off-by-one pagination offset — first page is always skipped.
pageIndexis 1-based (handler defaultspageto 1 viaParseRequestIntPositive), but the offset is computed aspageIndex*pageSize. For page 1, this skips the firstpageSizerecords entirely, shifting every page's results by one page.🐛 Proposed fix
func (s *Service) ListUsers(pageIndex, pageSize int, name, status, sort, orderBy string) ([]map[string]interface{}, error) { - users, _, err := s.userDAO.List(pageIndex*pageSize, pageSize, name, status, sort, orderBy) + offset := (pageIndex - 1) * pageSize + if offset < 0 { + offset = 0 + } + users, _, err := s.userDAO.List(offset, pageSize, name, status, sort, orderBy)📝 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.func (s *Service) ListUsers(pageIndex, pageSize int, name, status, sort, orderBy string) ([]map[string]interface{}, error) { offset := (pageIndex - 1) * pageSize if offset < 0 { offset = 0 } users, _, err := s.userDAO.List(offset, pageSize, name, status, sort, orderBy)🤖 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/admin/service.go` around lines 143 - 144, Update the offset calculation in Service.ListUsers to use a zero-based page offset, multiplying pageSize by pageIndex minus one before passing it to userDAO.List. Preserve the existing pageSize, filtering, sorting, and ordering arguments.internal/common/http.go (1)
109-111: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wrapping a
nilerror with%wproduces a malformed error message.When the input parses successfully to a negative value (e.g.
top=-1),errisnilhere, sofmt.Errorf("%w: ...", err, ...)yields a garbled%!w(<nil>)message that gets echoed to the client.🐛 Proposed fix
if parameterInt < 0 { - return defaultValue, fmt.Errorf("%w: %s must be a positive integer or zero", err, parameterName) + return defaultValue, fmt.Errorf("%s must be a positive integer or zero", parameterName) }📝 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.if parameterInt < 0 { return defaultValue, fmt.Errorf("%s must be a positive integer or zero", parameterName) }🤖 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/common/http.go` around lines 109 - 111, Update the negative-value branch in the parameter parsing function to avoid wrapping the nil parse error; return a standalone descriptive error for negative parameterInt values while preserving the existing defaultValue return and positive-or-zero validation message.internal/common/version.go (1)
42-43: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Stale comment references old package path.
Line 43 still says "Assuming this file is in internal/utility" but the package was renamed to
common. As per coding guidelines, documentation comments should be aligned with the current runtime path.♻️ Proposed fix
- // Assuming this file is in internal/utility, VERSION is in project root + // Assuming this file is in internal/common, VERSION is in project root📝 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.// Get the path to VERSION file // Assuming this file is in internal/common, VERSION is in project root🤖 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/common/version.go` around lines 42 - 43, Update the comment above the VERSION path logic to reference the current internal/common package instead of internal/utility, leaving the surrounding implementation unchanged.Source: Coding guidelines
internal/service/system.go (1)
88-94: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify GetRAGFlowType is defined in internal/common/version.go rg -n 'func GetRAGFlowType' internal/common/version.goRepository: infiniflow/ragflow
Length of output: 156
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Searching for GetRAGFlowType across the repository..." rg -n 'GetRAGFlowType' . echo echo "Inspecting likely version-related files in internal/common..." fd -a -t f 'version\.go|.*common.*\.go' internal/common || trueRepository: infiniflow/ragflow
Length of output: 550
🏁 Script executed:
#!/bin/bash set -euo pipefail wc -l internal/common/version.go echo cat -n internal/common/version.goRepository: infiniflow/ragflow
Length of output: 2830
common.GetRAGFlowType()is undefined
internal/service/system.gocallscommon.GetRAGFlowType(), butinternal/common/version.goonly definesGetRAGFlowVersion(). This will not compile unless you add the missing helper or switch to the existing source of truth for the type string.🧰 Tools
🪛 GitHub Actions: sep-tests / 0_ragflow_tests_elasticsearch (go).txt
[error] 90-90: Go build failed: undefined: common.GetRAGFlowType (internal/service/system.go:90:24)
🪛 GitHub Actions: sep-tests / 1_ragflow_tests_infinity (go).txt
[error] 90-90: Go build failed: undefined: common.GetRAGFlowType
🪛 GitHub Actions: sep-tests / ragflow_tests_elasticsearch (go)
[error] 90-90: Go build failed: undefined: common.GetRAGFlowType
🪛 GitHub Actions: sep-tests / ragflow_tests_infinity (go)
[error] 90-90: Go build failed: undefined: common.GetRAGFlowType (internal/service/system.go:90:24).
🤖 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/system.go` around lines 88 - 94, Fix GetVersion in SystemService so it no longer calls the undefined common.GetRAGFlowType symbol. Either add the missing helper alongside GetRAGFlowVersion in the common version implementation, or reuse the existing source of truth for the type string, while preserving the VersionResponse Type field behavior.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/common/version_ee.go (1)
18-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse constants for immutable edition identifiers.
OpenSourceVersionandEnterpriseEditionare exported mutable variables, yetinternal/admin/handler.go:209-232uses them as the runtime OSS/EE dispatch contract and the same type is exposed by the version endpoints. Any importing package can reassign these values and silently change routing or response data. Declare them asconstinstead.Proposed fix
-var OpenSourceVersion = "open source" -var EnterpriseEdition = "enterprise edition" +const OpenSourceVersion = "open source" +const EnterpriseEdition = "enterprise edition"🤖 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/common/version_ee.go` around lines 18 - 22, Change OpenSourceVersion and EnterpriseEdition from exported mutable variables to string constants, preserving their existing names and values so GetRAGFlowType and the OSS/EE dispatch and version endpoint contracts remain unchanged.
🤖 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.
Nitpick comments:
In `@internal/common/version_ee.go`:
- Around line 18-22: Change OpenSourceVersion and EnterpriseEdition from
exported mutable variables to string constants, preserving their existing names
and values so GetRAGFlowType and the OSS/EE dispatch and version endpoint
contracts remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 434995b0-2a05-4bec-bac4-7114f1132a3e
📒 Files selected for processing (1)
internal/common/version_ee.go
1 participant
Summary