Repository navigation
Handle missing project index in forked executions - #12911
Conversation
Signed-off-by: ulofiai <309826581+ulofiai@users.noreply.github.com>
elharo
left a comment
There was a problem hiding this comment.
Has this happened in production? It needs a test.
|
I don't have a confirmed production occurrence. I picked this up from #12600, where the missing project ID can cause an NPE through auto-unboxing. I'll add a regression test that reproduces the missing project-index entry and verifies that we get a |
gnodet
left a comment
There was a problem hiding this comment.
✅ Looks Good
The fix correctly addresses issue #12600 — changing int to Integer and adding a null check before auto-unboxing prevents the NPE when a forked execution references a project not in the reactor. The thrown LifecycleExecutionException uses the correct constructor and produces a descriptive error message naming the offending projectId.
Agree with @elharo's request for a regression test. The test should set up a MojoExecution with forked executions referencing a projectId absent from the reactor's ProjectIndex, then assert that executeForkedExecutions throws LifecycleExecutionException (not NullPointerException) with the expected message.
Note: A pre-existing design observation (not introduced by this PR): ProjectIndex exposes its internal maps via mutable getters (getProjects(), getIndices()), and those maps are mutated within the forked execution loop. This is fine in the current single-threaded usage but worth noting for future hardening.
📋 PR Metadata
| Aspect | Current | Suggested |
|---|---|---|
| Labels | (none) | bug |
| Milestone | (none) | 4.1.0 |
🔀 Backport Status
int index = projectIndex.getIndices().get(projectId)) exists in maintenance branches:
maven-3.9.x(line 425) — no backport PR foundmaven-3.10.x(line 434) — no backport PR foundmaven-4.0.x(line 441) — no backport PR found
Consider creating backport PRs once this fix (with test) is merged.
🤖 This review was generated by ForgeBot. Feedback? File an issue on the ForgeBot repo.
Test originally written for PR apache#12907 (our parallel fix for apache#12600). Contributed here since this PR has the better exception constructor (includes MojoExecution and MavenProject context). See: apache#12907 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
left a comment
There was a problem hiding this comment.
LGTM — the null guard with the richer LifecycleExecutionException(String, MojoExecution, MavenProject) constructor is the right approach. Test coverage added and passing.
* fix: handle missing forked project index Signed-off-by: ulofiai <309826581+ulofiai@users.noreply.github.com> * Add test for forked execution project index null guard Test originally written for PR #12907 (our parallel fix for #12600). Contributed here since this PR has the better exception constructor (includes MojoExecution and MavenProject context). See: #12907 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Signed-off-by: ulofiai <309826581+ulofiai@users.noreply.github.com> Co-authored-by: ulofiai <309826581+ulofiai@users.noreply.github.com> Co-authored-by: Guillaume Nodet <gnodet@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
The enriched messages dropped the phrase MojoExecutorForkedExecutionTest asserts on, breaking maven-core after apache#12911 landed on master. Restore the original wording and keep the parallel-build/extension context appended.
* fix: handle missing forked project index Signed-off-by: ulofiai <309826581+ulofiai@users.noreply.github.com> * Add test for forked execution project index null guard Test originally written for PR #12907 (our parallel fix for #12600). Contributed here since this PR has the better exception constructor (includes MojoExecution and MavenProject context). See: #12907 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Signed-off-by: ulofiai <309826581+ulofiai@users.noreply.github.com> Co-authored-by: ulofiai <309826581+ulofiai@users.noreply.github.com> Co-authored-by: Guillaume Nodet <gnodet@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
#12918) * fix: handle missing forked project index * Add test for forked execution project index null guard Test originally written for PR #12907 (our parallel fix for #12600). Contributed here since this PR has the better exception constructor (includes MojoExecution and MavenProject context). See: #12907 --------- Signed-off-by: ulofiai <309826581+ulofiai@users.noreply.github.com> Co-authored-by: 2ako <ulofi@protonmail.com> Co-authored-by: ulofiai <309826581+ulofiai@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Backport of #12911 (merged to master) to maven-3.10.x. Guards against NullPointerException from auto-unboxing when ProjectIndex.getIndices().get() returns null for a project not in the reactor. Throws descriptive LifecycleExecutionException instead. Test adapted for 3.10.x (Java 8 APIs, @Inject field injection). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Backport of #12911 (merged to master) to maven-3.10.x. Guards against NullPointerException from auto-unboxing when ProjectIndex.getIndices().get() returns null for a project not in the reactor. Throws descriptive LifecycleExecutionException instead. Test adapted for 3.10.x (Java 8 APIs, @Inject field injection). Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Fixes #12600.
Avoids auto-unboxing a missing reactor project index and reports a LifecycleExecutionException that identifies the missing project instead of a NullPointerException.