From ff9f50f63f1a80f9f555234d5ee18689cf8a8bff Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sun, 18 Sep 2016 16:49:34 +0200 Subject: [PATCH] #886 Do not include players with lastlogin = 0 in autopurge or default purge --- .../authme/command/CommandInitializer.java | 1 + .../executable/authme/PurgeCommand.java | 16 +++++++- .../authme/datasource/CacheDataSource.java | 4 +- .../xephi/authme/datasource/DataSource.java | 4 +- .../fr/xephi/authme/datasource/FlatFile.java | 27 +------------ .../fr/xephi/authme/datasource/MySQL.java | 7 +++- .../fr/xephi/authme/datasource/SQLite.java | 7 +++- .../xephi/authme/task/purge/PurgeService.java | 11 ++++-- .../executable/authme/PurgeCommandTest.java | 38 ++++++++++++++++++- .../AbstractDataSourceIntegrationTest.java | 10 +++-- .../AbstractResourceClosingTest.java | 1 + .../authme/task/purge/PurgeServiceTest.java | 25 ++++++------ 12 files changed, 97 insertions(+), 54 deletions(-) diff --git a/src/main/java/fr/xephi/authme/command/CommandInitializer.java b/src/main/java/fr/xephi/authme/command/CommandInitializer.java index b29fd2150..edbbdb201 100644 --- a/src/main/java/fr/xephi/authme/command/CommandInitializer.java +++ b/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(); diff --git a/src/main/java/fr/xephi/authme/command/executable/authme/PurgeCommand.java b/src/main/java/fr/xephi/authme/command/executable/authme/PurgeCommand.java index 21552d81f..4a3e86cfc 100644 --- a/src/main/java/fr/xephi/authme/command/executable/authme/PurgeCommand.java +++ b/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); } } diff --git a/src/main/java/fr/xephi/authme/datasource/CacheDataSource.java b/src/main/java/fr/xephi/authme/datasource/CacheDataSource.java index bab51daf1..7032733d0 100644 --- a/src/main/java/fr/xephi/authme/datasource/CacheDataSource.java +++ b/src/main/java/fr/xephi/authme/datasource/CacheDataSource.java @@ -138,8 +138,8 @@ public class CacheDataSource implements DataSource { } @Override - public Set getRecordsToPurge(long until) { - return source.getRecordsToPurge(until); + public Set getRecordsToPurge(long until, boolean includeEntriesWithLastLoginZero) { + return source.getRecordsToPurge(until, includeEntriesWithLastLoginZero); } @Override diff --git a/src/main/java/fr/xephi/authme/datasource/DataSource.java b/src/main/java/fr/xephi/authme/datasource/DataSource.java index 5b25fde44..98e29adab 100644 --- a/src/main/java/fr/xephi/authme/datasource/DataSource.java +++ b/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 issue #886 * @return The account names selected to purge */ - Set getRecordsToPurge(long until); + Set getRecordsToPurge(long until, boolean includeEntriesWithLastLoginZero); /** * Purge the given players from the database. diff --git a/src/main/java/fr/xephi/authme/datasource/FlatFile.java b/src/main/java/fr/xephi/authme/datasource/FlatFile.java index f3f180146..779d36407 100644 --- a/src/main/java/fr/xephi/authme/datasource/FlatFile.java +++ b/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 getRecordsToPurge(long until) { - BufferedReader br = null; - Set 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 getRecordsToPurge(long until, boolean includeEntriesWithLastLoginZero) { + throw new UnsupportedOperationException("Flat file no longer supported"); } @Override diff --git a/src/main/java/fr/xephi/authme/datasource/MySQL.java b/src/main/java/fr/xephi/authme/datasource/MySQL.java index da0ad602c..05f22d0c9 100644 --- a/src/main/java/fr/xephi/authme/datasource/MySQL.java +++ b/src/main/java/fr/xephi/authme/datasource/MySQL.java @@ -572,10 +572,13 @@ public class MySQL implements DataSource { } @Override - public Set getRecordsToPurge(long until) { + public Set getRecordsToPurge(long until, boolean includeEntriesWithLastLoginZero) { Set list = new HashSet<>(); - String select = "SELECT " + col.NAME + " FROM " + tableName + " WHERE " + col.LAST_LOGIN + " 0"; + } try (Connection con = getConnection(); PreparedStatement selectPst = con.prepareStatement(select)) { selectPst.setLong(1, until); diff --git a/src/main/java/fr/xephi/authme/datasource/SQLite.java b/src/main/java/fr/xephi/authme/datasource/SQLite.java index d9c847f43..1e554671d 100644 --- a/src/main/java/fr/xephi/authme/datasource/SQLite.java +++ b/src/main/java/fr/xephi/authme/datasource/SQLite.java @@ -299,10 +299,13 @@ public class SQLite implements DataSource { } @Override - public Set getRecordsToPurge(long until) { + public Set getRecordsToPurge(long until, boolean includeEntriesWithLastLoginZero) { Set list = new HashSet<>(); - String select = "SELECT " + col.NAME + " FROM " + tableName + " WHERE " + col.LAST_LOGIN + " 0"; + } try (PreparedStatement selectPst = con.prepareStatement(select)) { selectPst.setLong(1, until); try (ResultSet rs = selectPst.executeQuery()) { diff --git a/src/main/java/fr/xephi/authme/task/purge/PurgeService.java b/src/main/java/fr/xephi/authme/task/purge/PurgeService.java index f8c33e4c7..ee4370f88 100644 --- a/src/main/java/fr/xephi/authme/task/purge/PurgeService.java +++ b/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 toPurge = dataSource.getRecordsToPurge(until); + Set toPurge = dataSource.getRecordsToPurge(until, includeEntriesWithLastLoginZero); if (CollectionUtils.isEmpty(toPurge)) { logAndSendMessage(sender, "No players to purge"); return; diff --git a/src/test/java/fr/xephi/authme/command/executable/authme/PurgeCommandTest.java b/src/test/java/fr/xephi/authme/command/executable/authme/PurgeCommandTest.java index 68a34db76..63e1008ad 100644 --- a/src/test/java/fr/xephi/authme/command/executable/authme/PurgeCommandTest.java +++ b/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 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 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))); } diff --git a/src/test/java/fr/xephi/authme/datasource/AbstractDataSourceIntegrationTest.java b/src/test/java/fr/xephi/authme/datasource/AbstractDataSourceIntegrationTest.java index 424a23206..6399221c2 100644 --- a/src/test/java/fr/xephi/authme/datasource/AbstractDataSourceIntegrationTest.java +++ b/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 records1 = dataSource.getRecordsToPurge(1450000000); - Set records2 = dataSource.getRecordsToPurge(1460000000); + Set records1 = dataSource.getRecordsToPurge(1450000000, true); + Set 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)); diff --git a/src/test/java/fr/xephi/authme/datasource/AbstractResourceClosingTest.java b/src/test/java/fr/xephi/authme/datasource/AbstractResourceClosingTest.java index a5ab1d500..ee6bdd048 100644 --- a/src/test/java/fr/xephi/authme/datasource/AbstractResourceClosingTest.java +++ b/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(); diff --git a/src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java b/src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java index 842861d17..3f626f4d1 100644 --- a/src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java +++ b/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 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 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.emptySet()); + final long delay = 123012301L; + final boolean includeLastLoginZeroEntries = true; + given(dataSource.getRecordsToPurge(delay, includeLastLoginZeroEntries)).willReturn(Collections.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 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 namesInTask = ReflectionTestUtils.getFieldValue(PurgeTask.class, task, "toPurge"); - assertThat(senderInTask, Matchers.equalTo(senderUuid)); + assertThat(senderInTask, equalTo(senderUuid)); assertThat(namesInTask, containsInAnyOrder(names.toArray())); } }