feat: add relativePath validation - #12742
Conversation
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).
gnodet
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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) vsERROR(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.
|
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. |
gnodet
left a comment
There was a problem hiding this comment.
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
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).