[MSHARED-1290] Fix PropertyUtils cycle detection results in false positives

src/main/java/org/apache/maven/shared/filtering/PropertyUtils.java

@@ -218,10 +218,10 @@ private static String getPropertyValue(String k, Properties p, Logger logger) {
                 // else prefix the original string with the
                 // resolved property ( so it can be parsed further )
                 // taking recursion into account.
-                if (nv == null || nv.equals(k) || k.equals(nk)) {
+                if (nv == null || k.equals(nk)) {
                     ret.append("${").append(nk).append("}");
                 } else {
-                    v = nv + v;
+                    v = nv + v.replace("${" + nk + "}", nv);
                 }
             }
         }

src/test/java/org/apache/maven/shared/filtering/PropertyUtilsTest.java

@@ -30,6 +30,7 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
 
 /**
  * @author Olivier Lamy
@@ -53,9 +54,11 @@ public void testBasic() throws Exception {
             writer.flush();
         }
 
-        Properties prop = PropertyUtils.loadPropertyFile(basicProp, false, false);
+        Logger logger = mock(Logger.class);
+        Properties prop = PropertyUtils.loadPropertyFile(basicProp, false, false, logger);
         assertTrue(prop.getProperty("key").equals("gani_man"));
         assertTrue(prop.getProperty("ghost").equals("${non_existent}"));
+        verifyNoInteractions(logger);
     }
 
     public void testSystemProperties() throws Exception {
@@ -71,8 +74,10 @@ public void testSystemProperties() throws Exception {
             writer.flush();
         }
 
-        Properties prop = PropertyUtils.loadPropertyFile(systemProp, false, true);
+        Logger logger = mock(Logger.class);
+        Properties prop = PropertyUtils.loadPropertyFile(systemProp, false, true, logger);
         assertTrue(prop.getProperty("key").equals(System.getProperty("user.dir")));
+        verifyNoInteractions(logger);
     }
 
     public void testException() throws Exception {
@@ -88,16 +93,18 @@ public void testException() throws Exception {
         }
     }
 
-    public void testloadpropertiesFile() throws Exception {
+    public void testLoadPropertiesFile() throws Exception {
         File propertyFile = new File(getBasedir() + "/src/test/units-files/propertyutils-test.properties");
         Properties baseProps = new Properties();
         baseProps.put("pom.version", "realVersion");
 
-        Properties interpolated = PropertyUtils.loadPropertyFile(propertyFile, baseProps);
+        Logger logger = mock(Logger.class);
+        Properties interpolated = PropertyUtils.loadPropertyFile(propertyFile, baseProps, logger);
         assertEquals("realVersion", interpolated.get("version"));
         assertEquals("${foo}", interpolated.get("foo"));
         assertEquals("realVersion", interpolated.get("bar"));
         assertEquals("none filtered", interpolated.get("none"));
+        assertWarn(logger, "Circular reference between properties detected: foo => foo");
     }
 
     /**
@@ -120,7 +127,6 @@ public void testCircularReferences() throws IOException {
         }
 
         Logger logger = mock(Logger.class);
-
         Properties prop = PropertyUtils.loadPropertyFile(basicProp, null, logger);
         assertEquals("${test2}", prop.getProperty("test"));
         assertEquals("${test2}", prop.getProperty("test2"));
@@ -151,7 +157,6 @@ public void testCircularReferences3Vars() throws IOException {
         }
 
         Logger logger = mock(Logger.class);
-
         Properties prop = PropertyUtils.loadPropertyFile(basicProp, null, logger);
         assertEquals("${test2}", prop.getProperty("test"));
         assertEquals("${test3}", prop.getProperty("test2"));
@@ -171,4 +176,48 @@ private void assertWarn(Logger mock, String... expected) {
             assertTrue(messages.contains(str));
         }
     }
+
+    public void testNonCircularReferences1Var3Times() throws IOException {
+        File basicProp = new File(testDirectory, "non-circular.properties");
+
+        if (basicProp.exists()) {
+            basicProp.delete();
+        }
+
+        basicProp.createNewFile();
+        try (FileWriter writer = new FileWriter(basicProp)) {
+            writer.write("depends=p1 >= ${version}, p2 >= ${version}, p3 >= ${version}\n");
+            writer.write("version=1.2.3\n");
+            writer.flush();
+        }
+
+        Logger logger = mock(Logger.class);
+        Properties prop = PropertyUtils.loadPropertyFile(basicProp, null, logger);
+        assertEquals("p1 >= 1.2.3, p2 >= 1.2.3, p3 >= 1.2.3", prop.getProperty("depends"));
+        assertEquals("1.2.3", prop.getProperty("version"));
+        verifyNoInteractions(logger);
+    }
+
+    public void testNonCircularReferences2Vars2Times() throws IOException {
+        File basicProp = new File(testDirectory, "non-circular.properties");
+
+        if (basicProp.exists()) {
+            basicProp.delete();
+        }
+
+        basicProp.createNewFile();
+        try (FileWriter writer = new FileWriter(basicProp)) {
+            writer.write("test=${test2} ${test3} ${test2} ${test3}\n");
+            writer.write("test2=${test3} ${test3}\n");
+            writer.write("test3=test\n");
+            writer.flush();
+        }
+
+        Logger logger = mock(Logger.class);
+        Properties prop = PropertyUtils.loadPropertyFile(basicProp, null, logger);
+        assertEquals("test test test test test test", prop.getProperty("test"));
+        assertEquals("test test", prop.getProperty("test2"));
+        assertEquals("test", prop.getProperty("test3"));
+        verifyNoInteractions(logger);
+    }
 }