From f9417ec15000581eb8196daf6c2df22748ce5e0a Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 12 May 2026 14:11:40 +0200 Subject: [PATCH 1/2] Add logging to YAML cycle detection Builds on #634. When a cyclic YAML alias is detected during recursive node construction, log a warn-level message including the current key path instead of silently dropping the cyclic branch. Also switch the visited-set bookkeeping from `System.identityHashCode` to an `IdentityHashMap`-backed set: reference equality is the right semantics for alias-cycle detection and eliminates the rare false positives that hash-code collisions could otherwise produce. --- .../AbstractYAMLBasedConfiguration.java | 104 +++++++++--------- .../configuration2/TestYAMLConfiguration.java | 10 ++ 2 files changed, 63 insertions(+), 51 deletions(-) diff --git a/src/main/java/org/apache/commons/configuration2/AbstractYAMLBasedConfiguration.java b/src/main/java/org/apache/commons/configuration2/AbstractYAMLBasedConfiguration.java index a1573ec1f2..3ac83c7731 100644 --- a/src/main/java/org/apache/commons/configuration2/AbstractYAMLBasedConfiguration.java +++ b/src/main/java/org/apache/commons/configuration2/AbstractYAMLBasedConfiguration.java @@ -21,7 +21,7 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -71,55 +71,6 @@ private static void addEntry(final Map map, final String key, fi } } - /** - * Creates a part of the hierarchical nodes structure of the resulting configuration. The passed in element is converted into one or multiple configuration - * nodes. (If list structures are involved, multiple nodes are returned.) - * - * @param key the key of the new node(s). - * @param elem the element to be processed. - * @param visited the set of visited objects. - * @return a list with configuration nodes representing the element - */ - private static List constructHierarchy(final String key, final Object elem, final Set visited) { - if (elem instanceof Map) { - return isVisisted(elem, visited) ? Collections.emptyList() : parseMap((Map) elem, key, visited); - } - if (elem instanceof Collection) { - return isVisisted(elem, visited) ? Collections.emptyList() : parseCollection((Collection) elem, key, visited); - } - return Collections.singletonList(new ImmutableNode.Builder().name(key).value(elem).create()); - } - - private static boolean isVisisted(final Object elem, final Set visited) { - return !visited.add(System.identityHashCode(elem)); - } - - /** - * Parses a collection structure. The elements of the collection are processed recursively. - * - * @param col the collection to be processed. - * @param key the key under which this collection is to be stored. - * @param visited the set of visited objects. - * @return a node representing this collection. - */ - private static List parseCollection(final Collection col, final String key, final Set visited) { - return col.stream().flatMap(elem -> constructHierarchy(key, elem, visited).stream()).collect(Collectors.toList()); - } - - /** - * Parses a map structure. The single keys of the map are processed recursively. - * - * @param map the map to be processed. - * @param key the key under which this map is to be stored. - * @param visited the set of visited objects. - * @return a node representing this map - */ - private static List parseMap(final Map map, final String key, final Set visited) { - final ImmutableNode.Builder subtree = new ImmutableNode.Builder().name(key); - map.forEach((k, v) -> constructHierarchy(k, v, visited).forEach(subtree::addChild)); - return Collections.singletonList(subtree.create()); - } - /** * Internal helper method to wrap an exception in a {@code ConfigurationException}. * @@ -150,6 +101,31 @@ protected AbstractYAMLBasedConfiguration(final HierarchicalConfiguration + * If an element has already been visited along the current recursion path, it is skipped and a warning is logged. This protects against cyclic YAML + * aliases that would otherwise cause infinite recursion. + *

