From 1678901e02f00414a6b480e6ecde9baeda476c1e Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sun, 12 Mar 2017 15:56:08 +0100 Subject: [PATCH] #1113 Attempt to merge new LimboPlayer with an existing one - Extract some logic into LimboServiceHelper to keep LimboService slim - Create LimboServiceHelper#merge to merge two LimboPlayers associated with a Player. E.g. if an admin unregisters an online player that has not logged in, the creation of a LimboPlayer is triggered while there already is one in LimboService --- .../xephi/authme/data/limbo/LimboService.java | 142 ++++++------------ .../authme/data/limbo/LimboServiceHelper.java | 103 +++++++++++++ .../data/limbo/LimboServiceHelperTest.java | 82 ++++++++++ .../authme/data/limbo/LimboServiceTest.java | 13 +- 4 files changed, 240 insertions(+), 100 deletions(-) create mode 100644 src/main/java/fr/xephi/authme/data/limbo/LimboServiceHelper.java create mode 100644 src/test/java/fr/xephi/authme/data/limbo/LimboServiceHelperTest.java diff --git a/src/main/java/fr/xephi/authme/data/limbo/LimboService.java b/src/main/java/fr/xephi/authme/data/limbo/LimboService.java index b0c93d7d..290b7947 100644 --- a/src/main/java/fr/xephi/authme/data/limbo/LimboService.java +++ b/src/main/java/fr/xephi/authme/data/limbo/LimboService.java @@ -1,11 +1,7 @@ package fr.xephi.authme.data.limbo; import fr.xephi.authme.ConsoleLogger; -import fr.xephi.authme.permission.PermissionsManager; import fr.xephi.authme.settings.Settings; -import fr.xephi.authme.settings.SpawnLoader; -import fr.xephi.authme.settings.properties.RestrictionSettings; -import org.bukkit.Location; import org.bukkit.entity.Player; import javax.inject.Inject; @@ -25,21 +21,61 @@ public class LimboService { private final Map entries = new ConcurrentHashMap<>(); - @Inject - private SpawnLoader spawnLoader; - - @Inject - private PermissionsManager permissionsManager; - @Inject private Settings settings; @Inject private LimboPlayerTaskManager taskManager; + @Inject + private LimboServiceHelper limboServiceHelper; + LimboService() { } + /** + * Creates a LimboPlayer for the given player and revokes all "limbo data" from the player. + * + * @param player the player to process + * @param isRegistered whether or not the player is registered + */ + public void createLimboPlayer(Player player, boolean isRegistered) { + final String name = player.getName().toLowerCase(); + + LimboPlayer existingLimbo = entries.remove(name); + if (existingLimbo != null) { + existingLimbo.clearTasks(); + ConsoleLogger.debug("LimboPlayer for `{0}` was already present", name); + } + + LimboPlayer limboPlayer = limboServiceHelper.merge( + limboServiceHelper.createLimboPlayer(player, isRegistered), existingLimbo); + + taskManager.registerMessageTask(player, limboPlayer, isRegistered); + taskManager.registerTimeoutTask(player, limboPlayer); + limboServiceHelper.revokeLimboStates(player); + entries.put(name, limboPlayer); + } + + /** + * Returns the limbo player for the given name, or null otherwise. + * + * @param name the name to retrieve the data for + * @return the associated limbo player, or null if none available + */ + public LimboPlayer getLimboPlayer(String name) { + return entries.get(name.toLowerCase()); + } + + /** + * Returns whether there is a limbo player for the given name. + * + * @param name the name to check + * @return true if present, false otherwise + */ + public boolean hasLimboPlayer(String name) { + return entries.containsKey(name.toLowerCase()); + } /** * Restores the limbo data and subsequently deletes the entry. @@ -65,51 +101,9 @@ public class LimboService { } } - /** - * Returns the limbo player for the given name, or null otherwise. - * - * @param name the name to retrieve the data for - * @return the associated limbo player, or null if none available - */ - public LimboPlayer getLimboPlayer(String name) { - return entries.get(name.toLowerCase()); - } - - /** - * Returns whether there is a limbo player for the given name. - * - * @param name the name to check - * @return true if present, false otherwise - */ - public boolean hasLimboPlayer(String name) { - return entries.containsKey(name.toLowerCase()); - } - - /** - * Creates a LimboPlayer for the given player and revokes all "limbo data" from the player. - * - * @param player the player to process - * @param isRegistered whether or not the player is registered - */ - public void createLimboPlayer(Player player, boolean isRegistered) { - final String name = player.getName().toLowerCase(); - - LimboPlayer existingLimbo = entries.remove(name); - if (existingLimbo != null) { - existingLimbo.clearTasks(); - ConsoleLogger.debug("LimboPlayer for `{0}` was already present", name); - } - - LimboPlayer limboPlayer = newLimboPlayer(player, isRegistered); - taskManager.registerMessageTask(player, limboPlayer, isRegistered); - taskManager.registerTimeoutTask(player, limboPlayer); - revokeLimboStates(player); - entries.put(name, limboPlayer); - } - /** * Creates new tasks for the given player and cancels the old ones for a newly registered player. - * This resets his time to log in (TimeoutTask) and updates the messages he is shown (MessageTask). + * This resets his time to log in (TimeoutTask) and updates the message he is shown (MessageTask). * * @param player the player to reset the tasks for */ @@ -148,48 +142,6 @@ public class LimboService { .ifPresent(limbo -> LimboPlayerTaskManager.setMuted(limbo.getMessageTask(), false)); } - /** - * Creates a LimboPlayer with the given player's details. - * - * @param player the player to process - * @param isRegistered whether the player is registered - * @return limbo player with the player's data - */ - private LimboPlayer newLimboPlayer(Player player, boolean isRegistered) { - Location location = spawnLoader.getPlayerLocationOrSpawn(player); - // For safety reasons an unregistered player should not have OP status after registration - boolean isOperator = isRegistered && player.isOp(); - boolean flyEnabled = player.getAllowFlight(); - float walkSpeed = player.getWalkSpeed(); - float flySpeed = player.getFlySpeed(); - String playerGroup = ""; - if (permissionsManager.hasGroupSupport()) { - playerGroup = permissionsManager.getPrimaryGroup(player); - } - ConsoleLogger.debug("Player `{0}` has primary group `{1}`", player.getName(), playerGroup); - - return new LimboPlayer(location, isOperator, playerGroup, flyEnabled, walkSpeed, flySpeed); - } - - /** - * Removes the data that is saved in a LimboPlayer from the player. - *

