From 1dfccf513ab8ebd8c7e8acd088b16e5ecc426793 Mon Sep 17 00:00:00 2001 From: Huxley Date: Sat, 4 Jul 2026 16:24:22 +0800 Subject: [PATCH] Add structured logging and centralized error handling - Initialize slog in the serve command with terminal/file support - Introduce `AppError` with HTTP status for unified service-layer errors - Replace ad-hoc `api.Error` calls with `api.RespondError` - Wrap internal errors with `model.NewInternalError` and add reference IDs - Add `RequestID` middleware and switch router from `gin.Default` to `gin.New` --- cmd/serve.go | 7 +++ config.example.yaml | 5 +++ internal/api/response.go | 48 ++++++++++++++++++++ internal/app/webapp.go | 3 +- internal/config/config.go | 38 ++++++++++++++++ internal/config/load.go | 4 ++ internal/handler/account.go | 14 +++--- internal/handler/auth.go | 21 +++++---- internal/handler/file.go | 44 +++++++----------- internal/handler/file_test.go | 2 +- internal/model/errors.go | 28 +++++++++++- internal/server/router.go | 9 +++- internal/service/auth.go | 57 ++++++++++++----------- internal/service/auth_test.go | 7 ++- internal/service/file.go | 85 +++++++++++++++++++---------------- internal/service/file_test.go | 43 ++++++++++-------- 16 files changed, 281 insertions(+), 134 deletions(-) diff --git a/cmd/serve.go b/cmd/serve.go index 682223d..ae94f44 100644 --- a/cmd/serve.go +++ b/cmd/serve.go @@ -3,6 +3,7 @@ package cmd import ( "context" "fmt" + "log/slog" "os" "os/signal" "syscall" @@ -11,6 +12,7 @@ import ( "github.com/dhao2001/mygo/internal/app" "github.com/dhao2001/mygo/internal/config" + mygolog "github.com/dhao2001/mygo/internal/log" "github.com/dhao2001/mygo/internal/server" ) @@ -26,6 +28,11 @@ var serveCmd = &cobra.Command{ return fmt.Errorf("load config: %w", err) } + // Set up structured logging before anything else. + appLogger := mygolog.NewLogger(cfg.Log) + slog.SetDefault(appLogger) + slog.Info("mygo server starting") + ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) defer stop() diff --git a/config.example.yaml b/config.example.yaml index fce6528..47340f1 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -22,6 +22,11 @@ storage: # Max upload file size in bytes. 0 = unlimited max_upload_size: 104857600 # 100 MB +log: + level: info # terminal: debug, info, warn, error + file_path: "" # empty = no file logging + file_level: debug # file: debug, info, warn, error + jwt: secret: change-me-in-production access_ttl: 15m diff --git a/internal/api/response.go b/internal/api/response.go index 478fcb7..f55022d 100644 --- a/internal/api/response.go +++ b/internal/api/response.go @@ -1,7 +1,16 @@ package api import ( + "crypto/rand" + "encoding/hex" + "errors" + "fmt" + "log/slog" + "net/http" + "github.com/gin-gonic/gin" + + "github.com/dhao2001/mygo/internal/model" ) // ErrorResponse is the standard JSON body for HTTP API errors. @@ -22,3 +31,42 @@ func Error(c *gin.Context, status int, message string) { }, }) } + +// RespondError unpacks an error from the service layer and writes a JSON +// error response. It expects *model.AppError; unexpected non-AppError +// values are treated as 500 with a safe message. For 500+ errors, a +// random reference hash is included in the response so operators can +// correlate it with server logs. +func RespondError(c *gin.Context, err error) { + var ae *model.AppError + if !errors.As(err, &ae) { + ref := randomHex(8) + slog.ErrorContext(c.Request.Context(), "non-AppError escaped to handler", + "ref", ref, "error", err, "type", fmt.Sprintf("%T", err)) + Error(c, http.StatusInternalServerError, "internal server error (ref: "+ref+")") + return + } + + // 500+ errors: log full detail with a reference hash, return sanitized message. + if ae.Status >= 500 { + ref := randomHex(8) + slog.ErrorContext(c.Request.Context(), "internal server error", + "ref", ref, slog.Any("error", ae.Err), "message", ae.Message) + Error(c, ae.Status, "internal server error (ref: "+ref+")") + return + } + + // 4xx errors with internal cause: log at WARN for diagnostics. + if ae.Err != nil { + slog.WarnContext(c.Request.Context(), ae.Message, + "status", ae.Status, slog.Any("error", ae.Err)) + } + + Error(c, ae.Status, ae.Message) +} + +func randomHex(n int) string { + b := make([]byte, n) + rand.Read(b) + return hex.EncodeToString(b) +} diff --git a/internal/app/webapp.go b/internal/app/webapp.go index 5edd262..ba554ff 100644 --- a/internal/app/webapp.go +++ b/internal/app/webapp.go @@ -2,6 +2,7 @@ package app import ( "fmt" + "log/slog" "gorm.io/gorm" @@ -56,7 +57,7 @@ func Bootstrap(cfg *config.Config) (*WebApp, error) { return nil, fmt.Errorf("init storage: %w", err) } - fileService := service.NewFileService(fileRepo, store, cfg.Storage.MaxUploadSize) + fileService := service.NewFileService(fileRepo, store, cfg.Storage.MaxUploadSize, slog.Default()) return &WebApp{ Config: cfg, diff --git a/internal/config/config.go b/internal/config/config.go index 24fdfe4..819ee67 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -3,6 +3,7 @@ package config import ( "errors" "fmt" + "log/slog" "net" "time" ) @@ -12,6 +13,7 @@ type Config struct { Database DatabaseConfig `mapstructure:"database"` Storage StorageConfig `mapstructure:"storage"` JWT JWTConfig `mapstructure:"jwt"` + Log LogConfig `mapstructure:"log"` } type ServerConfig struct { @@ -54,6 +56,30 @@ type JWTConfig struct { RefreshTTL time.Duration `mapstructure:"refresh_ttl"` } +type LogConfig struct { + Level string `mapstructure:"level"` + FilePath string `mapstructure:"file_path"` + FileLevel string `mapstructure:"file_level"` +} + +// ParseLogLevel converts a string to slog.Level. +func ParseLogLevel(s string) (slog.Level, error) { + if s == "" { + return slog.LevelInfo, nil + } + switch s { + case "debug": + return slog.LevelDebug, nil + case "info": + return slog.LevelInfo, nil + case "warn": + return slog.LevelWarn, nil + case "error": + return slog.LevelError, nil + default: + return slog.LevelInfo, fmt.Errorf("unknown log level: %q (use debug, info, warn, or error)", s) + } +} func (c *Config) Validate() error { var errs []error @@ -107,5 +133,17 @@ func (c *Config) Validate() error { errs = append(errs, errors.New("jwt.refresh_ttl: must be positive")) } + if c.Log.Level != "" { + if _, err := ParseLogLevel(c.Log.Level); err != nil { + errs = append(errs, fmt.Errorf("log.level: %w", err)) + } + } + + if c.Log.FileLevel != "" { + if _, err := ParseLogLevel(c.Log.FileLevel); err != nil { + errs = append(errs, fmt.Errorf("log.file_level: %w", err)) + } + } + return errors.Join(errs...) } diff --git a/internal/config/load.go b/internal/config/load.go index 0861549..801382f 100644 --- a/internal/config/load.go +++ b/internal/config/load.go @@ -27,6 +27,10 @@ func defaults(v *viper.Viper) { v.SetDefault("jwt.secret", "dev-secret-do-not-use-in-production") v.SetDefault("jwt.access_ttl", "15m") v.SetDefault("jwt.refresh_ttl", "168h") + + v.SetDefault("log.level", "info") + v.SetDefault("log.file_path", "") + v.SetDefault("log.file_level", "debug") } func New() *viper.Viper { diff --git a/internal/handler/account.go b/internal/handler/account.go index 92ae18a..fc9906a 100644 --- a/internal/handler/account.go +++ b/internal/handler/account.go @@ -1,6 +1,7 @@ package handler import ( + "log/slog" "net/http" "github.com/gin-gonic/gin" @@ -37,7 +38,7 @@ func (h *AccountHandler) ListPasskeys(c *gin.Context) { creds, err := h.authService.ListPasskeys(c.Request.Context(), userID) if err != nil { - api.Error(c, http.StatusInternalServerError, err.Error()) + api.RespondError(c, err) return } @@ -54,13 +55,14 @@ func (h *AccountHandler) CreatePasskey(c *gin.Context) { var req createPasskeyRequest if err := c.ShouldBindJSON(&req); err != nil { - api.Error(c, http.StatusBadRequest, "invalid request: "+err.Error()) + slog.DebugContext(c.Request.Context(), "create passkey bind failed", "error", err) + api.Error(c, http.StatusBadRequest, "invalid request body") return } pk, err := h.authService.CreatePasskey(c.Request.Context(), userID, req.Label) if err != nil { - api.Error(c, http.StatusInternalServerError, err.Error()) + api.RespondError(c, err) return } @@ -78,11 +80,7 @@ func (h *AccountHandler) RevokePasskey(c *gin.Context) { } if err := h.authService.RevokePasskey(c.Request.Context(), userID, passkeyID); err != nil { - if err == model.ErrForbidden { - api.Error(c, http.StatusForbidden, err.Error()) - return - } - api.Error(c, http.StatusInternalServerError, err.Error()) + api.RespondError(c, err) return } diff --git a/internal/handler/auth.go b/internal/handler/auth.go index 091b4b9..fe225a3 100644 --- a/internal/handler/auth.go +++ b/internal/handler/auth.go @@ -1,6 +1,7 @@ package handler import ( + "log/slog" "net/http" "github.com/gin-gonic/gin" @@ -38,13 +39,14 @@ type tokenRequest struct { func (h *AuthHandler) Register(c *gin.Context) { var req registerRequest if err := c.ShouldBindJSON(&req); err != nil { - api.Error(c, http.StatusBadRequest, "invalid request: "+err.Error()) + slog.DebugContext(c.Request.Context(), "register bind failed", "error", err) + api.Error(c, http.StatusBadRequest, "invalid request body") return } user, err := h.authService.Register(c.Request.Context(), req.Username, req.Email, req.Password) if err != nil { - api.Error(c, http.StatusConflict, err.Error()) + api.RespondError(c, err) return } @@ -55,13 +57,14 @@ func (h *AuthHandler) Register(c *gin.Context) { func (h *AuthHandler) Login(c *gin.Context) { var req loginRequest if err := c.ShouldBindJSON(&req); err != nil { - api.Error(c, http.StatusBadRequest, "invalid request: "+err.Error()) + slog.DebugContext(c.Request.Context(), "login bind failed", "error", err) + api.Error(c, http.StatusBadRequest, "invalid request body") return } pair, err := h.authService.Login(c.Request.Context(), req.Email, req.Password) if err != nil { - api.Error(c, http.StatusUnauthorized, err.Error()) + api.RespondError(c, err) return } @@ -72,13 +75,14 @@ func (h *AuthHandler) Login(c *gin.Context) { func (h *AuthHandler) Refresh(c *gin.Context) { var req tokenRequest if err := c.ShouldBindJSON(&req); err != nil { - api.Error(c, http.StatusBadRequest, "invalid request: "+err.Error()) + slog.DebugContext(c.Request.Context(), "refresh bind failed", "error", err) + api.Error(c, http.StatusBadRequest, "invalid request body") return } pair, err := h.authService.Refresh(c.Request.Context(), req.RefreshToken) if err != nil { - api.Error(c, http.StatusUnauthorized, err.Error()) + api.RespondError(c, err) return } @@ -89,12 +93,13 @@ func (h *AuthHandler) Refresh(c *gin.Context) { func (h *AuthHandler) Logout(c *gin.Context) { var req tokenRequest if err := c.ShouldBindJSON(&req); err != nil { - api.Error(c, http.StatusBadRequest, "invalid request: "+err.Error()) + slog.DebugContext(c.Request.Context(), "logout bind failed", "error", err) + api.Error(c, http.StatusBadRequest, "invalid request body") return } if err := h.authService.Logout(c.Request.Context(), req.RefreshToken); err != nil { - api.Error(c, http.StatusInternalServerError, err.Error()) + api.RespondError(c, err) return } diff --git a/internal/handler/file.go b/internal/handler/file.go index 56ac17a..99eb1b0 100644 --- a/internal/handler/file.go +++ b/internal/handler/file.go @@ -1,9 +1,9 @@ package handler import ( - "errors" "fmt" "io" + "log/slog" "mime" "net/http" "net/url" @@ -49,13 +49,14 @@ func (h *FileHandler) Upload(c *gin.Context) { if mediaType == "application/json" { var req createDirRequest if err := c.ShouldBindJSON(&req); err != nil { - api.Error(c, http.StatusBadRequest, "invalid request: "+err.Error()) + slog.DebugContext(c.Request.Context(), "create dir bind failed", "error", err) + api.Error(c, http.StatusBadRequest, "invalid request body") return } dir, err := h.fileService.CreateDir(c.Request.Context(), userID, req.ParentID, req.Name) if err != nil { - api.Error(c, mapError(err), err.Error()) + api.RespondError(c, err) return } @@ -65,7 +66,8 @@ func (h *FileHandler) Upload(c *gin.Context) { // File upload via multipart. if err := c.Request.ParseMultipartForm(32 << 20); err != nil { - api.Error(c, http.StatusBadRequest, "invalid multipart form: "+err.Error()) + slog.DebugContext(c.Request.Context(), "parse multipart form failed", "error", err) + api.Error(c, http.StatusBadRequest, "invalid multipart form") return } @@ -77,20 +79,21 @@ func (h *FileHandler) Upload(c *gin.Context) { header, err := c.FormFile("file") if err != nil { - api.Error(c, http.StatusBadRequest, "missing file field: "+err.Error()) + slog.DebugContext(c.Request.Context(), "form file missing", "error", err) + api.Error(c, http.StatusBadRequest, "missing file field") return } file, err := header.Open() if err != nil { - api.Error(c, http.StatusInternalServerError, "open uploaded file: "+err.Error()) + api.RespondError(c, model.NewInternalError("open uploaded file", err)) return } defer file.Close() info, err := h.fileService.Upload(c.Request.Context(), userID, parentID, header.Filename, file) if err != nil { - api.Error(c, mapError(err), err.Error()) + api.RespondError(c, err) return } @@ -124,7 +127,7 @@ func (h *FileHandler) List(c *gin.Context) { list, err := h.fileService.List(c.Request.Context(), userID, parentID, offset, limit) if err != nil { - api.Error(c, mapError(err), err.Error()) + api.RespondError(c, err) return } @@ -138,7 +141,7 @@ func (h *FileHandler) Get(c *gin.Context) { info, err := h.fileService.Get(c.Request.Context(), userID, fileID) if err != nil { - api.Error(c, mapError(err), err.Error()) + api.RespondError(c, err) return } @@ -152,7 +155,7 @@ func (h *FileHandler) Download(c *gin.Context) { reader, info, err := h.fileService.Download(c.Request.Context(), userID, fileID) if err != nil { - api.Error(c, mapError(err), err.Error()) + api.RespondError(c, err) return } defer reader.Close() @@ -177,7 +180,8 @@ func (h *FileHandler) Update(c *gin.Context) { var req updateFileRequest if err := c.ShouldBindJSON(&req); err != nil { - api.Error(c, http.StatusBadRequest, "invalid request: "+err.Error()) + slog.DebugContext(c.Request.Context(), "update file bind failed", "error", err) + api.Error(c, http.StatusBadRequest, "invalid request body") return } @@ -189,7 +193,7 @@ func (h *FileHandler) Update(c *gin.Context) { info, err := h.fileService.Update(c.Request.Context(), userID, fileID, req.Name, req.ParentID) if err != nil { - api.Error(c, mapError(err), err.Error()) + api.RespondError(c, err) return } @@ -202,23 +206,9 @@ func (h *FileHandler) Delete(c *gin.Context) { fileID := c.Param("id") if err := h.fileService.Delete(c.Request.Context(), userID, fileID); err != nil { - api.Error(c, mapError(err), err.Error()) + api.RespondError(c, err) return } c.Status(http.StatusNoContent) } - -// mapError maps sentinel errors to HTTP status codes. -func mapError(err error) int { - switch { - case errors.Is(err, model.ErrNotFound): - return http.StatusNotFound - case errors.Is(err, model.ErrForbidden): - return http.StatusForbidden - case errors.Is(err, model.ErrDuplicate): - return http.StatusConflict - default: - return http.StatusInternalServerError - } -} diff --git a/internal/handler/file_test.go b/internal/handler/file_test.go index 68f2062..3f36118 100644 --- a/internal/handler/file_test.go +++ b/internal/handler/file_test.go @@ -70,7 +70,7 @@ func setupFileHandler(t *testing.T) *FileHandler { fileRepo := repository.NewFileRepository(db) store := newInMemStore() - fileService := service.NewFileService(fileRepo, store, 0) + fileService := service.NewFileService(fileRepo, store, 0, nil) return NewFileHandler(fileService) } diff --git a/internal/model/errors.go b/internal/model/errors.go index 9f77f81..a4d8719 100644 --- a/internal/model/errors.go +++ b/internal/model/errors.go @@ -1,6 +1,10 @@ package model -import "errors" +import ( + "errors" + "fmt" + "net/http" +) var ( ErrNotFound = errors.New("resource not found") @@ -8,3 +12,25 @@ var ( ErrUnauthorized = errors.New("unauthorized") ErrForbidden = errors.New("forbidden") ) + +// AppError is a service-layer error that carries an HTTP status, a user-safe +// message, and an optional internal cause for logging. Handlers unpack it +// transparently via api.RespondError. +type AppError struct { + Status int // HTTP status code + Message string // safe for API response + Err error // internal cause (nil = user-caused, skip logging) +} + +func (e *AppError) Error() string { return e.Message } +func (e *AppError) Unwrap() error { return e.Err } + +// NewInternalError creates a 500 AppError with a wrapped internal cause. +// msg describes the operation that failed; cause is the underlying error. +func NewInternalError(msg string, cause error) *AppError { + return &AppError{ + Status: http.StatusInternalServerError, + Message: "internal server error", + Err: fmt.Errorf("%s: %w", msg, cause), + } +} diff --git a/internal/server/router.go b/internal/server/router.go index 97f5bb3..cba43c4 100644 --- a/internal/server/router.go +++ b/internal/server/router.go @@ -4,11 +4,18 @@ import ( "github.com/gin-gonic/gin" "github.com/dhao2001/mygo/internal/app" + "github.com/dhao2001/mygo/internal/middleware" ) // NewRouter builds the Gin router and registers API routes. func NewRouter(webApp *app.WebApp) *gin.Engine { - router := gin.Default() + router := gin.New() + + // Request ID must be first — every subsequent middleware and handler + // gets access to req_id in the context. + router.Use(middleware.RequestID()) + router.Use(gin.Logger()) + router.Use(gin.Recovery()) v1 := router.Group("/api/v1") diff --git a/internal/service/auth.go b/internal/service/auth.go index 444eda6..4b24563 100644 --- a/internal/service/auth.go +++ b/internal/service/auth.go @@ -3,7 +3,7 @@ package service import ( "context" "errors" - "fmt" + "net/http" "strings" "time" @@ -59,12 +59,12 @@ func NewAuthService( // Register creates a new user account. func (s *AuthService) Register(ctx context.Context, username, email, password string) (*model.User, error) { if username == "" || email == "" || password == "" { - return nil, fmt.Errorf("username, email, and password are required") + return nil, &model.AppError{Status: http.StatusBadRequest, Message: "username, email, and password are required"} } passwordHash, err := auth.HashPassword(password) if err != nil { - return nil, fmt.Errorf("hash password: %w", err) + return nil, model.NewInternalError("hash password", err) } user := &model.User{ @@ -76,9 +76,9 @@ func (s *AuthService) Register(ctx context.Context, username, email, password st if err := s.userRepo.Create(ctx, user); err != nil { if errors.Is(err, model.ErrDuplicate) { - return nil, fmt.Errorf("username or email already exists") + return nil, &model.AppError{Status: http.StatusConflict, Message: "username or email already exists"} } - return nil, fmt.Errorf("create user: %w", err) + return nil, model.NewInternalError("create user", err) } return user, nil @@ -89,13 +89,13 @@ func (s *AuthService) Login(ctx context.Context, email, password string) (*Token user, err := s.userRepo.FindByEmail(ctx, email) if err != nil { if errors.Is(err, model.ErrNotFound) { - return nil, fmt.Errorf("invalid email or password") + return nil, &model.AppError{Status: http.StatusUnauthorized, Message: "invalid email or password"} } - return nil, fmt.Errorf("find user: %w", err) + return nil, model.NewInternalError("find user", err) } if err := auth.VerifyPassword(user.PasswordHash, password); err != nil { - return nil, fmt.Errorf("invalid email or password") + return nil, &model.AppError{Status: http.StatusUnauthorized, Message: "invalid email or password"} } return s.issueTokens(ctx, user.ID) @@ -106,28 +106,28 @@ func (s *AuthService) Login(ctx context.Context, email, password string) (*Token func (s *AuthService) Refresh(ctx context.Context, refreshTokenStr string) (*TokenPair, error) { claims, err := auth.ParseToken(refreshTokenStr, s.jwtSecret) if err != nil { - return nil, fmt.Errorf("invalid token") + return nil, &model.AppError{Status: http.StatusUnauthorized, Message: "invalid token"} } if claims.Type != auth.TokenRefresh { - return nil, fmt.Errorf("invalid token") + return nil, &model.AppError{Status: http.StatusUnauthorized, Message: "invalid token"} } tokenHash := auth.HashToken(refreshTokenStr) session, err := s.sessionRepo.FindByTokenHash(ctx, tokenHash) if err != nil { if errors.Is(err, model.ErrNotFound) { - return nil, fmt.Errorf("invalid token") + return nil, &model.AppError{Status: http.StatusUnauthorized, Message: "invalid token"} } - return nil, fmt.Errorf("find session: %w", err) + return nil, model.NewInternalError("find session", err) } if session.UserID != claims.UserID { - return nil, fmt.Errorf("invalid token") + return nil, &model.AppError{Status: http.StatusUnauthorized, Message: "invalid token"} } if err := s.sessionRepo.Delete(ctx, session.ID); err != nil { - return nil, fmt.Errorf("delete old session: %w", err) + return nil, model.NewInternalError("delete old session", err) } return s.issueTokens(ctx, claims.UserID) @@ -141,7 +141,7 @@ func (s *AuthService) Logout(ctx context.Context, refreshTokenStr string) error if errors.Is(err, model.ErrNotFound) { return nil } - return fmt.Errorf("find session: %w", err) + return model.NewInternalError("find session", err) } return s.sessionRepo.Delete(ctx, session.ID) @@ -151,7 +151,7 @@ func (s *AuthService) Logout(ctx context.Context, refreshTokenStr string) error func (s *AuthService) CreatePasskey(ctx context.Context, userID, label string) (*CreatedPasskey, error) { raw, hash, err := auth.GenerateToken() if err != nil { - return nil, fmt.Errorf("generate token: %w", err) + return nil, model.NewInternalError("generate token", err) } cred := &model.Credential{ @@ -163,7 +163,7 @@ func (s *AuthService) CreatePasskey(ctx context.Context, userID, label string) ( } if err := s.credentialRepo.Create(ctx, cred); err != nil { - return nil, fmt.Errorf("create credential: %w", err) + return nil, model.NewInternalError("create credential", err) } return &CreatedPasskey{ @@ -176,24 +176,24 @@ func (s *AuthService) CreatePasskey(ctx context.Context, userID, label string) ( // LoginWithPasskey authenticates a user using an app passkey token. func (s *AuthService) LoginWithPasskey(ctx context.Context, tokenStr string) (*TokenPair, error) { if !strings.HasPrefix(tokenStr, "mygo_") { - return nil, fmt.Errorf("invalid passkey format") + return nil, &model.AppError{Status: http.StatusUnauthorized, Message: "invalid passkey format"} } tokenHash := auth.HashToken(tokenStr) cred, err := s.credentialRepo.FindByHash(ctx, tokenHash) if err != nil { if errors.Is(err, model.ErrNotFound) { - return nil, fmt.Errorf("invalid passkey") + return nil, &model.AppError{Status: http.StatusUnauthorized, Message: "invalid passkey"} } - return nil, fmt.Errorf("find credential: %w", err) + return nil, model.NewInternalError("find credential", err) } if cred.Type != "app_passkey" { - return nil, fmt.Errorf("invalid credential type") + return nil, &model.AppError{Status: http.StatusUnauthorized, Message: "invalid credential type"} } if err := s.credentialRepo.UpdateLastUsed(ctx, cred.ID); err != nil { - return nil, fmt.Errorf("update last used: %w", err) + return nil, model.NewInternalError("update last used", err) } return s.issueTokens(ctx, cred.UserID) @@ -208,11 +208,14 @@ func (s *AuthService) ListPasskeys(ctx context.Context, userID string) ([]model. func (s *AuthService) RevokePasskey(ctx context.Context, userID, credID string) error { cred, err := s.credentialRepo.FindByID(ctx, credID) if err != nil { - return fmt.Errorf("find credential: %w", err) + if errors.Is(err, model.ErrNotFound) { + return &model.AppError{Status: http.StatusNotFound, Message: "passkey not found", Err: model.ErrNotFound} + } + return model.NewInternalError("find credential", err) } if cred.UserID != userID { - return model.ErrForbidden + return &model.AppError{Status: http.StatusForbidden, Message: "access denied", Err: model.ErrForbidden} } return s.credentialRepo.Delete(ctx, credID) @@ -221,12 +224,12 @@ func (s *AuthService) RevokePasskey(ctx context.Context, userID, credID string) func (s *AuthService) issueTokens(ctx context.Context, userID string) (*TokenPair, error) { accessToken, err := auth.GenerateAccessToken(userID, s.jwtSecret, s.accessTTL) if err != nil { - return nil, fmt.Errorf("generate access token: %w", err) + return nil, model.NewInternalError("generate access token", err) } refreshToken, err := auth.GenerateRefreshToken(userID, s.jwtSecret, s.refreshTTL) if err != nil { - return nil, fmt.Errorf("generate refresh token: %w", err) + return nil, model.NewInternalError("generate refresh token", err) } session := &model.Session{ @@ -237,7 +240,7 @@ func (s *AuthService) issueTokens(ctx context.Context, userID string) (*TokenPai } if err := s.sessionRepo.Create(ctx, session); err != nil { - return nil, fmt.Errorf("create session: %w", err) + return nil, model.NewInternalError("create session", err) } return &TokenPair{ diff --git a/internal/service/auth_test.go b/internal/service/auth_test.go index 4cbcde9..3901c7f 100644 --- a/internal/service/auth_test.go +++ b/internal/service/auth_test.go @@ -2,6 +2,8 @@ package service import ( "context" + "errors" + "net/http" "testing" "time" @@ -336,8 +338,9 @@ func TestAuthService_RevokePasskeyNotOwner(t *testing.T) { claimsBob, _ := auth.ParseToken(pairBob.AccessToken, []byte("test-secret")) err = svc.RevokePasskey(ctx, claimsBob.UserID, pk.ID) - if err != model.ErrForbidden { - t.Fatalf("expected ErrForbidden, got %v", err) + var ae *model.AppError + if !errors.As(err, &ae) || ae.Status != http.StatusForbidden { + t.Fatalf("expected AppError 403 Forbidden, got %v", err) } } diff --git a/internal/service/file.go b/internal/service/file.go index 99f2f87..30b8198 100644 --- a/internal/service/file.go +++ b/internal/service/file.go @@ -8,6 +8,8 @@ import ( "errors" "fmt" "io" + "log/slog" + "net/http" "strings" "time" @@ -44,6 +46,7 @@ type FileService struct { fileRepo repository.FileRepository storage storage.StorageBackend maxUploadSize int64 + logger *slog.Logger } // NewFileService creates a FileService. @@ -51,11 +54,16 @@ func NewFileService( fileRepo repository.FileRepository, storage storage.StorageBackend, maxUploadSize int64, + logger *slog.Logger, ) *FileService { + if logger == nil { + logger = slog.Default() + } return &FileService{ fileRepo: fileRepo, storage: storage, maxUploadSize: maxUploadSize, + logger: logger, } } @@ -74,9 +82,9 @@ func (s *FileService) Upload(ctx context.Context, userID string, parentID *strin // Check for name conflicts in the target directory. if _, err := s.fileRepo.FindByNameAndParent(ctx, userID, parentID, fileName); err == nil { - return nil, fmt.Errorf("a file with this name already exists in this directory") + return nil, &model.AppError{Status: http.StatusConflict, Message: "a file with this name already exists in this directory"} } else if !errors.Is(err, model.ErrNotFound) { - return nil, fmt.Errorf("check name conflict: %w", err) + return nil, model.NewInternalError("check name conflict", err) } // Limit the reader if a max upload size is configured. @@ -88,7 +96,7 @@ func (s *FileService) Upload(ctx context.Context, userID string, parentID *strin head := make([]byte, 512) n, readErr := io.ReadFull(reader, head) if readErr != nil && readErr != io.ErrUnexpectedEOF && readErr != io.EOF { - return nil, fmt.Errorf("read for mime detection: %w", readErr) + return nil, model.NewInternalError("read for mime detection", readErr) } head = head[:n] mimeType := mimetype.Detect(head).String() @@ -103,13 +111,13 @@ func (s *FileService) Upload(ctx context.Context, userID string, parentID *strin written, err := s.storage.Save(ctx, storagePath, teeReader) if err != nil { - return nil, fmt.Errorf("save file: %w", err) + return nil, model.NewInternalError("save file", err) } if s.maxUploadSize > 0 && written > s.maxUploadSize { // Clean up the oversized file. _ = s.storage.Delete(ctx, storagePath) - return nil, fmt.Errorf("file size exceeds the maximum allowed size of %d bytes", s.maxUploadSize) + return nil, &model.AppError{Status: http.StatusBadRequest, Message: fmt.Sprintf("file size exceeds the maximum allowed size of %d bytes", s.maxUploadSize)} } file := &model.File{ @@ -128,9 +136,9 @@ func (s *FileService) Upload(ctx context.Context, userID string, parentID *strin // Compensate: remove the stored file. _ = s.storage.Delete(ctx, storagePath) if errors.Is(err, model.ErrDuplicate) { - return nil, fmt.Errorf("a file with this name already exists in this directory") + return nil, &model.AppError{Status: http.StatusConflict, Message: "a file with this name already exists in this directory"} } - return nil, fmt.Errorf("create file record: %w", err) + return nil, model.NewInternalError("create file record", err) } return modelToFileInfo(file), nil @@ -145,12 +153,12 @@ func (s *FileService) Download(ctx context.Context, userID, fileID string) (io.R } if file.IsDir { - return nil, nil, fmt.Errorf("cannot download a directory") + return nil, nil, &model.AppError{Status: http.StatusBadRequest, Message: "cannot download a directory"} } reader, err := s.storage.Open(ctx, file.StoragePath) if err != nil { - return nil, nil, fmt.Errorf("open file: %w", err) + return nil, nil, model.NewInternalError("open file", err) } return reader, modelToFileInfo(file), nil @@ -175,7 +183,7 @@ func (s *FileService) List(ctx context.Context, userID string, parentID *string, files, total, err := s.fileRepo.FindByParentIDPaginated(ctx, userID, parentID, offset, limit) if err != nil { - return nil, fmt.Errorf("list files: %w", err) + return nil, model.NewInternalError("list files", err) } infos := make([]FileInfo, 0, len(files)) @@ -204,7 +212,7 @@ func (s *FileService) Update(ctx context.Context, userID, fileID string, newName // If parent is changing, verify the new parent. if newParentID != nil { if *newParentID == fileID { - return nil, fmt.Errorf("cannot move a directory into itself") + return nil, &model.AppError{Status: http.StatusBadRequest, Message: "cannot move a directory into itself"} } if err := s.verifyParent(ctx, userID, *newParentID); err != nil { return nil, err @@ -216,17 +224,17 @@ func (s *FileService) Update(ctx context.Context, userID, fileID string, newName if newName != "" || newParentID != nil { conflict, err := s.fileRepo.FindByNameAndParent(ctx, userID, file.ParentID, file.Name) if err == nil && conflict.ID != fileID { - return nil, fmt.Errorf("a file with this name already exists in the target directory") + return nil, &model.AppError{Status: http.StatusConflict, Message: "a file with this name already exists in the target directory"} } else if err != nil && !errors.Is(err, model.ErrNotFound) { - return nil, fmt.Errorf("check name conflict: %w", err) + return nil, model.NewInternalError("check name conflict", err) } } if err := s.fileRepo.Update(ctx, file); err != nil { if errors.Is(err, model.ErrDuplicate) { - return nil, fmt.Errorf("a file with this name already exists in the target directory") + return nil, &model.AppError{Status: http.StatusConflict, Message: "a file with this name already exists in the target directory"} } - return nil, fmt.Errorf("update file: %w", err) + return nil, model.NewInternalError("update file", err) } return modelToFileInfo(file), nil @@ -243,23 +251,22 @@ func (s *FileService) Delete(ctx context.Context, userID, fileID string) error { if file.IsDir { _, total, err := s.fileRepo.FindByParentIDPaginated(ctx, userID, &fileID, 0, 1) if err != nil { - return fmt.Errorf("check directory contents: %w", err) + return model.NewInternalError("check directory contents", err) } if total > 0 { - return fmt.Errorf("directory is not empty") + return &model.AppError{Status: http.StatusConflict, Message: "directory is not empty"} } } if err := s.fileRepo.Delete(ctx, fileID); err != nil { - return fmt.Errorf("delete file record: %w", err) + return model.NewInternalError("delete file record", err) } // Remove content from storage (directories have no stored content). if !file.IsDir { if err := s.storage.Delete(ctx, file.StoragePath); err != nil { - // Log but don't fail — the DB record is already gone. - // A periodic cleanup job can handle orphaned files later. - _ = err + s.logger.WarnContext(ctx, "failed to delete file from storage", + "path", file.StoragePath, "error", err) } } @@ -280,24 +287,24 @@ func (s *FileService) CreateDir(ctx context.Context, userID string, parentID *st // Check for name conflicts in the target directory. if _, err := s.fileRepo.FindByNameAndParent(ctx, userID, parentID, dirName); err == nil { - return nil, fmt.Errorf("a file with this name already exists in this directory") + return nil, &model.AppError{Status: http.StatusConflict, Message: "a file with this name already exists in this directory"} } else if !errors.Is(err, model.ErrNotFound) { - return nil, fmt.Errorf("check name conflict: %w", err) + return nil, model.NewInternalError("check name conflict", err) } dir := &model.File{ - ID: uuid.NewString(), - UserID: userID, + ID: uuid.NewString(), + UserID: userID, ParentID: parentID, - Name: dirName, - IsDir: true, + Name: dirName, + IsDir: true, } if err := s.fileRepo.Create(ctx, dir); err != nil { if errors.Is(err, model.ErrDuplicate) { - return nil, fmt.Errorf("a file with this name already exists in this directory") + return nil, &model.AppError{Status: http.StatusConflict, Message: "a file with this name already exists in this directory"} } - return nil, fmt.Errorf("create directory record: %w", err) + return nil, model.NewInternalError("create directory record", err) } return modelToFileInfo(dir), nil @@ -308,13 +315,13 @@ func (s *FileService) getOwnedFile(ctx context.Context, userID, fileID string) ( file, err := s.fileRepo.FindByID(ctx, fileID) if err != nil { if errors.Is(err, model.ErrNotFound) { - return nil, model.ErrNotFound + return nil, &model.AppError{Status: http.StatusNotFound, Message: "file not found", Err: model.ErrNotFound} } - return nil, fmt.Errorf("find file: %w", err) + return nil, model.NewInternalError("find file", err) } if file.UserID != userID { - return nil, model.ErrForbidden + return nil, &model.AppError{Status: http.StatusForbidden, Message: "access denied", Err: model.ErrForbidden} } return file, nil @@ -325,17 +332,17 @@ func (s *FileService) verifyParent(ctx context.Context, userID, parentID string) parent, err := s.fileRepo.FindByID(ctx, parentID) if err != nil { if errors.Is(err, model.ErrNotFound) { - return fmt.Errorf("parent directory not found") + return &model.AppError{Status: http.StatusNotFound, Message: "parent directory not found"} } - return fmt.Errorf("find parent: %w", err) + return model.NewInternalError("find parent", err) } if parent.UserID != userID { - return model.ErrForbidden + return &model.AppError{Status: http.StatusForbidden, Message: "access denied", Err: model.ErrForbidden} } if !parent.IsDir { - return fmt.Errorf("parent is not a directory") + return &model.AppError{Status: http.StatusBadRequest, Message: "parent is not a directory"} } return nil @@ -345,13 +352,13 @@ func (s *FileService) verifyParent(ctx context.Context, userID, parentID string) // null bytes, or are reserved names "." and "..". func validateFileName(name string) error { if name == "" { - return fmt.Errorf("file name must not be empty") + return &model.AppError{Status: http.StatusBadRequest, Message: "file name must not be empty"} } if strings.ContainsAny(name, "/\\\x00") { - return fmt.Errorf("file name contains invalid characters") + return &model.AppError{Status: http.StatusBadRequest, Message: "file name contains invalid characters"} } if name == "." || name == ".." { - return fmt.Errorf("file name is reserved: %s", name) + return &model.AppError{Status: http.StatusBadRequest, Message: fmt.Sprintf("file name is reserved: %s", name)} } return nil } diff --git a/internal/service/file_test.go b/internal/service/file_test.go index fcce35c..be1f147 100644 --- a/internal/service/file_test.go +++ b/internal/service/file_test.go @@ -3,7 +3,9 @@ package service import ( "bytes" "context" + "errors" "io" + "net/http" "strings" "sync" "testing" @@ -16,6 +18,21 @@ import ( "github.com/dhao2001/mygo/internal/storage" ) +// assertAppError checks that err is an *model.AppError with the expected status. +func assertAppError(t *testing.T, err error, wantStatus int) bool { + t.Helper() + var ae *model.AppError + if !errors.As(err, &ae) { + t.Errorf("expected *model.AppError, got %T: %v", err, err) + return false + } + if ae.Status != wantStatus { + t.Errorf("status = %d, want %d; message = %q", ae.Status, wantStatus, ae.Message) + return false + } + return true +} + // memStorage is an in-memory implementation of storage.StorageBackend for tests. type memStorage struct { mu sync.RWMutex @@ -70,7 +87,7 @@ func setupFileService(t *testing.T) *FileService { fileRepo := repository.NewFileRepository(db) store := newMemStorage() - return NewFileService(fileRepo, store, 0) // 0 = unlimited + return NewFileService(fileRepo, store, 0, nil) // 0 = unlimited, nil logger defaults to slog.Default() } func TestFileService_Upload(t *testing.T) { @@ -214,9 +231,7 @@ func TestFileService_DownloadForbidden(t *testing.T) { } _, _, err = svc.Download(ctx, "user2", info.ID) - if err != model.ErrForbidden { - t.Errorf("expected ErrForbidden, got %v", err) - } + assertAppError(t, err, http.StatusForbidden) } func TestFileService_DownloadDirectory(t *testing.T) { @@ -257,9 +272,7 @@ func TestFileService_GetNotFound(t *testing.T) { ctx := context.Background() _, err := svc.Get(ctx, "user1", "nonexistent") - if err != model.ErrNotFound { - t.Errorf("expected ErrNotFound, got %v", err) - } + assertAppError(t, err, http.StatusNotFound) } func TestFileService_GetForbidden(t *testing.T) { @@ -272,9 +285,7 @@ func TestFileService_GetForbidden(t *testing.T) { } _, err = svc.Get(ctx, "user2", info.ID) - if err != model.ErrForbidden { - t.Errorf("expected ErrForbidden, got %v", err) - } + assertAppError(t, err, http.StatusForbidden) } func TestFileService_List(t *testing.T) { @@ -372,9 +383,7 @@ func TestFileService_Delete(t *testing.T) { } _, err = svc.Get(ctx, "user1", info.ID) - if err != model.ErrNotFound { - t.Errorf("expected ErrNotFound after delete, got %v", err) - } + assertAppError(t, err, http.StatusNotFound) } func TestFileService_DeleteNonEmptyDirectory(t *testing.T) { @@ -406,9 +415,7 @@ func TestFileService_DeleteForbidden(t *testing.T) { } err = svc.Delete(ctx, "user2", info.ID) - if err != model.ErrForbidden { - t.Errorf("expected ErrForbidden, got %v", err) - } + assertAppError(t, err, http.StatusForbidden) } func TestFileService_CreateDir(t *testing.T) { @@ -452,7 +459,5 @@ func TestFileService_VerifyParentForbidden(t *testing.T) { } _, err = svc.Upload(ctx, "user2", &dir.ID, "stolen.txt", strings.NewReader("data")) - if err != model.ErrForbidden { - t.Errorf("expected ErrForbidden, got %v", err) - } + assertAppError(t, err, http.StatusForbidden) }