Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! このプルリクエストは、協賛活動の更新APIを実装し、関連するスポンサースタイルリンクの管理を改善します。既存のリンクと新しいリクエストの差分を効率的に計算し、データベーストランザクションを通じて一貫性のある更新を保証することで、データの整合性を維持しながら柔軟な更新を可能にします。 Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
このプルリクエストは、協賛活動の更新APIを実装するもので、ドメインロジック、ユースケース、リポジトリ、ハンドラ層にわたる変更が含まれています。全体的に、トランザクションを利用した堅牢な更新処理が実装されており、ドメインロジックの差分計算もテストで十分にカバーされています。いくつかの改善点として、パフォーマンス向上のためのN+1問題の解消、トランザクション管理のコードの簡潔化、テストコードの未使用フィールドの削除を提案します。これらの点を修正することで、より保守性が高く効率的なコードになるでしょう。
| existing SponsorStyleLinks | ||
| requested SponsorStyleLinks | ||
| wantDeleteIDs []int | ||
| wantCreateCount int |
There was a problem hiding this comment.
wantCreateCount フィールドがテストケース内で定義されていますが、使用されていません。コードの可読性を保つために、この未使用のフィールドは削除することをお勧めします。
| if err := u.repo.Update(ctx, tx, activity); err != nil { | ||
| u.transactionRepo.RollBack(ctx, tx) | ||
| return generated.SponsorshipActivity{}, err | ||
| } | ||
|
|
||
| // NOTE: N+1問題はあるが、協賛スタイルは件数が多くない想定のため、許容 by わかぺ | ||
| // 5. 削除 | ||
| for _, linkID := range toDeleteIDs { | ||
| if err := u.repo.DeleteSponsorStyleLinkByID(ctx, tx, linkID); err != nil { | ||
| u.transactionRepo.RollBack(ctx, tx) | ||
| return generated.SponsorshipActivity{}, err | ||
| } | ||
| } | ||
|
|
||
| // 6. 追加 | ||
| for _, link := range toCreate { | ||
| category := generated.SponsorStyleCategory(link.Category) | ||
| newLink := generated.ActivitySponsorStyleLink{ | ||
| SponsorStyleId: &link.Style.ID, | ||
| Category: &category, | ||
| } | ||
| if err := u.repo.CreateSponsorStyleLink(ctx, tx, newLink, id); err != nil { | ||
| u.transactionRepo.RollBack(ctx, tx) | ||
| return generated.SponsorshipActivity{}, err | ||
| } | ||
| } | ||
|
|
||
| // 7. コミット | ||
| if err := u.transactionRepo.Commit(ctx, tx); err != nil { | ||
| return generated.SponsorshipActivity{}, err | ||
| } |
There was a problem hiding this comment.
トランザクション内の各エラー処理で u.transactionRepo.RollBack(ctx, tx) を呼び出しており、コードが冗長になっています。トランザクションを扱う処理をクロージャ（無名関数）にまとめることで、エラーハンドリングとロールバック処理を1箇所に集約でき、コードがよりクリーンになります。
err = func(tx *sql.Tx) error {
// 4. メイン情報更新
if err := u.repo.Update(ctx, tx, activity); err != nil {
return err
}
// NOTE: N+1問題はあるが、協賛スタイルは件数が多くない想定のため、許容 by わかぺ
// 5. 削除
for _, linkID := range toDeleteIDs {
if err := u.repo.DeleteSponsorStyleLinkByID(ctx, tx, linkID); err != nil {
return err
}
}
// 6. 追加
for _, link := range toCreate {
category := generated.SponsorStyleCategory(link.Category)
newLink := generated.ActivitySponsorStyleLink{
SponsorStyleId: &link.Style.ID,
Category: &category,
}
if err := u.repo.CreateSponsorStyleLink(ctx, tx, newLink, id); err != nil {
return err
}
}
// 7. コミット
return u.transactionRepo.Commit(ctx, tx)
}(tx)
if err != nil {
u.transactionRepo.RollBack(ctx, tx)
return generated.SponsorshipActivity{}, err
}| for _, linkID := range toDeleteIDs { | ||
| if err := u.repo.DeleteSponsorStyleLinkByID(ctx, tx, linkID); err != nil { | ||
| u.transactionRepo.RollBack(ctx, tx) | ||
| return generated.SponsorshipActivity{}, err | ||
| } | ||
| } | ||
|
|
||
| // 6. 追加 | ||
| for _, link := range toCreate { | ||
| category := generated.SponsorStyleCategory(link.Category) | ||
| newLink := generated.ActivitySponsorStyleLink{ | ||
| SponsorStyleId: &link.Style.ID, | ||
| Category: &category, | ||
| } | ||
| if err := u.repo.CreateSponsorStyleLink(ctx, tx, newLink, id); err != nil { | ||
| u.transactionRepo.RollBack(ctx, tx) | ||
| return generated.SponsorshipActivity{}, err | ||
| } | ||
| } |
There was a problem hiding this comment.
// NOTE にも書かれている通り、ここの削除と追加のループはN+1問題を引き起こします。協賛スタイルの数が少ないうちは問題になりにくいですが、将来的なパフォーマンスを考慮し、バルク操作にまとめることをお勧めします。
具体的には、以下のような変更が考えられます。
- 削除:
DeleteSponsorStyleLinkByIDの代わりにDeleteSponsorStyleLinksByIDs(ctx, tx, toDeleteIDs)のようなメソッドをリポジトリに作成し、DELETE FROM ... WHERE id IN (...)を一度に実行します。 - 追加:
CreateSponsorStyleLinkの代わりにCreateSponsorStyleLinks(ctx, tx, toCreate, id)のようなメソッドを作成し、INSERT INTO ... VALUES (...), (...), ...のように一括で登録します。
これにより、データベースへのラウンドトリップが削減され、パフォーマンスが向上します。
There was a problem hiding this comment.
Pull request overview
協賛活動（SponsorshipActivity）の更新 API（PUT /sponsorship_activities/:id）を実装し、協賛スタイルリンク（activity_sponsor_style_links）を「差分（維持/追加/削除）」にもとづいて更新できるようにするPRです。
Changes:
SponsorStyleLinks.Diffを追加し、既存リンクと更新リクエストの差分を計算- UseCase に更新処理（トランザクション内で活動本体更新＋リンクの追加/削除）を実装
- Repository に既存リンク取得・更新・リンク削除のSQL実装を追加し、Handler の PUT を実装
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| api/internals/usecase/sponsorship_activity_usecase.go | 更新ユースケースを実装（差分計算→トランザクション更新） |
| api/internals/domain/sponsorship_activity.go | SponsorStyleLinks と Diff ロジックを追加 |
| api/internals/domain/sponsorship_activity_test.go | Diff のユニットテストを追加 |
| api/externals/repository/sponsorship_activity_repository.go | 既存リンク取得・Update・リンク削除を追加 |
| api/externals/handler/sponsorship_activity_handler.go | PUT /sponsorship_activities/:id のハンドラを実装 |
| cat := "" | ||
| if detail.Category != nil { | ||
| cat = string(*detail.Category) | ||
| } | ||
| requestedLinks = append(requestedLinks, domain.NewSponsorStyleLink(*detail.SponsorStyleId, cat)) | ||
| } |
There was a problem hiding this comment.
detail.Category が nil の場合に cat を空文字にしているため、PUT で category を省略すると DB に空文字が保存される（または enum/制約で失敗する）挙動になります。CreateSponsorshipActivity では Category をそのまま（nil のまま）渡しているので、更新系も nil を保持して NULL として更新/挿入できるようにするか、category を必須入力としてバリデーションしてください。
| remarks := "" | ||
| if req.Remarks != nil { | ||
| remarks = *req.Remarks | ||
| } | ||
| activity := domain.SponsorshipActivity{ | ||
| ID: id, | ||
| YearPeriodsID: req.YearPeriodsId, | ||
| SponsorID: req.SponsorId, | ||
| UserID: req.UserId, | ||
| ActivityStatus: string(req.ActivityStatus), | ||
| FeasibilityStatus: string(req.FeasibilityStatus), | ||
| DesignProgress: string(req.DesignProgress), | ||
| Remarks: remarks, | ||
| } |
There was a problem hiding this comment.
req.Remarks が nil の場合に空文字へ変換して更新しているため、リクエストで remarks を省略すると既存の NULL/値が空文字で上書きされます（Create は nil のまま保存できる実装）。PUT の仕様として省略時はエラーにするか、nil は NULL 更新できるように（sql.NullString や *string を利用する等）扱いを揃えるのが望ましいです。
| } | ||
|
|
||
| // 7. コミット | ||
| if err := u.transactionRepo.Commit(ctx, tx); err != nil { |
There was a problem hiding this comment.
Commit に失敗した場合に RollBack を呼んでいないため、トランザクションが未クローズのまま残りロック/コネクション枯渇の原因になります。他の UseCase では commit 失敗時に rollback しているので、この箇所も同様に rollback してからエラーを返すようにしてください。
| if err := u.transactionRepo.Commit(ctx, tx); err != nil { | |
| if err := u.transactionRepo.Commit(ctx, tx); err != nil { | |
| u.transactionRepo.RollBack(ctx, tx) |
| return generated.SponsorshipActivity{}, err | ||
| } | ||
|
|
||
| // NOTE: N+1問題はあるが、協賛スタイルは件数が多くない想定のため、許容 by わかぺ |
There was a problem hiding this comment.
コメントで「N+1問題」とありますが、ここは1リクエスト内での複数 DELETE/INSERT のループであり一般的な N+1（親取得→子を件数分SELECT）とは異なります。また「by 個人名」の記述はコードベース内の恒久コメントとしては情報価値が低いので、コスト/許容理由を技術的に記述する形に修正するのがよいです。
| // NOTE: N+1問題はあるが、協賛スタイルは件数が多くない想定のため、許容 by わかぺ | |
| // NOTE: 協賛スタイルの削除・追加は1リクエスト内で複数回の DELETE/INSERT をループで発行している。 | |
| // 想定される協賛スタイル数は多くなく、現状のパフォーマンスコストは許容範囲と判断している。 | |
| // 今後スタイル数が増加しボトルネックとなる場合は、バルク操作等による最適化を検討すること。 |
| Where(goqu.I("activity_sponsor_style_links.sponsorship_activity_id").Eq(sponsorshipActivityID)) | ||
|
|
There was a problem hiding this comment.
GetSponsorStyleLinksBySponsorshipActivityID のクエリに ORDER BY が無いため、同一キー（sponsor_style_id + category）が複数存在するケースで「どの既存 link ID を維持するか」がDBの返却順に依存して不安定になります（UseCase の Diff は最初にマッチした既存を維持する実装）。ID維持の要件があるなら、activity_sponsor_style_links.id 等で ORDER BY を付けて決定的にしてください。
| Where(goqu.I("activity_sponsor_style_links.sponsorship_activity_id").Eq(sponsorshipActivityID)) | |
| Where(goqu.I("activity_sponsor_style_links.sponsorship_activity_id").Eq(sponsorshipActivityID)). | |
| Order(goqu.I("activity_sponsor_style_links.id").Asc()) |
| return nil, err | ||
| } | ||
| links = append(links, link) | ||
| } |
There was a problem hiding this comment.
rows.Next() のループ後に rows.Err() を確認していないため、イテレーション中に発生した遅延エラー（ネットワーク/ドライバ都合など）を見逃す可能性があります。ループ終了後に rows.Err() をチェックしてエラーを返すようにしてください。
| } | |
| } | |
| if err := rows.Err(); err != nil { | |
| return nil, err | |
| } |
| existing SponsorStyleLinks | ||
| requested SponsorStyleLinks | ||
| wantDeleteIDs []int | ||
| wantCreateCount int |
There was a problem hiding this comment.
テストケース struct に wantCreateCount フィールドがありますが、どのケースでも値がセットされずアサーションにも使われていません。未使用フィールドは誤解を生むので削除するか、意図があるなら実際に検証に使うようにしてください。
| wantCreateCount int |
| if detail.SponsorStyleId == nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
req.SponsorStyleDetails の要素で SponsorStyleId が nil の場合に continue しているため、クライアントの不正/欠損データを静かに無視してしまい、結果として requestedLinks が空になって既存リンクが全削除される等の事故が起きえます。SponsorStyleId が nil の要素が来た時点で 4xx を返すなど、入力として明確に弾く方が安全です。
Deploying finansu with
|
| Latest commit: |
5a4b97e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://49308346.finansu.pages.dev |
| Branch Preview URL: | https://feat-kubosaka-update-sponsor.finansu.pages.dev |
対応Issue
resolve #0
概要
協賛活動（SponsorshipActivity）の更新API (
PUT /sponsorship_activities/:id) を実装した。SponsorshipActivity.Diffメソッドを追加し、既存のActivitySponsorStyleLinksと新規リクエストの差分を計算（維持・追加・削除に分類）UpdateSponsorshipActivityを実装し、差分に基づいてリンクの追加・削除を実行UpdateSponsorshipActivity、CreateActivitySponsorStyleLinks、DeleteActivitySponsorStyleLinkを実装UpdateSponsorshipActivityを実装画面スクリーンショット等
なし（API実装のみ）
テスト項目
PUT /sponsorship_activities/:idで完全一致（変更なし）の場合、既存レコードのIDが維持されること備考
なし