Skip to content

feat: add relativePath validation - #12742

Open
cstamas wants to merge 6 commits into
apache:maven-3.10.xfrom
cstamas:maven-3.10.x-relativePathValidation
Open

feat: add relativePath validation#12742
cstamas wants to merge 6 commits into
apache:maven-3.10.xfrom
cstamas:maven-3.10.x-relativePathValidation

Conversation

@cstamas

@cstamas cstamas commented Aug 12, 2026

Copy link
Copy Markdown
Member

As it seems no Maven 3.x version did validate it. Validation fails if project being built (uses strict validation), while dependencies are not failing (uses minimal validation).

As it seems no Maven 3.x version did validate it. Validation fails
if project being built (uses strict validation), while dependencies
are not failing (uses minimal validation).
@cstamas cstamas added this to the 3.10.0 milestone Aug 12, 2026
@cstamas cstamas self-assigned this Aug 12, 2026

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for looking into this! A few concerns about the approach:

Not a backport of a Maven 4 validation

Maven 4's DefaultModelValidator has no validateBannedCharacters call on parent.relativePath. The only relativePath validation in Maven 4 is the 4.1.0-specific check about "only specify relativePath or groupId/artifactId" — which is unrelated. So this is a net-new validation, not a backport.

Severity is too aggressive

The PR uses Severity.ERROR with Version.BASE, meaning it would break builds that previously worked fine. Both existing validateBannedCharacters calls in 3.10.x use errOn31 (gradual severity), not a hard error. Maven 3.x has always silently handled these — the path resolution returns null and Maven falls back to repository resolution. Introducing a hard error in a minor release would be a breaking change.

Banned character list is inconsistent with the resolver

The PR bans \ (backslash), but FileModelSource.getRelatedSource() explicitly does:

relPath = relPath.replace('\\', File.separatorChar);

Backslashes are expected and handled as path separators. Banning them in validation contradicts the resolver.

Would break existing projects

POMs on Maven Central (like artemis-project-2.33.0.pom) have <relativePath>org.apache:apache</relativePath>. When Maven resolves dependencies from such POMs, it goes through getRelatedSource() — if the path doesn't resolve, Maven falls back to repository resolution. This validation would fail builds that have always worked.

What Maven 4 does instead

The approach taken in #12740 / #12741 is to catch InvalidPathException at the resolution layer and return null — matching the behavior on platforms where the path is syntactically valid but doesn't exist. No validation error, no warning — just graceful handling of broken relativePath values from POMs users can't control.

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Correction to my earlier comment: Maven 4 does reject broken relativePath at build time, but through a different mechanism — it happens in the model builder during parent resolution, not in the model validator.

For BUILD_PROJECT requests, when source.resolve(locator, parentPath) returns null (which is what happens after #12740 catches InvalidPathException), wrongParentRelativePath() fires a Severity.FATAL error:

'parent.relativePath' points at 'org.apache:apache' but no POM could be found, please verify your project structure

For dependency POMs (non-BUILD_PROJECT), it silently falls through to repository resolution — no error.

So the end result for project builds is similar to what this PR does (reject with error). The differences are:

  • Where: model builder (parent resolution) vs model validator (POM validation)
  • Severity: FATAL (Maven 4) vs ERROR (this PR)
  • Message: Maven 4's message is more helpful ("no POM could be found, please verify your project structure") vs a generic banned-characters message

The concern about the banned character list being inconsistent with the resolver (banning \ when the resolver explicitly handles it as a path separator) still stands.

@cstamas

cstamas commented Aug 12, 2026

Copy link
Copy Markdown
Member Author

Also, AI is wrong in "Would break existing projects", this is false. Maven for dependencies uses MIN validation, where this branch does not kick in at all. For projects it uses STRICT/MAX validation, and yes, the goal is to fail projects. What is released (and is used as dependency) will work, but 3.10 will prevent new, broken in this respect (invalid value in built project POM parent relativePath) to be built and deployed. User will need to fix it.

@cstamas
cstamas requested a review from gnodet August 12, 2026 18:36
@cstamas cstamas added the enhancement New feature or request label Aug 12, 2026
@cstamas
cstamas marked this pull request as ready for review August 12, 2026 19:35

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Clean, well-scoped PR that adds previously missing banned-character validation for parent.relativePath on the Maven 3.10.x branch. The implementation follows existing patterns in DefaultModelValidator and includes both positive and negative tests.

Non-blocking suggestion:

ILLEGAL_RELATIVE_PATH_FS_CHARS is hardcoded as a string literal rather than derived from ILLEGAL_FS_CHARS. The existing constants ILLEGAL_VERSION_CHARS and ILLEGAL_REPO_ID_CHARS both reference ILLEGAL_FS_CHARS directly. If ILLEGAL_FS_CHARS were ever updated, this constant could silently get out of sync. Consider:

private static final String ILLEGAL_RELATIVE_PATH_FS_CHARS =
        ILLEGAL_FS_CHARS.replace("\\", "").replace("/", "");

Note: The Maven 4 DefaultModelValidator doesn't currently have equivalent validation for parent.relativePath. If this is important enough for 3.10.x, a follow-up PR for the 4.x line may be worth considering.

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of gnodet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants