From ff99b63385a55cb4ef706517051281fdd2165b6a Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sun, 2 Jul 2017 10:55:16 +0200 Subject: [PATCH] #1265 Limbo: fallback to old "group" during deserialization, favor old limbo's groups over new limbo's --- .../xephi/authme/data/limbo/LimboPlayer.java | 6 ++--- .../authme/data/limbo/LimboServiceHelper.java | 12 ++++++++-- .../persistence/LimboPlayerDeserializer.java | 12 ++++++---- .../persistence/LimboPlayerSerializer.java | 6 ++--- .../data/limbo/LimboPlayerMatchers.java | 24 ++++++++++++++----- .../data/limbo/LimboServiceHelperTest.java | 5 ++-- ...istributedFilesPersistenceHandlerTest.java | 8 +++---- ...IndividualFilesPersistenceHandlerTest.java | 3 ++- .../authme/data/limbo/seg16-8-limbo.json | 5 +++- 9 files changed, 55 insertions(+), 26 deletions(-) diff --git a/src/main/java/fr/xephi/authme/data/limbo/LimboPlayer.java b/src/main/java/fr/xephi/authme/data/limbo/LimboPlayer.java index 8d05c3a0..b7ea415c 100644 --- a/src/main/java/fr/xephi/authme/data/limbo/LimboPlayer.java +++ b/src/main/java/fr/xephi/authme/data/limbo/LimboPlayer.java @@ -5,7 +5,6 @@ import org.bukkit.Location; import org.bukkit.scheduler.BukkitTask; import java.util.Collection; -import java.util.List; /** * Represents a player which is not logged in and keeps track of certain states (like OP status, flying) @@ -18,14 +17,15 @@ public class LimboPlayer { private final boolean canFly; private final boolean operator; - private Collection groups; + private final Collection groups; private final Location loc; private final float walkSpeed; private final float flySpeed; private BukkitTask timeoutTask = null; private MessageTask messageTask = null; - public LimboPlayer(Location loc, boolean operator, Collection groups, boolean fly, float walkSpeed, float flySpeed) { + public LimboPlayer(Location loc, boolean operator, Collection groups, boolean fly, float walkSpeed, + float flySpeed) { this.loc = loc; this.operator = operator; this.groups = groups; diff --git a/src/main/java/fr/xephi/authme/data/limbo/LimboServiceHelper.java b/src/main/java/fr/xephi/authme/data/limbo/LimboServiceHelper.java index a6f57de0..4e8248e3 100644 --- a/src/main/java/fr/xephi/authme/data/limbo/LimboServiceHelper.java +++ b/src/main/java/fr/xephi/authme/data/limbo/LimboServiceHelper.java @@ -12,6 +12,8 @@ import javax.inject.Inject; import java.util.Collection; import java.util.Collections; +import static fr.xephi.authme.util.Utils.isCollectionEmpty; + /** * Helper class for the LimboService. */ @@ -70,7 +72,7 @@ class LimboServiceHelper { *
    *
  • isOperator, allowFlight: true if either limbo has true
    • *
  • flySpeed, walkSpeed: maximum value of either limbo player
    • - *
  • group, location: from old limbo if not empty/null, otherwise from new limbo
    • + *
  • groups, location: from old limbo if not empty/null, otherwise from new limbo
    • *
