Remove migration of commands from config.yml to commands.yml

- Migration was shipped with the 5.2 release and is now becoming harder to maintain; since it's quite old we drop it

src/main/java/fr/xephi/authme/settings/SettingsMigrationService.java

@@ -2,7 +2,6 @@ package fr.xephi.authme.settings;
 
 import ch.jalu.configme.migration.PlainMigrationService;
 import ch.jalu.configme.properties.Property;
-import ch.jalu.configme.properties.StringListProperty;
 import ch.jalu.configme.resource.PropertyResource;
 import com.google.common.base.MoreObjects;
 import fr.xephi.authme.ConsoleLogger;
@@ -19,7 +18,6 @@ import javax.inject.Inject;
 import java.io.File;
 import java.io.FileWriter;
 import java.io.IOException;
-import java.util.Collections;
 import java.util.List;
 import java.util.Set;
 
@@ -39,15 +37,6 @@ public class SettingsMigrationService extends PlainMigrationService {
 
     private final File pluginFolder;
 
-    // Stores old commands that need to be migrated to the new commands configuration
-    // We need to store it in here for retrieval when we build the CommandConfig. Retrieving it from the config.yml is
-    // not possible since this migration service may trigger config.yml to be resaved. As the old command settings
-    // don't exist in the code anymore, as soon as config.yml is resaved we lose this information.
-    private List<String> onLoginCommands = Collections.emptyList();
-    private List<String> onLoginConsoleCommands = Collections.emptyList();
-    private List<String> onRegisterCommands = Collections.emptyList();
-    private List<String> onRegisterConsoleCommands = Collections.emptyList();
-
     @Inject
     SettingsMigrationService(@DataFolder File pluginFolder) {
         this.pluginFolder = pluginFolder;
@@ -62,8 +51,6 @@ public class SettingsMigrationService extends PlainMigrationService {
             changes = true;
         }
 
-        gatherOldCommandSettings(resource);
-
         // Note ljacqu 20160211: Concatenating migration methods with | instead of the usual ||
         // ensures that all migrations will be performed
         return changes
@@ -97,35 +84,6 @@ public class SettingsMigrationService extends PlainMigrationService {
         return false;
     }
 
-    // ----------------
-    // Forced commands relocation (from config.yml to commands.yml)
-    // ----------------
-    private void gatherOldCommandSettings(PropertyResource resource) {
-        onLoginCommands = getStringList(resource, "settings.forceCommands");
-        onLoginConsoleCommands = getStringList(resource, "settings.forceCommandsAsConsole");
-        onRegisterCommands = getStringList(resource, "settings.forceRegisterCommands");
-        onRegisterConsoleCommands = getStringList(resource, "settings.forceRegisterCommandsAsConsole");
-    }
-
-    private List<String> getStringList(PropertyResource resource, String path) {
-        return new StringListProperty(path).getValue(resource);
-    }
-
-    public List<String> getOnLoginCommands() {
-        return onLoginCommands;
-    }
-
-    public List<String> getOnLoginConsoleCommands() {
-        return onLoginConsoleCommands;
-    }
-
-    public List<String> getOnRegisterCommands() {
-        return onRegisterCommands;
-    }
-
-    public List<String> getOnRegisterConsoleCommands() {
-        return onRegisterConsoleCommands;
-    }
 
     // --------
     // Specific migrations

src/main/java/fr/xephi/authme/settings/commandconfig/CommandMigrationService.java

@@ -5,16 +5,8 @@ import ch.jalu.configme.properties.Property;
 import ch.jalu.configme.resource.PropertyResource;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
-import fr.xephi.authme.ConsoleLogger;
-import fr.xephi.authme.settings.SettingsMigrationService;
-import fr.xephi.authme.util.RandomStringUtils;
 
-import javax.inject.Inject;
 import java.util.List;
-import java.util.Map;
-import java.util.function.BiFunction;
-import java.util.function.Function;
-import java.util.stream.Collectors;
 
 /**
  * Migrates the commands from their old location, in config.yml, to the dedicated commands configuration file.
@@ -26,131 +18,20 @@ class CommandMigrationService implements MigrationService {
     static final List<String> COMMAND_CONFIG_PROPERTIES = ImmutableList.of(
         "onJoin", "onLogin", "onSessionLogin", "onFirstLogin", "onRegister", "onUnregister", "onLogout");
 
-    @Inject
-    private SettingsMigrationService settingsMigrationService;
-
     CommandMigrationService() {
     }
 
     @Override
     public boolean checkAndMigrate(PropertyResource resource, List<Property<?>> properties) {
         final CommandConfig commandConfig = CommandSettingsHolder.COMMANDS.getValue(resource);
-        final boolean didMoveCommands = transformOldCommands(commandConfig);
+        if (isFileEmpty(resource)) {
-
-        if (didMoveCommands || isFileEmpty(resource)) {
             resource.setValue("", commandConfig);
             return true;
         }
         return false;
     }
 
-    private boolean isFileEmpty(PropertyResource resource) {
+    private static boolean isFileEmpty(PropertyResource resource) {
         return COMMAND_CONFIG_PROPERTIES.stream().anyMatch(property -> resource.getObject(property) == null);
     }
-
-    /**
-     * Adds command settings from their old location (in config.yml) to the given command configuration object.
-     *
-     * @param commandConfig the command config object to move old commands to
-     * @return true if commands have been moved, false if no migration was necessary
-     */
-    @VisibleForTesting
-    boolean transformOldCommands(CommandConfig commandConfig) {
-        boolean didMoveCommands = false;
-        for (MigratableCommandSection section : MigratableCommandSection.values()) {
-            didMoveCommands |= section.convertCommands(settingsMigrationService, commandConfig);
-        }
-        return didMoveCommands;
-    }
-
-    /**
-     * Enum defining the forced command settings that should be moved from config.yml to the new commands.yml file.
-     */
-    private enum MigratableCommandSection {
-
-        ON_JOIN(
-            SettingsMigrationService::getOnLoginCommands,
-            Executor.PLAYER,
-            CommandConfig::getOnLogin,
-            OnLoginCommand::new),
-
-        ON_JOIN_CONSOLE(
-            SettingsMigrationService::getOnLoginConsoleCommands,
-            Executor.CONSOLE,
-            CommandConfig::getOnLogin,
-            OnLoginCommand::new),
-
-        ON_REGISTER(
-            SettingsMigrationService::getOnRegisterCommands,
-            Executor.PLAYER,
-            CommandConfig::getOnRegister),
-
-        ON_REGISTER_CONSOLE(
-            SettingsMigrationService::getOnRegisterConsoleCommands,
-            Executor.CONSOLE,
-            CommandConfig::getOnRegister);
-
-        private final Function<SettingsMigrationService, List<String>> legacyCommandsGetter;
-        private final Executor executor;
-        private final Function<CommandConfig, Map<String, Command>> commandMapGetter;
-        private final BiFunction<String, Executor, Command> commandConstructor;
-
-        /**
-         * Constructor.
-         *
-         * @param legacyCommandsGetter getter on MigrationService to get the deprecated command entries
-         * @param executor the executor of the commands
-         * @param commandMapGetter the getter for the commands map in the new settings structure to add the old
-         *                         settings to after conversion
-         */
-        MigratableCommandSection(Function<SettingsMigrationService, List<String>> legacyCommandsGetter,
-                                 Executor executor,
-                                 Function<CommandConfig, Map<String, Command>> commandMapGetter) {
-            this(legacyCommandsGetter, executor, commandMapGetter, Command::new);
-        }
-
-        /**
-         * Constructor.
-         *
-         * @param legacyCommandsGetter getter on MigrationService to get the deprecated command entries
-         * @param executor the executor of the commands
-         * @param commandMapGetter the getter for the commands map in the new settings structure to add the old
-         *                         settings to after conversion
-         * @param commandConstructor constructor for creating a command object
-         */
-        <T extends Command> MigratableCommandSection(
-            Function<SettingsMigrationService, List<String>> legacyCommandsGetter,
-            Executor executor,
-            Function<CommandConfig, Map<String, T>> commandMapGetter,
-            BiFunction<String, Executor, T> commandConstructor) {
-
-            this.legacyCommandsGetter = legacyCommandsGetter;
-            this.executor = executor;
-            // This is horrible to be doing but this way we don't need to cast in convertCommands()
-            this.commandMapGetter = (Function) commandMapGetter;
-            this.commandConstructor = (BiFunction) commandConstructor;
-        }
-
-        /**
-         * Adds the commands from the sections' settings migration service to the appropriate place in the new
-         * command config object.
-         *
-         * @param settingsMigrationService settings migration service to read old commands from
-         * @param commandConfig command config object to add converted commands to
-         * @return true if there were commands to migrate, false otherwise
-         */
-        boolean convertCommands(SettingsMigrationService settingsMigrationService, CommandConfig commandConfig) {
-            List<Command> commands = legacyCommandsGetter.apply(settingsMigrationService).stream()
-                .map(cmd -> commandConstructor.apply(cmd, executor)).collect(Collectors.toList());
-
-            if (commands.isEmpty()) {
-                return false;
-            }
-            Map<String, Command> commandMap = commandMapGetter.apply(commandConfig);
-            commands.forEach(cmd -> commandMap.put(RandomStringUtils.generate(10), cmd));
-            ConsoleLogger.info("Moving " + commands.size() + " commands of type " + this
-                + " from config.yml to commands.yml");
-            return true;
-        }
-    }
 }

src/test/java/fr/xephi/authme/settings/SettingsMigrationServiceTest.java

@@ -97,25 +97,6 @@ public class SettingsMigrationServiceTest {
         assertThat(migrationService.returnedValues, contains(true, false));
     }
 
-    @Test
-    public void shouldKeepOldForceCommandSettings() throws IOException {
-        // given
-        File dataFolder = temporaryFolder.newFolder();
-        File configFile = new File(dataFolder, "config.yml");
-        Files.copy(getJarFile(OLD_CONFIG_FILE), configFile);
-        PropertyResource resource = new YamlFileResource(configFile);
-        SettingsMigrationService migrationService = new SettingsMigrationService(dataFolder);
-
-        // when
-        migrationService.performMigrations(resource, AuthMeSettingsRetriever.buildConfigurationData().getProperties());
-
-        // then
-        assertThat(migrationService.getOnLoginCommands(), contains("spawn"));
-        assertThat(migrationService.getOnLoginConsoleCommands(), contains("sethome %p:lastloc", "msg %p Welcome back"));
-        assertThat(migrationService.getOnRegisterCommands(), contains("me registers", "msg CONSOLE hi"));
-        assertThat(migrationService.getOnRegisterConsoleCommands(), contains("sethome %p:regloc"));
-    }
-
     private void verifyHasUpToDateSettings(Settings settings, File dataFolder) throws IOException {
         assertThat(settings.getProperty(ALLOWED_NICKNAME_CHARACTERS), equalTo(ALLOWED_NICKNAME_CHARACTERS.getDefaultValue()));
         assertThat(settings.getProperty(DELAY_JOIN_MESSAGE), equalTo(true));

src/test/java/fr/xephi/authme/settings/commandconfig/CommandConfigTestHelper.java

@@ -1,35 +0,0 @@
-package fr.xephi.authme.settings.commandconfig;
-
-import org.hamcrest.Description;
-import org.hamcrest.Matcher;
-import org.hamcrest.TypeSafeMatcher;
-
-/**
- * Helper class for tests around the command configuration.
- */
-final class CommandConfigTestHelper {
-
-    private CommandConfigTestHelper() {
-    }
-
-    /**
-     * Returns a matcher for verifying a {@link Command} object.
-     *
-     * @param cmd the expected command line
-     * @param executor the expected executor
-     * @return the matcher
-     */
-    static Matcher<Command> isCommand(String cmd, Executor executor) {
-        return new TypeSafeMatcher<Command>() {
-            @Override
-            protected boolean matchesSafely(Command item) {
-                return executor.equals(item.getExecutor()) && cmd.equals(item.getCommand());
-            }
-
-            @Override
-            public void describeTo(Description description) {
-                description.appendText("Command '" + cmd + "' run by '" + executor + "'");
-            }
-        };
-    }
-}

src/test/java/fr/xephi/authme/settings/commandconfig/CommandMigrationServiceTest.java

@@ -6,34 +6,17 @@ import ch.jalu.configme.configurationdata.ConfigurationDataBuilder;
 import ch.jalu.configme.resource.PropertyResource;
 import ch.jalu.configme.resource.YamlFileResource;
 import fr.xephi.authme.TestHelper;
-import fr.xephi.authme.settings.SettingsMigrationService;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.InjectMocks;
-import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnitRunner;
 
 import java.io.File;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.LinkedHashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
 
-import static fr.xephi.authme.settings.commandconfig.CommandConfigTestHelper.isCommand;
-import static java.util.Collections.emptyList;
-import static org.hamcrest.Matchers.anEmptyMap;
-import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.equalTo;
-import static org.hamcrest.Matchers.sameInstance;
 import static org.junit.Assert.assertThat;
-import static org.mockito.BDDMockito.given;
-import static org.mockito.Mockito.spy;
-import static org.mockito.Mockito.verifyZeroInteractions;
 
 /**
  * Test for {@link CommandMigrationService}.
@@ -44,81 +27,11 @@ public class CommandMigrationServiceTest {
     @InjectMocks
     private CommandMigrationService commandMigrationService;
 
-    @Mock
-    private SettingsMigrationService settingsMigrationService;
-
-    private CommandConfig commandConfig = new CommandConfig();
-
     @BeforeClass
     public static void setUpLogger() {
         TestHelper.setupLogger();
     }
 
-    @Test
-    public void shouldNotPerformAnyMigration() {
-        // given
-        given(settingsMigrationService.getOnLoginCommands()).willReturn(emptyList());
-        given(settingsMigrationService.getOnLoginConsoleCommands()).willReturn(emptyList());
-        given(settingsMigrationService.getOnRegisterCommands()).willReturn(emptyList());
-        given(settingsMigrationService.getOnRegisterConsoleCommands()).willReturn(emptyList());
-        commandConfig.getOnRegister().put("existing", new Command("existing cmd", Executor.PLAYER));
-        CommandConfig configSpy = spy(commandConfig);
-
-        // when
-        boolean result = commandMigrationService.transformOldCommands(configSpy);
-
-        // then
-        assertThat(result, equalTo(false));
-        verifyZeroInteractions(configSpy);
-        assertThat(configSpy.getOnRegister().keySet(), contains("existing"));
-        assertThat(configSpy.getOnLogin(), anEmptyMap());
-    }
-
-    @Test
-    @SuppressWarnings("unchecked")
-    public void shouldPerformMigration() {
-        // given
-        List<String> onLogin = Collections.singletonList("on login command");
-        given(settingsMigrationService.getOnLoginCommands()).willReturn(onLogin);
-        List<String> onLoginConsole = Arrays.asList("cmd1", "cmd2 %p", "cmd3");
-        given(settingsMigrationService.getOnLoginConsoleCommands()).willReturn(onLoginConsole);
-        given(settingsMigrationService.getOnRegisterCommands()).willReturn(emptyList());
-        List<String> onRegisterConsole = Arrays.asList("log %p registered", "whois %p");
-        given(settingsMigrationService.getOnRegisterConsoleCommands()).willReturn(onRegisterConsole);
-
-        Map<String, OnLoginCommand> onLoginCommands = new LinkedHashMap<>();
-        OnLoginCommand existingCommand = new OnLoginCommand("helpop %p has many alts", Executor.CONSOLE);
-        existingCommand.setIfNumberOfAccountsAtLeast(Optional.of(2));
-        onLoginCommands.put("alert_on_alts", existingCommand);
-        commandConfig.setOnLogin(onLoginCommands);
-        Map<String, Command> onRegisterCommands = new LinkedHashMap<>();
-        onRegisterCommands.put("ex_cmd", new Command("existing", Executor.CONSOLE));
-        onRegisterCommands.put("ex_cmd2", new Command("existing2", Executor.PLAYER));
-        commandConfig.setOnRegister(onRegisterCommands);
-
-        // when
-        boolean result = commandMigrationService.transformOldCommands(commandConfig);
-
-        // then
-        assertThat(result, equalTo(true));
-        assertThat(commandConfig.getOnLogin(), sameInstance(onLoginCommands));
-        Collection<OnLoginCommand> loginCmdList = onLoginCommands.values();
-        assertThat(loginCmdList, contains(
-            equalTo(existingCommand),
-            isCommand("on login command", Executor.PLAYER),
-            isCommand("cmd1", Executor.CONSOLE),
-            isCommand("cmd2 %p", Executor.CONSOLE),
-            isCommand("cmd3", Executor.CONSOLE)));
-
-        assertThat(commandConfig.getOnRegister(), sameInstance(onRegisterCommands));
-        Collection<Command> registerCmdList = onRegisterCommands.values();
-        assertThat(registerCmdList, contains(
-            isCommand("existing", Executor.CONSOLE),
-            isCommand("existing2", Executor.PLAYER),
-            isCommand("log %p registered", Executor.CONSOLE),
-            isCommand("whois %p", Executor.CONSOLE)));
-    }
-
     @Test
     public void shouldRewriteForEmptyFile() {
         // given

src/test/java/fr/xephi/authme/settings/commandconfig/CommandYmlConsistencyTest.java

@@ -4,22 +4,17 @@ import ch.jalu.configme.configurationdata.ConfigurationDataBuilder;
 import ch.jalu.configme.resource.PropertyResource;
 import ch.jalu.configme.resource.YamlFileResource;
 import fr.xephi.authme.TestHelper;
-import fr.xephi.authme.settings.SettingsMigrationService;
-import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
 import org.junit.runner.RunWith;
 import org.mockito.InjectMocks;
-import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnitRunner;
 
 import java.io.File;
-import java.util.Collections;
 
 import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertThat;
-import static org.mockito.BDDMockito.given;
 
 /**
  * Tests that commands.yml is well-formed.
@@ -30,20 +25,9 @@ public class CommandYmlConsistencyTest {
     @InjectMocks
     private CommandMigrationService commandMigrationService;
 
-    @Mock
-    private SettingsMigrationService settingsMigrationService;
-
     @Rule
     public TemporaryFolder temporaryFolder = new TemporaryFolder();
 
-    @Before
-    public void setUpSettingsMigrationService() {
-        given(settingsMigrationService.getOnLoginCommands()).willReturn(Collections.emptyList());
-        given(settingsMigrationService.getOnLoginConsoleCommands()).willReturn(Collections.emptyList());
-        given(settingsMigrationService.getOnRegisterCommands()).willReturn(Collections.emptyList());
-        given(settingsMigrationService.getOnRegisterConsoleCommands()).willReturn(Collections.emptyList());
-    }
-
     @Test
     public void shouldLoadWithNoMigrations() {
         // given