Merged
Conversation
```python
>>> import glob
>>> import shutil
>>> files = glob.glob("internal/model/*_impl*.go")
>>> mvs = [(f, f.replace("_impl", "")) for f in files]
>>> for (src, dst) in mvs:
... shutil.move(src, dst)
```
📝 WalkthroughWalkthroughドメイン型とリポジトリインターフェースを Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/model/application_test.go (1)
16-32:⚠️ Potential issue | 🟡 Minor
toExpectedApplicationResponseでTargetsとStatusesがコメントアウトされているLine 29-30 で
TargetsとStatusesがコメントアウトされているため、GetApplicationsのテストではこれらのフィールドが検証されません。これが意図的な一時措置であれば、TODO コメントを追加して追跡できるようにしてください。実際のレスポンスにこれらの値が含まれている場合、テストの比較結果に影響する可能性があります。この TODO の追跡用 issue を作成しましょうか？
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/model/application_test.go` around lines 16 - 32, The helper toExpectedApplicationResponse currently omits Targets and Statuses (they are commented out), which causes GetApplications tests to skip those fields; fix by restoring those fields in the returned service.ApplicationResponse (set Targets: rd.Targets and Statuses: rd.Statuses) inside the toExpectedApplicationResponse function so the test compares full responses, or if omission was intentional add a clear TODO comment in toExpectedApplicationResponse explaining why Targets/Statuses are excluded and create an issue reference so the missing assertions are tracked.internal/router/application.go (1)
619-625:⚠️ Potential issue | 🟡 Minorリクエストボディの
statusフィールドに対する明示的な検証がない
GetApplicationsではクエリパラメータのstatusに対してValid()による検証を行っているが（Line 129-132）、PutStatusではバインド後にreq.Statusの値が有効かどうかをチェックしていない。未知のステータス文字列が送られた場合、実際には後続の状態遷移チェック（IsAbleAccountManagerChangeState等）で弾かれるが、返るエラーメッセージが「Xからinvalid_statusへは変更できない」という誤解を招く内容になる。🛡️ 修正案
var req PutStatus if err = c.Bind(&req); err != nil { logger.Info("could not get status from request", zap.Error(err)) return service.NewBadInputError("could not get status from request"). WithInternal(err) } + if !req.Status.Valid() { + return service.NewBadInputError(fmt.Sprintf("invalid status: %q", req.Status)) + }（
service.StatusにValid()メソッドが存在する前提。上記suggest_optional_refactorのシェルスクリプトで確認可能）🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/router/application.go` around lines 619 - 625, バインド後にPutStatusのreq.Statusが有効なステータスか検証していないため、不正な文字列が来たとき誤解を招くエラーメッセージになるので、c.Bind後にreq.StatusのValid()（service.Status.Valid()を前提）を呼んで検証し、無効ならlogger.Infoで記録してservice.NewBadInputError("invalid status")のような明確な入力エラーを返すように修正してください（参照箇所: PutStatus, req.Status, Valid(), GetApplicationsでの検証と後続のIsAbleAccountManagerChangeStateとの違いを意識）。
🧹 Nitpick comments (11)
internal/service/account_manager.go (1)
10-12: サービス層の型における JSON タグの不整合。
AccountManagerにはjson:"id"タグが付いていますが、同じ PR で追加された他のサービス層の型（Tag、Comment）には JSON タグがありません。サービス層の型はドメインモデルとして扱われるべきで、シリアライゼーションの責務はルーター/ハンドラー層に委ねるのが一般的です。統一するなら、JSON タグを削除してルーター層のレスポンス型に任せることを推奨します。
♻️ JSON タグ削除の提案
type AccountManager struct { - ID uuid.UUID `json:"id"` + ID uuid.UUID }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/account_manager.go` around lines 10 - 12, AccountManager struct has a JSON tag on the ID field that creates inconsistency with other service-layer types (Tag, Comment); remove the `json:"id"` tag from the AccountManager type (the ID field in the AccountManager struct) so service/domain models contain no serialization tags and leave JSON marshaling to the router/handler response types, ensuring Tag and Comment remain unchanged.internal/service/comment.go (1)
11-17: フィールド命名の改善提案。2 点ほど命名に関する提案です：
User uuid.UUID→UserID uuid.UUID: UUID を格納するフィールドであるため、UserIDの方が意図が明確です。Comment string→ 構造体名と同名のフィールドは混乱を招く可能性があります（例:comment.Commentのようなアクセスが必要になります）。ContentやBodyなどへの変更を検討してください。既存のコードベースとの互換性や命名規約を優先して判断してください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/comment.go` around lines 11 - 17, The Comment struct fields User and Comment are ambiguously named; rename User to UserID and rename Comment to either Content or Body (pick one consistent name) in the Comment struct declaration, then update all references (constructors, methods, serializers/deserializers, DB mappers, JSON tags, tests, and any code using comment.User or comment.Comment) to use the new names (e.g., Comment.UserID and Comment.Content/Body) to preserve behavior and compilation; ensure any ORM or JSON tags are adjusted to maintain the same persisted/serialized keys if required.internal/router/user_test.go (1)
22-33:modelUserToUserは削除予定（TODO）— 対応を支援できますTODO コメントが示す通り、
userFromModelUserが既に存在するため本関数は冗長です。シグネチャの差異（ポインタ vs 値）を統一した上で統合する方針でよければ、修正を作成することができます。対応を自動化したい場合は、新しいイシューを作成しましょうか？
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/router/user_test.go` around lines 22 - 33, Remove the redundant modelUserToUser function and consolidate conversion logic into the existing userFromModelUser; either (A) delete modelUserToUser and update any callers to call userFromModelUser (adjusting call sites if they currently pass a pointer/value), or (B) change userFromModelUser's signature to accept *service.User (or to return the same pointer/value shape) so both call sites can use it—ensure the ID/Name/DisplayName/AccountManager/CreatedAt/UpdatedAt/DeletedAt fields are preserved in the unified conversion and update imports/usages accordingly.internal/model/file.go (1)
72-79: Userエッジ未ロード時のpanicリスクを避けるガードを検討してください。コメントで注意喚起されていますが、
entfile.Edges.Userが未ロードの場合にnil参照になります。明示的なガードや「必ずWithUserで読む」前提のドキュメント化があると安全です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/model/file.go` around lines 72 - 79, The ConvertEntFileToModelFile function currently assumes entfile.Edges.User is loaded and will panic if nil; add an explicit nil-check on entfile.Edges.User before accessing .ID (or otherwise validate entfile.Edges != nil), and handle the missing edge by setting service.File.CreatedBy to the zero value (or an explicit empty string/ID) or return an error/alternate value per your API contract; also update callers or document that ConvertEntFileToModelFile requires the User edge to be loaded (e.g., ensure queries use WithUser) so the intent is clear.internal/model/tag.go (1)
77-85: エクスポートされたConvertEntTagToModelTagに nil チェックがない
application_status.goのconvertEntApplicationStatusToModelApplicationStatusは nil チェックを行っています。エクスポートされたこの関数にも同様のガードがあると一貫性が保たれます。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/model/tag.go` around lines 77 - 85, ConvertEntTagToModelTag lacks a nil guard and will panic if enttag is nil; add an initial nil check in ConvertEntTagToModelTag (the exported function) that returns nil when enttag == nil before accessing enttag.ID/Name/DeletedAt, mirroring the pattern used in convertEntApplicationStatusToModelApplicationStatus so callers receive a nil *service.Tag instead of a runtime panic.internal/model/comment.go (1)
108-116: エクスポートされたConvertEntCommentToModelCommentに nil チェックがない
convertEntApplicationStatusToModelApplicationStatus（Line 44 in application_status.go）は引数の nil チェックを行っていますが、こちらのエクスポートされた関数にはありません。外部から呼ばれる可能性があるため、整合性を持たせた方が安全です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/model/comment.go` around lines 108 - 116, The exported function ConvertEntCommentToModelComment lacks a nil check on its comment parameter; update it to mirror convertEntApplicationStatusToModelApplicationStatus by early-returning nil when comment == nil, so callers get a nil *service.Comment instead of panicking; keep the rest of the mapping (ID, User, Comment, CreatedAt, UpdatedAt) unchanged and preserve the same signature.internal/model/application_test.go (1)
679-685: Line 685 のrequire.NoErrorが冗長
errは Line 679 のCreateTagから取得され、Line 680 ですでにチェックされています。Line 685 のrequire.NoError(t, err)は同じerrを再チェックしており、不要です。♻️ 修正案
tag, err := repo.CreateTag(ctx, random.AlphaNumeric(t, 20)) require.NoError(t, err) target := &service.ApplicationTarget{ Target: user.ID, Amount: random.Numeric(t, 10000), } - require.NoError(t, err) application, err := repo.CreateApplication(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/model/application_test.go` around lines 679 - 685, The second require.NoError(t, err) is redundant because err was already checked after calling repo.CreateTag; remove the duplicate assertion (the require.NoError that references err after creating the ApplicationTarget) so the test only validates err once. Locate the repo.CreateTag call (assigning tag, err) and the subsequent service.ApplicationTarget construction and delete the extra require.NoError(t, err) that follows it.internal/model/application.go (1)
301-307: アプリケーションの冗長な no-op 更新Line 301-307 は同じ
applicationIDに対して何もフィールドを変更せずにSaveしています。この操作は不要に見えます。何らかの副作用（例:UpdatedAtの自動更新）を意図している場合はコメントを追加してください。♻️ 修正案
- _, err = tx.Client().Application. - UpdateOneID(applicationID). - Save(ctx) - if err != nil { - err = RollbackWithError(tx, err) - return nil, applicationErrorConverter.convert(err) - } -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/model/application.go` around lines 301 - 307, 現在の tx.Client().Application.UpdateOneID(applicationID).Save(ctx) 呼び出しはフィールド変更がなく実質 no-op になっているので、不要なら削除して RollbackWithError/ applicationErrorConverter.convert を呼ぶパスを適切に整理してください；もし UpdatedAt 等の副作用（自動更新）を意図しているなら、その意図を明示するコメントを追加するか、意図したフィールド（例: UpdatedAt を明示的に更新するロジック）を UpdateOneID(...).SetXxx(...) で行うようにし、不要な Save 呼び出しを残さないようにしてください。internal/model/application_status.go (1)
41-53:Edges.Userが nil の場合、Line 49 でパニックが発生する可能性がある
convertEntApplicationStatusToModelApplicationStatusはapplicationStatus.Edges.User.IDに直接アクセスしていますが、WithUser()なしでクエリされたent.ApplicationStatusが渡された場合、nil ポインタ参照でパニックします。現在のすべての呼び出し元（CreateStatus、GetApplication、UpdateApplication）ではWithUser()を明示的に使用しているため安全ですが、将来的なコード変更で防御的チェックがあると更に堅牢です。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/model/application_status.go` around lines 41 - 53, convertEntApplicationStatusToModelApplicationStatus currently dereferences applicationStatus.Edges.User.ID which can panic if Edges.User is nil; update the function to defensively check if applicationStatus.Edges != nil && applicationStatus.Edges.User != nil before reading .ID and populate CreatedBy with a safe zero/nullable value when the user edge is missing, while leaving the rest of the mapping (Status via convertEntApplicationStatusToModelStatus, ID, CreatedAt) unchanged; make the check inside convertEntApplicationStatusToModelApplicationStatus so all callers (CreateStatus, GetApplication, UpdateApplication) become robust against ent objects queried without WithUser().internal/service/application.go (1)
25-30:Application構造体の使用方法を確認してください
Application構造体は内部ではinternal/model/file_test.goで使用されていますが、使用範囲は限定的です。テストコードでIDフィールドのみが設定され、Amount、CreatedAt、UpdatedAtフィールドは一切使用されていません。ApplicationRepositoryインターフェースもApplicationResponse、ApplicationDetail、ApplicationQueryを返しており、この構造体を返すことはありません。フィールドの大半が不要な場合は、構造体の設計を見直すか、テストをuuid.UUIDで直接操作するようにリファクタリングすることを検討してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/application.go` around lines 25 - 30, The Application struct currently defines unused fields (Amount, CreatedAt, UpdatedAt) while tests (internal/model/file_test.go) only set ID and repository interfaces (ApplicationRepository returning ApplicationResponse/ApplicationDetail/ApplicationQuery) never return Application; either trim Application to only ID or remove the struct and update tests to use uuid.UUID directly: update internal/model/file_test.go to stop constructing Application and instead use uuid.UUID values, and verify ApplicationRepository-related types (ApplicationResponse, ApplicationDetail, ApplicationQuery) remain the single sources of truth; if other code relies on Application, keep only required fields (ID) and remove/replace Amount, CreatedAt, UpdatedAt to minimize dead code.internal/router/application.go (1)
525-533: ラムダ引数cが外側のc *echo.ContextをシャドウしているLine 562の
c.JSON(...)は外側のc *echo.Contextを正しく参照しているため動作上の問題はないが、ラムダ引数の命名が紛らわしい。♻️ リファクタリング案
- comments := lo.Map(application.Comments, func(c *service.Comment, _ int) *CommentDetail { + comments := lo.Map(application.Comments, func(comment *service.Comment, _ int) *CommentDetail { return &CommentDetail{ - ID: c.ID, - User: c.User, - Comment: c.Comment, - CreatedAt: c.CreatedAt, - UpdatedAt: c.UpdatedAt, + ID: comment.ID, + User: comment.User, + Comment: comment.Comment, + CreatedAt: comment.CreatedAt, + UpdatedAt: comment.UpdatedAt, } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/router/application.go` around lines 525 - 533, The lambda passed to lo.Map is using parameter name `c`, which shadows the outer `c *echo.Context` and is confusing; change the lambda parameter in the lo.Map call over application.Comments to a non-conflicting name (e.g., `cm` or `comment`) and update all uses inside the lambda that currently reference `c` to the new name so the mapping still constructs &CommentDetail{ID: ..., User: ..., Comment: ..., CreatedAt: ..., UpdatedAt: ...} without shadowing the outer context variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/model/account_manager.go`:
- Around line 33-41: The Update().Save(ctx) call in AddAccountManagers (and
similarly in DeleteAccountManagers) currently ignores the returned affected-rows
count so calls with non-existent userIDs silently succeed; change the
implementation to capture the result as rowsAffected, err :=
repo.client.User.Update().Where(user.IDIn(userIDs...)).SetAccountManager(true).Save(ctx),
convert the err as before, then validate rowsAffected (e.g., if rowsAffected !=
len(userIDs) or rowsAffected == 0) and return a meaningful error when not all
IDs were found/updated so callers can detect missing users.
In `@internal/model/application_status.go`:
- Around line 55-69: The switch in convertEntApplicationStatusToModelStatus
lacks a default branch so unexpected entStatus.String() values return the zero
value; add a default case in the switch inside
convertEntApplicationStatusToModelStatus that panics (or otherwise fails fast)
with a descriptive message containing entStatus.String() to make data
inconsistencies visible (use fmt.Sprintf or a logger) so the function never
silently returns an invalid service.Status for unknown applicationstatus.Status
values.
In `@internal/model/application.go`:
- Around line 121-124: Protect access to the Status edge to avoid index out of
range panics: in the mapping where you call
convertEntApplicationResponseToModelApplicationResponse (the block referencing
r, r.Edges.Tag, r.Edges.Status[0], r.Edges.User) create a local status variable,
set it to nil or a sensible zero/default, and only assign r.Edges.Status[0] if
len(r.Edges.Status) > 0, then pass that variable to
convertEntApplicationResponseToModelApplicationResponse; apply the same guard
pattern to the other callers in GetApplication and UpdateApplication that
directly index r.Edges.Status[0].
- Around line 228-245: GetApplication loads comments without their User edge,
causing a nil pointer when accessing c.Edges.User.ID; fix the application query
to load comment users by replacing the comment loader WithComment() with a query
that includes the user edge (use QueryComment().WithUser() or the equivalent ent
pattern) so r.Edges.Comment entries have Edges.User populated; keep
ConvertEntCommentToModelComment(c, c.Edges.User.ID) as-is once the User edge is
loaded (see how UpdateApplication uses QueryComment().WithUser() for the correct
reference).
- Around line 28-64: The applicationsq builder can remain nil for unknown
query.Sort values and the created_at sort directions are inverted versus the
usual "-" = descending convention; update the sort handling in the block that
builds applicationsq (look for applicationsq, repo.client.Application.Query(),
query.Sort, application.FieldCreatedAt, application.FieldTitle, and
applicationstatus.FieldCreatedAt): add a final else/default branch that
constructs a safe default query (e.g., order by created_at descending with
WithTag/WithStatus/WithUser) so applicationsq is never nil, and swap the
ordering for the created_at cases so "*query.Sort == \"-created_at\"" maps to
Desc and the plain "created_at" maps to Asc (or align both to your intended
convention consistently).
- Around line 342-358: UpdateApplication: comment and file fetch errors (from
updated.QueryComment() and updated.QueryFile()) return converted errors without
calling tx.RollbackWithError, relying only on defer panic recovery; change those
error returns to call tx.RollbackWithError(err) before returning the converted
error. Specifically, in the blocks handling entcomments and entfiles, invoke
tx.RollbackWithError(err) (using the transaction variable used in
UpdateApplication) and then return the converted error via
applicationErrorConverter.convert(err) to match other error handling paths and
ensure immediate rollback instead of deferring to commit failure.
- Around line 23-127: GetApplications has repeated
Query().WithTag().WithStatus(...).WithUser() chains per sort branch; refactor by
creating a base applicationsq :=
repo.client.Application.Query().WithTag().WithStatus(func(q
*ent.ApplicationStatusQuery){q.Order(ent.Desc(applicationstatus.FieldCreatedAt))}).WithUser()
once, then apply ordering based on query.Sort (map ""/"created_at" to
Order(ent.Desc(application.FieldCreatedAt)), "-created_at" to
Order(ent.Asc(...)), "title" to Order(ent.Asc(application.FieldTitle)), "-title"
to Order(ent.Desc(application.FieldTitle))) before adding filters (Target,
Status, Since, Until, Tag, CreatedBy), limits/offsets and calling All(ctx);
preserve the existing applicationstatus ordering and use the same identifiers
(GetApplications, applicationsq, applicationstatus.FieldCreatedAt,
application.FieldCreatedAt, application.FieldTitle) so the behavior remains
identical.
In `@internal/model/comment.go`:
- Around line 64-85: UpdateComment lacks a DB-level ownership check when
changing ApplicationID; add the same ownership verification used in
DeleteComment so a comment cannot be moved between applications. Specifically,
before calling
UpdateOneID(commentID).SetApplicationID(applicationID)...Save(ctx) ensure the
update is constrained to the owning application (either load the comment with
repo.client.Comment.Query().Where(comment.IDEQ(commentID)).Only(ctx) and verify
its ApplicationID equals the provided applicationID, or perform the update via
repo.client.Comment.Update().Where(comment.IDEQ(commentID),
comment.ApplicationIDEQ(applicationID)).UpdateOneID/Save so the DB rejects
non‑owners), and return the converted error via commentErrorConverter.convert on
mismatch/failure.
In `@internal/model/file.go`:
- Around line 17-39: CreateFile performs two separate DB updates
(repo.client.File.Create and repo.client.Application.UpdateOneID.AddFile) and
must be atomic; wrap both operations in an ent transaction (use
repo.client.Tx(ctx) to begin a tx, perform tx.File.Create(...) and then
tx.Application.UpdateOneID(applicationID).AddFile(created).Save(ctx), commit the
tx when both succeed and roll back on any error) so the File isn't left orphaned
if the Application update fails; alternatively, if you prefer not to use ent.Tx,
ensure you delete the created File on Application update failure. Refer to
EntRepository.CreateFile, repo.client.File, repo.client.Application and use
ent.Tx for the transaction handling.
In `@internal/service/application_file.go`:
- Around line 4-5: The ApplicationFileRepository interface is currently empty
and should declare its contract like the other interfaces (TagRepository,
CommentRepository, AccountManagerRepository) or be annotated for future work;
update the ApplicationFileRepository definition to either (a) add the required
method signatures for managing application files (e.g., Save, GetByID, Delete,
ListByAppID — name and params consistent with existing repository patterns) or
(b) add a clear TODO comment above the interface and a brief go:generate mock
directive note stating methods will be added, so the intent is explicit and mock
generation is meaningful; refer to the ApplicationFileRepository type and the
existing TagRepository/CommentRepository/AccountManagerRepository patterns to
match naming and signatures.
In `@internal/service/application_status.go`:
- Around line 41-44: 現在の Status.MarshalJSON returns an empty string for
invalid/zero values which hides errors; update Status.MarshalJSON to validate
the receiver (Status) first and return a non-nil error for unset/invalid enum
values instead of json.Marshal("") so callers can detect bad data. Locate the
MarshalJSON method on type Status and add a validity check (e.g., compare
against known constants or implement an IsValid method) and if invalid return an
error; otherwise return json.Marshal(s.String()).
In `@internal/service/application_tag.go`:
- Around line 4-5: ApplicationTagRepository
が空のインターフェースになっているため任意の型で満たされコンパイル時制約になっていません。該当箇所（ApplicationTagRepository と同様に
ApplicationFileRepository）を見つけ、元の model
パッケージに定義されていたメソッド群を移植してインターフェースに明示的に追加するか、まだ未実装ならその意図を示すコメントを入れるか、あるいは当面
Repository への組み込みを取り下げてください（対象シンボル: ApplicationTagRepository,
ApplicationFileRepository, 元の model のメソッド名を参照して補完）。
In `@internal/service/repository.go`:
- Around line 3-14: Repository embeds ApplicationFileRepository and
ApplicationTagRepository which are currently empty interfaces (defined in
application_file.go and application_tag.go), so their embedding does not enforce
any methods; either remove or comment out these embeddings from the Repository
interface, or mark them with a TODO and add a clear comment explaining they are
placeholders until real methods are added in ApplicationFileRepository and
ApplicationTagRepository; update any related tests/types to reflect the change
and ensure Repository still compiles and enforces the intended method sets
(e.g., leave AccountManagerRepository, CommentRepository, FileRepository,
ApplicationStatusRepository, ApplicationTargetRepository, ApplicationRepository,
TagRepository, UserRepository intact).
---
Outside diff comments:
In `@internal/model/application_test.go`:
- Around line 16-32: The helper toExpectedApplicationResponse currently omits
Targets and Statuses (they are commented out), which causes GetApplications
tests to skip those fields; fix by restoring those fields in the returned
service.ApplicationResponse (set Targets: rd.Targets and Statuses: rd.Statuses)
inside the toExpectedApplicationResponse function so the test compares full
responses, or if omission was intentional add a clear TODO comment in
toExpectedApplicationResponse explaining why Targets/Statuses are excluded and
create an issue reference so the missing assertions are tracked.
In `@internal/router/application.go`:
- Around line 619-625:
バインド後にPutStatusのreq.Statusが有効なステータスか検証していないため、不正な文字列が来たとき誤解を招くエラーメッセージになるので、c.Bind後にreq.StatusのValid()（service.Status.Valid()を前提）を呼んで検証し、無効ならlogger.Infoで記録してservice.NewBadInputError("invalid
status")のような明確な入力エラーを返すように修正してください（参照箇所: PutStatus, req.Status, Valid(),
GetApplicationsでの検証と後続のIsAbleAccountManagerChangeStateとの違いを意識）。
---
Nitpick comments:
In `@internal/model/application_status.go`:
- Around line 41-53: convertEntApplicationStatusToModelApplicationStatus
currently dereferences applicationStatus.Edges.User.ID which can panic if
Edges.User is nil; update the function to defensively check if
applicationStatus.Edges != nil && applicationStatus.Edges.User != nil before
reading .ID and populate CreatedBy with a safe zero/nullable value when the user
edge is missing, while leaving the rest of the mapping (Status via
convertEntApplicationStatusToModelStatus, ID, CreatedAt) unchanged; make the
check inside convertEntApplicationStatusToModelApplicationStatus so all callers
(CreateStatus, GetApplication, UpdateApplication) become robust against ent
objects queried without WithUser().
In `@internal/model/application_test.go`:
- Around line 679-685: The second require.NoError(t, err) is redundant because
err was already checked after calling repo.CreateTag; remove the duplicate
assertion (the require.NoError that references err after creating the
ApplicationTarget) so the test only validates err once. Locate the
repo.CreateTag call (assigning tag, err) and the subsequent
service.ApplicationTarget construction and delete the extra require.NoError(t,
err) that follows it.
In `@internal/model/application.go`:
- Around line 301-307: 現在の
tx.Client().Application.UpdateOneID(applicationID).Save(ctx) 呼び出しはフィールド変更がなく実質
no-op になっているので、不要なら削除して RollbackWithError/ applicationErrorConverter.convert
を呼ぶパスを適切に整理してください；もし UpdatedAt
等の副作用（自動更新）を意図しているなら、その意図を明示するコメントを追加するか、意図したフィールド（例: UpdatedAt を明示的に更新するロジック）を
UpdateOneID(...).SetXxx(...) で行うようにし、不要な Save 呼び出しを残さないようにしてください。
In `@internal/model/comment.go`:
- Around line 108-116: The exported function ConvertEntCommentToModelComment
lacks a nil check on its comment parameter; update it to mirror
convertEntApplicationStatusToModelApplicationStatus by early-returning nil when
comment == nil, so callers get a nil *service.Comment instead of panicking; keep
the rest of the mapping (ID, User, Comment, CreatedAt, UpdatedAt) unchanged and
preserve the same signature.
In `@internal/model/file.go`:
- Around line 72-79: The ConvertEntFileToModelFile function currently assumes
entfile.Edges.User is loaded and will panic if nil; add an explicit nil-check on
entfile.Edges.User before accessing .ID (or otherwise validate entfile.Edges !=
nil), and handle the missing edge by setting service.File.CreatedBy to the zero
value (or an explicit empty string/ID) or return an error/alternate value per
your API contract; also update callers or document that
ConvertEntFileToModelFile requires the User edge to be loaded (e.g., ensure
queries use WithUser) so the intent is clear.
In `@internal/model/tag.go`:
- Around line 77-85: ConvertEntTagToModelTag lacks a nil guard and will panic if
enttag is nil; add an initial nil check in ConvertEntTagToModelTag (the exported
function) that returns nil when enttag == nil before accessing
enttag.ID/Name/DeletedAt, mirroring the pattern used in
convertEntApplicationStatusToModelApplicationStatus so callers receive a nil
*service.Tag instead of a runtime panic.
In `@internal/router/application.go`:
- Around line 525-533: The lambda passed to lo.Map is using parameter name `c`,
which shadows the outer `c *echo.Context` and is confusing; change the lambda
parameter in the lo.Map call over application.Comments to a non-conflicting name
(e.g., `cm` or `comment`) and update all uses inside the lambda that currently
reference `c` to the new name so the mapping still constructs &CommentDetail{ID:
..., User: ..., Comment: ..., CreatedAt: ..., UpdatedAt: ...} without shadowing
the outer context variable.
In `@internal/router/user_test.go`:
- Around line 22-33: Remove the redundant modelUserToUser function and
consolidate conversion logic into the existing userFromModelUser; either (A)
delete modelUserToUser and update any callers to call userFromModelUser
(adjusting call sites if they currently pass a pointer/value), or (B) change
userFromModelUser's signature to accept *service.User (or to return the same
pointer/value shape) so both call sites can use it—ensure the
ID/Name/DisplayName/AccountManager/CreatedAt/UpdatedAt/DeletedAt fields are
preserved in the unified conversion and update imports/usages accordingly.
In `@internal/service/account_manager.go`:
- Around line 10-12: AccountManager struct has a JSON tag on the ID field that
creates inconsistency with other service-layer types (Tag, Comment); remove the
`json:"id"` tag from the AccountManager type (the ID field in the AccountManager
struct) so service/domain models contain no serialization tags and leave JSON
marshaling to the router/handler response types, ensuring Tag and Comment remain
unchanged.
In `@internal/service/application.go`:
- Around line 25-30: The Application struct currently defines unused fields
(Amount, CreatedAt, UpdatedAt) while tests (internal/model/file_test.go) only
set ID and repository interfaces (ApplicationRepository returning
ApplicationResponse/ApplicationDetail/ApplicationQuery) never return
Application; either trim Application to only ID or remove the struct and update
tests to use uuid.UUID directly: update internal/model/file_test.go to stop
constructing Application and instead use uuid.UUID values, and verify
ApplicationRepository-related types (ApplicationResponse, ApplicationDetail,
ApplicationQuery) remain the single sources of truth; if other code relies on
Application, keep only required fields (ID) and remove/replace Amount,
CreatedAt, UpdatedAt to minimize dead code.
In `@internal/service/comment.go`:
- Around line 11-17: The Comment struct fields User and Comment are ambiguously
named; rename User to UserID and rename Comment to either Content or Body (pick
one consistent name) in the Comment struct declaration, then update all
references (constructors, methods, serializers/deserializers, DB mappers, JSON
tags, tests, and any code using comment.User or comment.Comment) to use the new
names (e.g., Comment.UserID and Comment.Content/Body) to preserve behavior and
compilation; ensure any ORM or JSON tags are adjusted to maintain the same
persisted/serialized keys if required.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
internal/model/account_manager.gointernal/model/account_manager_impl.gointernal/model/account_manager_test.gointernal/model/application.gointernal/model/application_impl.gointernal/model/application_status.gointernal/model/application_status_impl.gointernal/model/application_status_test.gointernal/model/application_target.gointernal/model/application_target_impl.gointernal/model/application_target_test.gointernal/model/application_test.gointernal/model/comment.gointernal/model/comment_impl.gointernal/model/comment_test.gointernal/model/file.gointernal/model/file_impl.gointernal/model/file_test.gointernal/model/repository.gointernal/model/tag.gointernal/model/tag_impl.gointernal/model/tag_test.gointernal/model/user.gointernal/model/user_impl.gointernal/model/user_test.gointernal/router/account_manager.gointernal/router/account_manager_test.gointernal/router/application.gointernal/router/application_test.gointernal/router/auth.gointernal/router/file_test.gointernal/router/router.gointernal/router/router_test.gointernal/router/tag.gointernal/router/tag_test.gointernal/router/user.gointernal/router/user_test.gointernal/service/account_manager.gointernal/service/application.gointernal/service/application_file.gointernal/service/application_status.gointernal/service/application_tag.gointernal/service/application_target.gointernal/service/comment.gointernal/service/file.gointernal/service/mock_service/mock_account_manager.gointernal/service/mock_service/mock_application.gointernal/service/mock_service/mock_application_file.gointernal/service/mock_service/mock_application_status.gointernal/service/mock_service/mock_application_tag.gointernal/service/mock_service/mock_application_target.gointernal/service/mock_service/mock_comment.gointernal/service/mock_service/mock_file.gointernal/service/mock_service/mock_tag.gointernal/service/mock_service/mock_user.gointernal/service/repository.gointernal/service/tag.gointernal/service/user.go
💤 Files with no reviewable changes (9)
- internal/model/repository.go
- internal/model/application_status_impl.go
- internal/model/account_manager_impl.go
- internal/model/tag_impl.go
- internal/model/application_impl.go
- internal/model/user_impl.go
- internal/model/comment_impl.go
- internal/model/file_impl.go
- internal/model/application_target_impl.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v2 #1011 +/- ##
========================================
- Coverage 7.28% 7.24% -0.04%
========================================
Files 118 118
Lines 16924 16946 +22
========================================
- Hits 1233 1228 -5
- Misses 15609 15637 +28
+ Partials 82 81 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
internal/model/application.go (3)
125-128:⚠️ Potential issue | 🟠 Major
r.Edges.Status[0]がステータスのないアプリケーションで index out of range パニックを起こすLine 127 で
r.Edges.Status[0]に直接アクセスしていますが、ステータスが存在しない場合（データ不整合など）にパニックします。同様の問題がGetApplicationの Line 261 とUpdateApplicationの Line 321 にもあります。🐛 修正案
reqres := lo.Map(applications, func(r *ent.Application, _ int) *service.ApplicationResponse { + if len(r.Edges.Status) == 0 { + return nil + } return convertEntApplicationResponseToModelApplicationResponse( r, r.Edges.Tag, r.Edges.Status[0], r.Edges.User) }) + reqres = lo.Filter(reqres, func(r *service.ApplicationResponse, _ int) bool { + return r != nil + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/model/application.go` around lines 125 - 128, The code directly indexes r.Edges.Status[0] which panics when Status is empty; update all call sites (the lo.Map mapping that calls convertEntApplicationResponseToModelApplicationResponse, GetApplication, and UpdateApplication) to first check len(r.Edges.Status) > 0 and pass either r.Edges.Status[0] or nil/a sensible zero-value to convertEntApplicationResponseToModelApplicationResponse (or adjust that function to accept a nil/optional status), ensuring safe handling when no status exists to prevent index-out-of-range panics.
346-362:⚠️ Potential issue | 🟠 Major
UpdateApplicationでコメント・ファイル取得エラー時にトランザクションがロールバックされないLine 349-351（コメント取得）と Line 359-362（ファイル取得）でエラーが発生した場合、
RollbackWithErrorを呼ばずにエラーを返しています。他のエラーハンドリングと一貫性を持たせるべきです。🐛 修正案
entcomments, err := updated.QueryComment(). WithUser(). All(ctx) if err != nil { + err = RollbackWithError(tx, err) return nil, applicationErrorConverter.convert(err) }entfiles, err := updated.QueryFile().All(ctx) if err != nil { + err = RollbackWithError(tx, err) return nil, applicationErrorConverter.convert(err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/model/application.go` around lines 346 - 362, In UpdateApplication, when updated.QueryComment().WithUser().All(ctx) or updated.QueryFile().All(ctx) returns an error you must roll back the transaction before returning; change the error paths for the comment and file queries to call tx.RollbackWithError(err) (or the existing transaction rollback helper used elsewhere) and then return applicationErrorConverter.convert(err), mirroring other error handling in UpdateApplication so the transaction is consistently rolled back on these failures (refer to updated.QueryComment, updated.QueryFile, UpdateApplication and RollbackWithError/tx).
231-249:⚠️ Potential issue | 🔴 Critical
GetApplicationでコメントのUserエッジがロードされておらず、nil ポインタパニックが発生するLine 232 の
WithComment()はコメントエンティティのみをロードし、コメントに紐づく User エッジはロードしません。しかし Line 248 でc.Edges.User.IDにアクセスしており、Edges.Userが nil のためパニックします。
UpdateApplication（Line 346-348）ではQueryComment().WithUser()で正しくロードしています。🐛 修正案
WithUser(). - WithComment(). + WithComment(func(q *ent.CommentQuery) { + q.WithUser() + }). WithFile().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/model/application.go` around lines 231 - 249, GetApplication currently calls WithComment() which only loads comment entities but not their User edge, causing a nil pointer when ConvertEntCommentToModelComment reads c.Edges.User.ID; update the ent query in GetApplication to load the comment's user (e.g., replace WithComment() with a nested load like QueryComment().WithUser() or use WithComment(func(q *ent.CommentQuery) { q.WithUser() }) so that c.Edges.User is populated before calling ConvertEntCommentToModelComment).
🧹 Nitpick comments (3)
internal/model/application.go (2)
305-311: 冗長な更新操作Lines 305-311 の更新は Lines 293-299 で既に行われた更新と重複しており、実質的に何も変更しません（
SetTitle、SetContent、タグの操作なし）。このコードブロックは不要と思われます。♻️ 削除提案
- _, err = tx.Client().Application. - UpdateOneID(applicationID). - Save(ctx) - if err != nil { - err = RollbackWithError(tx, err) - return nil, applicationErrorConverter.convert(err) - } -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/model/application.go` around lines 305 - 311, このブロックは既に Lines 293-299 で同じ更新を行っているため冗長です：tx.Client().Application.UpdateOneID(applicationID).Save(ctx) とそのエラーハンドリング（RollbackWithError と applicationErrorConverter.convert）を削除して、重複する更新を取り除いてください。削除後にトランザクションの整合性とエラーパスが残ること（RollbackWithError 呼び出しや他の必要なエラーチェックが消えないこと）を確認し、必要なら既存の更新ブロックに対するテストが通るかを検証してください。
388-408:statusおよびuserパラメータの nil チェックがない
applicationに対しては nil チェックがありますが、statusとuserパラメータには nil チェックがありません。Line 400 と Line 403 で nil ポインタパニックが発生する可能性があります。🛡️ 防御的なコード例
func convertEntApplicationResponseToModelApplicationResponse( application *ent.Application, tags []*ent.Tag, status *ent.ApplicationStatus, user *ent.User, ) *service.ApplicationResponse { - if application == nil { + if application == nil || status == nil || user == nil { return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/model/application.go` around lines 388 - 408, In convertEntApplicationResponseToModelApplicationResponse add defensive nil checks for the status and user params before referencing status.Status and user.ID: if status or user is nil, either return nil early or populate the ApplicationResponse with safe defaults (e.g., empty/unknown Status via convertEntApplicationStatusToModelStatus or zero value for CreatedBy) so you avoid nil pointer panics when calling convertEntApplicationStatusToModelStatus(&status.Status) and accessing user.ID.internal/model/application_status.go (1)
42-54:Edges.Userが nil の場合にパニックする可能性
convertEntApplicationStatusToModelApplicationStatusはapplicationStatus.Edges.User.IDに直接アクセスしていますが、WithUser()でロードされていない場合や、User エッジが存在しない場合に nil ポインタパニックが発生します。現状の呼び出し元（
CreateStatusLine 34、GetApplicationLine 229）ではWithUser()が呼ばれているため問題ありませんが、将来的な呼び出しでの安全性を考慮すると、nil チェックの追加を検討してください。🛡️ 防御的なコード例
func convertEntApplicationStatusToModelApplicationStatus( applicationStatus *ent.ApplicationStatus, ) *service.ApplicationStatus { if applicationStatus == nil { return nil } + var createdBy uuid.UUID + if applicationStatus.Edges.User != nil { + createdBy = applicationStatus.Edges.User.ID + } return &service.ApplicationStatus{ ID: applicationStatus.ID, - CreatedBy: applicationStatus.Edges.User.ID, + CreatedBy: createdBy, Status: convertEntApplicationStatusToModelStatus(&applicationStatus.Status), CreatedAt: applicationStatus.CreatedAt, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/model/application_status.go` around lines 42 - 54, convertEntApplicationStatusToModelApplicationStatus dereferences applicationStatus.Edges.User.ID and can panic if the User edge wasn't loaded; add a defensive nil-check for applicationStatus.Edges and applicationStatus.Edges.User and set CreatedBy to a safe zero/empty value (or nil equivalent) when missing. Update the function convertEntApplicationStatusToModelApplicationStatus to guard access to Edges.User.ID and ensure callers like CreateStatus and GetApplication still work when WithUser() is present but the function also remains safe for future callers that may not load the User edge. Keep the rest of the mapping (Status, ID, CreatedAt) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/model/application.go`:
- Line 321: The code currently indexes entstatuses[0] into variable status which
will panic if entstatuses is empty; update the logic around entstatuses (the
slice and the assignment to status) to check len(entstatuses) == 0 before
accessing index 0 and handle that case appropriately (e.g., return an error,
nil, or set a sensible default), ensuring callers of the surrounding function
handle the new error/empty-case path.
---
Duplicate comments:
In `@internal/model/application.go`:
- Around line 125-128: The code directly indexes r.Edges.Status[0] which panics
when Status is empty; update all call sites (the lo.Map mapping that calls
convertEntApplicationResponseToModelApplicationResponse, GetApplication, and
UpdateApplication) to first check len(r.Edges.Status) > 0 and pass either
r.Edges.Status[0] or nil/a sensible zero-value to
convertEntApplicationResponseToModelApplicationResponse (or adjust that function
to accept a nil/optional status), ensuring safe handling when no status exists
to prevent index-out-of-range panics.
- Around line 346-362: In UpdateApplication, when
updated.QueryComment().WithUser().All(ctx) or updated.QueryFile().All(ctx)
returns an error you must roll back the transaction before returning; change the
error paths for the comment and file queries to call tx.RollbackWithError(err)
(or the existing transaction rollback helper used elsewhere) and then return
applicationErrorConverter.convert(err), mirroring other error handling in
UpdateApplication so the transaction is consistently rolled back on these
failures (refer to updated.QueryComment, updated.QueryFile, UpdateApplication
and RollbackWithError/tx).
- Around line 231-249: GetApplication currently calls WithComment() which only
loads comment entities but not their User edge, causing a nil pointer when
ConvertEntCommentToModelComment reads c.Edges.User.ID; update the ent query in
GetApplication to load the comment's user (e.g., replace WithComment() with a
nested load like QueryComment().WithUser() or use WithComment(func(q
*ent.CommentQuery) { q.WithUser() }) so that c.Edges.User is populated before
calling ConvertEntCommentToModelComment).
---
Nitpick comments:
In `@internal/model/application_status.go`:
- Around line 42-54: convertEntApplicationStatusToModelApplicationStatus
dereferences applicationStatus.Edges.User.ID and can panic if the User edge
wasn't loaded; add a defensive nil-check for applicationStatus.Edges and
applicationStatus.Edges.User and set CreatedBy to a safe zero/empty value (or
nil equivalent) when missing. Update the function
convertEntApplicationStatusToModelApplicationStatus to guard access to
Edges.User.ID and ensure callers like CreateStatus and GetApplication still work
when WithUser() is present but the function also remains safe for future callers
that may not load the User edge. Keep the rest of the mapping (Status, ID,
CreatedAt) unchanged.
In `@internal/model/application.go`:
- Around line 305-311: このブロックは既に Lines 293-299
で同じ更新を行っているため冗長です：tx.Client().Application.UpdateOneID(applicationID).Save(ctx)
とそのエラーハンドリング（RollbackWithError と
applicationErrorConverter.convert）を削除して、重複する更新を取り除いてください。削除後にトランザクションの整合性とエラーパスが残ること（RollbackWithError
呼び出しや他の必要なエラーチェックが消えないこと）を確認し、必要なら既存の更新ブロックに対するテストが通るかを検証してください。
- Around line 388-408: In
convertEntApplicationResponseToModelApplicationResponse add defensive nil checks
for the status and user params before referencing status.Status and user.ID: if
status or user is nil, either return nil early or populate the
ApplicationResponse with safe defaults (e.g., empty/unknown Status via
convertEntApplicationStatusToModelStatus or zero value for CreatedBy) so you
avoid nil pointer panics when calling
convertEntApplicationStatusToModelStatus(&status.Status) and accessing user.ID.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/storage/local.go (1)
40-45:⚠️ Potential issue | 🟡 Minor
os.Openのエラーハンドリングを改善してください現在、
os.OpenからのすべてのエラーがErrFileNotFoundとして返されていますが、これはパーミッションエラー（os.ErrPermission）などの他のエラーを隠してしまう可能性があります。os.ErrNotExistのみをErrFileNotFoundに変換することを検討してください。🛠️ 提案される修正
func (l *Local) Open(_ context.Context, filename string) (io.ReadCloser, error) { r, err := os.Open(l.getFilePath(filename)) if err != nil { + if errors.Is(err, os.ErrNotExist) { + return nil, ErrFileNotFound + } - return nil, ErrFileNotFound + return nil, err } return r, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/local.go` around lines 40 - 45, In Local.Open, don't map every os.Open error to ErrFileNotFound; detect existence errors using os.IsNotExist(err) (or compare to os.ErrNotExist) and only return ErrFileNotFound for that case, otherwise return the original err (so permission and other errors are preserved); update the Open method (function Local.Open) to check the error from os.Open and conditionally wrap/translate the error instead of unconditionally returning ErrFileNotFound.internal/router/router_test.go (1)
77-88:⚠️ Potential issue | 🟡 MinormakeUserヘルパー関数にDeletedAtフィールドの初期化が不足しています。
service.User型への移行は正しく行われていますが、関数内でDeletedAtフィールドが初期化されていません。service.User構造体の定義（internal/service/user.go:19）ではDeletedAtフィールドが存在するため、以下のように明示的に初期化してください：return &service.User{ ID: uuid.New(), Name: random.AlphaNumeric(t, 20), DisplayName: random.AlphaNumeric(t, 20), AccountManager: accountManager, CreatedAt: date, UpdatedAt: date, DeletedAt: nulltime.NullTime{}, // またはテストケースに応じた適切な値 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/router/router_test.go` around lines 77 - 88, The helper makeUser currently constructs a *service.User but omits initializing the DeletedAt field; update the return value in makeUser to include DeletedAt (e.g., set DeletedAt to nulltime.NullTime{} or another appropriate zero/nullable value for tests) so the created *service.User fully matches the service.User struct (ensure the change is made in function makeUser and references the DeletedAt field on service.User).
♻️ Duplicate comments (2)
internal/model/file.go (1)
17-40:⚠️ Potential issue | 🟠 MajorCreateFile の2段階更新が未だ非原子的です。
Line 23〜Line 37 で
File作成とApplication更新が分離されており、後段失敗時に孤立データが残ります。TODO が残っているため、ent.Txで同一トランザクションにまとめてください。🛠️ 修正案（トランザクション化）
func (repo *EntRepository) CreateFile( ctx context.Context, name, mimetype string, applicationID, userID uuid.UUID, ) (*service.File, error) { + tx, err := repo.client.Tx(ctx) + if err != nil { + return nil, err + } + defer func() { _ = tx.Rollback() }() + id := uuid.New() - created, err := repo.client.File. + created, err := tx.File. Create(). SetID(id). SetName(name). SetMimeType(mimetype). SetUserID(userID). Save(ctx) if err != nil { return nil, fileErrorConverter.convert(err) } - _, err = repo.client.Application. + _, err = tx.Application. UpdateOneID(applicationID). AddFile(created). Save(ctx) if err != nil { return nil, fileErrorConverter.convert(err) } + + if err := tx.Commit(); err != nil { + return nil, fileErrorConverter.convert(err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/model/file.go` around lines 17 - 40, The CreateFile implementation does two separate DB operations (repo.client.File.Create...Save and repo.client.Application.UpdateOneID...AddFile...Save) and must be wrapped in a single ent transaction to avoid orphaned files; open an ent.Tx (repo.client.Tx(ctx)), run the file creation and application update against the transaction client (use tx.File.Create...Save and tx.Application.UpdateOneID(...).AddFile(...).Save), roll back the tx and convert/return the error with fileErrorConverter.convert if any operation fails, and commit the tx at the end; keep references to EntRepository.CreateFile, repo.client (use tx instead), and fileErrorConverter.convert when locating code to change.internal/model/application.go (1)
30-48:⚠️ Potential issue | 🟠 Major
created_atと-created_atのソート方向が慣習と逆転しています実装では
created_atが降順、-created_atが昇順になっており、同ファイル内のtitle/-titleの実装（title= 昇順、-title= 降順）と矛盾しています。標準的な慣習では-プレフィックスが降順を意味するため、このコードはAPI利用者の期待を裏切ります。修正案
- } else if *query.Sort == "-created_at" { + } else if *query.Sort == "-created_at" { applicationsq = repo.client.Application. Query(). WithTag(). WithStatus(func(q *ent.ApplicationStatusQuery) { q.Order(ent.Desc(applicationstatus.FieldCreatedAt)) }). WithUser(). - Order(ent.Asc(application.FieldCreatedAt)) + Order(ent.Desc(application.FieldCreatedAt))- if query.Sort == nil || *query.Sort == "" || *query.Sort == "created_at" { + if query.Sort == nil || *query.Sort == "" || *query.Sort == "created_at" { applicationsq = repo.client.Application. Query(). WithTag(). WithStatus(func(q *ent.ApplicationStatusQuery) { q.Order(ent.Desc(applicationstatus.FieldCreatedAt)) }). WithUser(). - Order(ent.Desc(application.FieldCreatedAt)) + Order(ent.Asc(application.FieldCreatedAt))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/model/application.go` around lines 30 - 48, The created_at sort directions are reversed: in the query handling for query.Sort (nil/""/"created_at" and "-created_at") swap the Order directions so that "created_at" uses Order(ent.Asc(application.FieldCreatedAt)) and the corresponding status order uses q.Order(ent.Asc(applicationstatus.FieldCreatedAt)), while "-created_at" uses Order(ent.Desc(application.FieldCreatedAt)) and q.Order(ent.Desc(applicationstatus.FieldCreatedAt)); update the branches that set applicationsq (the repo.client.Application.Query() blocks that call WithStatus and Order) accordingly so behavior matches the title/-title convention.
🧹 Nitpick comments (2)
internal/storage/s3.go (1)
38-39: FIXME:filenameに引用符が含まれる場合の問題
ContentDispositionヘッダーでfilenameをそのまま埋め込んでいますが、ファイル名に"が含まれる場合、ヘッダーインジェクションやパースエラーの原因になります。RFC 5987 に準拠したエンコーディング、または"のエスケープ処理を検討してください。この修正を実装するコードを生成しましょうか？
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/s3.go` around lines 38 - 39, The ContentDisposition header is built by directly interpolating filename, which breaks if filename contains quotes or non-ASCII characters; replace the raw fmt.Sprintf(...) usage for ContentDisposition with a safe encoder: use mime.FormatMediaType("attachment", map[string]string{"filename": filename}) (which will quote/escape per RFC rules) or, if you must support RFC5987 for non-ASCII, build an encoded filename* parameter (e.g. filename*=UTF-8''<url-encoded-filename>) and set ContentDisposition to that value; update the code that sets ContentDisposition to use mime.FormatMediaType or the RFC5987-encoded string instead of the current fmt.Sprintf, referencing ContentDisposition and filename in the s3 upload code.internal/model/file.go (1)
73-80:ConvertEntFileToModelFileの User edge 依存を削除して堅牢性を向上させてください。現在の呼び出し元（
GetFileメソッド）では.WithUser()が明示的に使われているため安全ですが、この public 関数は他のパッケージから呼び出される可能性があります。line 79 のentfile.Edges.User.IDはWithUser()なしで呼ばれると nil 参照になります。代わりに private フィールドfile_userを活用するか、nil ガードを追加してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/model/file.go` around lines 73 - 80, The ConvertEntFileToModelFile function currently dereferences entfile.Edges.User.ID which can nil-panic when WithUser() isn't used; update ConvertEntFileToModelFile to guard against a nil entfile.Edges.User (e.g., if entfile.Edges.User != nil use entfile.Edges.User.ID) and otherwise fall back to the persisted user reference (the generated private field `file_user` / equivalent on ent.File) when populating service.File.CreatedBy so the function is safe to call from other packages; ensure you only change ConvertEntFileToModelFile and keep service.File population logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/router/router_test.go`:
- Around line 77-88: The helper makeUser currently constructs a *service.User
but omits initializing the DeletedAt field; update the return value in makeUser
to include DeletedAt (e.g., set DeletedAt to nulltime.NullTime{} or another
appropriate zero/nullable value for tests) so the created *service.User fully
matches the service.User struct (ensure the change is made in function makeUser
and references the DeletedAt field on service.User).
In `@internal/storage/local.go`:
- Around line 40-45: In Local.Open, don't map every os.Open error to
ErrFileNotFound; detect existence errors using os.IsNotExist(err) (or compare to
os.ErrNotExist) and only return ErrFileNotFound for that case, otherwise return
the original err (so permission and other errors are preserved); update the Open
method (function Local.Open) to check the error from os.Open and conditionally
wrap/translate the error instead of unconditionally returning ErrFileNotFound.
---
Duplicate comments:
In `@internal/model/application.go`:
- Around line 30-48: The created_at sort directions are reversed: in the query
handling for query.Sort (nil/""/"created_at" and "-created_at") swap the Order
directions so that "created_at" uses Order(ent.Asc(application.FieldCreatedAt))
and the corresponding status order uses
q.Order(ent.Asc(applicationstatus.FieldCreatedAt)), while "-created_at" uses
Order(ent.Desc(application.FieldCreatedAt)) and
q.Order(ent.Desc(applicationstatus.FieldCreatedAt)); update the branches that
set applicationsq (the repo.client.Application.Query() blocks that call
WithStatus and Order) accordingly so behavior matches the title/-title
convention.
In `@internal/model/file.go`:
- Around line 17-40: The CreateFile implementation does two separate DB
operations (repo.client.File.Create...Save and
repo.client.Application.UpdateOneID...AddFile...Save) and must be wrapped in a
single ent transaction to avoid orphaned files; open an ent.Tx
(repo.client.Tx(ctx)), run the file creation and application update against the
transaction client (use tx.File.Create...Save and
tx.Application.UpdateOneID(...).AddFile(...).Save), roll back the tx and
convert/return the error with fileErrorConverter.convert if any operation fails,
and commit the tx at the end; keep references to EntRepository.CreateFile,
repo.client (use tx instead), and fileErrorConverter.convert when locating code
to change.
---
Nitpick comments:
In `@internal/model/file.go`:
- Around line 73-80: The ConvertEntFileToModelFile function currently
dereferences entfile.Edges.User.ID which can nil-panic when WithUser() isn't
used; update ConvertEntFileToModelFile to guard against a nil entfile.Edges.User
(e.g., if entfile.Edges.User != nil use entfile.Edges.User.ID) and otherwise
fall back to the persisted user reference (the generated private field
`file_user` / equivalent on ent.File) when populating service.File.CreatedBy so
the function is safe to call from other packages; ensure you only change
ConvertEntFileToModelFile and keep service.File population logic intact.
In `@internal/storage/s3.go`:
- Around line 38-39: The ContentDisposition header is built by directly
interpolating filename, which breaks if filename contains quotes or non-ASCII
characters; replace the raw fmt.Sprintf(...) usage for ContentDisposition with a
safe encoder: use mime.FormatMediaType("attachment",
map[string]string{"filename": filename}) (which will quote/escape per RFC rules)
or, if you must support RFC5987 for non-ASCII, build an encoded filename*
parameter (e.g. filename*=UTF-8''<url-encoded-filename>) and set
ContentDisposition to that value; update the code that sets ContentDisposition
to use mime.FormatMediaType or the RFC5987-encoded string instead of the current
fmt.Sprintf, referencing ContentDisposition and filename in the s3 upload code.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
internal/model/application.gointernal/model/file.gointernal/router/router.gointernal/router/router_test.gointernal/service/application_status.gointernal/service/mock_service/mock_storage.gointernal/service/storage.gointernal/storage/error.gointernal/storage/local.gointernal/storage/s3.gointernal/storage/swift.gomain.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/service/application_status.go
- internal/router/router.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #997
Summary by CodeRabbit
リリースノート