Partly cleanup handling moved paths.

* Add a test for moving a simple config value (not sections).
* Add a flag to @Moved to allow explicitly skipping sections.
  (Should enable moving values into a child path of the previous one.)
* Don't set values before inside of processMoved...

NCPCore/src/main/java/fr/neatmonster/nocheatplus/config/Moved.java

@@ -16,5 +16,18 @@ import java.lang.annotation.Target;
 @Target(ElementType.FIELD)
 @Retention(RetentionPolicy.RUNTIME)
 public @interface Moved{
+
+    /**
+     * The new path where the content has moved to.
+     * @return
+     */
     public String newPath() default "";
+
+    /**
+     * Only added to be able to explicitly deny moving configuration sections.
+     * Moving configuration sections might not actually be supported.
+     * 
+     * @return If to allow moving configuration sections.
+     */
+    public boolean configurationSection() default false;
 }

NCPCore/src/main/java/fr/neatmonster/nocheatplus/config/PathUtils.java

@@ -12,9 +12,7 @@ import java.util.LinkedHashSet;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
-import java.util.logging.Logger;
 
-import org.bukkit.Bukkit;
 import org.bukkit.configuration.ConfigurationSection;
 import org.bukkit.configuration.InvalidConfigurationException;
 import org.bukkit.configuration.MemoryConfiguration;
