Skip to content

Go: fix bugs - #16911

Merged
JinHai-CN merged 2 commits into
infiniflow:mainfrom
JinHai-CN:fix547
Jul 14, 2026
Merged

Go: fix bugs#16911
JinHai-CN merged 2 commits into
infiniflow:mainfrom
JinHai-CN:fix547

Conversation

@JinHai-CN

Copy link
Copy Markdown
Contributor

Summary

  1. Fix 'list users' in admin CLI
  2. Change 3 slow test cases priority.

Signed-off-by: Jin Hai <haijin.chn@gmail.com>
@JinHai-CN JinHai-CN added the ci Continue Integration label Jul 14, 2026
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Admin user-listing responses now terminate correctly by edition, message queue health checks handle an uninitialized engine, and CLI errors include response bodies. Three RESTful API tests are reclassified from p2 to p3.

Changes

Admin flow and diagnostics

Layer / File(s) Summary
ListUsers response control flow
internal/admin/handler.go
The open-source branch returns after its success response, while the shared post-switch response is removed.
Admin status and error diagnostics
internal/admin/service.go, internal/cli/admin_command.go
Nil message queue engines receive an explicit status response, and JSON parsing errors include the raw response body.

Test priority markers

Layer / File(s) Summary
RESTful API test marker updates
test/testcases/restful_api/*, test/testcases/test_http_api/test_dataset_management/*
Three tests are moved from the p2 marker to p3.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: buua436

Poem

A rabbit checks the queue at dawn,
Finds absent engines safely gone.
List users hops, then leaves the track,
Error crumbs now lead the trail back.
Three tests wear p3 with pride—
Ears up, changes verified!

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title is too generic to convey the actual changes, though it is broadly related to bug fixes in Go. Use a specific title like "Fix admin CLI list-users and reprioritize three slow tests".
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description includes the required Summary section and clearly lists the two main changes.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@JinHai-CN
JinHai-CN marked this pull request as ready for review July 14, 2026 11:31
@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. 🐞 bug Something isn't working, pull request that fix bug. labels Jul 14, 2026
@JinHai-CN
JinHai-CN marked this pull request as draft July 14, 2026 11:32
@JinHai-CN
JinHai-CN marked this pull request as ready for review July 14, 2026 11:32

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/admin/handler.go (1)

221-230: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Restore the enterprise success response.

After ListUsersEE succeeds, the handler exits the switch without calling common.SuccessWithData, leaving enterprise requests with an empty response. The CLI in internal/cli/admin_command.go then fails to unmarshal the response. Add the success response and an explicit return to the enterprise branch.

Suggested fix
 	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
 		}
+		common.SuccessWithData(c, users, "List users")
+		return
 	default:
🤖 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 221 - 230, Update the
common.EnterpriseEdition branch in the handler after a successful ListUsersEE
call to send the users through common.SuccessWithData and then explicitly
return. Preserve the existing error response and default invalid-type handling.
🤖 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.

Outside diff comments:
In `@internal/admin/handler.go`:
- Around line 221-230: Update the common.EnterpriseEdition branch in the handler
after a successful ListUsersEE call to send the users through
common.SuccessWithData and then explicitly return. Preserve the existing error
response and default invalid-type handling.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 87e17b35-c148-413c-ab05-675ca7a5973b

📥 Commits

Reviewing files that changed from the base of the PR and between 26161ce and de34566.

📒 Files selected for processing (6)
  • internal/admin/handler.go
  • internal/admin/service.go
  • internal/cli/admin_command.go
  • test/testcases/restful_api/test_dify_retrieval_routes_unit.py
  • test/testcases/restful_api/test_openai_compatible.py
  • test/testcases/test_http_api/test_dataset_management/test_dify_retrieval_routes_unit.py

@JinHai-CN JinHai-CN removed the ci Continue Integration label Jul 14, 2026
@JinHai-CN JinHai-CN added the ci Continue Integration label Jul 14, 2026
@codecov

codecov Bot commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.16%. Comparing base (55d5879) to head (a70e975).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #16911   +/-   ##
=======================================
  Coverage   93.16%   93.16%           
=======================================
  Files          10       10           
  Lines         717      717           
  Branches      118      118           
=======================================
  Hits          668      668           
  Misses         29       29           
  Partials       20       20           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JinHai-CN
JinHai-CN merged commit c242669 into infiniflow:main Jul 14, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🐞 bug Something isn't working, pull request that fix bug. ci Continue Integration size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant