Fix URISyntaxException in MavenPomDownloader for unresolved version placeholders#7313
Fix URISyntaxException in MavenPomDownloader for unresolved version placeholders#7313
Conversation
…placeholders
When a parent POM version contains unresolved placeholders like ${revision}
and the parent isn't found locally, the download method would construct a URI
with illegal { } characters, causing URISyntaxException. Added an early check
to throw MavenDownloadingException instead, which callers already handle
gracefully by falling through to property resolution.
|
The above is in response to the following stacktrace in recipe execution With the trigger on line 589 here: |
| var downloader = new MavenPomDownloader(pomsByPath, ctx); | ||
|
|
||
| assertThrows(IllegalArgumentException.class, () -> downloader.download(gav, Objects.requireNonNull(pom.getParent()).getRelativePath(), resolvedPom, singletonList(nonexistentRepo))); | ||
| assertThrows(MavenDownloadingException.class, () -> downloader.download(gav, Objects.requireNonNull(pom.getParent()).getRelativePath(), resolvedPom, singletonList(nonexistentRepo))); |
There was a problem hiding this comment.
This test & assertion on an IllegalArgumentException was explicitly added in
| // Since the parent can't be found by GAV match either, the version still | ||
| // contains an unresolved placeholder, so download throws MavenDownloadingException | ||
| // instead of URISyntaxException from URI.create(). | ||
| assertThrows(MavenDownloadingException.class, |
There was a problem hiding this comment.
This assertion was added in
| throw new MavenDownloadingException("Unable to download POM " + gav + | ||
| ". Version contains unresolved property placeholder.", null, originalGav); |
There was a problem hiding this comment.
Copying a few interesting patterns here:
The MavenDownloadingException was nearly always caught & handled (it's a checked exception after all), but the IllegalArgumentException from not being able to construct the URL would have propagated except in rare cases. This change then ensures we're more predictable at runtime for such poms that use ${revision}.
Jenson3210
left a comment
There was a problem hiding this comment.
Easy to follow change due to the good amount of rationale behind the changes.
Not sure how I feel about the comments in "prod" code being quite long even for a descriptive piece of code
if (gav.getVersion().contains("${")) { is quite self-explanatory and would at most require 1 comment placeholders in URI would also throw or even none.
Not going to trigger a new pipeline for this only comment.

Summary
${revision}and the parent isn't found in project POMs or via relative path,MavenPomDownloader.download()would construct a URI with illegal{}characters, causingURISyntaxException.${, throwMavenDownloadingExceptioninstead of attempting to build an invalid URI.ResolvedPom.resolveParentPom()already catchMavenDownloadingExceptionfor placeholder versions and fall through to property resolution, so this is handled gracefully.Test plan
MavenPomDownloaderTestassertions updated to expectMavenDownloadingExceptioninstead ofIllegalArgumentException/ExceptionMavenPomDownloaderTestpass