Prevent NPE in inheritance assembler - #12632
Conversation
… is root path or artifactId is null
elharo
changed the title
elharo
changed the title
Removed assertNotNull checks for assembler in tests.
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
The production code fix for the NPE in getChildPathAdjustment() is correct and minimal — the getFileName() != null guard handles root paths where getFileName() returns null, with a sensible fallback to artifactId. However, the test rewrite has a regression issue.
Deleted existing test coverage (high severity)
The PR completely replaces the test file content, deleting testPluginConfiguration() and the entire XML-based test infrastructure (testInheritance helper, getModel, getPom, xmlunit comparison). These tests were added in commit d58c96bc36 by hboutemy specifically to validate reportSet inheritance in the Maven 4 (impl) model builder.
While the compat test at compat/maven-model-builder/.../DefaultInheritanceAssemblerTest.java still uses the XML resources, it exercises the compat (Maven 3) assembler, not the impl (Maven 4) DefaultInheritanceAssembler which uses InheritanceModelMerger — a different code path.
The new NPE regression tests are valuable and should be added alongside the existing tests, not used to replace them.
Minor: trivial assertions (low severity)
The assertNotNull(assembler) calls in each test method only verify that @BeforeEach ran — they don't test the assembler's behavior.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of gnodet
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR fixes a NullPointerException in DefaultInheritanceAssembler.getChildPathAdjustment() when the child model’s project directory is the filesystem root (so getFileName() returns null) and/or when artifactId is null, and adds regression tests to cover these scenarios.
Changes:
- Added a null check for
child.getProjectDirectory().getFileName()before calling.toString(). - Replaced prior XML-based inheritance tests with focused regression tests asserting no NPE for the identified edge cases.
- Introduced a test case covering a combined “null artifactId + root project directory” scenario with modules configured.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultInheritanceAssembler.java | Adds a guard to prevent NPE when project directory is root and getFileName() is null. |
| impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultInheritanceAssemblerTest.java | Adds regression tests asserting assembling inheritance does not throw for null artifactId and root project directory cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void testAssembleWithNullArtifactIdDoesNotThrowNpe() { | ||
| Model parent = Model.newBuilder() | ||
| .modelVersion("4.0.0") | ||
| .groupId("test") | ||
| .artifactId("parent") | ||
| .version("1.0") | ||
| .build(); | ||
|
|
||
| public void testInheritance(String baseName) throws Exception { | ||
| testInheritance(baseName, false); | ||
| testInheritance(baseName, true); | ||
| } | ||
| Model child = Model.newBuilder() | ||
| .modelVersion("4.0.0") | ||
| .groupId("test") | ||
| .version("1.0") | ||
| .build(); | ||
|
|
||
| public void testInheritance(String baseName, boolean fromRepo) throws Exception { | ||
| Model parent = getModel(baseName + "-parent"); | ||
| Model child = getModel(baseName + "-child"); | ||
| assertDoesNotThrow(() -> assembler.assembleModelInheritance(child, parent, null, null)); | ||
| } |
| .pomFile(Paths.get("/pom.xml")) | ||
| .build(); |
| .modules(List.of("../child/pom.xml")) | ||
| .build(); |
Restore XML-based inheritance tests alongside new NPE tests
fixes #12603
Bug
DefaultInheritanceAssembler.getChildPathAdjustment()throws NPE when:child.getProjectDirectory().getFileName()returns null (root path like/)child.getArtifactId()returns nullFix
child.getProjectDirectory().getFileName()before calling.toString()Testing
Added
DefaultInheritanceAssemblerTestwith three test cases:getFileName()returns null)Co-authored-by: elharo