#603 Pass settings migration service as constructor parameter (work in progress)

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

@@ -44,6 +44,7 @@ import fr.xephi.authme.security.PasswordSecurity;
 import fr.xephi.authme.security.crypts.SHA256;
 import fr.xephi.authme.settings.NewSetting;
 import fr.xephi.authme.settings.Settings;
+import fr.xephi.authme.settings.SettingsMigrationService;
 import fr.xephi.authme.settings.SpawnLoader;
 import fr.xephi.authme.settings.properties.DatabaseSettings;
 import fr.xephi.authme.settings.properties.EmailSettings;
@@ -467,8 +468,9 @@ public class AuthMe extends JavaPlugin {
     private NewSetting createNewSetting() {
         File configFile = new File(getDataFolder(), "config.yml");
         PropertyMap properties = SettingsFieldRetriever.getAllPropertyFields();
+        SettingsMigrationService migrationService = new SettingsMigrationService();
         return FileUtils.copyFileFromResource(configFile, "config.yml")
-            ? new NewSetting(configFile, getDataFolder(), properties)
+            ? new NewSetting(configFile, getDataFolder(), properties, migrationService)
             : null;
     }
 

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

@@ -33,6 +33,7 @@ public class NewSetting {
     private final File pluginFolder;
     private final File configFile;
     private final PropertyMap propertyMap;
+    private final SettingsMigrationService migrationService;
     private FileConfiguration configuration;
     /** The file with the localized messages based on {@link PluginSettings#MESSAGES_LANGUAGE}. */
     private File messagesFile;
@@ -44,12 +45,16 @@ public class NewSetting {
      *
      * @param configFile The configuration file
      * @param pluginFolder The AuthMe plugin folder
+     * @param propertyMap Collection of all available settings
+     * @param migrationService Migration service to check the settings file with
      */
-    public NewSetting(File configFile, File pluginFolder, PropertyMap propertyMap) {
+    public NewSetting(File configFile, File pluginFolder, PropertyMap propertyMap,
+                      SettingsMigrationService migrationService) {
         this.configuration = YamlConfiguration.loadConfiguration(configFile);
         this.configFile = configFile;
         this.pluginFolder = pluginFolder;
         this.propertyMap = propertyMap;
+        this.migrationService = migrationService;
         validateAndLoadOptions();
     }
 
@@ -59,15 +64,18 @@ public class NewSetting {
      * @param configuration The FileConfiguration object to use
      * @param configFile The file to write to
      * @param propertyMap The property map whose properties should be verified for presence, or null to skip this
+     * @param migrationService Migration service, or null to skip migration checks
      */
     @VisibleForTesting
-    NewSetting(FileConfiguration configuration, File configFile, PropertyMap propertyMap) {
+    NewSetting(FileConfiguration configuration, File configFile, PropertyMap propertyMap,
+               SettingsMigrationService migrationService) {
         this.configuration = configuration;
         this.configFile = configFile;
         this.pluginFolder = new File("");
         this.propertyMap = propertyMap;
+        this.migrationService = migrationService;
 
-        if (propertyMap != null) {
+        if (propertyMap != null && migrationService != null) {
             validateAndLoadOptions();
         }
     }
@@ -184,7 +192,7 @@ public class NewSetting {
     }
 
     private void validateAndLoadOptions() {
-        if (SettingsMigrationService.checkAndMigrate(configuration, propertyMap, pluginFolder)) {
+        if (migrationService.checkAndMigrate(configuration, propertyMap, pluginFolder)) {
             ConsoleLogger.info("Merged new config options");
             ConsoleLogger.info("Please check your config.yml file for new settings!");
             save();

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

@@ -1,6 +1,5 @@
 package fr.xephi.authme.settings;
 
-import com.google.common.annotations.VisibleForTesting;
 import fr.xephi.authme.ConsoleLogger;
 import fr.xephi.authme.settings.domain.Property;
 import fr.xephi.authme.settings.propertymap.PropertyMap;
@@ -18,25 +17,23 @@ import static fr.xephi.authme.settings.properties.RestrictionSettings.ALLOWED_NI
 /**
  * Service for verifying that the configuration is up-to-date.
  */
-public final class SettingsMigrationService {
+public class SettingsMigrationService {
-
-    private SettingsMigrationService() {
-    }
 
     /**
-     * Checks the config file and does any necessary migrations.
+     * Checks the config file and performs any necessary migrations.
      *
      * @param configuration The file configuration to check and migrate
      * @param propertyMap The property map of all existing properties
      * @param pluginFolder The plugin folder
      * @return True if there is a change and the config must be saved, false if the config is up-to-date
      */
-    public static boolean checkAndMigrate(FileConfiguration configuration, PropertyMap propertyMap, File pluginFolder) {
+    public boolean checkAndMigrate(FileConfiguration configuration, PropertyMap propertyMap, File pluginFolder) {
-        return performMigrations(configuration, pluginFolder) || hasDeprecatedProperties(configuration)
+        return performMigrations(configuration, pluginFolder)
+            || hasDeprecatedProperties(configuration)
             || !containsAllSettings(configuration, propertyMap);
     }
 
-    private static boolean performMigrations(FileConfiguration configuration, File pluginFolder) {
+    private boolean performMigrations(FileConfiguration configuration, File pluginFolder) {
         boolean changes = false;
         if ("[a-zA-Z0-9_?]*".equals(configuration.getString(ALLOWED_NICKNAME_CHARACTERS.getPath()))) {
             configuration.set(ALLOWED_NICKNAME_CHARACTERS.getPath(), "[a-zA-Z0-9_]*");
@@ -50,8 +47,7 @@ public final class SettingsMigrationService {
             | migrateJoinLeaveMessages(configuration);
     }
 
-    @VisibleForTesting
+    public boolean containsAllSettings(FileConfiguration configuration, PropertyMap propertyMap) {
-    static boolean containsAllSettings(FileConfiguration configuration, PropertyMap propertyMap) {
         for (Property<?> property : propertyMap.keySet()) {
             if (!property.isPresent(configuration)) {
                 return false;
@@ -80,16 +76,16 @@ public final class SettingsMigrationService {
      * Check if {@code Email.mailText} is present and move it to the Email.html file if it doesn't exist yet.
      *
      * @param configuration The file configuration to verify
-     * @param dataFolder The plugin data folder
+     * @param pluginFolder The plugin data folder
      * @return True if a migration has been completed, false otherwise
      */
-    private static boolean performMailTextToFileMigration(FileConfiguration configuration, File dataFolder) {
+    private static boolean performMailTextToFileMigration(FileConfiguration configuration, File pluginFolder) {
         final String oldSettingPath = "Email.mailText";
         if (!configuration.contains(oldSettingPath)) {
             return false;
         }
 
-        final File emailFile = new File(dataFolder, "email.html");
+        final File emailFile = new File(pluginFolder, "email.html");
         final String mailText = configuration.getString(oldSettingPath)
             .replace("<playername>", "<playername />")
             .replace("<servername>", "<servername />")

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

@@ -35,10 +35,10 @@ public class ConfigFileConsistencyTest {
         // given
         File configFile = TestHelper.getJarFile(CONFIG_FILE);
         FileConfiguration configuration = YamlConfiguration.loadConfiguration(configFile);
+        SettingsMigrationService migration = new SettingsMigrationService();
 
         // when
-        boolean result = SettingsMigrationService.containsAllSettings(
+        boolean result = migration.containsAllSettings(configuration, SettingsFieldRetriever.getAllPropertyFields());
-            configuration, SettingsFieldRetriever.getAllPropertyFields());
 
         // then
         if (!result) {

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

@@ -8,9 +8,9 @@ import fr.xephi.authme.settings.domain.Property;
 import fr.xephi.authme.settings.properties.TestConfiguration;
 import fr.xephi.authme.settings.properties.TestEnum;
 import fr.xephi.authme.settings.propertymap.PropertyMap;
+import org.bukkit.configuration.file.FileConfiguration;
 import org.bukkit.configuration.file.YamlConfiguration;
 import org.junit.BeforeClass;
-import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
@@ -25,6 +25,9 @@ import java.util.Map;
 import static fr.xephi.authme.settings.domain.Property.newProperty;
 import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertThat;
+import static org.mockito.BDDMockito.given;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.mock;
 
 /**
  * Integration test for {@link NewSetting}.
@@ -56,7 +59,7 @@ public class NewSettingIntegrationTest {
         File newFile = temporaryFolder.newFile();
 
         // when / then
-        NewSetting settings = new NewSetting(configuration, newFile, propertyMap);
+        NewSetting settings = new NewSetting(configuration, newFile, propertyMap, new PlainSettingsMigrationService());
         Map<Property<?>, Object> expectedValues = ImmutableMap.<Property<?>, Object>builder()
             .put(TestConfiguration.DURATION_IN_SECONDS, 22)
             .put(TestConfiguration.SYSTEM_NAME, "Custom sys name")
@@ -82,13 +85,13 @@ public class NewSettingIntegrationTest {
         File file = copyFileFromResources(INCOMPLETE_FILE);
         YamlConfiguration configuration = YamlConfiguration.loadConfiguration(file);
         // Expectation: File is rewritten to since it does not have all configurations
-        new NewSetting(configuration, file, propertyMap);
+        new NewSetting(configuration, file, propertyMap, new PlainSettingsMigrationService());
 
         // Load the settings again -> checks that what we wrote can be loaded again
         configuration = YamlConfiguration.loadConfiguration(file);
 
         // then
-        NewSetting settings = new NewSetting(configuration, file, propertyMap);
+        NewSetting settings = new NewSetting(configuration, file, propertyMap, new PlainSettingsMigrationService());
         Map<Property<?>, Object> expectedValues = ImmutableMap.<Property<?>, Object>builder()
             .put(TestConfiguration.DURATION_IN_SECONDS, 22)
             .put(TestConfiguration.SYSTEM_NAME, "[TestDefaultValue]")
@@ -125,14 +128,14 @@ public class NewSettingIntegrationTest {
         }
 
         // when
-        new NewSetting(configuration, file, propertyMap);
+        new NewSetting(configuration, file, propertyMap, new PlainSettingsMigrationService());
         // reload the file as settings should have been rewritten
         configuration = YamlConfiguration.loadConfiguration(file);
 
         // then
         // assert that we won't rewrite the settings again! One rewrite should produce a valid, complete configuration
         File unusedFile = new File("config-difficult-values.unused.yml");
-        NewSetting settings = new NewSetting(configuration, unusedFile, propertyMap);
+        NewSetting settings = new NewSetting(configuration, unusedFile, propertyMap, new PlainSettingsMigrationService());
         assertThat(unusedFile.exists(), equalTo(false));
         assertThat(configuration.contains(TestConfiguration.DUST_LEVEL.getPath()), equalTo(true));
 
@@ -158,13 +161,15 @@ public class NewSettingIntegrationTest {
     }
 
     @Test
-    @Ignore
-    // TODO #603: Un-ignore once migration service is passed to settings
     public void shouldReloadSettings() throws IOException {
         // given
         YamlConfiguration configuration = YamlConfiguration.loadConfiguration(temporaryFolder.newFile());
         File fullConfigFile = copyFileFromResources(COMPLETE_FILE);
-        NewSetting settings = new NewSetting(configuration, fullConfigFile, null);
+        SettingsMigrationService migrationService = mock(SettingsMigrationService.class);
+        given(migrationService.checkAndMigrate(any(FileConfiguration.class), any(PropertyMap.class), any(File.class)))
+            .willReturn(false);
+        NewSetting settings = new NewSetting(configuration, fullConfigFile, TestConfiguration.generatePropertyMap(),
+            new PlainSettingsMigrationService());
 
         // when
         assertThat(settings.getProperty(TestConfiguration.RATIO_ORDER),

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

@@ -6,7 +6,6 @@ import fr.xephi.authme.settings.properties.TestConfiguration;
 import fr.xephi.authme.settings.properties.TestEnum;
 import org.bukkit.configuration.file.YamlConfiguration;
 import org.junit.BeforeClass;
-import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
@@ -59,7 +58,7 @@ public class NewSettingTest {
         setReturnValue(configuration, TestConfiguration.SYSTEM_NAME, "myTestSys");
 
         // when / then
-        NewSetting settings = new NewSetting(configuration, null, null);
+        NewSetting settings = new NewSetting(configuration, null, null, null);
 
         assertThat(settings.getProperty(TestConfiguration.VERSION_NUMBER), equalTo(20));
         assertThat(settings.getProperty(TestConfiguration.SKIP_BORING_FEATURES), equalTo(true));
@@ -75,7 +74,7 @@ public class NewSettingTest {
     public void shouldReturnDefaultFile() throws IOException {
         // given
         YamlConfiguration configuration = mock(YamlConfiguration.class);
-        NewSetting settings = new NewSetting(configuration, null, null);
+        NewSetting settings = new NewSetting(configuration, null, null, null);
 
         // when
         String defaultFile = settings.getDefaultMessagesFile();
@@ -91,7 +90,7 @@ public class NewSettingTest {
     public void shouldSetProperty() {
         // given
         YamlConfiguration configuration = mock(YamlConfiguration.class);
-        NewSetting settings = new NewSetting(configuration, null, null);
+        NewSetting settings = new NewSetting(configuration, null, null, null);
 
         // when
         settings.setProperty(TestConfiguration.DUST_LEVEL, -4);
@@ -101,14 +100,13 @@ public class NewSettingTest {
     }
 
     @Test
-    @Ignore
-    // TODO #603: Un-ignore once migration service is injected into settings
     public void shouldReturnMessagesFile() {
         // given
         YamlConfiguration configuration = mock(YamlConfiguration.class);
         given(configuration.contains(anyString())).willReturn(true);
         given(configuration.getString(eq(MESSAGES_LANGUAGE.getPath()), anyString())).willReturn("fr");
-        NewSetting settings = new NewSetting(configuration, null, TestConfiguration.generatePropertyMap());
+        NewSetting settings = new NewSetting(configuration, null, TestConfiguration.generatePropertyMap(),
+            new PlainSettingsMigrationService());
 
         // when
         File messagesFile = settings.getMessagesFile();

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

@@ -0,0 +1,20 @@
+package fr.xephi.authme.settings;
+
+import fr.xephi.authme.settings.propertymap.PropertyMap;
+import org.bukkit.configuration.file.FileConfiguration;
+
+import java.io.File;
+
+/**
+ * Simple settings migration service simply returning {@code true} if all properties are present,
+ * {@code false} otherwise.
+ */
+public class PlainSettingsMigrationService extends SettingsMigrationService {
+
+    // See parent javadoc: true = some migration had to be done, false = config file is up-to-date
+    @Override
+    public boolean checkAndMigrate(FileConfiguration configuration, PropertyMap propertyMap, File pluginFolder) {
+        return !super.containsAllSettings(configuration, propertyMap);
+    }
+
+}

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

@@ -42,9 +42,10 @@ public class SettingsMigrationServiceTest {
         FileConfiguration configuration = YamlConfiguration.loadConfiguration(configTestFile);
         PropertyMap propertyMap = SettingsFieldRetriever.getAllPropertyFields();
         assumeThat(testFolder.listFiles(), arrayWithSize(1));
+        SettingsMigrationService migrationService = new SettingsMigrationService();
 
         // when
-        boolean result = SettingsMigrationService.checkAndMigrate(configuration, propertyMap, testFolder);
+        boolean result = migrationService.checkAndMigrate(configuration, propertyMap, testFolder);
 
         // then
         assertThat(result, equalTo(false));