Improve node ordering when updating configs

bootstrap/viaproxy/src/main/java/org/geysermc/geyser/platform/viaproxy/GeyserViaProxyPlugin.java

@@ -228,7 +228,7 @@ public class GeyserViaProxyPlugin extends ViaProxyPlugin implements GeyserPlugin
                     int interval = pingPassthroughInterval.getInt();
                     if (interval < 15 && ViaProxy.getConfig().getTargetVersion() != null && ViaProxy.getConfig().getTargetVersion().olderThanOrEqualTo(LegacyProtocolVersion.r1_6_4)) {
                         // <= 1.6.4 servers sometimes block incoming connections from an IP address if too many connections are made
-                        node.set(15);
+                        pingPassthroughInterval.set(15);
                     }
                 } catch (SerializationException e) {
                     throw new RuntimeException(e);

core/src/main/java/org/geysermc/geyser/configuration/ConfigLoader.java

@@ -29,6 +29,7 @@ import org.checkerframework.checker.nullness.qual.Nullable;
 import org.spongepowered.configurate.CommentedConfigurationNode;
 import org.spongepowered.configurate.interfaces.InterfaceDefaultOptions;
 import org.spongepowered.configurate.transformation.ConfigurationTransformation;
+import org.spongepowered.configurate.yaml.NodeStyle;
 import org.spongepowered.configurate.yaml.YamlConfigurationLoader;
 
 import java.io.File;
@@ -61,13 +62,16 @@ public final class ConfigLoader {
     public static <T extends GeyserConfig> T load(File file, Class<T> configClass, @Nullable Consumer<CommentedConfigurationNode> transformer) throws IOException {
         var loader = YamlConfigurationLoader.builder()
                 .file(file)
-                .defaultOptions(options -> InterfaceDefaultOptions.addTo(options
+                .indent(2)
+                .nodeStyle(NodeStyle.BLOCK)
+                .defaultOptions(options -> InterfaceDefaultOptions.addTo(options)
+                    .shouldCopyDefaults(false) // If we use ConfigurationNode#get(type, default), do not write the default back to the node.
                     .header(HEADER)
-                    .serializers(builder -> builder.register(new LowercaseEnumSerializer()))))
+                    .serializers(builder -> builder.register(new LowercaseEnumSerializer())))
                 .build();
-        var node = loader.load();
+
-        // temp fix for node.virtual() being broken
+        CommentedConfigurationNode node = loader.load();
-        boolean virtual = file.exists();
+        boolean originallyEmpty = !file.exists() || node.isNull();
 
         // Note for Tim? Needed or else Configurate breaks.
         var migrations = ConfigurationTransformation.versionedBuilder()
@@ -120,14 +124,31 @@ public final class ConfigLoader {
 
         T config = node.get(configClass);
 
-        if (virtual || currentVersion != newVersion) {
+        // Serialize the instance to ensure strict field ordering. Additionally, If we serialized back
-            loader.save(node);
+        // to the old node, existing nodes would only have their value changed, keeping their position
+        // at the top of the ordered map, forcing all new nodes to the bottom (regardless of field order).
+        // For that reason, we must also create a new node.
+        CommentedConfigurationNode newRoot = CommentedConfigurationNode.root(loader.defaultOptions());
+        newRoot.set(config);
+
+        if (originallyEmpty || currentVersion != newVersion) {
+
+            if (!originallyEmpty && currentVersion != 4) {
+                // Only copy comments over if the file already existed, and we are going to replace it
+
+                // Second case: Version 4 is pre-configurate where there were commented out nodes.
+                // These get treated as comments on lower nodes, which produces very undesirable results.
+
+                ConfigurationCommentMover.moveComments(node, newRoot);
+            }
+
+            loader.save(newRoot);
         }
 
         if (transformer != null) {
-            // Do not let the transformer change the node.
+            // We transform AFTER saving so that these specific transformations aren't applied to file.
-            transformer.accept(node);
+            transformer.accept(newRoot);
-            config = node.get(configClass);
+            config = newRoot.get(configClass);
         }
 
         return config;

core/src/main/java/org/geysermc/geyser/configuration/ConfigurationCommentMover.java

@@ -0,0 +1,85 @@
+/*
+ * Copyright (c) 2024 GeyserMC. http://geysermc.org
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ * @author GeyserMC
+ * @link https://github.com/GeyserMC/Geyser
+ */
+
+package org.geysermc.geyser.configuration;
+
+import org.checkerframework.checker.nullness.qual.NonNull;
+import org.spongepowered.configurate.CommentedConfigurationNode;
+import org.spongepowered.configurate.ConfigurationNode;
+import org.spongepowered.configurate.ConfigurationVisitor;
+
+/**
+ * Moves comments from a different node and puts them on this node
+ */
+public final class ConfigurationCommentMover implements ConfigurationVisitor.Stateless<RuntimeException> {
+
+    private final CommentedConfigurationNode otherRoot;
+
+    private ConfigurationCommentMover(@NonNull CommentedConfigurationNode otherNode) {
+        this.otherRoot = otherNode;
+    }
+
+    @Override
+    public void enterNode(final ConfigurationNode node) {
+        if (!(node instanceof CommentedConfigurationNode destination)) {
+            // Should not occur because all nodes in a tree are the same type,
+            // and our static method below ensures this visitor is only used on CommentedConfigurationNodes
+            throw new IllegalStateException(node.path() + " is not a CommentedConfigurationNode");
+        }
+        // Node with the same path
+        CommentedConfigurationNode source = otherRoot.node(node.path());
+
+        moveSingle(source, destination);
+    }
+
+    private static void moveSingle(@NonNull CommentedConfigurationNode source, @NonNull CommentedConfigurationNode destination) {
+        // Only transfer the comment, overriding if necessary
+        String comment = source.comment();
+        if (comment != null) {
+            destination.comment(comment);
+        }
+    }
+
+    /**
+     * Moves comments from a source node and its children to a destination node and its children (of a different tree), overriding if necessary.
+     * Comments are only moved to the destination node and its children which exist.
+     * Comments are only moved to and from nodes with the exact same path.
+     *
+     * @param source the source of the comments, which must be the topmost parent of a tree
+     * @param destination the destination of the comments, any node in a different tree
+     */
+    public static void moveComments(@NonNull CommentedConfigurationNode source, @NonNull CommentedConfigurationNode destination) {
+        if (source.parent() != null) {
+            throw new IllegalArgumentException("source is not the base of the tree it is within: " + source.path());
+        }
+
+        if (source.isNull()) {
+            // It has no value(s), but may still have a comment on it. Don't both traversing the whole destination tree.
+            moveSingle(source, destination);
+        } else {
+            destination.visit(new ConfigurationCommentMover(source));
+        }
+    }
+}

core/src/test/java/org/geysermc/geyser/configuration/ConfigLoaderTest.java

@@ -0,0 +1,79 @@
+/*
+ * Copyright (c) 2024 GeyserMC. http://geysermc.org
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ *
+ * @author GeyserMC
+ * @link https://github.com/GeyserMC/Geyser
+ */
+
+package org.geysermc.geyser.configuration;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.spongepowered.configurate.CommentedConfigurationNode;
+import org.spongepowered.configurate.util.CheckedConsumer;
+
+import java.io.File;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class ConfigLoaderTest {
+
+    @TempDir
+    Path tempDirectory;
+
+    // Hack until improving ConfigLoader
+    CommentedConfigurationNode config1;
+    CommentedConfigurationNode config2;
+
+    @Test
+    void testCreateNewConfig() throws Exception {
+        // Test that the result of generating a config, and the result of reading it back after writing it, is the same
+
+        File file = tempDirectory.resolve("config.yml").toFile();
+
+        forAllConfigs(type -> {
+            ConfigLoader.load(file, type, n -> this.config1 = n.copy());
+            long initialModification = file.lastModified();
+            assertTrue(file.exists()); // should have been created
+            List<String> firstContents = Files.readAllLines(file.toPath());
+
+            ConfigLoader.load(file, type, n -> this.config2 = n.copy());
+            List<String> secondContents = Files.readAllLines(file.toPath());
+
+            assertEquals(initialModification, file.lastModified()); // should not have been touched
+            assertEquals(firstContents, secondContents);
+
+            // Must ignore this, as when the config is read back, the header is interpreted as a comment on the first node in the map
+            config1.node("java").comment(null);
+            config2.node("java").comment(null);
+            assertEquals(config1, config2);
+        });
+    }
+
+    void forAllConfigs(CheckedConsumer<Class<? extends GeyserConfig>, Exception> consumer) throws Exception {
+        consumer.accept(GeyserRemoteConfig.class);
+        consumer.accept(GeyserPluginConfig.class);
+    }
+}