refactor(channelserver): eliminate *sql.Tx from repository interfaces

Hide transaction management inside repository implementations so
interfaces only expose domain types, enabling clean mocking and
decoupling handlers from PostgreSQL internals.

- Replace BeginTx + UpdateEventQuestStartTime with batch
  UpdateEventQuestStartTimes that manages its own transaction
- Remove tx parameter from CreateApplication, add composite
  CreateApplicationWithMail for atomic scout+mail operations
- Remove SendMailTx from MailRepo (sole caller migrated)
- Remove database/sql import from repo_interfaces.go
This commit is contained in:
Houmgaor
2026-02-21 14:56:59 +01:00
parent 35d8471d59
commit 6fbd294575
10 changed files with 130 additions and 124 deletions

View File

@@ -60,7 +60,7 @@ func handleMsgMhfOperateGuild(s *Session, p mhfpacket.MHFPacket) {
_ = s.server.guildRepo.Save(guild) _ = s.server.guildRepo.Save(guild)
} }
case mhfpacket.OperateGuildApply: case mhfpacket.OperateGuildApply:
err = s.server.guildRepo.CreateApplication(guild.ID, s.charID, s.charID, GuildApplicationTypeApplied, nil) err = s.server.guildRepo.CreateApplication(guild.ID, s.charID, s.charID, GuildApplicationTypeApplied)
if err == nil { if err == nil {
bf.WriteUint32(guild.LeaderCharID) bf.WriteUint32(guild.LeaderCharID)
} else { } else {

View File

@@ -45,38 +45,14 @@ func handleMsgMhfPostGuildScout(s *Session, p mhfpacket.MHFPacket) {
return return
} }
transaction, err := s.server.db.Begin() err = s.server.guildRepo.CreateApplicationWithMail(
guildInfo.ID, pkt.CharID, s.charID, GuildApplicationTypeInvited,
if err != nil { s.charID, pkt.CharID,
s.logger.Error("Failed to begin transaction for guild scout", zap.Error(err))
doAckBufFail(s, pkt.AckHandle, nil)
return
}
err = s.server.guildRepo.CreateApplication(guildInfo.ID, pkt.CharID, s.charID, GuildApplicationTypeInvited, transaction)
if err != nil {
_ = transaction.Rollback()
s.logger.Error("Failed to create guild scout application", zap.Error(err))
doAckBufFail(s, pkt.AckHandle, nil)
return
}
err = s.server.mailRepo.SendMailTx(transaction, s.charID, pkt.CharID,
s.server.i18n.guild.invite.title, s.server.i18n.guild.invite.title,
fmt.Sprintf(s.server.i18n.guild.invite.body, guildInfo.Name), fmt.Sprintf(s.server.i18n.guild.invite.body, guildInfo.Name))
0, 0, true, false)
if err != nil { if err != nil {
_ = transaction.Rollback() s.logger.Error("Failed to create guild scout application with mail", zap.Error(err))
doAckBufFail(s, pkt.AckHandle, nil)
return
}
err = transaction.Commit()
if err != nil {
s.logger.Error("Failed to commit guild scout transaction", zap.Error(err))
doAckBufFail(s, pkt.AckHandle, nil) doAckBufFail(s, pkt.AckHandle, nil)
return return
} }

View File

@@ -342,12 +342,7 @@ func handleMsgMhfEnumerateQuest(s *Session, p mhfpacket.MHFPacket) {
quests, err := s.server.eventRepo.GetEventQuests() quests, err := s.server.eventRepo.GetEventQuests()
if err == nil { if err == nil {
currentTime := time.Now() currentTime := time.Now()
tx, err := s.server.eventRepo.BeginTx() var updates []EventQuestUpdate
if err != nil {
s.logger.Error("Failed to begin transaction for event quests", zap.Error(err))
doAckBufSucceed(s, pkt.AckHandle, bf.Data())
return
}
for i, eq := range quests { for i, eq := range quests {
// Use the Event Cycling system // Use the Event Cycling system
@@ -364,11 +359,7 @@ func handleMsgMhfEnumerateQuest(s *Session, p mhfpacket.MHFPacket) {
// Normalize rotationTime to 12PM JST to align with the in-game events update notification. // Normalize rotationTime to 12PM JST to align with the in-game events update notification.
newRotationTime := time.Date(rotationTime.Year(), rotationTime.Month(), rotationTime.Day(), 12, 0, 0, 0, TimeAdjusted().Location()) newRotationTime := time.Date(rotationTime.Year(), rotationTime.Month(), rotationTime.Day(), 12, 0, 0, 0, TimeAdjusted().Location())
err = s.server.eventRepo.UpdateEventQuestStartTime(tx, eq.ID, newRotationTime) updates = append(updates, EventQuestUpdate{ID: eq.ID, StartTime: newRotationTime})
if err != nil {
_ = tx.Rollback()
break
}
quests[i].StartTime = newRotationTime // Set the new start time so the quest can be used/removed immediately. quests[i].StartTime = newRotationTime // Set the new start time so the quest can be used/removed immediately.
eq = quests[i] eq = quests[i]
} }
@@ -399,7 +390,9 @@ func handleMsgMhfEnumerateQuest(s *Session, p mhfpacket.MHFPacket) {
} }
} }
_ = tx.Commit() if err := s.server.eventRepo.UpdateEventQuestStartTimes(updates); err != nil {
s.logger.Error("Failed to update event quest start times", zap.Error(err))
}
} }
tuneValues := []tuneValue{ tuneValues := []tuneValue{

View File

@@ -1,7 +1,6 @@
package channelserver package channelserver
import ( import (
"database/sql"
"time" "time"
"github.com/jmoiron/sqlx" "github.com/jmoiron/sqlx"
@@ -69,13 +68,26 @@ func (r *EventRepository) GetEventQuests() ([]EventQuest, error) {
return result, err return result, err
} }
// UpdateEventQuestStartTime updates the start_time for an event quest within a transaction. // EventQuestUpdate pairs a quest ID with its new start time.
func (r *EventRepository) UpdateEventQuestStartTime(tx *sql.Tx, id uint32, startTime time.Time) error { type EventQuestUpdate struct {
_, err := tx.Exec("UPDATE event_quests SET start_time = $1 WHERE id = $2", startTime, id) ID uint32
return err StartTime time.Time
} }
// BeginTx starts a new database transaction. // UpdateEventQuestStartTimes batch-updates start times within a single transaction.
func (r *EventRepository) BeginTx() (*sql.Tx, error) { func (r *EventRepository) UpdateEventQuestStartTimes(updates []EventQuestUpdate) error {
return r.db.Begin() if len(updates) == 0 {
return nil
}
tx, err := r.db.Begin()
if err != nil {
return err
}
for _, u := range updates {
if _, err := tx.Exec("UPDATE event_quests SET start_time = $1 WHERE id = $2", u.StartTime, u.ID); err != nil {
_ = tx.Rollback()
return err
}
}
return tx.Commit()
} }

View File

@@ -96,65 +96,51 @@ func TestGetEventQuestsOrderByQuestID(t *testing.T) {
} }
} }
func TestBeginTxAndUpdateEventQuestStartTime(t *testing.T) { func TestUpdateEventQuestStartTimes(t *testing.T) {
repo, db := setupEventRepo(t) repo, db := setupEventRepo(t)
originalTime := time.Date(2025, 1, 1, 12, 0, 0, 0, time.UTC) originalTime := time.Date(2025, 1, 1, 12, 0, 0, 0, time.UTC)
questID := insertEventQuest(t, db, 1, 100, originalTime, 7, 3) id1 := insertEventQuest(t, db, 1, 100, originalTime, 7, 3)
id2 := insertEventQuest(t, db, 2, 200, originalTime, 5, 2)
newTime := time.Date(2025, 6, 15, 12, 0, 0, 0, time.UTC) newTime1 := time.Date(2025, 6, 15, 12, 0, 0, 0, time.UTC)
newTime2 := time.Date(2025, 7, 20, 12, 0, 0, 0, time.UTC)
tx, err := repo.BeginTx() err := repo.UpdateEventQuestStartTimes([]EventQuestUpdate{
{ID: id1, StartTime: newTime1},
{ID: id2, StartTime: newTime2},
})
if err != nil { if err != nil {
t.Fatalf("BeginTx failed: %v", err) t.Fatalf("UpdateEventQuestStartTimes failed: %v", err)
} }
if err := repo.UpdateEventQuestStartTime(tx, questID, newTime); err != nil { // Verify both updates
_ = tx.Rollback() var got1, got2 time.Time
t.Fatalf("UpdateEventQuestStartTime failed: %v", err) if err := db.QueryRow("SELECT start_time FROM event_quests WHERE id=$1", id1).Scan(&got1); err != nil {
t.Fatalf("Verification query failed for id1: %v", err)
} }
if !got1.Equal(newTime1) {
if err := tx.Commit(); err != nil { t.Errorf("Expected start_time %v for id1, got: %v", newTime1, got1)
t.Fatalf("Commit failed: %v", err)
} }
if err := db.QueryRow("SELECT start_time FROM event_quests WHERE id=$1", id2).Scan(&got2); err != nil {
// Verify the update t.Fatalf("Verification query failed for id2: %v", err)
var got time.Time
if err := db.QueryRow("SELECT start_time FROM event_quests WHERE id=$1", questID).Scan(&got); err != nil {
t.Fatalf("Verification query failed: %v", err)
} }
if !got.Equal(newTime) { if !got2.Equal(newTime2) {
t.Errorf("Expected start_time %v, got: %v", newTime, got) t.Errorf("Expected start_time %v for id2, got: %v", newTime2, got2)
} }
} }
func TestUpdateEventQuestStartTimeRollback(t *testing.T) { func TestUpdateEventQuestStartTimesEmpty(t *testing.T) {
repo, db := setupEventRepo(t) repo, _ := setupEventRepo(t)
originalTime := time.Date(2025, 1, 1, 12, 0, 0, 0, time.UTC) // Empty slice should be a no-op
questID := insertEventQuest(t, db, 1, 100, originalTime, 0, 0) err := repo.UpdateEventQuestStartTimes(nil)
tx, err := repo.BeginTx()
if err != nil { if err != nil {
t.Fatalf("BeginTx failed: %v", err) t.Fatalf("UpdateEventQuestStartTimes with nil should not error, got: %v", err)
} }
newTime := time.Date(2025, 6, 15, 12, 0, 0, 0, time.UTC) err = repo.UpdateEventQuestStartTimes([]EventQuestUpdate{})
if err := repo.UpdateEventQuestStartTime(tx, questID, newTime); err != nil { if err != nil {
t.Fatalf("UpdateEventQuestStartTime failed: %v", err) t.Fatalf("UpdateEventQuestStartTimes with empty slice should not error, got: %v", err)
}
// Rollback instead of commit
if err := tx.Rollback(); err != nil {
t.Fatalf("Rollback failed: %v", err)
}
// Verify original time is preserved
var got time.Time
if err := db.QueryRow("SELECT start_time FROM event_quests WHERE id=$1", questID).Scan(&got); err != nil {
t.Fatalf("Verification query failed: %v", err)
}
if !got.Equal(originalTime) {
t.Errorf("Expected original start_time %v after rollback, got: %v", originalTime, got)
} }
} }

