Skip to content
Open
Show file tree
Hide file tree
Changes from 22 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions domain/event.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ type WriteEventParams struct {
Open bool
}

func (e *WriteEventParams) TimeConsistency() bool {
return e.TimeStart.Before((e.TimeEnd))
}

type EventTagParams struct {
Name string
Locked bool
Expand Down
4 changes: 4 additions & 0 deletions domain/room.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,10 @@ type WriteRoomParams struct {
Admins []uuid.UUID
}

func (r *WriteRoomParams) TimeConsistency() bool {
return r.TimeStart.Before((r.TimeEnd))
}

type RoomService interface {
CreateUnVerifiedRoom(ctx context.Context, reqID uuid.UUID, params WriteRoomParams) (*Room, error)
CreateVerifiedRoom(ctx context.Context, reqID uuid.UUID, params WriteRoomParams) (*Room, error)
Expand Down
1 change: 1 addition & 0 deletions infra/db/converter.go
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,7 @@ func convTagTodomainTag(src Tag) (dst domain.Tag) {
func convUserTodomainUser(src User) (dst domain.User) {
dst.ID = src.ID
dst.State = src.State
dst.Privileged = src.Privilege
dst.Provider = &domain.Provider{
Issuer: src.Provider.Issuer,
Subject: src.Provider.Subject,
Expand Down
81 changes: 77 additions & 4 deletions infra/db/event.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,31 +76,104 @@

func createEvent(db *gorm.DB, args domain.UpsertEventArgs) (*Event, error) {
event := ConvWriteEventParamsToEvent(args)
var err error
var r *Room
r, err = getRoom(db, event.RoomID)
if err != nil {
return nil, err
}
Comment on lines +81 to +84
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

nil 返却が呼び出し側の panic 経路になっています。

CreateEvent / UpdateEvent はこの戻り値を err チェック前にデリファレンスしています。ここで nil, err を返すと、getRoomcreateOrGetTag の失敗がそのまま 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.

event.Room = *r

// tagを生成する
for i := range event.Tags {
tag, err := createOrGetTag(db, event.Tags[i].Tag.Name)
if err != nil {
return nil, err
}
event.Tags[i].EventID = event.ID
event.Tags[i].TagID = tag.ID
event.Tags[i].Tag = *tag
}

err := db.Create(&event).Error
// 対応する Room, Group はService層で確認済み
event.ID, err = uuid.NewV4()
if err != nil {
return nil, err
}
err = db.Create(&event).Error
return &event, err
}

func updateEvent(db *gorm.DB, eventID uuid.UUID, args domain.UpsertEventArgs) (*Event, error) {
event := ConvWriteEventParamsToEvent(args)
event.ID = eventID
var err error
var r *Room
r, err = getRoom(db, event.RoomID)
if err != nil {
return nil, err
}
event.Room = *r

// tagは自動生成する
for i := range event.Tags {
tag, err := createOrGetTag(db, event.Tags[i].Tag.Name)
if err != nil {
return nil, err
}
event.Tags[i].EventID = event.ID
event.Tags[i].TagID = tag.ID
}

// 対応する Room, Group はService層で確認済み
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
}
}
Comment on lines +132 to +154
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

削除された 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.


err := db.Session(&gorm.Session{FullSaveAssociations: true}).
Omit("CreatedAt").Save(&event).Error
return &event, err
}

func addEventTag(db *gorm.DB, eventID uuid.UUID, params domain.EventTagParams) error {
eventTag := ConvdomainEventTagParamsToEventTag(params)
eventTag.EventID = eventID
err := db.Create(&eventTag).Error
tag, err := createOrGetTag(db, eventTag.Tag.Name)

Check failure on line 159 in infra/db/event.go

View workflow job for this annotation

GitHub Actions / Lint

ineffectual assignment to err (ineffassign)
eventTag.TagID = tag.ID
err = db.Create(&eventTag).Error
Comment on lines +162 to +167
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

if errors.Is(defaultErrorHandling(err), ErrDuplicateEntry) {
return db.Omit("CreatedAt").Save(&eventTag).Error
}
return err
}

func deleteEvent(db *gorm.DB, eventID uuid.UUID) error {
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
Comment on lines +175 to 183
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

依存行削除と 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.

}

Expand Down
59 changes: 55 additions & 4 deletions infra/db/group.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,13 @@ func convSPGroupToSuuidUUID(src []*Group) (dst []uuid.UUID) {

func createGroup(db *gorm.DB, args domain.UpsertGroupArgs) (*Group, error) {
group := ConvWriteGroupParamsToGroup(args)
err := db.Create(&group).Error
// IDを新規発行
var err error
group.ID, err = uuid.NewV4()
if err != nil {
return nil, err
}
err = db.Create(&group).Error
if err != nil {
return nil, err
}
Expand All @@ -107,8 +113,41 @@ func createGroup(db *gorm.DB, args domain.UpsertGroupArgs) (*Group, error) {
func updateGroup(db *gorm.DB, groupID uuid.UUID, args domain.UpsertGroupArgs) (*Group, error) {
group := ConvWriteGroupParamsToGroup(args)
group.ID = groupID
err := db.Session(&gorm.Session{FullSaveAssociations: true}).
Omit("CreatedAt").Save(&group).Error
// groupIDは存在する
var err error
// CreatedBy は readonly
// GroupMember, GroupAdmin も User は readonly

// err = db.Session(&gorm.Session{FullSaveAssociations: true}).
// Omit("CreatedAt").Save(&group).Error

// 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 {
err = db.Save(&member).Error
if err != nil {
return nil, err
}
}
// GroupAdmin の更新
for _, admin := range group.Admins {
err := db.Save(&admin).Error
if err != nil {
return nil, err
}
}
return &group, err
Comment on lines +145 to 181
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

}

Expand All @@ -121,14 +160,25 @@ func addMemberToGroup(db *gorm.DB, groupID, userID uuid.UUID) error {
onConflictClause := clause.OnConflict{
DoUpdates: clause.AssignmentColumns([]string{"updated_at", "deleted_at"}),
}

// groupMemberはhook登録なし
return db.Clauses(onConflictClause).Create(&groupMember).Error
}

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
Comment on lines +201 to 212
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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 197 to 213
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


Expand All @@ -137,6 +187,7 @@ func deleteMemberOfGroup(db *gorm.DB, groupID, userID uuid.UUID) error {
GroupID: groupID,
UserID: userID,
}
// 1メンバーの削除 hooksは登録なし
return db.Delete(&groupMember).Error
}

Expand Down
Loading
Loading