Open
Conversation
user hooksの処理をdb.SaveUserに移動 user errors WIP room
📝 WalkthroughWalkthroughWriteEventParams／WriteRoomParams に TimeConsistency() を追加。インフラ層でUUID生成・関連エンティティの個別永続化・DBフック多くを無効化し、トークン保存前に暗号化する処理を追加。サービス層で時間整合性・Room未定義の検証と新エラー群を導入しました。 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EventSvc as EventService
participant RoomSvc as RoomService
participant DB as Database
Client->>EventSvc: CreateEvent(params)
EventSvc->>EventSvc: params.TimeConsistency() を検証
alt TimeConsistency 不正
EventSvc-->>Client: ErrTimeConsistency
else
alt params.RoomID == uuid.Nil
EventSvc->>RoomSvc: CreateUnVerifiedRoom(Place)
RoomSvc->>DB: Save Room (UUID 生成)
DB-->>RoomSvc: Room(ID)
RoomSvc-->>EventSvc: Room ID
EventSvc->>EventSvc: params に RoomID を設定
end
EventSvc->>DB: Save Event（event.ID UUID生成）
DB-->>EventSvc: Event 作成完了
EventSvc->>DB: 個別に Tags/Admins/Attendees を create/save
DB-->>EventSvc: 各関連保存完了
EventSvc-->>Client: Success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
iChemy
reviewed
Feb 14, 2026
service/room_impl.go
Outdated
Comment on lines
+47
to
+50
| // そもそもNilだとIsRoomAdminsで弾かれるはず | ||
| // if roomID == uuid.Nil { | ||
| // return nil,ErrRoomUndefined | ||
| // } |
Contributor
There was a problem hiding this comment.
IsRoomAdmins の前に持っていくのが良さそう．
iChemy
reviewed
Feb 14, 2026
Contributor
iChemy
left a comment
iChemy
left a comment
There was a problem hiding this comment.
(もしかしたらまだ見落としがあるかもしれないけど)適当にコメントしてみました．
コード内のコメントアウトの内容は大体同意という感じです．
(がどこか別のプルリクでより適切な形に変更していきたい)
hooks の削除に際して，必ずしも削除される hooks の動作を忠実に再現する必要はないと思っています(User 系の hooks とか)．
infra/db/user.go
Outdated
Comment on lines
+121
to
+124
| // BeforeCreate が呼ばれる | ||
| if user.ID != uuid.Nil { | ||
| // 単純にIDが違う場合 | ||
| return ErrRecordNotFound |
Contributor
There was a problem hiding this comment.
User の BeforeCreate を再現したがっていると思われますが，その場合 user.ID が uuid.Nil ならエラーにならず user 変数の内容をそのまま Create するのが正しい動作な気がします(というか現状 traQ ユーザーしか knoQ を使えず， traQ ユーザーの uuid をそのまま knoQ の User の ID に利用しているので， user.ID が uuid.Nil になることはないはず...)
infra/db/user.go
Outdated
Comment on lines
+127
to
+130
| user.ID ,err = uuid.NewV4() | ||
| if err != nil { | ||
| return err | ||
| } |
Contributor
There was a problem hiding this comment.
上で述べた通り，knoQ は traQ ユーザーのみを現状受け付けているので，knoQ 独自にユーザーのIDを作る必要もないのかなと．
(多分，knoQを作成していた当初は traQ のアカウントと紐づけなくても使えるようにしようとしていたのだろうけど，今はその予定もないので...)
service/event_impl.go
Outdated
Comment on lines
+53
to
+71
| // RoomIDの存在を確認 RoomがなくPlaceがあれば新たに作成 | ||
|
|
||
| // if params.RoomID == uuid.Nil { | ||
| // if params.Place == "" { | ||
| // roomParams := domain.WriteRoomParams { | ||
| // Place: params.Place, | ||
| // TimeStart: params.TimeStart, | ||
| // TimeEnd: params.TimeEnd, | ||
| // Admins: params.Admins, | ||
| // } | ||
| // // UnVerifiedを仮定 | ||
| // _,err = s.CreateUnVerifiedRoom(ctx,reqID,roomParams) | ||
| // if err != nil { | ||
| // return nil,err | ||
| // } | ||
| // } else{ | ||
| // return nil, ErrRoomUndefined | ||
| // } | ||
| // } |
Contributor
There was a problem hiding this comment.
現状これがなくても Event の hooks のおかげで動作しますが，合っても正しく動作する(Room の ID を p に渡す必要があるが)と思うので，実装してもいい気がしています．
それこそ，RoomID が Nil のとき，どのように部屋が扱われるかは service 層が担うべき部分かと思うので
Contributor
|
あまり関係ないけど，非進捗部屋開催のイベントの更新の際に毎回非進捗部屋を作成して更新するのやめたいとも思ったりしている． |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
service/room_impl.go (3)
30-33: フォーマットの修正Go の標準スタイルに従うため、
ifの後と括弧内のスペースを修正してください。📝 フォーマット修正
// 時間整合性の確認 - if(!params.TimeStart.Before(params.TimeEnd)){ - return nil,ErrTimeConsistency + if !params.TimeStart.Before(params.TimeEnd) { + return nil, ErrTimeConsistency }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/room_impl.go` around lines 30 - 33, Fix the Go formatting for the time-consistency check: remove the parentheses around the condition and add a space after the if so it reads like "if !params.TimeStart.Before(params.TimeEnd) { ... }"; apply this to the block that returns ErrTimeConsistency (referring to params.TimeStart.Before(params.TimeEnd) and ErrTimeConsistency) to match Go standard style.
57-60: フォーマットの修正Line 58-59 も同様にフォーマットを修正してください。
📝 フォーマット修正
// 時間整合性の確認 - if(!params.TimeStart.Before(params.TimeEnd)){ - return nil,ErrTimeConsistency + if !params.TimeStart.Before(params.TimeEnd) { + return nil, ErrTimeConsistency }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/room_impl.go` around lines 57 - 60, The if-statement checking time consistency is misformatted; update the conditional to follow Go formatting conventions by adding a space after the if keyword, spacing around the negation and before the opening brace, and spacing after the comma in the return; specifically adjust the line using params.TimeStart.Before(params.TimeEnd) and the return that yields ErrTimeConsistency so it becomes: if !params.TimeStart.Before(params.TimeEnd) { return nil, ErrTimeConsistency }.
12-15: 時間整合性チェックの重複とフォーマットの問題時間整合性の検証が
CreateUnVerifiedRoom、CreateVerifiedRoom、UpdateRoomの3箇所で重複しています。また、Go の標準フォーマットに従っていない箇所があります。♻️ ヘルパー関数への抽出とフォーマット修正の提案
ヘルパー関数を追加:
func validateTimeConsistency(start, end time.Time) error { if !start.Before(end) { return ErrTimeConsistency } return nil }各関数での使用例:
func (s *service) CreateUnVerifiedRoom(ctx context.Context, reqID uuid.UUID, params domain.WriteRoomParams) (*domain.Room, error) { - // 時間整合性の確認 - if(!params.TimeStart.Before(params.TimeEnd)){ - return nil,ErrTimeConsistency - } + if err := validateTimeConsistency(params.TimeStart, params.TimeEnd); err != nil { + return nil, err + } p := domain.CreateRoomArgs{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/room_impl.go` around lines 12 - 15, Extract the duplicated time-order check into a helper validateTimeConsistency(start, end time.Time) error that returns ErrTimeConsistency when !start.Before(end), and replace the inline checks in CreateUnVerifiedRoom, CreateVerifiedRoom, and UpdateRoom (which currently reference params.TimeStart and params.TimeEnd) with calls to validateTimeConsistency(params.TimeStart, params.TimeEnd); also ensure the helper and call sites use Go's time.Time methods and formatting conventions (no custom format logic) and import time where needed.infra/db/user.go (1)
121-132: コメントの意図が不明瞭Line 123 のコメント「単純にIDが違う場合」は、実際のロジックと一致していません。このブランチは「指定されたIDのユーザーが存在しない場合」に ErrRecordNotFound を返す処理です。コメントを修正することで、意図がより明確になります。
また、Line 127 のフォーマットに軽微な問題があります（カンマ後のスペース欠如）。
📝 コメントとフォーマットの修正案
if errors.Is(err, gorm.ErrRecordNotFound) { // BeforeCreate が呼ばれる - if user.ID != uuid.Nil { - // 単純にIDが違う場合 + if user.ID != uuid.Nil { + // 指定されたIDのユーザーが存在しない場合 return ErrRecordNotFound } else { // IDなしで新たに作成する場合 - user.ID ,err = uuid.NewV4() + user.ID, err = uuid.NewV4() if err != nil { return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/user.go` around lines 121 - 132, The comment on the branch that returns ErrRecordNotFound is misleading: update the comment near the ErrRecordNotFound return to state that this branch handles "when the specified user ID does not correspond to an existing user" (or similar clearer wording) so it matches the logic, and fix the minor formatting bug in the BeforeCreate code by adding a space after the comma in the assignment to user.ID (change user.ID ,err = uuid.NewV4() to user.ID, err = uuid.NewV4()); keep the rest of the logic (ErrRecordNotFound, uuid.NewV4(), tx.Create(&user).Error) 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 `@infra/db/room.go`:
- Around line 95-101: The comment saying "BeforeSave, BeforeUpdate が発火" is
incorrect for the Room model—remove or correct that comment; then fix the
admins-not-being-saved bug by ensuring RoomAdmin associations created by
ConvUpdateRoomParamsToRoom are persisted: after deleting existing admins with
db.Where("room_id", room.ID).Delete(&RoomAdmin{}), call Save on the room using a
GORM session with FullSaveAssociations enabled (e.g., use
db.Session(&gorm.Session{FullSaveAssociations:
true}).Omit("verified","CreatedAt").Save(&room)) so new room.Admins are saved;
keep the omit fields and existing deletion logic intact.
In `@service/errors.go`:
- Around line 12-19: The service returns ErrTimeConsistency directly from
functions in room_impl.go (references: ErrTimeConsistency) which bypasses the
global mapping; change those direct returns to wrap the error with
defaultErrorHandling (e.g., return nil,
defaultErrorHandling(ErrTimeConsistency)) so they go through the same mapping
path, or alternatively update handleDBError to also map
service.ErrTimeConsistency to the appropriate domain error; adjust every
occurrence in room_impl.go where ErrTimeConsistency is returned (the three
validation sites) to use defaultErrorHandling unless you choose to extend
handleDBError instead.
In `@service/event_impl.go`:
- Around line 53-73: コメント内の条件が逆になっているため、将来このブロックを有効化する際は条件を反転させてください:
params.RoomID が uuid.Nil の場合に、Place が空であれば ErrRoomUndefined を返し、Place が指定されていれば
domain.WriteRoomParams を組み立てて s.CreateUnVerifiedRoom(ctx, reqID, roomParams)
を呼び出して作成処理を行うように修正してください。関連する識別子: params.RoomID, params.Place,
domain.WriteRoomParams, s.CreateUnVerifiedRoom, ErrRoomUndefined.
---
Nitpick comments:
In `@infra/db/user.go`:
- Around line 121-132: The comment on the branch that returns ErrRecordNotFound
is misleading: update the comment near the ErrRecordNotFound return to state
that this branch handles "when the specified user ID does not correspond to an
existing user" (or similar clearer wording) so it matches the logic, and fix the
minor formatting bug in the BeforeCreate code by adding a space after the comma
in the assignment to user.ID (change user.ID ,err = uuid.NewV4() to user.ID, err
= uuid.NewV4()); keep the rest of the logic (ErrRecordNotFound, uuid.NewV4(),
tx.Create(&user).Error) unchanged.
In `@service/room_impl.go`:
- Around line 30-33: Fix the Go formatting for the time-consistency check:
remove the parentheses around the condition and add a space after the if so it
reads like "if !params.TimeStart.Before(params.TimeEnd) { ... }"; apply this to
the block that returns ErrTimeConsistency (referring to
params.TimeStart.Before(params.TimeEnd) and ErrTimeConsistency) to match Go
standard style.
- Around line 57-60: The if-statement checking time consistency is misformatted;
update the conditional to follow Go formatting conventions by adding a space
after the if keyword, spacing around the negation and before the opening brace,
and spacing after the comma in the return; specifically adjust the line using
params.TimeStart.Before(params.TimeEnd) and the return that yields
ErrTimeConsistency so it becomes: if !params.TimeStart.Before(params.TimeEnd) {
return nil, ErrTimeConsistency }.
- Around line 12-15: Extract the duplicated time-order check into a helper
validateTimeConsistency(start, end time.Time) error that returns
ErrTimeConsistency when !start.Before(end), and replace the inline checks in
CreateUnVerifiedRoom, CreateVerifiedRoom, and UpdateRoom (which currently
reference params.TimeStart and params.TimeEnd) with calls to
validateTimeConsistency(params.TimeStart, params.TimeEnd); also ensure the
helper and call sites use Go's time.Time methods and formatting conventions (no
custom format logic) and import time where needed.
Comment on lines
+95
to
+101
| // BeforeSave, BeforeUpdate が発火 | ||
| err := db.Where("room_id", room.ID).Delete(&RoomAdmin{}).Error | ||
| if err!=nil{ | ||
| return nil,err | ||
| } | ||
| // 時間整合性は service で確認済み | ||
| err = db.Omit("verified", "CreatedAt").Save(&room).Error |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the Room model and Admins association
echo "=== infra/db/room.go の内容を確認 ==="
cat -n infra/db/room.go
echo ""
echo "=== RoomAdmin モデルの定義を確認 ==="
rg -n "type RoomAdmin" --type=go -A20
echo ""
echo "=== Room モデルの定義を確認 ==="
rg -n "type Room struct" --type=go -A30
echo ""
echo "=== updateRoom のテストを確認 ==="
fd ".*test.*\.go" | xargs rg -l "UpdateRoom|updateRoom" 2>/dev/null | head -5Repository: traPtitech/knoQ
Length of output: 12992
🏁 Script executed:
#!/bin/bash
# Find hooks.go and check BeforeSave hook for Room
echo "=== hooks.go を検索 ==="
fd "hooks\.go" --type f
echo ""
echo "=== Room 関連の BeforeSave/BeforeUpdate フック定義を確認 ==="
rg -n "BeforeSave|BeforeUpdate" --type=go -B2 -A5 | head -40
echo ""
echo "=== room_test.go でのテスト内容を確認 ==="
head -150 infra/db/room_test.goRepository: traPtitech/knoQ
Length of output: 3811
🏁 Script executed:
#!/bin/bash
# Search for where RoomAdmin is created/updated to understand if Admins management is separate
echo "=== RoomAdmin 作成処理を検索 ==="
rg -n "RoomAdmin\{\|Append.*RoomAdmin" --type=go -B2 -A2
echo ""
echo "=== ConvUpdateRoomParamsToRoom を確認 ==="
rg -n "ConvUpdateRoomParamsToRoom" --type=go -B2 -A15Repository: traPtitech/knoQ
Length of output: 1935
Admins 関連付けが保存されない実装上のバグと誤解釈なコメント
-
Line 95 のコメント:
BeforeSave, BeforeUpdate が発火は誤りです。Room モデルにはこれらのフックは存在しません（Event や EventTag にのみ存在）。コメントを削除するか、正確に更新してください。 -
Line 96-101: Admins 関連付けが保存されない:
ConvUpdateRoomParamsToRoomは新しい RoomAdmin オブジェクトをroom.Adminsスライスに作成していますが、SaveをFullSaveAssociations: trueなしで呼び出すため、新しい Admins がデータベースに保存されません。コメントアウトされているコード（103-104行目）が示すとおり、以下のように修正が必要です：
- // BeforeSave, BeforeUpdate が発火
- err := db.Where("room_id", room.ID).Delete(&RoomAdmin{}).Error
+ err := db.Where("room_id = ?", room.ID).Delete(&RoomAdmin{}).Error
if err != nil {
return nil, err
}
// 時間整合性は service で確認済み
- err = db.Omit("verified", "CreatedAt").Save(&room).Error
-
- // err := db.Session(&gorm.Session{FullSaveAssociations: true}).
- // Omit("verified", "CreatedAt").Save(&room).Error
+ err = db.Session(&gorm.Session{FullSaveAssociations: true}).
+ Omit("verified", "CreatedAt").Save(&room).Error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/db/room.go` around lines 95 - 101, The comment saying "BeforeSave,
BeforeUpdate が発火" is incorrect for the Room model—remove or correct that
comment; then fix the admins-not-being-saved bug by ensuring RoomAdmin
associations created by ConvUpdateRoomParamsToRoom are persisted: after deleting
existing admins with db.Where("room_id", room.ID).Delete(&RoomAdmin{}), call
Save on the room using a GORM session with FullSaveAssociations enabled (e.g.,
use db.Session(&gorm.Session{FullSaveAssociations:
true}).Omit("verified","CreatedAt").Save(&room)) so new room.Admins are saved;
keep the omit fields and existing deletion logic intact.
Comment on lines
+12
to
+19
| var ( | ||
| ErrInvalidArgs = errors.New("invalid args") | ||
| ErrTimeConsistency = errors.New("inconsistent time") | ||
| ErrExpression = errors.New("invalid expression") | ||
| ErrRoomUndefined = errors.New("invalid room or args") | ||
| ErrNoAdmins = errors.New("no admins") | ||
| ErrDuplicateEntry = errors.New("duplicate entry") | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check service/errors.go and trace error handling flow
echo "=== service/errors.go content ==="
cat -n service/errors.go
echo ""
echo "=== ErrTimeConsistency usage locations ==="
rg -n "ErrTimeConsistency" --type=go -B1 -A1
echo ""
echo "=== handleDBError implementation ==="
rg -n "func.*handleDBError" --type=go -A20
echo ""
echo "=== defaultErrorHandling implementation ==="
rg -n "func.*defaultErrorHandling" --type=go -A20
echo ""
echo "=== Check room_impl.go for ErrTimeConsistency usage ==="
rg -n "ErrTimeConsistency" service/room_impl.go -B2 -A2Repository: traPtitech/knoQ
Length of output: 7130
🏁 Script executed:
#!/bin/bash
# Check HTTP handler code that uses service methods and how errors are handled
echo "=== Find HTTP handlers ==="
fd -e go -path "*/infra/api*" -o -path "*/handler*" | head -20
echo ""
echo "=== Search for service calls in handlers ==="
rg -n "\.CreateRoom\(|\.UpdateRoom\(" --type=go -B2 -A5 | head -50
echo ""
echo "=== Check if service.ErrTimeConsistency is handled in handlers ==="
rg -n "service\.Err" --type=go | grep -i handle
echo ""
echo "=== Check how errors from service are handled ==="
rg -n "s\..*Room\(" --type=go -A10 | grep -A5 "if err"Repository: traPtitech/knoQ
Length of output: 2308
🏁 Script executed:
#!/bin/bash
# Check all service.Err* direct returns vs wrapped returns
echo "=== All service.Err* returns without defaultErrorHandling wrapping ==="
rg -n "return.*service\.Err|return nil.*Err(Time|Invalid|Expression|Room|NoAdmins|Duplicate)" service/ --type=go
echo ""
echo "=== Count returns with defaultErrorHandling ==="
rg -n "return.*defaultErrorHandling" service/ --type=go | wc -l
echo ""
echo "=== Check all Err* constants usage in service layer ==="
rg -n "Err(TimeConsistency|InvalidArgs|Expression|RoomUndefined|NoAdmins|DuplicateEntry)" service/ --type=go -c | sortRepository: traPtitech/knoQ
Length of output: 591
サービス層の検証エラーがエラーハンドリングマッピングをバイパスしている
service/room_impl.go の時間整合性チェック（14行目、32行目、59行目）で ErrTimeConsistency が defaultErrorHandling を通さずに直接返されています。これにより、DB層から返されるエラーは domain 層のエラー型にマップされる一方、サービス層の検証エラーはマップされないという不整合が発生しています。
修正案：
- サービス層の検証エラーを
defaultErrorHandlingでラップする - または、
handleDBErrorでservice.ErrTimeConsistencyもマップする
return nil, defaultErrorHandling(ErrTimeConsistency)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/errors.go` around lines 12 - 19, The service returns
ErrTimeConsistency directly from functions in room_impl.go (references:
ErrTimeConsistency) which bypasses the global mapping; change those direct
returns to wrap the error with defaultErrorHandling (e.g., return nil,
defaultErrorHandling(ErrTimeConsistency)) so they go through the same mapping
path, or alternatively update handleDBError to also map
service.ErrTimeConsistency to the appropriate domain error; adjust every
occurrence in room_impl.go where ErrTimeConsistency is returned (the three
validation sites) to use defaultErrorHandling unless you choose to extend
handleDBError instead.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
service/room_impl.go (1)
47-50: コメントアウトされた nil チェックは削除推奨です。
将来の混乱防止のため、不要なら削除しておくのが無難です。🧹 例
- // そもそもNilだとIsRoomAdminsで弾かれるはず - // if roomID == uuid.Nil { - // return nil,ErrRoomUndefined - // }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/room_impl.go` around lines 47 - 50, Remove the commented-out nil check for roomID (the lines checking "if roomID == uuid.Nil { return nil, ErrRoomUndefined }") from room_impl.go to avoid clutter and future confusion; confirm that IsRoomAdmins (or other callers) already handle uuid.Nil so no behavior changes occur, and if not, either re-enable a proper runtime check in the Room-related entrypoint (e.g., the function containing IsRoomAdmins) or add a unit test ensuring uuid.Nil is rejected.infra/db/hooks.go (1)
192-218: 大きなコメントアウトブロックは削除かドキュメント移動を検討してください。
将来的に有効化する意図があるなら別途設計メモに移した方がコードが読みやすくなります。🧹 例（削除）
-// func (r *Room) BeforeSave(_ *gorm.DB) (err error) { -// ... -// } - -// func (r *Room) BeforeUpdate(tx *gorm.DB) (err error) { -// ... -// } - -/* -// BeforeCreate is hook -// func (u *User) BeforeCreate(_ *gorm.DB) (err error) { -// ... -// } -*/Also applies to: 300-312
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/hooks.go` around lines 192 - 218, Remove the large commented-out GORM hooks or relocate them to design documentation: either delete the block containing Room.BeforeSave and Room.BeforeUpdate (including references to ConvRoomTodomainRoom, NewValueError, ErrTimeConsistency, and RoomAdmin) or extract the logic and rationale into a design note (e.g., a markdown file) and leave a one-line TODO comment with a link to that note; ensure no dead/commented code remains in infra/db/hooks.go to keep the file clean and maintainable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infra/db/user.go`:
- Around line 121-132: The BeforeCreate hook currently returns ErrRecordNotFound
whenever user.ID != uuid.Nil which prevents creating new users that supply an
ID; instead, change BeforeCreate so it only generates a new UUID when user.ID ==
uuid.Nil (use uuid.NewV4) and in all other cases attempt tx.Create(&user).Error
rather than returning ErrRecordNotFound; reference the BeforeCreate function,
the user.ID check, uuid.NewV4 call, tx.Create(&user).Error and ErrRecordNotFound
and remove the early ErrRecordNotFound branch so provided IDs are allowed and
creation is attempted.
---
Duplicate comments:
In `@infra/db/room.go`:
- Around line 91-104: The Room save path can fail to persist room.Admins and has
an ambiguous Where clause: change the delete to use explicit GORM condition
syntax (e.g., db.Where("room_id = ?", room.ID).Delete(&RoomAdmin{}) ) and
persist associations by using a session with FullSaveAssociations when saving
the room (use db.Session(&gorm.Session{FullSaveAssociations:
true}).Omit("verified", "CreatedAt").Save(&room).Error ); also remove or update
the misleading "BeforeSave, BeforeUpdate が発火" comment so it no longer implies
hooks will run in a way that could be misunderstood.
In `@service/event_impl.go`:
- Around line 53-73: The commented logic for creating a room is inverted: when
params.RoomID == uuid.Nil you should create a room only if params.Place != ""
and otherwise return ErrRoomUndefined. Update the block around params.RoomID,
flipping the inner condition so that if params.RoomID == uuid.Nil and
params.Place != "" you build domain.WriteRoomParams (using params.Place,
TimeStart, TimeEnd, Admins) and call s.CreateUnVerifiedRoom(ctx, reqID,
roomParams), propagating any error; if params.Place == "" return
ErrRoomUndefined. Ensure you reference params.RoomID, params.Place,
domain.WriteRoomParams, s.CreateUnVerifiedRoom and ErrRoomUndefined when making
the fix.
In `@service/room_impl.go`:
- Around line 12-15: The service currently returns ErrTimeConsistency directly
from the time-consistency check (using params.TimeStart and params.TimeEnd),
which bypasses the shared error mapping; change the direct return (return nil,
ErrTimeConsistency) to route the error through the common handler (e.g., call
defaultErrorHandling or the project's error-mapping helper) so the error goes
through the unified mapping path; update the same pattern wherever
ErrTimeConsistency (and similar service-layer errors) are returned directly so
all returns use defaultErrorHandling(ctx, ErrTimeConsistency) or the equivalent
mapping function referenced by your codebase.
---
Nitpick comments:
In `@infra/db/hooks.go`:
- Around line 192-218: Remove the large commented-out GORM hooks or relocate
them to design documentation: either delete the block containing Room.BeforeSave
and Room.BeforeUpdate (including references to ConvRoomTodomainRoom,
NewValueError, ErrTimeConsistency, and RoomAdmin) or extract the logic and
rationale into a design note (e.g., a markdown file) and leave a one-line TODO
comment with a link to that note; ensure no dead/commented code remains in
infra/db/hooks.go to keep the file clean and maintainable.
In `@service/room_impl.go`:
- Around line 47-50: Remove the commented-out nil check for roomID (the lines
checking "if roomID == uuid.Nil { return nil, ErrRoomUndefined }") from
room_impl.go to avoid clutter and future confusion; confirm that IsRoomAdmins
(or other callers) already handle uuid.Nil so no behavior changes occur, and if
not, either re-enable a proper runtime check in the Room-related entrypoint
(e.g., the function containing IsRoomAdmins) or add a unit test ensuring
uuid.Nil is rejected.
infra/db/user.go
Outdated
Comment on lines
+121
to
+132
| // BeforeCreate が呼ばれる | ||
| if user.ID != uuid.Nil { | ||
| // 単純にIDが違う場合 | ||
| return ErrRecordNotFound | ||
| } else { | ||
| // IDなしで新たに作成する場合 | ||
| user.ID ,err = uuid.NewV4() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return tx.Create(&user).Error | ||
| } |
There was a problem hiding this comment.
新規ユーザー作成が成立しなくなっています。
user.ID が設定済みでも未存在なら作成すべきケース（SaveUser/SyncUsers）が想定されるため、現在の ErrRecordNotFound 返却は動作破壊になり得ます。CIの「record not found」警告とも整合します。
✅ 修正案
- if user.ID != uuid.Nil {
- // 単純にIDが違う場合
- return ErrRecordNotFound
- } else {
- // IDなしで新たに作成する場合
- user.ID ,err = uuid.NewV4()
- if err != nil {
- return err
- }
- return tx.Create(&user).Error
- }
+ if user.ID == uuid.Nil {
+ user.ID, err = uuid.NewV4()
+ if err != nil {
+ return err
+ }
+ }
+ return tx.Create(&user).Error📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // BeforeCreate が呼ばれる | |
| if user.ID != uuid.Nil { | |
| // 単純にIDが違う場合 | |
| return ErrRecordNotFound | |
| } else { | |
| // IDなしで新たに作成する場合 | |
| user.ID ,err = uuid.NewV4() | |
| if err != nil { | |
| return err | |
| } | |
| return tx.Create(&user).Error | |
| } | |
| // BeforeCreate が呼ばれる | |
| if user.ID == uuid.Nil { | |
| user.ID, err = uuid.NewV4() | |
| if err != nil { | |
| return err | |
| } | |
| } | |
| return tx.Create(&user).Error |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/db/user.go` around lines 121 - 132, The BeforeCreate hook currently
returns ErrRecordNotFound whenever user.ID != uuid.Nil which prevents creating
new users that supply an ID; instead, change BeforeCreate so it only generates a
new UUID when user.ID == uuid.Nil (use uuid.NewV4) and in all other cases
attempt tx.Create(&user).Error rather than returning ErrRecordNotFound;
reference the BeforeCreate function, the user.ID check, uuid.NewV4 call,
tx.Create(&user).Error and ErrRecordNotFound and remove the early
ErrRecordNotFound branch so provided IDs are allowed and creation is attempted.
Contributor
Author
|
FullSaveAssociations を外してそれぞれの要素を個別にInsert/Updateするように変更したのでトランザクション処理にしないと不具合が発生する説が出ています。 |
Contributor
|
Line 48 in 6f7de9b ここ， convUserTodomainUser の実装が間違っていて privilege の情報が全部 false になっちゃってる気がする...?
|
Contributor
|
詳しく調べきれてないけど， |
Contributor
|
Contributor
Author
|
getで得られるprivilegeが全部falseで手元で権限いじっても403になっちゃうのは再現できたんですが、こちらの手元だとパニックは出てないです。 |
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
infra/db/room.go (1)
104-107:⚠️ Potential issue | 🔴 CriticalWhere句の構文が不正です - RoomAdmin の削除が正しく機能しません。
db.Where("room_id", room.ID)は比較演算子=とプレースホルダー?が欠落しています。この構文では GORM が意図した通りに動作しない可能性があります。
infra/db/group.goのupdateGroup関数（Line 122）では正しいパターンが使用されています:db.Where("group_id = ?", group.ID)🐛 修正案
- err := db.Where("room_id", room.ID).Delete(&RoomAdmin{}).Error + err := db.Where("room_id = ?", room.ID).Delete(&RoomAdmin{}).Error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/room.go` around lines 104 - 107, The Where clause used to delete RoomAdmin records is malformed; update the db.Where call so it uses an equality operator and a placeholder and passes room.ID as the parameter (i.e., use a Where pattern like "room_id = ?" with room.ID) before calling Delete(&RoomAdmin{}), following the same approach used in updateGroup in infra/db/group.go to ensure GORM binds the value correctly.
🧹 Nitpick comments (1)
infra/db/hooks.go (1)
22-143: コメントアウトされたコードの整理を検討してください。リファクタリング中の参照用としてコメントアウトされたコードを残すことは理解できますが、リファクタリング完了後はこれらのコメントを削除することを検討してください。長期的にはコードの可読性を低下させる可能性があります。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/hooks.go` around lines 22 - 143, The file contains multiple large commented-out hook implementations (BeforeSave, BeforeCreate, BeforeUpdate, BeforeDelete) that were left for refactor reference; remove these dead blocks and keep only the active hook AfterSave to improve readability. Specifically, delete the commented bodies for the methods named BeforeSave, BeforeCreate, BeforeUpdate, and BeforeDelete (and any inline comments solely tied to those disabled blocks) while preserving any tests or behavior relying on AfterSave, getEvent, eventFullPreload, ConvEventTodomainEvent, and Devent.AdminsValidation() referenced in the remaining code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infra/db/event.go`:
- Around line 112-117: The loop saving event.Admins and event.Attendees ignores
errors from db.Save, risking partial persistence; update the logic to check the
returned error for each db.Save call and surface/handle failures (return error
or roll back) — preferably wrap the whole save sequence in a transaction using
db.Transaction (or Begin/Commit/Rollback) so that in functions handling event
(where db.Save is called) you call db.Save(&admin) and db.Save(&attendee), check
the error value after each call, and abort/rollback and return that error on
failure to avoid partial data writes.
In `@infra/db/group.go`:
- Around line 124-145: The UpdateGroup flow performs multiple separate DB writes
(deleting GroupMember and GroupAdmin, saving groups via
db.Omit("CreatedAt").Save(&group), then recreating members/admins) without a
transaction, so partial failures can leave inconsistent state; wrap the entire
sequence in a single transaction (use db.Transaction or begin/commit/rollback)
and replace all db.* calls with the transaction handle
(tx.Where(...).Delete(&GroupMember{}), tx.Where(...).Delete(&GroupAdmin{}),
tx.Omit("CreatedAt").Save(&group), and tx.Save(...) inside the loops for
group.Members and group.Admins), ensure you return any error to trigger rollback
and only commit when all operations succeed.
- Around line 165-176: DeleteGroup currently issues three separate DELETEs
(GroupMember, GroupAdmin, then Group) without a transaction, risking partial
deletion and broken integrity; wrap the sequence in a single DB transaction
(using db.Transaction or explicit Begin/Commit/Rollback) so that if any delete
(the Where(...).Delete(&GroupMember{}), Where(...).Delete(&GroupAdmin{}), or
Delete(&group)) fails the transaction is rolled back and an error returned;
alternatively ensure the schema has ON DELETE CASCADE for the group FK, but the
immediate fix is to run those three operations inside a transaction in
DeleteGroup.
- Around line 137-143: The loops over group.Members and group.Admins use "for
member := range group.Members" which assigns the index, not the element, so
db.Save(&member) is saving the index instead of the structs; change both loops
to iterate elements (e.g., "for i := range ..." replaced with "for _, member :=
range group.Members" and "for _, admin := range group.Admins" or equivalent),
call db.Save with the element pointer (db.Save(&member)/db.Save(&admin)), and
add error handling after each save to check and return/log any DB error from the
Save calls in the surrounding function (refer to group.Members, group.Admins,
db.Save, member, admin).
In `@infra/db/room.go`:
- Around line 87-89: room.Admins をループして db.Save(&admin)
している箇所でエラーを無視しているため、保存失敗時に部分的な不整合が起きます。db.Save の返り値を受け取って result.Error
をチェックし、エラーがあれば適切に返すか（呼び出し元で扱うため）ロールバックできるようにトランザクション（tx := db.Begin(),
tx.Save(...), tx.Rollback()/tx.Commit()）を使って処理をまとめてください；該当の識別子は room.Admins と
db.Save(&admin) です。
- Around line 108-110: The admin save loop ignores errors—like in createRoom you
must capture and handle the result of db.Save for each admin; replace the bare
db.Save(&admin) in the loop over room.Admins with an error-aware save (inspect
the returned result/Error) and propagate or log the error appropriately (e.g.,
return the error from the surrounding function or handle consistently with
createRoom) so failures saving Admin records are not silently dropped.
In `@infra/db/tag.go`:
- Around line 38-46: Don't pre-fill tag.ID before calling db.FirstOrCreate
because GORM will use the non-zero primary key value as the sole WHERE condition
and won't find an existing tag by Name; instead remove the uuid.NewV4()
assignment to tag.ID and call FirstOrCreate with a query by Name and supply the
generated ID via Attrs (e.g. use db.Where("name = ?", tag.Name).Attrs(Tag{ID:
uuid.NewV4()}).FirstOrCreate(&tag)) so the ID is only applied on create;
reference tag.ID, uuid.NewV4(), db.FirstOrCreate, db.Where and Attrs to locate
and change the code.
In `@infra/db/user.go`:
- Around line 129-135: The panic comes from always calling saveToken(tx,
&user.Token) even when user.Token is nil/empty (SyncUsers builds Users without
tokens); update saveUser to check user.Token for nil and for empty fields before
invoking saveToken so you skip token saving when absent, and apply the same
guard in both create and update paths around the tx.Save(user.Provider) /
tx.Create(user) flows; reference saveUser, saveToken, and SyncUsers when
locating and changing the code.
In `@service/event_impl.go`:
- Around line 35-39: The code assigns p.RoomID from r.ID before checking err
after calling s.CreateUnVerifiedRoom, which can panic if r is nil; move the
p.RoomID = r.ID assignment to after the error check (or only assign when err ==
nil && r != nil) in the CreateUnVerifiedRoom handling block(s) (same fix for the
second occurrence around the CreateUnVerifiedRoom call at the later block lines
~91-95) so the original error is preserved and nil dereference avoided.
- Around line 26-44: The code creates a Room (via CreateUnVerifiedRoom) and then
persists the Event in separate calls, which is non-atomic and can leave a
dangling Room if the Event save fails; change the flow so Room creation and
Event persistence occur inside a single repository transaction (e.g. add/use a
repository transactional boundary like Repo.RunInTx(ctx, func(tx) { ... }) or
s.repo.WithTx) so both CreateUnVerifiedRoom and the Event save happen on the
same tx, rollback on errors, and return ErrRoomUndefined only when neither Room
params nor RoomID are provided; update calls referencing CreateUnVerifiedRoom,
params.RoomID and the Event save logic to use the transactional context.
- Around line 82-100: In UpdateEvent, when params.RoomID == uuid.Nil you must
reuse the existing event's room instead of always creating a new one: if
currentEvent exists, set p.RoomID = currentEvent.Room.ID by default; only call
CreateUnVerifiedRoom (the method CreateUnVerifiedRoom and variable r) when
params.Place is non-empty AND is different from currentEvent.Room.Place (i.e.,
the caller is explicitly replacing the room); also ensure you check err
immediately after CreateUnVerifiedRoom before using r.ID and return
ErrRoomUndefined only when there is no currentEvent and no params.Place.
---
Duplicate comments:
In `@infra/db/room.go`:
- Around line 104-107: The Where clause used to delete RoomAdmin records is
malformed; update the db.Where call so it uses an equality operator and a
placeholder and passes room.ID as the parameter (i.e., use a Where pattern like
"room_id = ?" with room.ID) before calling Delete(&RoomAdmin{}), following the
same approach used in updateGroup in infra/db/group.go to ensure GORM binds the
value correctly.
---
Nitpick comments:
In `@infra/db/hooks.go`:
- Around line 22-143: The file contains multiple large commented-out hook
implementations (BeforeSave, BeforeCreate, BeforeUpdate, BeforeDelete) that were
left for refactor reference; remove these dead blocks and keep only the active
hook AfterSave to improve readability. Specifically, delete the commented bodies
for the methods named BeforeSave, BeforeCreate, BeforeUpdate, and BeforeDelete
(and any inline comments solely tied to those disabled blocks) while preserving
any tests or behavior relying on AfterSave, getEvent, eventFullPreload,
ConvEventTodomainEvent, and Devent.AdminsValidation() referenced in the
remaining code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8277937c-7f90-4397-9fba-b6f5d81c4a23
📒 Files selected for processing (12)
domain/event.godomain/room.goinfra/db/event.goinfra/db/group.goinfra/db/hooks.goinfra/db/room.goinfra/db/tag.goinfra/db/token.goinfra/db/user.goservice/errors.goservice/event_impl.goservice/room_impl.go
Comment on lines
+124
to
145
| // GroupMenberの削除 | ||
| err = db.Where("group_id = ?", group.ID).Delete(&GroupMember{}).Error | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // GroupAdminの削除 | ||
| err = db.Where("group_id = ?", group.ID).Delete(&GroupAdmin{}).Error | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| err = db.Omit("CreatedAt").Save(&group).Error | ||
|
|
||
| // GroupMember の更新 | ||
| for member := range group.Members { | ||
| db.Save(&member) | ||
| } | ||
| // GroupAdmin の更新 | ||
| for admin := range group.Admins { | ||
| db.Save(&admin) | ||
| } | ||
| return &group, err |
There was a problem hiding this comment.
UpdateGroup を transaction で包まないと、中途半端な状態が永続化されます。
ここは GroupMember 削除 → GroupAdmin 削除 → groups 更新 → 関連再作成が別々の書き込みです。service/group_impl.go:28-42 側でも transaction を張っていないので、途中で 1 本でも失敗するとメンバー/管理者だけ消えたグループが残ります。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/db/group.go` around lines 124 - 145, The UpdateGroup flow performs
multiple separate DB writes (deleting GroupMember and GroupAdmin, saving groups
via db.Omit("CreatedAt").Save(&group), then recreating members/admins) without a
transaction, so partial failures can leave inconsistent state; wrap the entire
sequence in a single transaction (use db.Transaction or begin/commit/rollback)
and replace all db.* calls with the transaction handle
(tx.Where(...).Delete(&GroupMember{}), tx.Where(...).Delete(&GroupAdmin{}),
tx.Omit("CreatedAt").Save(&group), and tx.Save(...) inside the loops for
group.Members and group.Admins), ensure you return any error to trigger rollback
and only commit when all operations succeed.
Comment on lines
+165
to
176
| // Group の GroupMember を削除 | ||
| err := db.Where("group_id = ?", group.ID).Delete(&GroupMember{}).Error | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // GroupAdmin を削除 | ||
| err = db.Where("group_id = ?", group.ID).Delete(&GroupAdmin{}).Error | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // Group を削除 | ||
| return db.Delete(&group).Error |
There was a problem hiding this comment.
DeleteGroup も部分削除のまま終わる可能性があります。
migration/v8/model.go:106-131 では group 側に ON DELETE CASCADE がないので、3 本の DELETE を transaction なしで順に流すと途中失敗時に整合性が壊れます。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/db/group.go` around lines 165 - 176, DeleteGroup currently issues three
separate DELETEs (GroupMember, GroupAdmin, then Group) without a transaction,
risking partial deletion and broken integrity; wrap the sequence in a single DB
transaction (using db.Transaction or explicit Begin/Commit/Rollback) so that if
any delete (the Where(...).Delete(&GroupMember{}),
Where(...).Delete(&GroupAdmin{}), or Delete(&group)) fails the transaction is
rolled back and an error returned; alternatively ensure the schema has ON DELETE
CASCADE for the group FK, but the immediate fix is to run those three operations
inside a transaction in DeleteGroup.
Comment on lines
+26
to
+44
| if params.RoomID == uuid.Nil { | ||
| if params.Place != "" { | ||
| roomParams := domain.WriteRoomParams{ | ||
| Place: params.Place, | ||
| TimeStart: params.TimeStart, | ||
| TimeEnd: params.TimeEnd, | ||
| Admins: params.Admins, | ||
| } | ||
| // UnVerifiedを仮定 | ||
| var r *domain.Room | ||
| r, err = s.CreateUnVerifiedRoom(ctx, reqID, roomParams) | ||
| p.RoomID = r.ID | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } else { | ||
| return nil, ErrRoomUndefined | ||
| } | ||
| } |
There was a problem hiding this comment.
room 作成と event 保存が非原子的です。
この分岐は先に room を永続化し、その後で event を保存しています。infra/db/event.go:77-120 でも transaction で束ねていないので、後段が失敗すると room だけ残ります。ここは 1 つの repository transaction に寄せた方が安全です。
Also applies to: 82-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/event_impl.go` around lines 26 - 44, The code creates a Room (via
CreateUnVerifiedRoom) and then persists the Event in separate calls, which is
non-atomic and can leave a dangling Room if the Event save fails; change the
flow so Room creation and Event persistence occur inside a single repository
transaction (e.g. add/use a repository transactional boundary like
Repo.RunInTx(ctx, func(tx) { ... }) or s.repo.WithTx) so both
CreateUnVerifiedRoom and the Event save happen on the same tx, rollback on
errors, and return ErrRoomUndefined only when neither Room params nor RoomID are
provided; update calls referencing CreateUnVerifiedRoom, params.RoomID and the
Event save logic to use the transactional context.
Comment on lines
+82
to
+100
| if params.RoomID == uuid.Nil { | ||
| if params.Place != "" { | ||
| roomParams := domain.WriteRoomParams{ | ||
| Place: params.Place, | ||
| TimeStart: params.TimeStart, | ||
| TimeEnd: params.TimeEnd, | ||
| Admins: params.Admins, | ||
| } | ||
| // UnVerifiedを仮定 | ||
| var r *domain.Room | ||
| r, err = s.CreateUnVerifiedRoom(ctx, reqID, roomParams) | ||
| p.RoomID = r.ID | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } else { | ||
| return nil, ErrRoomUndefined | ||
| } | ||
| } |
There was a problem hiding this comment.
UpdateEvent で既存 room を再利用せず、更新のたびに非進捗部屋を増やします。
ここは RoomID == uuid.Nil && Place != "" だけで常に CreateUnVerifiedRoom に進んでいます。currentEvent はすでに取得済みなので、RoomID が省略された更新はまず currentEvent.Room.ID を引き継ぎ、明示的に差し替える場合だけ新規 room を作る方が安全です。
修正案
if params.RoomID == uuid.Nil {
- if params.Place != "" {
+ if currentEvent.Room.ID != uuid.Nil {
+ p.RoomID = currentEvent.Room.ID
+ } else if params.Place != "" {
roomParams := domain.WriteRoomParams{
Place: params.Place,
TimeStart: params.TimeStart,
TimeEnd: params.TimeEnd,
Admins: params.Admins,
}
// UnVerifiedを仮定
var r *domain.Room
r, err = s.CreateUnVerifiedRoom(ctx, reqID, roomParams)
- p.RoomID = r.ID
if err != nil {
return nil, err
}
+ p.RoomID = r.ID
} else {
return nil, ErrRoomUndefined
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if params.RoomID == uuid.Nil { | |
| if params.Place != "" { | |
| roomParams := domain.WriteRoomParams{ | |
| Place: params.Place, | |
| TimeStart: params.TimeStart, | |
| TimeEnd: params.TimeEnd, | |
| Admins: params.Admins, | |
| } | |
| // UnVerifiedを仮定 | |
| var r *domain.Room | |
| r, err = s.CreateUnVerifiedRoom(ctx, reqID, roomParams) | |
| p.RoomID = r.ID | |
| if err != nil { | |
| return nil, err | |
| } | |
| } else { | |
| return nil, ErrRoomUndefined | |
| } | |
| } | |
| if params.RoomID == uuid.Nil { | |
| if currentEvent.Room.ID != uuid.Nil { | |
| p.RoomID = currentEvent.Room.ID | |
| } else if params.Place != "" { | |
| roomParams := domain.WriteRoomParams{ | |
| Place: params.Place, | |
| TimeStart: params.TimeStart, | |
| TimeEnd: params.TimeEnd, | |
| Admins: params.Admins, | |
| } | |
| // UnVerifiedを仮定 | |
| var r *domain.Room | |
| r, err = s.CreateUnVerifiedRoom(ctx, reqID, roomParams) | |
| if err != nil { | |
| return nil, err | |
| } | |
| p.RoomID = r.ID | |
| } else { | |
| return nil, ErrRoomUndefined | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/event_impl.go` around lines 82 - 100, In UpdateEvent, when
params.RoomID == uuid.Nil you must reuse the existing event's room instead of
always creating a new one: if currentEvent exists, set p.RoomID =
currentEvent.Room.ID by default; only call CreateUnVerifiedRoom (the method
CreateUnVerifiedRoom and variable r) when params.Place is non-empty AND is
different from currentEvent.Room.Place (i.e., the caller is explicitly replacing
the room); also ensure you check err immediately after CreateUnVerifiedRoom
before using r.ID and return ErrRoomUndefined only when there is no currentEvent
and no params.Place.
Contributor
すみません．これ，私が privilege が全部 false になる問題を解決しようと手元で勝手に Lines 413 to 421 in 904fe1f に変更を入れて privilege も渡すように直してしまってたからでした． |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
infra/db/group.go (1)
113-152:⚠️ Potential issue | 🟠 MajorupdateGroup がトランザクションで囲まれていません
GroupMember削除 →GroupAdmin削除 → Group 保存 → Members 再作成 → Admins 再作成が個別の DB 操作として実行されています。途中で失敗すると、例えば Members だけ削除された状態でグループが残る可能性があります。
service/group_impl.go:28-42でもトランザクションを張っていないため、この層でトランザクションを適用することを検討してください。♻️ トランザクション適用案
func updateGroup(db *gorm.DB, groupID uuid.UUID, args domain.UpsertGroupArgs) (*Group, error) { group := ConvWriteGroupParamsToGroup(args) group.ID = groupID - // groupIDは存在する - var err error - // CreatedBy は readonly - // GroupMember, GroupAdmin も User は readonly - - // GroupMenberの削除 - err = db.Where("group_id = ?", group.ID).Delete(&GroupMember{}).Error - ... + var resultGroup *Group + err := db.Transaction(func(tx *gorm.DB) error { + // GroupMemberの削除 + if err := tx.Where("group_id = ?", group.ID).Delete(&GroupMember{}).Error; err != nil { + return err + } + // GroupAdminの削除 + if err := tx.Where("group_id = ?", group.ID).Delete(&GroupAdmin{}).Error; err != nil { + return err + } + if err := tx.Omit("CreatedAt").Save(&group).Error; err != nil { + return err + } + // GroupMember/GroupAdmin の更新... + resultGroup = &group + return nil + }) + return resultGroup, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/group.go` around lines 113 - 152, The updateGroup function performs multiple separate DB operations (deleting GroupMember/GroupAdmin, saving Group, then recreating Members/Admins) without a transaction; wrap the whole sequence in a single GORM transaction (use db.Transaction or db.Begin/Commit/Rollback) and replace db with the transactional tx inside the closure so any error returns cause rollback. Update updateGroup (and callers if needed) to return the tx error directly and ensure ConvWriteGroupParamsToGroup, Delete/Save calls on GroupMember and GroupAdmin all use the same tx to maintain atomicity.service/event_impl.go (1)
35-40:⚠️ Potential issue | 🔴 Critical部屋作成失敗時に nil 参照が発生します
CreateUnVerifiedRoomが失敗した場合、rは nil になりますが、エラーチェック（Line 38-40）の前にp.RoomID = r.ID（Line 37）を実行しているため、panic が発生します。UpdateEvent の修正パターン（Lines 94-98）に合わせてください。
🐛 修正案
// UnVerifiedを仮定 var r *domain.Room r, err = s.CreateUnVerifiedRoom(ctx, reqID, roomParams) - p.RoomID = r.ID if err != nil { return nil, err } + p.RoomID = r.ID } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/event_impl.go` around lines 35 - 40, CreateUnVerifiedRoom can return nil on error but code assigns p.RoomID = r.ID before checking err, causing a nil deref; change the order to check err immediately after calling CreateUnVerifiedRoom (i.e., if err != nil { return nil, err }) and only then assign p.RoomID = r.ID using the non-nil r, mirroring the UpdateEvent handling pattern; ensure you reference the call to CreateUnVerifiedRoom, the local r variable, and the p.RoomID assignment when making the change.
🧹 Nitpick comments (2)
infra/db/room.go (1)
105-106: コメントが不正確ですLine 105 のコメント「BeforeSave, BeforeUpdate が発火」は誤りです。
infra/db/hooks.goを確認すると、Room モデルには BeforeSave/BeforeUpdate フックが登録されておらず、AfterSave のみ存在します。コメントを削除するか、正確な内容に修正してください。♻️ 修正案
- // BeforeSave, BeforeUpdate が発火 // RoomAdmin を更新 err := db.Where("room_id = ?", room.ID).Delete(&RoomAdmin{}).Error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/room.go` around lines 105 - 106, コメントが不正確なので修正してください：Room に対して BeforeSave/BeforeUpdate フックは登録されておらず AfterSave のみが存在するため、該当箇所のコメント（「BeforeSave, BeforeUpdate が発火」）を削除するか、正確に「AfterSave が発火し、RoomAdmin を更新する」などに書き換えてください。参照するシンボルは Room モデルと infra/db/hooks.go の AfterSave フック、および RoomAdmin 更新ロジックです。infra/db/hooks.go (1)
20-140: コメントアウトされたコードの削除を検討してくださいリファクタリングが安定した後、コメントアウトされたフック実装を削除することを検討してください。現在はリファクタリングの過程でドキュメントとして有用ですが、長期的にはコードの可読性を低下させます。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/hooks.go` around lines 20 - 140, The file contains multiple large commented-out GORM hook implementations (BeforeSave, BeforeCreate, BeforeUpdate, BeforeDelete) that reduce readability; remove these dead blocks now that refactoring is stable: delete the commented implementations for BeforeSave, BeforeCreate, BeforeUpdate, and BeforeDelete while keeping the active AfterSave hook and any helper functions it depends on (e.g., getEvent, eventFullPreload, ConvEventTodomainEvent) intact; if you want to preserve history, move the removed commented code to a single migration/archival file or a VCS branch rather than leaving it commented in infra/db/hooks.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infra/db/event.go`:
- Around line 159-161: The call to createOrGetTag does not check the returned
error; after calling createOrGetTag(db, eventTag.Tag.Name) you must verify err
before using tag.ID or calling db.Create(&eventTag). Modify the code around
createOrGetTag to: check if err != nil, return or propagate the error (or handle
it appropriately), only set eventTag.TagID = tag.ID and call
db.Create(&eventTag) when err is nil, and ensure any created/returned errors are
passed up from the enclosing function.
- Around line 88-103: The code assigns event.Tags[i].EventID = event.ID before
event.ID is generated (uuid.NewV4()), so tags get a nil UUID; move the UUID
generation and its error check (the uuid.NewV4() call that sets event.ID) to
before the loop that iterates event.Tags, then continue calling createOrGetTag
and assigning EventID/TagID/Tag, and finally persist with db.Create(&event);
keep existing error handling around uuid.NewV4(), createOrGetTag, and db.Create
to propagate failures.
In `@infra/db/group.go`:
- Around line 167-183: deleteGroup currently performs separate deletes for
GroupMember, GroupAdmin and Group which can leave the DB in an inconsistent
state on partial failure; wrap these operations in a single transaction (use
db.Transaction or begin a tx and defer rollback/commit) and run the deletes
against the transactional handle (e.g., tx.Where(...).Delete(&GroupMember{}),
tx.Where(...).Delete(&GroupAdmin{}), tx.Delete(&group)); if any delete returns
an error return that error and ensure the transaction is rolled back, otherwise
commit the transaction before returning nil.
- Around line 135-143: After calling db.Omit("CreatedAt").Save(&group).Error you
must check and handle that error before attempting to save GroupMember entries;
update the save flow in the function that performs the group persistence so that
the result of db.Omit("CreatedAt").Save(&group).Error (the local variable err)
is tested and, if non-nil, returned immediately instead of proceeding to the for
_, member := range group.Members loop that calls db.Save(&member).Error; ensure
the existing member-save error handling remains in place.
In `@infra/db/user.go`:
- Around line 129-137: The Provider save result isn’t checked, so failures in
tx.Save(user.Provider) can be ignored; update the create flow to capture the
result of tx.Save(user.Provider) (e.g., res := tx.Save(user.Provider)) and if
res.Error != nil return that error before proceeding to token handling and
tx.Create(user), keeping the existing saveToken(user.Token) checks intact.
- Around line 146-154: The call to tx.Updates(user.Provider) lacks error
checking so a Provider update failure could be ignored; update the code in the
same function to capture the result of tx.Updates(user.Provider) (e.g. err :=
tx.Updates(user.Provider).Error), return that error if non-nil before proceeding
to token handling, and only then continue with setting user.Token.UserID and
calling saveToken(tx, &user.Token) and finally return tx.Updates(user).Error;
ensure you reference tx.Updates(user.Provider), saveToken,
user.Token.Oauth2Token, and tx.Updates(user) when making the change.
---
Duplicate comments:
In `@infra/db/group.go`:
- Around line 113-152: The updateGroup function performs multiple separate DB
operations (deleting GroupMember/GroupAdmin, saving Group, then recreating
Members/Admins) without a transaction; wrap the whole sequence in a single GORM
transaction (use db.Transaction or db.Begin/Commit/Rollback) and replace db with
the transactional tx inside the closure so any error returns cause rollback.
Update updateGroup (and callers if needed) to return the tx error directly and
ensure ConvWriteGroupParamsToGroup, Delete/Save calls on GroupMember and
GroupAdmin all use the same tx to maintain atomicity.
In `@service/event_impl.go`:
- Around line 35-40: CreateUnVerifiedRoom can return nil on error but code
assigns p.RoomID = r.ID before checking err, causing a nil deref; change the
order to check err immediately after calling CreateUnVerifiedRoom (i.e., if err
!= nil { return nil, err }) and only then assign p.RoomID = r.ID using the
non-nil r, mirroring the UpdateEvent handling pattern; ensure you reference the
call to CreateUnVerifiedRoom, the local r variable, and the p.RoomID assignment
when making the change.
---
Nitpick comments:
In `@infra/db/hooks.go`:
- Around line 20-140: The file contains multiple large commented-out GORM hook
implementations (BeforeSave, BeforeCreate, BeforeUpdate, BeforeDelete) that
reduce readability; remove these dead blocks now that refactoring is stable:
delete the commented implementations for BeforeSave, BeforeCreate, BeforeUpdate,
and BeforeDelete while keeping the active AfterSave hook and any helper
functions it depends on (e.g., getEvent, eventFullPreload,
ConvEventTodomainEvent) intact; if you want to preserve history, move the
removed commented code to a single migration/archival file or a VCS branch
rather than leaving it commented in infra/db/hooks.go.
In `@infra/db/room.go`:
- Around line 105-106: コメントが不正確なので修正してください：Room に対して BeforeSave/BeforeUpdate
フックは登録されておらず AfterSave のみが存在するため、該当箇所のコメント（「BeforeSave, BeforeUpdate
が発火」）を削除するか、正確に「AfterSave が発火し、RoomAdmin を更新する」などに書き換えてください。参照するシンボルは Room モデルと
infra/db/hooks.go の AfterSave フック、および RoomAdmin 更新ロジックです。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca52d74e-e391-46a3-acb4-2abab6742685
📒 Files selected for processing (8)
infra/db/converter.goinfra/db/event.goinfra/db/group.goinfra/db/hooks.goinfra/db/room.goinfra/db/tag.goinfra/db/user.goservice/event_impl.go
Comment on lines
+159
to
+161
| tag, err := createOrGetTag(db, eventTag.Tag.Name) | ||
| eventTag.TagID = tag.ID | ||
| err = db.Create(&eventTag).Error |
There was a problem hiding this comment.
createOrGetTag のエラーが確認されていません
Line 159 で createOrGetTag から返されるエラーが Line 161 の db.Create 前にチェックされていません。タグの取得/作成に失敗しても処理が続行されます。
🔧 修正案
tag, err := createOrGetTag(db, eventTag.Tag.Name)
+ if err != nil {
+ return err
+ }
eventTag.TagID = tag.ID
err = db.Create(&eventTag).Error📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| tag, err := createOrGetTag(db, eventTag.Tag.Name) | |
| eventTag.TagID = tag.ID | |
| err = db.Create(&eventTag).Error | |
| tag, err := createOrGetTag(db, eventTag.Tag.Name) | |
| if err != nil { | |
| return err | |
| } | |
| eventTag.TagID = tag.ID | |
| err = db.Create(&eventTag).Error |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/db/event.go` around lines 159 - 161, The call to createOrGetTag does
not check the returned error; after calling createOrGetTag(db,
eventTag.Tag.Name) you must verify err before using tag.ID or calling
db.Create(&eventTag). Modify the code around createOrGetTag to: check if err !=
nil, return or propagate the error (or handle it appropriately), only set
eventTag.TagID = tag.ID and call db.Create(&eventTag) when err is nil, and
ensure any created/returned errors are passed up from the enclosing function.
Comment on lines
167
to
183
| func deleteGroup(db *gorm.DB, groupID uuid.UUID) error { | ||
| group := Group{ | ||
| ID: groupID, | ||
| } | ||
| // Group の GroupMember を削除 | ||
| err := db.Where("group_id = ?", group.ID).Delete(&GroupMember{}).Error | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // GroupAdmin を削除 | ||
| err = db.Where("group_id = ?", group.ID).Delete(&GroupAdmin{}).Error | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // Group を削除 | ||
| return db.Delete(&group).Error | ||
| } |
There was a problem hiding this comment.
deleteGroup がトランザクションで囲まれていません
GroupMember 削除 → GroupAdmin 削除 → Group 削除が個別に実行されており、途中で失敗すると不整合が発生します。updateGroup と同様にトランザクションで囲むことを検討してください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/db/group.go` around lines 167 - 183, deleteGroup currently performs
separate deletes for GroupMember, GroupAdmin and Group which can leave the DB in
an inconsistent state on partial failure; wrap these operations in a single
transaction (use db.Transaction or begin a tx and defer rollback/commit) and run
the deletes against the transactional handle (e.g.,
tx.Where(...).Delete(&GroupMember{}), tx.Where(...).Delete(&GroupAdmin{}),
tx.Delete(&group)); if any delete returns an error return that error and ensure
the transaction is rolled back, otherwise commit the transaction before
returning nil.
iChemy
reviewed
Mar 17, 2026
| } | ||
|
|
||
| func createOrGetTag(db *gorm.DB, name string) (*Tag, error) { | ||
| id,err := uuid.NewV4() |
Contributor
There was a problem hiding this comment.
すでに存在しているタグを取得したいときも uuid 作成するのどうなんだろう (適当なコメント)
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
infra/db/event.go (1)
87-105:⚠️ Potential issue | 🟠 Major明示保存に切り替えた分、ここはトランザクションでまとめたいです。
createEventは Tag 解決とevents作成が別書き込み、updateEventはevents更新と関連更新が別書き込みになっています。途中で 1 クエリでも失敗すると、Tag だけ作成済み / Event だけ更新済み / 関連の一部だけ更新済み、の半端な状態が残ります。FullSaveAssociationsを外したなら、このまとまりの atomicity はdb.Transactionで担保したいです。Also applies to: 131-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/event.go` around lines 87 - 105, Wrap the multi-write operations in createEvent (the Tag resolution loop using createOrGetTag and the db.Create(&event) call) inside a db.Transaction so all Tag creations and the Event insert are atomic; use the transaction handle (tx) instead of db when calling createOrGetTag and tx.Create, returning an error to trigger rollback on failure. Do the same for updateEvent where events and related records are updated (the block around the separate writes at lines referenced in the review), switching to tx.Save/tx.Model/tx.* calls and ensuring any error returns cause the transaction to rollback. Ensure createOrGetTag is invoked with the transaction handle (or has a tx-aware variant) so Tag creation uses the same transaction. Handle and return errors from the transaction closure so partial writes cannot persist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@infra/db/event.go`:
- Around line 132-154: The current update saves event.Admins, event.Attendees
and event.Tags but never removes relations dropped in the request, leaving stale
rows; modify the update in infra/db/event.go to explicitly replace related
records — either wrap in a transaction and run delete-by-event_id for
Admin/Attendee/Tag before inserting the new slices (use db.Where("event_id = ?",
event.ID).Delete(...)) or use GORM's association replace semantics
(Model(&event).Association("Admins").Replace(event.Admins) and likewise for
"Attendees" and "Tags") so the DB state matches the incoming
event.Admins/event.Attendees/event.Tags exactly.
- Around line 175-183: Wrap the three deletes (EventTag, EventAdmin, and Event)
in a single DB transaction to ensure atomicity: begin a transaction using the
same db instance (e.g., db.Transaction or db.Begin()/Commit()/Rollback), perform
the two tx.Where(...).Delete(&EventTag{}), tx.Where(...).Delete(&EventAdmin{}),
and finally tx.Delete(&Event{ID: eventID}), and return any error to trigger
rollback; replace uses of db in these operations with the transaction variable
(tx) so all deletions occur in one transactional unit.
- Around line 81-84: The helpers (getRoom, createOrGetTag and similar) currently
return (nil, err), which causes CreateEvent/UpdateEvent to dereference a nil
pointer and panic; update either the callers or the helpers so the contract is
safe: prefer modifying getRoom and createOrGetTag to return a non-nil zero-value
object along with the error (so callers can safely check err before using
fields), and audit CreateEvent and UpdateEvent to ensure they check err
immediately before any dereference; search for getRoom, createOrGetTag,
CreateEvent, and UpdateEvent and implement the non-nil-on-error return or early
err-return checks consistently.
In `@infra/db/user.go`:
- Around line 121-140: When creating a new User (when user.ID == uuid.Nil) you
must assign the generated ID to the Provider before saving it: after generating
user.ID (uuid.NewV4()) set user.Provider.UserID = user.ID so that
tx.Save(user.Provider) persists the correct foreign key; keep the existing token
handling (user.Token.UserID = user.ID and saveToken(tx, &user.Token)) as-is and
then call tx.Create(user).Error.
- Around line 149-153: The code unconditionally calls tx.Updates(user.Provider)
which violates the function contract that "user.Provider is not updated when
empty"—guard this by checking whether the Provider is zero-valued before
performing the related update (i.e., in the same function that calls
tx.Updates(user.Provider) within infra/db/user.go, used by
SyncUsers/saveUser/getAllUsers paths); if Provider has no identifying fields
(e.g., ID or other primary key) or is otherwise empty, skip tx.Updates entirely;
otherwise proceed with tx.Updates(user.Provider). Ensure the guard uses the
concrete field(s) that indicate a populated Provider (not just nil-check) so
preloaded vs non-preloaded code paths behave correctly.
---
Duplicate comments:
In `@infra/db/event.go`:
- Around line 87-105: Wrap the multi-write operations in createEvent (the Tag
resolution loop using createOrGetTag and the db.Create(&event) call) inside a
db.Transaction so all Tag creations and the Event insert are atomic; use the
transaction handle (tx) instead of db when calling createOrGetTag and tx.Create,
returning an error to trigger rollback on failure. Do the same for updateEvent
where events and related records are updated (the block around the separate
writes at lines referenced in the review), switching to tx.Save/tx.Model/tx.*
calls and ensuring any error returns cause the transaction to rollback. Ensure
createOrGetTag is invoked with the transaction handle (or has a tx-aware
variant) so Tag creation uses the same transaction. Handle and return errors
from the transaction closure so partial writes cannot persist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d914fe4e-226a-4c84-99e0-c6fadf839537
📒 Files selected for processing (3)
infra/db/event.goinfra/db/group.goinfra/db/user.go
🚧 Files skipped from review as they are similar to previous changes (1)
- infra/db/group.go
Comment on lines
+81
to
+84
| r, err = getRoom(db, event.RoomID) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
nil 返却が呼び出し側の panic 経路になっています。
CreateEvent / UpdateEvent はこの戻り値を err チェック前にデリファレンスしています。ここで nil, err を返すと、getRoom や createOrGetTag の失敗がそのまま panic に化けます。呼び出し側を先に err 判定へ寄せるか、少なくともこの helper 群の「エラー時でも非 nil を返す」契約は崩さない方が安全です。
Also applies to: 94-95, 115-117, 123-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/db/event.go` around lines 81 - 84, The helpers (getRoom, createOrGetTag
and similar) currently return (nil, err), which causes CreateEvent/UpdateEvent
to dereference a nil pointer and panic; update either the callers or the helpers
so the contract is safe: prefer modifying getRoom and createOrGetTag to return a
non-nil zero-value object along with the error (so callers can safely check err
before using fields), and audit CreateEvent and UpdateEvent to ensure they check
err immediately before any dereference; search for getRoom, createOrGetTag,
CreateEvent, and UpdateEvent and implement the non-nil-on-error return or early
err-return checks consistently.
Comment on lines
+132
to
+154
| err = db.Save(&event).Error | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| for _, admin := range event.Admins { | ||
| err = db.Save(&admin).Error | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| for _, attendee := range event.Attendees { | ||
| err = db.Save(&attendee).Error | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| for _, etag := range event.Tags { | ||
| err = db.Save(&etag).Error | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
削除された Admin / Attendee / Tag が残り続けます。
今の更新処理は現在の配列を Save しているだけで、リクエストから消えた関連を削除していません。イベント編集で管理者や参加者、タグを外しても旧レコードが残るので、更新結果が入力と一致しなくなります。event_id 単位の delete+insert か、少なくとも置換 semantics を明示した処理が必要です。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/db/event.go` around lines 132 - 154, The current update saves
event.Admins, event.Attendees and event.Tags but never removes relations dropped
in the request, leaving stale rows; modify the update in infra/db/event.go to
explicitly replace related records — either wrap in a transaction and run
delete-by-event_id for Admin/Attendee/Tag before inserting the new slices (use
db.Where("event_id = ?", event.ID).Delete(...)) or use GORM's association
replace semantics (Model(&event).Association("Admins").Replace(event.Admins) and
likewise for "Attendees" and "Tags") so the DB state matches the incoming
event.Admins/event.Attendees/event.Tags exactly.
Comment on lines
+175
to
183
| err := db.Where("event_id = ?", eventID).Delete(&EventTag{}).Error | ||
| if err != nil { | ||
| return err | ||
| } | ||
| err = db.Where("event_id = ?", eventID).Delete(&EventAdmin{}).Error | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return db.Delete(&Event{ID: eventID}).Error |
There was a problem hiding this comment.
依存行削除と Event 本体削除も同一トランザクションにしてください。
EventAdmin 側は明示削除が必要ですが、今の 3 クエリ分割だと最後の events 削除だけ失敗したときに Event だけ残ります。削除フローもまとめて rollback できる形にしておかないと、ここでも不整合が出ます。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/db/event.go` around lines 175 - 183, Wrap the three deletes (EventTag,
EventAdmin, and Event) in a single DB transaction to ensure atomicity: begin a
transaction using the same db instance (e.g., db.Transaction or
db.Begin()/Commit()/Rollback), perform the two
tx.Where(...).Delete(&EventTag{}), tx.Where(...).Delete(&EventAdmin{}), and
finally tx.Delete(&Event{ID: eventID}), and return any error to trigger
rollback; replace uses of db in these operations with the transaction variable
(tx) so all deletions occur in one transactional unit.
infra/db/user.go
Outdated
Comment on lines
+121
to
+140
| // Userを新たに Create | ||
| if user.ID == uuid.Nil { | ||
| // IDを新たに作成する場合 | ||
| user.ID, err = uuid.NewV4() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| // 関連の作成 | ||
| err = tx.Save(user.Provider).Error | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if user.Token.Oauth2Token != nil { | ||
| user.Token.UserID = user.ID | ||
| if err := saveToken(tx, &user.Token); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return tx.Create(user).Error |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd infra/db user.goRepository: traPtitech/knoQ
Length of output: 153
🏁 Script executed:
cat -n infra/db/user.go | sed -n '110,165p'Repository: traPtitech/knoQ
Length of output: 1788
🏁 Script executed:
cat -n infra/db/model.go | sed -n '1,100p'Repository: traPtitech/knoQ
Length of output: 3719
🏁 Script executed:
cat -n infra/db/token.go | sed -n '75,100p'Repository: traPtitech/knoQ
Length of output: 605
🏁 Script executed:
cd infra/db && grep -n "Provider\|Token" user.go | head -30Repository: traPtitech/knoQ
Length of output: 908
🏁 Script executed:
rg "FullSaveAssociations" infra/Repository: traPtitech/knoQ
Length of output: 393
🏁 Script executed:
cd infra && git log --oneline -5Repository: traPtitech/knoQ
Length of output: 90
新規作成時に Provider.UserID を同期してください。
新規ユーザー作成時 (user.ID == uuid.Nil)、線 124 で UUID を生成した後、線 134 で tx.Save(user.Provider) を実行していますが、その前に user.Provider.UserID = user.ID を設定していません。一方で同じ流れで Token に対しては線 139 で user.Token.UserID = user.ID と正しく同期されています。Provider も同様に UUID 生成後すぐに user.Provider.UserID = user.ID を追加して、Save 前に主キーを確定させてください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/db/user.go` around lines 121 - 140, When creating a new User (when
user.ID == uuid.Nil) you must assign the generated ID to the Provider before
saving it: after generating user.ID (uuid.NewV4()) set user.Provider.UserID =
user.ID so that tx.Save(user.Provider) persists the correct foreign key; keep
the existing token handling (user.Token.UserID = user.ID and saveToken(tx,
&user.Token)) as-is and then call tx.Create(user).Error.
Comment on lines
+149
to
+153
| // 関連の更新 | ||
| err = tx.Updates(user.Provider).Error | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l infra/db/user.goRepository: traPtitech/knoQ
Length of output: 79
🏁 Script executed:
cat -n infra/db/user.go | sed -n '1,200p'Repository: traPtitech/knoQ
Length of output: 6101
🏁 Script executed:
rg "type Provider struct" infra/db/Repository: traPtitech/knoQ
Length of output: 99
🏁 Script executed:
cat -n infra/db/*.go | grep -A 10 "type Provider struct"Repository: traPtitech/knoQ
Length of output: 403
🏁 Script executed:
rg "saveUser\(" infra/db/user.goRepository: traPtitech/knoQ
Length of output: 213
🏁 Script executed:
cat -n infra/db/user.go | sed -n '16,40p'Repository: traPtitech/knoQ
Length of output: 850
関連の無条件更新は契約違反です。空のProviderをガードしてください。
Line 115 の関数コメントで「user.Providerは空の時、更新されません」と明記されていますが、Line 154 は無条件に tx.Updates(user.Provider) を実行しています。
一方、Line 71 の getAllUsers(tx, false) は Provider を preload しないため、Line 82-84 の state-only 更新パス（SyncUsers → saveUser）では zero-valued な Provider が渡されます。この場合、関連の更新は実行すべきではありません。
修正イメージ
// 関連の更新
-err = tx.Updates(user.Provider).Error
-if err != nil {
- return err
+if user.Provider.UserID != uuid.Nil {
+ if err := tx.Updates(&user.Provider).Error; err != nil {
+ return err
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // 関連の更新 | |
| err = tx.Updates(user.Provider).Error | |
| if err != nil { | |
| return err | |
| } | |
| // 関連の更新 | |
| if user.Provider.UserID != uuid.Nil { | |
| if err := tx.Updates(&user.Provider).Error; err != nil { | |
| return err | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/db/user.go` around lines 149 - 153, The code unconditionally calls
tx.Updates(user.Provider) which violates the function contract that
"user.Provider is not updated when empty"—guard this by checking whether the
Provider is zero-valued before performing the related update (i.e., in the same
function that calls tx.Updates(user.Provider) within infra/db/user.go, used by
SyncUsers/saveUser/getAllUsers paths); if Provider has no identifying fields
(e.g., ID or other primary key) or is otherwise empty, skip tx.Updates entirely;
otherwise proceed with tx.Updates(user.Provider). Ensure the guard uses the
concrete field(s) that indicate a populated Provider (not just nil-check) so
preloaded vs non-preloaded code paths behave correctly.
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 (5)
infra/db/user.go (2)
36-38:⚠️ Potential issue | 🔴 Criticalエラー時に nil ポインタ参照でパニックします。
saveUserがエラーを返した場合、uはnilになりますが、Line 37 で*uをデリファレンスしています。これがPRコメントで報告されたパニックの原因です。🔧 修正案
u, err := saveUser(repo.db, &user) - du := convUserTodomainUser(*u) - return &du, defaultErrorHandling(err) + if err != nil { + return nil, defaultErrorHandling(err) + } + du := convUserTodomainUser(*u) + return &du, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/user.go` around lines 36 - 38, When saveUser returns an error you currently dereference u causing a nil-pointer panic; update the caller around saveUser(repo.db, &user) to check err before using u (e.g., if err != nil return nil, defaultErrorHandling(err)), and only call convUserTodomainUser(*u) when err is nil. Ensure you reference the variables and functions saveUser, u, err, convUserTodomainUser, and defaultErrorHandling so the nil-check is placed immediately after the saveUser call.
46-49:⚠️ Potential issue | 🟡 Minor
GetUserでも同様に nil ポインタ参照のリスクがあります。
getUserがエラーを返した場合、uは空の構造体ですが、エラーハンドリング前に処理を続行しています。🔧 修正案
func (repo *gormRepository) GetUser(userID uuid.UUID) (*domain.User, error) { u, err := getUser(userPreload(repo.db), userID) + if err != nil { + return nil, defaultErrorHandling(err) + } du := convUserTodomainUser(*u) - return &du, defaultErrorHandling(err) + return &du, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/user.go` around lines 46 - 49, GetUser currently dereferences u before checking for errors from getUser, risking a nil-pointer; change GetUser (gormRepository.GetUser) to check the error (and that u != nil) immediately after calling getUser(userPreload(repo.db), userID) and if err != nil (or u == nil) return nil, defaultErrorHandling(err) instead of continuing to convUserTodomainUser; only call convUserTodomainUser(*u) and return &du, nil when the call succeeded.infra/db/group.go (3)
17-21:⚠️ Potential issue | 🔴 Criticalエラー時に nil ポインタ参照でパニックします。
createGroupがエラーを返した場合、gはnilになりますが、Line 19 で*gをデリファレンスしています。🔧 修正案
func (repo *gormRepository) CreateGroup(args domain.UpsertGroupArgs) (*domain.Group, error) { g, err := createGroup(repo.db, args) + if err != nil { + return nil, defaultErrorHandling(err) + } domainGroup := convGroupTodomainGroup(*g) - return &domainGroup, defaultErrorHandling(err) + return &domainGroup, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/group.go` around lines 17 - 21, CreateGroup dereferences g even when createGroup returns an error, causing a nil pointer panic; update CreateGroup to check err immediately after calling createGroup and return nil with defaultErrorHandling(err) if err != nil, only calling convGroupTodomainGroup(*g) and returning the domain group when err is nil; reference CreateGroup, createGroup, convGroupTodomainGroup, and defaultErrorHandling to locate and fix the logic flow.
23-27:⚠️ Potential issue | 🔴 Critical
UpdateGroupでも同様に nil ポインタ参照のリスクがあります。
updateGroupがエラーを返した場合、gはnilになる可能性があります。🔧 修正案
func (repo *gormRepository) UpdateGroup(groupID uuid.UUID, args domain.UpsertGroupArgs) (*domain.Group, error) { g, err := updateGroup(repo.db, groupID, args) + if err != nil { + return nil, defaultErrorHandling(err) + } domainGroup := convGroupTodomainGroup(*g) - return &domainGroup, defaultErrorHandling(err) + return &domainGroup, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/group.go` around lines 23 - 27, The UpdateGroup method dereferences g without checking err or g, risking a nil pointer if updateGroup returned an error; modify UpdateGroup to check if err != nil (and/or if g == nil) immediately after calling updateGroup and return nil with defaultErrorHandling(err) (or a suitable not-found error) before calling convGroupTodomainGroup, ensuring convGroupTodomainGroup is only called with a non-nil *g.
44-48:⚠️ Potential issue | 🟡 Minor
GetGroupでエラーハンドリング前に変換を行っています。エラー時でも空の
Group構造体を変換してしまいます。エラーを先にチェックして early return することを推奨します。🔧 修正案
func (repo *gormRepository) GetGroup(groupID uuid.UUID) (*domain.Group, error) { g, err := getGroup(groupFullPreload(repo.db), groupID) + if err != nil { + return nil, defaultErrorHandling(err) + } domainGroup := convGroupTodomainGroup(*g) - return &domainGroup, defaultErrorHandling(err) + return &domainGroup, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/group.go` around lines 44 - 48, GetGroup currently calls convGroupTodomainGroup(*g) before checking the error from getGroup, which can dereference a nil pointer and converts an empty Group on error; modify GetGroup to check err (and whether g is nil) immediately after calling getGroup (use the returned err with defaultErrorHandling(err) and return early on error) and only call convGroupTodomainGroup(g) when err is nil (reference functions: GetGroup, getGroup, convGroupTodomainGroup, defaultErrorHandling).
♻️ Duplicate comments (4)
infra/db/group.go (2)
134-182:⚠️ Potential issue | 🟠 Major
updateGroupがトランザクションで囲まれていません。
GroupMember削除 →GroupAdmin削除 →group保存 →Members/Admins再作成が別々の書き込みです。途中で失敗すると、メンバー/管理者だけ消えたグループが残ります。PRコメントでも「トランザクションで囲わないと不具合が発生する可能性」と指摘されています。🔧 修正案
func updateGroup(db *gorm.DB, groupID uuid.UUID, args domain.UpsertGroupArgs) (*Group, error) { group := ConvWriteGroupParamsToGroup(args) group.ID = groupID - // groupIDは存在する - var err error + var resultGroup *Group + + err := db.Transaction(func(tx *gorm.DB) error { + // GroupMemberの削除 + if err := tx.Where("group_id = ?", group.ID).Delete(&GroupMember{}).Error; err != nil { + return err + } + // GroupAdminの削除 + if err := tx.Where("group_id = ?", group.ID).Delete(&GroupAdmin{}).Error; err != nil { + return err + } + + if err := tx.Omit("CreatedAt").Save(&group).Error; err != nil { + return err + } + + // GroupMember の更新 + for _, member := range group.Members { + if err := tx.Save(&member).Error; err != nil { + return err + } + } + // GroupAdmin の更新 + for _, admin := range group.Admins { + if err := tx.Save(&admin).Error; err != nil { + return err + } + } + + if err := validateGroup(tx, &group); err != nil { + return err + } + resultGroup = &group + return nil + }) - // ... existing code ... - - return &group, err + return resultGroup, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/group.go` around lines 134 - 182, The updateGroup function performs multiple separate writes (deleting GroupMember/GroupAdmin, saving Group, then saving Members/Admins) and must be wrapped in a DB transaction to ensure atomicity; change updateGroup to run all DB operations inside a gorm transaction (use db.Transaction or Begin/Commit/Rollback) so that the deletions, group save, member/admin saves and the call to validateGroup all use the same tx and are rolled back on any error, and return the transaction error if it fails; update references inside the transaction to use the transaction handle (tx) instead of db and ensure validateGroup is called against tx (or adapted to accept a *gorm.DB) so the whole sequence is committed or rolled back together.
197-213:⚠️ Potential issue | 🟠 Major
deleteGroupもトランザクションで囲まれていません。
GroupMember削除 →GroupAdmin削除 →Group削除が個別に実行されており、途中で失敗すると整合性が壊れます。🔧 修正案
func deleteGroup(db *gorm.DB, groupID uuid.UUID) error { group := Group{ ID: groupID, } - // Group の GroupMember を削除 - err := db.Where("group_id = ?", group.ID).Delete(&GroupMember{}).Error - if err != nil { - return err - } - // GroupAdmin を削除 - err = db.Where("group_id = ?", group.ID).Delete(&GroupAdmin{}).Error - if err != nil { - return err - } - // Group を削除 - return db.Delete(&group).Error + return db.Transaction(func(tx *gorm.DB) error { + // Group の GroupMember を削除 + if err := tx.Where("group_id = ?", group.ID).Delete(&GroupMember{}).Error; err != nil { + return err + } + // GroupAdmin を削除 + if err := tx.Where("group_id = ?", group.ID).Delete(&GroupAdmin{}).Error; err != nil { + return err + } + // Group を削除 + return tx.Delete(&group).Error + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/group.go` around lines 197 - 213, deleteGroup performs three separate deletes (GroupMember, GroupAdmin, Group) without a transaction, risking partial deletes on failure; wrap the sequence in a single GORM transaction using db.Transaction(func(tx *gorm.DB) error { ... }) and replace db with tx for the three operations (the deletes for GroupMember, GroupAdmin and the final Delete(&group)), returning any error from inside the transaction so GORM will rollback on error and commit only if all deletes succeed.infra/db/user.go (2)
153-157:⚠️ Potential issue | 🟠 Major空の Provider を無条件で更新しており、契約違反です。
Line 115 のコメントで「user.Providerは空の時、更新されません」と明記されていますが、Line 154 は無条件に
tx.Updates(user.Provider)を実行しています。SyncUsers→getAllUsers(tx, false)のパスでは Provider が preload されないため、空の Provider が渡されます。🔧 修正案
// 関連の更新 - err = tx.Updates(user.Provider).Error - if err != nil { - return err + if user.Provider.UserID != uuid.Nil { + err = tx.Updates(&user.Provider).Error + if err != nil { + return err + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/user.go` around lines 153 - 157, The code unconditionally runs tx.Updates(user.Provider) which can send an empty Provider and violate invariants; change the logic in the update path to only call tx.Updates when user.Provider is present and contains meaningful fields (e.g. guard that user.Provider != nil and its key fields are non-empty) — for the SyncUsers/getAllUsers path ensure you check user.Provider before calling tx.Updates(user.Provider) (or skip/omit provider updates or use conditional field updates via tx.Model(&user).Updates(...)) so empty/preload-missing Provider structs are not written to DB.
122-134:⚠️ Potential issue | 🟠 Major新規UUID生成時に
Provider.UserIDが同期されていません。Line 124 で
user.IDを新規生成した場合、user.Provider.UserIDは更新されず、Line 134 のtx.Save(user.Provider)で外部キー不整合が発生します。🔧 修正案
if user.ID == uuid.Nil { // IDを新たに作成する場合 user.ID, err = uuid.NewV4() if err != nil { return err } + user.Provider.UserID = user.ID }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/user.go` around lines 122 - 134, When you generate a new user.ID in the insert flow, ensure the provider's foreign key is synchronized: after setting user.ID (uuid.NewV4()), also set user.Provider.UserID = user.ID (and ensure user.Provider is non-nil) before calling tx.Create(user) or tx.Save(user.Provider); alternatively create/save the Provider via association (e.g., tx.Create(user) with user.Provider.UserID already set) so the Provider's UserID matches the newly generated user.ID and avoids FK mismatch in tx.Save(user.Provider).
🧹 Nitpick comments (4)
infra/db/group.go (1)
88-103:getGroupを2回呼んでいますが、1回にまとめられます。Line 89 で
Preload("Admins")のみ、Line 97 でgroupFullPreloadを呼んでいます。最初からgroupFullPreloadを使用すれば、DBアクセスを1回減らせます。AdminsValidationはAdminsのみを必要とするため、full preload でも動作します。♻️ 改善案
func validateGroup(db *gorm.DB, g *Group) (err error) { - group, err := getGroup(db.Preload("Admins"), g.ID) + group, err := getGroup(groupFullPreload(db), g.ID) if err != nil { return err } Dgroup := ConvGroupTodomainGroup(*group) if !Dgroup.AdminsValidation() { return NewValueError(ErrNoAdmins, "admins") } - group, err = getGroup(groupFullPreload(db), g.ID) - if err != nil { - return err - } *g = *group return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/group.go` around lines 88 - 103, The validateGroup function calls getGroup twice (once with db.Preload("Admins") and again with groupFullPreload), so change it to a single call: call getGroup with groupFullPreload(db) and reuse that result for ConvGroupTodomainGroup and the AdminsValidation check, then assign *g = *group and return; keep existing error handling unchanged. This removes the redundant DB fetch while still providing Admins to ConvGroupTodomainGroup/AdminsValidation.infra/db/hooks.go (3)
8-18: 設計意図のコメントは参考になりますが、削除を検討してください。このコメントブロックはリファクタリングの意図を説明していますが、コードがマージされた後は不要になります。PR説明やコミットメッセージに記載し、コードからは削除することを推奨します。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/hooks.go` around lines 8 - 18, Remove the large design-intent comment block (the multi-line Japanese comment starting with "やりたいこと" and ending with "AfterSave") from infra/db/hooks.go; move any necessary details into the PR description or a commit message instead so the source file contains only implementation-relevant comments and not high-level refactor notes.
105-123: Event と Room は AfterSave が有効で、Group は無効という非一貫性があります。
Event.AfterSaveとRoom.AfterSaveは「dbが正しく動作しているかの確認」として残されていますが、Group.AfterSave（Lines 255-270）はコメントアウトされています。group.goのvalidateGroupが代替として機能しますが、設計の一貫性の観点から、同じパターンを適用することを検討してください。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/hooks.go` around lines 105 - 123, There is inconsistency: Event.AfterSave and Room.AfterSave run DB-level post-save checks while Group.AfterSave is commented out and uses validateGroup instead; pick one pattern and apply it consistently—either re-enable Group.AfterSave and implement the same logic as Event.AfterSave (use getGroup/getGroupFullPreload, convert to domain object and call AdminsValidation, returning NewValueError on failure and reloading full preload into the saved struct) or refactor Event.AfterSave and Room.AfterSave to call a shared validateGroup-like function (extract the validation logic from AfterSave into validateEvent/validateRoom) and remove AfterSave hooks; update references to getEvent, eventFullPreload, Group.AfterSave, validateGroup accordingly.
312-322: Token 保存時の暗号化パターンを強制する構造的改善を検討してください。
Token.BeforeSaveが無効化され、暗号化ロジックがsaveToken関数（infra/db/token.go:83-93）に移行しました。現在のコードベースではsaveTokenが 2 箇所（infra/db/user.go:140, 160）で正しく使用されており、トークンは暗号化して保存されています。ただし、将来的に
db.Saveやdb.Createを直接使用するコードが追加された場合、トークンが平文で保存されるリスクがあります。以下のいずれかで対策を検討してください：
Token構造体に保存メソッドを追加し、直接の GORM 操作を防止する- コードレビューガイドラインに
saveTokenの使用を明記する🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/db/hooks.go` around lines 312 - 322, Reinstate structural encryption by re-enabling Token.BeforeSave to automatically encrypt Token.AccessToken (use encryptByGCM(tokenKey, t.AccessToken) and set t.AccessToken to the ciphertext) so any direct GORM Create/Save will encrypt; additionally add a Token.Save(db *gorm.DB) method that calls db.Save(t) (or db.Create when new) and update callers to use saveToken -> Token.Save, plus add a comment in infra/db/token.go documenting that AccessToken must not be written directly and that Token.BeforeSave enforces encryption.
🤖 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 `@infra/db/group.go`:
- Around line 17-21: CreateGroup dereferences g even when createGroup returns an
error, causing a nil pointer panic; update CreateGroup to check err immediately
after calling createGroup and return nil with defaultErrorHandling(err) if err
!= nil, only calling convGroupTodomainGroup(*g) and returning the domain group
when err is nil; reference CreateGroup, createGroup, convGroupTodomainGroup, and
defaultErrorHandling to locate and fix the logic flow.
- Around line 23-27: The UpdateGroup method dereferences g without checking err
or g, risking a nil pointer if updateGroup returned an error; modify UpdateGroup
to check if err != nil (and/or if g == nil) immediately after calling
updateGroup and return nil with defaultErrorHandling(err) (or a suitable
not-found error) before calling convGroupTodomainGroup, ensuring
convGroupTodomainGroup is only called with a non-nil *g.
- Around line 44-48: GetGroup currently calls convGroupTodomainGroup(*g) before
checking the error from getGroup, which can dereference a nil pointer and
converts an empty Group on error; modify GetGroup to check err (and whether g is
nil) immediately after calling getGroup (use the returned err with
defaultErrorHandling(err) and return early on error) and only call
convGroupTodomainGroup(g) when err is nil (reference functions: GetGroup,
getGroup, convGroupTodomainGroup, defaultErrorHandling).
In `@infra/db/user.go`:
- Around line 36-38: When saveUser returns an error you currently dereference u
causing a nil-pointer panic; update the caller around saveUser(repo.db, &user)
to check err before using u (e.g., if err != nil return nil,
defaultErrorHandling(err)), and only call convUserTodomainUser(*u) when err is
nil. Ensure you reference the variables and functions saveUser, u, err,
convUserTodomainUser, and defaultErrorHandling so the nil-check is placed
immediately after the saveUser call.
- Around line 46-49: GetUser currently dereferences u before checking for errors
from getUser, risking a nil-pointer; change GetUser (gormRepository.GetUser) to
check the error (and that u != nil) immediately after calling
getUser(userPreload(repo.db), userID) and if err != nil (or u == nil) return
nil, defaultErrorHandling(err) instead of continuing to convUserTodomainUser;
only call convUserTodomainUser(*u) and return &du, nil when the call succeeded.
---
Duplicate comments:
In `@infra/db/group.go`:
- Around line 134-182: The updateGroup function performs multiple separate
writes (deleting GroupMember/GroupAdmin, saving Group, then saving
Members/Admins) and must be wrapped in a DB transaction to ensure atomicity;
change updateGroup to run all DB operations inside a gorm transaction (use
db.Transaction or Begin/Commit/Rollback) so that the deletions, group save,
member/admin saves and the call to validateGroup all use the same tx and are
rolled back on any error, and return the transaction error if it fails; update
references inside the transaction to use the transaction handle (tx) instead of
db and ensure validateGroup is called against tx (or adapted to accept a
*gorm.DB) so the whole sequence is committed or rolled back together.
- Around line 197-213: deleteGroup performs three separate deletes (GroupMember,
GroupAdmin, Group) without a transaction, risking partial deletes on failure;
wrap the sequence in a single GORM transaction using db.Transaction(func(tx
*gorm.DB) error { ... }) and replace db with tx for the three operations (the
deletes for GroupMember, GroupAdmin and the final Delete(&group)), returning any
error from inside the transaction so GORM will rollback on error and commit only
if all deletes succeed.
In `@infra/db/user.go`:
- Around line 153-157: The code unconditionally runs tx.Updates(user.Provider)
which can send an empty Provider and violate invariants; change the logic in the
update path to only call tx.Updates when user.Provider is present and contains
meaningful fields (e.g. guard that user.Provider != nil and its key fields are
non-empty) — for the SyncUsers/getAllUsers path ensure you check user.Provider
before calling tx.Updates(user.Provider) (or skip/omit provider updates or use
conditional field updates via tx.Model(&user).Updates(...)) so
empty/preload-missing Provider structs are not written to DB.
- Around line 122-134: When you generate a new user.ID in the insert flow,
ensure the provider's foreign key is synchronized: after setting user.ID
(uuid.NewV4()), also set user.Provider.UserID = user.ID (and ensure
user.Provider is non-nil) before calling tx.Create(user) or
tx.Save(user.Provider); alternatively create/save the Provider via association
(e.g., tx.Create(user) with user.Provider.UserID already set) so the Provider's
UserID matches the newly generated user.ID and avoids FK mismatch in
tx.Save(user.Provider).
---
Nitpick comments:
In `@infra/db/group.go`:
- Around line 88-103: The validateGroup function calls getGroup twice (once with
db.Preload("Admins") and again with groupFullPreload), so change it to a single
call: call getGroup with groupFullPreload(db) and reuse that result for
ConvGroupTodomainGroup and the AdminsValidation check, then assign *g = *group
and return; keep existing error handling unchanged. This removes the redundant
DB fetch while still providing Admins to
ConvGroupTodomainGroup/AdminsValidation.
In `@infra/db/hooks.go`:
- Around line 8-18: Remove the large design-intent comment block (the multi-line
Japanese comment starting with "やりたいこと" and ending with "AfterSave") from
infra/db/hooks.go; move any necessary details into the PR description or a
commit message instead so the source file contains only implementation-relevant
comments and not high-level refactor notes.
- Around line 105-123: There is inconsistency: Event.AfterSave and
Room.AfterSave run DB-level post-save checks while Group.AfterSave is commented
out and uses validateGroup instead; pick one pattern and apply it
consistently—either re-enable Group.AfterSave and implement the same logic as
Event.AfterSave (use getGroup/getGroupFullPreload, convert to domain object and
call AdminsValidation, returning NewValueError on failure and reloading full
preload into the saved struct) or refactor Event.AfterSave and Room.AfterSave to
call a shared validateGroup-like function (extract the validation logic from
AfterSave into validateEvent/validateRoom) and remove AfterSave hooks; update
references to getEvent, eventFullPreload, Group.AfterSave, validateGroup
accordingly.
- Around line 312-322: Reinstate structural encryption by re-enabling
Token.BeforeSave to automatically encrypt Token.AccessToken (use
encryptByGCM(tokenKey, t.AccessToken) and set t.AccessToken to the ciphertext)
so any direct GORM Create/Save will encrypt; additionally add a Token.Save(db
*gorm.DB) method that calls db.Save(t) (or db.Create when new) and update
callers to use saveToken -> Token.Save, plus add a comment in infra/db/token.go
documenting that AccessToken must not be written directly and that
Token.BeforeSave enforces encryption.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 686a9c95-f991-4578-96e2-48f1a03c48c8
📒 Files selected for processing (3)
infra/db/group.goinfra/db/hooks.goinfra/db/user.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
2 participants
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.
db/model.go で参照関係がよく読めればFullSaveAssociationsはあっても良さそうなんですが、現状だと分かりにくいので明示的に変更 or modelにコメントを多数追加をしたいところです。
現在は
を書き換えています。
Summary by CodeRabbit
リリースノート
New Features
Bug Fixes
Improvements