Skip to content

Fix URISyntaxException in MavenPomDownloader for unresolved version placeholders #7313

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
timtebeek merged 2 commits into from
Apr 8, 2026
Merged

Fix URISyntaxException in MavenPomDownloader for unresolved version placeholders #7313

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d528a78
Fix URISyntaxException in MavenPomDownloader for unresolved property …
timtebeek Apr 8, 2026
a3d3322
Apply suggestion from @timtebeek
timtebeek Apr 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions rewrite-maven/src/main/java/org/openrewrite/maven/internal/MavenPomDownloader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,16 @@ public Pom download(GroupArtifactVersion gav,
gav = resolveNamedVersion(gav, containingPom, repositories, ctx);
String versionMaybeDatedSnapshot = datedSnapshotVersion(gav, containingPom, repositories, ctx);
gav = handleSnapshotTimestampVersion(gav);

// If the version still contains unresolved property placeholders (e.g. ${revision}),
// downloading from any repository will never succeed and would cause URISyntaxException
// due to illegal '{' / '}' characters in the constructed URI.
//noinspection DataFlowIssue
Comment thread
timtebeek marked this conversation as resolved.
Outdated
if (gav.getVersion().contains("${")) {
throw new MavenDownloadingException("Unable to download POM " + gav +
". Version contains unresolved property placeholder.", null, originalGav);
Comment on lines +563 to +564
Copy link
Copy Markdown
Member Author

@timtebeek timtebeek Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method is called from different paths; now checking how each treats the exception signature change from IllegalArgumentException to MavenDownloadingException:

Image

Copy link
Copy Markdown
Member Author

@timtebeek timtebeek Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copying a few interesting patterns here:

rewrite/rewrite-gradle/src/main/java/org/openrewrite/gradle/internal/AddDependencyVisitor.java

Lines 291 to 292 in 24b27e1

} catch (MavenDownloadingException | MavenDownloadingExceptions | IllegalArgumentException e) {
return Markup.warn(buildScript, e);

rewrite/rewrite-gradle/src/main/java/org/openrewrite/gradle/UpgradeDependencyVersion.java

Lines 782 to 786 in a636e5c

} catch (MavenDownloadingException ignored) {
// If we can't download the POM, we can still just upgrade the dependency version
}
return dependency.withDeclaredVersion(selectedVersion).getTree();

rewrite/rewrite-gradle/src/main/java/org/openrewrite/gradle/RemoveRedundantDependencyVersions.java

Lines 192 to 193 in b8612b5

} catch (MavenDownloadingException e) {
throw new RuntimeException(e);

rewrite/rewrite-maven/src/main/java/org/openrewrite/maven/RemoveRedundantDependencyVersions.java

Lines 288 to 289 in e0d7e9a

} catch (Exception e) {
return false;

rewrite/rewrite-maven/src/main/java/org/openrewrite/maven/UpgradeDependencyVersion.java

Lines 217 to 218 in 22fd6f5

} catch (MavenDownloadingException e) {
document = e.warn(document);

rewrite/rewrite-maven/src/main/java/org/openrewrite/maven/trait/MavenDependency.java

Lines 111 to 124 in de200b2

try {
// This is a best effort attempt to see if the pom is there anyway, in spite of the
// fact that it's not in the metadata. Usually it won't be, only in situations like the
// MapR repository mentioned in the comment above will it be.
Pom pom = new MavenPomDownloader(emptyMap(), ctx,
mrr.getMavenSettings(), mrr.getActiveProfiles()).download(new GroupArtifactVersion(groupId, artifactId, ((ExactVersion) versionComparator).getVersion()),
null, null, mrr.getPom().getRepositories());
if (pom.getGav().getVersion().equals(exactVersion) &&
!exactVersion.equals(finalVersion) &&
versionComparator.compare(finalVersion, finalVersion, exactVersion) <= 0) {
return exactVersion;
}
} catch (MavenDownloadingException e) {
return null;

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}.

}

Iterable<MavenRepository> normalizedRepos = distinctNormalizedRepositories(repositories, containingPom, gav.getVersion());

Timer.Sample sample = Timer.start();
Expand Down
9 changes: 5 additions & 4 deletions rewrite-maven/src/test/java/org/openrewrite/maven/internal/MavenPomDownloaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1108,7 +1108,7 @@ void shouldThrowExceptionForModulesInModulesWithNoRightProperty() {

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)));
Copy link
Copy Markdown
Member Author

@timtebeek timtebeek Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test & assertion on an IllegalArgumentException was explicitly added in

}

@Test
Expand Down Expand Up @@ -1197,9 +1197,10 @@ void emptyRelativePathSkipsLocalParentLookup() {
assertDoesNotThrow(() -> downloader.download(requestedGav, null, resolvedPom, singletonList(nonexistentRepo)));

// With empty relativePath (<relativePath/>), step 3 is skipped entirely.
// Since the parent can't be found by GAV match either, and the remote
// repo doesn't exist, download fails — proving local lookup was skipped.
assertThrows(Exception.class,
// 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,
Copy link
Copy Markdown
Member Author

@timtebeek timtebeek Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion was added in

() -> downloader.download(requestedGav, "", resolvedPom, singletonList(nonexistentRepo)));
}

Expand Down
Loading
⚡