Skip to content

Honor empty <relativePath/> in MavenPomDownloader#6875

Merged
Jenson3210 merged 4 commits intoopenrewrite:mainfrom
Jenson3210:fix-empty-relativepath-handling
Mar 5, 2026
Merged

Honor empty <relativePath/> in MavenPomDownloader#6875
Jenson3210 merged 4 commits intoopenrewrite:mainfrom
Jenson3210:fix-empty-relativepath-handling

Conversation

@Jenson3210
Copy link
Contributor

@Jenson3210 Jenson3210 commented Mar 4, 2026

Summary

  • Distinguish between omitted <relativePath> (defaults to ../pom.xml) and explicit empty <relativePath/> (skip local parent lookup) in MavenPomDownloader
  • Fix affects both getParentWithinProject() and the relative-path resolution block in download()

Problem

When a POM declares <relativePath/> (empty self-closing tag), Maven skips local parent lookup entirely and goes straight to remote repositories (DefaultModelBuilder.java#L1048). OpenRewrite's MavenPomDownloader was treating empty relativePath the same as omitted because both null and "" pass through StringUtils.isBlank(), causing it to default to ../pom.xml in both cases.

Solution

Check for empty string explicitly before the isBlank fallback:

  • null (element absent) → default to ../pom.xml
  • "" (explicit empty <relativePath/>) → skip local parent lookup
  • non-blank string → use that path

Test plan

  • Existing tests pass (MavenPomDownloaderTest, MavenParserTest, ChangeParentPomTest, ResolvedPomTest)

  • New test emptyRelativePathSkipsLocalParentLookup verifies the fix

  • Fixes moderneinc/customer-requests#1950

When a POM declares `<relativePath/>` (empty self-closing tag), Maven
skips local parent lookup entirely and goes straight to remote
repositories. OpenRewrite's MavenPomDownloader was treating this the
same as an omitted `<relativePath>` (which defaults to `../pom.xml`)
because both `null` and `""` pass through `StringUtils.isBlank()`.

This fix distinguishes between:
- `null` (element absent) → default to `../pom.xml`
- `""` (explicit empty element) → skip local parent lookup
- non-blank string → use that path

Affects both `getParentWithinProject()` and the relative-path block
in `download()`.

Fixes moderneinc/customer-requests#1950
@Jenson3210
Copy link
Contributor Author

@MBoegers I could use your recent investigations to get bit more insights on this one.
Is this a valid change?

…eholder versions

The existing emptyRelativePathSkipsLocalParentLookup test has a gap: the
parent is found via GAV iteration (stage 2) before the path-based lookup
(stage 3) is ever reached, so it passes with or without the fix.

This new test uses a placeholder version ${my.version} that can't be
resolved in stage 2 (the property isn't defined). Without the fix,
stage 3 would default the empty relativePath to ".." and find the parent
locally (the "${" in the version relaxes the version check). With the
fix, stage 3 is correctly skipped and the download falls through to the
remote repo.
@MBoegers
Copy link
Contributor

MBoegers commented Mar 5, 2026

Added another test:

The test exercises these steps through download():

  1. Stage 1 exact GAV lookup in projectPomsByGav: no match because ${my.version} != 1.0.0
  2. Stage 2 iterate all project POMs, find test:parent, try to resolve the placeholder: fails because the parent doesn’t define my.version, so ${my.version} stays unresolved and doesn’t match 1.0.0
  3. Stage 3 path-based lookup using relativePath:
  • Without fix: empty relativePath defaults to .., resolves child/../pom.xml, finds the parent, and the ${ in the version relaxes the version check → returns local POM (wrong)
  • With fix: empty relativePath means “skip local lookup” → block is skipped entirely
    Remote download — falls through to fetch from the nonexistent repo → throws exception

The assertThrows(Exception.class) confirms stage 3 was skipped — if it wasn’t, the local POM would be returned and no exception would be thrown.

Copy link
Contributor

@MBoegers MBoegers left a comment

Choose a reason for hiding this comment

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

LGTM with the additional test we should have both tracks covered

@github-project-automation github-project-automation bot moved this from In Progress to Ready to Review in OpenRewrite Mar 5, 2026
The previous test passed because the parent was found by GAV match
(step 1 in download()), making the relativePath irrelevant. The new
test uses a placeholder version ${revision} that can only be resolved
via relativePath-based local lookup (step 3), proving that
<relativePath/> correctly skips that step.
…dy covers it

The emptyRelativePathSkipsPathBasedLookupWithPlaceholderVersion test
uses the same placeholder-version approach and is a subset of the
emptyRelativePathSkipsLocalParentLookup test, which additionally
verifies that null relativePath DOES find the parent locally.
@Jenson3210 Jenson3210 marked this pull request as ready for review March 5, 2026 10:11
@Jenson3210 Jenson3210 merged commit cb33ce3 into openrewrite:main Mar 5, 2026
1 check passed
@github-project-automation github-project-automation bot moved this from Ready to Review to Done in OpenRewrite Mar 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants