This is an automated email from the ASF dual-hosted git repository. ppkarwasz pushed a commit to branch fix/cycle-logging in repository https://gitbox.apache.org/repos/asf/commons-configuration.git
commit f9417ec15000581eb8196daf6c2df22748ce5e0a Author: Piotr P. Karwasz <[email protected]> AuthorDate: Tue May 12 14:11:40 2026 +0200 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 a1573ec1f..3ac83c773 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.ArrayList; 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 @@ public class AbstractYAMLBasedConfiguration extends BaseHierarchicalConfiguratio } } - /** - * 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<ImmutableNode> constructHierarchy(final String key, final Object elem, final Set<Object> visited) { - if (elem instanceof Map) { - return isVisisted(elem, visited) ? Collections.emptyList() : parseMap((Map<String, Object>) elem, key, visited); - } - if (elem instanceof Collection) { - return isVisisted(elem, visited) ? Collections.emptyList() : parseCollection((Collection<Object>) elem, key, visited); - } - return Collections.singletonList(new ImmutableNode.Builder().name(key).value(elem).create()); - } - - private static boolean isVisisted(final Object elem, final Set<Object> 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<ImmutableNode> parseCollection(final Collection<Object> col, final String key, final Set<Object> 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<ImmutableNode> parseMap(final Map<String, Object> map, final String key, final Set<Object> 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 @@ public class AbstractYAMLBasedConfiguration extends BaseHierarchicalConfiguratio initLogger(new ConfigurationLogger(getClass())); } + /** + * 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.) + * <p> + * 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. + * </p> + * + * @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<ImmutableNode> constructHierarchy(final String key, final Object elem, final Set<Object> visited) { + if (elem instanceof Map || elem instanceof Collection) { + if (visited.add(elem)) { + return elem instanceof Map ? parseMap((Map<String, Object>) elem, key, visited) : parseCollection((Collection<Object>) 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 @@ public class AbstractYAMLBasedConfiguration extends BaseHierarchicalConfiguratio * @param map the map to be processed */ protected void load(final Map<String, Object> map) { - final List<ImmutableNode> roots = constructHierarchy(StringUtils.EMPTY, map, new HashSet<>()); + final List<ImmutableNode> 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<ImmutableNode> parseCollection(final Collection<Object> col, final String key, final Set<Object> 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<ImmutableNode> parseMap(final Map<String, Object> map, final String key, final Set<Object> 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 412b31647..3b42ad02b 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.StringReader; 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 @@ public class TestYAMLConfiguration { @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
