Refactor Vault economy integration.

This commit reworks how MobArena interfaces with Vault economy provider
instances. Instead of affixing an Economy _instance_ to the main plugin
at "setup time", we create an instance of the new _Finance_ abstraction
at "(re)load time", and then use this new abstraction at every call site
(really just the MoneyThing) instead of Economy. This accomplishes two
important things:

1. Due to how the Bukkit load order works and how Vault providers are
registered with the plugin, the Finance abstraction gives us a means of
deferring the retrieval of the Economy provider needed to integrate with
the underlying economy plugin (or script). MobArena still uses Finance
as if it was a "complete" object and is completely unaware of what goes
on under the hodd. This is a much more elegant solution compared to the
terrible past proposals of fragmenting MobArena's bootstrapping phase,
especially considering the past proposals were server tick-centric...

2. It creates a simple and maintainable interface between MobArena and
Vault, effectively giving control back to MobArena as to how everything
should mesh together.

The biggest downside to this solution is that we no longer know if an
economy provider is available for when we need it at a convenient time
in the startup process. Instead, we need to react accordingly when an
economy operation is invoked. The implementation takes a somewhat mild
approach by simply _logging_ any issues encountered and defaulting to
"best effort" operations when an economy provider is not available. At
worst, this is a bunch of log spam and potentially a lot of work for
server owners to clean up after (compensating players for rewards and
stuff like that), but MobArena doesn't handle arbitrary exceptions in
its join/leave and start/end procedures very well, so it's probably a
better way to go about it than throwing exceptions.

If the server does not have Vault installed, an UnsupportedFinance is
created. This implementation just logs errors and "fails to work" on
every operation. If Vault is installed, a VaultFinance is created, and
this implementation should only log errors if a Vault economy provider
couldn't be found. We could have opted for a single implementation, but
this approach allows us to potentially support other economy registrars
in the future, so we (probably) won't have to make sweeping changes in
order to swap to something else.

Closes #797

changelog.md

@@ -11,6 +11,9 @@ These changes will (most likely) be included in the next version.
 
 
 ## [Unreleased]
+### Added
+- MobArena now properly supports Vault economy providers registered after MobArena has started. This should make it possible to use custom economy providers that aren't built into Vault, such as those created with Denizen.
+
 ### Changed
 - Recurrent waves can now be randomized. If two or more recurrent waves clash on wave number, frequency, _and_ priority, MobArena will now randomly pick between them. This should make it easier to create more varied and interesting wave setups without having to resort to only massively randomized default waves.
 - Single waves can now be randomized. If two or more single waves clash on wave number, MobArena will now randomly pick between them. This means it is now possible to make randomly selected bosses for boss waves, for instance.

src/main/java/com/garbagemule/MobArena/MobArena.java

@@ -2,6 +2,8 @@ package com.garbagemule.MobArena;
 
 import com.garbagemule.MobArena.commands.CommandHandler;
 import com.garbagemule.MobArena.config.LoadsConfigFile;
+import com.garbagemule.MobArena.finance.Finance;
+import com.garbagemule.MobArena.finance.FinanceFactory;
 import com.garbagemule.MobArena.events.MobArenaPreReloadEvent;
 import com.garbagemule.MobArena.events.MobArenaReloadEvent;
 import com.garbagemule.MobArena.formula.FormulaMacros;
@@ -26,16 +28,12 @@ import com.garbagemule.MobArena.things.ThingManager;
 import com.garbagemule.MobArena.things.ThingPickerManager;
 import com.garbagemule.MobArena.util.config.ConfigUtils;
 import com.garbagemule.MobArena.waves.ability.AbilityManager;
-import net.milkbowl.vault.economy.Economy;
 import org.bstats.bukkit.Metrics;
 import org.bukkit.ChatColor;
 import org.bukkit.configuration.InvalidConfigurationException;
 import org.bukkit.configuration.file.FileConfiguration;
 import org.bukkit.configuration.file.YamlConfiguration;
-import org.bukkit.plugin.Plugin;
 import org.bukkit.plugin.PluginManager;