+ * + * @param key the key of the new node(s). + * @param elem the element to be processed. + * @param visited the set of visited objects (identity-based). + * @return a list with configuration nodes representing the element + */ + @SuppressWarnings("unchecked") + private List constructHierarchy(final String key, final Object elem, final Set visited) { + if (elem instanceof Map || elem instanceof Collection) { + if (visited.add(elem)) { + return elem instanceof Map ? parseMap((Map) elem, key, visited) : parseCollection((Collection) elem, key, visited); + } + getLogger().warn(String.format("Cycle detected in YAML structure at key '%s'; skipping reference to avoid infinite recursion.", key)); + return Collections.emptyList(); + } + return Collections.singletonList(new ImmutableNode.Builder().name(key).value(elem).create()); + } + /** * Constructs a YAML map, i.e. String -> Object from a given configuration node. * @@ -169,9 +145,35 @@ protected Map constructMap(final ImmutableNode node) { * @param map the map to be processed */ protected void load(final Map map) { - final List roots = constructHierarchy(StringUtils.EMPTY, map, new HashSet<>()); + final List roots = constructHierarchy(StringUtils.EMPTY, map, Collections.newSetFromMap(new IdentityHashMap<>())); if (!roots.isEmpty()) { getNodeModel().setRootNode(roots.get(0)); } } + + /** + * Parses a collection structure. The elements of the collection are processed recursively. + * + * @param col the collection to be processed. + * @param key the key under which this collection is to be stored. + * @param visited the set of visited objects. + * @return a node representing this collection. + */ + private List parseCollection(final Collection col, final String key, final Set visited) { + return col.stream().flatMap(elem -> constructHierarchy(key, elem, visited).stream()).collect(Collectors.toList()); + } + + /** + * Parses a map structure. The single keys of the map are processed recursively. + * + * @param map the map to be processed. + * @param key the key under which this map is to be stored. + * @param visited the set of visited objects. + * @return a node representing this map + */ + private List parseMap(final Map map, final String key, final Set visited) { + final ImmutableNode.Builder subtree = new ImmutableNode.Builder().name(key); + map.forEach((k, v) -> constructHierarchy(k, v, visited).forEach(subtree::addChild)); + return Collections.singletonList(subtree.create()); + } } diff --git a/src/test/java/org/apache/commons/configuration2/TestYAMLConfiguration.java b/src/test/java/org/apache/commons/configuration2/TestYAMLConfiguration.java index 412b316479..3b42ad02b1 100644 --- a/src/test/java/org/apache/commons/configuration2/TestYAMLConfiguration.java +++ b/src/test/java/org/apache/commons/configuration2/TestYAMLConfiguration.java @@ -17,10 +17,15 @@ package org.apache.commons.configuration2; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.contains; +import static org.mockito.Mockito.atLeastOnce; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; import java.io.ByteArrayInputStream; import java.io.File; @@ -30,10 +35,12 @@ import java.io.StringWriter; import java.nio.charset.StandardCharsets; import java.util.Arrays; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import org.apache.commons.configuration2.ex.ConfigurationException; +import org.apache.commons.configuration2.io.ConfigurationLogger; import org.apache.commons.configuration2.io.FileHandler; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -72,8 +79,11 @@ void testCopyConstructor() { @Test void testCycle() throws ConfigurationException { final YAMLConfiguration configuration = new YAMLConfiguration(); + final ConfigurationLogger logger = mock(ConfigurationLogger.class); + configuration.setLogger(logger); final FileHandler handler = new FileHandler(configuration); handler.load(new File("src/test/resources/org/apache/commons/configuration2/yaml/cycle.yaml")); + verify(logger, atLeastOnce()).warn(contains("Cycle detected")); } @Test From dd752f728c1553779777987334e73787a136df9c Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 12 May 2026 14:26:39 +0200 Subject: [PATCH 2/2] fix: unused imports --- .../apache/commons/configuration2/TestYAMLConfiguration.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/test/java/org/apache/commons/configuration2/TestYAMLConfiguration.java b/src/test/java/org/apache/commons/configuration2/TestYAMLConfiguration.java index 3b42ad02b1..ec003c6c1d 100644 --- a/src/test/java/org/apache/commons/configuration2/TestYAMLConfiguration.java +++ b/src/test/java/org/apache/commons/configuration2/TestYAMLConfiguration.java @@ -17,7 +17,6 @@ package org.apache.commons.configuration2; -import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertInstanceOf; @@ -35,7 +34,6 @@ import java.io.StringWriter; import java.nio.charset.StandardCharsets; import java.util.Arrays; -import java.util.LinkedHashMap; import java.util.List; import java.util.Map;