You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "gnodet (via GitHub)" <gi...@apache.org> on 2026/08/08 01:33:55 UTC
[PR] Structured BuilderProblem pipeline for DiagnosticCollector [maven]
gnodet opened a new pull request, #12702:
URL: https://github.com/apache/maven/pull/12702
## Summary
Follow-up to the build report / warning mode chain. This PR implements the structured `BuilderProblem` pipeline for the 4 pathways identified in the foundation work, plus a full migration of the `PluginValidationManager` interface.
### Commit 1: Pipe structured BuilderProblems into DiagnosticCollector (Pathways 1–4)
**Pathway 2 — Plugin parameter validation** (`AbstractMavenPluginParametersValidator`): 3 validators now create `BuilderProblem` with structured key, severity, and suggestion, and pipe through `PluginValidationManager` → `DiagnosticCollector`.
**Pathway 3 — Plugin dependency validation** (`AbstractMavenPluginDependenciesValidator`): 4 validators now create `BuilderProblem` with structured key and pipe through `PluginValidationManager` → `DiagnosticCollector`.
**Pathway 4 — Plugin manager Contextualizable check** (`DefaultMavenPluginManager`): creates `BuilderProblem` with key `plugin-validation:contextualizable` and pipes through `PluginValidationManager` → `DiagnosticCollector`.
### Commit 2: Migrate PluginValidationManager to native BuilderProblem API
- Changed `PluginValidationManager` interface: 3 abstract methods now accept `BuilderProblem` instead of `String`
- Added `@Deprecated(since = "4.1.0", forRemoval = true)` String-based default methods as backward-compat adapters
- Updated all 9 call sites across 8 files to create `BuilderProblem` natively
## PR chain
- #12694 → #12695 → #12697 → #12698 → **this PR**
- #12699 also targets #12698
## Test plan
- [x] `mvn test -pl impl/maven-core` — tests pass
- [ ] CI
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Structured BuilderProblem pipeline for DiagnosticCollector [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #12702:
URL: https://github.com/apache/maven/pull/12702#discussion_r3743198689
##########
impl/maven-core/src/main/java/org/apache/maven/project/collector/DefaultProjectsSelector.java:
##########
@@ -100,4 +112,26 @@ public List<MavenProject> selectProjects(List<File> files, MavenExecutionRequest
Review Comment:
This `toBuilderProblem(ModelProblem)` method is an exact duplicate of the one in `DefaultMaven.java`. If the conversion logic changes, both copies must be updated in lockstep. Consider extracting to a shared utility method.
##########
impl/maven-core/src/main/java/org/apache/maven/DefaultMaven.java:
##########
@@ -660,6 +670,28 @@ private Result<? extends ProjectDependencyGraph> buildGraph(MavenSession session
return graphResult;
}
+ /**
+ * Converts a compat {@link ModelProblem} to the Maven 4 {@link BuilderProblem} API,
+ * preserving source, line, column, severity, and message.
+ */
+ private static BuilderProblem toBuilderProblem(ModelProblem problem) {
+ BuilderProblem.Severity severity =
+ switch (problem.getSeverity()) {
Review Comment:
The diagnostic key is generated as `"model:" + problem.getMessage().hashCode()`. Using `String.hashCode()` for deduplication keys is fragile — hash collisions would cause unrelated problems to be silently deduplicated. The same pattern appears in `DefaultProjectsSelector` and the deprecated adapters in `PluginValidationManager`.
Elsewhere in this PR, human-readable keys are used (e.g. `"plugin-validation:contextualizable"`, `"plugin-validation:maven2-plugin"`). Consider using a more collision-resistant approach here too — e.g., incorporating the problem source and a truncated/normalized message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Structured BuilderProblem pipeline for DiagnosticCollector [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #12702:
URL: https://github.com/apache/maven/pull/12702#discussion_r3792747117
##########
apache-maven/src/assembly/maven/bin/mvn.cmd:
##########
@@ -275,6 +275,8 @@ if "%~1"=="--debug" (
set "MAVEN_MAIN_CLASS=org.apache.maven.cling.MavenShellCling"
) else if "%~1"=="--up" (
set "MAVEN_MAIN_CLASS=org.apache.maven.cling.MavenUpCling"
+) else if "%~1"=="--log" (
Review Comment:
**[Medium — Windows parity gap]** The Unix `mvn` script strips routing flags (`--debug`, `--yjp`, `--enc`, `--shell`, `--up`, `--log`) from `$@` before exec (lines 316-327), but the Windows `.cmd` passes `%*` unmodified. Running `mvn --log validate` on Windows will pass `--log` through to Commons CLI, where it can collide with `--log-file` via prefix matching.
Windows batch's `%*` cannot be modified by `shift`, so equivalent stripping logic would need a `for` loop to rebuild the argument list.
##########
impl/maven-core/src/main/java/org/apache/maven/DefaultMaven.java:
##########
@@ -660,6 +670,28 @@ private Result<? extends ProjectDependencyGraph> buildGraph(MavenSession session
return graphResult;
}
+ /**
+ * Converts a compat {@link ModelProblem} to the Maven 4 {@link BuilderProblem} API,
+ * preserving source, line, column, severity, and message.
+ */
+ private static BuilderProblem toBuilderProblem(ModelProblem problem) {
+ BuilderProblem.Severity severity =
+ switch (problem.getSeverity()) {
+ case FATAL -> BuilderProblem.Severity.FATAL;
+ case ERROR -> BuilderProblem.Severity.ERROR;
+ default -> BuilderProblem.Severity.WARNING;
+ };
+ return BuilderProblem.builder()
+ .source(problem.getSource())
+ .lineNumber(problem.getLineNumber())
+ .columnNumber(problem.getColumnNumber())
+ .exception(problem.getException())
+ .message(problem.getMessage())
+ .severity(severity)
+ .key("model:" + problem.getMessage().hashCode())
Review Comment:
**[Low — Fragile dedup key]** `String.hashCode()` is a 32-bit hash that can produce collisions (e.g., `"Aa"` and `"BB"` both hash to 2112). If two different model problems collide, one gets silently deduplicated. Consider using the full message string or a stronger hash for the key.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org