-import org.bukkit.plugin.RegisteredServiceProvider;
-import org.bukkit.plugin.ServicesManager;
 import org.bukkit.plugin.java.JavaPlugin;
 
 import java.io.File;
@@ -51,8 +49,7 @@ public class MobArena extends JavaPlugin
 {
     private ArenaMaster arenaMaster;
 
-    // Vault
+    private Finance finance;
-    private Economy economy;
 
     private FileConfiguration config;
     private LoadsConfigFile loadsConfigFile;
@@ -112,7 +109,6 @@ public class MobArena extends JavaPlugin
             setupArenaMaster();
             setupCommandHandler();
 
-            setupVault();
             setupBossAbilities();
             setupListeners();
             setupMetrics();
@@ -149,24 +145,6 @@ public class MobArena extends JavaPlugin
         getCommand("ma").setExecutor(new CommandHandler(this));
     }
 
-    private void setupVault() {
-        Plugin vaultPlugin = this.getServer().getPluginManager().getPlugin("Vault");
-        if (vaultPlugin == null) {
-            getLogger().info("Vault was not found. Economy rewards will not work.");
-            return;
-        }
-
-        ServicesManager manager = this.getServer().getServicesManager();
-        RegisteredServiceProvider<Economy> e = manager.getRegistration(net.milkbowl.vault.economy.Economy.class);
-
-        if (e != null) {
-            economy = e.getProvider();
-            getLogger().info("Vault found; economy rewards enabled.");
-        } else {
-            getLogger().warning("Vault found, but no economy plugin detected. Economy rewards will not work!");
-        }
-    }
-
     private void setupBossAbilities() {
         AbilityManager.loadCoreAbilities();
         AbilityManager.loadCustomAbilities(getDataFolder());
@@ -193,6 +171,7 @@ public class MobArena extends JavaPlugin
         getServer().getPluginManager().callEvent(pre);
 
         try {
+            reloadFinance();
             reloadConfig();
             reloadGlobalMessenger();
             reloadFormulaMacros();
@@ -209,6 +188,10 @@ public class MobArena extends JavaPlugin
         getServer().getPluginManager().callEvent(post);
     }
 
+    private void reloadFinance() {
+        finance = FinanceFactory.create(getServer(), getLogger());
+    }
+
     @Override
     public void reloadConfig() {
         if (loadsConfigFile == null) {
@@ -305,8 +288,8 @@ public class MobArena extends JavaPlugin
         return arenaMaster;
     }
 
-    public Economy getEconomy() {
+    public Finance getFinance() {
-        return economy;
+        return finance;
     }
 
     public Messenger getGlobalMessenger() {

src/main/java/com/garbagemule/MobArena/finance/Finance.java

@@ -0,0 +1,15 @@
+package com.garbagemule.MobArena.finance;
+
+import org.bukkit.entity.Player;
+
+public interface Finance {
+
+    double getBalance(Player player);
+
+    boolean deposit(Player player, double amount);
+
+    boolean withdraw(Player player, double amount);
+
+    String format(double amount);
+
+}

src/main/java/com/garbagemule/MobArena/finance/FinanceFactory.java

@@ -0,0 +1,25 @@
+package com.garbagemule.MobArena.finance;
+
+import org.bukkit.Server;
+import org.bukkit.plugin.Plugin;
+import org.bukkit.plugin.ServicesManager;
+
+import java.util.logging.Logger;
+
+public class FinanceFactory {
+
+    FinanceFactory() {
+        // OK BOSS
+    }
+
+    public static Finance create(Server server, Logger log) {
+        Plugin plugin = server.getPluginManager().getPlugin("Vault");
+        if (plugin == null) {
+            return new UnsupportedFinance(log);
+        }
+
+        ServicesManager services = server.getServicesManager();
+        return new VaultFinance(services, log);
+    }
+
+}

src/main/java/com/garbagemule/MobArena/finance/UnsupportedFinance.java

@@ -0,0 +1,39 @@
+package com.garbagemule.MobArena.finance;
+
+import org.bukkit.entity.Player;
+
+import java.util.logging.Logger;
+
+public class UnsupportedFinance implements Finance {
+
+    private final Logger log;
+
+    public UnsupportedFinance(Logger log) {
+        this.log = log;
+    }
+
+    @Override
+    public double getBalance(Player player) {
+        log.severe("Economy operations are only supported via Vault!");
+        return -1.0;
+    }
+
+    @Override
+    public boolean deposit(Player player, double amount) {
+        log.severe("Economy operations are only supported via Vault!");
+        return false;
+    }
+
+    @Override
+    public boolean withdraw(Player player, double amount) {
+        log.severe("Economy operations are only supported via Vault!");
+        return false;
+    }
+
+    @Override
+    public String format(double amount) {
+        log.severe("Economy operations are only supported via Vault!");
+        return "ERROR";
+    }
+
+}

src/main/java/com/garbagemule/MobArena/finance/VaultFinance.java

@@ -0,0 +1,78 @@
+package com.garbagemule.MobArena.finance;
+
+import net.milkbowl.vault.economy.Economy;
+import net.milkbowl.vault.economy.EconomyResponse;
+import org.bukkit.entity.Player;
+import org.bukkit.plugin.RegisteredServiceProvider;
+import org.bukkit.plugin.ServicesManager;
+
+import java.util.logging.Logger;
+
+public class VaultFinance implements Finance {
+
+    private final ServicesManager services;
+    private final Logger log;
+
+    private Economy economy;
+
+    public VaultFinance(ServicesManager services, Logger log) {
+        this.services = services;
+        this.log = log;
+    }
+
+    @Override
+    public double getBalance(Player player) {
+        try {
+            return getEconomy().getBalance(player);
+        } catch (IllegalStateException e) {
+            log.severe("Failed to check balance of player " + player.getName() + " because: " + e.getMessage());
+            return 0;
+        }
+    }
+
+    @Override
+    public boolean deposit(Player player, double amount) {
+        try {
+            EconomyResponse res = getEconomy().depositPlayer(player, amount);
+            return res.type == EconomyResponse.ResponseType.SUCCESS;
+        } catch (IllegalStateException e) {
+            log.severe("Failed to give " + amount + " economy money to player " + player.getName() + " because: " + e.getMessage());
+            return false;
+        }
+    }
+
+    @Override
+    public boolean withdraw(Player player, double amount) {
+        try {
+            EconomyResponse res = getEconomy().withdrawPlayer(player, amount);
+            return res.type == EconomyResponse.ResponseType.SUCCESS;
+        } catch (IllegalStateException e) {
+            log.severe("Failed to take " + amount + " economy money from player " + player.getName() + " because: " + e.getMessage());
+            return false;
+        }
+    }
+
+    @Override
+    public String format(double amount) {
+        try {
+            return getEconomy().format(amount);
+        } catch (IllegalStateException e) {
+            log.severe("Failed to format " + amount + " as economy money because: " + e.getMessage());
+            return "ERROR";
+        }
+    }
+
+    private Economy getEconomy() {
+        if (economy != null) {
+            return economy;
+        }
+
+        RegisteredServiceProvider<Economy> provider = services.getRegistration(Economy.class);
+        if (provider == null) {
+            throw new IllegalStateException("No Vault economy provider found!");
+        }
+
+        return (economy = provider.getProvider());
+    }
+
+}

src/main/java/com/garbagemule/MobArena/things/MoneyThing.java

@@ -1,52 +1,36 @@
 package com.garbagemule.MobArena.things;
 
-import net.milkbowl.vault.economy.Economy;
+import com.garbagemule.MobArena.finance.Finance;
-import net.milkbowl.vault.economy.EconomyResponse;
-import net.milkbowl.vault.economy.EconomyResponse.ResponseType;
 import org.bukkit.entity.Player;
 
 public class MoneyThing implements Thing {
 
-    private final Economy economy;
+    private final Finance finance;
     private final double amount;
 
-    public MoneyThing(Economy economy, double amount) {
+    public MoneyThing(Finance finance, double amount) {
-        this.economy = economy;
+        this.finance = finance;
         this.amount = amount;
     }
 
     @Override
     public boolean giveTo(Player player) {
-        if (economy == null) {
+        return finance.deposit(player, amount);
-            return false;
-        }
-        EconomyResponse result = economy.depositPlayer(player, amount);
-        return result.type == ResponseType.SUCCESS;
     }
 
     @Override
     public boolean takeFrom(Player player) {
-        if (economy == null) {
+        return finance.withdraw(player, amount);
-            return false;
-        }
-        EconomyResponse result = economy.withdrawPlayer(player, amount);
-        return result.type == ResponseType.SUCCESS;
     }
 
     @Override
     public boolean heldBy(Player player) {
-        if (economy == null) {
+        return finance.getBalance(player) >= amount;
-            return false;
-        }
-        return economy.getBalance(player) >= amount;
     }
 
     @Override
     public String toString() {
-        if (economy == null) {
+        return finance.format(amount);
-            return "$" + amount;
-        }
-        return economy.format(amount);
     }
 
 }

src/main/java/com/garbagemule/MobArena/things/MoneyThingParser.java

@@ -1,7 +1,7 @@
 package com.garbagemule.MobArena.things;
 
 import com.garbagemule.MobArena.MobArena;
-import net.milkbowl.vault.economy.Economy;
+import com.garbagemule.MobArena.finance.Finance;
 
 class MoneyThingParser implements ThingParser {
 
@@ -20,11 +20,11 @@ class MoneyThingParser implements ThingParser {
         if (money == null) {
             return null;
         }
-        Economy economy = plugin.getEconomy();
+
-        if (economy == null) {
+        Finance finance = plugin.getFinance();
-            plugin.getLogger().severe("Vault or economy plugin missing while parsing: " + s);
+        double amount = Double.parseDouble(money);
-        }
+
-        return new MoneyThing(economy, Double.parseDouble(money));
+        return new MoneyThing(finance, amount);
     }
 
     private String trimPrefix(String s) {

src/test/java/com/garbagemule/MobArena/finance/FinanceFactoryTest.java

@@ -0,0 +1,58 @@
+package com.garbagemule.MobArena.finance;
+
+import org.bukkit.Server;
+import org.bukkit.plugin.Plugin;
+import org.bukkit.plugin.PluginManager;
+import org.junit.Test;
+
+import java.util.logging.Logger;
+
+import static org.hamcrest.CoreMatchers.instanceOf;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+public class FinanceFactoryTest {
+
+    @Test
+    public void looksUpVaultPluginFromPluginManager() {
+        Server server = mock(Server.class);
+        Logger log = mock(Logger.class);
+        PluginManager plugins = mock(PluginManager.class);
+        when(server.getPluginManager()).thenReturn(plugins);
+
+        FinanceFactory.create(server, log);
+
+        verify(plugins).getPlugin("Vault");
+    }
+
+    @Test
+    public void createsUnsupportedFinanceIfVaultNotPresent() {
+        Server server = mock(Server.class);
+        Logger log = mock(Logger.class);
+        PluginManager plugins = mock(PluginManager.class);
+        when(server.getPluginManager()).thenReturn(plugins);
+        when(plugins.getPlugin(any())).thenReturn(null);
+
+        Finance result = FinanceFactory.create(server, log);
+
+        assertThat(result, instanceOf(UnsupportedFinance.class));
+    }
+
+    @Test
+    public void createsVaultFinanceIfVaultIsPresent() {
+        Server server = mock(Server.class);
+        Logger log = mock(Logger.class);
+        Plugin vault = mock(Plugin.class);
+        PluginManager plugins = mock(PluginManager.class);
+        when(server.getPluginManager()).thenReturn(plugins);
+        when(plugins.getPlugin(any())).thenReturn(vault);
+
+        Finance result = FinanceFactory.create(server, log);
+
+        assertThat(result, instanceOf(VaultFinance.class));
+    }
+
+}

src/test/java/com/garbagemule/MobArena/finance/UnsupportedFinanceTest.java

@@ -0,0 +1,82 @@
+package com.garbagemule.MobArena.finance;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.logging.Logger;
+
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+
+public class UnsupportedFinanceTest {
+
+    private Logger log;
+    private UnsupportedFinance subject;
+
+    @Before
+    public void setup() {
+        log = mock(Logger.class);
+
+        subject = new UnsupportedFinance(log);
+    }
+
+    @Test
+    public void logsErrorOnGetBalance() {
+        subject.getBalance(null);
+
+        verify(log).severe(anyString());
+    }
+
+    @Test
+    public void returnsNegativeOneOnGetBalance() {
+        double result = subject.getBalance(null);
+
+        assertThat(result, equalTo(-1.0));
+    }
+
+    @Test
+    public void logsErrorOnDeposit() {
+        subject.deposit(null, 1337);
+
+        verify(log).severe(anyString());
+    }
+
+    @Test
+    public void returnsFalseOnDeposit() {
+        boolean result = subject.deposit(null, 1337);
+
+        assertThat(result, equalTo(false));
+    }
+
+    @Test
+    public void logsErrorOnWithdraw() {
+        subject.withdraw(null, 1337);
+
+        verify(log).severe(anyString());
+    }
+
+    @Test
+    public void returnsFalseOnWithdraw() {
+        boolean result = subject.withdraw(null, 1337);
+
+        assertThat(result, equalTo(false));
+    }
+
+    @Test
+    public void logsErrorOnFormat() {
+        subject.format(1337);
+
+        verify(log).severe(anyString());
+    }
+
+    @Test
+    public void returnsErrorStringOnFormat() {
+        String result = subject.format(1337);
+
+        assertThat(result, equalTo("ERROR"));
+    }
+
+}

src/test/java/com/garbagemule/MobArena/finance/VaultFinanceTest.java

@@ -0,0 +1,146 @@
+package com.garbagemule.MobArena.finance;
+
+import net.milkbowl.vault.economy.Economy;
+import net.milkbowl.vault.economy.EconomyResponse;
+import org.bukkit.entity.Player;
+import org.bukkit.plugin.Plugin;
+import org.bukkit.plugin.RegisteredServiceProvider;
+import org.bukkit.plugin.ServicePriority;
+import org.bukkit.plugin.ServicesManager;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.logging.Logger;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyDouble;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+public class VaultFinanceTest {
+
+    private ServicesManager services;
+    private Logger log;
+    private VaultFinance subject;
+
+    @Before
+    public void setup() {
+        services = mock(ServicesManager.class);
+        log = mock(Logger.class);
+
+        subject = new VaultFinance(services, log);
+    }
+
+    @Test
+    public void looksUpEconomyFromServicesManager() {
+        Player player = mock(Player.class);
+        when(player.getName()).thenReturn("Test Subject 287");
+
+        subject.getBalance(player);
+
+        verify(services).getRegistration(Economy.class);
+    }
+
+    @Test
+    public void logsErrorIfNoEconomyFound() {
+        Player player = mock(Player.class);
+        when(player.getName()).thenReturn("Test Subject 287");
+        when(services.getRegistration(Economy.class)).thenReturn(null);
+
+        subject.getBalance(player);
+
+        verify(log).severe(anyString());
+    }
+
+    @Test
+    public void initializationIsRepeatedIfEconomyIsNotFound() {
+        Player player = mock(Player.class);
+        when(player.getName()).thenReturn("Test Subject 287");
+        when(services.getRegistration(Economy.class)).thenReturn(null);
+
+        subject.getBalance(player);
+        subject.getBalance(player);
+        subject.getBalance(player);
+
+        verify(services, times(3)).getRegistration(Economy.class);
+    }
+
+    @Test
+    public void initializationIsIdempotentIfEconomyIsFound() {
+        Player player = mock(Player.class);
+        Economy economy = mock(Economy.class);
+        RegisteredServiceProvider<Economy> provider = createProvider(economy);
+        when(services.getRegistration(Economy.class)).thenReturn(provider);
+
+        subject.getBalance(player);
+
+        verify(services, times(1)).getRegistration(Economy.class);
+    }
+
+    @Test
+    public void delegatesGetBalanceToEconomyWithGivenPlayer() {
+        Player player = mock(Player.class);
+        Economy economy = mock(Economy.class);
+        RegisteredServiceProvider<Economy> provider = createProvider(economy);
+        when(services.getRegistration(Economy.class)).thenReturn(provider);
+
+        subject.getBalance(player);
+
+        verify(economy).getBalance(player);
+    }
+
+    @Test
+    public void delegatesDepositToEconomyWithGivenPlayerAndAmount() {
+        Player player = mock(Player.class);
+        double amount = 1337;
+        Economy economy = mock(Economy.class);
+        EconomyResponse res = new EconomyResponse(0, 0, EconomyResponse.ResponseType.SUCCESS, null);
+        RegisteredServiceProvider<Economy> provider = createProvider(economy);
+        when(services.getRegistration(Economy.class)).thenReturn(provider);
+        when(economy.depositPlayer(any(Player.class), anyDouble())).thenReturn(res);
+
+        subject.deposit(player, amount);
+
+        verify(economy).depositPlayer(player, amount);
+    }
+
+    @Test
+    public void delegatesWithdrawToEconomyWithGivenPlayerAndAmount() {
+        Player player = mock(Player.class);
+        double amount = 1337;
+        Economy economy = mock(Economy.class);
+        EconomyResponse res = new EconomyResponse(0, 0, EconomyResponse.ResponseType.SUCCESS, null);
+        RegisteredServiceProvider<Economy> provider = createProvider(economy);
+        when(services.getRegistration(Economy.class)).thenReturn(provider);
+        when(economy.withdrawPlayer(any(Player.class), anyDouble())).thenReturn(res);
+
+        subject.withdraw(player, amount);
+
+        verify(economy).withdrawPlayer(player, amount);
+    }
+
+    @Test
+    public void delegatesFormatToEconomyWithGivenAmount() {
+        double amount = 1337;
+        Economy economy = mock(Economy.class);
+        RegisteredServiceProvider<Economy> provider = createProvider(economy);
+        when(services.getRegistration(Economy.class)).thenReturn(provider);
+
+        subject.format(amount);
+
+        verify(economy).format(amount);
+    }
+
+    private RegisteredServiceProvider<Economy> createProvider(Economy economy) {
+        return new RegisteredServiceProvider<>(
+            Economy.class,
+            economy,
+            ServicePriority.Normal,
+            mock(Plugin.class)
+        );
+    }
+
+}

src/test/java/com/garbagemule/MobArena/things/MoneyThingParserTest.java

@@ -1,23 +1,20 @@
 package com.garbagemule.MobArena.things;
 
 import com.garbagemule.MobArena.MobArena;
-import net.milkbowl.vault.economy.Economy;
+import com.garbagemule.MobArena.finance.Finance;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 
-import java.util.logging.Logger;
-
 import static org.hamcrest.CoreMatchers.*;
 import static org.hamcrest.MatcherAssert.*;
-import static org.mockito.ArgumentMatchers.*;
 import static org.mockito.Mockito.*;
 
 public class MoneyThingParserTest {
 
-    private MoneyThingParser subject;
     private MobArena plugin;
+    private MoneyThingParser subject;
 
     @Rule
     public ExpectedException exception = ExpectedException.none();
@@ -25,8 +22,7 @@ public class MoneyThingParserTest {
     @Before
     public void setup() {
         plugin = mock(MobArena.class);
-        Economy economy = mock(Economy.class);
+        when(plugin.getFinance()).thenReturn(mock(Finance.class));
-        when(plugin.getEconomy()).thenReturn(economy);
 
         subject = new MoneyThingParser(plugin);
     }
@@ -52,17 +48,6 @@ public class MoneyThingParserTest {
         assertThat(result, not(nullValue()));
     }
 
-    @Test
-    public void nullEconomyNullMoney() {
-        Logger logger = mock(Logger.class);
-        when(plugin.getEconomy()).thenReturn(null);
-        when(plugin.getLogger()).thenReturn(logger);
-
-        subject.parse("$500");
-
-        verify(logger).severe(anyString());
-    }
-
     @Test
     public void numberFormatForNaughtyValues() {
         exception.expect(NumberFormatException.class);