diff --git a/changelog.md b/changelog.md index a72a955..619caef 100644 --- a/changelog.md +++ b/changelog.md @@ -15,6 +15,7 @@ These changes will (most likely) be included in the next version. - The new per-arena setting `join-interrupt-timer` makes it possible to add a "delay" to the join and spec commands. If the player moves or takes damage during this delay, the command is interrupted. This should help prevent exploits on PvP servers. - Right-clicking is now allowed in the lobby. This makes it possible to activate blocks like buttons and levers. - Snow and ice no longer melts in arenas. +- Much of the parsing logic has been rewritten so that MobArena now logs more user-friendly errors when it encounters invalid values in the config-file. Thanks to: - PrinceIonia and Nesseley for help with test of dev builds diff --git a/src/main/java/com/garbagemule/MobArena/ArenaImpl.java b/src/main/java/com/garbagemule/MobArena/ArenaImpl.java index b3c1234..a320b97 100644 --- a/src/main/java/com/garbagemule/MobArena/ArenaImpl.java +++ b/src/main/java/com/garbagemule/MobArena/ArenaImpl.java @@ -20,6 +20,7 @@ import com.garbagemule.MobArena.region.ArenaRegion; import com.garbagemule.MobArena.repairable.Repairable; import com.garbagemule.MobArena.repairable.RepairableComparator; import com.garbagemule.MobArena.repairable.RepairableContainer; +import com.garbagemule.MobArena.things.InvalidThingInputString; import com.garbagemule.MobArena.things.Thing; import com.garbagemule.MobArena.util.ClassChests; import com.garbagemule.MobArena.util.inventory.InventoryManager; @@ -205,13 +206,9 @@ public class ArenaImpl implements Arena for (String fee : feeString.split(",")) { try { Thing thing = plugin.getThingManager().parse(fee.trim()); - if (thing == null) { - plugin.getLogger().warning("Failed to parse entry fee: " + fee.trim()); - } else { - this.entryFee.add(thing); - } - } catch (Exception e) { - plugin.getLogger().severe("Exception parsing entry fee '" + fee.trim() + "': " + e.getLocalizedMessage()); + this.entryFee.add(thing); + } catch (InvalidThingInputString e) { + throw new ConfigError("Failed to parse entry fee of arena " + name + ": " + e.getInput()); } } } diff --git a/src/main/java/com/garbagemule/MobArena/ArenaMasterImpl.java b/src/main/java/com/garbagemule/MobArena/ArenaMasterImpl.java index 677d41a..6f1a75c 100644 --- a/src/main/java/com/garbagemule/MobArena/ArenaMasterImpl.java +++ b/src/main/java/com/garbagemule/MobArena/ArenaMasterImpl.java @@ -5,6 +5,7 @@ import static com.garbagemule.MobArena.util.config.ConfigUtils.parseLocation; import com.garbagemule.MobArena.framework.Arena; import com.garbagemule.MobArena.framework.ArenaMaster; +import com.garbagemule.MobArena.things.InvalidThingInputString; import com.garbagemule.MobArena.things.Thing; import com.garbagemule.MobArena.util.JoinInterruptTimer; import com.garbagemule.MobArena.util.config.ConfigUtils; @@ -340,9 +341,8 @@ public class ArenaMasterImpl implements ArenaMaster if (priceString != null) { try { price = plugin.getThingManager().parse(priceString); - } catch (Exception e) { - plugin.getLogger().warning("Exception parsing class price: " + e.getLocalizedMessage()); - price = null; + } catch (InvalidThingInputString e) { + throw new ConfigError("Failed to parse price of class " + classname + ": " + e.getInput()); } } @@ -380,13 +380,15 @@ public class ArenaMasterImpl implements ArenaMaster items = Arrays.asList(value.split(",")); } - List things = items.stream() - .map(String::trim) - .map(plugin.getThingManager()::parse) - .filter(Objects::nonNull) - .collect(Collectors.toList()); - - arenaClass.setItems(things); + try { + List things = items.stream() + .map(String::trim) + .map(plugin.getThingManager()::parse) + .collect(Collectors.toList()); + arenaClass.setItems(things); + } catch (InvalidThingInputString e) { + throw new ConfigError("Failed to parse item for class " + arenaClass.getConfigName() + ": " + e.getInput()); + } } private void loadClassArmor(ConfigurationSection section, ArenaClass arenaClass) { @@ -394,76 +396,91 @@ public class ArenaMasterImpl implements ArenaMaster loadClassArmorLegacyNode(section, arenaClass); // Specific armor pieces - loadClassArmorPiece(section, "helmet", arenaClass::setHelmet); - loadClassArmorPiece(section, "chestplate", arenaClass::setChestplate); - loadClassArmorPiece(section, "leggings", arenaClass::setLeggings); - loadClassArmorPiece(section, "boots", arenaClass::setBoots); - loadClassArmorPiece(section, "offhand", arenaClass::setOffHand); + String name = arenaClass.getConfigName(); + loadClassArmorPiece(section, "helmet", name, arenaClass::setHelmet); + loadClassArmorPiece(section, "chestplate", name, arenaClass::setChestplate); + loadClassArmorPiece(section, "leggings", name, arenaClass::setLeggings); + loadClassArmorPiece(section, "boots", name, arenaClass::setBoots); + loadClassArmorPiece(section, "offhand", name, arenaClass::setOffHand); } private void loadClassArmorLegacyNode(ConfigurationSection section, ArenaClass arenaClass) { List armor = section.getStringList("armor"); if (armor == null || armor.isEmpty()) { - String value = section.getString("armor", ""); + String value = section.getString("armor", null); + if (value == null || value.isEmpty()) { + return; + } armor = Arrays.asList(value.split(",")); } - // Prepend "armor:" for the armor thing parser - List things = armor.stream() - .map(String::trim) - .map(s -> "armor:" + s) - .map(plugin.getThingManager()::parse) - .filter(Objects::nonNull) - .collect(Collectors.toList()); - - arenaClass.setArmor(things); + try { + // Prepend "armor:" for the armor thing parser + List things = armor.stream() + .map(String::trim) + .map(s -> plugin.getThingManager().parse("armor", s)) + .collect(Collectors.toList()); + arenaClass.setArmor(things); + } catch (InvalidThingInputString e) { + throw new ConfigError("Failed to parse armor for class " + arenaClass.getConfigName() + ": " + e.getInput()); + } } - private void loadClassArmorPiece(ConfigurationSection section, String slot, Consumer setter) { + private void loadClassArmorPiece(ConfigurationSection section, String slot, String name, Consumer setter) { String value = section.getString(slot, null); if (value == null) { return; } - // Prepend the slot name for the item parser - Thing thing = plugin.getThingManager().parse(slot + ":" + value); - if (thing == null) { - return; + try { + // Prepend the slot name for the item parser + Thing thing = plugin.getThingManager().parse(slot, value); + setter.accept(thing); + } catch (InvalidThingInputString e) { + throw new ConfigError("Failed to parse " + slot + " slot for class " + name + ": " + e.getInput()); } - setter.accept(thing); } private void loadClassPotionEffects(ConfigurationSection section, ArenaClass arenaClass) { List effects = section.getStringList("effects"); if (effects == null || effects.isEmpty()) { - String value = section.getString("effects", ""); + String value = section.getString("effects", null); + if (value == null || value.isEmpty()) { + return; + } effects = Arrays.asList(value.split(",")); } - // Prepend "effect:" for the potion effect thing parser - List things = effects.stream() - .map(String::trim) - .map(s -> "effect:" + s) - .map(plugin.getThingManager()::parse) - .filter(Objects::nonNull) - .collect(Collectors.toList()); + try { + // Prepend "effect:" for the potion effect thing parser + List things = effects.stream() + .map(String::trim) + .map(s -> plugin.getThingManager().parse("effect", s)) + .collect(Collectors.toList()); - arenaClass.setEffects(things); + arenaClass.setEffects(things); + } catch (InvalidThingInputString e) { + throw new ConfigError("Failed to parse potion effect of class " + arenaClass.getConfigName() + ": " + e.getInput()); + } } private void loadClassPermissions(ArenaClass arenaClass, ConfigurationSection section) { - section.getStringList("permissions").stream() - .map(perm -> "perm:" + perm) - .map(plugin.getThingManager()::parse) - .filter(Objects::nonNull) - .forEach(arenaClass::addPermission); + try { + section.getStringList("permissions").stream() + .map(s -> plugin.getThingManager().parse("perm", s)) + .forEach(arenaClass::addPermission); + } catch (InvalidThingInputString e) { + throw new ConfigError("Failed to parse permission of class " + arenaClass.getConfigName() + ": " + e.getInput()); + } } private void loadClassLobbyPermissions(ArenaClass arenaClass, ConfigurationSection section) { - section.getStringList("lobby-permissions").stream() - .map(perm -> "perm:" + perm) - .map(plugin.getThingManager()::parse) - .filter(Objects::nonNull) - .forEach(arenaClass::addLobbyPermission); + try { + section.getStringList("lobby-permissions").stream() + .map(s -> plugin.getThingManager().parse("perm", s)) + .forEach(arenaClass::addLobbyPermission); + } catch (InvalidThingInputString e) { + throw new ConfigError("Failed to parse lobby-permission of class " + arenaClass.getConfigName() + ": " + e.getInput()); + } } /** diff --git a/src/main/java/com/garbagemule/MobArena/ConfigError.java b/src/main/java/com/garbagemule/MobArena/ConfigError.java new file mode 100644 index 0000000..6cd54b6 --- /dev/null +++ b/src/main/java/com/garbagemule/MobArena/ConfigError.java @@ -0,0 +1,7 @@ +package com.garbagemule.MobArena; + +public class ConfigError extends RuntimeException { + public ConfigError(String message) { + super(message); + } +} diff --git a/src/main/java/com/garbagemule/MobArena/MAUtils.java b/src/main/java/com/garbagemule/MobArena/MAUtils.java index 1832a35..46d20b8 100644 --- a/src/main/java/com/garbagemule/MobArena/MAUtils.java +++ b/src/main/java/com/garbagemule/MobArena/MAUtils.java @@ -3,6 +3,7 @@ package com.garbagemule.MobArena; import com.garbagemule.MobArena.framework.Arena; import com.garbagemule.MobArena.framework.ArenaMaster; import com.garbagemule.MobArena.region.ArenaRegion; +import com.garbagemule.MobArena.things.InvalidThingInputString; import com.garbagemule.MobArena.things.Thing; import com.garbagemule.MobArena.util.ItemParser; import com.garbagemule.MobArena.util.TextUtils; @@ -66,13 +67,9 @@ public class MAUtils for (String reward : rewards.split(",")) { try { Thing thing = plugin.getThingManager().parse(reward.trim()); - if (thing == null) { - plugin.getLogger().warning("Failed to parse reward: " + reward.trim()); - } else { - things.add(thing); - } - } catch (Exception e) { - plugin.getLogger().severe("Exception parsing reward '" + reward.trim() + "': " + e.getLocalizedMessage()); + things.add(thing); + } catch (InvalidThingInputString e) { + throw new ConfigError("Failed to parse reward for wave " + wave + " in the '" + type + "' branch of arena " + arena + ": " + e.getInput()); } } result.put(wave, things); diff --git a/src/main/java/com/garbagemule/MobArena/MobArena.java b/src/main/java/com/garbagemule/MobArena/MobArena.java index 2141b63..6e3bec3 100644 --- a/src/main/java/com/garbagemule/MobArena/MobArena.java +++ b/src/main/java/com/garbagemule/MobArena/MobArena.java @@ -100,7 +100,12 @@ public class MobArena extends JavaPlugin // Set up the ArenaMaster arenaMaster = new ArenaMasterImpl(this); - arenaMaster.initialize(); + try { + arenaMaster.initialize(); + } catch (ConfigError e) { + getLogger().severe(e.getMessage()); + return; + } // Load signs after Messenger and ArenaMaster reloadSigns(); diff --git a/src/main/java/com/garbagemule/MobArena/things/InvalidThingInputString.java b/src/main/java/com/garbagemule/MobArena/things/InvalidThingInputString.java new file mode 100644 index 0000000..d0d83f2 --- /dev/null +++ b/src/main/java/com/garbagemule/MobArena/things/InvalidThingInputString.java @@ -0,0 +1,14 @@ +package com.garbagemule.MobArena.things; + +public class InvalidThingInputString extends RuntimeException { + private String input; + + InvalidThingInputString(String input) { + super("Invalid input: " + input); + this.input = input; + } + + public String getInput() { + return input; + } +} diff --git a/src/main/java/com/garbagemule/MobArena/things/ThingManager.java b/src/main/java/com/garbagemule/MobArena/things/ThingManager.java index 31927ea..f236833 100644 --- a/src/main/java/com/garbagemule/MobArena/things/ThingManager.java +++ b/src/main/java/com/garbagemule/MobArena/things/ThingManager.java @@ -64,6 +64,22 @@ public class ThingManager implements ThingParser { @Override public Thing parse(String s) { + Thing thing = parseThing(s); + if (thing != null) { + return thing; + } + throw new InvalidThingInputString(s); + } + + public Thing parse(String prefix, String s) { + Thing thing = parseThing(prefix + ":" + s); + if (thing != null) { + return thing; + } + throw new InvalidThingInputString(s); + } + + private Thing parseThing(String s) { for (ThingParser parser : parsers) { Thing thing = parser.parse(s); if (thing != null) { diff --git a/src/main/java/com/garbagemule/MobArena/waves/WaveParser.java b/src/main/java/com/garbagemule/MobArena/waves/WaveParser.java index 6278f5e..6e4c746 100644 --- a/src/main/java/com/garbagemule/MobArena/waves/WaveParser.java +++ b/src/main/java/com/garbagemule/MobArena/waves/WaveParser.java @@ -1,7 +1,9 @@ package com.garbagemule.MobArena.waves; +import com.garbagemule.MobArena.ConfigError; import com.garbagemule.MobArena.framework.Arena; import com.garbagemule.MobArena.region.ArenaRegion; +import com.garbagemule.MobArena.things.InvalidThingInputString; import com.garbagemule.MobArena.things.Thing; import com.garbagemule.MobArena.things.ThingManager; import com.garbagemule.MobArena.util.ItemParser; @@ -134,7 +136,7 @@ public class WaveParser List spawnpoints = getSpawnpoints(arena, name, config); // Potion effects - List effects = getPotionEffects(config); + List effects = getPotionEffects(arena, name, config); // Recurrent must have priority + frequency, single must have firstWave if (branch == WaveBranch.RECURRENT && (priority == -1 || frequency <= 0)) { @@ -238,7 +240,7 @@ public class WaveParser private static Wave parseUpgradeWave(Arena arena, String name, ConfigurationSection config) { ThingManager thingman = arena.getPlugin().getThingManager(); - Map> upgrades = getUpgradeMap(config, thingman); + Map> upgrades = getUpgradeMap(config, name, arena, thingman); if (upgrades == null || upgrades.isEmpty()) { arena.getPlugin().getLogger().warning(WaveError.UPGRADE_MAP_MISSING.format(name, arena.configName())); return null; @@ -307,13 +309,9 @@ public class WaveParser if (rew != null) { try { Thing thing = arena.getPlugin().getThingManager().parse(rew.trim()); - if (thing == null) { - arena.getPlugin().getLogger().warning("Failed to parse boss reward: " + rew.trim()); - } else { - result.setReward(thing); - } - } catch (Exception e) { - arena.getPlugin().getLogger().severe("Exception parsing boss reward '" + rew.trim() + "': " + e.getLocalizedMessage()); + result.setReward(thing); + } catch (InvalidThingInputString e) { + throw new ConfigError("Failed to parse boss reward in wave " + name + " of arena " + arena.configName() + ": " + e.getInput()); } } @@ -404,27 +402,41 @@ public class WaveParser return result; } - private static List getPotionEffects(ConfigurationSection config) { - String value = config.getString("effects"); - if (value == null) { - value = config.getString("potions"); + private static List getPotionEffects(Arena arena, String name, ConfigurationSection config) { + /* + * TODO: Make this consistent with the rest somehow + * - Things only work for Players (currently) + */ + List effects = config.getStringList("effects"); + if (effects == null || effects.isEmpty()) { + String value = config.getString("effects", null); + if (value != null && !value.isEmpty()) { + effects = Arrays.asList(value.split(",")); + } else { + effects = config.getStringList("potions"); + if (effects == null || effects.isEmpty()) { + value = config.getString("potions", null); + if (value != null && !value.isEmpty()) { + effects = Arrays.asList(value.split(",")); + } else { + return Collections.emptyList(); + } + } + } } - if (value != null) { - List parsed = PotionEffectParser.parsePotionEffects(value); - return (parsed != null) ? parsed : Collections.emptyList(); - } - - List list = config.getStringList("effects"); - if (list.isEmpty()) { - list = config.getStringList("potions"); - } - return list.stream() - .map(PotionEffectParser::parsePotionEffect) - .filter(Objects::nonNull) + return effects.stream() + .map(String::trim) + .map(input -> { + PotionEffect effect = PotionEffectParser.parsePotionEffect(input, false); + if (effect == null) { + throw new ConfigError("Failed to parse potion effect for wave " + name + " of arena " + arena.configName() + ": " + input); + } + return effect; + }) .collect(Collectors.toList()); } - private static Map> getUpgradeMap(ConfigurationSection config, ThingManager thingman) { + private static Map> getUpgradeMap(ConfigurationSection config, String name, Arena arena, ThingManager thingman) { ConfigurationSection section = config.getConfigurationSection("upgrades"); if (section == null) { return null; @@ -442,60 +454,86 @@ public class WaveParser // Legacy support Object val = config.get(path + className, null); if (val instanceof String) { - String s = (String) val; - List things = Arrays.asList(s.split(",")).stream() - .map(String::trim) - .map(thingman::parse) - .filter(Objects::nonNull) - .collect(Collectors.toList()); + List things = loadUpgradesFromString(className, (String) val, name, arena, thingman); upgrades.put(className.toLowerCase(), things); } // New complex setup else if (val instanceof ConfigurationSection) { - ConfigurationSection classSection = (ConfigurationSection) val; - List list = new ArrayList<>(); - - // Items - List items = classSection.getStringList("items"); - if (items == null || items.isEmpty()) { - String value = classSection.getString("items", ""); - items = Arrays.asList(value.split(",")); - } - items.stream() - .map(String::trim) - .map(thingman::parse) - .filter(Objects::nonNull) - .forEach(list::add); - - // Armor - List armor = classSection.getStringList("armor"); - if (armor == null || armor.isEmpty()) { - String value = classSection.getString("armor", ""); - armor = Arrays.asList(value.split(",")); - } - - // Prepend "armor:" for the armor thing parser - armor.stream() - .map(String::trim) - .map(s -> "armor:" + s) - .map(thingman::parse) - .filter(Objects::nonNull) - .forEach(list::add); - - // Prepend "perm:" for the permission thing parser - classSection.getStringList("permissions").stream() - .map(perm -> "perm:" + perm) - .map(thingman::parse) - .filter(Objects::nonNull) - .forEach(list::add); - - // Put in the map + List list = loadUpgradesFromSection(className, (ConfigurationSection) val, name, arena, thingman); upgrades.put(className.toLowerCase(), list); } } return upgrades; } + + private static List loadUpgradesFromString(String className, String value, String name, Arena arena, ThingManager thingman) { + if (value == null || value.isEmpty()) { + return Collections.emptyList(); + } + try { + return Arrays.stream(value.split(",")) + .map(String::trim) + .map(thingman::parse) + .collect(Collectors.toList()); + } catch (InvalidThingInputString e) { + throw new ConfigError("Failed to parse upgrades for class " + className + " in wave " + name + " of arena " + arena.configName() + ": " + e.getInput()); + } + } + + private static List loadUpgradesFromSection(String className, ConfigurationSection classSection, String name, Arena arena, ThingManager thingman) { + List list = new ArrayList<>(); + + // Items + List items = classSection.getStringList("items"); + if (items == null || items.isEmpty()) { + String value = classSection.getString("items", null); + if (value == null || value.isEmpty()) { + items = Collections.emptyList(); + } else { + items = Arrays.asList(value.split(",")); + } + } + try { + items.stream() + .map(String::trim) + .map(thingman::parse) + .forEach(list::add); + } catch (InvalidThingInputString e) { + throw new ConfigError("Failed to parse item upgrades for class " + className + " in wave " + name + " of arena " + arena.configName() + ": " + e.getInput()); + } + + // Armor + List armor = classSection.getStringList("armor"); + if (armor == null || armor.isEmpty()) { + String value = classSection.getString("armor", null); + if (value == null || value.isEmpty()) { + armor = Collections.emptyList(); + } else { + armor = Arrays.asList(value.split(",")); + } + } + try { + // Prepend "armor:" for the armor thing parser + armor.stream() + .map(String::trim) + .map(s -> thingman.parse("armor", s)) + .forEach(list::add); + } catch (InvalidThingInputString e) { + throw new ConfigError("Failed to parse armor upgrades for class " + className + " in wave " + name + " of arena " + arena.configName() + ": " + e.getInput()); + } + + try { + // Prepend "perm:" for the permission thing parser + classSection.getStringList("permissions").stream() + .map(perm -> thingman.parse("perm", perm)) + .forEach(list::add); + } catch (InvalidThingInputString e) { + throw new ConfigError("Failed to parse permission upgrades for class " + className + " in wave " + name + " of arena " + arena.configName() + ": " + e.getInput()); + } + + return list; + } public static Wave createDefaultWave() { SortedMap monsters = new TreeMap<>(); diff --git a/src/test/java/com/garbagemule/MobArena/things/ThingManagerTest.java b/src/test/java/com/garbagemule/MobArena/things/ThingManagerTest.java index 9e3bdf7..28f033d 100644 --- a/src/test/java/com/garbagemule/MobArena/things/ThingManagerTest.java +++ b/src/test/java/com/garbagemule/MobArena/things/ThingManagerTest.java @@ -1,7 +1,6 @@ package com.garbagemule.MobArena.things; -import static org.hamcrest.CoreMatchers.nullValue; -import static org.hamcrest.MatcherAssert.assertThat; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -10,13 +9,18 @@ import static org.mockito.Mockito.when; import com.garbagemule.MobArena.MobArena; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import org.mockito.InOrder; public class ThingManagerTest { private ThingManager subject; + @Rule + public ExpectedException exception = ExpectedException.none(); + @Before public void setup() { MobArena plugin = mock(MobArena.class); @@ -27,6 +31,7 @@ public class ThingManagerTest { public void afterCoreParsersInOrder() { ThingParser first = mock(ThingParser.class); ThingParser second = mock(ThingParser.class); + when(second.parse(anyString())).thenReturn(mock(Thing.class)); subject.register(first /*, false */); subject.register(second /*, false */); @@ -41,6 +46,7 @@ public class ThingManagerTest { public void beforeCoreParsersInverseOrder() { ThingParser first = mock(ThingParser.class); ThingParser second = mock(ThingParser.class); + when(first.parse(anyString())).thenReturn(mock(Thing.class)); subject.register(first, true); subject.register(second, true); @@ -72,7 +78,7 @@ public class ThingManagerTest { } @Test - public void returnsNullIfNoParsersSucceed() { + public void throwsIfNoParsersSucceed() { ThingParser first = mock(ThingParser.class); ThingParser second = mock(ThingParser.class); when(first.parse("thing")).thenReturn(null); @@ -80,9 +86,9 @@ public class ThingManagerTest { subject.register(first); subject.register(second); - Thing result = subject.parse("thing"); + exception.expect(InvalidThingInputString.class); - assertThat(result, nullValue()); + subject.parse("thing"); } }