- * Note that teleportation on the player is performed by {@link fr.xephi.authme.service.TeleportationService} and - * changing the permission group is handled by {@link fr.xephi.authme.permission.AuthGroupHandler}. - * - * @param player the player to set defaults to - */ - private void revokeLimboStates(Player player) { - player.setOp(false); - player.setAllowFlight(false); - - if (!settings.getProperty(RestrictionSettings.ALLOW_UNAUTHED_MOVEMENT) - && settings.getProperty(RestrictionSettings.REMOVE_SPEED)) { - player.setFlySpeed(0.0f); - player.setWalkSpeed(0.0f); - } - } - /** * Returns the limbo player for the given player or logs an error. * diff --git a/src/main/java/fr/xephi/authme/data/limbo/LimboServiceHelper.java b/src/main/java/fr/xephi/authme/data/limbo/LimboServiceHelper.java new file mode 100644 index 00000000..fc693346 --- /dev/null +++ b/src/main/java/fr/xephi/authme/data/limbo/LimboServiceHelper.java @@ -0,0 +1,103 @@ +package fr.xephi.authme.data.limbo; + +import fr.xephi.authme.ConsoleLogger; +import fr.xephi.authme.permission.PermissionsManager; +import fr.xephi.authme.settings.Settings; +import fr.xephi.authme.settings.SpawnLoader; +import fr.xephi.authme.settings.properties.RestrictionSettings; +import org.bukkit.Location; +import org.bukkit.entity.Player; + +import javax.inject.Inject; + +/** + * Helper class for the LimboService. + */ +class LimboServiceHelper { + + @Inject + private SpawnLoader spawnLoader; + + @Inject + private PermissionsManager permissionsManager; + + @Inject + private Settings settings; + + /** + * Creates a LimboPlayer with the given player's details. + * + * @param player the player to process + * @param isRegistered whether the player is registered + * @return limbo player with the player's data + */ + LimboPlayer createLimboPlayer(Player player, boolean isRegistered) { + Location location = spawnLoader.getPlayerLocationOrSpawn(player); + // For safety reasons an unregistered player should not have OP status after registration + boolean isOperator = isRegistered && player.isOp(); + boolean flyEnabled = player.getAllowFlight(); + float walkSpeed = player.getWalkSpeed(); + float flySpeed = player.getFlySpeed(); + String playerGroup = permissionsManager.hasGroupSupport() + ? permissionsManager.getPrimaryGroup(player) : ""; + ConsoleLogger.debug("Player `{0}` has primary group `{1}`", player.getName(), playerGroup); + + return new LimboPlayer(location, isOperator, playerGroup, flyEnabled, walkSpeed, flySpeed); + } + + /** + * Removes the data that is saved in a LimboPlayer from the player. + *

+ * Note that teleportation on the player is performed by {@link fr.xephi.authme.service.TeleportationService} and + * changing the permission group is handled by {@link fr.xephi.authme.permission.AuthGroupHandler}. + * + * @param player the player to set defaults to + */ + void revokeLimboStates(Player player) { + player.setOp(false); + player.setAllowFlight(false); + + if (!settings.getProperty(RestrictionSettings.ALLOW_UNAUTHED_MOVEMENT) + && settings.getProperty(RestrictionSettings.REMOVE_SPEED)) { + player.setFlySpeed(0.0f); + player.setWalkSpeed(0.0f); + } + } + + /** + * Merges two existing LimboPlayer instances of a player. Merging is done the following way: + *

+ * + * @param newLimbo the new limbo player + * @param oldLimbo the old limbo player + * @return merged limbo player if both arguments are not null, otherwise the first non-null argument + */ + LimboPlayer merge(LimboPlayer newLimbo, LimboPlayer oldLimbo) { + if (newLimbo == null) { + return oldLimbo; + } else if (oldLimbo == null) { + return newLimbo; + } + + boolean isOperator = newLimbo.isOperator() || oldLimbo.isOperator(); + boolean canFly = newLimbo.isCanFly() || oldLimbo.isCanFly(); + float flySpeed = Math.max(newLimbo.getFlySpeed(), oldLimbo.getFlySpeed()); + float walkSpeed = Math.max(newLimbo.getWalkSpeed(), oldLimbo.getWalkSpeed()); + String group = firstNotEmpty(newLimbo.getGroup(), oldLimbo.getGroup()); + + return new LimboPlayer(oldLimbo.getLocation(), isOperator, group, canFly, walkSpeed, flySpeed); + } + + private static String firstNotEmpty(String newGroup, String oldGroup) { + ConsoleLogger.debug("Limbo merge: new and old perm groups are `{0}` and `{1}`", newGroup, oldGroup); + if ("".equals(oldGroup)) { + return newGroup; + } + return oldGroup; + } +} diff --git a/src/test/java/fr/xephi/authme/data/limbo/LimboServiceHelperTest.java b/src/test/java/fr/xephi/authme/data/limbo/LimboServiceHelperTest.java new file mode 100644 index 00000000..77a21d69 --- /dev/null +++ b/src/test/java/fr/xephi/authme/data/limbo/LimboServiceHelperTest.java @@ -0,0 +1,82 @@ +package fr.xephi.authme.data.limbo; + +import org.bukkit.Location; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.InjectMocks; +import org.mockito.junit.MockitoJUnitRunner; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; +import static org.junit.Assert.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verifyZeroInteractions; + +/** + * Test for {@link LimboServiceHelper}. + *

+ * Note: some methods are tested directly where they are used via {@link LimboServiceTest}. + */ +@RunWith(MockitoJUnitRunner.class) +public class LimboServiceHelperTest { + + @InjectMocks + private LimboServiceHelper limboServiceHelper; + + @Test + public void shouldMergeLimboPlayers() { + // given + Location newLocation = mock(Location.class); + LimboPlayer newLimbo = new LimboPlayer(newLocation, false, "grp-new", false, 0.0f, 0.0f); + Location oldLocation = mock(Location.class); + LimboPlayer oldLimbo = new LimboPlayer(oldLocation, true, "grp-old", true, 0.1f, 0.8f); + + // when + LimboPlayer result = limboServiceHelper.merge(newLimbo, oldLimbo); + + // then + assertThat(result.getLocation(), equalTo(oldLocation)); + assertThat(result.isOperator(), equalTo(true)); + assertThat(result.getGroup(), equalTo("grp-old")); + assertThat(result.isCanFly(), equalTo(true)); + assertThat(result.getWalkSpeed(), equalTo(0.1f)); + assertThat(result.getFlySpeed(), equalTo(0.8f)); + } + + @Test + public void shouldFallBackToNewLimboForMissingData() { + // given + Location newLocation = mock(Location.class); + LimboPlayer newLimbo = new LimboPlayer(newLocation, false, "grp-new", true, 0.3f, 0.0f); + Location oldLocation = mock(Location.class); + LimboPlayer oldLimbo = new LimboPlayer(oldLocation, false, "", false, 0.1f, 0.1f); + + // when + LimboPlayer result = limboServiceHelper.merge(newLimbo, oldLimbo); + + // then + assertThat(result.getLocation(), equalTo(oldLocation)); + assertThat(result.isOperator(), equalTo(false)); + assertThat(result.getGroup(), equalTo("grp-new")); + assertThat(result.isCanFly(), equalTo(true)); + assertThat(result.getWalkSpeed(), equalTo(0.3f)); + assertThat(result.getFlySpeed(), equalTo(0.1f)); + } + + @Test + public void shouldHandleNullInputs() { + // given + LimboPlayer limbo = mock(LimboPlayer.class); + + // when + LimboPlayer result1 = limboServiceHelper.merge(limbo, null); + LimboPlayer result2 = limboServiceHelper.merge(null, limbo); + LimboPlayer result3 = limboServiceHelper.merge(null, null); + + // then + verifyZeroInteractions(limbo); + assertThat(result1, equalTo(limbo)); + assertThat(result2, equalTo(limbo)); + assertThat(result3, nullValue()); + } +} diff --git a/src/test/java/fr/xephi/authme/data/limbo/LimboServiceTest.java b/src/test/java/fr/xephi/authme/data/limbo/LimboServiceTest.java index 4abe481c..a2e791fa 100644 --- a/src/test/java/fr/xephi/authme/data/limbo/LimboServiceTest.java +++ b/src/test/java/fr/xephi/authme/data/limbo/LimboServiceTest.java @@ -1,5 +1,7 @@ package fr.xephi.authme.data.limbo; +import ch.jalu.injector.testing.DelayedInjectionRunner; +import ch.jalu.injector.testing.InjectDelayed; import fr.xephi.authme.ReflectionTestUtils; import fr.xephi.authme.TestHelper; import fr.xephi.authme.permission.PermissionsManager; @@ -13,10 +15,8 @@ import org.junit.Before; import org.junit.BeforeClass; import org.junit.Test; import org.junit.runner.RunWith; -import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.Mockito; -import org.mockito.junit.MockitoJUnitRunner; import java.util.Map; @@ -34,14 +34,17 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyZeroInteractions; /** - * Test for {@link LimboService}. + * Test for {@link LimboService}, and {@link LimboServiceHelper}. */ -@RunWith(MockitoJUnitRunner.class) +@RunWith(DelayedInjectionRunner.class) public class LimboServiceTest { - @InjectMocks + @InjectDelayed private LimboService limboService; + @InjectDelayed + private LimboServiceHelper limboServiceHelper; + @Mock private SpawnLoader spawnLoader;