From 3629c51fc1798533a6101f296db2858de88df66e Mon Sep 17 00:00:00 2001 From: ljacqu Date: Thu, 16 Jun 2016 22:52:11 +0200 Subject: [PATCH] #674 PurgeService: Always register if purging, reduce code duplication - Rename autoPurging to isPurging: we should always register if a purge task is in progress (regardless if autopurge or not) and deny any new requests - Reduce the same logic being coded multiple times by calling through the methods - DataSource: remove purgeBanned in favor of purgeRecords, both do exactly the same thing --- .../authme/PurgeBannedPlayersCommand.java | 2 +- .../authme/datasource/CacheDataSource.java | 12 +-- .../xephi/authme/datasource/DataSource.java | 7 -- .../fr/xephi/authme/datasource/FlatFile.java | 31 ------- .../fr/xephi/authme/datasource/MySQL.java | 17 +--- .../fr/xephi/authme/datasource/SQLite.java | 13 --- .../fr/xephi/authme/task/PurgeService.java | 87 +++++++++---------- .../java/fr/xephi/authme/task/PurgeTask.java | 12 +-- .../fr/xephi/authme/util/BukkitService.java | 9 ++ .../AbstractDataSourceIntegrationTest.java | 6 +- 10 files changed, 61 insertions(+), 135 deletions(-) diff --git a/src/main/java/fr/xephi/authme/command/executable/authme/PurgeBannedPlayersCommand.java b/src/main/java/fr/xephi/authme/command/executable/authme/PurgeBannedPlayersCommand.java index b298fd17a..30e3bd530 100644 --- a/src/main/java/fr/xephi/authme/command/executable/authme/PurgeBannedPlayersCommand.java +++ b/src/main/java/fr/xephi/authme/command/executable/authme/PurgeBannedPlayersCommand.java @@ -32,6 +32,6 @@ public class PurgeBannedPlayersCommand implements ExecutableCommand { namedBanned.add(offlinePlayer.getName().toLowerCase()); } - purgeService.purgeBanned(sender, namedBanned, bannedPlayers); + purgeService.purgePlayers(sender, namedBanned, bannedPlayers.toArray(new OfflinePlayer[bannedPlayers.size()])); } } diff --git a/src/main/java/fr/xephi/authme/datasource/CacheDataSource.java b/src/main/java/fr/xephi/authme/datasource/CacheDataSource.java index 547110e40..473633e4c 100644 --- a/src/main/java/fr/xephi/authme/datasource/CacheDataSource.java +++ b/src/main/java/fr/xephi/authme/datasource/CacheDataSource.java @@ -143,14 +143,6 @@ public class CacheDataSource implements DataSource { return source.getRecordsToPurge(until); } - @Override - public void purgeRecords(Set toPurge) { - source.purgeRecords(toPurge); - for (String name : toPurge) { - cachedAuths.invalidate(name); - } - } - @Override public boolean removeAuth(String name) { name = name.toLowerCase(); @@ -193,8 +185,8 @@ public class CacheDataSource implements DataSource { } @Override - public void purgeBanned(final Set banned) { - source.purgeBanned(banned); + public void purgeRecords(final Set banned) { + source.purgeRecords(banned); cachedAuths.invalidateAll(banned); } diff --git a/src/main/java/fr/xephi/authme/datasource/DataSource.java b/src/main/java/fr/xephi/authme/datasource/DataSource.java index 674841721..f1bb991a5 100644 --- a/src/main/java/fr/xephi/authme/datasource/DataSource.java +++ b/src/main/java/fr/xephi/authme/datasource/DataSource.java @@ -129,13 +129,6 @@ public interface DataSource extends Reloadable { */ void close(); - /** - * Purge all given players, i.e. delete all players whose name is in the list. - * - * @param banned the list of players to delete - */ - void purgeBanned(Set banned); - /** * Return the data source type. * diff --git a/src/main/java/fr/xephi/authme/datasource/FlatFile.java b/src/main/java/fr/xephi/authme/datasource/FlatFile.java index dc15bcb9a..7ae6026e1 100644 --- a/src/main/java/fr/xephi/authme/datasource/FlatFile.java +++ b/src/main/java/fr/xephi/authme/datasource/FlatFile.java @@ -441,37 +441,6 @@ public class FlatFile implements DataSource { return 0; } - @Override - public void purgeBanned(Set banned) { - BufferedReader br = null; - BufferedWriter bw = null; - ArrayList lines = new ArrayList<>(); - try { - br = new BufferedReader(new FileReader(source)); - String line; - while ((line = br.readLine()) != null) { - String[] args = line.split(":"); - try { - if (banned.contains(args[0])) { - lines.add(line); - } - } catch (NullPointerException | ArrayIndexOutOfBoundsException ignored) { - } - } - bw = new BufferedWriter(new FileWriter(source)); - for (String l : lines) { - bw.write(l + "\n"); - } - - } catch (IOException ex) { - ConsoleLogger.showError(ex.getMessage()); - - } finally { - silentClose(br); - silentClose(bw); - } - } - @Override public DataSourceType getType() { return DataSourceType.FILE; diff --git a/src/main/java/fr/xephi/authme/datasource/MySQL.java b/src/main/java/fr/xephi/authme/datasource/MySQL.java index 02aa66e3a..fbf51eb92 100644 --- a/src/main/java/fr/xephi/authme/datasource/MySQL.java +++ b/src/main/java/fr/xephi/authme/datasource/MySQL.java @@ -632,19 +632,6 @@ public class MySQL implements DataSource { return list; } - @Override - public void purgeRecords(Set toPurge) { - String delete = "DELETE FROM " + tableName + " WHERE " + col.NAME + "=?;"; - try (Connection con = getConnection(); PreparedStatement deletePst = con.prepareStatement(delete)) { - for (String name : toPurge) { - deletePst.setString(1, name); - deletePst.executeUpdate(); - } - } catch (SQLException ex) { - logSqlException(ex); - } - } - @Override public boolean removeAuth(String user) { user = user.toLowerCase(); @@ -752,10 +739,10 @@ public class MySQL implements DataSource { } @Override - public void purgeBanned(Set banned) { + public void purgeRecords(Set toPurge) { String sql = "DELETE FROM " + tableName + " WHERE " + col.NAME + "=?;"; try (Connection con = getConnection(); PreparedStatement pst = con.prepareStatement(sql)) { - for (String name : banned) { + for (String name : toPurge) { pst.setString(1, name); pst.executeUpdate(); } diff --git a/src/main/java/fr/xephi/authme/datasource/SQLite.java b/src/main/java/fr/xephi/authme/datasource/SQLite.java index fa8ddedda..d5109af26 100644 --- a/src/main/java/fr/xephi/authme/datasource/SQLite.java +++ b/src/main/java/fr/xephi/authme/datasource/SQLite.java @@ -452,19 +452,6 @@ public class SQLite implements DataSource { return 0; } - @Override - public void purgeBanned(Set banned) { - String sql = "DELETE FROM " + tableName + " WHERE " + col.NAME + "=?;"; - try (PreparedStatement pst = con.prepareStatement(sql)) { - for (String name : banned) { - pst.setString(1, name); - pst.executeUpdate(); - } - } catch (SQLException ex) { - logSqlException(ex); - } - } - @Override public DataSourceType getType() { return DataSourceType.SQLITE; diff --git a/src/main/java/fr/xephi/authme/task/PurgeService.java b/src/main/java/fr/xephi/authme/task/PurgeService.java index b577e4e5a..b7c680ba6 100644 --- a/src/main/java/fr/xephi/authme/task/PurgeService.java +++ b/src/main/java/fr/xephi/authme/task/PurgeService.java @@ -11,7 +11,6 @@ import fr.xephi.authme.settings.properties.PurgeSettings; import fr.xephi.authme.util.BukkitService; import fr.xephi.authme.util.CollectionUtils; import fr.xephi.authme.util.Utils; -import org.bukkit.Bukkit; import org.bukkit.ChatColor; import org.bukkit.OfflinePlayer; import org.bukkit.Server; @@ -46,10 +45,9 @@ public class PurgeService implements Reloadable { @Inject private Server server; - private boolean autoPurging = false; + private boolean isPurging = false; // Settings - private boolean useAutoPurge; private boolean removeEssentialsFiles; private boolean removePlayerDat; private boolean removeLimitedCreativeInventories; @@ -58,55 +56,40 @@ public class PurgeService implements Reloadable { private int daysBeforePurge; /** - * Return whether an automatic purge is in progress. + * Return whether a purge is in progress. * * @return True if purging. */ - public boolean isAutoPurging() { - return this.autoPurging; + public boolean isPurging() { + return this.isPurging; } /** - * Set if an automatic purge is currently in progress. + * Set if a purge is currently in progress. * - * @param autoPurging True if automatically purging. + * @param purging True if purging. */ - void setAutoPurging(boolean autoPurging) { - this.autoPurging = autoPurging; + void setPurging(boolean purging) { + this.isPurging = purging; } /** - * Purges players from the database. Ran on startup. + * Purges players from the database. Run on startup if enabled. */ public void runAutoPurge() { - if (!useAutoPurge || autoPurging) { + if (!settings.getProperty(PurgeSettings.USE_AUTO_PURGE)) { + return; + } else if (daysBeforePurge <= 0) { + ConsoleLogger.showError("Configured days before purging must be positive"); return; } - this.autoPurging = true; - - // Get the initial list of players to purge ConsoleLogger.info("Automatically purging the database..."); Calendar calendar = Calendar.getInstance(); calendar.add(Calendar.DATE, daysBeforePurge); long until = calendar.getTimeInMillis(); - Set initialPurge = dataSource.getRecordsToPurge(until); - if (CollectionUtils.isEmpty(initialPurge)) { - return; - } - - // Remove players from the purge list if they have bypass permission - Set toPurge = getFinalPurgeList(initialPurge); - - // Purge players from the database - dataSource.purgeRecords(toPurge); - ConsoleLogger.info("Purged the database: " + toPurge.size() + " accounts removed!"); - ConsoleLogger.info("Purging user accounts..."); - - // Schedule a PurgeTask - PurgeTask purgeTask = new PurgeTask(this, Bukkit.getConsoleSender(), toPurge, Bukkit.getOfflinePlayers(), true); - bukkitService.runTaskAsynchronously(purgeTask); + runPurge(null, until); } /** @@ -119,28 +102,34 @@ public class PurgeService implements Reloadable { //todo: note this should may run async because it may executes a SQL-Query Set initialPurge = dataSource.getRecordsToPurge(until); if (CollectionUtils.isEmpty(initialPurge)) { + logAndSendMessage(sender, "No players to purge"); return; } Set toPurge = getFinalPurgeList(initialPurge); - - // Purge records from the database - dataSource.purgeRecords(toPurge); - sender.sendMessage(ChatColor.GOLD + "Deleted " + toPurge.size() + " user accounts"); - sender.sendMessage(ChatColor.GOLD + "Purging user accounts..."); - - // Schedule a PurgeTask - PurgeTask purgeTask = new PurgeTask(this, sender, toPurge, Bukkit.getOfflinePlayers(), false); - bukkitService.runTaskAsynchronously(purgeTask); + purgePlayers(sender, toPurge, bukkitService.getOfflinePlayers()); } - public void purgeBanned(CommandSender sender, Set bannedNames, Set bannedPlayers) { + /** + * Purges the given list of player names. + * + * @param sender Sender running the command. + * @param names The names to remove. + * @param players Collection of OfflinePlayers (including those with the given names). + */ + public void purgePlayers(CommandSender sender, Set names, OfflinePlayer[] players) { //todo: note this should may run async because it may executes a SQL-Query - dataSource.purgeBanned(bannedNames); + if (isPurging) { + logAndSendMessage(sender, "Purge is already in progress! Aborting purge request"); + return; + } - OfflinePlayer[] bannedPlayersArray = new OfflinePlayer[bannedPlayers.size()]; - bannedPlayers.toArray(bannedPlayersArray); - PurgeTask purgeTask = new PurgeTask(this, sender, bannedNames, bannedPlayersArray, false); + dataSource.purgeRecords(names); + logAndSendMessage(sender, ChatColor.GOLD + "Deleted " + names.size() + " user accounts"); + logAndSendMessage(sender, ChatColor.GOLD + "Purging user accounts..."); + + isPurging = true; + PurgeTask purgeTask = new PurgeTask(this, sender, names, players); bukkitService.runTaskAsynchronously(purgeTask); } @@ -295,10 +284,16 @@ public class PurgeService implements Reloadable { ConsoleLogger.info("AutoPurge: Removed permissions from " + cleared.size() + " player(s)."); } + private static void logAndSendMessage(CommandSender sender, String message) { + ConsoleLogger.info(message); + if (sender != null) { + sender.sendMessage(message); + } + } + @PostConstruct @Override public void reload() { - this.useAutoPurge = settings.getProperty(PurgeSettings.USE_AUTO_PURGE); this.removeEssentialsFiles = settings.getProperty(PurgeSettings.REMOVE_ESSENTIALS_FILES); this.removePlayerDat = settings.getProperty(PurgeSettings.REMOVE_PLAYER_DAT); this.removeAntiXrayFiles = settings.getProperty(PurgeSettings.REMOVE_ANTI_XRAY_FILE); diff --git a/src/main/java/fr/xephi/authme/task/PurgeTask.java b/src/main/java/fr/xephi/authme/task/PurgeTask.java index edfb1b505..e2252da8d 100644 --- a/src/main/java/fr/xephi/authme/task/PurgeTask.java +++ b/src/main/java/fr/xephi/authme/task/PurgeTask.java @@ -23,14 +23,11 @@ public class PurgeTask extends BukkitRunnable { private final Set toPurge; private final OfflinePlayer[] offlinePlayers; - - private final boolean autoPurging; private final int totalPurgeCount; private int currentPage = 0; - public PurgeTask(PurgeService service, CommandSender sender, Set toPurge, OfflinePlayer[] offlinePlayers, - boolean autoPurging) { + public PurgeTask(PurgeService service, CommandSender sender, Set toPurge, OfflinePlayer[] offlinePlayers) { this.purgeService = service; if (sender instanceof Player) { this.sender = ((Player) sender).getUniqueId(); @@ -40,7 +37,6 @@ public class PurgeTask extends BukkitRunnable { this.toPurge = toPurge; this.totalPurgeCount = toPurge.size(); - this.autoPurging = autoPurging; this.offlinePlayers = offlinePlayers; } @@ -101,10 +97,8 @@ public class PurgeTask extends BukkitRunnable { // Show a status message sendMessage(ChatColor.GREEN + "[AuthMe] Database has been purged correctly"); - ConsoleLogger.info("AutoPurge Finished!"); - if (autoPurging) { - purgeService.setAutoPurging(false); - } + ConsoleLogger.info("Purge Finished!"); + purgeService.setPurging(false); } private void sendMessage(String message) { diff --git a/src/main/java/fr/xephi/authme/util/BukkitService.java b/src/main/java/fr/xephi/authme/util/BukkitService.java index 8b94fe49e..a88ebafc3 100644 --- a/src/main/java/fr/xephi/authme/util/BukkitService.java +++ b/src/main/java/fr/xephi/authme/util/BukkitService.java @@ -135,6 +135,15 @@ public class BukkitService { return Bukkit.getBannedPlayers(); } + /** + * Gets every player that has ever played on this server. + * + * @return an array containing all previous players + */ + public OfflinePlayer[] getOfflinePlayers() { + return Bukkit.getOfflinePlayers(); + } + /** * Safe way to retrieve the list of online players from the server. Depending on the * implementation of the server, either an array of {@link Player} instances is being returned, diff --git a/src/test/java/fr/xephi/authme/datasource/AbstractDataSourceIntegrationTest.java b/src/test/java/fr/xephi/authme/datasource/AbstractDataSourceIntegrationTest.java index bf8d54d5e..e9488a40e 100644 --- a/src/test/java/fr/xephi/authme/datasource/AbstractDataSourceIntegrationTest.java +++ b/src/test/java/fr/xephi/authme/datasource/AbstractDataSourceIntegrationTest.java @@ -5,13 +5,13 @@ import fr.xephi.authme.security.crypts.HashedPassword; import org.junit.Test; import java.util.Arrays; +import java.util.HashSet; import java.util.List; +import java.util.Set; import static fr.xephi.authme.AuthMeMatchers.equalToHash; import static fr.xephi.authme.AuthMeMatchers.hasAuthBasicData; import static fr.xephi.authme.AuthMeMatchers.hasAuthLocation; -import java.util.HashSet; -import java.util.Set; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.hasSize; @@ -227,7 +227,7 @@ public abstract class AbstractDataSourceIntegrationTest { assumeThat(dataSource.getAccountsRegistered(), equalTo(2)); // when - dataSource.purgeBanned(playersToDelete); + dataSource.purgeRecords(playersToDelete); // then assertThat(dataSource.getAccountsRegistered(), equalTo(1));