#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

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<String, LimboPlayer> 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.
-     * <p>
-     * 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.
      *

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.
+     * <p>
+     * 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:
+     * <ul>
+     *  <li><code>isOperator, allowFlight</code>: true if either limbo has true</li>
+     *  <li><code>flySpeed, walkSpeed</code>: maximum value of either limbo player</li>
+     *  <li><code>group</code>: from old limbo if not empty, otherwise from new limbo</li>
+     *  <li><code>location</code>: from old limbo</li>
+     * </ul>
+     *
+     * @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;
+    }
+}

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}.
+ * <p>
+ * 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());
+    }
+}

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;