Copilot commented on code in PR #1233:
URL:
https://github.com/apache/maven-javadoc-plugin/pull/1233#discussion_r4481689444
##########
src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java:
##########
@@ -826,9 +826,7 @@ protected static void invokeMaven(
.cwd(projectFile.getAbsoluteFile().getParentFile().toPath())
.arguments(arguments);
File javaHome = getJavaHome(log);
- if (javaHome != null) {
- request.environmentVariable("JAVA_HOME",
javaHome.getAbsolutePath());
- }
+ request.environmentVariable("JAVA_HOME", javaHome.getAbsolutePath());
Review Comment:
`getJavaHome(log)` can legitimately return `null` (e.g., neither `java.home`
nor `JAVA_HOME` is set, or an earlier validation fails). Calling
`javaHome.getAbsolutePath()` will then throw an NPE. Restore the null check
before setting the environment variable, or change `getJavaHome` to never
return null (e.g., throw with a clear error) and keep this call safe.
##########
src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java:
##########
@@ -4921,45 +4917,38 @@ private void addTagletsFromTagletArtifacts(List<String>
arguments) throws MavenR
tagletsPath = JavadocUtil.pruneFiles(tagletsPath);
for (String tagletJar : tagletsPath) {
- if (!tagletJar.toLowerCase(Locale.ENGLISH).endsWith(".jar")) {
- continue;
- }
-
- List<String> tagletClasses;
- try {
- tagletClasses = JavadocUtil.getTagletClassNames(new
File(tagletJar));
- } catch (IOException e) {
- if (getLog().isWarnEnabled()) {
- getLog().warn("Unable to auto-detect Taglet class names
from '" + tagletJar
- + "'. Try to specify them with <taglets/>.");
- }
- if (getLog().isDebugEnabled()) {
- getLog().debug("IOException: " + e.getMessage(), e);
- }
- continue;
- } catch (ClassNotFoundException e) {
- if (getLog().isWarnEnabled()) {
- getLog().warn("Unable to auto-detect Taglet class names
from '" + tagletJar
- + "'. Try to specify them with <taglets/>.");
- }
- if (getLog().isDebugEnabled()) {
- getLog().debug("ClassNotFoundException: " +
e.getMessage(), e);
- }
- continue;
- } catch (NoClassDefFoundError e) {
- if (getLog().isWarnEnabled()) {
- getLog().warn("Unable to auto-detect Taglet class names
from '" + tagletJar
- + "'. Try to specify them with <taglets/>.");
- }
- if (getLog().isDebugEnabled()) {
- getLog().debug("NoClassDefFoundError: " + e.getMessage(),
e);
- }
- continue;
- }
-
- if (tagletClasses != null && !tagletClasses.isEmpty()) {
- for (String tagletClass : tagletClasses) {
- addArgIfNotEmpty(arguments, "-taglet",
JavadocUtil.quotedArgument(tagletClass));
+ if (tagletJar.toLowerCase(Locale.ENGLISH).endsWith(".jar")) {
+ try {
+ List<String> tagletClasses =
JavadocUtil.getTagletClassNames(new File(tagletJar));
+ if (!tagletClasses.isEmpty()) {
Review Comment:
The prior implementation guarded `tagletClasses != null` before calling
`isEmpty()`. Unless `JavadocUtil.getTagletClassNames(...)` is guaranteed to
return a non-null list on all paths, this will introduce an NPE. Prefer
returning an empty list from `getTagletClassNames` (best), or restore the null
guard here.
##########
src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java:
##########
@@ -1489,12 +1487,8 @@ private static BufferedReader getReader(URL url,
Settings settings) throws IOExc
public void close() throws IOException {
super.close();
- if (httpMethod != null) {
- httpMethod.releaseConnection();
- }
- if (httpClient != null) {
- httpClient.close();
- }
+ httpMethod.releaseConnection();
+ httpClient.close();
Review Comment:
Removing the null checks here can introduce NPEs during `close()` if
`httpMethod`/`httpClient` were not successfully created (e.g., exception during
setup) but the reader wrapper is still constructed/closed. Restoring the null
guards avoids masking the original failure with a cleanup-time NPE.
##########
src/test/java/org/apache/maven/plugins/javadoc/JavadocReportTest.java:
##########
@@ -1019,9 +1019,7 @@ void testAuthProxy(JavadocReport mojo) throws Exception {
// 'http://commons.apache.org/logging/apidocs'"
// ) );
} finally {
- if (proxyServer != null) {
- proxyServer.stop();
- }
+ proxyServer.stop();
Review Comment:
Stopping the proxy server unconditionally in `finally` can throw an NPE if
`proxyServer` was never successfully created (e.g., failure earlier in the
test) and will mask the original exception. The previous null check should be
kept unless construction is guaranteed to succeed before entering the `try`.
##########
src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java:
##########
@@ -1131,7 +1129,7 @@ private static File getJavaHome(Log log) {
javaHome = new File(javaHomeValue);
}
- if (javaHome == null || !javaHome.exists()) {
+ if (!javaHome.exists()) {
Review Comment:
This now dereferences `javaHome` unconditionally. If `javaHome` was not set
from system properties or environment variables, this will throw an NPE. The
previous `javaHome == null || !javaHome.exists()` check (or an equivalent
guard) should be kept unless `javaHome` is guaranteed initialized on all paths.
##########
src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java:
##########
@@ -5075,7 +5062,7 @@ && isJavadocVMInitError(output)) {
+ "' dir.\n");
}
- if (output != null && !output.isEmpty()) {
+ if (output != null) {
getLog().info(output);
}
Review Comment:
This change logs `output` even when it’s an empty string, which can add
noisy blank lines to build logs. Consider keeping the non-empty check for
logging (while still allowing `output != null` where needed for subsequent
processing).
##########
src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java:
##########
@@ -3895,9 +3894,6 @@ private synchronized void resolveDependencyBundles()
throws IOException {
if (dependencyJavadocBundles == null) {
dependencyJavadocBundles =
resourceResolver.resolveDependencyJavadocBundles(getDependencySourceResolverConfig());
Review Comment:
This removes the fallback initialization when
`resolveDependencyJavadocBundles(...)` returns `null`. If that resolver can
return null (as the prior code suggested), `dependencyJavadocBundles` will
remain null and can cause downstream NPEs. Either restore the fallback to an
empty list, or update the resolver contract to always return a non-null list
and enforce it there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
