#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

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()]));
     }
 }

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<String> 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<String> banned) {
+    public void purgeRecords(final Set<String> banned) {
-        source.purgeBanned(banned);
+        source.purgeRecords(banned);
         cachedAuths.invalidateAll(banned);
     }
 

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<String> banned);
-
     /**
      * Return the data source type.
      *

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<String> banned) {
-        BufferedReader br = null;
-        BufferedWriter bw = null;
-        ArrayList<String> 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;

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<String> 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<String> banned) {
+    public void purgeRecords(Set<String> 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();
             }

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<String> 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;

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() {
+    public boolean isPurging() {
-        return this.autoPurging;
+        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) {
+    void setPurging(boolean purging) {
-        this.autoPurging = autoPurging;
+        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<String> initialPurge = dataSource.getRecordsToPurge(until);
 
-        if (CollectionUtils.isEmpty(initialPurge)) {
+        runPurge(null, until);
-            return;
-        }
-
-        // Remove players from the purge list if they have bypass permission
-        Set<String> 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);
     }
 
     /**
@@ -119,28 +102,34 @@ public class PurgeService implements Reloadable {
         //todo: note this should may run async because it may executes a SQL-Query
         Set<String> initialPurge = dataSource.getRecordsToPurge(until);
         if (CollectionUtils.isEmpty(initialPurge)) {
+            logAndSendMessage(sender, "No players to purge");
             return;
         }
 
         Set<String> toPurge = getFinalPurgeList(initialPurge);
-
+        purgePlayers(sender, toPurge, bukkitService.getOfflinePlayers());
-        // 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);
     }
 
-    public void purgeBanned(CommandSender sender, Set<String> bannedNames, Set<OfflinePlayer> 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<String> 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()];
+        dataSource.purgeRecords(names);
-        bannedPlayers.toArray(bannedPlayersArray);
+        logAndSendMessage(sender, ChatColor.GOLD + "Deleted " + names.size() + " user accounts");
-        PurgeTask purgeTask = new PurgeTask(this, sender, bannedNames, bannedPlayersArray, false);
+        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);

src/main/java/fr/xephi/authme/task/PurgeTask.java

@@ -23,14 +23,11 @@ public class PurgeTask extends BukkitRunnable {
     private final Set<String> toPurge;
 
     private final OfflinePlayer[] offlinePlayers;
-
-    private final boolean autoPurging;
     private final int totalPurgeCount;
 
     private int currentPage = 0;
 
-    public PurgeTask(PurgeService service, CommandSender sender, Set<String> toPurge, OfflinePlayer[] offlinePlayers,
+    public PurgeTask(PurgeService service, CommandSender sender, Set<String> toPurge, OfflinePlayer[] offlinePlayers) {
-                     boolean autoPurging) {
         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!");
+        ConsoleLogger.info("Purge Finished!");
-        if (autoPurging) {
+        purgeService.setPurging(false);
-            purgeService.setAutoPurging(false);
-        }
     }
 
     private void sendMessage(String message) {

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,

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));