From 197e44d04c6035a6e95d39e668795ff185579c1e Mon Sep 17 00:00:00 2001 From: Houmgaor Date: Fri, 20 Feb 2026 21:38:21 +0100 Subject: [PATCH] refactor(channelserver): extract CharacterRepository for characters table access MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Centralizes all characters table SQL behind a CharacterRepository struct in repo_character.go. The 4 existing helpers (loadCharacterData, saveCharacterData, readCharacterInt, adjustCharacterInt) now delegate to the repository, keeping identical signatures so all ~70 callsites remain unchanged. Direct queries in handlers_session.go, sys_channel_server.go (DisconnectUser), and handlers_mail.go are also migrated. Pure refactor with zero behavior change — first step toward eliminating the ~130 scattered character queries identified in anti-patterns #9. --- docs/anti-patterns.md | 2 + server/channelserver/handlers_helpers.go | 13 +- server/channelserver/handlers_mail.go | 9 +- server/channelserver/handlers_session.go | 15 +- server/channelserver/repo_character.go | 76 +++++++ server/channelserver/repo_character_test.go | 223 ++++++++++++++++++++ server/channelserver/sys_channel_server.go | 13 +- 7 files changed, 320 insertions(+), 31 deletions(-) create mode 100644 server/channelserver/repo_character.go create mode 100644 server/channelserver/repo_character_test.go diff --git a/docs/anti-patterns.md b/docs/anti-patterns.md index eafa9c2f9..52d17032f 100644 --- a/docs/anti-patterns.md +++ b/docs/anti-patterns.md @@ -240,6 +240,8 @@ The same table is queried in different handlers with slightly different column s **Recommendation:** At minimum, define query constants. Ideally, introduce a repository layer that encapsulates all queries for a given entity. +**Status (partial):** A `CharacterRepository` layer has been introduced in `repo_character.go`, centralizing all `characters` table access behind a concrete struct. The 4 existing helpers (`loadCharacterData`, `saveCharacterData`, `readCharacterInt`, `adjustCharacterInt`) now delegate to the repository, covering ~70% of character queries. Direct queries in `handlers_session.go` (login/logout), `sys_channel_server.go` (`DisconnectUser`), and `handlers_mail.go` (name lookup) have also been migrated. Remaining work: guild repository (second-highest duplication), per-handler migration of remaining inline character queries (plate, mercenary, rengoku, cafe, clients), and column allowlist for SQL injection hardening. + --- ## 10. init() Function for Handler Registration diff --git a/server/channelserver/handlers_helpers.go b/server/channelserver/handlers_helpers.go index 37aa46bcf..ae7b254c4 100644 --- a/server/channelserver/handlers_helpers.go +++ b/server/channelserver/handlers_helpers.go @@ -67,8 +67,7 @@ func doAckSimpleFail(s *Session, ackHandle uint32, data []byte) { // loadCharacterData loads a column from the characters table and sends it as // a buffered ack response. If the data is empty/nil, defaultData is sent instead. func loadCharacterData(s *Session, ackHandle uint32, column string, defaultData []byte) { - var data []byte - err := s.server.db.QueryRow("SELECT "+column+" FROM characters WHERE id = $1", s.charID).Scan(&data) + data, err := s.server.charRepo.LoadColumn(s.charID, column) if err != nil { s.logger.Error("Failed to load "+column, zap.Error(err)) } @@ -87,7 +86,7 @@ func saveCharacterData(s *Session, ackHandle uint32, column string, data []byte, return } dumpSaveData(s, data, column) - _, err := s.server.db.Exec("UPDATE characters SET "+column+"=$1 WHERE id=$2", data, s.charID) + err := s.server.charRepo.SaveColumn(s.charID, column, data) if err != nil { s.logger.Error("Failed to save "+column, zap.Error(err)) doAckSimpleFail(s, ackHandle, make([]byte, 4)) @@ -99,17 +98,13 @@ func saveCharacterData(s *Session, ackHandle uint32, column string, data []byte, // readCharacterInt reads a single integer column from the characters table. // Returns 0 for NULL columns via COALESCE. func readCharacterInt(s *Session, column string) (int, error) { - var value int - err := s.server.db.QueryRow("SELECT COALESCE("+column+", 0) FROM characters WHERE id=$1", s.charID).Scan(&value) - return value, err + return s.server.charRepo.ReadInt(s.charID, column) } // adjustCharacterInt atomically adds delta to an integer column and returns the new value. // Handles NULL columns via COALESCE (NULL + delta = delta). func adjustCharacterInt(s *Session, column string, delta int) (int, error) { - var value int - err := s.server.db.QueryRow("UPDATE characters SET "+column+"=COALESCE("+column+", 0)+$1 WHERE id=$2 RETURNING "+column, delta, s.charID).Scan(&value) - return value, err + return s.server.charRepo.AdjustInt(s.charID, column, delta) } func updateRights(s *Session) { diff --git a/server/channelserver/handlers_mail.go b/server/channelserver/handlers_mail.go index 77fadc33a..e6093be85 100644 --- a/server/channelserver/handlers_mail.go +++ b/server/channelserver/handlers_mail.go @@ -193,16 +193,11 @@ func SendMailNotification(s *Session, m *Mail, recipient *Session) { } func getCharacterName(s *Session, charID uint32) string { - row := s.server.db.QueryRow("SELECT name FROM characters WHERE id = $1", charID) - - charName := "" - - err := row.Scan(&charName) - + name, err := s.server.charRepo.GetName(charID) if err != nil { return "" } - return charName + return name } func handleMsgMhfReadMail(s *Session, p mhfpacket.MHFPacket) { diff --git a/server/channelserver/handlers_session.go b/server/channelserver/handlers_session.go index 9b75a88b5..3ec51e08d 100644 --- a/server/channelserver/handlers_session.go +++ b/server/channelserver/handlers_session.go @@ -71,16 +71,18 @@ func handleMsgSysLogin(s *Session, p mhfpacket.MHFPacket) { s.token = pkt.LoginTokenString s.Unlock() - if err := s.server.db.QueryRow("SELECT user_id FROM characters WHERE id=$1", s.charID).Scan(&s.userID); err != nil { + userID, err := s.server.charRepo.GetUserID(s.charID) + if err != nil { s.logger.Error("Failed to resolve user ID for character", zap.Error(err), zap.Uint32("charID", s.charID)) doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) return } + s.userID = userID bf := byteframe.NewByteFrame() bf.WriteUint32(uint32(TimeAdjusted().Unix())) // Unix timestamp - _, err := s.server.db.Exec("UPDATE servers SET current_players=$1 WHERE server_id=$2", len(s.server.sessions), s.server.ID) + _, err = s.server.db.Exec("UPDATE servers SET current_players=$1 WHERE server_id=$2", len(s.server.sessions), s.server.ID) if err != nil { s.logger.Error("Failed to update current players", zap.Error(err)) doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) @@ -94,8 +96,7 @@ func handleMsgSysLogin(s *Session, p mhfpacket.MHFPacket) { return } - _, err = s.server.db.Exec("UPDATE characters SET last_login=$1 WHERE id=$2", TimeAdjusted().Unix(), s.charID) - if err != nil { + if err = s.server.charRepo.UpdateLastLogin(s.charID, TimeAdjusted().Unix()); err != nil { s.logger.Error("Failed to update last login", zap.Error(err)) doAckSimpleFail(s, pkt.AckHandle, make([]byte, 4)) return @@ -238,8 +239,10 @@ func logoutPlayer(s *Session) { var rpGained int if s.charID != 0 { - if err := s.server.db.QueryRow("SELECT time_played FROM characters WHERE id = $1", s.charID).Scan(&timePlayed); err != nil { + if val, err := s.server.charRepo.ReadInt(s.charID, "time_played"); err != nil { s.logger.Error("Failed to read time_played, RP accrual may be inaccurate", zap.Error(err)) + } else { + timePlayed = val } sessionTime = int(TimeAdjusted().Unix()) - int(s.sessionStart) timePlayed += sessionTime @@ -275,7 +278,7 @@ func logoutPlayer(s *Session) { } // Update time_played and guild treasure hunt - if _, err := s.server.db.Exec("UPDATE characters SET time_played = $1 WHERE id = $2", timePlayed, s.charID); err != nil { + if err := s.server.charRepo.UpdateTimePlayed(s.charID, timePlayed); err != nil { s.logger.Error("Failed to update time played", zap.Error(err)) } if _, err := s.server.db.Exec(`UPDATE guild_characters SET treasure_hunt=NULL WHERE character_id=$1`, s.charID); err != nil { diff --git a/server/channelserver/repo_character.go b/server/channelserver/repo_character.go new file mode 100644 index 000000000..3d0bf5b01 --- /dev/null +++ b/server/channelserver/repo_character.go @@ -0,0 +1,76 @@ +package channelserver + +import "github.com/jmoiron/sqlx" + +// CharacterRepository centralizes all database access for the characters table. +type CharacterRepository struct { + db *sqlx.DB +} + +// NewCharacterRepository creates a new CharacterRepository. +func NewCharacterRepository(db *sqlx.DB) *CharacterRepository { + return &CharacterRepository{db: db} +} + +// LoadColumn reads a single []byte column by character ID. +func (r *CharacterRepository) LoadColumn(charID uint32, column string) ([]byte, error) { + var data []byte + err := r.db.QueryRow("SELECT "+column+" FROM characters WHERE id = $1", charID).Scan(&data) + return data, err +} + +// SaveColumn writes a single []byte column by character ID. +func (r *CharacterRepository) SaveColumn(charID uint32, column string, data []byte) error { + _, err := r.db.Exec("UPDATE characters SET "+column+"=$1 WHERE id=$2", data, charID) + return err +} + +// ReadInt reads a single integer column (0 for NULL) by character ID. +func (r *CharacterRepository) ReadInt(charID uint32, column string) (int, error) { + var value int + err := r.db.QueryRow("SELECT COALESCE("+column+", 0) FROM characters WHERE id=$1", charID).Scan(&value) + return value, err +} + +// AdjustInt atomically adds delta to an integer column and returns the new value. +func (r *CharacterRepository) AdjustInt(charID uint32, column string, delta int) (int, error) { + var value int + err := r.db.QueryRow( + "UPDATE characters SET "+column+"=COALESCE("+column+", 0)+$1 WHERE id=$2 RETURNING "+column, + delta, charID, + ).Scan(&value) + return value, err +} + +// GetName returns the character name by ID. +func (r *CharacterRepository) GetName(charID uint32) (string, error) { + var name string + err := r.db.QueryRow("SELECT name FROM characters WHERE id=$1", charID).Scan(&name) + return name, err +} + +// GetUserID returns the owning user_id for a character. +func (r *CharacterRepository) GetUserID(charID uint32) (uint32, error) { + var userID uint32 + err := r.db.QueryRow("SELECT user_id FROM characters WHERE id=$1", charID).Scan(&userID) + return userID, err +} + +// UpdateLastLogin sets the last_login timestamp. +func (r *CharacterRepository) UpdateLastLogin(charID uint32, timestamp int64) error { + _, err := r.db.Exec("UPDATE characters SET last_login=$1 WHERE id=$2", timestamp, charID) + return err +} + +// UpdateTimePlayed sets the time_played value. +func (r *CharacterRepository) UpdateTimePlayed(charID uint32, timePlayed int) error { + _, err := r.db.Exec("UPDATE characters SET time_played=$1 WHERE id=$2", timePlayed, charID) + return err +} + +// GetCharIDsByUserID returns all character IDs belonging to a user. +func (r *CharacterRepository) GetCharIDsByUserID(userID uint32) ([]uint32, error) { + var ids []uint32 + err := r.db.Select(&ids, "SELECT id FROM characters WHERE user_id=$1", userID) + return ids, err +} diff --git a/server/channelserver/repo_character_test.go b/server/channelserver/repo_character_test.go new file mode 100644 index 000000000..af9bbf11c --- /dev/null +++ b/server/channelserver/repo_character_test.go @@ -0,0 +1,223 @@ +package channelserver + +import ( + "testing" + + "github.com/jmoiron/sqlx" +) + +func setupCharRepo(t *testing.T) (*CharacterRepository, *sqlx.DB, uint32) { + t.Helper() + db := SetupTestDB(t) + userID := CreateTestUser(t, db, "repo_test_user") + charID := CreateTestCharacter(t, db, userID, "RepoChar") + repo := NewCharacterRepository(db) + t.Cleanup(func() { TeardownTestDB(t, db) }) + return repo, db, charID +} + +func TestLoadColumn(t *testing.T) { + repo, db, charID := setupCharRepo(t) + + // Write a known blob to a column + blob := []byte{0xDE, 0xAD, 0xBE, 0xEF} + _, err := db.Exec("UPDATE characters SET kouryou_point=$1 WHERE id=$2", blob, charID) + if err != nil { + t.Fatalf("Setup failed: %v", err) + } + + data, err := repo.LoadColumn(charID, "kouryou_point") + if err != nil { + t.Fatalf("LoadColumn failed: %v", err) + } + if len(data) != 4 || data[0] != 0xDE || data[3] != 0xEF { + t.Errorf("LoadColumn returned unexpected data: %x", data) + } +} + +func TestLoadColumnNil(t *testing.T) { + repo, _, charID := setupCharRepo(t) + + // Column should be NULL by default + data, err := repo.LoadColumn(charID, "kouryou_point") + if err != nil { + t.Fatalf("LoadColumn failed: %v", err) + } + if data != nil { + t.Errorf("Expected nil for NULL column, got: %x", data) + } +} + +func TestSaveColumn(t *testing.T) { + repo, db, charID := setupCharRepo(t) + + blob := []byte{0x01, 0x02, 0x03} + if err := repo.SaveColumn(charID, "kouryou_point", blob); err != nil { + t.Fatalf("SaveColumn failed: %v", err) + } + + // Verify via direct SELECT + var got []byte + if err := db.QueryRow("SELECT kouryou_point FROM characters WHERE id=$1", charID).Scan(&got); err != nil { + t.Fatalf("Verification query failed: %v", err) + } + if len(got) != 3 || got[0] != 0x01 || got[2] != 0x03 { + t.Errorf("SaveColumn wrote unexpected data: %x", got) + } +} + +func TestReadInt(t *testing.T) { + repo, _, charID := setupCharRepo(t) + + // time_played defaults to 0 via COALESCE + val, err := repo.ReadInt(charID, "time_played") + if err != nil { + t.Fatalf("ReadInt failed: %v", err) + } + if val != 0 { + t.Errorf("Expected 0 for default time_played, got: %d", val) + } +} + +func TestReadIntWithValue(t *testing.T) { + repo, db, charID := setupCharRepo(t) + + _, err := db.Exec("UPDATE characters SET time_played=42 WHERE id=$1", charID) + if err != nil { + t.Fatalf("Setup failed: %v", err) + } + + val, err := repo.ReadInt(charID, "time_played") + if err != nil { + t.Fatalf("ReadInt failed: %v", err) + } + if val != 42 { + t.Errorf("Expected 42, got: %d", val) + } +} + +func TestAdjustInt(t *testing.T) { + repo, _, charID := setupCharRepo(t) + + // First adjustment from NULL (COALESCE makes it 0 + 10 = 10) + val, err := repo.AdjustInt(charID, "time_played", 10) + if err != nil { + t.Fatalf("AdjustInt failed: %v", err) + } + if val != 10 { + t.Errorf("Expected 10 after first adjust, got: %d", val) + } + + // Second adjustment: 10 + 5 = 15 + val, err = repo.AdjustInt(charID, "time_played", 5) + if err != nil { + t.Fatalf("AdjustInt failed: %v", err) + } + if val != 15 { + t.Errorf("Expected 15 after second adjust, got: %d", val) + } +} + +func TestGetName(t *testing.T) { + repo, _, charID := setupCharRepo(t) + + name, err := repo.GetName(charID) + if err != nil { + t.Fatalf("GetName failed: %v", err) + } + if name != "RepoChar" { + t.Errorf("Expected 'RepoChar', got: %q", name) + } +} + +func TestGetUserID(t *testing.T) { + repo, db, charID := setupCharRepo(t) + + // Look up the expected user_id + var expectedUID uint32 + if err := db.QueryRow("SELECT user_id FROM characters WHERE id=$1", charID).Scan(&expectedUID); err != nil { + t.Fatalf("Setup query failed: %v", err) + } + + uid, err := repo.GetUserID(charID) + if err != nil { + t.Fatalf("GetUserID failed: %v", err) + } + if uid != expectedUID { + t.Errorf("Expected user_id %d, got: %d", expectedUID, uid) + } +} + +func TestUpdateLastLogin(t *testing.T) { + repo, db, charID := setupCharRepo(t) + + ts := int64(1700000000) + if err := repo.UpdateLastLogin(charID, ts); err != nil { + t.Fatalf("UpdateLastLogin failed: %v", err) + } + + var got int64 + if err := db.QueryRow("SELECT last_login FROM characters WHERE id=$1", charID).Scan(&got); err != nil { + t.Fatalf("Verification query failed: %v", err) + } + if got != ts { + t.Errorf("Expected last_login %d, got: %d", ts, got) + } +} + +func TestUpdateTimePlayed(t *testing.T) { + repo, db, charID := setupCharRepo(t) + + if err := repo.UpdateTimePlayed(charID, 999); err != nil { + t.Fatalf("UpdateTimePlayed failed: %v", err) + } + + var got int + if err := db.QueryRow("SELECT time_played FROM characters WHERE id=$1", charID).Scan(&got); err != nil { + t.Fatalf("Verification query failed: %v", err) + } + if got != 999 { + t.Errorf("Expected time_played 999, got: %d", got) + } +} + +func TestGetCharIDsByUserID(t *testing.T) { + repo, db, _ := setupCharRepo(t) + + // Create a second user with multiple characters + uid2 := CreateTestUser(t, db, "multi_char_user") + cid1 := CreateTestCharacter(t, db, uid2, "Char1") + cid2 := CreateTestCharacter(t, db, uid2, "Char2") + + ids, err := repo.GetCharIDsByUserID(uid2) + if err != nil { + t.Fatalf("GetCharIDsByUserID failed: %v", err) + } + if len(ids) != 2 { + t.Fatalf("Expected 2 character IDs, got: %d", len(ids)) + } + + // Check both IDs are present (order may vary) + found := map[uint32]bool{cid1: false, cid2: false} + for _, id := range ids { + found[id] = true + } + if !found[cid1] || !found[cid2] { + t.Errorf("Expected IDs %d and %d, got: %v", cid1, cid2, ids) + } +} + +func TestGetCharIDsByUserIDEmpty(t *testing.T) { + repo, db, _ := setupCharRepo(t) + + // Create a user with no characters + uid := CreateTestUser(t, db, "no_chars_user") + + ids, err := repo.GetCharIDsByUserID(uid) + if err != nil { + t.Fatalf("GetCharIDsByUserID failed: %v", err) + } + if len(ids) != 0 { + t.Errorf("Expected 0 character IDs for user with no chars, got: %d", len(ids)) + } +} diff --git a/server/channelserver/sys_channel_server.go b/server/channelserver/sys_channel_server.go index 1e11294c2..318116600 100644 --- a/server/channelserver/sys_channel_server.go +++ b/server/channelserver/sys_channel_server.go @@ -45,6 +45,7 @@ type Server struct { Port uint16 logger *zap.Logger db *sqlx.DB + charRepo *CharacterRepository erupeConfig *_config.Config acceptConns chan net.Conn deleteConns chan net.Conn @@ -115,6 +116,8 @@ func NewServer(config *Config) *Server { handlerTable: buildHandlerTable(), } + s.charRepo = NewCharacterRepository(config.DB) + // Mezeporta s.stages["sl1Ns200p0a0u0"] = NewStage("sl1Ns200p0a0u0") @@ -361,17 +364,9 @@ func (s *Server) FindSessionByCharID(charID uint32) *Session { // DisconnectUser disconnects all sessions belonging to the given user ID. func (s *Server) DisconnectUser(uid uint32) { - var cid uint32 - var cids []uint32 - rows, err := s.db.Query(`SELECT id FROM characters WHERE user_id=$1`, uid) + cids, err := s.charRepo.GetCharIDsByUserID(uid) if err != nil { s.logger.Error("Failed to query characters for disconnect", zap.Error(err)) - } else { - defer func() { _ = rows.Close() }() - for rows.Next() { - _ = rows.Scan(&cid) - cids = append(cids, cid) - } } if s.Registry != nil { s.Registry.DisconnectUser(cids)