fix: 部屋のステータスが変更されていないときログを残さない #320#353
Open
ynta-3 wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
repository/gormrepository/room_status_test.go (1)
130-155: 回帰テストは良いです。Type=nil同値ケースも追加するとさらに堅いです今回の実装は
Typeのnil分岐を含むため、Type:nilかつTopic同一で2回更新して履歴が増えないケースも追加しておくと、将来の回帰を防ぎやすいです。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@repository/gormrepository/room_status_test.go` around lines 130 - 155, Add a regression subcase covering the Type=nil equality path: in the test "同じ内容で更新しても履歴は増えない" create a model.RoomStatus with Type set to nil and Topic set to a random topic, call mustSetRoomStatus(t, r, room.ID, status, operatorID) then call r.SetRoomStatus(t.Context(), room.ID, status, operatorID) and assert no error and that r.GetRoomStatusLogs(t.Context(), room.ID) returns exactly one log entry; this ensures the same-content-with-nil-Type branch in mustSetRoomStatus / SetRoomStatus is tested.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@repository/gormrepository/room_status.go`:
- Around line 22-31: 現在の実装は currentStatus を先に読んでから roomStatusContentEquals
で判定し別操作で更新しているため TOCTOU が発生します。読取→判定→更新の一連をトランザクション内で部屋行ロックして直列化するように修正してください:
tx 上で gorm.G[model.RoomStatus](tx).Clauses(clause.Locking{Strength:"UPDATE"})
を使って room_id = ? の行を SELECT ... FOR UPDATE で取得し、そのロック下で
roomStatusContentEquals(currentStatus, status) を評価してから履歴追加/更新処理を行う（roomID と
currentStatus を扱う全ての分岐をこのロック内に移動する）。
---
Nitpick comments:
In `@repository/gormrepository/room_status_test.go`:
- Around line 130-155: Add a regression subcase covering the Type=nil equality
path: in the test "同じ内容で更新しても履歴は増えない" create a model.RoomStatus with Type set to
nil and Topic set to a random topic, call mustSetRoomStatus(t, r, room.ID,
status, operatorID) then call r.SetRoomStatus(t.Context(), room.ID, status,
operatorID) and assert no error and that r.GetRoomStatusLogs(t.Context(),
room.ID) returns exactly one log entry; this ensures the
same-content-with-nil-Type branch in mustSetRoomStatus / SetRoomStatus is
tested.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 898e12b1-8f92-43be-aa14-103059e503e1
📒 Files selected for processing (2)
repository/gormrepository/room_status.gorepository/gormrepository/room_status_test.go
Comment on lines
+22
to
+31
| currentStatus, err := gorm.G[model.RoomStatus](tx). | ||
| Where("room_id = ?", roomID). | ||
| Take(ctx) | ||
|
|
||
| if err != nil && !errors.Is(err, gorm.ErrRecordNotFound) { | ||
| return err | ||
| } | ||
|
|
||
| if err == nil && roomStatusContentEquals(currentStatus, status) { | ||
| return nil |
There was a problem hiding this comment.
同時更新時に履歴が重複しうる TOCTOU があります
Line 22 での読取結果を Line 30 で判定してから別操作で更新しているため、同一 roomID への同時リクエストで両方がログを書けます。
「内容が未変更なら履歴を増やさない」という要件を競合時にも満たすには、部屋単位で直列化してください。
🔧 修正案（部屋行ロックで直列化）
return r.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error {
+ if _, err := gorm.G[model.Room](tx).
+ Clauses(clause.Locking{Strength: "UPDATE"}).
+ Where("id = ?", roomID).
+ Take(ctx); err != nil {
+ if errors.Is(err, gorm.ErrRecordNotFound) {
+ return repository.ErrRoomNotFound
+ }
+ return err
+ }
+
currentStatus, err := gorm.G[model.RoomStatus](tx).
Where("room_id = ?", roomID).
Take(ctx)
if err != nil && !errors.Is(err, gorm.ErrRecordNotFound) {
return err
}
if err == nil && roomStatusContentEquals(currentStatus, status) {
return nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@repository/gormrepository/room_status.go` around lines 22 - 31, 現在の実装は
currentStatus を先に読んでから roomStatusContentEquals で判定し別操作で更新しているため TOCTOU
が発生します。読取→判定→更新の一連をトランザクション内で部屋行ロックして直列化するように修正してください: tx 上で
gorm.G[model.RoomStatus](tx).Clauses(clause.Locking{Strength:"UPDATE"}) を使って
room_id = ? の行を SELECT ... FOR UPDATE で取得し、そのロック下で
roomStatusContentEquals(currentStatus, status) を評価してから履歴追加/更新処理を行う（roomID と
currentStatus を扱う全ての分岐をこのロック内に移動する）。
akimon658
requested changes
Jun 6, 2026
akimon658
left a comment
akimon658
left a comment
Member
There was a problem hiding this comment.
めちゃくちゃレビュー遅くなりましたが方針は良さそうです！
CodeRabbitの指摘も修正してもらえると 🙏 （修正案のコードが正しいかは分からないのでよしなに）
| assert.NoError(t, err) | ||
|
|
||
| logs, err := r.GetRoomStatusLogs(t.Context(), room.ID) | ||
| assert.NoError(t, err) |
Member
There was a problem hiding this comment.
すごく細かいことですが、ここはrequire.NoErrorを使ってほしいかもです
今テストしたい（assertしたい）のはSetRoomStatusの挙動とログの件数であり、GetRoomStatusLogsがエラーを返さず成功するのはテストの前提であるためです
Suggested change
| assert.NoError(t, err) | |
| require.NoError(t, err) |
Comment on lines
+26
to
+28
| if err != nil && !errors.Is(err, gorm.ErrRecordNotFound) { | ||
| return err | ||
| } |
Member
There was a problem hiding this comment.
Suggested change
| if err != nil && !errors.Is(err, gorm.ErrRecordNotFound) { | |
| return err | |
| } | |
| if err != nil { | |
| if errors.Is(err, gorm.ErrRecordNotFound) { | |
| return repository.ErrRoomNotFound | |
| } | |
| return err | |
| } |
こうした上で、次のif文のerr == nilや44行目の条件分岐を消すとより流れが分かりやすくなると思います
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.
close #320
Summary by CodeRabbit
リリースノート
バグ修正
テスト