diff --git a/src/main/java/fr/xephi/authme/AuthMe.java b/src/main/java/fr/xephi/authme/AuthMe.java index 3fe90ba97..58d791d5e 100644 --- a/src/main/java/fr/xephi/authme/AuthMe.java +++ b/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; } diff --git a/src/main/java/fr/xephi/authme/settings/NewSetting.java b/src/main/java/fr/xephi/authme/settings/NewSetting.java index 809655834..2c5df97e7 100644 --- a/src/main/java/fr/xephi/authme/settings/NewSetting.java +++ b/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(); diff --git a/src/main/java/fr/xephi/authme/settings/SettingsMigrationService.java b/src/main/java/fr/xephi/authme/settings/SettingsMigrationService.java index 72806a8ba..a3d70dd16 100644 --- a/src/main/java/fr/xephi/authme/settings/SettingsMigrationService.java +++ b/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 { - - private SettingsMigrationService() { - } +public class 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) { - return performMigrations(configuration, pluginFolder) || hasDeprecatedProperties(configuration) + public boolean checkAndMigrate(FileConfiguration configuration, PropertyMap propertyMap, File pluginFolder) { + 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 - static boolean containsAllSettings(FileConfiguration configuration, PropertyMap propertyMap) { + public 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("", "") .replace("", "") diff --git a/src/test/java/fr/xephi/authme/settings/ConfigFileConsistencyTest.java b/src/test/java/fr/xephi/authme/settings/ConfigFileConsistencyTest.java index 7c9170f39..6a7d7fe1c 100644 --- a/src/test/java/fr/xephi/authme/settings/ConfigFileConsistencyTest.java +++ b/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( - configuration, SettingsFieldRetriever.getAllPropertyFields()); + boolean result = migration.containsAllSettings(configuration, SettingsFieldRetriever.getAllPropertyFields()); // then if (!result) { diff --git a/src/test/java/fr/xephi/authme/settings/NewSettingIntegrationTest.java b/src/test/java/fr/xephi/authme/settings/NewSettingIntegrationTest.java index b56f125c7..246c67dd0 100644 --- a/src/test/java/fr/xephi/authme/settings/NewSettingIntegrationTest.java +++ b/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, Object> expectedValues = ImmutableMap., 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, Object> expectedValues = ImmutableMap., 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), diff --git a/src/test/java/fr/xephi/authme/settings/NewSettingTest.java b/src/test/java/fr/xephi/authme/settings/NewSettingTest.java index 047289808..a6edca0fa 100644 --- a/src/test/java/fr/xephi/authme/settings/NewSettingTest.java +++ b/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(); diff --git a/src/test/java/fr/xephi/authme/settings/PlainSettingsMigrationService.java b/src/test/java/fr/xephi/authme/settings/PlainSettingsMigrationService.java new file mode 100644 index 000000000..47a68226e --- /dev/null +++ b/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); + } + +} diff --git a/src/test/java/fr/xephi/authme/settings/SettingsMigrationServiceTest.java b/src/test/java/fr/xephi/authme/settings/SettingsMigrationServiceTest.java index cc89a463a..a85f86e1b 100644 --- a/src/test/java/fr/xephi/authme/settings/SettingsMigrationServiceTest.java +++ b/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));