Handle missing package metadata in model ids#12146
Conversation
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Thanks for addressing this long-standing issue (MNG-6932). The fix is correct in principle -- guarding against Class#getPackage() returning null prevents the NullPointerException.
A few observations below.
Claude Code on behalf of Guillaume Nodet
|
|
||
| public static final String DEFAULT_LIFECYCLE_MODELID = "org.apache.maven:maven-core:" | ||
| + DefaultLifecycleRegistry.class.getPackage().getImplementationVersion() | ||
| + getImplementationVersion(DefaultLifecycleRegistry.class) |
There was a problem hiding this comment.
DEFAULT_LIFECYCLE_MODELID is a static final constant whose value feeds directly into DEFAULT_LIFECYCLE_INPUT_LOCATION on the next line. Before this PR, when getPackage() returned null, the class failed at load time with an ExceptionInInitializerError (wrapping an NPE).
With this PR, the class loads successfully, but the model id will contain the literal string "null" via string concatenation (e.g. "org.apache.maven:maven-core:null:default-lifecycle-bindings"), which may silently produce confusing diagnostics later. The other three call sites have the same characteristic.
It might be worth returning a fallback like "unknown" instead of null, so the concatenated model id remains parseable/debuggable:
| + getImplementationVersion(DefaultLifecycleRegistry.class) | |
| + getImplementationVersion(DefaultLifecycleRegistry.class) |
(the suggestion is a no-op to keep this visible -- the actual change would be in getImplementationVersion to return "unknown" instead of null)
| static String getImplementationVersion(Class<?> clazz) { | ||
| Package packageInfo = clazz.getPackage(); | ||
| return packageInfo != null ? packageInfo.getImplementationVersion() : null; | ||
| } |
There was a problem hiding this comment.
The identical getImplementationVersion helper is copy-pasted verbatim into four separate classes (DefaultReportingConverter, both DefaultSuperPomProvider variants, and here). Consider extracting it into a shared utility to avoid the duplication. If the compat module intentionally avoids depending on impl internals, the duplication there may be acceptable, but the two impl/ modules could still share one copy.
|
|
||
| @Test | ||
| void returnsNullImplementationVersionWhenClassHasNoPackage() { | ||
| assertNull(DefaultReportingConverter.getImplementationVersion(int.class)); |
There was a problem hiding this comment.
Using int.class (a primitive type whose getPackage() returns null) is a clever way to trigger the null-package path. However, it would strengthen the test to also verify the case where getPackage() is non-null but getImplementationVersion() itself returns null -- which is the far more common scenario in IDE and test-classpath environments (classes loaded without a JAR manifest). A regular class from the test classpath would exercise that path.
Also, all four test classes are identical except for the class under test -- a parameterized test or a shared test helper could reduce the boilerplate.
2 participants
Fixes #8403.
This avoids NullPointerException when Maven code builds internal model ids in environments where Class#getPackage() returns null, while preserving the previous model id contents when package metadata exists but the implementation version is absent. The fallback is applied to the remaining implementation-version based model ids in the current codebase.
Validation: