From 69c225c8506ed18d2126a000cce68c0238d10ed0 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Fri, 8 Jan 2016 21:22:26 +0100 Subject: [PATCH] #347 Create tests and add check for missing settings in NewSetting --- .../authme/settings/custom/NewSetting.java | 71 ++----- .../custom/SettingsFieldRetriever.java | 67 +++++++ .../authme/settings/domain/Property.java | 10 + .../authme/settings/domain/PropertyType.java | 20 +- .../settings/propertymap/PropertyMap.java | 13 +- .../settings/custom/NewSettingsWriteTest.java | 1 - .../settings/domain/PropertyTypeTest.java | 182 ++++++++++++++++++ .../settings/propertymap/PropertyMapTest.java | 4 +- 8 files changed, 309 insertions(+), 59 deletions(-) create mode 100644 src/main/java/fr/xephi/authme/settings/custom/SettingsFieldRetriever.java create mode 100644 src/test/java/fr/xephi/authme/settings/domain/PropertyTypeTest.java diff --git a/src/main/java/fr/xephi/authme/settings/custom/NewSetting.java b/src/main/java/fr/xephi/authme/settings/custom/NewSetting.java index 138c37d3d..86a15b0d5 100644 --- a/src/main/java/fr/xephi/authme/settings/custom/NewSetting.java +++ b/src/main/java/fr/xephi/authme/settings/custom/NewSetting.java @@ -1,9 +1,8 @@ package fr.xephi.authme.settings.custom; +import com.google.common.annotations.VisibleForTesting; import fr.xephi.authme.ConsoleLogger; -import fr.xephi.authme.settings.domain.Comment; import fr.xephi.authme.settings.domain.Property; -import fr.xephi.authme.settings.domain.SettingsClass; import fr.xephi.authme.settings.propertymap.PropertyMap; import fr.xephi.authme.util.CollectionUtils; import fr.xephi.authme.util.StringUtils; @@ -12,8 +11,6 @@ import org.bukkit.configuration.file.YamlConfiguration; import java.io.File; import java.io.FileWriter; import java.io.IOException; -import java.lang.reflect.Field; -import java.lang.reflect.Modifier; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -24,22 +21,21 @@ import java.util.Map; */ public class NewSetting { - private static final List> CONFIGURATION_CLASSES = Arrays.asList( - ConverterSettings.class, DatabaseSettings.class, EmailSettings.class, HooksSettings.class, - ProtectionSettings.class, PurgeSettings.class, SecuritySettings.class); - private File file; private YamlConfiguration configuration; public NewSetting(File file) { this.configuration = YamlConfiguration.loadConfiguration(file); this.file = file; + + PropertyMap propertyMap = SettingsFieldRetriever.getAllPropertyFields(); + if (!containsAllSettings(propertyMap)) { + save(propertyMap); + } } - // TODO: No way of passing just a YamlConfiguration object (later on for mocking purposes) ? - // If not, best is probably to keep this constructor as package-private with @VisibleForTesting - // but it's not a satisfying solution - public NewSetting(YamlConfiguration yamlConfiguration, String file) { + @VisibleForTesting + NewSetting(YamlConfiguration yamlConfiguration, String file) { this.configuration = yamlConfiguration; this.file = new File(file); } @@ -49,14 +45,16 @@ public class NewSetting { } public void save() { - PropertyMap properties = getAllPropertyFields(); + save(SettingsFieldRetriever.getAllPropertyFields()); + } + public void save(PropertyMap propertyMap) { try (FileWriter writer = new FileWriter(file)) { writer.write(""); // Contains all but the last node of the setting, e.g. [DataSource, mysql] for "DataSource.mysql.username" List currentPath = new ArrayList<>(); - for (Map.Entry entry : properties.entrySet()) { + for (Map.Entry, String[]> entry : propertyMap.entrySet()) { Property property = entry.getKey(); // Handle properties @@ -107,6 +105,15 @@ public class NewSetting { } } + private boolean containsAllSettings(PropertyMap propertyMap) { + for (Property property : propertyMap.keySet()) { + if (!property.isPresent(configuration)) { + return false; + } + } + return true; + } + private static String indent(int level) { // YAML uses indentation of 4 spaces StringBuilder sb = new StringBuilder(level * 4); @@ -116,40 +123,4 @@ public class NewSetting { return sb.toString(); } - private static PropertyMap getAllPropertyFields() { - PropertyMap properties = new PropertyMap(); - for (Class clazz : CONFIGURATION_CLASSES) { - Field[] declaredFields = clazz.getDeclaredFields(); - for (Field field : declaredFields) { - Property property = getFieldIfRelevant(field); - if (property != null) { - properties.put(property, getCommentsForField(field)); - } - } - } - return properties; - } - - private static String[] getCommentsForField(Field field) { - if (field.isAnnotationPresent(Comment.class)) { - return field.getAnnotation(Comment.class).value(); - } - return new String[0]; - } - - private static Property getFieldIfRelevant(Field field) { - field.setAccessible(true); - if (field.isAccessible() && Property.class.equals(field.getType()) && Modifier.isStatic(field.getModifiers())) { - try { - return (Property) field.get(null); - } catch (IllegalAccessException e) { - throw new IllegalStateException("Could not fetch field '" + field.getName() + "' from class '" - + field.getDeclaringClass().getSimpleName() + "': " + StringUtils.formatException(e)); - } - } - return null; - } - - - } diff --git a/src/main/java/fr/xephi/authme/settings/custom/SettingsFieldRetriever.java b/src/main/java/fr/xephi/authme/settings/custom/SettingsFieldRetriever.java new file mode 100644 index 000000000..9727ffac7 --- /dev/null +++ b/src/main/java/fr/xephi/authme/settings/custom/SettingsFieldRetriever.java @@ -0,0 +1,67 @@ +package fr.xephi.authme.settings.custom; + +import fr.xephi.authme.settings.domain.Comment; +import fr.xephi.authme.settings.domain.Property; +import fr.xephi.authme.settings.domain.SettingsClass; +import fr.xephi.authme.settings.propertymap.PropertyMap; +import fr.xephi.authme.util.StringUtils; + +import java.lang.reflect.Field; +import java.lang.reflect.Modifier; +import java.util.Arrays; +import java.util.List; + +/** + * Utility class responsible for the retrieval of all {@link Property} fields via reflections. + */ +final class SettingsFieldRetriever { + + /** The classes to scan for properties. */ + private static final List> CONFIGURATION_CLASSES = Arrays.asList( + ConverterSettings.class, DatabaseSettings.class, EmailSettings.class, HooksSettings.class, + ProtectionSettings.class, PurgeSettings.class, SecuritySettings.class); + + private SettingsFieldRetriever() { + } + + /** + * Scan all given classes for their properties and return the generated {@link PropertyMap}. + * + * @return PropertyMap containing all found properties and their associated comments + * @see #CONFIGURATION_CLASSES + */ + public static PropertyMap getAllPropertyFields() { + PropertyMap properties = new PropertyMap(); + for (Class clazz : CONFIGURATION_CLASSES) { + Field[] declaredFields = clazz.getDeclaredFields(); + for (Field field : declaredFields) { + Property property = getFieldIfRelevant(field); + if (property != null) { + properties.put(property, getCommentsForField(field)); + } + } + } + return properties; + } + + private static String[] getCommentsForField(Field field) { + if (field.isAnnotationPresent(Comment.class)) { + return field.getAnnotation(Comment.class).value(); + } + return new String[0]; + } + + private static Property getFieldIfRelevant(Field field) { + field.setAccessible(true); + if (field.isAccessible() && Property.class.equals(field.getType()) && Modifier.isStatic(field.getModifiers())) { + try { + return (Property) field.get(null); + } catch (IllegalAccessException e) { + throw new IllegalStateException("Could not fetch field '" + field.getName() + "' from class '" + + field.getDeclaringClass().getSimpleName() + "': " + StringUtils.formatException(e)); + } + } + return null; + } + +} diff --git a/src/main/java/fr/xephi/authme/settings/domain/Property.java b/src/main/java/fr/xephi/authme/settings/domain/Property.java index 809f7a987..c0fd2bfe0 100644 --- a/src/main/java/fr/xephi/authme/settings/domain/Property.java +++ b/src/main/java/fr/xephi/authme/settings/domain/Property.java @@ -74,6 +74,16 @@ public class Property { return type.asYaml(this, configuration); } + /** + * Return whether or not the given configuration file contains the property. + * + * @param configuration The configuration file to verify + * @return True if the property is present, false otherwise + */ + public boolean isPresent(YamlConfiguration configuration) { + return type.contains(this, configuration); + } + // ----- // Trivial getters // ----- diff --git a/src/main/java/fr/xephi/authme/settings/domain/PropertyType.java b/src/main/java/fr/xephi/authme/settings/domain/PropertyType.java index b243ef657..5338da7af 100644 --- a/src/main/java/fr/xephi/authme/settings/domain/PropertyType.java +++ b/src/main/java/fr/xephi/authme/settings/domain/PropertyType.java @@ -41,6 +41,17 @@ public abstract class PropertyType { return asYaml(getFromFile(property, configuration)); } + /** + * Return whether the property is present in the given configuration. + * + * @param property The property to search for + * @param configuration The configuration to verify + * @return True if the property is present, false otherwise + */ + public boolean contains(Property property, YamlConfiguration configuration) { + return configuration.contains(property.getPath()); + } + /** * Transform the given value to YAML. * @@ -49,10 +60,6 @@ public abstract class PropertyType { */ protected abstract List asYaml(T value); - protected boolean contains(Property property, YamlConfiguration configuration) { - return configuration.contains(property.getPath()); - } - /** * Boolean property. @@ -145,6 +152,11 @@ public abstract class PropertyType { } return resultLines; } + + @Override + public boolean contains(Property> property, YamlConfiguration configuration) { + return configuration.contains(property.getPath()) && configuration.isList(property.getPath()); + } } } diff --git a/src/main/java/fr/xephi/authme/settings/propertymap/PropertyMap.java b/src/main/java/fr/xephi/authme/settings/propertymap/PropertyMap.java index 934245a07..07cfc7144 100644 --- a/src/main/java/fr/xephi/authme/settings/propertymap/PropertyMap.java +++ b/src/main/java/fr/xephi/authme/settings/propertymap/PropertyMap.java @@ -14,7 +14,7 @@ import java.util.TreeMap; */ public class PropertyMap { - private Map propertyMap; + private Map, String[]> propertyMap; private PropertyMapComparator comparator; /** @@ -41,8 +41,17 @@ public class PropertyMap { * * @return The entry set */ - public Set> entrySet() { + public Set, String[]>> entrySet() { return propertyMap.entrySet(); } + /** + * Return the key set of the map, i.e. all property objects it holds. + * + * @return The key set + */ + public Set> keySet() { + return propertyMap.keySet(); + } + } diff --git a/src/test/java/fr/xephi/authme/settings/custom/NewSettingsWriteTest.java b/src/test/java/fr/xephi/authme/settings/custom/NewSettingsWriteTest.java index 1dadf75e6..ab7e763db 100644 --- a/src/test/java/fr/xephi/authme/settings/custom/NewSettingsWriteTest.java +++ b/src/test/java/fr/xephi/authme/settings/custom/NewSettingsWriteTest.java @@ -30,7 +30,6 @@ public class NewSettingsWriteTest { } - private File getConfigFile() { URL url = getClass().getClassLoader().getResource(CONFIG_FILE); if (url == null) { diff --git a/src/test/java/fr/xephi/authme/settings/domain/PropertyTypeTest.java b/src/test/java/fr/xephi/authme/settings/domain/PropertyTypeTest.java new file mode 100644 index 000000000..6dce2ad3e --- /dev/null +++ b/src/test/java/fr/xephi/authme/settings/domain/PropertyTypeTest.java @@ -0,0 +1,182 @@ +package fr.xephi.authme.settings.domain; + +import org.bukkit.configuration.file.YamlConfiguration; +import org.junit.BeforeClass; +import org.junit.Test; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; + +import java.util.Arrays; +import java.util.List; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.equalTo; +import static org.mockito.Matchers.anyBoolean; +import static org.mockito.Matchers.anyDouble; +import static org.mockito.Matchers.anyInt; +import static org.mockito.Matchers.anyString; +import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Test for {@link PropertyType} and the contained subtypes. + */ +public class PropertyTypeTest { + + private static YamlConfiguration configuration; + + @BeforeClass + public static void setUpYamlConfigurationMock() { + configuration = mock(YamlConfiguration.class); + + when(configuration.getBoolean(eq("bool.path.test"), anyBoolean())).thenReturn(true); + when(configuration.getBoolean(eq("bool.path.wrong"), anyBoolean())).thenAnswer(secondParameter()); + when(configuration.getDouble(eq("double.path.test"), anyDouble())).thenReturn(-6.4); + when(configuration.getDouble(eq("double.path.wrong"), anyDouble())).thenAnswer(secondParameter()); + when(configuration.getInt(eq("int.path.test"), anyInt())).thenReturn(27); + when(configuration.getInt(eq("int.path.wrong"), anyInt())).thenAnswer(secondParameter()); + when(configuration.getString(eq("str.path.test"), anyString())).thenReturn("Test value"); + when(configuration.getString(eq("str.path.wrong"), anyString())).thenAnswer(secondParameter()); + when(configuration.isList("list.path.test")).thenReturn(true); + when(configuration.getStringList("list.path.test")).thenReturn(Arrays.asList("test1", "Test2", "3rd test")); + when(configuration.isList("list.path.wrong")).thenReturn(false); + } + + /* Boolean */ + @Test + public void shouldGetBoolValue() { + // given + Property property = Property.newProperty("bool.path.test", false); + + // when + boolean result = property.getFromFile(configuration); + + // then + assertThat(result, equalTo(true)); + } + + @Test + public void shouldGetBoolDefault() { + // given + Property property = Property.newProperty("bool.path.wrong", true); + + // when + boolean result = property.getFromFile(configuration); + + // then + assertThat(result, equalTo(true)); + } + + /* Double */ + @Test + public void shouldGetDoubleValue() { + // given + Property property = Property.newProperty(PropertyType.DOUBLE, "double.path.test", 3.8); + + // when + double result = property.getFromFile(configuration); + + // then + assertThat(result, equalTo(-6.4)); + } + + @Test + public void shouldGetDoubleDefault() { + // given + Property property = Property.newProperty(PropertyType.DOUBLE, "double.path.wrong", 12.0); + + // when + double result = property.getFromFile(configuration); + + // then + assertThat(result, equalTo(12.0)); + } + + /* Integer */ + @Test + public void shouldGetIntValue() { + // given + Property property = Property.newProperty("int.path.test", 3); + + // when + int result = property.getFromFile(configuration); + + // then + assertThat(result, equalTo(27)); + } + + @Test + public void shouldGetIntDefault() { + // given + Property property = Property.newProperty("int.path.wrong", -10); + + // when + int result = property.getFromFile(configuration); + + // then + assertThat(result, equalTo(-10)); + } + + /* String */ + @Test + public void shouldGetStringValue() { + // given + Property property = Property.newProperty("str.path.test", "unused default"); + + // when + String result = property.getFromFile(configuration); + + // then + assertThat(result, equalTo("Test value")); + } + + @Test + public void shouldGetStringDefault() { + // given + Property property = Property.newProperty("str.path.wrong", "given default value"); + + // when + String result = property.getFromFile(configuration); + + // then + assertThat(result, equalTo("given default value")); + } + + /* String list */ + @Test + public void shouldGetStringListValue() { + // given + Property> property = Property.newProperty(PropertyType.STRING_LIST, "list.path.test", "1", "b"); + + // when + List result = property.getFromFile(configuration); + + // then + assertThat(result, contains("test1", "Test2", "3rd test")); + } + + @Test + public void shouldGetStringListDefault() { + // given + Property> property = + Property.newProperty(PropertyType.STRING_LIST, "list.path.wrong", "default", "list", "elements"); + + // when + List result = property.getFromFile(configuration); + + // then + assertThat(result, contains("default", "list", "elements")); + } + + private static Answer secondParameter() { + return new Answer() { + @Override + public T answer(InvocationOnMock invocation) throws Throwable { + // Return the second parameter -> the default + return (T) invocation.getArguments()[1]; + } + }; + } +} diff --git a/src/test/java/fr/xephi/authme/settings/propertymap/PropertyMapTest.java b/src/test/java/fr/xephi/authme/settings/propertymap/PropertyMapTest.java index 895b12e1c..40ea14b74 100644 --- a/src/test/java/fr/xephi/authme/settings/propertymap/PropertyMapTest.java +++ b/src/test/java/fr/xephi/authme/settings/propertymap/PropertyMapTest.java @@ -34,9 +34,9 @@ public class PropertyMapTest { } // then - Set> entrySet = map.entrySet(); + Set, String[]>> entrySet = map.entrySet(); List resultPaths = new ArrayList<>(entrySet.size()); - for (Map.Entry entry : entrySet) { + for (Map.Entry, String[]> entry : entrySet) { resultPaths.add(entry.getKey().getPath()); }