#347 Add NewSetting to command service

- Adjust NewSetting constructor to match needs in AuthMe
- Add NewSetting to the command service
- See CaptchaCommand for a sample replacement from Settings to NewSetting

src/main/java/fr/xephi/authme/AuthMe.java

@@ -48,6 +48,7 @@ import fr.xephi.authme.security.crypts.HashedPassword;
 import fr.xephi.authme.settings.OtherAccounts;
 import fr.xephi.authme.settings.Settings;
 import fr.xephi.authme.settings.Spawn;
+import fr.xephi.authme.settings.custom.NewSetting;
 import fr.xephi.authme.util.GeoLiteAPI;
 import fr.xephi.authme.util.StringUtils;
 import fr.xephi.authme.util.Utils;
@@ -67,6 +68,7 @@ import org.bukkit.scheduler.BukkitTask;
 import org.mcstats.Metrics;
 import org.mcstats.Metrics.Graph;
 
+import java.io.File;
 import java.io.IOException;
 import java.net.URL;
 import java.util.Calendar;
@@ -97,6 +99,7 @@ public class AuthMe extends JavaPlugin {
     private CommandHandler commandHandler = null;
     private PermissionsManager permsMan = null;
     private Settings settings;
+    private NewSetting newSettings;
     private Messages messages;
     private JsonCache playerBackup;
     private ModuleManager moduleManager;
@@ -213,6 +216,7 @@ public class AuthMe extends JavaPlugin {
             setEnabled(false);
             return;
         }
+        newSettings = createNewSetting();
 
         // Set up messages & password security
         messages = Messages.getInstance();
@@ -233,7 +237,7 @@ public class AuthMe extends JavaPlugin {
 
         // Set up the permissions manager and command handler
         permsMan = initializePermissionsManager();
-        commandHandler = initializeCommandHandler(permsMan, messages, passwordSecurity);
+        commandHandler = initializeCommandHandler(permsMan, messages, passwordSecurity, newSettings);
 
         // Set up the module manager
         setupModuleManager();
@@ -415,11 +419,12 @@ public class AuthMe extends JavaPlugin {
     }
 
     private CommandHandler initializeCommandHandler(PermissionsManager permissionsManager, Messages messages,
-                                                    PasswordSecurity passwordSecurity) {
+                                                    PasswordSecurity passwordSecurity, NewSetting settings) {
         HelpProvider helpProvider = new HelpProvider(permissionsManager);
         Set<CommandDescription> baseCommands = CommandInitializer.buildCommands();
         CommandMapper mapper = new CommandMapper(baseCommands, messages, permissionsManager, helpProvider);
-        CommandService commandService = new CommandService(this, mapper, helpProvider, messages, passwordSecurity);
+        CommandService commandService = new CommandService(
+            this, mapper, helpProvider, messages, passwordSecurity, settings);
         return new CommandHandler(commandService);
     }
 
@@ -441,19 +446,24 @@ public class AuthMe extends JavaPlugin {
      * @return True on success, false on failure.
      */
     private boolean loadSettings() {
-        // TODO: new configuration style (more files)
         try {
             settings = new Settings(this);
             Settings.reload();
         } catch (Exception e) {
             ConsoleLogger.writeStackTrace(e);
-            ConsoleLogger.showError("Can't load the configuration file... Something went wrong, to avoid security issues the server will shutdown!");
+            ConsoleLogger.showError("Can't load the configuration file... Something went wrong. "
+                + "To avoid security issues the server will shut down!");
             server.shutdown();
             return true;
         }
         return false;
     }
 
+    private NewSetting createNewSetting() {
+        File configFile = new File(getDataFolder() + "config.yml");
+        return new NewSetting(getConfig(), configFile);
+    }
+
     /**
      * Set up the console filter.
      */

src/main/java/fr/xephi/authme/command/CommandService.java

@@ -2,6 +2,8 @@ package fr.xephi.authme.command;
 
 import java.util.List;
 
+import fr.xephi.authme.settings.custom.NewSetting;
+import fr.xephi.authme.settings.domain.Property;
 import org.bukkit.command.CommandSender;
 
 import fr.xephi.authme.AuthMe;
@@ -24,6 +26,7 @@ public class CommandService {
     private final HelpProvider helpProvider;
     private final CommandMapper commandMapper;
     private final PasswordSecurity passwordSecurity;
+    private final NewSetting settings;
 
     /**
      * Constructor.
@@ -35,12 +38,13 @@ public class CommandService {
      * @param passwordSecurity The Password Security instance
      */
     public CommandService(AuthMe authMe, CommandMapper commandMapper, HelpProvider helpProvider, Messages messages,
-                          PasswordSecurity passwordSecurity) {
+                          PasswordSecurity passwordSecurity, NewSetting settings) {
         this.authMe = authMe;
         this.messages = messages;
         this.helpProvider = helpProvider;
         this.commandMapper = commandMapper;
         this.passwordSecurity = passwordSecurity;
+        this.settings = settings;
     }
 
     /**
@@ -156,4 +160,15 @@ public class CommandService {
         return messages.retrieve(key);
     }
 
+    /**
+     * Retrieve the given property's value.
+     *
+     * @param property The property to retrieve
+     * @param <T> The type of the property
+     * @return The property's value
+     */
+    public <T> T getProperty(Property<T> property) {
+        return settings.getProperty(property);
+    }
+
 }

src/main/java/fr/xephi/authme/command/executable/captcha/CaptchaCommand.java

@@ -6,7 +6,7 @@ import fr.xephi.authme.command.CommandService;
 import fr.xephi.authme.command.PlayerCommand;
 import fr.xephi.authme.output.MessageKey;
 import fr.xephi.authme.security.RandomString;
-import fr.xephi.authme.settings.Settings;
+import fr.xephi.authme.settings.custom.SecuritySettings;
 import fr.xephi.authme.util.Wrapper;
 import org.bukkit.entity.Player;
 
@@ -28,20 +28,20 @@ public class CaptchaCommand extends PlayerCommand {
             return;
         }
 
-        if (!Settings.useCaptcha) {
+        if (!commandService.getProperty(SecuritySettings.USE_CAPTCHA)) {
             commandService.send(player, MessageKey.USAGE_LOGIN);
             return;
         }
 
-
         if (!plugin.cap.containsKey(playerNameLowerCase)) {
             commandService.send(player, MessageKey.USAGE_LOGIN);
             return;
         }
 
-        if (Settings.useCaptcha && !captcha.equals(plugin.cap.get(playerNameLowerCase))) {
+        if (!captcha.equals(plugin.cap.get(playerNameLowerCase))) {
             plugin.cap.remove(playerNameLowerCase);
-            String randStr = RandomString.generate(Settings.captchaLength);
+            int captchaLength = commandService.getProperty(SecuritySettings.CAPTCHA_LENGTH);
+            String randStr = RandomString.generate(captchaLength);
             plugin.cap.put(playerNameLowerCase, randStr);
             commandService.send(player, MessageKey.CAPTCHA_WRONG_ERROR, plugin.cap.get(playerNameLowerCase));
             return;

src/main/java/fr/xephi/authme/command/executable/register/RegisterCommand.java

@@ -43,6 +43,6 @@ public class RegisterCommand extends PlayerCommand {
 
     @Override
     public String getAlternativeCommand() {
-        return "authme register <playername> <password>";
+        return "/authme register <playername> <password>";
     }
 }

src/main/java/fr/xephi/authme/settings/custom/NewSetting.java

@@ -6,7 +6,7 @@ import fr.xephi.authme.settings.domain.Property;
 import fr.xephi.authme.settings.propertymap.PropertyMap;
 import fr.xephi.authme.util.CollectionUtils;
 import fr.xephi.authme.util.StringUtils;
-import org.bukkit.configuration.file.YamlConfiguration;
+import org.bukkit.configuration.file.FileConfiguration;
 
 import java.io.File;
 import java.io.FileWriter;
@@ -22,41 +22,43 @@ import java.util.Map;
 public class NewSetting {
 
     private File file;
-    private YamlConfiguration configuration;
+    private FileConfiguration configuration;
 
     /**
      * Constructor.
      * Loads the file as YAML and checks its integrity.
      *
+     * @param configuration The configuration to interact with
      * @param file The configuration file
      */
-    public NewSetting(File file) {
+    public NewSetting(FileConfiguration configuration, File file) {
-        this.configuration = YamlConfiguration.loadConfiguration(file);
+        this.configuration = configuration;
         this.file = file;
 
-        PropertyMap propertyMap = SettingsFieldRetriever.getAllPropertyFields();
+        // TODO ljacqu 20160109: Ensure that save() works as desired (i.e. that it always produces valid YAML)
-        if (!containsAllSettings(propertyMap)) {
+        // and then uncomment the lines below. Once this is uncommented, the checks in the old Settings.java should
-            save(propertyMap);
+        // be removed as we should check to rewrite the config.yml file only at one place
-        }
+        // --------
+        // PropertyMap propertyMap = SettingsFieldRetriever.getAllPropertyFields();
+        // if (!containsAllSettings(propertyMap)) {
+        //     save(propertyMap);
+        // }
     }
 
     /**
-     * Simple constructor for testing purposes. Does not check for all properties and
+     * Constructor for testing purposes, allowing more options.
-     * never saves to the file.
      *
-     * @param yamlConfiguration The YamlConfiguration object to use
+     * @param configuration The FileConfiguration object to use
      * @param file The file to write to
      * @param propertyMap The property map whose properties should be verified for presence, or null to skip this
      */
     @VisibleForTesting
-    NewSetting(YamlConfiguration yamlConfiguration, File file, PropertyMap propertyMap) {
+    NewSetting(FileConfiguration configuration, File file, PropertyMap propertyMap) {
-        this.configuration = yamlConfiguration;
+        this.configuration = configuration;
         this.file = file;
 
-        if (propertyMap != null) {
+        if (propertyMap != null && !containsAllSettings(propertyMap)) {
-            if (!containsAllSettings(propertyMap)) {
+            save(propertyMap);
-                save(propertyMap);
-            }
         }
     }
 
@@ -67,7 +69,7 @@ public class NewSetting {
      * @param <T> The property's type
      * @return The property's value
      */
-    public <T> T getOption(Property<T> property) {
+    public <T> T getProperty(Property<T> property) {
         return property.getFromFile(configuration);
     }
 

src/main/java/fr/xephi/authme/settings/domain/EnumPropertyType.java

@@ -1,6 +1,6 @@
 package fr.xephi.authme.settings.domain;
 
-import org.bukkit.configuration.file.YamlConfiguration;
+import org.bukkit.configuration.file.FileConfiguration;
 
 import java.util.List;
 
@@ -19,7 +19,7 @@ class EnumPropertyType<E extends Enum<E>> extends PropertyType<E> {
     }
 
     @Override
-    public E getFromFile(Property<E> property, YamlConfiguration configuration) {
+    public E getFromFile(Property<E> property, FileConfiguration configuration) {
         String textValue = configuration.getString(property.getPath());
         if (textValue == null) {
             return property.getDefaultValue();
@@ -34,7 +34,7 @@ class EnumPropertyType<E extends Enum<E>> extends PropertyType<E> {
     }
 
     @Override
-    public boolean contains(Property<E> property, YamlConfiguration configuration) {
+    public boolean contains(Property<E> property, FileConfiguration configuration) {
         return super.contains(property, configuration)
             && mapToEnum(configuration.getString(property.getPath())) != null;
     }

src/main/java/fr/xephi/authme/settings/domain/Property.java

@@ -1,6 +1,6 @@
 package fr.xephi.authme.settings.domain;
 
-import org.bukkit.configuration.file.YamlConfiguration;
+import org.bukkit.configuration.file.FileConfiguration;
 
 import java.util.Arrays;
 import java.util.List;
@@ -60,7 +60,7 @@ public class Property<T> {
      * @param configuration The configuration to read the value from
      * @return The value, or default if not present
      */
-    public T getFromFile(YamlConfiguration configuration) {
+    public T getFromFile(FileConfiguration configuration) {
         return type.getFromFile(this, configuration);
     }
 
@@ -70,7 +70,7 @@ public class Property<T> {
      * @param configuration The configuration to read the value from
      * @return The property value as YAML
      */
-    public List<String> formatValueAsYaml(YamlConfiguration configuration) {
+    public List<String> formatValueAsYaml(FileConfiguration configuration) {
         return type.asYaml(this, configuration);
     }
 
@@ -80,7 +80,7 @@ public class Property<T> {
      * @param configuration The configuration file to verify
      * @return True if the property is present, false otherwise
      */
-    public boolean isPresent(YamlConfiguration configuration) {
+    public boolean isPresent(FileConfiguration configuration) {
         return type.contains(this, configuration);
     }
 

src/main/java/fr/xephi/authme/settings/domain/PropertyType.java

@@ -1,6 +1,6 @@
 package fr.xephi.authme.settings.domain;
 
-import org.bukkit.configuration.file.YamlConfiguration;
+import org.bukkit.configuration.file.FileConfiguration;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -28,7 +28,7 @@ public abstract class PropertyType<T> {
      * @param configuration The YAML configuration to read from
      * @return The read value, or the default value if absent
      */
-    public abstract T getFromFile(Property<T> property, YamlConfiguration configuration);
+    public abstract T getFromFile(Property<T> property, FileConfiguration configuration);
 
     /**
      * Return the property's value (or its default) as YAML.
@@ -37,7 +37,7 @@ public abstract class PropertyType<T> {
      * @param configuration The YAML configuration to read from
      * @return The read value or its default in YAML format
      */
-    public List<String> asYaml(Property<T> property, YamlConfiguration configuration) {
+    public List<String> asYaml(Property<T> property, FileConfiguration configuration) {
         return asYaml(getFromFile(property, configuration));
     }
 
@@ -48,7 +48,7 @@ public abstract class PropertyType<T> {
      * @param configuration The configuration to verify
      * @return True if the property is present, false otherwise
      */
-    public boolean contains(Property<T> property, YamlConfiguration configuration) {
+    public boolean contains(Property<T> property, FileConfiguration configuration) {
         return configuration.contains(property.getPath());
     }
 
@@ -66,7 +66,7 @@ public abstract class PropertyType<T> {
      */
     private static final class BooleanProperty extends PropertyType<Boolean> {
         @Override
-        public Boolean getFromFile(Property<Boolean> property, YamlConfiguration configuration) {
+        public Boolean getFromFile(Property<Boolean> property, FileConfiguration configuration) {
             return configuration.getBoolean(property.getPath(), property.getDefaultValue());
         }
 
@@ -81,7 +81,7 @@ public abstract class PropertyType<T> {
      */
     private static final class DoubleProperty extends PropertyType<Double> {
         @Override
-        public Double getFromFile(Property<Double> property, YamlConfiguration configuration) {
+        public Double getFromFile(Property<Double> property, FileConfiguration configuration) {
             return configuration.getDouble(property.getPath(), property.getDefaultValue());
         }
 
@@ -96,7 +96,7 @@ public abstract class PropertyType<T> {
      */
     private static final class IntegerProperty extends PropertyType<Integer> {
         @Override
-        public Integer getFromFile(Property<Integer> property, YamlConfiguration configuration) {
+        public Integer getFromFile(Property<Integer> property, FileConfiguration configuration) {
             return configuration.getInt(property.getPath(), property.getDefaultValue());
         }
 
@@ -111,7 +111,7 @@ public abstract class PropertyType<T> {
      */
     private static final class StringProperty extends PropertyType<String> {
         @Override
-        public String getFromFile(Property<String> property, YamlConfiguration configuration) {
+        public String getFromFile(Property<String> property, FileConfiguration configuration) {
             return configuration.getString(property.getPath(), property.getDefaultValue());
         }
 
@@ -131,7 +131,7 @@ public abstract class PropertyType<T> {
      */
     private static final class StringListProperty extends PropertyType<List<String>> {
         @Override
-        public List<String> getFromFile(Property<List<String>> property, YamlConfiguration configuration) {
+        public List<String> getFromFile(Property<List<String>> property, FileConfiguration configuration) {
             if (!configuration.isList(property.getPath())) {
                 return property.getDefaultValue();
             }
@@ -154,7 +154,7 @@ public abstract class PropertyType<T> {
         }
 
         @Override
-        public boolean contains(Property<List<String>> property, YamlConfiguration configuration) {
+        public boolean contains(Property<List<String>> property, FileConfiguration configuration) {
             return configuration.contains(property.getPath()) && configuration.isList(property.getPath());
         }
     }

src/test/java/fr/xephi/authme/command/CommandServiceTest.java

@@ -8,6 +8,9 @@ import fr.xephi.authme.output.Messages;
 import fr.xephi.authme.permission.PermissionsManager;
 import fr.xephi.authme.process.Management;
 import fr.xephi.authme.security.PasswordSecurity;
+import fr.xephi.authme.settings.custom.NewSetting;
+import fr.xephi.authme.settings.custom.SecuritySettings;
+import fr.xephi.authme.settings.domain.Property;
 import org.bukkit.command.CommandSender;
 import org.bukkit.entity.Player;
 import org.junit.Before;
@@ -36,6 +39,7 @@ public class CommandServiceTest {
     private Messages messages;
     private PasswordSecurity passwordSecurity;
     private CommandService commandService;
+    private NewSetting settings;
 
     @Before
     public void setUpService() {
@@ -44,7 +48,8 @@ public class CommandServiceTest {
         helpProvider = mock(HelpProvider.class);
         messages = mock(Messages.class);
         passwordSecurity = mock(PasswordSecurity.class);
-        commandService = new CommandService(authMe, commandMapper, helpProvider, messages, passwordSecurity);
+        settings = mock(NewSetting.class);
+        commandService = new CommandService(authMe, commandMapper, helpProvider, messages, passwordSecurity, settings);
     }
 
     @Test
@@ -195,4 +200,18 @@ public class CommandServiceTest {
         assertThat(result, equalTo(givenMessages));
         verify(messages).retrieve(key);
     }
+
+    @Test
+    public void shouldRetrieveProperty() {
+        // given
+        Property<Integer> property = SecuritySettings.CAPTCHA_LENGTH;
+        given(settings.getProperty(property)).willReturn(7);
+
+        // when
+        int result = settings.getProperty(property);
+
+        // then
+        assertThat(result, equalTo(7));
+        verify(settings).getProperty(property);
+    }
 }

src/test/java/fr/xephi/authme/command/executable/captcha/CaptchaCommandTest.java

@@ -5,7 +5,7 @@ import fr.xephi.authme.command.CommandService;
 import fr.xephi.authme.command.ExecutableCommand;
 import fr.xephi.authme.output.MessageKey;
 import fr.xephi.authme.output.Messages;
-import fr.xephi.authme.settings.Settings;
+import fr.xephi.authme.settings.custom.SecuritySettings;
 import fr.xephi.authme.util.WrapperMock;
 import org.bukkit.command.BlockCommandSender;
 import org.bukkit.command.CommandSender;
@@ -20,6 +20,7 @@ import java.util.Collections;
 
 import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertThat;
+import static org.mockito.BDDMockito.given;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -35,8 +36,8 @@ public class CaptchaCommandTest {
     @Before
     public void setUpWrapperMock() {
         wrapperMock = WrapperMock.createInstance();
-        Settings.useCaptcha = true;
         commandService = mock(CommandService.class);
+        given(commandService.getProperty(SecuritySettings.USE_CAPTCHA)).willReturn(true);
     }
 
     @Test
@@ -46,7 +47,7 @@ public class CaptchaCommandTest {
         ExecutableCommand command = new CaptchaCommand();
 
         // when
-        command.executeCommand(sender, new ArrayList<String>(), mock(CommandService.class));
+        command.executeCommand(sender, new ArrayList<String>(), commandService);
 
         // then
         assertThat(wrapperMock.wasMockCalled(AuthMe.class), equalTo(false));
@@ -54,14 +55,14 @@ public class CaptchaCommandTest {
     }
 
     @Test
-    @Ignore
     public void shouldRejectIfCaptchaIsNotUsed() {
         // given
         Player player = mockPlayerWithName("testplayer");
         ExecutableCommand command = new CaptchaCommand();
+        given(commandService.getProperty(SecuritySettings.USE_CAPTCHA)).willReturn(false);
 
         // when
-        command.executeCommand(player, Collections.singletonList("1234"), mock(CommandService.class));
+        command.executeCommand(player, Collections.singletonList("1234"), commandService);
 
         // then
         verify(commandService).send(player, MessageKey.USAGE_LOGIN);

src/test/java/fr/xephi/authme/settings/custom/NewSettingIntegrationTest.java

@@ -67,7 +67,7 @@ public class NewSettingIntegrationTest {
             .build();
         for (Map.Entry<Property<?>, Object> entry : expectedValues.entrySet()) {
             assertThat("Property '" + entry.getKey().getPath() + "' has expected value",
-                settings.getOption(entry.getKey()), equalTo(entry.getValue()));
+                settings.getProperty(entry.getKey()), equalTo(entry.getValue()));
         }
         assertThat(file.exists(), equalTo(false));
     }
@@ -100,7 +100,7 @@ public class NewSettingIntegrationTest {
             .build();
         for (Map.Entry<Property<?>, Object> entry : expectedValues.entrySet()) {
             assertThat("Property '" + entry.getKey().getPath() + "' has expected value",
-                settings.getOption(entry.getKey()), equalTo(entry.getValue()));
+                settings.getProperty(entry.getKey()), equalTo(entry.getValue()));
         }
     }
 

src/test/java/fr/xephi/authme/settings/custom/NewSettingTest.java

@@ -41,10 +41,10 @@ public class NewSettingTest {
         // when / then
         NewSetting settings = new NewSetting(file, new File("conf.txt"), null);
 
-        assertThat(settings.getOption(TestConfiguration.VERSION_NUMBER), equalTo(20));
+        assertThat(settings.getProperty(TestConfiguration.VERSION_NUMBER), equalTo(20));
-        assertThat(settings.getOption(TestConfiguration.SKIP_BORING_FEATURES), equalTo(true));
+        assertThat(settings.getProperty(TestConfiguration.SKIP_BORING_FEATURES), equalTo(true));
-        assertThat(settings.getOption(TestConfiguration.RATIO_LIMIT), equalTo(4.25));
+        assertThat(settings.getProperty(TestConfiguration.RATIO_LIMIT), equalTo(4.25));
-        assertThat(settings.getOption(TestConfiguration.SYSTEM_NAME), equalTo("myTestSys"));
+        assertThat(settings.getProperty(TestConfiguration.SYSTEM_NAME), equalTo("myTestSys"));
 
         assertDefaultValue(TestConfiguration.DURATION_IN_SECONDS, settings);
         assertDefaultValue(TestConfiguration.DUST_LEVEL, settings);
@@ -68,7 +68,7 @@ public class NewSettingTest {
 
     private static void assertDefaultValue(Property<?> property, NewSetting setting) {
         assertThat(property.getPath() + " has default value",
-            setting.getOption(property).equals(property.getDefaultValue()), equalTo(true));
+            setting.getProperty(property).equals(property.getDefaultValue()), equalTo(true));
     }
 
     private static <T> Answer<T> withDefaultArgument() {