Fix config sub-node duplication. (#7425)

* init commit

* add comments

src/main/java/ch/njol/skript/Skript.java

@@ -842,6 +842,8 @@ private void runTests() {
 					classes.removeIf(Class::isLocalClass);
 					// Test that requires package access. This is only present when compiling with src/test.
 					classes.add(Class.forName("ch.njol.skript.variables.FlatFileStorageTest"));
+					classes.add(Class.forName("ch.njol.skript.config.ConfigTest"));
+					classes.add(Class.forName("ch.njol.skript.config.NodeTest"));
 					size.set(classes.size());
 					for (Class<?> clazz : classes) {
 						if (SkriptAsyncJUnitTest.class.isAssignableFrom(clazz)) {

src/main/java/ch/njol/skript/config/Config.java

@@ -216,13 +216,41 @@ public boolean updateNodes(@NotNull Config newer) {
 		Set<Node> newNodes = discoverNodes(newer.getMainNode());
 		Set<Node> oldNodes = discoverNodes(getMainNode());
 
+		// find the nodes that are in the new config but not in the old one
 		newNodes.removeAll(oldNodes);
 		Set<Node> nodesToUpdate = new LinkedHashSet<>(newNodes);
 
 		if (nodesToUpdate.isEmpty())
 			return false;
 
 		for (Node node : nodesToUpdate) {
+			/*
+			 prevents nodes that are already in the config from being added again
+			 this happens when section nodes are added to the config, as their children
+			 are also carried over from the new config, but are also in 'nodesToUpdate'
+
+			 example:
+			 nodesToUpdate is this
+			 - x
+			 - x.y
+			 - x.z
+
+			 and if the method adds x, since x has children in the new config,
+			 it'll add the children to the to-be-updated config, so it'll add
+			 x:
+			   y: 'whatever'
+			   z: 'whatever'
+
+			 but it also wants to add x.y since that node previously did not exist,
+			 but now it does, so it duplicates it without that if statement
+			 x:
+			  y: 'whatever'
+			  y: 'whatever'
+			  z: 'whatever'
+			*/
+			if (get(node.getPathSteps()) != null)
+				continue;
+
 			Skript.debug("Updating node %s", node);
 			SectionNode newParent = node.getParent();
 			Preconditions.checkNotNull(newParent);
@@ -232,16 +260,22 @@ public boolean updateNodes(@NotNull Config newer) {
 
 			int index = node.getIndex();
 			if (index >= parent.size()) {
+				// in case we have some user-added comments or something goes wrong, to ensure index is within bounds
+
 				Skript.debug("Adding node %s to %s (size mismatch)", node, parent);
 				parent.add(node);
 				continue;
 			}
 
 			Node existing = parent.getAt(index);
-			if (existing != null) { // insert between existing
+			if (existing != null) {
+				// there's already something at the node we want to add the new node
+
 				Skript.debug("Adding node %s to %s at index %s", node, parent, index);
 				parent.add(index, node);
 			} else {
+				// there's nothing at the index we want to add the new node
+
 				Skript.debug("Adding node %s to %s", node, parent);
 				parent.add(node);
 			}

src/main/java/ch/njol/skript/config/EntryNode.java

@@ -1,6 +1,8 @@
 package ch.njol.skript.config;
 
+import java.util.Arrays;
 import java.util.Map.Entry;
+import java.util.Objects;
 
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -52,4 +54,20 @@ String save_i() {
 		return null;
 	}
 
+	@Override
+	public boolean equals(Object object) {
+		if (!(object instanceof EntryNode other))
+			return false;
+
+		// ignores comment as changing the comment would create
+		// a new node which may change the value, leading to
+		// unexpected config changes for the user
+		return Arrays.equals(this.getPathSteps(), other.getPathSteps());
+	}
+
+	@Override
+	public int hashCode() {
+		return Arrays.hashCode(this.getPathSteps());
+	}
+
 }

src/test/java/ch/njol/skript/config/ConfigTest.java

@@ -24,27 +24,30 @@ public void testUpdateNodes() {
 
 		for (Node node : newNodes) {
 			assertTrue("Node " + node + " was not updated", updatedNodes.contains(node));
-
-			if (node instanceof EntryNode entry) {
-				assertEquals(entry.getValue(), old.get(entry.getPathSteps()));
-			}
 		}
 
 		// keeps removed/user-added nodes
-		assertEquals("true", old.getValue("outdated value"));
+		assertEquals("true", old.get(new String[] {"outdated value"}));
 		assertEquals("true", old.get("a", "outdated value"));
 
 		// adds new nodes
-		assertEquals("h.c", old.getValue("true"));
-		assertEquals("k", old.getValue("true"));
+		assertEquals("true", old.get("h", "c"));
+		assertEquals("true", old.get(new String[] {"l"}));
 
 		// keeps values of nodes
-		assertEquals("false", old.getValue("j"));
+		assertEquals("false", old.get(new String[] {"j"}));
+		assertEquals("false", old.get(new String[] {"k"}));
 
 		// doesnt duplicate nested
 		SectionNode node = (SectionNode) old.get("h");
 		assertNotNull(node);
-		assertEquals(2, node.size());
+
+		int size = 0;
+		for (Node ignored : node) { // count non-void nodes
+			size++;
+		}
+
+		assertEquals(2, size);
 	}
 
 	private Config getConfig(String name) {

src/test/java/ch/njol/skript/config/NodeTest.java

@@ -16,7 +16,7 @@ public class NodeTest {
 	public void testGetPathSteps() {
 		Config newer = getConfig("new-config");
 
-		Node node = newer.get("a.b.c");
+		Node node = newer.getNodeAt("a", "b", "c");
 
 		assertNotNull(node);
 		assertArrayEquals(new String[] {"a", "b", "c"}, node.getPathSteps());

src/test/resources/new-config.sk

@@ -21,8 +21,9 @@ h:
 # Comment h
 i: false
 j: true # value differs from old config
+k: true # Comment k
 
-k: true
+l: true
 
 x.z:
 	y: true

src/test/resources/old-config.sk

@@ -12,6 +12,7 @@ a:
 # Comment a'
 i: false
 j: false # Comment j
+k: false # Comment k
 
 outdated value: true