From 76ca326590ff63ccf76a8eae1aadfffcc6a0e836 Mon Sep 17 00:00:00 2001 From: Konicai <71294714+Konicai@users.noreply.github.com> Date: Thu, 8 Aug 2024 21:15:47 -0500 Subject: [PATCH 1/3] Improve node ordering when updating configs --- .../viaproxy/GeyserViaProxyPlugin.java | 2 +- .../geyser/configuration/ConfigLoader.java | 41 ++++++--- .../ConfigurationCommentMover.java | 85 +++++++++++++++++++ .../configuration/ConfigLoaderTest.java | 79 +++++++++++++++++ 4 files changed, 196 insertions(+), 11 deletions(-) create mode 100644 core/src/main/java/org/geysermc/geyser/configuration/ConfigurationCommentMover.java create mode 100644 core/src/test/java/org/geysermc/geyser/configuration/ConfigLoaderTest.java diff --git a/bootstrap/viaproxy/src/main/java/org/geysermc/geyser/platform/viaproxy/GeyserViaProxyPlugin.java b/bootstrap/viaproxy/src/main/java/org/geysermc/geyser/platform/viaproxy/GeyserViaProxyPlugin.java index 59a0d7866..908ceec79 100644 --- a/bootstrap/viaproxy/src/main/java/org/geysermc/geyser/platform/viaproxy/GeyserViaProxyPlugin.java +++ b/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); diff --git a/core/src/main/java/org/geysermc/geyser/configuration/ConfigLoader.java b/core/src/main/java/org/geysermc/geyser/configuration/ConfigLoader.java index 43e5fa1f7..4872a8c17 100644 --- a/core/src/main/java/org/geysermc/geyser/configuration/ConfigLoader.java +++ b/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 load(File file, Class configClass, @Nullable Consumer 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 - boolean virtual = file.exists(); + + CommentedConfigurationNode node = loader.load(); + 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) { - loader.save(node); + // Serialize the instance to ensure strict field ordering. Additionally, If we serialized back + // 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. - transformer.accept(node); - config = node.get(configClass); + // We transform AFTER saving so that these specific transformations aren't applied to file. + transformer.accept(newRoot); + config = newRoot.get(configClass); } return config; diff --git a/core/src/main/java/org/geysermc/geyser/configuration/ConfigurationCommentMover.java b/core/src/main/java/org/geysermc/geyser/configuration/ConfigurationCommentMover.java new file mode 100644 index 000000000..4aed64da7 --- /dev/null +++ b/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 { + + 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)); + } + } +} diff --git a/core/src/test/java/org/geysermc/geyser/configuration/ConfigLoaderTest.java b/core/src/test/java/org/geysermc/geyser/configuration/ConfigLoaderTest.java new file mode 100644 index 000000000..0dcc7d33d --- /dev/null +++ b/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 firstContents = Files.readAllLines(file.toPath()); + + ConfigLoader.load(file, type, n -> this.config2 = n.copy()); + List 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, Exception> consumer) throws Exception { + consumer.accept(GeyserRemoteConfig.class); + consumer.accept(GeyserPluginConfig.class); + } +} From 0952f79c445515ab20b87519a550508eccae8ab8 Mon Sep 17 00:00:00 2001 From: Konicai <71294714+Konicai@users.noreply.github.com> Date: Thu, 8 Aug 2024 21:21:14 -0500 Subject: [PATCH 2/3] typo --- .../java/org/geysermc/geyser/configuration/ConfigLoader.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/geysermc/geyser/configuration/ConfigLoader.java b/core/src/main/java/org/geysermc/geyser/configuration/ConfigLoader.java index 4872a8c17..844c3f8f0 100644 --- a/core/src/main/java/org/geysermc/geyser/configuration/ConfigLoader.java +++ b/core/src/main/java/org/geysermc/geyser/configuration/ConfigLoader.java @@ -124,7 +124,7 @@ public final class ConfigLoader { T config = node.get(configClass); - // Serialize the instance to ensure strict field ordering. Additionally, If we serialized back + // Serialize the instance to ensure strict field ordering. Additionally, if we serialized back // 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. From e506c14eb4bfb5404269a620582c1d5ae252a251 Mon Sep 17 00:00:00 2001 From: Konicai <71294714+Konicai@users.noreply.github.com> Date: Thu, 8 Aug 2024 21:30:44 -0500 Subject: [PATCH 3/3] Better check for ignoring non-configurate configs for considering comment moving --- .../java/org/geysermc/geyser/configuration/ConfigLoader.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/geysermc/geyser/configuration/ConfigLoader.java b/core/src/main/java/org/geysermc/geyser/configuration/ConfigLoader.java index 844c3f8f0..e8e205275 100644 --- a/core/src/main/java/org/geysermc/geyser/configuration/ConfigLoader.java +++ b/core/src/main/java/org/geysermc/geyser/configuration/ConfigLoader.java @@ -133,7 +133,7 @@ public final class ConfigLoader { if (originallyEmpty || currentVersion != newVersion) { - if (!originallyEmpty && currentVersion != 4) { + 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.