infiniflow / infiniflow/ragflow
[P1] Converge two RetrievalTest handler implementations and unify error code convention
- Dominant language
- Go
- Stars
- 91k
- Forks
- 10.8k
- Avg merge
- 1d 14h
- Merged PRs (30d)
- 705
Description
> **Note:** This is for internal engineering discussion. External contributors are welcome to read, but prioritization is managed internally.
## Problem
Two Go handler endpoints call the same `service.ChunkService.RetrievalTest()` but have diverged across nearly every dimension:
| Dimension | `chunkHandler` (`/datasets/search`) | `searchBotHandler` (`/searchbots/retrieval_test`) |
|---|---|---|
| Request DTO | Direct bind to `service.RetrievalTestRequest` | Custom DTO + manual mapping |
| JSON tag | `dataset_ids` | `kb_id` (Python compat) |
| kb_id type | `[]string` (array only) | `StringSlice` (string or array) |
| Auth failure | `jsonError` → 200 | `c.JSON(401, ...)` |
| Validation errors | Hardcoded `400` | `common.CodeArgumentError` (101) |
| Service errors | `err.Error()` leaked | Generic message + `Warn` |
| Defaults | Inline 4 fields | Extracted function 7 fields |
| Error response shape | No `"data"` field | Has `"data": nil` |
| Success code | `0` literal | `int(common.CodeSuccess)` |
The divergence means:
1. **Same business operation, different API contract** — frontend/SDK needs two error-handling paths
2. **Security posture differs** — one endpoint leaks internal details, the other protects them
3. **Improvements flow one way** — fixes to one handler do not propagate to the other
4. **Error code confusion** — `101` vs `400` for the same class of error
## Two sub-issues
### A. Handler divergence
- Both call `service.ChunkService.RetrievalTest()` — extract a shared adapter
- `service.CallRetrievalTest(req, userID)` wrapping `Warn` + generic error
- Keep separate DTOs (different JSON tags are intentional per Python API compat)
### B. Error code convention across all handlers
Current state mixed across the codebase:
- Some handlers use `common.CodeArgumentError` (101)
- Some use hardcoded `400`
- Some use `jsonError()` which returns HTTP 200 with error code
- Some use `c.JSON(400/500)` directly
Need a single convention documented and enforced across all Go handlers.
## Proposed Solution
1. Extract `service.CallRetrievalTest()` — shared error handling wrapper
2. Unify chunkHandler error codes to `common.CodeArgumentError` / `common.CodeServerError`
3. Document error code convention for all handlers
4. Add lint rule to enforce convention
## Related
- P0: #15743 (systemic `err.Error()` leak)
(Identified during CTO-level code review of `feat/searchbots-retrieval-test` branch)
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by comparing the named chunkHandler and searchBotHandler implementations and tracing their calls to service.ChunkService.RetrievalTest(). Review the proposed service.CallRetrievalTest() boundary, then survey the other Go handlers for existing error-code patterns. Done means the two handlers share the intended adapter, the convention is documented and consistently enforced, and both endpoints retain their intentional DTO differences.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- api, backend, security, tooling
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100