diff --git a/internal/handler/message.go b/internal/handler/message.go index 11838cc4..ef2a5978 100644 --- a/internal/handler/message.go +++ b/internal/handler/message.go @@ -1,6 +1,7 @@ package handler import ( + stderrors "errors" "net/http" "strconv" "time" @@ -70,6 +71,15 @@ func (h *MessageHandler) LoadMessages(c *gin.Context) { logger.Infof(ctx, "Getting recent messages for session, session ID: %s, limit: %d", sessionID, limitInt) messages, err := h.MessageService.GetRecentMessagesBySession(ctx, sessionID, limitInt) if err != nil { + if stderrors.Is(err, errors.ErrSessionNotFound) { + // PR #1309 plumbed user-scope into the message service's + // session existence check; non-owner / wrong-tenant lookups + // surface as ErrSessionNotFound. Map to 404 so clients can + // tell "wrong URL" from a real 5xx. + logger.Warnf(ctx, "Session not found, ID: %s", sessionID) + c.Error(errors.NewNotFoundError(err.Error())) + return + } logger.ErrorWithFields(ctx, err, nil) c.Error(errors.NewInternalServerError(err.Error())) return @@ -104,6 +114,12 @@ func (h *MessageHandler) LoadMessages(c *gin.Context) { sessionID, beforeTime.Format(time.RFC3339Nano), limitInt) messages, err := h.MessageService.GetMessagesBySessionBeforeTime(ctx, sessionID, beforeTime, limitInt) if err != nil { + if stderrors.Is(err, errors.ErrSessionNotFound) { + // See note on the GetRecentMessagesBySession path above. + logger.Warnf(ctx, "Session not found, ID: %s", sessionID) + c.Error(errors.NewNotFoundError(err.Error())) + return + } logger.ErrorWithFields(ctx, err, nil) c.Error(errors.NewInternalServerError(err.Error())) return @@ -146,6 +162,14 @@ func (h *MessageHandler) DeleteMessage(c *gin.Context) { // Delete the message using the message service if err := h.MessageService.DeleteMessage(ctx, sessionID, messageID); err != nil { + if stderrors.Is(err, errors.ErrSessionNotFound) { + // See note on LoadMessages above — message-service operations + // surface ErrSessionNotFound when the caller can't see the + // owning session (post-#1309 user scope). + logger.Warnf(ctx, "Session not found, ID: %s", sessionID) + c.Error(errors.NewNotFoundError(err.Error())) + return + } logger.ErrorWithFields(ctx, err, nil) c.Error(errors.NewInternalServerError(err.Error())) return diff --git a/internal/handler/message_session_not_found_test.go b/internal/handler/message_session_not_found_test.go new file mode 100644 index 00000000..a614ba05 --- /dev/null +++ b/internal/handler/message_session_not_found_test.go @@ -0,0 +1,146 @@ +package handler + +import ( + "context" + stderrors "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/gin-gonic/gin" + + 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" +) + +// stubMessageService satisfies just the four MessageService methods this +// file exercises. Embedding the interface keeps the rest nil-panicky on +// purpose: any test that reaches an un-stubbed method should fail loudly +// rather than silently returning zero-values. +type stubMessageService struct { + interfaces.MessageService + getRecent func(ctx context.Context, sessionID string, limit int) ([]*types.Message, error) + getBeforeTime func(ctx context.Context, sessionID string, beforeTime time.Time, limit int) ([]*types.Message, error) + deleteMessage func(ctx context.Context, sessionID, id string) error +} + +func (s *stubMessageService) GetRecentMessagesBySession( + ctx context.Context, sessionID string, limit int, +) ([]*types.Message, error) { + return s.getRecent(ctx, sessionID, limit) +} + +func (s *stubMessageService) GetMessagesBySessionBeforeTime( + ctx context.Context, sessionID string, beforeTime time.Time, limit int, +) ([]*types.Message, error) { + return s.getBeforeTime(ctx, sessionID, beforeTime, limit) +} + +func (s *stubMessageService) DeleteMessage(ctx context.Context, sessionID, id string) error { + return s.deleteMessage(ctx, sessionID, id) +} + +// newMessageTestRouter mounts the standard ErrorHandler middleware so +// c.Error(NewNotFoundError(...)) renders as a real 404 envelope — +// matching production routing exactly. +func newMessageTestRouter(svc interfaces.MessageService) *gin.Engine { + gin.SetMode(gin.TestMode) + r := gin.New() + r.Use(middleware.ErrorHandler()) + h := &MessageHandler{MessageService: svc} + r.GET("/messages/:session_id/load", h.LoadMessages) + r.DELETE("/messages/:session_id/:id", h.DeleteMessage) + return r +} + +// TestLoadMessagesMapsErrSessionNotFoundToNotFound pins the fix for +// PR #1309's surfacing gap: when message-service ops can't see the +// owning session (non-owner / wrong tenant), the handler used to +// surface a 500 because ErrSessionNotFound flowed through the generic +// error branch. We now map it to 404 so clients can tell "wrong URL" +// from a real 5xx. +func TestLoadMessagesMapsErrSessionNotFoundToNotFound(t *testing.T) { + svc := &stubMessageService{ + getRecent: func(_ context.Context, _ string, _ int) ([]*types.Message, error) { + return nil, apperrors.ErrSessionNotFound + }, + } + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/messages/sess1/load?limit=20", nil) + newMessageTestRouter(svc).ServeHTTP(w, req) + if w.Code != http.StatusNotFound { + t.Fatalf("ErrSessionNotFound must map to 404, got %d body=%s", w.Code, w.Body.String()) + } +} + +// TestLoadMessagesBeforeTimeMapsErrSessionNotFoundToNotFound covers the +// second branch of LoadMessages (the `before_time` path), which has its +// own error-handling block. +func TestLoadMessagesBeforeTimeMapsErrSessionNotFoundToNotFound(t *testing.T) { + svc := &stubMessageService{ + getBeforeTime: func(_ context.Context, _ string, _ time.Time, _ int) ([]*types.Message, error) { + return nil, apperrors.ErrSessionNotFound + }, + } + w := httptest.NewRecorder() + url := "/messages/sess1/load?limit=20&before_time=" + time.Now().UTC().Format(time.RFC3339Nano) + req := httptest.NewRequest(http.MethodGet, url, nil) + newMessageTestRouter(svc).ServeHTTP(w, req) + if w.Code != http.StatusNotFound { + t.Fatalf("ErrSessionNotFound on before-time path must map to 404, got %d body=%s", + w.Code, w.Body.String()) + } +} + +// TestDeleteMessageMapsErrSessionNotFoundToNotFound mirrors the +// LoadMessages test for the delete endpoint. +func TestDeleteMessageMapsErrSessionNotFoundToNotFound(t *testing.T) { + svc := &stubMessageService{ + deleteMessage: func(_ context.Context, _, _ string) error { + return apperrors.ErrSessionNotFound + }, + } + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodDelete, "/messages/sess1/msg1", nil) + newMessageTestRouter(svc).ServeHTTP(w, req) + if w.Code != http.StatusNotFound { + t.Fatalf("ErrSessionNotFound on DeleteMessage must map to 404, got %d body=%s", + w.Code, w.Body.String()) + } +} + +// TestDeleteMessageHonoursWrappedErrSessionNotFound is the regression +// test against the stale `==` sentinel comparison the original PR used. +// errors.Is unwraps fmt.Errorf("%w", ...); a literal `==` does not. If +// anyone reverts the comparison, this test fails before the change ships. +func TestDeleteMessageHonoursWrappedErrSessionNotFound(t *testing.T) { + svc := &stubMessageService{ + deleteMessage: func(_ context.Context, _, _ string) error { + return fmt.Errorf("delete: %w", apperrors.ErrSessionNotFound) + }, + } + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodDelete, "/messages/sess1/msg1", nil) + newMessageTestRouter(svc).ServeHTTP(w, req) + if w.Code != http.StatusNotFound { + t.Fatalf("wrapped ErrSessionNotFound 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. + if !strings.Contains(w.Body.String(), `"code":1003`) { + t.Fatalf("expected NotFound envelope with code=1003, got body=%s", w.Body.String()) + } +} + +// guardAgainstStaleSentinelEquality is a compile-time assertion: if a +// future refactor accidentally drops the stderrors import, the test file +// stops compiling and the human gets a clear signal that something tied +// to errors.Is wrapping went away. +var _ = stderrors.Is diff --git a/internal/handler/session/handler.go b/internal/handler/session/handler.go index 117cb42e..51c2bed9 100644 --- a/internal/handler/session/handler.go +++ b/internal/handler/session/handler.go @@ -1,6 +1,7 @@ package session import ( + stderrors "errors" "net/http" "github.com/Tencent/WeKnora/internal/config" @@ -160,7 +161,7 @@ func (h *Handler) GetSession(c *gin.Context) { logger.Infof(ctx, "Retrieving session, ID: %s", id) session, err := h.sessionService.GetSession(ctx, id) if err != nil { - if err == errors.ErrSessionNotFound { + if stderrors.Is(err, errors.ErrSessionNotFound) { logger.Warnf(ctx, "Session not found, ID: %s", id) c.Error(errors.NewNotFoundError(err.Error())) return @@ -275,7 +276,7 @@ func (h *Handler) UpdateSession(c *gin.Context) { // Call service to update session if err := h.sessionService.UpdateSession(ctx, &session); err != nil { - if err == errors.ErrSessionNotFound { + if stderrors.Is(err, errors.ErrSessionNotFound) { logger.Warnf(ctx, "Session not found, ID: %s", id) c.Error(errors.NewNotFoundError(err.Error())) return @@ -326,7 +327,7 @@ func (h *Handler) DeleteSession(c *gin.Context) { // Call service to delete session if err := h.sessionService.DeleteSession(ctx, id); err != nil { - if err == errors.ErrSessionNotFound { + if stderrors.Is(err, errors.ErrSessionNotFound) { logger.Warnf(ctx, "Session not found, ID: %s", id) c.Error(errors.NewNotFoundError(err.Error())) return @@ -369,6 +370,11 @@ func (h *Handler) ClearSessionMessages(c *gin.Context) { logger.Infof(ctx, "Clearing all messages for session: %s", id) if err := h.messageService.ClearSessionMessages(ctx, id); err != nil { + if stderrors.Is(err, errors.ErrSessionNotFound) { + logger.Warnf(ctx, "Session not found, ID: %s", id) + c.Error(errors.NewNotFoundError(err.Error())) + return + } logger.ErrorWithFields(ctx, err, map[string]interface{}{"session_id": id}) c.Error(errors.NewInternalServerError(err.Error())) return @@ -442,7 +448,7 @@ func (h *Handler) BatchDeleteSessions(c *gin.Context) { } if err := h.sessionService.BatchDeleteSessions(ctx, sanitizedIDs); err != nil { - if err == errors.ErrSessionNotFound { + if stderrors.Is(err, errors.ErrSessionNotFound) { logger.Warnf(ctx, "No visible sessions found for batch delete") c.Error(errors.NewNotFoundError(err.Error())) return diff --git a/internal/handler/session/stream.go b/internal/handler/session/stream.go index 3da31e02..2269ecd4 100644 --- a/internal/handler/session/stream.go +++ b/internal/handler/session/stream.go @@ -2,6 +2,7 @@ package session import ( "context" + stderrors "errors" "fmt" "net/http" "time" @@ -54,7 +55,7 @@ func (h *Handler) ContinueStream(c *gin.Context) { // Verify that the session exists and belongs to this tenant _, err := h.sessionService.GetSession(ctx, sessionID) if err != nil { - if err == errors.ErrSessionNotFound { + if stderrors.Is(err, errors.ErrSessionNotFound) { logger.Warnf(ctx, "Session not found, ID: %s", sessionID) c.Error(errors.NewNotFoundError(err.Error())) } else { @@ -67,6 +68,15 @@ func (h *Handler) ContinueStream(c *gin.Context) { // Get the incomplete message message, err := h.messageService.GetMessage(ctx, sessionID, messageID) if err != nil { + if stderrors.Is(err, errors.ErrSessionNotFound) { + // PR #1309 plumbed user-scope into messageService.GetMessage's + // session existence check; non-owner / wrong-user lookups now + // surface as ErrSessionNotFound. Map to 404 so clients can tell + // "wrong URL" from a real 5xx instead of seeing a generic 500. + logger.Warnf(ctx, "Session not found, ID: %s", sessionID) + c.Error(errors.NewNotFoundError(err.Error())) + return + } logger.ErrorWithFields(ctx, err, nil) c.Error(errors.NewInternalServerError(err.Error())) return