#784 Separate purge execution and purge task creation

- Delete accounts in PurgeExecutor, after permission check

src/main/java/fr/xephi/authme/datasource/CacheDataSource.java

@@ -15,6 +15,7 @@ import fr.xephi.authme.cache.auth.PlayerCache;
 import fr.xephi.authme.security.crypts.HashedPassword;
 
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
 import java.util.Set;
 import java.util.concurrent.Callable;
@@ -184,7 +185,7 @@ public class CacheDataSource implements DataSource {
     }
 
     @Override
-    public void purgeRecords(final Set<String> banned) {
+    public void purgeRecords(final Collection<String> banned) {
         source.purgeRecords(banned);
         cachedAuths.invalidateAll(banned);
     }

src/main/java/fr/xephi/authme/datasource/DataSource.java

@@ -4,6 +4,7 @@ import fr.xephi.authme.cache.auth.PlayerAuth;
 import fr.xephi.authme.initialization.Reloadable;
 import fr.xephi.authme.security.crypts.HashedPassword;
 
+import java.util.Collection;
 import java.util.List;
 import java.util.Set;
 
@@ -82,7 +83,7 @@ public interface DataSource extends Reloadable {
      *
      * @param toPurge The players to purge
      */
-    void purgeRecords(Set<String> toPurge);
+    void purgeRecords(Collection<String> toPurge);
 
     /**
      * Remove a user record from the database.

src/main/java/fr/xephi/authme/datasource/FlatFile.java

@@ -17,6 +17,7 @@ import java.io.FileReader;
 import java.io.FileWriter;
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
@@ -256,7 +257,7 @@ public class FlatFile implements DataSource {
     }
 
     @Override
-    public void purgeRecords(Set<String> toPurge) {
+    public void purgeRecords(Collection<String> toPurge) {
         BufferedReader br = null;
         BufferedWriter bw = null;
         ArrayList<String> lines = new ArrayList<>();

src/main/java/fr/xephi/authme/datasource/MySQL.java

@@ -23,6 +23,7 @@ import java.sql.SQLException;
 import java.sql.Statement;
 import java.sql.Types;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
@@ -693,7 +694,7 @@ public class MySQL implements DataSource {
     }
 
     @Override
-    public void purgeRecords(Set<String> toPurge) {
+    public void purgeRecords(Collection<String> toPurge) {
         String sql = "DELETE FROM " + tableName + " WHERE " + col.NAME + "=?;";
         try (Connection con = getConnection(); PreparedStatement pst = con.prepareStatement(sql)) {
             for (String name : toPurge) {

src/main/java/fr/xephi/authme/datasource/SQLite.java

@@ -16,6 +16,7 @@ import java.sql.ResultSet;
 import java.sql.SQLException;
 import java.sql.Statement;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
@@ -317,7 +318,7 @@ public class SQLite implements DataSource {
     }
 
     @Override
-    public void purgeRecords(Set<String> toPurge) {
+    public void purgeRecords(Collection<String> toPurge) {
         String delete = "DELETE FROM " + tableName + " WHERE " + col.NAME + "=?;";
         try (PreparedStatement deletePst = con.prepareStatement(delete)) {
             for (String name : toPurge) {

src/main/java/fr/xephi/authme/task/purge/PurgeExecutor.java

@@ -0,0 +1,211 @@
+package fr.xephi.authme.task.purge;
+
+import fr.xephi.authme.ConsoleLogger;
+import fr.xephi.authme.datasource.DataSource;
+import fr.xephi.authme.hooks.PluginHooks;
+import fr.xephi.authme.permission.PermissionsManager;
+import fr.xephi.authme.settings.NewSetting;
+import fr.xephi.authme.settings.properties.PurgeSettings;
+import fr.xephi.authme.util.BukkitService;
+import fr.xephi.authme.util.Utils;
+import org.bukkit.ChatColor;
+import org.bukkit.OfflinePlayer;
+import org.bukkit.Server;
+
+import javax.inject.Inject;
+import java.io.File;
+import java.util.Collection;
+
+import static fr.xephi.authme.util.StringUtils.makePath;
+
+/**
+ * Executes the purge operations.
+ */
+class PurgeExecutor {
+
+    @Inject
+    private NewSetting settings;
+
+    @Inject
+    private DataSource dataSource;
+
+    @Inject
+    private PermissionsManager permissionsManager;
+
+    @Inject
+    private PluginHooks pluginHooks;
+
+    @Inject
+    private BukkitService bukkitService;
+
+    @Inject
+    private Server server;
+
+    PurgeExecutor() {
+    }
+
+    /**
+     * Performs the purge operations, i.e. deletes data and removes the files associated with the given
+     * players and names.
+     *
+     * @param players the players to purge
+     * @param names names to purge (lowercase)
+     */
+    public void executePurge(Collection<OfflinePlayer> players, Collection<String> names) {
+        // Purge other data
+        purgeFromAuthMe(names);
+        purgeEssentials(players);
+        purgeDat(players);
+        purgeLimitedCreative(names);
+        purgeAntiXray(names);
+        purgePermissions(players);
+    }
+
+    synchronized void purgeAntiXray(Collection<String> cleared) {
+        if (!settings.getProperty(PurgeSettings.REMOVE_ANTI_XRAY_FILE)) {
+            return;
+        }
+
+        int i = 0;
+        File dataFolder = new File("." + File.separator + "plugins" + File.separator + "AntiXRayData"
+            + File.separator + "PlayerData");
+        if (!dataFolder.exists() || !dataFolder.isDirectory()) {
+            return;
+        }
+
+        for (String file : dataFolder.list()) {
+            if (cleared.contains(file.toLowerCase())) {
+                File playerFile = new File(dataFolder, file);
+                if (playerFile.exists() && playerFile.delete()) {
+                    i++;
+                }
+            }
+        }
+
+        ConsoleLogger.info("AutoPurge: Removed " + i + " AntiXRayData Files");
+    }
+
+    /**
+     * Deletes the given accounts from AuthMe.
+     *
+     * @param names the name of the accounts to delete
+     */
+    synchronized void purgeFromAuthMe(Collection<String> names) {
+        dataSource.purgeRecords(names);
+        // TODO ljacqu 20160717: We shouldn't output namedBanned.size() but the actual total that was deleted
+        ConsoleLogger.info(ChatColor.GOLD + "Deleted " + names.size() + " user accounts");
+    }
+
+    synchronized void purgeLimitedCreative(Collection<String> cleared) {
+        if (!settings.getProperty(PurgeSettings.REMOVE_LIMITED_CREATIVE_INVENTORIES)) {
+            return;
+        }
+
+        int i = 0;
+        File dataFolder = new File("." + File.separator + "plugins" + File.separator + "LimitedCreative"
+            + File.separator + "inventories");
+        if (!dataFolder.exists() || !dataFolder.isDirectory()) {
+            return;
+        }
+        for (String file : dataFolder.list()) {
+            String name = file;
+            int idx;
+            idx = file.lastIndexOf("_creative.yml");
+            if (idx != -1) {
+                name = name.substring(0, idx);
+            } else {
+                idx = file.lastIndexOf("_adventure.yml");
+                if (idx != -1) {
+                    name = name.substring(0, idx);
+                } else {
+                    idx = file.lastIndexOf(".yml");
+                    if (idx != -1) {
+                        name = name.substring(0, idx);
+                    }
+                }
+            }
+            if (name.equals(file)) {
+                continue;
+            }
+            if (cleared.contains(name.toLowerCase())) {
+                File dataFile = new File(dataFolder, file);
+                if (dataFile.exists() && dataFile.delete()) {
+                    i++;
+                }
+            }
+        }
+        ConsoleLogger.info("AutoPurge: Removed " + i + " LimitedCreative Survival, Creative and Adventure files");
+    }
+
+    /**
+     * Removes the .dat file of the given players.
+     *
+     * @param cleared list of players to clear
+     */
+    synchronized void purgeDat(Collection<OfflinePlayer> cleared) {
+        if (!settings.getProperty(PurgeSettings.REMOVE_PLAYER_DAT)) {
+            return;
+        }
+
+        int i = 0;
+        File dataFolder = new File(server.getWorldContainer()
+            , makePath(settings.getProperty(PurgeSettings.DEFAULT_WORLD), "players"));
+
+        for (OfflinePlayer offlinePlayer : cleared) {
+            File playerFile = new File(dataFolder, Utils.getUUIDorName(offlinePlayer) + ".dat");
+            if (playerFile.delete()) {
+                i++;
+            }
+        }
+
+        ConsoleLogger.info("AutoPurge: Removed " + i + " .dat Files");
+    }
+
+    /**
+     * Removes the Essentials userdata file of each given player.
+     *
+     * @param cleared list of players to clear
+     */
+    synchronized void purgeEssentials(Collection<OfflinePlayer> cleared) {
+        if (!settings.getProperty(PurgeSettings.REMOVE_ESSENTIALS_FILES)) {
+            return;
+        }
+
+        int i = 0;
+        File essentialsDataFolder = pluginHooks.getEssentialsDataFolder();
+        if (essentialsDataFolder == null) {
+            ConsoleLogger.info("Cannot purge Essentials: plugin is not loaded");
+            return;
+        }
+
+        final File userDataFolder = new File(essentialsDataFolder, "userdata");
+        if (!userDataFolder.exists() || !userDataFolder.isDirectory()) {
+            return;
+        }
+
+        for (OfflinePlayer offlinePlayer : cleared) {
+            File playerFile = new File(userDataFolder, Utils.getUUIDorName(offlinePlayer) + ".yml");
+            if (playerFile.exists() && playerFile.delete()) {
+                i++;
+            }
+        }
+
+        ConsoleLogger.info("AutoPurge: Removed " + i + " EssentialsFiles");
+    }
+
+    // TODO: What is this method for? Is it correct?
+    // TODO: Make it work with OfflinePlayers group data.
+    synchronized void purgePermissions(Collection<OfflinePlayer> cleared) {
+        if (!settings.getProperty(PurgeSettings.REMOVE_PERMISSIONS)) {
+            return;
+        }
+
+        for (OfflinePlayer offlinePlayer : cleared) {
+            String name = offlinePlayer.getName();
+            permissionsManager.removeAllGroups(bukkitService.getPlayerExact(name));
+        }
+
+        ConsoleLogger.info("AutoPurge: Removed permissions from " + cleared.size() + " player(s).");
+    }
+    
+}

src/main/java/fr/xephi/authme/task/purge/PurgeService.java

@@ -2,26 +2,20 @@ package fr.xephi.authme.task.purge;
 
 import fr.xephi.authme.ConsoleLogger;
 import fr.xephi.authme.datasource.DataSource;
-import fr.xephi.authme.hooks.PluginHooks;
 import fr.xephi.authme.permission.PermissionsManager;
 import fr.xephi.authme.settings.NewSetting;
 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.ChatColor;
 import org.bukkit.OfflinePlayer;
-import org.bukkit.Server;
 import org.bukkit.command.CommandSender;
 import org.bukkit.command.ConsoleCommandSender;
 
 import javax.inject.Inject;
-import java.io.File;
 import java.util.Calendar;
+import java.util.Collection;
 import java.util.Set;
 
-import static fr.xephi.authme.util.StringUtils.makePath;
-
 public class PurgeService {
 
     @Inject
@@ -37,21 +31,11 @@ public class PurgeService {
     private PermissionsManager permissionsManager;
 
     @Inject
-    private PluginHooks pluginHooks;
+    private PurgeExecutor purgeExecutor;
-
-    @Inject
-    private Server server;
 
     private boolean isPurging = false;
 
-
+    PurgeService() {
-    /**
-     * Set if a purge is currently in progress.
-     *
-     * @param purging True if purging.
-     */
-    void setPurging(boolean purging) {
-        this.isPurging = purging;
     }
 
     /**
@@ -93,157 +77,40 @@ public class PurgeService {
     }
 
     /**
-     * Purges all banned players.
+     * 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
         if (isPurging) {
             logAndSendMessage(sender, "Purge is already in progress! Aborting purge request");
             return;
         }
 
-        // FIXME #784: We can no longer delete records here -> permission check happens inside PurgeTask
-        dataSource.purgeRecords(names);
-        // TODO ljacqu 20160717: We shouldn't output namedBanned.size() but the actual total that was deleted
-        logAndSendMessage(sender, ChatColor.GOLD + "Deleted " + names.size() + " user accounts");
-        logAndSendMessage(sender, ChatColor.GOLD + "Purging user accounts...");
-
         isPurging = true;
         PurgeTask purgeTask = new PurgeTask(this, permissionsManager, sender, names, players);
         bukkitService.runTaskAsynchronously(purgeTask);
     }
 
-    synchronized void purgeAntiXray(Set<String> cleared) {
+    /**
-        if (!settings.getProperty(PurgeSettings.REMOVE_ANTI_XRAY_FILE)) {
+     * Set if a purge is currently in progress.
-            return;
+     *
-        }
+     * @param purging True if purging.
-
+     */
-        int i = 0;
+    void setPurging(boolean purging) {
-        File dataFolder = new File("." + File.separator + "plugins" + File.separator + "AntiXRayData"
+        this.isPurging = purging;
-            + File.separator + "PlayerData");
-        if (!dataFolder.exists() || !dataFolder.isDirectory()) {
-            return;
-        }
-
-        for (String file : dataFolder.list()) {
-            if (cleared.contains(file.toLowerCase())) {
-                File playerFile = new File(dataFolder, file);
-                if (playerFile.exists() && playerFile.delete()) {
-                    i++;
-                }
-            }
-        }
-
-        ConsoleLogger.info("AutoPurge: Removed " + i + " AntiXRayData Files");
-    }
-
-    synchronized void purgeLimitedCreative(Set<String> cleared) {
-        if (!settings.getProperty(PurgeSettings.REMOVE_LIMITED_CREATIVE_INVENTORIES)) {
-            return;
-        }
-
-        int i = 0;
-        File dataFolder = new File("." + File.separator + "plugins" + File.separator + "LimitedCreative"
-            + File.separator + "inventories");
-        if (!dataFolder.exists() || !dataFolder.isDirectory()) {
-            return;
-        }
-        for (String file : dataFolder.list()) {
-            String name = file;
-            int idx;
-            idx = file.lastIndexOf("_creative.yml");
-            if (idx != -1) {
-                name = name.substring(0, idx);
-            } else {
-                idx = file.lastIndexOf("_adventure.yml");
-                if (idx != -1) {
-                    name = name.substring(0, idx);
-                } else {
-                    idx = file.lastIndexOf(".yml");
-                    if (idx != -1) {
-                        name = name.substring(0, idx);
-                    }
-                }
-            }
-            if (name.equals(file)) {
-                continue;
-            }
-            if (cleared.contains(name.toLowerCase())) {
-                File dataFile = new File(dataFolder, file);
-                if (dataFile.exists() && dataFile.delete()) {
-                    i++;
-                }
-            }
-        }
-        ConsoleLogger.info("AutoPurge: Removed " + i + " LimitedCreative Survival, Creative and Adventure files");
-    }
-
-    synchronized void purgeDat(Set<OfflinePlayer> cleared) {
-        if (!settings.getProperty(PurgeSettings.REMOVE_PLAYER_DAT)) {
-            return;
-        }
-
-        int i = 0;
-        File dataFolder = new File(server.getWorldContainer()
-            , makePath(settings.getProperty(PurgeSettings.DEFAULT_WORLD), "players"));
-
-        for (OfflinePlayer offlinePlayer : cleared) {
-            File playerFile = new File(dataFolder, Utils.getUUIDorName(offlinePlayer) + ".dat");
-            if (playerFile.delete()) {
-                i++;
-            }
-        }
-
-        ConsoleLogger.info("AutoPurge: Removed " + i + " .dat Files");
     }
 
     /**
-     * Method purgeEssentials.
+     * Perform purge operations for the given players and names.
      *
-     * @param cleared List of String
+     * @param players the players (associated with the names)
+     * @param names the lowercase names
      */
-    synchronized void purgeEssentials(Set<OfflinePlayer> cleared) {
+    void executePurge(Collection<OfflinePlayer> players, Collection<String> names) {
-        if (!settings.getProperty(PurgeSettings.REMOVE_ESSENTIALS_FILES)) {
+        purgeExecutor.executePurge(players, names);
-            return;
-        }
-
-        int i = 0;
-        File essentialsDataFolder = pluginHooks.getEssentialsDataFolder();
-        if (essentialsDataFolder == null) {
-            ConsoleLogger.info("Cannot purge Essentials: plugin is not loaded");
-            return;
-        }
-
-        final File userDataFolder = new File(essentialsDataFolder, "userdata");
-        if (!userDataFolder.exists() || !userDataFolder.isDirectory()) {
-            return;
-        }
-
-        for (OfflinePlayer offlinePlayer : cleared) {
-            File playerFile = new File(userDataFolder, Utils.getUUIDorName(offlinePlayer) + ".yml");
-            if (playerFile.exists() && playerFile.delete()) {
-                i++;
-            }
-        }
-
-        ConsoleLogger.info("AutoPurge: Removed " + i + " EssentialsFiles");
-    }
-
-    // TODO: What is this method for? Is it correct?
-    // TODO: Make it work with OfflinePlayers group data.
-    synchronized void purgePermissions(Set<OfflinePlayer> cleared) {
-        if (!settings.getProperty(PurgeSettings.REMOVE_PERMISSIONS)) {
-            return;
-        }
-
-        for (OfflinePlayer offlinePlayer : cleared) {
-            String name = offlinePlayer.getName();
-            permissionsManager.removeAllGroups(bukkitService.getPlayerExact(name));
-        }
-
-        ConsoleLogger.info("AutoPurge: Removed permissions from " + cleared.size() + " player(s).");
     }
 
     private static void logAndSendMessage(CommandSender sender, String message) {

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

@@ -14,7 +14,7 @@ import java.util.HashSet;
 import java.util.Set;
 import java.util.UUID;
 
-public class PurgeTask extends BukkitRunnable {
+class PurgeTask extends BukkitRunnable {
 
     //how many players we should check for each tick
     private static final int INTERVAL_CHECK = 5;
@@ -38,8 +38,8 @@ public class PurgeTask extends BukkitRunnable {
      * @param toPurge lowercase names to purge
      * @param offlinePlayers offline players to map to the names
      */
-    public PurgeTask(PurgeService service, PermissionsManager permissionsManager, CommandSender sender,
+    PurgeTask(PurgeService service, PermissionsManager permissionsManager, CommandSender sender,
-                     Set<String> toPurge, OfflinePlayer[] offlinePlayers) {
+              Set<String> toPurge, OfflinePlayer[] offlinePlayers) {
         this.purgeService = service;
         this.permissionsManager = permissionsManager;
 
@@ -93,22 +93,13 @@ public class PurgeTask extends BukkitRunnable {
         }
 
         currentPage++;
-        purgeData(playerPortion, namePortion);
+        purgeService.executePurge(playerPortion, namePortion);
         if (currentPage % 20 == 0) {
             int completed = totalPurgeCount - toPurge.size();
             sendMessage("[AuthMe] Purge progress " + completed + '/' + totalPurgeCount);
         }
     }
 
-    private void purgeData(Set<OfflinePlayer> playerPortion, Set<String> namePortion) {
-        // Purge other data
-        purgeService.purgeEssentials(playerPortion);
-        purgeService.purgeDat(playerPortion);
-        purgeService.purgeLimitedCreative(namePortion);
-        purgeService.purgeAntiXray(namePortion);
-        purgeService.purgePermissions(playerPortion);
-    }
-
     private void finish() {
         cancel();
 

src/test/java/fr/xephi/authme/datasource/AbstractResourceClosingTest.java

@@ -219,7 +219,7 @@ public abstract class AbstractResourceClosingTest {
 
             Object element = PARAM_VALUES.get(genericType);
             Preconditions.checkNotNull(element, "No sample element for list of generic type " + genericType);
-            if (List.class == parameterizedType.getRawType()) {
+            if (isAssignableFrom(parameterizedType.getRawType(), List.class)) {
                 return Arrays.asList(element, element, element);
             } else if (Set.class == parameterizedType.getRawType()) {
                 return new HashSet<>(Arrays.asList(element, element, element));
@@ -229,6 +229,11 @@ public abstract class AbstractResourceClosingTest {
         throw new IllegalStateException("Cannot build list for unexpected Type: " + type);
     }
 
+    private static boolean isAssignableFrom(Type type, Class<?> fromType) {
+        return (type instanceof Class<?>)
+            && ((Class<?>) type).isAssignableFrom(fromType);
+    }
+
     /* Initialize the map of test values to pass to methods to satisfy their signature. */
     private static Map<Class<?>, Object> getDefaultParameters() {
         HashedPassword hash = new HashedPassword("test", "test");

src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java

@@ -3,7 +3,6 @@ package fr.xephi.authme.task.purge;
 import fr.xephi.authme.ReflectionTestUtils;
 import fr.xephi.authme.TestHelper;
 import fr.xephi.authme.datasource.DataSource;
-import fr.xephi.authme.hooks.PluginHooks;
 import fr.xephi.authme.permission.PermissionsManager;
 import fr.xephi.authme.runner.BeforeInjecting;
 import fr.xephi.authme.runner.DelayedInjectionRunner;
@@ -12,7 +11,6 @@ import fr.xephi.authme.settings.NewSetting;
 import fr.xephi.authme.settings.properties.PurgeSettings;
 import fr.xephi.authme.util.BukkitService;
 import org.bukkit.OfflinePlayer;
-import org.bukkit.Server;
 import org.bukkit.command.CommandSender;
 import org.bukkit.entity.Player;
 import org.hamcrest.Matchers;
@@ -61,9 +59,7 @@ public class PurgeServiceTest {
     @Mock
     private PermissionsManager permissionsManager;
     @Mock
-    private PluginHooks pluginHooks;
+    private PurgeExecutor executor;
-    @Mock
-    private Server server;
 
     @BeforeClass
     public static void initLogger() {
@@ -116,7 +112,6 @@ public class PurgeServiceTest {
         ArgumentCaptor<Long> captor = ArgumentCaptor.forClass(Long.class);
         verify(dataSource).getRecordsToPurge(captor.capture());
         assertCorrectPurgeTimestamp(captor.getValue(), 60);
-        verify(dataSource).purgeRecords(playerNames);
         assertThat(Boolean.TRUE.equals(
             ReflectionTestUtils.getFieldValue(PurgeService.class, purgeService, "isPurging")), equalTo(true));
         verifyScheduledPurgeTask(null, playerNames);
@@ -156,9 +151,6 @@ public class PurgeServiceTest {
 
         // then
         verify(dataSource).getRecordsToPurge(delay);
-        verify(dataSource).purgeRecords(playerNames);
-        // FIXME #784: Deleting accounts needs to be handled differently
-        verify(sender).sendMessage(argThat(containsString("Deleted 4 user accounts")));
         verifyScheduledPurgeTask(uuid, playerNames);
     }