* * @param newLimbo the new limbo player @@ -88,7 +90,7 @@ class LimboServiceHelper { boolean canFly = newLimbo.isCanFly() || oldLimbo.isCanFly(); float flySpeed = Math.max(newLimbo.getFlySpeed(), oldLimbo.getFlySpeed()); float walkSpeed = Math.max(newLimbo.getWalkSpeed(), oldLimbo.getWalkSpeed()); - Collection groups = newLimbo.getGroups(); + Collection groups = getLimboGroups(oldLimbo.getGroups(), newLimbo.getGroups()); Location location = firstNotNull(oldLimbo.getLocation(), newLimbo.getLocation()); return new LimboPlayer(location, isOperator, groups, canFly, walkSpeed, flySpeed); @@ -97,4 +99,10 @@ class LimboServiceHelper { private static Location firstNotNull(Location first, Location second) { return first == null ? second : first; } + + private static Collection getLimboGroups(Collection oldLimboGroups, + Collection newLimboGroups) { + ConsoleLogger.debug("Limbo merge: new and old groups are `{0}` and `{1}`", newLimboGroups, oldLimboGroups); + return isCollectionEmpty(oldLimboGroups) ? newLimboGroups : oldLimboGroups; + } } diff --git a/src/main/java/fr/xephi/authme/data/limbo/persistence/LimboPlayerDeserializer.java b/src/main/java/fr/xephi/authme/data/limbo/persistence/LimboPlayerDeserializer.java index a6c66192..c5688541 100644 --- a/src/main/java/fr/xephi/authme/data/limbo/persistence/LimboPlayerDeserializer.java +++ b/src/main/java/fr/xephi/authme/data/limbo/persistence/LimboPlayerDeserializer.java @@ -29,12 +29,15 @@ import static fr.xephi.authme.data.limbo.persistence.LimboPlayerSerializer.LOC_Y import static fr.xephi.authme.data.limbo.persistence.LimboPlayerSerializer.LOC_YAW; import static fr.xephi.authme.data.limbo.persistence.LimboPlayerSerializer.LOC_Z; import static fr.xephi.authme.data.limbo.persistence.LimboPlayerSerializer.WALK_SPEED; +import static java.util.Optional.ofNullable; /** * Converts a JsonElement to a LimboPlayer. */ class LimboPlayerDeserializer implements JsonDeserializer { + private static final String GROUP_LEGACY = "group"; + private BukkitService bukkitService; LimboPlayerDeserializer(BukkitService bukkitService) { @@ -51,7 +54,7 @@ class LimboPlayerDeserializer implements JsonDeserializer { Location loc = deserializeLocation(jsonObject); boolean operator = getBoolean(jsonObject, IS_OP); - Collection groups = getStringList(jsonObject, GROUPS); + Collection groups = getLimboGroups(jsonObject); boolean canFly = getBoolean(jsonObject, CAN_FLY); float walkSpeed = getFloat(jsonObject, WALK_SPEED, LimboPlayer.DEFAULT_WALK_SPEED); float flySpeed = getFloat(jsonObject, FLY_SPEED, LimboPlayer.DEFAULT_FLY_SPEED); @@ -81,10 +84,11 @@ class LimboPlayerDeserializer implements JsonDeserializer { return element != null ? element.getAsString() : ""; } - private static List getStringList(JsonObject jsonObject, String memberName) { - JsonElement element = jsonObject.get(memberName); + private static List getLimboGroups(JsonObject jsonObject) { + JsonElement element = jsonObject.get(GROUPS); if (element == null) { - return Collections.emptyList(); + String legacyGroup = ofNullable(jsonObject.get(GROUP_LEGACY)).map(JsonElement::getAsString).orElse(null); + return legacyGroup == null ? Collections.emptyList() : Collections.singletonList(legacyGroup); } List result = new ArrayList<>(); JsonArray jsonArray = element.getAsJsonArray(); diff --git a/src/main/java/fr/xephi/authme/data/limbo/persistence/LimboPlayerSerializer.java b/src/main/java/fr/xephi/authme/data/limbo/persistence/LimboPlayerSerializer.java index 65cdb3eb..5f9b40ed 100644 --- a/src/main/java/fr/xephi/authme/data/limbo/persistence/LimboPlayerSerializer.java +++ b/src/main/java/fr/xephi/authme/data/limbo/persistence/LimboPlayerSerializer.java @@ -1,7 +1,6 @@ package fr.xephi.authme.data.limbo.persistence; import com.google.gson.Gson; -import com.google.gson.JsonArray; import com.google.gson.JsonElement; import com.google.gson.JsonObject; import com.google.gson.JsonSerializationContext; @@ -10,7 +9,6 @@ import fr.xephi.authme.data.limbo.LimboPlayer; import org.bukkit.Location; import java.lang.reflect.Type; -import java.util.Collection; /** * Converts a LimboPlayer to a JsonElement. @@ -31,6 +29,8 @@ class LimboPlayerSerializer implements JsonSerializer { static final String WALK_SPEED = "walk-speed"; static final String FLY_SPEED = "fly-speed"; + private static final Gson GSON = new Gson(); + @Override public JsonElement serialize(LimboPlayer limboPlayer, Type type, JsonSerializationContext context) { @@ -45,7 +45,7 @@ class LimboPlayerSerializer implements JsonSerializer { JsonObject obj = new JsonObject(); obj.add(LOCATION, locationObject); - obj.add(GROUPS, new Gson().toJsonTree(limboPlayer.getGroups()).getAsJsonArray()); //TODO: maybe we should store the GSON instance somewhere. -sg + obj.add(GROUPS, GSON.toJsonTree(limboPlayer.getGroups()).getAsJsonArray()); obj.addProperty(IS_OP, limboPlayer.isOperator()); obj.addProperty(CAN_FLY, limboPlayer.isCanFly()); diff --git a/src/test/java/fr/xephi/authme/data/limbo/LimboPlayerMatchers.java b/src/test/java/fr/xephi/authme/data/limbo/LimboPlayerMatchers.java index f8901bf3..bb5a452e 100644 --- a/src/test/java/fr/xephi/authme/data/limbo/LimboPlayerMatchers.java +++ b/src/test/java/fr/xephi/authme/data/limbo/LimboPlayerMatchers.java @@ -9,6 +9,7 @@ import org.hamcrest.TypeSafeMatcher; import java.util.Collection; import static java.lang.String.format; +import static org.hamcrest.collection.IsIterableContainingInOrder.contains; /** * Contains matchers for LimboPlayer. @@ -19,17 +20,20 @@ public final class LimboPlayerMatchers { } public static Matcher isLimbo(LimboPlayer limbo) { - return isLimbo(limbo.isOperator(), limbo.getGroups(), limbo.isCanFly(), - limbo.getWalkSpeed(), limbo.getFlySpeed()); + String[] groups = limbo.getGroups().toArray(new String[limbo.getGroups().size()]); + return isLimbo(limbo.isOperator(), limbo.isCanFly(), limbo.getWalkSpeed(), limbo.getFlySpeed(), groups); } - public static Matcher isLimbo(boolean isOp, Collection groups, boolean canFly, - float walkSpeed, float flySpeed) { + public static Matcher isLimbo(boolean isOp, boolean canFly, float walkSpeed, float flySpeed, + String... groups) { return new TypeSafeMatcher() { @Override protected boolean matchesSafely(LimboPlayer item) { - return item.isOperator() == isOp && item.getGroups().equals(groups) && item.isCanFly() == canFly - && walkSpeed == item.getWalkSpeed() && flySpeed == item.getFlySpeed(); + return item.isOperator() == isOp + && collectionContains(item.getGroups(), groups) + && item.isCanFly() == canFly + && walkSpeed == item.getWalkSpeed() + && flySpeed == item.getFlySpeed(); } @Override @@ -111,4 +115,12 @@ public final class LimboPlayerMatchers { return hasLocation(location.getWorld().getName(), location.getX(), location.getY(), location.getZ(), location.getYaw(), location.getPitch()); } + + // Hamcrest's contains() doesn't like it when there are no items, so we need to check for the empty case explicitly + private static boolean collectionContains(Collection givenItems, String... expectedItems) { + if (expectedItems.length == 0) { + return givenItems.isEmpty(); + } + return contains(expectedItems).matches(givenItems); + } } diff --git a/src/test/java/fr/xephi/authme/data/limbo/LimboServiceHelperTest.java b/src/test/java/fr/xephi/authme/data/limbo/LimboServiceHelperTest.java index fac99d83..fe798fac 100644 --- a/src/test/java/fr/xephi/authme/data/limbo/LimboServiceHelperTest.java +++ b/src/test/java/fr/xephi/authme/data/limbo/LimboServiceHelperTest.java @@ -8,6 +8,7 @@ import org.mockito.junit.MockitoJUnitRunner; import java.util.Collections; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.nullValue; import static org.junit.Assert.assertThat; @@ -39,7 +40,7 @@ public class LimboServiceHelperTest { // then assertThat(result.getLocation(), equalTo(oldLocation)); assertThat(result.isOperator(), equalTo(true)); - assertThat(result.getGroups(), equalTo(Collections.singletonList("grp-new"))); + assertThat(result.getGroups(), contains("grp-old")); assertThat(result.isCanFly(), equalTo(true)); assertThat(result.getWalkSpeed(), equalTo(0.1f)); assertThat(result.getFlySpeed(), equalTo(0.8f)); @@ -58,7 +59,7 @@ public class LimboServiceHelperTest { // then assertThat(result.getLocation(), equalTo(newLocation)); assertThat(result.isOperator(), equalTo(false)); - assertThat(result.getGroups(), equalTo(Collections.singletonList("grp-new"))); + assertThat(result.getGroups(), contains("grp-new")); assertThat(result.isCanFly(), equalTo(true)); assertThat(result.getWalkSpeed(), equalTo(0.3f)); assertThat(result.getFlySpeed(), equalTo(0.1f)); diff --git a/src/test/java/fr/xephi/authme/data/limbo/persistence/DistributedFilesPersistenceHandlerTest.java b/src/test/java/fr/xephi/authme/data/limbo/persistence/DistributedFilesPersistenceHandlerTest.java index 7d33282e..8357840d 100644 --- a/src/test/java/fr/xephi/authme/data/limbo/persistence/DistributedFilesPersistenceHandlerTest.java +++ b/src/test/java/fr/xephi/authme/data/limbo/persistence/DistributedFilesPersistenceHandlerTest.java @@ -46,22 +46,22 @@ public class DistributedFilesPersistenceHandlerTest { /** Player is in seg32-10110 and should be migrated into seg16-f. */ private static final UUID MIGRATED_UUID = fromString("f6a97c88-7c8f-c12e-4931-6206d4ca067d"); private static final Matcher MIGRATED_LIMBO_MATCHER = - isLimbo(false, Collections.singletonList("noob"), true, 0.2f, 0.1f); + isLimbo(false, true, 0.2f, 0.1f, "noob"); /** Existing player in seg16-f. */ private static final UUID UUID_FAB69 = fromString("fab69c88-2cd0-1fed-f00d-dead14ca067d"); private static final Matcher FAB69_MATCHER = - isLimbo(false, Collections.emptyList(), false, 0.2f, 0.1f); + isLimbo(false, false, 0.2f, 0.1f, ""); /** Player in seg16-8. */ private static final UUID UUID_STAFF = fromString("88897c88-7c8f-c12e-4931-6206d4ca067d"); private static final Matcher STAFF_MATCHER = - isLimbo(true, Collections.singletonList("staff"), false, 0.3f, 0.1f); + isLimbo(true, false, 0.3f, 0.1f, "staff", "mod"); /** Player in seg16-8. */ private static final UUID UUID_8C679 = fromString("8c679491-1234-abcd-9102-1fa6e0cc3f81"); private static final Matcher SC679_MATCHER = - isLimbo(false, Collections.singletonList("primary"), true, 0.1f, 0.0f); + isLimbo(false, true, 0.1f, 0.0f, "primary"); /** UUID for which no data is stored (belongs to a segment file that does not exist, seg16-4). */ private static final UUID UNKNOWN_UUID = fromString("42d1cc0b-8f12-d04a-e7ba-a067d05cdc39"); diff --git a/src/test/java/fr/xephi/authme/data/limbo/persistence/IndividualFilesPersistenceHandlerTest.java b/src/test/java/fr/xephi/authme/data/limbo/persistence/IndividualFilesPersistenceHandlerTest.java index 55bc7184..06df57bf 100644 --- a/src/test/java/fr/xephi/authme/data/limbo/persistence/IndividualFilesPersistenceHandlerTest.java +++ b/src/test/java/fr/xephi/authme/data/limbo/persistence/IndividualFilesPersistenceHandlerTest.java @@ -23,6 +23,7 @@ import java.nio.file.Files; import java.util.Collections; import java.util.UUID; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.nullValue; @@ -79,7 +80,7 @@ public class IndividualFilesPersistenceHandlerTest { assertThat(data.isCanFly(), equalTo(true)); assertThat(data.getWalkSpeed(), equalTo(0.2f)); assertThat(data.getFlySpeed(), equalTo(0.1f)); - assertThat(data.getGroups(), equalTo(Collections.singletonList("players"))); + assertThat(data.getGroups(), contains("players")); Location location = data.getLocation(); assertThat(location.getX(), equalTo(-113.219)); assertThat(location.getY(), equalTo(72.0)); diff --git a/src/test/resources/fr/xephi/authme/data/limbo/seg16-8-limbo.json b/src/test/resources/fr/xephi/authme/data/limbo/seg16-8-limbo.json index 84da8a26..f22e472a 100644 --- a/src/test/resources/fr/xephi/authme/data/limbo/seg16-8-limbo.json +++ b/src/test/resources/fr/xephi/authme/data/limbo/seg16-8-limbo.json @@ -8,7 +8,10 @@ "yaw": 222.14977, "pitch": 10.649977 }, - "group": "staff", + "groups": [ + "staff", + "mod" + ], "operator": true, "can-fly": false, "walk-speed": 0.3,