#886 Do not include players with lastlogin = 0 in autopurge or default purge

src/main/java/fr/xephi/authme/command/CommandInitializer.java

@@ -219,6 +219,7 @@ public class CommandInitializer {
             .description("Purge old data")
             .detailedDescription("Purge old AuthMeReloaded data longer than the specified amount of days ago.")
             .withArgument("days", "Number of days", false)
+            .withArgument("all", "Add 'all' at the end to also purge players with lastlogin = 0", true)
             .permission(AdminPermission.PURGE)
             .executableCommand(PurgeCommand.class)
             .build();

src/main/java/fr/xephi/authme/command/executable/authme/PurgeCommand.java

@@ -10,7 +10,7 @@ import java.util.Calendar;
 import java.util.List;
 
 /**
- * Command for purging the data of players which have not been since for a given number
+ * Command for purging the data of players which have not been online for a given number
  * of days. Depending on the settings, this removes player data in third-party plugins as well.
  */
 public class PurgeCommand implements ExecutableCommand {
@@ -41,12 +41,24 @@ public class PurgeCommand implements ExecutableCommand {
             return;
         }
 
+        // If second param is available, check that it is equal to "all"
+        boolean includeLastLoginZeroEntries = false;
+        if (arguments.size() >= 2) {
+            if ("all".equals(arguments.get(1))) {
+                includeLastLoginZeroEntries = true;
+            } else {
+                sender.sendMessage("Purge process aborted; use '/authme purge " + days + " all' "
+                    + "to include users with lastlogin = 0");
+                return;
+            }
+        }
+
         // Create a calender instance to determine the date
         Calendar calendar = Calendar.getInstance();
         calendar.add(Calendar.DATE, -days);
         long until = calendar.getTimeInMillis();
 
         // Run the purge
-        purgeService.runPurge(sender, until);
+        purgeService.runPurge(sender, until, includeLastLoginZeroEntries);
     }
 }

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

@@ -138,8 +138,8 @@ public class CacheDataSource implements DataSource {
     }
 
     @Override
