diff --git a/src/main/java/com/gmail/nossr50/database/SQLDatabaseManager.java b/src/main/java/com/gmail/nossr50/database/SQLDatabaseManager.java index b05f95945..e6e5d7354 100644 --- a/src/main/java/com/gmail/nossr50/database/SQLDatabaseManager.java +++ b/src/main/java/com/gmail/nossr50/database/SQLDatabaseManager.java @@ -574,105 +574,125 @@ public final class SQLDatabaseManager implements DatabaseManager { } public Map readRank(String playerName) { - Map skills = new HashMap<>(); + // NOTE: We keep HashMap so we can still use `null` as the "total" key, + // just like the original code. + Map ranks = new HashMap<>(); - Connection connection = null; - PreparedStatement statement = null; - ResultSet resultSet = null; + // Preload this player's skill levels & total in a single query + try (Connection connection = getConnection(PoolIdentifier.MISC)) { - try { - connection = getConnection(PoolIdentifier.MISC); + // 1) Load all relevant skill levels for this player in one shot + Map levels = new EnumMap<>(PrimarySkillType.class); + int totalLevel = 0; - // Per-skill rank - for (PrimarySkillType primarySkillType : SkillTools.NON_CHILD_SKILLS) { - String skillName = primarySkillType.name().toLowerCase(Locale.ENGLISH); + String loadSql = + "SELECT s.*, u.`user` " + + "FROM " + tablePrefix + "users u " + + "JOIN " + tablePrefix + "skills s ON s.user_id = u.id " + + "WHERE u.`user` = ?"; - String sql = "SELECT COUNT(*) AS 'rank' FROM " + tablePrefix + "users JOIN " - + tablePrefix + "skills ON user_id = id WHERE " + skillName + " > 0 " + - "AND " + skillName + " > (SELECT " + skillName + " FROM " + tablePrefix - + "users JOIN " + tablePrefix + "skills ON user_id = id " + - "WHERE `user` = ?)"; + try (PreparedStatement stmt = connection.prepareStatement(loadSql)) { + stmt.setString(1, playerName); - statement = connection.prepareStatement(sql); - statement.setString(1, playerName); - resultSet = statement.executeQuery(); - - resultSet.next(); - int rank = resultSet.getInt("rank"); - - resultSet.close(); - statement.close(); - - sql = "SELECT user, " + skillName + " FROM " + tablePrefix + "users JOIN " - + tablePrefix + "skills ON user_id = id WHERE " + skillName + " > 0 " + - "AND " + skillName + " = (SELECT " + skillName + " FROM " + tablePrefix - + "users JOIN " + tablePrefix + "skills ON user_id = id " + - "WHERE `user` = '" + playerName + "') ORDER BY user"; - - statement = connection.prepareStatement(sql); - resultSet = statement.executeQuery(); - - while (resultSet.next()) { - if (resultSet.getString("user").equalsIgnoreCase(playerName)) { - skills.put(primarySkillType, rank + resultSet.getRow()); - break; + try (ResultSet rs = stmt.executeQuery()) { + if (!rs.next()) { + // Player not found in DB, no ranks to report + return ranks; } - } - resultSet.close(); - statement.close(); - } + for (PrimarySkillType primarySkillType : SkillTools.NON_CHILD_SKILLS) { + String column = primarySkillType.name().toLowerCase(Locale.ENGLISH); + levels.put(primarySkillType, rs.getInt(column)); + } - // Total rank - String sql = - "SELECT COUNT(*) AS 'rank' FROM " + tablePrefix + "users JOIN " + tablePrefix - + "skills ON user_id = id " + - "WHERE " + ALL_QUERY_VERSION + " > 0 " + - "AND " + ALL_QUERY_VERSION + " > " + - "(SELECT " + ALL_QUERY_VERSION + " " + - "FROM " + tablePrefix + "users JOIN " + tablePrefix - + "skills ON user_id = id WHERE `user` = ?)"; - - statement = connection.prepareStatement(sql); - statement.setString(1, playerName); - resultSet = statement.executeQuery(); - - resultSet.next(); - int rank = resultSet.getInt("rank"); - - resultSet.close(); - statement.close(); - - sql = "SELECT user, " + ALL_QUERY_VERSION + " " + - "FROM " + tablePrefix + "users JOIN " + tablePrefix + "skills ON user_id = id " - + - "WHERE " + ALL_QUERY_VERSION + " > 0 " + - "AND " + ALL_QUERY_VERSION + " = " + - "(SELECT " + ALL_QUERY_VERSION + " " + - "FROM " + tablePrefix + "users JOIN " + tablePrefix - + "skills ON user_id = id WHERE `user` = ?) ORDER BY user"; - - statement = connection.prepareStatement(sql); - statement.setString(1, playerName); - resultSet = statement.executeQuery(); - - while (resultSet.next()) { - if (resultSet.getString("user").equalsIgnoreCase(playerName)) { - skills.put(null, rank + resultSet.getRow()); - break; + totalLevel = rs.getInt(ALL_QUERY_VERSION); // "total" column } } + + // Helper method to compute a rank (base + tie offset + 1) + // for any numeric column on the skills table. + class RankCalculator { + int computeRank(String columnName, int value) throws SQLException { + if (value <= 0) { + // Original logic effectively did not assign a rank when the value <= 0 + return -1; + } + + // Base: number of players with strictly higher value + String higherSql = + "SELECT COUNT(*) AS cnt " + + "FROM " + tablePrefix + "users u " + + "JOIN " + tablePrefix + "skills s ON s.user_id = u.id " + + "WHERE s." + columnName + " > ?"; + + int higherCount = 0; + try (PreparedStatement stmt = connection.prepareStatement(higherSql)) { + stmt.setInt(1, value); + try (ResultSet rs = stmt.executeQuery()) { + if (rs.next()) { + higherCount = rs.getInt("cnt"); + } + } + } + + // Tie offset: number of players with the same value whose username + // sorts alphabetically before this player's name. + String tieSql = + "SELECT COUNT(*) AS cnt " + + "FROM " + tablePrefix + "users u " + + "JOIN " + tablePrefix + "skills s ON s.user_id = u.id " + + "WHERE s." + columnName + " = ? " + + "AND s." + columnName + " > 0 " + + "AND u.`user` < ?"; + + int tieCount = 0; + try (PreparedStatement stmt = connection.prepareStatement(tieSql)) { + stmt.setInt(1, value); + stmt.setString(2, playerName); + try (ResultSet rs = stmt.executeQuery()) { + if (rs.next()) { + tieCount = rs.getInt("cnt"); + } + } + } + + // 1-based rank: higher values first, then alphabetical by username + return higherCount + tieCount + 1; + } + } + + RankCalculator rankCalculator = new RankCalculator(); + + // 2) Per-skill rank + for (PrimarySkillType primarySkillType : SkillTools.NON_CHILD_SKILLS) { + int level = levels.getOrDefault(primarySkillType, 0); + + int rank = rankCalculator.computeRank( + primarySkillType.name().toLowerCase(Locale.ENGLISH), + level + ); + + if (rank > 0) { + ranks.put(primarySkillType, rank); + } + } + + // 3) Total rank (null key matches original behavior) + if (totalLevel > 0) { + int totalRank = rankCalculator.computeRank(ALL_QUERY_VERSION, totalLevel); + if (totalRank > 0) { + ranks.put(null, totalRank); + } + } + } catch (SQLException ex) { logSQLException(ex); - } finally { - tryClose(resultSet); - tryClose(statement); - tryClose(connection); } - return skills; + return ranks; } + // --------------------------------------------------------------------- // New user / load profile // --------------------------------------------------------------------- @@ -815,7 +835,7 @@ public final class SQLDatabaseManager implements DatabaseManager { int id = getUserID(connection, playerName, uuid); if (id == -1) { - return createEmptyProfile(playerName); + return createEmptyProfile(playerName, uuid); } writeMissingRows(connection, id); @@ -895,7 +915,7 @@ public final class SQLDatabaseManager implements DatabaseManager { try (ResultSet resultSet = statement.executeQuery()) { if (!resultSet.next()) { - return createEmptyProfile(playerName); + return createEmptyProfile(playerName, uuid); } String nameInDb = resultSet.getString("username"); @@ -914,12 +934,12 @@ public final class SQLDatabaseManager implements DatabaseManager { } } catch (SQLException ex) { logSQLException(ex); - return createEmptyProfile(playerName); + return createEmptyProfile(playerName, uuid); } } - private PlayerProfile createEmptyProfile(@Nullable String playerName) { - return new PlayerProfile(playerName, mcMMO.p.getAdvancedConfig().getStartingLevel()); + private PlayerProfile createEmptyProfile(@Nullable String playerName, @Nullable UUID uuid) { + return new PlayerProfile(playerName, uuid, mcMMO.p.getAdvancedConfig().getStartingLevel()); } private boolean shouldUpdateUsername(@Nullable String playerName, @@ -1354,8 +1374,6 @@ public final class SQLDatabaseManager implements DatabaseManager { } private void deleteOrphans(Connection connection) throws SQLException { - LogUtils.debug(logger, "Killing orphans"); - try (Statement stmt = connection.createStatement()) { stmt.executeUpdate( "DELETE FROM `" + tablePrefix + "experience` " + @@ -1455,7 +1473,6 @@ public final class SQLDatabaseManager implements DatabaseManager { */ private void checkDatabaseStructure(Connection connection, UpgradeType upgrade) { if (!mcMMO.getUpgradeManager().shouldUpgrade(upgrade)) { - LogUtils.debug(logger, "Skipping " + upgrade.name() + " upgrade (unneeded)"); return; } @@ -1463,10 +1480,6 @@ public final class SQLDatabaseManager implements DatabaseManager { switch (upgrade) { case ADD_FISHING -> checkUpgradeAddFishing(statement); case ADD_BLAST_MINING_COOLDOWN -> checkUpgradeAddBlastMiningCooldown(statement); - case ADD_SQL_INDEXES -> { - // historical: currently disabled - // checkUpgradeAddSQLIndexes(statement); - } case ADD_MOB_HEALTHBARS -> checkUpgradeAddMobHealthbars(statement); case DROP_SQL_PARTY_NAMES -> checkUpgradeDropPartyNames(statement); case DROP_SPOUT -> checkUpgradeDropSpout(statement); @@ -1721,13 +1734,22 @@ public final class SQLDatabaseManager implements DatabaseManager { mcMMO.getUpgradeManager().setUpgradeCompleted(UpgradeType.ADD_SKILL_TOTAL); } catch (SQLException ex) { logSQLException(ex); + try { + connection.rollback(); + } catch (SQLException ignored) { + // best effort + } } finally { - connection.setAutoCommit(true); + try { + connection.setAutoCommit(true); + } catch (SQLException ignored) { + } tryClose(resultSet); tryClose(statement); } } + private void checkUpgradeDropSpout(final Statement statement) { ResultSet resultSet = null; diff --git a/src/main/java/com/gmail/nossr50/datatypes/database/UpgradeType.java b/src/main/java/com/gmail/nossr50/datatypes/database/UpgradeType.java index 13faa75b7..971c85018 100644 --- a/src/main/java/com/gmail/nossr50/datatypes/database/UpgradeType.java +++ b/src/main/java/com/gmail/nossr50/datatypes/database/UpgradeType.java @@ -3,7 +3,6 @@ package com.gmail.nossr50.datatypes.database; public enum UpgradeType { ADD_FISHING, ADD_BLAST_MINING_COOLDOWN, - ADD_SQL_INDEXES, ADD_MOB_HEALTHBARS, DROP_SQL_PARTY_NAMES, DROP_SPOUT, diff --git a/src/main/java/com/gmail/nossr50/util/LogUtils.java b/src/main/java/com/gmail/nossr50/util/LogUtils.java index e9c970465..cba9005aa 100644 --- a/src/main/java/com/gmail/nossr50/util/LogUtils.java +++ b/src/main/java/com/gmail/nossr50/util/LogUtils.java @@ -8,6 +8,7 @@ public class LogUtils { public static final String DEBUG_STR = "[D] "; public static void debug(@NotNull Logger logger, @NotNull String message) { + // Messages here will get filtered based on config settings via LogFilter logger.info(DEBUG_STR + message); } } diff --git a/src/test/java/com/gmail/nossr50/database/SQLDatabaseManagerTest.java b/src/test/java/com/gmail/nossr50/database/SQLDatabaseManagerTest.java index cfb32feae..7b2f7703a 100644 --- a/src/test/java/com/gmail/nossr50/database/SQLDatabaseManagerTest.java +++ b/src/test/java/com/gmail/nossr50/database/SQLDatabaseManagerTest.java @@ -1184,6 +1184,246 @@ class SQLDatabaseManagerTest { databaseManager.onDisable(); } + @ParameterizedTest(name = "{0} - readRank for unknown user returns empty map") + @MethodSource("dbFlavors") + void whenReadingRankForUnknownUserShouldReturnEmptyMap(DbFlavor flavor) { + truncateAllCoreTables(flavor); + SQLDatabaseManager databaseManager = createManagerFor(flavor); + + try { + Map ranks = databaseManager.readRank("ghost_" + flavor.name().toLowerCase()); + + assertThat(ranks) + .as("Unknown user should yield an empty rank map") + .isEmpty(); + } finally { + databaseManager.onDisable(); + } + } + + @ParameterizedTest(name = "{0} - readRank for zero-skill user returns empty map") + @MethodSource("dbFlavors") + void whenReadingRankForZeroSkillUserShouldReturnEmptyMap(DbFlavor flavor) { + truncateAllCoreTables(flavor); + SQLDatabaseManager databaseManager = createManagerFor(flavor); + + String zeroName = "zeros_" + flavor.name().toLowerCase(); + UUID zeroUuid = UUID.randomUUID(); + + try { + // newUser -> all skills 0, total 0 + databaseManager.newUser(zeroName, zeroUuid); + PlayerProfile zeroProfile = databaseManager.loadPlayerProfile(zeroUuid); + assertThat(databaseManager.saveUser(zeroProfile)).isTrue(); + + // Also create a powered user for sanity; zero user still should not be ranked + String poweredName = "nonzero_" + flavor.name().toLowerCase(); + UUID poweredUuid = UUID.randomUUID(); + createUserWithUniformNonChildSkills(databaseManager, poweredName, poweredUuid, 100); + + Map ranks = databaseManager.readRank(zeroName); + + assertThat(ranks) + .as("User with all skills at 0 should not have any rank entries") + .isEmpty(); + } finally { + databaseManager.onDisable(); + } + } + + @ParameterizedTest(name = "{0} - readRank with a single user → rank 1 for all non-child skills") + @MethodSource("dbFlavors") + void whenSingleUserShouldBeRankOneForAllNonChildSkills(DbFlavor flavor) { + truncateAllCoreTables(flavor); + SQLDatabaseManager databaseManager = createManagerFor(flavor); + + String soloName = "solo_" + flavor.name().toLowerCase(); + UUID soloUuid = UUID.randomUUID(); + + try { + // All non-child skills = 50 + createUserWithUniformNonChildSkills(databaseManager, soloName, soloUuid, 50); + + Map ranks = databaseManager.readRank(soloName); + + for (PrimarySkillType type : PrimarySkillType.values()) { + if (SkillTools.isChildSkill(type)) { + assertThat(ranks.get(type)) + .as("Child skill %s should have no rank", type) + .isNull(); + } else { + assertThat(ranks.get(type)) + .as("Solo player should be rank 1 for skill %s", type) + .isEqualTo(1); + } + } + + // Total rank (null key) should also be 1 + assertThat(ranks.get(null)) + .as("Solo player total rank should be 1") + .isEqualTo(1); + } finally { + databaseManager.onDisable(); + } + } + + @ParameterizedTest(name = "{0} - readRank alphabetical tiebreaker with only equal-skill users") + @MethodSource("dbFlavors") + void whenEqualSkillUsersOnlyShouldUseAlphabeticalTiebreaker(DbFlavor flavor) { + truncateAllCoreTables(flavor); + SQLDatabaseManager databaseManager = createManagerFor(flavor); + + String nameA = "aaa_" + flavor.name().toLowerCase(); + String nameB = "bbb_" + flavor.name().toLowerCase(); + String nameC = "ccc_" + flavor.name().toLowerCase(); + + UUID uuidA = UUID.randomUUID(); + UUID uuidB = UUID.randomUUID(); + UUID uuidC = UUID.randomUUID(); + + try { + // For simplicity, set only MINING and let total = mining + Map skillMap = Map.of(PrimarySkillType.MINING, 100); + + createUserWithSkills(databaseManager, nameA, uuidA, skillMap); + createUserWithSkills(databaseManager, nameB, uuidB, skillMap); + createUserWithSkills(databaseManager, nameC, uuidC, skillMap); + + Map ranksA = databaseManager.readRank(nameA); + Map ranksB = databaseManager.readRank(nameB); + Map ranksC = databaseManager.readRank(nameC); + + // Mining ranks: alphabetical order + assertThat(ranksA.get(PrimarySkillType.MINING)).isEqualTo(1); + assertThat(ranksB.get(PrimarySkillType.MINING)).isEqualTo(2); + assertThat(ranksC.get(PrimarySkillType.MINING)).isEqualTo(3); + + // Total ranks behave the same in this setup (total == mining) + assertThat(ranksA.get(null)).isEqualTo(1); + assertThat(ranksB.get(null)).isEqualTo(2); + assertThat(ranksC.get(null)).isEqualTo(3); + } finally { + databaseManager.onDisable(); + } + } + + @ParameterizedTest(name = "{0} - readRank tie group is offset by number of higher players") + @MethodSource("dbFlavors") + void whenEqualSkillUsersHaveHigherPlayerShouldOffsetByHigherCount(DbFlavor flavor) { + truncateAllCoreTables(flavor); + SQLDatabaseManager databaseManager = createManagerFor(flavor); + + String higherName = "zoe_" + flavor.name().toLowerCase(); + String nameA = "aaa2_" + flavor.name().toLowerCase(); + String nameB = "bbb2_" + flavor.name().toLowerCase(); + String nameC = "ccc2_" + flavor.name().toLowerCase(); + + UUID uuidHigher = UUID.randomUUID(); + UUID uuidA = UUID.randomUUID(); + UUID uuidB = UUID.randomUUID(); + UUID uuidC = UUID.randomUUID(); + + try { + // Higher player + createUserWithSkills( + databaseManager, + higherName, + uuidHigher, + Map.of(PrimarySkillType.MINING, 200) + ); + + // Tie group + Map tieSkills = Map.of(PrimarySkillType.MINING, 100); + createUserWithSkills(databaseManager, nameA, uuidA, tieSkills); + createUserWithSkills(databaseManager, nameB, uuidB, tieSkills); + createUserWithSkills(databaseManager, nameC, uuidC, tieSkills); + + Map higherRanks = databaseManager.readRank(higherName); + Map ranksA = databaseManager.readRank(nameA); + Map ranksB = databaseManager.readRank(nameB); + Map ranksC = databaseManager.readRank(nameC); + + // Higher player is rank 1 + assertThat(higherRanks.get(PrimarySkillType.MINING)).isEqualTo(1); + + // Others follow in alphabetical order, offset by 1 + assertThat(ranksA.get(PrimarySkillType.MINING)).isEqualTo(2); + assertThat(ranksB.get(PrimarySkillType.MINING)).isEqualTo(3); + assertThat(ranksC.get(PrimarySkillType.MINING)).isEqualTo(4); + } finally { + databaseManager.onDisable(); + } + } + + @ParameterizedTest(name = "{0} - readRank per-skill vs total ranking can differ") + @MethodSource("dbFlavors") + void whenDifferentSkillDistributionsShouldComputePerSkillAndTotalRanksSeparately(DbFlavor flavor) { + truncateAllCoreTables(flavor); + SQLDatabaseManager databaseManager = createManagerFor(flavor); + + String alphaName = "alpha_" + flavor.name().toLowerCase(); + String bravoName = "bravo_" + flavor.name().toLowerCase(); + String charlieName = "charlie_" + flavor.name().toLowerCase(); + + UUID alphaUuid = UUID.randomUUID(); + UUID bravoUuid = UUID.randomUUID(); + UUID charlieUuid = UUID.randomUUID(); + + try { + // alpha: mining 100, fishing 0 + createUserWithSkills( + databaseManager, + alphaName, + alphaUuid, + Map.of(PrimarySkillType.MINING, 100) + ); + + // bravo: mining 50, fishing 200 + createUserWithSkills( + databaseManager, + bravoName, + bravoUuid, + Map.of( + PrimarySkillType.MINING, 50, + PrimarySkillType.FISHING, 200 + ) + ); + + // charlie: mining 75, fishing 50 + createUserWithSkills( + databaseManager, + charlieName, + charlieUuid, + Map.of( + PrimarySkillType.MINING, 75, + PrimarySkillType.FISHING, 50 + ) + ); + + Map alphaRanks = databaseManager.readRank(alphaName); + Map bravoRanks = databaseManager.readRank(bravoName); + Map charlieRanks = databaseManager.readRank(charlieName); + + // --- Mining (100 > 75 > 50) --- + assertThat(alphaRanks.get(PrimarySkillType.MINING)).isEqualTo(1); + assertThat(charlieRanks.get(PrimarySkillType.MINING)).isEqualTo(2); + assertThat(bravoRanks.get(PrimarySkillType.MINING)).isEqualTo(3); + + // --- Fishing (200 > 50 > 0) --- + // alpha has 0 -> no rank entry for fishing + assertThat(alphaRanks.get(PrimarySkillType.FISHING)).isNull(); + + assertThat(bravoRanks.get(PrimarySkillType.FISHING)).isEqualTo(1); + assertThat(charlieRanks.get(PrimarySkillType.FISHING)).isEqualTo(2); + + // --- Total: alpha 100, bravo 250, charlie 125 --- + assertThat(bravoRanks.get(null)).isEqualTo(1); // 250 highest + assertThat(charlieRanks.get(null)).isEqualTo(2); // 125 + assertThat(alphaRanks.get(null)).isEqualTo(3); // 100 + } finally { + databaseManager.onDisable(); + } + } // ------------------------------------------------------------------------ // getDatabaseType @@ -1385,4 +1625,37 @@ class SQLDatabaseManagerTest { return resultSet.next(); } } + + // ------------------------------------------------------------------------ + // Helpers for readRank tests + // ------------------------------------------------------------------------ + + private void createUserWithUniformNonChildSkills(SQLDatabaseManager manager, + String name, + UUID uuid, + int level) { + manager.newUser(name, uuid); + PlayerProfile profile = manager.loadPlayerProfile(uuid); + for (PrimarySkillType type : PrimarySkillType.values()) { + if (SkillTools.isChildSkill(type)) { + continue; + } + profile.modifySkill(type, level); + } + assertThat(manager.saveUser(profile)).isTrue(); + } + + private void createUserWithSkills(SQLDatabaseManager manager, + String name, + UUID uuid, + Map levels) { + manager.newUser(name, uuid); + PlayerProfile profile = manager.loadPlayerProfile(uuid); + for (Map.Entry e : levels.entrySet()) { + // modifySkill adds; starting level is 0 in tests + profile.modifySkill(e.getKey(), e.getValue()); + } + assertThat(manager.saveUser(profile)).isTrue(); + } + }