infiniflow / infiniflow/ragflow

[P1] Converge two RetrievalTest handler implementations and unify error code convention

Open
#15,744 3 comments 1 reaction 0 assignees View on GitHub
☯️ refactor 🐖api
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.