diff --git a/internal/handler/faq.go b/internal/handler/faq.go index 022fcaf31..11a0f6ffb 100644 --- a/internal/handler/faq.go +++ b/internal/handler/faq.go @@ -2,11 +2,13 @@ package handler import ( "context" + stderrors "errors" "net/http" "strconv" "github.com/gin-gonic/gin" + "github.com/Tencent/WeKnora/internal/application/repository" "github.com/Tencent/WeKnora/internal/errors" "github.com/Tencent/WeKnora/internal/logger" "github.com/Tencent/WeKnora/internal/types" @@ -51,6 +53,14 @@ func (h *FAQHandler) effectiveCtxForKB(c *gin.Context, kbID string, requiredPerm } kb, err := h.kbService.GetKnowledgeBaseByID(ctx, kbID) if err != nil { + // ErrKnowledgeBaseNotFound is the expected response for a stale + // or probed kb id; the FAQ endpoints went out of their way to + // surface internal-server-error for those, which both confused + // clients (real 5xx vs. wrong URL) and burned ops attention on + // monitoring alerts. Mirror knowledgebase.go's mapping. + if stderrors.Is(err, repository.ErrKnowledgeBaseNotFound) { + return nil, errors.NewNotFoundError("knowledge base not found") + } logger.ErrorWithFields(ctx, err, nil) return nil, errors.NewInternalServerError(err.Error()) } diff --git a/internal/handler/initialization.go b/internal/handler/initialization.go index db8635cbe..c4018b152 100644 --- a/internal/handler/initialization.go +++ b/internal/handler/initialization.go @@ -3,6 +3,7 @@ package handler import ( "context" "encoding/json" + stderrors "errors" "fmt" "io" "math/rand" @@ -13,6 +14,7 @@ import ( "sync" "time" + "github.com/Tencent/WeKnora/internal/application/repository" chatpipeline "github.com/Tencent/WeKnora/internal/application/service/chat_pipeline" "github.com/Tencent/WeKnora/internal/assets" "github.com/Tencent/WeKnora/internal/config" @@ -505,6 +507,13 @@ func (h *InitializationHandler) bindInitializationRequest(ctx context.Context, c func (h *InitializationHandler) getKnowledgeBaseForInitialization(ctx context.Context, kbIdStr string) (*types.KnowledgeBase, error) { kb, err := h.kbService.GetKnowledgeBaseByID(ctx, kbIdStr) if err != nil { + // The repo's not-found sentinel must surface as 404, not 500. + // Without this, every probe of a stale kb id from the + // initialization flow burns ops attention with a fake server + // error. See knowledgebase.go:validateAndGetKnowledgeBase. + if stderrors.Is(err, repository.ErrKnowledgeBaseNotFound) { + return nil, errors.NewNotFoundError("知识库不存在") + } logger.ErrorWithFields(ctx, err, map[string]interface{}{"kbId": utils.SanitizeForLog(kbIdStr)}) return nil, errors.NewInternalServerError("获取知识库信息失败: " + err.Error()) } @@ -1303,6 +1312,12 @@ func (h *InitializationHandler) GetCurrentConfigByKB(c *gin.Context) { // 获取指定知识库信息 kb, err := h.kbService.GetKnowledgeBaseByID(ctx, kbIdStr) if err != nil { + // Mirror getKnowledgeBaseForInitialization above: missing / + // cross-tenant kb ids are 404, not 500. + if stderrors.Is(err, repository.ErrKnowledgeBaseNotFound) { + c.Error(errors.NewNotFoundError("知识库不存在")) + return + } logger.Error(ctx, "Failed to get knowledge base", err) c.Error(errors.NewInternalServerError("获取知识库信息失败: " + err.Error())) return diff --git a/internal/handler/knowledge.go b/internal/handler/knowledge.go index d94180f16..0b7d0b10f 100644 --- a/internal/handler/knowledge.go +++ b/internal/handler/knowledge.go @@ -76,6 +76,13 @@ func (h *KnowledgeHandler) validateKnowledgeBaseAccessWithKBID(c *gin.Context, k } kb, err := h.kbService.GetKnowledgeBaseByID(ctx, kbID) if err != nil { + // Same not-found-vs-real-error split as knowledgebase.go's + // validateAndGetKnowledgeBase: ErrKnowledgeBaseNotFound is the + // expected outcome for a probed/stale kb id and must surface as + // 404, not the generic 500 the original code produced. + if goerrors.Is(err, repository.ErrKnowledgeBaseNotFound) { + return nil, kbID, 0, "", errors.NewNotFoundError("knowledge base not found") + } logger.ErrorWithFields(ctx, err, nil) return nil, kbID, 0, "", errors.NewInternalServerError(err.Error()) } diff --git a/internal/handler/knowledgebase.go b/internal/handler/knowledgebase.go index 3abd0447e..2b64a925d 100644 --- a/internal/handler/knowledgebase.go +++ b/internal/handler/knowledgebase.go @@ -181,6 +181,14 @@ func (h *KnowledgeBaseHandler) validateAndGetKnowledgeBase(c *gin.Context) (*typ // Verify tenant has permission to access this knowledge base kb, err := h.service.GetKnowledgeBaseByID(ctx, id) if err != nil { + // repo.GetKnowledgeBaseByID surfaces ErrKnowledgeBaseNotFound for + // missing or cross-tenant rows. Map it to 404 here so the four + // callers (Get / Update / Delete / TogglePin / Copy / Hybrid-search + // path) don't have to wrap NewInternalServerError into a 500 for + // every probe of a non-existent id. + if stderrors.Is(err, repository.ErrKnowledgeBaseNotFound) { + return nil, id, 0, "", apperrors.NewNotFoundError("knowledge base not found") + } logger.ErrorWithFields(ctx, err, nil) return nil, id, 0, "", apperrors.NewInternalServerError(err.Error()) } diff --git a/internal/handler/knowledgebase_not_found_test.go b/internal/handler/knowledgebase_not_found_test.go new file mode 100644 index 000000000..eb6f7a281 --- /dev/null +++ b/internal/handler/knowledgebase_not_found_test.go @@ -0,0 +1,135 @@ +package handler + +import ( + "context" + stderrors "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gin-gonic/gin" + + "github.com/Tencent/WeKnora/internal/application/repository" + apperrors "github.com/Tencent/WeKnora/internal/errors" + "github.com/Tencent/WeKnora/internal/middleware" + "github.com/Tencent/WeKnora/internal/types" + "github.com/Tencent/WeKnora/internal/types/interfaces" +) + +// validateAndGetKnowledgeBase (knowledgebase.go) and the four sibling +// helpers in faq.go / knowledge.go / tag.go / initialization.go used to +// wrap every Get*ByID error — including the well-known +// repository.ErrKnowledgeBaseNotFound sentinel — as a 500. That turned +// every probe of a stale or cross-tenant KB id into a fake "internal +// server error" envelope. These tests pin the corrected mapping (404) +// at the HTTP boundary so a future refactor can't quietly regress it. +// +// The wrapped-sentinel cases are the actual security boundary: if a +// caller drops the stderrors.Is comparison and reverts to `==`, the +// fmt.Errorf("%w") path silently fails over to 500 and the tests below +// fail before the change ships. + +// stubKBOnlyService implements just enough of KnowledgeBaseService to +// drive validateAndGetKnowledgeBase. Embedding the interface keeps every +// other method nil-panicky on purpose so a future test that reaches +// outside the contract fails loudly. +type stubKBOnlyService struct { + interfaces.KnowledgeBaseService + getByID func(ctx context.Context, id string) (*types.KnowledgeBase, error) + fillKnowledgeBaseCounts func(ctx context.Context, kb *types.KnowledgeBase) error +} + +func (s *stubKBOnlyService) GetKnowledgeBaseByID(ctx context.Context, id string) (*types.KnowledgeBase, error) { + return s.getByID(ctx, id) +} + +func (s *stubKBOnlyService) FillKnowledgeBaseCounts(ctx context.Context, kb *types.KnowledgeBase) error { + if s.fillKnowledgeBaseCounts != nil { + return s.fillKnowledgeBaseCounts(ctx, kb) + } + return nil +} + +// newKBHandlerTestRouter mounts the production ErrorHandler so +// c.Error(NewNotFoundError(...)) renders as the real 404 envelope. +// Tenant id and user id are injected by a tiny middleware so +// validateAndGetKnowledgeBase doesn't bail at the unauthorized branch. +func newKBHandlerTestRouter(svc interfaces.KnowledgeBaseService) *gin.Engine { + gin.SetMode(gin.TestMode) + r := gin.New() + r.Use(middleware.ErrorHandler()) + r.Use(func(c *gin.Context) { + c.Set(types.TenantIDContextKey.String(), uint64(1)) + c.Set(types.UserIDContextKey.String(), "u-test") + c.Next() + }) + h := &KnowledgeBaseHandler{service: svc} + r.GET("/knowledge-bases/:id", h.GetKnowledgeBase) + return r +} + +func TestKBHandlerMapsErrKnowledgeBaseNotFoundToNotFound(t *testing.T) { + svc := &stubKBOnlyService{ + getByID: func(_ context.Context, _ string) (*types.KnowledgeBase, error) { + return nil, repository.ErrKnowledgeBaseNotFound + }, + } + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/knowledge-bases/missing-kb", nil) + newKBHandlerTestRouter(svc).ServeHTTP(w, req) + if w.Code != http.StatusNotFound { + t.Fatalf("ErrKnowledgeBaseNotFound must map to 404, got %d body=%s", w.Code, w.Body.String()) + } +} + +func TestKBHandlerHonoursWrappedErrKnowledgeBaseNotFound(t *testing.T) { + // Regression test against a stale `==` sentinel comparison: errors.Is + // unwraps fmt.Errorf("%w", ...); a literal `==` does not. If anyone + // reverts the comparison this test fails before the change ships. + svc := &stubKBOnlyService{ + getByID: func(_ context.Context, _ string) (*types.KnowledgeBase, error) { + return nil, fmt.Errorf("loading kb: %w", repository.ErrKnowledgeBaseNotFound) + }, + } + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/knowledge-bases/missing-kb", nil) + newKBHandlerTestRouter(svc).ServeHTTP(w, req) + if w.Code != http.StatusNotFound { + t.Fatalf("wrapped ErrKnowledgeBaseNotFound must still map to 404, got %d body=%s", + w.Code, w.Body.String()) + } + // Defence-in-depth: the response body should expose the AppError + // envelope (ErrorHandler renders {success,error:{code,message}}), + // not the bare gin error string. Code 1003 is ErrNotFound. + if !strings.Contains(w.Body.String(), `"code":1003`) { + t.Fatalf("expected NotFound envelope with code=1003, got body=%s", w.Body.String()) + } +} + +func TestKBHandlerKeeps500ForGenuineInfraErrors(t *testing.T) { + // The mapping is *only* for the not-found sentinel — every other + // error must still surface as a real 5xx so monitoring catches + // genuine DB / repo failures. + svc := &stubKBOnlyService{ + getByID: func(_ context.Context, _ string) (*types.KnowledgeBase, error) { + return nil, stderrors.New("connection refused") + }, + } + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/knowledge-bases/some-kb", nil) + newKBHandlerTestRouter(svc).ServeHTTP(w, req) + if w.Code != http.StatusInternalServerError { + t.Fatalf("non-sentinel errors must remain 500, got %d body=%s", w.Code, w.Body.String()) + } +} + +// guardAgainstStaleSentinelEquality is a compile-time assertion: if a +// future refactor accidentally drops the apperrors / stderrors imports +// from THIS file, the test stops compiling and the human gets a clear +// signal that something tied to the not-found mapping went away. +var ( + _ = stderrors.Is + _ = apperrors.NewNotFoundError +) diff --git a/internal/handler/tag.go b/internal/handler/tag.go index 25b3e7b25..95f4f5bd4 100644 --- a/internal/handler/tag.go +++ b/internal/handler/tag.go @@ -2,11 +2,13 @@ package handler import ( "context" + stderrors "errors" "net/http" "strconv" "github.com/gin-gonic/gin" + "github.com/Tencent/WeKnora/internal/application/repository" "github.com/Tencent/WeKnora/internal/errors" "github.com/Tencent/WeKnora/internal/logger" "github.com/Tencent/WeKnora/internal/types" @@ -55,6 +57,12 @@ func (h *TagHandler) effectiveCtxForKB(c *gin.Context, kbID string) (context.Con } kb, err := h.kbService.GetKnowledgeBaseByID(ctx, kbID) if err != nil { + // Same not-found-vs-real-error split as the other helper sites: + // a missing or cross-tenant kb id should render as 404 so clients + // can tell "wrong URL" from a real 5xx. + if stderrors.Is(err, repository.ErrKnowledgeBaseNotFound) { + return nil, errors.NewNotFoundError("knowledge base not found") + } logger.ErrorWithFields(ctx, err, nil) return nil, errors.NewInternalServerError(err.Error()) }