From 0dd3af9a68a7e4558ba05e6dca44a57567b8e09f Mon Sep 17 00:00:00 2001 From: Rsl1122 Date: Sat, 5 Jan 2019 20:21:10 +0200 Subject: [PATCH] ConfigNode copy now preserves the original properly Fixed a possible future issue where tree up-traversal is broken if a node in a tree is passed to a copy operation. This was because the parent node was updated in the process of copy if no old node was present. Fixed by creating a new node when copying instead of reusing the object being copied --- .../system/settings/config/ConfigNode.java | 36 ++++++++++----- .../settings/config/ConfigNodeTest.java | 46 ++++++++++++++++++- 2 files changed, 69 insertions(+), 13 deletions(-) diff --git a/Plan/common/src/main/java/com/djrapitops/plan/system/settings/config/ConfigNode.java b/Plan/common/src/main/java/com/djrapitops/plan/system/settings/config/ConfigNode.java index 7084449e4..ece41903d 100644 --- a/Plan/common/src/main/java/com/djrapitops/plan/system/settings/config/ConfigNode.java +++ b/Plan/common/src/main/java/com/djrapitops/plan/system/settings/config/ConfigNode.java @@ -91,12 +91,16 @@ public class ConfigNode { String key = parts[0]; String leftover = parts[1]; + // Add a new child ConfigNode child; if (!childNodes.containsKey(key)) { child = addChild(new ConfigNode(key, newParent, null)); } else { child = childNodes.get(key); } + + // If the path ends return the leaf node + // Otherwise continue recursively. return leftover.isEmpty() ? child : child.addNode(leftover); } throw new IllegalArgumentException("Can not add a node with empty path"); @@ -123,6 +127,13 @@ public class ConfigNode { updateParent(null); } + /** + * Add a new child ConfigNode. + * + * @param child ConfigNode to add. + * If from another config tree, the parent is 'cut', which breaks the old tree traversal. + * @return Return the node given, now part of this tree. + */ protected ConfigNode addChild(ConfigNode child) { getNode(child.key).ifPresent(ConfigNode::remove); childNodes.put(child.key, child); @@ -274,37 +285,38 @@ public class ConfigNode { } public void copyMissing(ConfigNode from) { + // Override comment conditionally if (comment.size() < from.comment.size()) { comment = from.comment; } + // Override value conditionally if (value == null && from.value != null) { value = from.value; } + // Copy all nodes from 'from' for (String key : from.nodeOrder) { ConfigNode newChild = from.childNodes.get(key); - if (childNodes.containsKey(key)) { - ConfigNode oldChild = childNodes.get(key); - oldChild.copyMissing(newChild); - } else { - addChild(newChild); - } + // Copy values recursively to children + ConfigNode created = addNode(key); + created.copyMissing(newChild); } } public void copyAll(ConfigNode from) { + // Override comment and value unconditionally. comment = from.comment; value = from.value; + + // Copy all nodes from 'from' for (String key : from.nodeOrder) { ConfigNode newChild = from.childNodes.get(key); - if (childNodes.containsKey(key)) { - ConfigNode oldChild = childNodes.get(key); - oldChild.copyAll(newChild); - } else { - addChild(newChild); - } + + // Copy values recursively to children + ConfigNode created = addNode(key); + created.copyAll(newChild); } } diff --git a/Plan/common/src/test/java/com/djrapitops/plan/system/settings/config/ConfigNodeTest.java b/Plan/common/src/test/java/com/djrapitops/plan/system/settings/config/ConfigNodeTest.java index bd89e928f..69c5dd4a4 100644 --- a/Plan/common/src/test/java/com/djrapitops/plan/system/settings/config/ConfigNodeTest.java +++ b/Plan/common/src/test/java/com/djrapitops/plan/system/settings/config/ConfigNodeTest.java @@ -36,6 +36,7 @@ class ConfigNodeTest { private static final String STRING_NODE_WITH_DOUBLE_QUOTES = "String_node_with_double_quotes"; private static final String FIRST_LEVEL = "1st_level"; private static final String SECOND_LEVEL = "2nd_level"; + private static final String THIRD_LEVEL = "3rd_level"; private ConfigNode testTree; @@ -46,8 +47,10 @@ class ConfigNodeTest { testTree.addChild(new ConfigNode(STRING_NODE_WITH_QUOTES, testTree, "\"'String'\"")); testTree.addChild(new ConfigNode(STRING_NODE_WITH_DOUBLE_QUOTES, testTree, "'\"String\"'")); ConfigNode emptyNode = new ConfigNode(FIRST_LEVEL, testTree, null); + ConfigNode secondLevel = new ConfigNode(SECOND_LEVEL, emptyNode, "String"); testTree.addChild(emptyNode); - emptyNode.addChild(new ConfigNode(SECOND_LEVEL, emptyNode, "String")); + emptyNode.addChild(secondLevel); + secondLevel.addChild(new ConfigNode(THIRD_LEVEL, secondLevel, "3")); assertTrue(testTree.childNodes.containsKey(SIMPLE_STRING_NODE), "Tree construction failed, addChild does not work."); } @@ -288,4 +291,45 @@ class ConfigNodeTest { testTree.set(SIMPLE_STRING_NODE, adding); assertTrue(testTree.getNode(SIMPLE_STRING_NODE + "." + SECOND_LEVEL).isPresent()); } + + @Test + void copyAllOverridesAllValues() { + ConfigNode overridingWith = testTree.getNode(FIRST_LEVEL).orElseThrow(() -> new AssertionError("Fail")); + ConfigNode added = testTree.addNode("Test." + SECOND_LEVEL + "." + THIRD_LEVEL); + added.set("ORIGINAL"); + ConfigNode node = testTree.getNode("Test").orElseThrow(() -> new AssertionError("Fail")); + node.copyAll(overridingWith); + + String original = testTree.getString(FIRST_LEVEL); + String copied = testTree.getString("Test"); + assertEquals(original, copied); + + original = testTree.getString(FIRST_LEVEL + "." + SECOND_LEVEL); + copied = testTree.getString("Test." + SECOND_LEVEL); + assertEquals(original, copied); + + original = testTree.getString(FIRST_LEVEL + "." + SECOND_LEVEL + "." + THIRD_LEVEL); + copied = testTree.getString("Test." + SECOND_LEVEL + "." + THIRD_LEVEL); + assertEquals(original, copied); + } + + @Test + void copyDefaultAddsProperValues() { + ConfigNode overridingWith = testTree.getNode(FIRST_LEVEL).orElseThrow(() -> new AssertionError("Fail")); + ConfigNode added = testTree.addNode("Test." + SECOND_LEVEL + "." + THIRD_LEVEL); + added.set("ORIGINAL"); + ConfigNode node = testTree.getNode("Test").orElseThrow(() -> new AssertionError("Fail")); + node.copyMissing(overridingWith); + + String original = testTree.getString(FIRST_LEVEL); + String copied = testTree.getString("Test"); + assertEquals(original, copied); + + original = testTree.getString(FIRST_LEVEL + "." + SECOND_LEVEL); + copied = testTree.getString("Test." + SECOND_LEVEL); + assertEquals(original, copied); + + copied = testTree.getString("Test." + SECOND_LEVEL + "." + THIRD_LEVEL); + assertEquals("ORIGINAL", copied); + } } \ No newline at end of file