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
This pull request implements new APIs for creating (POST) and retrieving (GET by ID) sponsorship activities. However, it introduces a critical security vulnerability due to a lack of essential authorization checks; the POST endpoint allows creating activities for any user, and the GET endpoint allows viewing any activity by ID without verifying ownership or permissions, which could lead to unauthorized data access and manipulation. Additionally, several improvements are suggested, including enhancing error handling in the GetSponsorshipActivitiesId handler, optimizing SQL query execution in the FindByID repository method, constantizing hardcoded strings in the CreateSponsorshipActivity use case, and removing development comments.
Wakai111
force-pushed
the
feat/wakape/create-sponsorship-activity
branch
from
f7e8766 to
692ab9c
Compare
Deploying finansu with
|
| Latest commit: |
f616f3f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1c518f4b.finansu.pages.dev |
| Branch Preview URL: | https://feat-wakape-create-sponsorsh.finansu.pages.dev |
hikahana
left a comment
hikahana
left a comment
There was a problem hiding this comment.
generatedしてるんだからそれを使ってください。
変数命名長くても良いのでわかりやすくしてください。読みづらいです。
aiに書かせるのはいいですが、あなたがこのコードを理解してください。生成aiに全て書かせたように見えますがどうですか？
| // リクエスト用 | ||
| type CreateSponsorshipActivityRequest struct { | ||
| YearPeriodsID int `json:"yearPeriodsId"` | ||
| SponsorID int `json:"sponsorId"` | ||
| UserID int `json:"userId"` | ||
| ActivityStatus string `json:"activityStatus"` | ||
| FeasibilityStatus string `json:"feasibilityStatus"` | ||
| DesignProgress string `json:"designProgress"` | ||
| Remarks string `json:"remarks"` | ||
| SponsorStyleDetails []CreateSponsorStyleRequest `json:"sponsorStyleDetails"` | ||
| } | ||
|
|
||
| // リクエスト用 | ||
| type CreateSponsorStyleRequest struct { | ||
| SponsorStyleID int `json:"sponsorStyleId"` | ||
| Category string `json:"category"` | ||
| } |
There was a problem hiding this comment.
[ask]
openapiで定義してると思うんだけどなんでこれを使わないでdomainに書いたの？
意図ありですか？
Line 3326 in ff8cb9f
FinanSu/api/generated/openapi_gen.go
Line 109 in ff8cb9f
| return u.repo.Find(ctx, id) | ||
| } | ||
| // 基本データを取得 | ||
| activity, err := u.repo.FindByID(ctx, id) |
There was a problem hiding this comment.
[must]
変数命名に関しては、activityだけだとなんのか分からんから↓みたいまで具体的に書いてほしい。
SponsorshipActivityByID
| if req.YearPeriodsID == 0 || req.SponsorID == 0 || req.UserID == 0 { | ||
| return echo.NewHTTPError(http.StatusBadRequest, "必須項目（年度期間ID、企業ID、担当者ID）が不足しています。") | ||
| } |
There was a problem hiding this comment.
[must]
openapi_genだとポインタで定義になるから==0ではなくて
=! nilのようにnilでないことで必須チェックになると思う。
で、必須チェックするのはyearPeriodsIdとUserIdだけだと思う。
// SponsorStyleDetails 登録したい協賛プラン情報のリスト
SponsorStyleDetails *[]struct {
Category *CreateSponsorshipActivityRequestSponsorStyleDetailsCategory `json:"category,omitempty"`
SponsorStyleId *int `json:"sponsorStyleId,omitempty"`
} `json:"sponsorStyleDetails,omitempty"`
UserId int `json:"userId"`
YearPeriodsId int `json:"yearPeriodsId"`
| } | ||
|
|
||
| // 登録処理（ユースケースの呼び出し） | ||
| createdActivity, err := h.sponsorshipActivityUseCase.CreateSponsorshipActivity(c.Request().Context(), req) |
There was a problem hiding this comment.
| createdActivity, err := h.sponsorshipActivityUseCase.CreateSponsorshipActivity(c.Request().Context(), req) | |
| CreateSponsorshipActivity, err := h.sponsorshipActivityUseCase.CreateSponsorshipActivity(c.Request().Context(), req) |
| func (h *Handler) PostSponsorshipActivities(c echo.Context) error { | ||
| return c.String(http.StatusOK, "PostSponsorshipActivities: Mock Response") | ||
| // リクエストボディを受け取るための構造体を用意 | ||
| var req domain.CreateSponsorshipActivityRequest |
There was a problem hiding this comment.
| var req domain.CreateSponsorshipActivityRequest | |
| var createSponsorshipActivityRequest generated.CreateSponsorshipActivityRequest |
とかで命名して欲しい。reqは避けてほしい何なのか分からないから
| func (u *sponsorshipActivityUseCase) CreateSponsorshipActivity(ctx context.Context, activity domain.SponsorshipActivity) (domain.SponsorshipActivity, error) { | ||
| u.repo.Create(ctx, activity) | ||
| // プラン一覧を取得 | ||
| styles, err := u.repo.GetStyleDetailsByActivityIDs(ctx, []int{id}) |
There was a problem hiding this comment.
| styles, err := u.repo.GetStyleDetailsByActivityIDs(ctx, []int{id}) | |
| sponsorStyleDetailsStyle, err := u.repo.GetSponsorStyleDetailsByActivityIDs(ctx, []int{id}) |
| // 未着手の補完 | ||
| if req.ActivityStatus == "" { | ||
| req.ActivityStatus = string(generated.ActivityStatusUnstarted) | ||
| } | ||
| if req.FeasibilityStatus == "" { | ||
| req.FeasibilityStatus = string(generated.Unstarted) | ||
| } | ||
| if req.DesignProgress == "" { | ||
| req.DesignProgress = string(generated.DesignProgressUnstarted) | ||
| } |
There was a problem hiding this comment.
[ask]
ここ補完処理をしていると思うんだけど、openapiの定義では必須にしている。
必須にしているなら空ならhandlerの段階でバリデーションで弾かれるからこの処理がされることはないと思うので不要だと思う。
けど、最初の設計としてはどう考えていたの？何もなければ未着手にしたい？
どっちの責務にしたい？クライアント側の責務にするならこれは不要で。サーバー側のせきむにするならrequiredを外してサーバー側にデフォルト値を入れるとかになると思う。
CreateSponsorshipActivityRequest:
type: object
description: 新規作成時のリクエストボディ
required:
- yearPeriodsId
- sponsorId
- userId
- activityStatus
- feasibilityStatus
- designProgress
properties:
yearPeriodsId:
type: integer
example: 5
sponsorId:
type: integer
example: 101
userId:
type: integer
example: 5
activityStatus:
$ref: "#/components/schemas/ActivityStatus"
feasibilityStatus:
$ref: "#/components/schemas/FeasibilityStatus"
designProgress:
$ref: "#/components/schemas/DesignProgress"
| } | ||
|
|
||
| // トランザクション開始 | ||
| tx, err := u.repo.Begin(ctx) |
| return domain.SponsorshipActivity{}, err | ||
| } | ||
|
|
||
| defer tx.Rollback() |
| } | ||
|
|
||
| // コミット | ||
| if err := tx.Commit(); err != nil { |
2 participants
対応Issue
概要
協賛活動の「新規作成(POST)」と「詳細取得(GET ID)」のAPIを実装しました。
POST /sponsorship_activities：活動本体とプラン紐付け（中間テーブル）を安全に一括登録する処理を実装GET /sponsorship_activities/{id}：ID指定による詳細取得を実装画面スクリーンショット等
バックエンドのAPI実装のため、画面の変更はありません。
テスト項目
POST /sponsorship_activitiesを実行し、レスポンスが正常に返ってくることsponsorship_activitiesとactivity_sponsor_style_linksの両方に正しくデータが保存されていることGET /sponsorship_activities/{id}を実行し、作成したデータが正しく取得できること備考
本PRがマージされたあとに、この実装に依存した「更新・削除(PUT/DELETE)」のPRを提出予定です。