@@ -25,6 +23,8 @@ import fr.neatmonster.nocheatplus.utilities.ds.prefixtree.SimpleCharPrefixTree;
 
 public class PathUtils {
 
+    // TODO: Make this a class to be able to process multiple files with different paths and annotations.
+
     // Deprecated paths.
     private static final Set<String> deprecatedFields = new LinkedHashSet<String>();
     private static final SimpleCharPrefixTree deprecatedPrefixes = new SimpleCharPrefixTree();
@@ -34,7 +34,7 @@ public class PathUtils {
     private static final SimpleCharPrefixTree globalOnlyPrefixes = new SimpleCharPrefixTree();
 
     // Paths moved to other paths.
-    private static final Map<String, String> movedPaths = new LinkedHashMap<String, String>();
+    private static final Map<String, Moved> movedPaths = new LinkedHashMap<String, Moved>();
 
     static{
         initPaths();
@@ -94,7 +94,7 @@ public class PathUtils {
     private static void addMoved(final Field field, final Moved rel) {
         try {
             final String path = field.get(null).toString();
-            movedPaths.put(path, rel.newPath());
+            movedPaths.put(path, rel);
         } catch (IllegalArgumentException e) {
         } catch (IllegalAccessException e) {
         }
@@ -107,11 +107,10 @@ public class PathUtils {
      * @param msgHeader
      * @param warnedPaths Paths which were found, can be null.
      */
-    protected static void warnPaths(final ConfigFile config, final CharPrefixTree<?, ?> paths, final String msgPrefix, final Set<String> warnedPaths){
-        final Logger logger = Bukkit.getLogger();
+    protected static void warnPaths(final ConfigurationSection config, final CharPrefixTree<?, ?> paths, final String msgPrefix, final Set<String> warnedPaths) {
         for (final String path : config.getKeys(true)) {
             if (paths.hasPrefix(path)) {
-                logger.warning("[NoCheatPlus] Config path '" + path + "'" + msgPrefix);
+                StaticLog.logWarning("[NoCheatPlus] Config path '" + path + "'" + msgPrefix);
                 if (warnedPaths != null) {
                     warnedPaths.add(path);
                 }
@@ -120,7 +119,7 @@ public class PathUtils {
     }
 
     /**
-     * Run all warning checks and alter config if necessary (GlobalConfig, Deprecated, Moved).
+     * Run all warning checks and alter the configuration file if necessary (GlobalConfig, Deprecated, Moved).
      * @param file
      * @param configName
      */
@@ -128,24 +127,9 @@ public class PathUtils {
         ConfigFile config = new ConfigFile();
         try {
             config.load(file);
-            final Set<String> removePaths = new LinkedHashSet<String>();
-            final Map<String, Object> addPaths = new LinkedHashMap<String, Object>();
-            if (isWorldConfig){
-                // TODO: might remove these [though some global only paths might actually work].
-                processGlobalOnlyPaths(config, configName, null);
-            }
-            processDeprecatedPaths(config, configName, removePaths);
-            processMovedPaths(config, configName, removePaths, addPaths);
-            boolean changed = false;
-            if (!removePaths.isEmpty()){
-                config = removePaths(config, removePaths);
-                changed = true;
-            }
-            if (!addPaths.isEmpty()){
-                setPaths(config, addPaths);
-                changed = true;
-            }
-            if (changed){
+            ConfigFile newConfig = processPaths(config, configName, isWorldConfig);
+            if (newConfig != null) {
+                config = newConfig;
                 try{
                     config.save(file);
                 }
@@ -161,14 +145,67 @@ public class PathUtils {
         }
     }
 
+    /**
+     * Process configuration annotations, might change the given configuration or return a new one.
+     * @param config Configuration instance.
+     * @param configName A name for logging errors.
+     * @param isWorldConfig
+     * @return A new configuration instance if paths had to be removed (i.e. a
+     *         new one was needed), null if the same configuration instance
+     *         could be used (paths still might have been added).
+     */
+    public static ConfigFile processPaths(ConfigFile config, final String configName, final boolean isWorldConfig) {
+        final Set<String> removePaths = new LinkedHashSet<String>();
+        final Map<String, Object> addPaths = new LinkedHashMap<String, Object>();
+        if (isWorldConfig) {
+            // TODO: might remove these [though some global only paths might actually work].
+            processGlobalOnlyPaths(config, configName, null);
+        }
+        processDeprecatedPaths(config, configName, removePaths);
+        processMovedPaths(config, configName, removePaths, addPaths);
+        boolean changed = false;
+        if (!removePaths.isEmpty()) {
+            config = removePaths(config, removePaths);
+            changed = true;
+        }
+        if (!addPaths.isEmpty()) {
+            setPaths(config, addPaths, false);
+            changed = true;
+        }
+        if (changed) {
+            return config;
+        } else {
+            return null;
+        }
+    }
+
     /**
      * Set paths.
      * @param config
      * @param addPaths
      */
-    public static void setPaths(final ConfigFile config, final Map<String, Object> setPaths) {
+    public static void setPaths(final ConfigurationSection config, final Map<String, Object> setPaths, final boolean override) {
+        setPaths(config, setPaths, "", override);
+    }
+
+    /**
+     * Recursive version supporting ConfigurationSection. 
+     * @param config
+     * @param setPaths
+     * @param pathPrefix
+     */
+    protected static void setPaths(final ConfigurationSection config, final Map<String, Object> setPaths, final String pathPrefix, final boolean override) {
         for (final Entry<String, Object> entry : setPaths.entrySet()) {
-            config.set(entry.getKey(), entry.getValue());
+            final String path = entry.getKey();
+            final Object value = entry.getValue();
+            if (value instanceof ConfigurationSection) {
+                // Deep or not should not matter here.
+                setPaths(config, ((ConfigurationSection) value).getValues(true), pathPrefix + path + ".", override);
+            } else {
+                if (override || !config.contains(path)) {
+                    config.set(pathPrefix + path, value);
+                }
+            }
         }
     }
 
@@ -187,6 +224,7 @@ public class PathUtils {
         for (final Entry<String, Object> entry : config.getValues(true).entrySet()) {
             final String path = entry.getKey();
             final Object value = entry.getValue();
+            // TODO: To support moving entire sections, this needs to be changed.
             if (value instanceof ConfigurationSection) {
                 continue;
             }
@@ -203,26 +241,32 @@ public class PathUtils {
      * @param configName
      * @param removePaths
      * @param addPaths 
-     * @return If entries were added (paths to be removed are processed later).
      */
-    protected static void processMovedPaths(final ConfigFile config, final String configName, final Set<String> removePaths, final Map<String, Object> addPaths) {
-        final Logger logger = Bukkit.getLogger();
-        for (final Entry<String, String> entry : movedPaths.entrySet()){
+    protected static void processMovedPaths(final ConfigurationSection config, final String configName, final Set<String> removePaths, final Map<String, Object> addPaths) {
+        for (final Entry<String, Moved> entry : movedPaths.entrySet()) {
             final String path = entry.getKey();
+            final Object value = config.get(path);
             if (config.contains(path)) {
-                final String newPath = entry.getValue();
+                final Moved moved = entry.getValue();
+                if (value instanceof ConfigurationSection) {
+                    if (moved.configurationSection()) {
+                        // TODO: Ensure those can be processed at all.
+                    } else {
+                        // Ignore configuration sections.
+                        continue;
+                    }
+                }
+                final String newPath = moved.newPath();
                 final String to;
-                if (newPath == null | newPath.isEmpty()){
+                if (newPath == null || newPath.isEmpty()) {
                     to = ".";
                 }
                 else {
                     to = " to '" + newPath + "'.";
-                    final Object value = config.get(path);
-                    config.set(newPath, value);
                     addPaths.put(newPath, value);
                     removePaths.add(path);
                 }
-                logger.warning("[NoCheatPlus] Config path '" + path + "' (" + configName + ") has been moved" + to);
+                StaticLog.logWarning("[NoCheatPlus] Config path '" + path + "' (" + configName + ") has been moved" + to);
             }
         }
     }
@@ -233,7 +277,7 @@ public class PathUtils {
      * @param paths
      * @param configName
      */
-    protected static void processDeprecatedPaths(ConfigFile config, String configName, final Set<String> removePaths){
+    protected static void processDeprecatedPaths(ConfigurationSection config, String configName, final Set<String> removePaths) {
         warnPaths(config, deprecatedPrefixes, " (" + configName + ") is not in use anymore.", removePaths);
     }
 
@@ -243,7 +287,7 @@ public class PathUtils {
      * @param paths
      * @param configName
      */
-    protected static void processGlobalOnlyPaths(ConfigFile config, String configName, final Set<String> removePaths){
+    protected static void processGlobalOnlyPaths(ConfigurationSection config, String configName, final Set<String> removePaths) {
         warnPaths(config, globalOnlyPrefixes, " (" + configName + ") should only be set in the global configuration.", removePaths);
     }
 
@@ -252,41 +296,61 @@ public class PathUtils {
      * @param defaultConfig
      * @return
      */
-    public static MemoryConfiguration getWorldsDefaultConfig(final ConfigFile defaultConfig){
+    public static MemoryConfiguration getWorldsDefaultConfig(final MemoryConfiguration defaultConfig) {
         final char sep = defaultConfig.options().pathSeparator();
         final MemoryConfiguration config = new ConfigFile();
         config.options().pathSeparator(sep);
         final Map<String, Object> defaults = defaultConfig.getValues(false);
         for (final Entry<String, Object> entry : defaults.entrySet()) {
             final String part = entry.getKey();
-            if (!part.isEmpty() && !mayBeInWorldConfig(part)) continue;
+            if (!part.isEmpty() && !mayBeInWorldConfig(part)) {
+                continue;
+            }
             final Object value = entry.getValue();
-            if (value instanceof ConfigurationSection) addWorldConfigSection(config, (ConfigurationSection) value, part, sep);
-            else config.set(part, value);
+            if (value instanceof ConfigurationSection) {
+                addWorldConfigSection(config, (ConfigurationSection) value, part, sep);
+            }
+            else {
+                config.set(part, value);
+            }
         }
         return config;
     }
 
-    protected static void addWorldConfigSection(final MemoryConfiguration config, final ConfigurationSection section, final String path, final char sep) {
+    protected static void addWorldConfigSection(final ConfigurationSection config, final ConfigurationSection section, final String path, final char sep) {
         final Map<String, Object> values = section.getValues(false);
         for (final Entry<String, Object> entry : values.entrySet()) {
             final String fullPath = path + sep + entry.getKey();
-            if (!mayBeInWorldConfig(fullPath)) continue;
+            if (!mayBeInWorldConfig(fullPath)) {
+                continue;
+            }
             final Object value = entry.getValue();
-            if (value instanceof ConfigurationSection) addWorldConfigSection(config, (ConfigurationSection) value, fullPath, sep);
-            else config.set(fullPath, value);
+            if (value instanceof ConfigurationSection) {
+                addWorldConfigSection(config, (ConfigurationSection) value, fullPath, sep);
+            }
+            else {
+                config.set(fullPath, value);
+            }
         }
     }
 
     public static boolean mayBeInWorldConfig(final String path) {
-        if (globalOnlyPrefixes.hasPrefix(path)) return false;
+        if (globalOnlyPrefixes.hasPrefix(path)) {
+            return false;
+        } else {
             return mayBeInConfig(path);
         }
+    }
 
     public static boolean mayBeInConfig(final String path) {
-        if (deprecatedPrefixes.hasPrefix(path)) return false;
-        if (movedPaths.containsKey(path)) return false;
+        if (deprecatedPrefixes.hasPrefix(path)) {
+            return false;
+        }
+        else if (movedPaths.containsKey(path)) {
+            return false;
+        } else {
             return true;
         }
+    }
 
 }

NCPPlugin/src/test/java/fr/neatmonster/nocheatplus/test/TestConfig.java

@@ -5,6 +5,9 @@ import static org.junit.Assert.fail;
 import org.bukkit.Material;
 import org.junit.Test;
 
+import fr.neatmonster.nocheatplus.config.ConfPaths;
+import fr.neatmonster.nocheatplus.config.ConfigFile;
+import fr.neatmonster.nocheatplus.config.PathUtils;
 import fr.neatmonster.nocheatplus.config.RawConfigFile;
 
 public class TestConfig {
@@ -35,6 +38,28 @@ public class TestConfig {
             testReadMaterial(mat.name(), mat);
             testReadMaterial(Integer.toString(mat.getId()), mat);
         }
+    }
+
+    // TODO: More ConfigFile tests, once processing gets changed.
+
+    @Test
+    public void testMovePaths() {
+        // TODO: Set StaticLog to not log.
+        ConfigFile config = new ConfigFile();
+
+        // Simple moved boolean.
+        config.set(ConfPaths.LOGGING_FILE, false);
+        config = PathUtils.processPaths(config, "test", false);
+        if (config == null) {
+            fail("Expect config to be changed at all.");
+        }
+        if (config.contains(ConfPaths.LOGGING_FILE)) {
+            fail("Expect path be removed: " + ConfPaths.LOGGING_FILE);
+        }
+        Boolean val = config.getBoolean(ConfPaths.LOGGING_BACKEND_FILE_ACTIVE, true);
+        if (val == null || val.booleanValue()) {
+            fail("Expect new path to be set to false: " + ConfPaths.LOGGING_BACKEND_FILE_ACTIVE);
+        }
+    }
 
 }
-}