View File

@@ -270,15 +270,30 @@ func (r *GuildRepository) AcceptApplication(guildID, charID uint32) error {
} }
// CreateApplication inserts a guild application or invitation. // CreateApplication inserts a guild application or invitation.
// If tx is non-nil, the operation participates in the given transaction. func (r *GuildRepository) CreateApplication(guildID, charID, actorID uint32, appType GuildApplicationType) error {
func (r *GuildRepository) CreateApplication(guildID, charID, actorID uint32, appType GuildApplicationType, tx *sql.Tx) error { _, err := r.db.Exec(
query := `INSERT INTO guild_applications (guild_id, character_id, actor_id, application_type) VALUES ($1, $2, $3, $4)` `INSERT INTO guild_applications (guild_id, character_id, actor_id, application_type) VALUES ($1, $2, $3, $4)`,
if tx != nil { guildID, charID, actorID, appType)
_, err := tx.Exec(query, guildID, charID, actorID, appType) return err
}
// CreateApplicationWithMail atomically creates an application and sends a notification mail.
func (r *GuildRepository) CreateApplicationWithMail(guildID, charID, actorID uint32, appType GuildApplicationType, mailSenderID, mailRecipientID uint32, mailSubject, mailBody string) error {
tx, err := r.db.Begin()
if err != nil {
return err return err
} }
_, err := r.db.Exec(query, guildID, charID, actorID, appType) if _, err := tx.Exec(
return err `INSERT INTO guild_applications (guild_id, character_id, actor_id, application_type) VALUES ($1, $2, $3, $4)`,
guildID, charID, actorID, appType); err != nil {
_ = tx.Rollback()
return err
}
if _, err := tx.Exec(mailInsertQuery, mailSenderID, mailRecipientID, mailSubject, mailBody, 0, 0, true, false); err != nil {
_ = tx.Rollback()
return err
}
return tx.Commit()
} }
// CancelInvitation removes an invitation for a character. // CancelInvitation removes an invitation for a character.

