fix: shift-cardsのGetUsersByShift呼び出し回数を削減しN+1を解消 - #438
Conversation
createShiftCardFromGroupが1カード内の15分スロットごと・前後スロット分でGetUsersByShiftを 繰り返し呼び出しており、呼び出し1回につきJOIN1本+year/date/time/weatherの単票Find4本の 計5クエリを発行していた。UsersByTimes(ANY($4)による複数time_idの一括取得)を追加し、 カード単位で1回のバッチ取得にまとめることで呼び出し回数を削減する。 year/date/time/weatherの単票FindはgetShiftMembersForTime等のいずれからも参照されていなかった ため、新しいバッチ取得メソッドでは意図的に取得しない。go-sqlmockによるクエリ回数アサーション テストを追加し、実DBでの修正前後のレスポンス一致も確認済み。
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughShiftカード生成時に本体・前後スロットのユーザーを一括取得し、time_id別にメンバーへ変換する処理へ変更しました。リポジトリのSQL、テスト用依存関係、sqlmockテストも追加しています。 ChangesShiftカードのメンバー一括取得
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GetShiftCardsByUserAndDateAndWeather
participant shiftRepository
participant SQLDatabase
Client->>GetShiftCardsByUserAndDateAndWeather: Request shift cards
GetShiftCardsByUserAndDateAndWeather->>shiftRepository: UsersByTimes for card time_ids
shiftRepository->>SQLDatabase: Execute JOIN with ANY(time_ids)
SQLDatabase-->>shiftRepository: Return user rows
shiftRepository-->>GetShiftCardsByUserAndDateAndWeather: Return users by time_id
GetShiftCardsByUserAndDateAndWeather-->>Client: Return shift cards and members
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/lib/usecase/shift_usecase.go (2)
168-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winコメントがGoDoc形式になっている。
getUsersByTimes（168-171行目）とtoShiftMembers（212行目）のコメントは、いずれも関数名を主語にした説明文（「〜は…」）で始まっており、GoDocの慣習的パターンに該当します。コーディングガイドラインでは「コメントは日本語で記述し、GoDoc形式は使用しない」と定められているため、関数名を主語にしない自由記述に変更することを推奨します。As per coding guidelines, "コメントは日本語で記述し、GoDoc 形式は使用しない。"
✏️ 修正例
-// getUsersByTimes は同一task/year/date/weatherで複数time_idにまたがるシフトメンバーを -// 1クエリでまとめて取得し、time_idごとに振り分ける（shift-cardsのN+1解消）。 -// year/date/time/weatherの個別Findは呼び出し元(getShiftMembersForTime等)で -// 使われていなかったため、意図的に取得しない。 +// 同一task/year/date/weatherで複数time_idにまたがるシフトメンバーを1クエリでまとめて取得し、 +// time_idごとに振り分ける（shift-cardsのN+1解消）。 +// year/date/time/weatherの個別Findは呼び出し元(getShiftMembersForTime等)で +// 使われていなかったため、意図的に取得しない。-// toShiftMembers はUser群をShiftMemberに変換する（grade/bureauはマップから引く） +// User群をShiftMemberに変換する（grade/bureauはマップから引く）Also applies to: 212-212
🤖 Prompt for 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. In `@api/lib/usecase/shift_usecase.go` around lines 168 - 172, 関数名を主語にしたGoDoc形式になっているgetUsersByTimesとtoShiftMembersのコメントを、関数名で始めない日本語の自由記述へ変更してください。コメントの内容自体は維持し、指定された2つの関数コメントのみを修正してください。Source: Coding guidelines
616-661: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
getNextTimeString(c, last.Time.ID)が同一カードで重複して呼ばれている。616行目（EndTime算出）と652行目（hasNext判定、今回新規追加）はいずれも
a.getNextTimeString(c, last.Time.ID)を呼び出しており、返す値は同一です。さらに、最終スロットのeTime再計算（682行目、既存）でもshift.Time.ID == last.Time.IDの場合に同じ呼び出しが発生し、カード1件あたり最大3回の重複クエリが生じます（テストファイルのexpectNoMoreTimes(mock, 3)のコメントでもtime_id=7への3回のFindとして明記されています）。本PRの目的がshift-cardsエンドポイントのDBラウンドトリップ削減であることを踏まえると、この結果を1回計算して再利用することで、カードごとに2回分のクエリをさらに削減できます。
♻️ 修正例（概要）
- endTime, err := a.getNextTimeString(c, last.Time.ID) + nextOfLast, nextOfLastErr := a.getNextTimeString(c, last.Time.ID) + endTime := nextOfLast + err := nextOfLastErr if err != nil { endTime = last.Time.Time // フォールバック } ... - if s, err := a.getNextTimeString(c, last.Time.ID); err == nil && s != "" { - nextStart = s + if nextOfLastErr == nil && nextOfLast != "" { + nextStart = nextOfLast（末尾スロットのeTime再計算（682行目付近）にも同様に
nextOfLastを再利用できます）🤖 Prompt for 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. In `@api/lib/usecase/shift_usecase.go` around lines 616 - 661, Cache the result of getNextTimeString(c, last.Time.ID) in the shift-card construction flow and reuse it for ShiftCard.EndTime and the hasNext calculation instead of calling it again. Also reuse the cached result in the final-slot eTime recalculation near the existing shift.Time.ID == last.Time.ID handling, preserving the current fallback and next-slot behavior.
🤖 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.
Nitpick comments:
In `@api/lib/usecase/shift_usecase.go`:
- Around line 168-172:
関数名を主語にしたGoDoc形式になっているgetUsersByTimesとtoShiftMembersのコメントを、関数名で始めない日本語の自由記述へ変更してください。コメントの内容自体は維持し、指定された2つの関数コメントのみを修正してください。
- Around line 616-661: Cache the result of getNextTimeString(c, last.Time.ID) in
the shift-card construction flow and reuse it for ShiftCard.EndTime and the
hasNext calculation instead of calling it again. Also reuse the cached result in
the final-slot eTime recalculation near the existing shift.Time.ID ==
last.Time.ID handling, preserving the current fallback and next-slot behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: df7ce829-dd45-4904-9b80-c34699828f2b
⛔ Files ignored due to path filters (1)
api/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
api/go.modapi/lib/internals/repository/shift_repository.goapi/lib/usecase/shift_usecase.goapi/lib/usecase/shift_usecase_shiftcards_sqlmock_test.go
getNextTimeString(last.Time.ID)がEndTime算出・前後判定・最終スロットのeTime 再計算で3回重複して呼ばれていたのを1回にまとめ、カードごとにさらにクエリを 削減した。コメントもGoDoc形式(関数名を主語にした文)からプロジェクト規約に 沿った形に修正。
インフラ班によるシナリオ①再試験の結果を8章として追記。シフトデータの 分散(1日50件→date_id1〜4に12件ずつ)、pgbouncer導入、N+1修正(PR #438) それぞれの効果を切り分けて記録。pgbouncerは500エラー・タイムアウトを 解消するが、N+1修正はその後も独立して+31%スループット・約4倍のレイ テンシ改善をもたらすことを実測。 また、本試験が2.4節の方針(隔離DBのみで試験)から外れ共有HAクラスタ上で 実施された点を確認事項として明記した。
1 participant
対応Issue
resolve #437
概要
GET /shift-cards/users/:user_id/dates/:date_id/weathers/:weather_id（モバイル「マイシフト」画面が使用）のカード生成で、GetUsersByShiftが15分スロットごと・前後スロット分で繰り返し呼ばれていたのを、カード単位の1クエリバッチ取得に変更した。ShiftRepositoryにUsersByTimesを追加（Users()のJOIN形状＋ANY($4)配列パラメータ、user_repository.goのFindByNamesと同じ考え方）getUsersByTimes/toShiftMembersを追加し、createShiftCardFromGroupを再構成。グループ内の全time_id（本体スロット＋前後1枠）を先に集めて1回だけバッチ取得するGetUsersByShift自体・ShowUsersByShiftエンドポイントは無変更（単一time_id版として引き続き使用されるため）GetUsersByShiftが内部で発行していたyear/date/time/weatherの単票Find4本は、呼び出し元のいずれからも参照されていなかったため、新しいバッチ取得メソッドでは意図的に取得しない背景（なぜ今このタイミングで対応するか）
2026-07-20の負荷試験（
hey -z 1m -c 400 -q 0.5、目標約200 req/s）でこのエンドポイントが実測28.4 req/s・平均12.2秒・p95 18.8秒、HTTP 500が153件・タイムアウトが203件を記録し不合格となった。DBリーダーCPUが最大約90%まで上昇しており、45th開催（2026年9月上旬）を控えたデプロイ判断に直結する。docs/development/test-roadmap.mdはフェーズ2（repository層ゴールデンテスト、実DB統合）完了後にこの種のリファクタへ着手することを推奨しているが、フェーズ2は未着手（フェーズ1の割り振りが2026-07-23に出たばかり）。本番負荷試験が既に不合格となっている状況を踏まえ、本issueに限りフェーズ2を待たずに着手した。安全網としてgo-sqlmockによるクエリ回数アサーション＋既存CI（go-test.yml）で代替している。なお本件は既存issue #247のセクション「1.2 ShiftCard関連のメンバー取得メソッドのN+1問題」と同じ関数群を扱うが、#247のその記述内容自体（
GetUsersByShift内部の個別ユーザー取得、grade/bureauの個別取得）は既に別途解消済みだったため、今回の負荷試験で新たに見つかった「呼び出し回数」の問題として#437を切り出している（詳細はissue本文参照）。画面スクリーンショット等
テスト項目
cd api && go test ./... -count=1がグリーンshift_usecase_shiftcards_sqlmock_test.go: 単一カード(6スロット)でユーザー取得クエリが1回にまとまることMatchExpectationsInOrder(false)でmapの反復順に依存しない形）git stashでリバート）と修正後でレスポンスJSONが完全に一致することを確認済み（air のホットリロードで両方の状態を実機確認）備考
notification_usecase.goのprocessGroup、user/date/weatherバッチ化）とは別件Summary by CodeRabbit
改善
テスト