Fix NPE in DefaultModelBuilder#11922
Conversation
|
@utafrali @gnodet @cstamas Thank you for the fix and reviewing! |
| if (inputModel.getParent() != null && inputModel.getParent().getRelativePath() == null) { | ||
| String relPath; | ||
| if (parentModel.getPomFile() != null && isBuildRequest()) { | ||
| if (parentModel.getPomFile() != null && inputModel.getPomFile() != null && isBuildRequest()) { |
There was a problem hiding this comment.
Just try to understand if it is as expected / a correct behavior to fall back inputModel's relPath to ".." when its getPomFile() is null.
Non-binding +1.
There was a problem hiding this comment.
This comment has been resolved, but my "Resolve comment" is rejected by 401 (I have no idea why...). Please kindly move forward. Thank you!
|
Yep, that's expected - when getPomFile() is null, we fall back to '..' to keep a valid path for the model hierarchy. |
|
no problem, moving forward with the PR |
|
@gnodet Just a kindly ping to check if you have time to review this change. Thank you! |
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of Guillaume Nodet
The fix is correct — inputModel.getPomFile() is dereferenced on the next line without a null check, and the stack trace in #11919 confirms this is the exact crash site. Adding it to the existing guard condition is the right approach since the else branch already handles this case by defaulting relPath to "..".
Two observations:
-
No test coverage. This is a one-line defensive null check so the risk is low, but a unit test that exercises
readEffectiveModelwith anullpom file on the input model (simulating the cyclonedx-maven-plugin scenario) would prevent regressions. -
Minor: guard ordering. The
isBuildRequest()check is cheaper than the twogetPomFile()null checks. Moving it first would short-circuit earlier in the common non-build-request path, though this is purely cosmetic for a code path that is not hot.
| if (inputModel.getParent() != null && inputModel.getParent().getRelativePath() == null) { | ||
| String relPath; | ||
| if (parentModel.getPomFile() != null && isBuildRequest()) { | ||
| if (parentModel.getPomFile() != null && inputModel.getPomFile() != null && isBuildRequest()) { |
There was a problem hiding this comment.
The null check is correct and matches the existing parentModel.getPomFile() != null guard.
Nit: since isBuildRequest() is a simple boolean and both getPomFile() calls involve virtual dispatch, you could reorder for marginally faster short-circuiting in the non-build-request case:
| if (parentModel.getPomFile() != null && inputModel.getPomFile() != null && isBuildRequest()) { | |
| if (isBuildRequest() && parentModel.getPomFile() != null && inputModel.getPomFile() != null) { |
Not a blocker — just a thought.
gnodet
left a comment
There was a problem hiding this comment.
LGTM — the null check is correct and matches the existing guard pattern.
Claude Code on behalf of Guillaume Nodet
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fixes #11919
Got an NPE in DefaultModelBuilder when reading the effective model with Maven 4 RC5. The code was already checking if parentModel has a pom file, but then it was calling getPomFile() on inputModel without checking if it was null. Added the null check before using it.
mvn verifyto make sure basic checks pass.