Skip to content

🩹 Fix error handling #1010

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
H1rono merged 1 commit into from
Feb 23, 2026
Merged

🩹 Fix error handling #1010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 13 additions & 19 deletions internal/router/file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (

"github.com/google/uuid"
"github.com/labstack/echo/v5"
"github.com/traPtitech/Jomon/internal/ent"
"github.com/traPtitech/Jomon/internal/logging"
"github.com/traPtitech/Jomon/internal/service"
"go.uber.org/zap"
Expand Down Expand Up @@ -118,12 +117,10 @@ func (h Handlers) GetFile(c *echo.Context) error {

file, err := h.Repository.GetFile(ctx, fileID)
if err != nil {
if ent.IsNotFound(err) {
logger.Info("could not find file in repository", zap.String("ID", fileID.String()))
return echo.ErrNotFound.Wrap(err)
}
logger.Error("failed to get file from repository", zap.Error(err))
return service.NewUnexpectedError(err)
logger.Info(
"file not found in repository",
zap.String("ID", fileID.String()), zap.Error(err))
return err
}
Comment on lines 118 to 124
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

ログメッセージとレベルがすべてのエラーに誤適用されている

GetFile が返すエラーは「ファイルが存在しない」ケースに限りません。DB 接続失敗やクエリエラーが発生した場合でも logger.Info("file not found in repository", ...) が記録されてしまい、実際のエラー内容を誤解させます。以前は ent.IsNotFound の場合のみこのメッセージが使われ、それ以外は別経路（logger.Error + NewUnexpectedError）で処理されていました。

DB 障害を Info レベルで "file not found" として記録すると、インフラ問題の検知が遅れます。

🔧 修正案 
 file, err := h.Repository.GetFile(ctx, fileID)
 if err != nil {
-    logger.Info(
-        "file not found in repository",
-        zap.String("ID", fileID.String()), zap.Error(err))
-    return err
+    logger.Error(
+        "failed to get file from repository",
+        zap.String("ID", fileID.String()), zap.Error(err))
+    return err
 }

また return err で生の ent エラーが返ることによる HTTP ステータスのマッピングについては、middleware.go 側のコメントで生成した確認スクリプトで合わせて検証してください。

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@internal/router/file.go` around lines 118 - 124, GetFile error handling
currently logs every error as info "file not found in repository" and returns
the raw error; change it to distinguish ent.IsNotFound(err) from other errors:
when ent.IsNotFound(err) keep the Info-level "file not found" log (including
fileID) and return the appropriate not-found error, but for all other errors log
with logger.Error (include fileID and the error) and return a wrapped unexpected
error (e.g. NewUnexpectedError) so DB/infra failures are surfaced at error
level; update handling around h.Repository.GetFile, logger.Info/logger.Error,
ent.IsNotFound and the return path instead of returning the raw err.


modifiedAt := file.CreatedAt.Truncate(time.Second)
Expand Down Expand Up @@ -174,12 +171,10 @@ func (h Handlers) GetFileMeta(c *echo.Context) error {

file, err := h.Repository.GetFile(ctx, fileID)
if err != nil {
if ent.IsNotFound(err) {
logger.Info("could not find file in repository", zap.String("ID", fileID.String()))
return echo.ErrNotFound.Wrap(err)
}
logger.Error("failed to get file from repository", zap.Error(err))
return service.NewUnexpectedError(err)
logger.Info(
"file not found in repository",
zap.String("ID", fileID.String()), zap.Error(err))
return err
}
Comment on lines 172 to 178
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

GetFile エラーパスと同じ：ログメッセージ・レベルが誤り

GetFileMeta でも GetFile のエラーパスと同様に、DB エラーを含むすべての失敗に対して logger.Info("file not found in repository", ...) が記録されます。

🔧 修正案 
 file, err := h.Repository.GetFile(ctx, fileID)
 if err != nil {
-    logger.Info(
-        "file not found in repository",
-        zap.String("ID", fileID.String()), zap.Error(err))
-    return err
+    logger.Error(
+        "failed to get file from repository",
+        zap.String("ID", fileID.String()), zap.Error(err))
+    return err
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
file, err := h.Repository.GetFile(ctx, fileID)
if err != nil {
if ent.IsNotFound(err) {
logger.Info("could not find file in repository", zap.String("ID", fileID.String()))
return echo.ErrNotFound.Wrap(err)
}
logger.Error("failed to get file from repository", zap.Error(err))
return service.NewUnexpectedError(err)
logger.Info(
"file not found in repository",
zap.String("ID", fileID.String()), zap.Error(err))
return err
}
file, err := h.Repository.GetFile(ctx, fileID)
if err != nil {
logger.Error(
"failed to get file from repository",
zap.String("ID", fileID.String()), zap.Error(err))
return err
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@internal/router/file.go` around lines 172 - 178, GetFile のエラーパスは GetFileMeta
と同様に DB エラーなど全ての失敗を単に logger.Info で「file not found in
repository」と記録しているため、ログレベルと文言を修正してください — h.Repository.GetFile 呼び出し後のエラーハンドリングで
logger.Info を logger.Error に変更し、メッセージを「failed to get file from
repository」や「failed to get file metadata」など失敗を示す文言にしつつ zap.String("ID",
fileID.String()) と zap.Error(err) を含める（GetFileMeta の該当箇所と同様の形式に合わせる）ことで、実際の DB
エラーを適切なレベルで記録してください.


return c.JSON(http.StatusOK, &FileMetaResponse{
Expand Down Expand Up @@ -208,17 +203,16 @@ func (h Handlers) DeleteFile(c *echo.Context) error {

err = h.Repository.DeleteFile(ctx, fileID)
if err != nil {
if ent.IsConstraintError(err) {
logger.Info("constraint error while deleting file", zap.Error(err))
return echo.ErrBadRequest.Wrap(err)
}
logger.Error("failed to delete file in repository", zap.Error(err))
logger.Error("failed to delete file in repository",
zap.String("ID", fileID.String()), zap.Error(err))
return service.NewUnexpectedError(err)
}

err = h.Storage.Delete(ctx, fileID.String())
if err != nil {
logger.Error("failed to delete file in storage", zap.Error(err))
logger.Error(
"failed to delete file in storage",
zap.String("ID", fileID.String()), zap.Error(err))
return service.NewUnexpectedError(err)
}

Expand Down
9 changes: 1 addition & 8 deletions internal/router/middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (

"github.com/google/uuid"
"github.com/labstack/echo/v5"
"github.com/traPtitech/Jomon/internal/ent"
"github.com/traPtitech/Jomon/internal/logging"
"github.com/traPtitech/Jomon/internal/router/wrapsession"
"go.uber.org/zap"
Expand Down Expand Up @@ -90,7 +89,6 @@ func (h Handlers) AccessLoggingMiddleware(next echo.HandlerFunc) echo.HandlerFun
func (h Handlers) CheckLoginMiddleware(next echo.HandlerFunc) echo.HandlerFunc {
return func(c *echo.Context) error {
ctx := c.Request().Context()
logger := logging.GetLogger(ctx)

id, err := wrapsession.WithSession(
c, h.SessionName, func(w *wrapsession.W) (uuid.UUID, error) {
Expand All @@ -106,12 +104,7 @@ func (h Handlers) CheckLoginMiddleware(next echo.HandlerFunc) echo.HandlerFunc {
}
user, err := h.Repository.GetUserByID(ctx, id)
if err != nil {
if ent.IsNotFound(err) {
logger.Info("user not found in repository", zap.Error(err))
return echo.NewHTTPError(http.StatusUnauthorized, "you are not logged in")
}
logger.Error("failed to get user from repository", zap.Error(err))
return echo.ErrInternalServerError.Wrap(err)
return err
}
c.Set(loginUserKey, userFromModelUser(*user))

Expand Down