-    public Set<String> getRecordsToPurge(long until) {
-        return source.getRecordsToPurge(until);
+    public Set<String> getRecordsToPurge(long until, boolean includeEntriesWithLastLoginZero) {
+        return source.getRecordsToPurge(until, includeEntriesWithLastLoginZero);
     }
 
     @Override

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

@@ -74,9 +74,11 @@ public interface DataSource extends Reloadable {
      * Get all records in the database whose last login was before the given time.
      *
      * @param until The minimum last login
+     * @param includeEntriesWithLastLoginZero Whether entries with lastlogin = 0 should be included or not,
+     *        see <a href="https://github.com/Xephi/AuthMeReloaded/issues/886">issue #886</a>
      * @return The account names selected to purge
      */
-    Set<String> getRecordsToPurge(long until);
+    Set<String> getRecordsToPurge(long until, boolean includeEntriesWithLastLoginZero);
 
     /**
      * Purge the given players from the database.

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

@@ -15,7 +15,6 @@ 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;
 
@@ -211,30 +210,8 @@ public class FlatFile implements DataSource {
     }
 
     @Override
-    public Set<String> getRecordsToPurge(long until) {
-        BufferedReader br = null;
-        Set<String> list = new HashSet<>();
-
-        try {
-            br = new BufferedReader(new FileReader(source));
-            String line;
-            while ((line = br.readLine()) != null) {
-                String[] args = line.split(":");
-                if (args.length >= 4) {
-                    if (Long.parseLong(args[3]) >= until) {
-                        list.add(args[0]);
-                        continue;
-                    }
-                }
-            }
-        } catch (IOException ex) {
-            ConsoleLogger.warning(ex.getMessage());
-            return list;
-        } finally {
-            silentClose(br);
-        }
-
-        return list;
+    public Set<String> getRecordsToPurge(long until, boolean includeEntriesWithLastLoginZero) {
+        throw new UnsupportedOperationException("Flat file no longer supported");
     }
 
     @Override

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

@@ -572,10 +572,13 @@ public class MySQL implements DataSource {
     }
 
     @Override
-    public Set<String> getRecordsToPurge(long until) {
+    public Set<String> getRecordsToPurge(long until, boolean includeEntriesWithLastLoginZero) {
         Set<String> list = new HashSet<>();
 
-        String select = "SELECT " + col.NAME + " FROM " + tableName + " WHERE " + col.LAST_LOGIN + "<?;";
+        String select = "SELECT " + col.NAME + " FROM " + tableName + " WHERE " + col.LAST_LOGIN + " < ?";
+        if (!includeEntriesWithLastLoginZero) {
+            select += " AND " + col.LAST_LOGIN + " <> 0";
+        }
         try (Connection con = getConnection();
              PreparedStatement selectPst = con.prepareStatement(select)) {
             selectPst.setLong(1, until);

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

@@ -299,10 +299,13 @@ public class SQLite implements DataSource {
     }
 
     @Override
-    public Set<String> getRecordsToPurge(long until) {
+    public Set<String> getRecordsToPurge(long until, boolean includeEntriesWithLastLoginZero) {
         Set<String> list = new HashSet<>();
 
-        String select = "SELECT " + col.NAME + " FROM " + tableName + " WHERE " + col.LAST_LOGIN + "<?;";
+        String select = "SELECT " + col.NAME + " FROM " + tableName + " WHERE " + col.LAST_LOGIN + " < ?";
+        if (!includeEntriesWithLastLoginZero) {
+            select += " AND " + col.LAST_LOGIN + " <> 0";
+        }
         try (PreparedStatement selectPst = con.prepareStatement(select)) {
             selectPst.setLong(1, until);
             try (ResultSet rs = selectPst.executeQuery()) {

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

@@ -16,6 +16,9 @@ import java.util.Calendar;
 import java.util.Collection;
 import java.util.Set;
 
+/**
+ * Initiates purge tasks.
+ */
 public class PurgeService {
 
     @Inject
@@ -33,6 +36,7 @@ public class PurgeService {
     @Inject
     private PurgeExecutor purgeExecutor;
 
+    /** Keeps track of whether a purge task is currently running. */
     private boolean isPurging = false;
 
     PurgeService() {
@@ -55,7 +59,7 @@ public class PurgeService {
         calendar.add(Calendar.DATE, -daysBeforePurge);
         long until = calendar.getTimeInMillis();
 
-        runPurge(null, until);
+        runPurge(null, until, false);
     }
 
     /**
@@ -64,10 +68,11 @@ public class PurgeService {
      *
      * @param sender Sender running the command
      * @param until The last login threshold in milliseconds
+     * @param includeEntriesWithLastLoginZero True to also purge players with lastlogin = 0, false otherwise
      */
-    public void runPurge(CommandSender sender, long until) {
+    public void runPurge(CommandSender sender, long until, boolean includeEntriesWithLastLoginZero) {
         //todo: note this should may run async because it may executes a SQL-Query
-        Set<String> toPurge = dataSource.getRecordsToPurge(until);
+        Set<String> toPurge = dataSource.getRecordsToPurge(until, includeEntriesWithLastLoginZero);
         if (CollectionUtils.isEmpty(toPurge)) {
             logAndSendMessage(sender, "No players to purge");
             return;

src/test/java/fr/xephi/authme/command/executable/authme/PurgeCommandTest.java

@@ -9,6 +9,7 @@ import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.runners.MockitoJUnitRunner;
 
+import java.util.Arrays;
 import java.util.Calendar;
 import java.util.Collections;
 
@@ -73,7 +74,7 @@ public class PurgeCommandTest {
 
         // then
         ArgumentCaptor<Long> captor = ArgumentCaptor.forClass(Long.class);
-        verify(purgeService).runPurge(eq(sender), captor.capture());
+        verify(purgeService).runPurge(eq(sender), captor.capture(), eq(false));
 
         // Check the timestamp with a certain tolerance
         int toleranceMillis = 100;
@@ -82,6 +83,41 @@ public class PurgeCommandTest {
         assertIsCloseTo(captor.getValue(), calendar.getTimeInMillis(), toleranceMillis);
     }
 
+    @Test
+    public void shouldProcessCommandWithAllParameter() {
+        // given
+        String interval = "32";
+        CommandSender sender = mock(CommandSender.class);
+
+        // when
+        command.executeCommand(sender, Arrays.asList(interval, "all"));
+
+        // then
+        ArgumentCaptor<Long> captor = ArgumentCaptor.forClass(Long.class);
+        verify(purgeService).runPurge(eq(sender), captor.capture(), eq(true));
+
+        // Check the timestamp with a certain tolerance
+        int toleranceMillis = 100;
+        Calendar calendar = Calendar.getInstance();
+        calendar.add(Calendar.DATE, -Integer.valueOf(interval));
+        assertIsCloseTo(captor.getValue(), calendar.getTimeInMillis(), toleranceMillis);
+    }
+
+    @Test
+    public void shouldRejectCommandWithInvalidSecondParameter() {
+        // given
+        String interval = "80";
+        CommandSender sender = mock(CommandSender.class);
+
+        // when
+        command.executeCommand(sender, Arrays.asList(interval, "bogus"));
+
+        // then
+        verify(sender).sendMessage(
+            argThat(containsString("Purge process aborted; use '/authme purge " + interval + " all'")));
+        verifyZeroInteractions(purgeService);
+    }
+
     private static void assertIsCloseTo(long value1, long value2, long tolerance) {
         assertThat(Math.abs(value1 - value2), not(greaterThan(tolerance)));
     }

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

@@ -328,14 +328,16 @@ public abstract class AbstractDataSourceIntegrationTest {
     public void shouldGetRecordsToPurge() {
         // given
         DataSource dataSource = getDataSource();
-        // 1453242857 -> user, 1449136800 -> bobby
+        PlayerAuth auth = PlayerAuth.builder().name("potato").lastLogin(0).build();
+        dataSource.saveAuth(auth);
+        // 1453242857 -> user, 1449136800 -> bobby, 0 -> potato
 
         // when
-        Set<String> records1 = dataSource.getRecordsToPurge(1450000000);
-        Set<String> records2 = dataSource.getRecordsToPurge(1460000000);
+        Set<String> records1 = dataSource.getRecordsToPurge(1450000000, true);
+        Set<String> records2 = dataSource.getRecordsToPurge(1460000000, false);
 
         // then
-        assertThat(records1, contains("bobby"));
+        assertThat(records1, containsInAnyOrder("bobby", "potato"));
         assertThat(records2, containsInAnyOrder("bobby", "user"));
         // check that the entry was not deleted because of running this command
         assertThat(dataSource.isAuthAvailable("bobby"), equalTo(true));

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

@@ -241,6 +241,7 @@ public abstract class AbstractResourceClosingTest {
             .put(String.class, "test")
             .put(int.class, 3)
             .put(long.class, 102L)
+            .put(boolean.class, true)
             .put(PlayerAuth.class, PlayerAuth.builder().name("test").realName("test").password(hash).build())
             .put(HashedPassword.class, hash)
             .build();

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

@@ -10,7 +10,6 @@ import fr.xephi.authme.util.BukkitService;
 import org.bukkit.OfflinePlayer;
 import org.bukkit.command.CommandSender;
 import org.bukkit.entity.Player;
-import org.hamcrest.Matchers;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -100,7 +99,7 @@ public class PurgeServiceTest {
         given(settings.getProperty(PurgeSettings.USE_AUTO_PURGE)).willReturn(true);
         given(settings.getProperty(PurgeSettings.DAYS_BEFORE_REMOVE_PLAYER)).willReturn(60);
         Set<String> playerNames = newHashSet("alpha", "bravo", "charlie", "delta");
-        given(dataSource.getRecordsToPurge(anyLong())).willReturn(playerNames);
+        given(dataSource.getRecordsToPurge(anyLong(), eq(false))).willReturn(playerNames);
         mockReturnedOfflinePlayers();
 
         // when
@@ -108,7 +107,7 @@ public class PurgeServiceTest {
 
         // then
         ArgumentCaptor<Long> captor = ArgumentCaptor.forClass(Long.class);
-        verify(dataSource).getRecordsToPurge(captor.capture());
+        verify(dataSource).getRecordsToPurge(captor.capture(), eq(false));
         assertCorrectPurgeTimestamp(captor.getValue(), 60);
         assertThat(Boolean.TRUE, equalTo(
             ReflectionTestUtils.getFieldValue(PurgeService.class, purgeService, "isPurging")));
@@ -118,15 +117,16 @@ public class PurgeServiceTest {
     @Test
     public void shouldRecognizeNoPlayersToPurge() {
         // given
-        long delay = 123012301L;
-        given(dataSource.getRecordsToPurge(delay)).willReturn(Collections.<String>emptySet());
+        final long delay = 123012301L;
+        final boolean includeLastLoginZeroEntries = true;
+        given(dataSource.getRecordsToPurge(delay, includeLastLoginZeroEntries)).willReturn(Collections.<String>emptySet());
         CommandSender sender = mock(CommandSender.class);
 
         // when
-        purgeService.runPurge(sender, delay);
+        purgeService.runPurge(sender, delay, includeLastLoginZeroEntries);
 
         // then
-        verify(dataSource).getRecordsToPurge(delay);
+        verify(dataSource).getRecordsToPurge(delay, includeLastLoginZeroEntries);
         verify(dataSource, never()).purgeRecords(anyCollectionOf(String.class));
         verify(sender).sendMessage("No players to purge");
         verifyZeroInteractions(bukkitService, permissionsManager);
@@ -135,19 +135,20 @@ public class PurgeServiceTest {
     @Test
     public void shouldRunPurge() {
         // given
-        long delay = 1809714L;
+        final long delay = 1809714L;
+        final boolean includeLastLoginZeroEntries = false;
         Set<String> playerNames = newHashSet("charlie", "delta", "echo", "foxtrot");
-        given(dataSource.getRecordsToPurge(delay)).willReturn(playerNames);
+        given(dataSource.getRecordsToPurge(delay, includeLastLoginZeroEntries)).willReturn(playerNames);
         mockReturnedOfflinePlayers();
         Player sender = mock(Player.class);
         UUID uuid = UUID.randomUUID();
         given(sender.getUniqueId()).willReturn(uuid);
 
         // when
-        purgeService.runPurge(sender, delay);
+        purgeService.runPurge(sender, delay, includeLastLoginZeroEntries);
 
         // then
-        verify(dataSource).getRecordsToPurge(delay);
+        verify(dataSource).getRecordsToPurge(delay, includeLastLoginZeroEntries);
         verifyScheduledPurgeTask(uuid, playerNames);
     }
 
@@ -214,7 +215,7 @@ public class PurgeServiceTest {
 
         Object senderInTask = ReflectionTestUtils.getFieldValue(PurgeTask.class, task, "sender");
         Set<String> namesInTask = ReflectionTestUtils.getFieldValue(PurgeTask.class, task, "toPurge");
-        assertThat(senderInTask, Matchers.<Object>equalTo(senderUuid));
+        assertThat(senderInTask, equalTo(senderUuid));
         assertThat(namesInTask, containsInAnyOrder(names.toArray()));
     }
 }