Fix infinite loop in MavenPomDownloader

[philippe-granet]

What's changed?

Fix an infinite loop in MavenPomDownloader

What's your motivation?

Since this MR #4472 , there is an infinite loop in MavenPomDownloader.download() when root pom have a parent.

• Add processing for nested modules with a parent version property #4472

[ErhardSiegl]

I think this fix would break projects that use a revision property and where the module structure is more than one level deep. I assume @marcel-gepardec has an example for that. The code wants to traverse to the root-pom, but apparently the condition for reaching the root is not good enough.

[timtebeek]

Thanks for the quick work here @philippe-granet ; I'll give @marcel-gepardec a little time to chime in, but ideally we work this in before tomorrow's release.

[github-actions]

Some suggestions could not be made:

• rewrite-core/src/test/java/org/openrewrite/RecipeLifecycleTest.java
  • lines 142-142

[nmck257]

Thanks for the quick work here @philippe-granet ; I'll give @marcel-gepardec a little time to chime in, but ideally we work this in before tomorrow's release.

I also ran into this infinite loop with some local testing :)
imo, if we don't land on a root fix in time for that release, then #4472 should just be reverted for the release until a thorough fix is available.
(but I am grateful for the intended enhancement; I've tried to fix the ${revision} pattern in the past and it keeps coming back to haunt me)

edit: for more detail, I fell into the infinite loop when some project pom had a dependency whose transitive dependencies led back to the same group-artifact as a project pom, but with a different version. the parent variable's initial value therefore pointed to the parent of the pom we were looking for, which happened to not be a project pom, which doomed the if-check inside the nested loop.

[ErhardSiegl]

@marcel-gepardec wont be available today before noon CET. Its unlikely that he can provide a fix today especially since the code is really complicated imho. In my opinion the code would need a more in depth rework.

[timtebeek]

Thanks for that context @ErhardSiegl ; then I propose we merge the change already, given that the same tests still pass, it fixes an issue for two folks (at least), and might only break when the module structure is deeper. That last case can then be picked up separately with a matching test.

A test for the scenarios @philippe-granet & @nmck257 encountered would also be helpful to avoid regressions at that point, but let's not hold up this fix and the release for that. Thanks all!

[nmck257]

well this is odd -- one of the tests from that PR (shouldNotThrowExceptionForModulesInModulesWithRightProperty) is already failing on my machine at main, but I see CI is happy.

I'm on windows; Java URI syntax isn't OS-specific?...

org.opentest4j.AssertionFailedError: Unexpected exception thrown: java.lang.IllegalArgumentException: Illegal character in path at index 52: file:///C:/Users/me/.m2/repository/test/test2/${test}/test2-${test}.pom

	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:152)
	at org.junit.jupiter.api.AssertDoesNotThrow.createAssertionFailedError(AssertDoesNotThrow.java:84)
	at org.junit.jupiter.api.AssertDoesNotThrow.assertDoesNotThrow(AssertDoesNotThrow.java:75)
	at org.junit.jupiter.api.AssertDoesNotThrow.assertDoesNotThrow(AssertDoesNotThrow.java:58)
	at org.junit.jupiter.api.Assertions.assertDoesNotThrow(Assertions.java:3259)
	at org.openrewrite.maven.internal.MavenPomDownloaderTest$WithNativeHttpURLConnectionAndTLS.shouldNotThrowExceptionForModulesInModulesWithRightProperty(MavenPomDownloaderTest.java:675)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: java.lang.IllegalArgumentException: Illegal character in path at index 52: file:///C:/Users/me/.m2/repository/test/test2/${test}/test2-${test}.pom
	at java.base/java.net.URI.create(URI.java:906)
	at org.openrewrite.maven.internal.MavenPomDownloader.download(MavenPomDownloader.java:560)
	at org.openrewrite.maven.internal.MavenPomDownloaderTest$WithNativeHttpURLConnectionAndTLS.lambda$shouldNotThrowExceptionForModulesInModulesWithRightProperty$5(MavenPomDownloaderTest.java:675)
	at org.junit.jupiter.api.AssertDoesNotThrow.assertDoesNotThrow(AssertDoesNotThrow.java:71)
	... 7 more
Caused by: java.net.URISyntaxException: Illegal character in path at index 52: file:///C:/Users/me/.m2/repository/test/test2/${test}/test2-${test}.pom
	at java.base/java.net.URI$Parser.fail(URI.java:2974)
	at java.base/java.net.URI$Parser.checkChars(URI.java:3145)
	at java.base/java.net.URI$Parser.parseHierarchical(URI.java:3227)
	at java.base/java.net.URI$Parser.parse(URI.java:3175)
	at java.base/java.net.URI.<init>(URI.java:623)
	at java.base/java.net.URI.create(URI.java:904)
	... 10 more

[marcel-gepardec]

@nmck257 @timtebeek I fixed the issue with the infinity loop and the failing test in

• Fix failing test and infinity loop in MavenPomDownloader #4609