Fix potential NPE in getJdkHome() with null guards - #185
Conversation
| .getValue(); | ||
| ToolchainModel model = toolchain.getModel(); | ||
| if (model == null) { | ||
| return null; |
There was a problem hiding this comment.
How does the calling code react when this method returns null? Have we just moved the NullPointerException up a method in the stack?
There was a problem hiding this comment.
Objects.equals(null, null) returns true, and Objects.equals(null, "value") returns false. So if we cannot determine the JDK home of the selected toolchain (null), it will not match the current JDK home (unless that is also null), and the IfSame block is simply skipped the toolchain is used as-is. No NPE is moved upstream; the null is handled safely by Objects.equals.
There was a problem hiding this comment.
Pull request overview
This PR hardens SelectJdkToolchainMojo.getJdkHome() against null/invalid configuration chains so JdkMode.IfSame comparisons don’t fail with NullPointerException (and now also avoid a potential ClassCastException).
Changes:
- Add null guards for
toolchain.getModel(),model.getConfiguration(), and missingjdkHomechild. - Use
instanceof Xpp3Dombefore casting configuration to preventClassCastException. - Add a new unit test class covering the null-chain scenarios for
getJdkHome().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/main/java/org/apache/maven/plugins/toolchain/jdk/SelectJdkToolchainMojo.java | Reworks getJdkHome() to be null-safe and type-safe when reading jdkHome from toolchain configuration. |
| src/test/java/org/apache/maven/plugins/toolchain/jdk/SelectJdkToolchainMojoTest.java | Adds unit tests for getJdkHome() null scenarios (and should be extended to cover non-Xpp3Dom config). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Test | ||
| void testGetJdkHomeWithMissingJdkHomeChild() throws Exception { | ||
| Xpp3Dom config = new Xpp3Dom("configuration"); | ||
| ToolchainModel model = new ToolchainModel(); | ||
| model.setConfiguration(config); | ||
| ToolchainPrivate toolchain = createStub(model); | ||
| assertNull(invokeGetJdkHome(toolchain)); | ||
| } |
| private Object invokeGetJdkHome(ToolchainPrivate toolchain) throws Exception { | ||
| SelectJdkToolchainMojo mojo = new SelectJdkToolchainMojo(); | ||
| Method method = SelectJdkToolchainMojo.class.getDeclaredMethod("getJdkHome", ToolchainPrivate.class); | ||
| method.setAccessible(true); | ||
| return method.invoke(mojo, toolchain); | ||
| } |
Add null guards to SelectJdkToolchainMojo.getJdkHome() to prevent
NullPointerException when:
- toolchain.getModel() returns null
- model.getConfiguration() returns null
- configuration.getChild('jdkHome') returns null
Includes unit test proving all three NPE scenarios.
8c8bc92 to
436191a
Compare
Fix potential NullPointerException in
SelectJdkToolchainMojo.getJdkHome()by adding null guards for:toolchain.getModel()returning nullmodel.getConfiguration()returning nullconfiguration.getChild("jdkHome")returning nullUses
instanceof Xpp3Domto also prevent ClassCastException if configuration is not Xpp3Dom.Includes unit test with
ToolchainPrivatestubs that prove all three NPE scenarios against the original code.fixes #168