Skip to content

fix: usecase 層の error 握り潰しを修正 (#358)#362

Merged
taminororo merged 2 commits into
developfrom
fix/358-errcheck-write-ops
Jun 21, 2026
Merged

fix: usecase 層の error 握り潰しを修正 (#358)#362
taminororo merged 2 commits into
developfrom
fix/358-errcheck-write-ops

Conversation

@taminororo

@taminororo taminororo commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

errcheck で検出された usecase 層の write 系メソッド(Update/Create/Destroy)の戻り値 error 握り潰しを解消(#358)。あわせて同5ファイル内の関連する error ハンドリングの綻び(bcrypt 失敗・strconv パースエラー喪失・Create/Update 後の err 上書き)も整理した。

Close #358

変更内容

ファイル 主な修正
shift_usecase.go UpdateShiftAdmin/UpdateShiftsFromGASUpdate を error 処理。action_logCreate は best-effort のため log-and-continue(メイン処理は中断しない)。dateID/taskID のパースエラーを伝播
mail_auth_usecase.go WebSignUp/WebSignInbcryptCreate_makeRandomStrsessionRep.Create の err チェック追加(_makeRandomStr の err 上書きバグを解消)
place/task/user_usecase.go Create/UpdateFindNewRecord の戻り値 err チェック追加

新規メッセージは既存の日本語規約 errors.Wrapf(err, "〜失敗: %v") に統一。既存の英語メッセージ(cannot connect SQL 等)は不変。

errcheck の変化

修正前: 22 件
修正後: 12 件(#358 の write 系 10 件すべて解消)

残 12 件はすべてスコープ外(別issue):

スコープ外(別途対応)

Test plan

Summary by CodeRabbit

リリースノート

  • Bug Fixes
    • ユーザー認証処理におけるエラーハンドリングを強化し、パスワードハッシュ生成失敗時の処理をより確実に実行するよう改善しました。
    • データ作成・更新処理全般において、エラー発生時に確実に処理を停止し、不正な状態でのデータ操作を防止するようにしました。
    • 各リポジトリ呼び出し時のエラー検出精度を向上させ、システム全体の信頼性を改善しました。

errcheck で検出された write 系メソッド(Update/Create/Destroy)の戻り値 error の握り潰しを解消。あわせて同ファイル群の error ハンドリングを整理した。

- shift_usecase: UpdateShiftAdmin/UpdateShiftsFromGAS の Update を error 処理。action_log の Create は best-effort のため log-and-continue(メイン処理は中断しない)。UpdateShiftsFromGAS の dateID/taskID パースエラーを伝播。
- mail_auth/place/task/user_usecase: Create/Update/Delete・bcrypt・FindNewRecord の戻り値 error チェックを追加。WebSignIn の _makeRandomStr/sessionRep.Create の err 上書きを解消。新規メッセージは日本語規約に統一。

errcheck: 22 → 12(残12件は defer Close=#359, e.Start/c.JSON=#316 でスコープ外)。

Close #358
@coderabbitai

coderabbitai Bot commented Jun 21, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@taminororo, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 17 minutes and 51 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 63a14ab9-c45a-4e58-a02e-0ab45da20390

📥 Commits

Reviewing files that changed from the base of the PR and between a6a797d and 2aa6862.

📒 Files selected for processing (4)
  • api/lib/usecase/mail_auth_usecase.go
  • api/lib/usecase/shift_usecase.go
  • api/lib/usecase/task_usecase.go
  • api/lib/usecase/user_usecase.go
📝 Walkthrough

ウォークトレース

mail_authplacetaskusershift の全5usecaseファイルで、errcheck が指摘していたCreate/Update/Delete系呼び出しのエラー無視を修正した。失敗時は早期returnまたはlog.Printfによるログ記録に統一されている。

変更内容

Usecase errcheck対応:エラー伝播の統一

Layer / File(s) Summary
Auth・Place・User・Task の基本CRUD エラー伝播
api/lib/usecase/mail_auth_usecase.go, api/lib/usecase/place_usecase.go, api/lib/usecase/user_usecase.go, api/lib/usecase/task_usecase.go
WebSignUp/WebSignInのbcrypt・セッション操作、CreatePlace/UpdatePlaceGetUserByID/CreateUser/UpdateUser/GetCurrentUserGetTaskByID/CreateTask/UpdateTask にてエラー無視を早期returnに変更。
User・Task の GAS バッチ更新エラー伝播
api/lib/usecase/user_usecase.go, api/lib/usecase/task_usecase.go
UpdateUsersFromGASUpdateTasksAndPlacesFromGAS で、Find/Update/bcryptハッシュ生成の各失敗を errors.Wrapf でラップして返すよう変更。
Shift の Admin 操作・GAS バッチ更新エラー伝播
api/lib/usecase/shift_usecase.go
UpdateShiftAdminrep.Update エラー検知、DeleteShiftAdminactionLogRepo.Create 失敗を log.Printf に変更。UpdateShiftsFromGASFindByUniqueAtoirep.Update のエラー伝播と actionLogRepo.Create 失敗の log.Printf 記録を追加。

推定レビュー工数

🎯 3 (Moderate) | ⏱️ ~20 minutes

関連PR

  • NUTFes/SeeFT#254: shift_usecase.goUpdateShiftsFromGAS における action_log_repo.Create の呼び出し実装を含んでおり、本PRで変更しているエラー処理の対象箇所と直接同じ関数・リポジトリ呼び出しが関連している。

推奨レビュアー

  • KazumaSun

🐇 エラーを握りつぶさないで
ちゃんと伝えようよ、失敗したこと
return err と書けば
バグが隠れて暴れることもない
うさぎも見逃さない、ちゃんとログに残すよ ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed PR タイトルは「usecase 層の error 握り潰しを修正」と明確に述べており、変更内容の主要な目的を正確に要約している。
Description check ✅ Passed PR説明は対応Issue番号、詳細な変更内容表、errcheck の改善状況、スコープ外項目の明記とテストプランを含む充実した内容である。
Linked Issues check ✅ Passed Issue #358 の全 10 件の write 系 errcheck 違反が解決されている。mail_auth_usecase.go での bcrypt・Create・sessionRep.Create のエラーチェック追加、shift/place/task/user_usecase.go での Update・Create・FindNewRecord のエラー処理追加が確認でき、要件を満たしている。
Out of Scope Changes check ✅ Passed 変更は Issue #358 のスコープ内に限定されており、#359/#316 など別 issue 関連の変更は含まれていない。PR説明でスコープ外項目を明確に区別している。

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/358-errcheck-write-ops

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@api/lib/usecase/mail_auth_usecase.go`:
- Around line 143-151: The DeleteByUserID call on sessionRep is currently
executing before password validation with bcrypt.CompareHashAndPassword,
creating a security vulnerability where incorrect password attempts can force
logout for users. Move the entire session deletion block (the if statement
checking err from u.sessionRep.DeleteByUserID) to execute only after the
CompareHashAndPassword check succeeds, ensuring sessions are only deleted after
successful authentication.

In `@api/lib/usecase/shift_usecase.go`:
- Line 1497: The Scan operation on existRow at line 1497 incorrectly treats all
errors as "record not found". Currently, the condition only checks if err is
nil, but DB errors other than sql.ErrNoRows will still fall through and cause
new record creation at line 1536, potentially creating duplicates. Refactor the
error handling to explicitly check if the error is sql.ErrNoRows before allowing
fallback to new creation, and properly return or handle other database errors
instead of proceeding to create a new shift record.

In `@api/lib/usecase/user_usecase.go`:
- Line 379: Replace the string comparison `err.Error() == "sql: no rows in
result set"` in the conditional check at line 379 with the standard Go error
comparison pattern using `errors.Is(err, sql.ErrNoRows)`. This approach is more
robust and follows Go conventions for error handling. Ensure the sql package is
imported if it is not already present in the file.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 282ade98-4b17-43eb-9ac5-c2139c8820d8

📥 Commits

Reviewing files that changed from the base of the PR and between 4e61fd4 and a6a797d.

📒 Files selected for processing (5)
  • api/lib/usecase/mail_auth_usecase.go
  • api/lib/usecase/place_usecase.go
  • api/lib/usecase/shift_usecase.go
  • api/lib/usecase/task_usecase.go
  • api/lib/usecase/user_usecase.go

Comment thread api/lib/usecase/mail_auth_usecase.go Outdated
Comment thread api/lib/usecase/shift_usecase.go Outdated
Comment thread api/lib/usecase/user_usecase.go Outdated
- WebSignIn: 既存セッション削除をパスワード検証の後へ移動。認証前に削除すると、学籍番号を知るだけで誤パスワード試行による強制ログアウトが可能だった。
- UpdateShiftsFromGAS: 既存シフト確認 Scan の非 sql.ErrNoRows エラーを新規作成へフォールバックさせず return するよう変更。DBエラー時の重複生成を防止。
- sql.ErrNoRows の判定を文字列比較から errors.Is(err, sql.ErrNoRows) に統一(user_usecase 1箇所・task_usecase 2箇所)。
@taminororo

Copy link
Copy Markdown
Collaborator Author

CodeRabbit 指摘3件、いずれも妥当だったため対応しました(commit 2aa6862)。

1. mail_auth_usecase.go WebSignIn — パスワード検証前のセッション削除
   → 既存セッション削除を CompareHashAndPassword 成功後へ移動。
     認証前削除だと学籍番号だけで誤パス試行→強制ログアウトが可能だった。実害ありのため修正。

2. shift_usecase.go UpdateShiftsFromGAS — Scan の非 ErrNoRows を新規作成へフォールバック
   → scanErr を取り出し、!errors.Is(scanErr, sql.ErrNoRows) のDBエラーは return。
     未存在(ErrNoRows)と zero-id のときのみ新規作成へ。DBエラー時の重複生成を防止。

3. user_usecase.go:379 — 文字列比較を errors.Is に
   → errors.Is(err, sql.ErrNoRows) に変更。
     同じパターンが task_usecase.go の2箇所(UpdateTasksAndPlacesFromGAS)にもあったため、
     そちらも合わせて統一(database/sql import 追加)。

検証: cd api && go build ./... 通過、golangci-lint run の件数は変化なし(新規違反なし)。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Go static] errcheck: 書き込み系(Update/Create/Delete)の戻り値エラー処理 (10件)

1 participant