Files
WeKnora/internal/handler/knowledgebase_not_found_test.go
wizardchen 4fb089d4d7 fix(kb): map ErrKnowledgeBaseNotFound to 404 across handler helpers
Five handler helpers (validateAndGetKnowledgeBase in knowledgebase.go,
validateKnowledgeBaseAccessWithKBID in knowledge.go, the kbService
guards in faq.go and tag.go, and getKnowledgeBaseForInitialization +
the per-kb config getter in initialization.go) wrapped every
GetKnowledgeBaseByID error — including the well-known
repository.ErrKnowledgeBaseNotFound sentinel — as
NewInternalServerError. The result was that every probe of a stale or
cross-tenant kb id surfaced as a 500 instead of the 404 it should have
been, both confusing clients ("real 5xx vs. wrong URL") and burning
ops attention on monitoring alerts.

The mapping pattern is the same as PR #1336 for sessions: detect the
sentinel via stderrors.Is and emit NewNotFoundError; everything else
still surfaces as a 500 so genuine DB / repo failures keep firing the
alerts that matter. Caught during the RBAC e2e smoke run on
feat/rbac, where a deliberate cross-tenant kb-id probe produced
HTTP 500 + body "knowledge base not found" — the smoking gun.

Tests: new internal/handler/knowledgebase_not_found_test.go covers
three cases — bare sentinel, fmt.Errorf("%w") wrapped sentinel
(regression guard against a future revert to `==`), and a non-sentinel
infrastructure error that must still 500. All three pass.

The full handler test package is green.
2026-05-14 20:12:41 +08:00

136 lines
5.2 KiB
Go

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
)