Skip to content

Fix serve trace empty match to return 200 with empty JSON result - #127

Open
gyp15618976002 wants to merge 1 commit into
c2j:mainfrom
gyp15618976002:fix/issue-125-trace-empty-result
Open

gyp15618976002 wants to merge 1 commit into
c2j:mainfrom
gyp15618976002:fix/issue-125-trace-empty-result

Conversation

@gyp15618976002

Copy link
Copy Markdown

125问题修复

@c2j c2j left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

⚠️ 建议在 PR #128 合并后 rebase

验证结果

检查项 结果
cargo check --features serve
集成测试 serve_api ✅ 8/8

代码评审

trace 空匹配返回 200: ✅

  • 将"查询无结果"视为正常业务状态(200)而非资源不存在(404),与 search-sql 端点行为一致
  • 返回结构与正常 trace 响应完全一致(target=null, callers=[], callees=[], caller_count=0, callee_count=0, truncated=false)

⚠️ 需要关注

  1. 路由顺序: 此 PR 基于旧的主分支,.fallback() 仍在 .layer(middleware) 之后,导致 404 请求绕过 access log。PR #128 已修复此问题。建议在 #128 合并后 rebase。

  2. 测试端口冲突: test_serve_trace_empty_match_returns_200 使用 port 19883,与 #128test_serve_access_log_combined_format 相同。rebase 后需改为不同端口(如 19884)。

建议操作

# 在 #128 合并后:
git fetch origin main
git rebase origin/main
# 修改测试端口为 19884
cargo test --features serve --test serve_api -- --test-threads=1

@c2j c2j left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

PR #127 Review — Fix serve trace empty match to return 200 with empty JSON result

总结:/api/v1/trace 在无匹配时从 404 改为 200 并返回空结构体;测试已新增验证此行为。

需要作者处理:

  1. 请在 PR 描述或 CHANGELOG 明确为何将 404 改为 200(兼容性/语义原因),并列出受影响的已知客户端场景。
  2. handler 签名由 Result<impl IntoResponse, StatusCode> 改为 Json,请确认路由注册与中间件兼容;若项目其他 handler 仍使用 Result,建议保持一致或在 PR 描述说明原因。
  3. 建议再增加一个测试,确保在出现内部错误时仍返回合适的错误状态码,而不是统一映射为 200。
  4. 当前 PR 状态为 CONFLICTING,请先 rebase/resolve 与 main 的冲突并重新 push。
  5. 在 PR 里列出本地复现命令输出(cargo build / cargo test --features serve)的快照,便于审查者核对。

结论建议:在完成上述项后再合并。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants