Rewrite ThingParser usages.

This commit changes how the ThingParser is used throughout the code base. Instead of blindly filtering out null values, we're now throwing a new InvalidThingInputString exception. This exception is caught in an outer scope that has more context. The exception is then unwrapped and rethrown as a ConfigError with the additional context. In the outermost scope of (re)loading the config-file, the ConfigError is caught and printed, and then (re)loading stops gracefully.

We still need a proper way to handle loads/reloads consistently to get rid of the default command usage message, but this is a good step towards better usability in the face of user errors.

Fixes #478
This commit is contained in:
Andreas Troelsen 2018-08-05 22:48:35 +02:00
parent 924a419b47
commit cd82a89626
10 changed files with 238 additions and 140 deletions

View File

@ -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

View File

@ -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());
}
}
}

View File

@ -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<Thing> things = items.stream()
.map(String::trim)
.map(plugin.getThingManager()::parse)
.filter(Objects::nonNull)
.collect(Collectors.toList());
arenaClass.setItems(things);
try {
List<Thing> 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<String> 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<Thing> 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<Thing> 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<Thing> setter) {
private void loadClassArmorPiece(ConfigurationSection section, String slot, String name, Consumer<Thing> 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<String> 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<Thing> 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<Thing> 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());
}
}
/**

View File

@ -0,0 +1,7 @@
package com.garbagemule.MobArena;
public class ConfigError extends RuntimeException {
public ConfigError(String message) {
super(message);
}
}

View File

@ -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);

View File

@ -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();

View File

@ -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;
}
}

View File

@ -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) {

View File

@ -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<Location> spawnpoints = getSpawnpoints(arena, name, config);
// Potion effects
List<PotionEffect> effects = getPotionEffects(config);
List<PotionEffect> 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<String,List<Thing>> upgrades = getUpgradeMap(config, thingman);
Map<String,List<Thing>> 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<PotionEffect> getPotionEffects(ConfigurationSection config) {
String value = config.getString("effects");
if (value == null) {
value = config.getString("potions");
private static List<PotionEffect> getPotionEffects(Arena arena, String name, ConfigurationSection config) {
/*
* TODO: Make this consistent with the rest somehow
* - Things only work for Players (currently)
*/
List<String> 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<PotionEffect> parsed = PotionEffectParser.parsePotionEffects(value);
return (parsed != null) ? parsed : Collections.emptyList();
}
List<String> 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<String,List<Thing>> getUpgradeMap(ConfigurationSection config, ThingManager thingman) {
private static Map<String,List<Thing>> 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<Thing> things = Arrays.asList(s.split(",")).stream()
.map(String::trim)
.map(thingman::parse)
.filter(Objects::nonNull)
.collect(Collectors.toList());
List<Thing> 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<Thing> list = new ArrayList<>();
// Items
List<String> 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<String> 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<Thing> list = loadUpgradesFromSection(className, (ConfigurationSection) val, name, arena, thingman);
upgrades.put(className.toLowerCase(), list);
}
}
return upgrades;
}
private static List<Thing> 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<Thing> loadUpgradesFromSection(String className, ConfigurationSection classSection, String name, Arena arena, ThingManager thingman) {
List<Thing> list = new ArrayList<>();
// Items
List<String> 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<String> 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<Integer,MACreature> monsters = new TreeMap<>();

View File

@ -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");
}
}