View File

@@ -270,7 +270,7 @@ func TestApplicationWorkflow(t *testing.T) {
applicantID := CreateTestCharacter(t, db, user2, "Applicant") applicantID := CreateTestCharacter(t, db, user2, "Applicant")
// Create application // Create application
err := repo.CreateApplication(guildID, applicantID, applicantID, GuildApplicationTypeApplied, nil) err := repo.CreateApplication(guildID, applicantID, applicantID, GuildApplicationTypeApplied)
if err != nil { if err != nil {
t.Fatalf("CreateApplication failed: %v", err) t.Fatalf("CreateApplication failed: %v", err)
} }
@@ -324,7 +324,7 @@ func TestRejectApplication(t *testing.T) {
user2 := CreateTestUser(t, db, "reject_user") user2 := CreateTestUser(t, db, "reject_user")
applicantID := CreateTestCharacter(t, db, user2, "Rejected") applicantID := CreateTestCharacter(t, db, user2, "Rejected")
err := repo.CreateApplication(guildID, applicantID, applicantID, GuildApplicationTypeApplied, nil) err := repo.CreateApplication(guildID, applicantID, applicantID, GuildApplicationTypeApplied)
if err != nil { if err != nil {
t.Fatalf("CreateApplication failed: %v", err) t.Fatalf("CreateApplication failed: %v", err)
} }
@@ -539,7 +539,7 @@ func TestCancelInvitation(t *testing.T) {
user2 := CreateTestUser(t, db, "invite_user") user2 := CreateTestUser(t, db, "invite_user")
char2 := CreateTestCharacter(t, db, user2, "Invited") char2 := CreateTestCharacter(t, db, user2, "Invited")
if err := repo.CreateApplication(guildID, char2, leaderID, GuildApplicationTypeInvited, nil); err != nil { if err := repo.CreateApplication(guildID, char2, leaderID, GuildApplicationTypeInvited); err != nil {
t.Fatalf("CreateApplication (invited) failed: %v", err) t.Fatalf("CreateApplication (invited) failed: %v", err)
} }
@@ -562,7 +562,7 @@ func TestListInvitedCharacters(t *testing.T) {
user2 := CreateTestUser(t, db, "scout_user") user2 := CreateTestUser(t, db, "scout_user")
char2 := CreateTestCharacter(t, db, user2, "Scouted") char2 := CreateTestCharacter(t, db, user2, "Scouted")
if err := repo.CreateApplication(guildID, char2, leaderID, GuildApplicationTypeInvited, nil); err != nil { if err := repo.CreateApplication(guildID, char2, leaderID, GuildApplicationTypeInvited); err != nil {
t.Fatalf("CreateApplication failed: %v", err) t.Fatalf("CreateApplication failed: %v", err)
} }
@@ -602,7 +602,7 @@ func TestGetByCharIDWithApplication(t *testing.T) {
user2 := CreateTestUser(t, db, "app_char_user") user2 := CreateTestUser(t, db, "app_char_user")
char2 := CreateTestCharacter(t, db, user2, "Applicant2") char2 := CreateTestCharacter(t, db, user2, "Applicant2")
if err := repo.CreateApplication(guildID, char2, char2, GuildApplicationTypeApplied, nil); err != nil { if err := repo.CreateApplication(guildID, char2, char2, GuildApplicationTypeApplied); err != nil {
t.Fatalf("CreateApplication failed: %v", err) t.Fatalf("CreateApplication failed: %v", err)
} }
@@ -624,7 +624,7 @@ func TestGetMembersApplicants(t *testing.T) {
user2 := CreateTestUser(t, db, "applicant_member_user") user2 := CreateTestUser(t, db, "applicant_member_user")
char2 := CreateTestCharacter(t, db, user2, "AppMember") char2 := CreateTestCharacter(t, db, user2, "AppMember")
if err := repo.CreateApplication(guildID, char2, char2, GuildApplicationTypeApplied, nil); err != nil { if err := repo.CreateApplication(guildID, char2, char2, GuildApplicationTypeApplied); err != nil {
t.Fatalf("CreateApplication failed: %v", err) t.Fatalf("CreateApplication failed: %v", err)
} }
@@ -1485,3 +1485,39 @@ func TestDisbandCleansUpAlliance(t *testing.T) {
t.Errorf("Expected alliance to be deleted after parent guild disband, got: %+v", alliance) t.Errorf("Expected alliance to be deleted after parent guild disband, got: %+v", alliance)
} }
} }
// --- CreateApplicationWithMail ---
func TestCreateApplicationWithMail(t *testing.T) {
repo, db, guildID, leaderID := setupGuildRepo(t)
user2 := CreateTestUser(t, db, "scout_mail_user")
char2 := CreateTestCharacter(t, db, user2, "ScoutTarget")
err := repo.CreateApplicationWithMail(
guildID, char2, leaderID, GuildApplicationTypeInvited,
leaderID, char2, "Guild Invite", "You have been invited!")
if err != nil {
t.Fatalf("CreateApplicationWithMail failed: %v", err)
}
// Verify application was created
has, err := repo.HasApplication(guildID, char2)
if err != nil {
t.Fatalf("HasApplication failed: %v", err)
}
if !has {
t.Error("Expected application to exist after CreateApplicationWithMail")
}
// Verify mail was sent
var mailCount int
if err := db.QueryRow(
"SELECT COUNT(*) FROM mail WHERE sender_id=$1 AND recipient_id=$2 AND subject=$3",
leaderID, char2, "Guild Invite").Scan(&mailCount); err != nil {
t.Fatalf("Mail verification query failed: %v", err)
}
if mailCount != 1 {
t.Errorf("Expected 1 mail row, got %d", mailCount)
}
}

View File

@@ -1,7 +1,6 @@
package channelserver package channelserver
import ( import (
"database/sql"
"time" "time"
) )
@@ -52,7 +51,8 @@ type GuildRepo interface {
Disband(guildID uint32) error Disband(guildID uint32) error
RemoveCharacter(charID uint32) error RemoveCharacter(charID uint32) error
AcceptApplication(guildID, charID uint32) error AcceptApplication(guildID, charID uint32) error
CreateApplication(guildID, charID, actorID uint32, appType GuildApplicationType, tx *sql.Tx) error CreateApplication(guildID, charID, actorID uint32, appType GuildApplicationType) error
CreateApplicationWithMail(guildID, charID, actorID uint32, appType GuildApplicationType, mailSenderID, mailRecipientID uint32, mailSubject, mailBody string) error
CancelInvitation(guildID, charID uint32) error CancelInvitation(guildID, charID uint32) error
RejectApplication(guildID, charID uint32) error RejectApplication(guildID, charID uint32) error
ArrangeCharacters(charIDs []uint32) error ArrangeCharacters(charIDs []uint32) error
@@ -228,7 +228,6 @@ type RengokuRepo interface {
// MailRepo defines the contract for in-game mail data access. // MailRepo defines the contract for in-game mail data access.
type MailRepo interface { type MailRepo interface {
SendMail(senderID, recipientID uint32, subject, body string, itemID, itemAmount uint16, isGuildInvite, isSystemMessage bool) error SendMail(senderID, recipientID uint32, subject, body string, itemID, itemAmount uint16, isGuildInvite, isSystemMessage bool) error
SendMailTx(tx *sql.Tx, senderID, recipientID uint32, subject, body string, itemID, itemAmount uint16, isGuildInvite, isSystemMessage bool) error
GetListForCharacter(charID uint32) ([]Mail, error) GetListForCharacter(charID uint32) ([]Mail, error)
GetByID(id int) (*Mail, error) GetByID(id int) (*Mail, error)
MarkRead(id int) error MarkRead(id int) error
@@ -272,8 +271,7 @@ type EventRepo interface {
InsertLoginBoost(charID uint32, weekReq uint8, expiration, reset time.Time) error InsertLoginBoost(charID uint32, weekReq uint8, expiration, reset time.Time) error
UpdateLoginBoost(charID uint32, weekReq uint8, expiration, reset time.Time) error UpdateLoginBoost(charID uint32, weekReq uint8, expiration, reset time.Time) error
GetEventQuests() ([]EventQuest, error) GetEventQuests() ([]EventQuest, error)
UpdateEventQuestStartTime(tx *sql.Tx, id uint32, startTime time.Time) error UpdateEventQuestStartTimes(updates []EventQuestUpdate) error
BeginTx() (*sql.Tx, error)
} }
// AchievementRepo defines the contract for achievement data access. // AchievementRepo defines the contract for achievement data access.

View File

@@ -1,8 +1,6 @@
package channelserver package channelserver
import ( import (
"database/sql"
"github.com/jmoiron/sqlx" "github.com/jmoiron/sqlx"
) )
@@ -27,12 +25,6 @@ func (r *MailRepository) SendMail(senderID, recipientID uint32, subject, body st
return err return err
} }
// SendMailTx inserts a new mail row within an existing transaction.
func (r *MailRepository) SendMailTx(tx *sql.Tx, senderID, recipientID uint32, subject, body string, itemID, itemAmount uint16, isGuildInvite, isSystemMessage bool) error {
_, err := tx.Exec(mailInsertQuery, senderID, recipientID, subject, body, itemID, itemAmount, isGuildInvite, isSystemMessage)
return err
}
// GetListForCharacter loads all non-deleted mail for a character (max 32). // GetListForCharacter loads all non-deleted mail for a character (max 32).
func (r *MailRepository) GetListForCharacter(charID uint32) ([]Mail, error) { func (r *MailRepository) GetListForCharacter(charID uint32) ([]Mail, error) {
rows, err := r.db.Queryx(` rows, err := r.db.Queryx(`

View File

@@ -1,7 +1,6 @@
package channelserver package channelserver
import ( import (
"database/sql"
"errors" "errors"
"time" "time"
) )
@@ -102,10 +101,6 @@ func (m *mockMailRepo) SendMail(senderID, recipientID uint32, subject, body stri
return m.sendErr return m.sendErr
} }
func (m *mockMailRepo) SendMailTx(_ *sql.Tx, senderID, recipientID uint32, subject, body string, itemID, itemAmount uint16, isGuildInvite, isSystemMessage bool) error {
return m.SendMail(senderID, recipientID, subject, body, itemID, itemAmount, isGuildInvite, isSystemMessage)
}
// --- mockCharacterRepo --- // --- mockCharacterRepo ---
type mockCharacterRepo struct { type mockCharacterRepo struct {
@@ -271,7 +266,10 @@ func (m *mockGuildRepoForMail) Save(_ *Guild) error { return
func (m *mockGuildRepoForMail) Disband(_ uint32) error { return nil } func (m *mockGuildRepoForMail) Disband(_ uint32) error { return nil }
func (m *mockGuildRepoForMail) RemoveCharacter(_ uint32) error { return nil } func (m *mockGuildRepoForMail) RemoveCharacter(_ uint32) error { return nil }
func (m *mockGuildRepoForMail) AcceptApplication(_, _ uint32) error { return nil } func (m *mockGuildRepoForMail) AcceptApplication(_, _ uint32) error { return nil }
func (m *mockGuildRepoForMail) CreateApplication(_, _, _ uint32, _ GuildApplicationType, _ *sql.Tx) error { func (m *mockGuildRepoForMail) CreateApplication(_, _, _ uint32, _ GuildApplicationType) error {
return nil
}
func (m *mockGuildRepoForMail) CreateApplicationWithMail(_, _, _ uint32, _ GuildApplicationType, _, _ uint32, _, _ string) error {
return nil return nil
} }
func (m *mockGuildRepoForMail) CancelInvitation(_, _ uint32) error { return nil } func (m *mockGuildRepoForMail) CancelInvitation(_, _ uint32) error { return nil }