Fix unwarranted circular property definition error from CircularDefin…

…itionPreventer
mojohaus · Oct 18, 2023 · c816bf2 · c816bf2
1 parent 182eac3
commit c816bf2
Show file tree

Hide file tree

Showing 2 changed files with 48 additions and 7 deletions.
diff --git a/src/main/java/org/codehaus/mojo/properties/CircularDefinitionPreventer.java b/src/main/java/org/codehaus/mojo/properties/CircularDefinitionPreventer.java
@@ -19,11 +19,11 @@
  * under the License.
  */
 
-import java.util.HashSet;
+import java.util.HashMap;
 import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.List;
-import java.util.Set;
+import java.util.Map;
 
 class CircularDefinitionPreventer {
     private static class VisitedProperty {
@@ -39,7 +39,8 @@ private VisitedProperty(String key, String value) {
 
     private final List<VisitedProperty> entriesVisited = new LinkedList<>();
 
-    private final Set<String> keysUsed = new HashSet<>();
+    // boolean value denoted whether or not the property is deemed "resolvable"
+    private final Map<String, Boolean> keysUsed = new HashMap<>();
 
     /**
      * @param key The key.
@@ -48,11 +49,13 @@ private VisitedProperty(String key, String value) {
      */
     public CircularDefinitionPreventer visited(String key, String value) {
         entriesVisited.add(new VisitedProperty(key, value));
-        if (keysUsed.contains(key) && !isValueResolved(value)) {
+
+        if (value != null && isValueResolved(value)) {
+            keysUsed.replaceAll((k, resolvable) -> true);
+        } else if (keysUsed.containsKey(key) && Boolean.FALSE.equals(keysUsed.get(key))) {
             circularDefinition();
-        } else {
-            keysUsed.add(key);
         }
+        keysUsed.putIfAbsent(key, false);
 
         return this;
     }

diff --git a/src/test/java/org/codehaus/mojo/properties/PropertyResolverTest.java b/src/test/java/org/codehaus/mojo/properties/PropertyResolverTest.java
@@ -86,6 +86,21 @@ public void propertyIncludesAnotherPropertyMoreThanOnce() throws MojoFailureExce
         assertEquals("value value", value);
     }
 
+    @Test
+    public void propertyIncludesAnotherPropertyMoreThanOnceWithIntermediaries() throws MojoFailureException {
+        Properties properties = new Properties();
+        properties.setProperty("p1", "value");
+        properties.setProperty("p2", "${p1}");
+        properties.setProperty("p3", "${p2} ${p2}");
+        properties.setProperty("p4", "${p1} ${p2} ${p3}");
+
+        String value3 = resolver.getPropertyValue("p3", properties, new Properties());
+        String value4 = resolver.getPropertyValue("p4", properties, new Properties());
+
+        assertEquals("value value", value3);
+        assertEquals("value value value value", value4);
+    }
+
     @Test
     public void malformedPlaceholderIsLeftAsIs() {
         Properties properties = new Properties();
@@ -138,7 +153,7 @@ public void propertyDefinedAsItselfIsIllegal() {
     public void circularReferenceIsIllegal() throws MojoFailureException {
         Properties properties = new Properties();
         properties.setProperty("p1", "${p2}");
-        properties.setProperty("p2", "${p1}}");
+        properties.setProperty("p2", "${p1}");
 
         String value = null;
         try {
@@ -151,6 +166,27 @@ public void circularReferenceIsIllegal() throws MojoFailureException {
         assertNull(value);
     }
 
+    @Test
+    public void circularReferenceWithIntermediariesIsIllegal() throws MojoFailureException {
+        Properties properties = new Properties();
+        properties.setProperty("p1", "${p4}");
+        properties.setProperty("p2", "${p1}");
+        properties.setProperty("p3", "${p2}");
+        properties.setProperty("p4", "${p3}");
+
+        String value = null;
+        try {
+            value = resolver.getPropertyValue("p2", properties, new Properties());
+        } catch (IllegalArgumentException e) {
+            assertThat(e.getMessage(), containsString("p1"));
+            assertThat(e.getMessage(), containsString("p2"));
+            assertThat(e.getMessage(), containsString("p3"));
+            assertThat(e.getMessage(), containsString("p4"));
+        }
+
+        assertNull(value);
+    }
+
     @Test
     public void valueIsObtainedFromSystemProperty() {
         Properties saved = System.getProperties();
@@ -186,6 +222,7 @@ public void missingPropertyIsTolerated() {
         assertEquals("", resolver.getPropertyValue("non-existent", new Properties(), null));
     }
 
+    @Test
     public void testDefaultValueForUnresolvedPropertyWithEnabledFlag() {
         Properties properties = new Properties();
         properties.setProperty("p1", "${unknown:}");
@@ -239,6 +276,7 @@ public void testDefaultValueForUnresolvedPropertyWithEnabledFlag() {
      * with the flag disabled (default behavior) nothing gets replaced
      * ':' is treated as a regular character and part of the property name
      */
+    @Test
     public void testDefaultValueForUnresolvedPropertyWithDisabledFlag() {
         Properties properties = new Properties();
         properties.setProperty("p1", "${unknown:}");