gnodet-bot commented on code in PR #1213:
URL:
https://github.com/apache/maven-plugin-tools/pull/1213#discussion_r4301868520
##########
maven-plugin-tools-api/src/test/java/org/apache/maven/tools/plugin/javadoc/JavadocSiteTest.java:
##########
@@ -124,6 +129,38 @@ void
classLinkFromPackageAndClassNameWithExternalJavadocRequiringModules() throw
assertEquals(baseUri.resolve("java.base/java/lang/String.html"),
site.createLink("java.lang", "String"));
}
+ @Test
+ void resolvesBaseUrlBeforeRequestingPackageList() throws Exception {
Review Comment:
⚠️ **Bug: test doesn't reproduce the bug it claims to test**
The test server serves `/javadoc/package-list` with HTTP 200 (line 142–148
of the new test). The _old_ code calls `url.resolve("package-list")` where `url
= http://localhost:PORT/javadoc/`; `URI.resolve("package-list")` yields
`http://localhost:PORT/javadoc/package-list`, which the server answers with
200. The test therefore passes equally with and without the fix — it does not
demonstrate that the redirect resolution is necessary.
To actually reproduce the bug, the test server should return 404 for
`/javadoc/package-list` and serve the file only under the post-redirect path
(`/javadoc/index.html/package-list` or similar), so that the old code fails but
the new code (which follows the redirect first) succeeds.
##########
maven-plugin-tools-api/src/main/java/org/apache/maven/tools/plugin/javadoc/JavadocSite.java:
##########
@@ -119,23 +119,27 @@ class JavadocSite {
* @throws IOException
*/
JavadocSite(final URI url, final Settings settings) throws IOException {
+ URI redirectedUrl = getRedirectedUrl(url, settings);
Review Comment:
💡 **Unconditional extra HTTP round-trip per Javadoc site**
`getRedirectedUrl` is called here for every `http`/`https` Javadoc site on
every build, even when the URL doesn't actually redirect. For projects linking
many external Javadoc sites this adds one complete HTTP request (possibly a
full redirect chain) per site per build invocation.
Consider one of:
- Deferring the redirect resolution until a `FileNotFoundException` is
encountered on `package-list`/`element-list` (only pay the cost when there's
evidence of a redirect problem).
- Caching the resolved URL across builds.
- Using a single-request strategy: issue the HEAD request with
redirect-following enabled and record the final URL; this removes the extra
round-trip when no redirect exists by reusing what would already be the first
real request.
##########
maven-plugin-tools-api/src/test/java/org/apache/maven/tools/plugin/javadoc/JavadocSiteTest.java:
##########
@@ -20,14 +20,19 @@
import java.io.BufferedReader;
import java.io.IOException;
+import java.io.OutputStream;
+import java.net.InetSocketAddress;
import java.net.URI;
import java.net.URISyntaxException;
+import java.nio.charset.StandardCharsets;
import java.util.AbstractMap;
import java.util.Collections;
import java.util.Map;
import java.util.regex.Pattern;
import java.util.stream.Stream;
+import com.sun.net.httpserver.HttpServer;
Review Comment:
💡 **`com.sun.net.httpserver.HttpServer` is a JDK-internal API**
`com.sun.net.httpserver` lives in module `jdk.httpserver`, which is not a
guaranteed part of every JDK distribution (e.g. stripped / embedded builds may
omit it). It is also an internal (`com.sun.*`) package subject to future access
restrictions.
A portable alternative in the existing dependency graph would be
`MockWebServer` (OkHttp, already available in many test scopes) or a simple
`ServerSocket`-based stub. If this JDK stays the minimum target and the module
is guaranteed to be present, add a comment explaining the constraint.
##########
maven-plugin-tools-api/src/main/java/org/apache/maven/tools/plugin/javadoc/JavadocSite.java:
##########
@@ -200,6 +204,29 @@ static Map<String, String> getPackageListWithModules(final
URI url, final Settin
}
}
+ static URI getRedirectedUrl(final URI url, final Settings settings) throws
IOException {
+ if ("file".equals(url.getScheme())) {
+ return url;
+ }
+
+ try (CloseableHttpClient httpClient = createHttpClient(settings,
url.toURL())) {
+ HttpGet httpMethod = new HttpGet(url.toString());
Review Comment:
⚠️ **Use `HttpHead` instead of `HttpGet`; consume the entity if `HttpGet` is
kept**
The response body is never read or consumed — only
`getStatusLine().getStatusCode()` and `getRedirectLocations()` are used. This
has two problems:
1. **Wrong HTTP method**: `HttpHead` is semantically correct for "I only
need the final URL after redirects" — it avoids downloading the full Javadoc
index page (which can be large).
2. **Connection leak**: If `HttpGet` is kept, the response entity must be
explicitly consumed with `EntityUtils.consume(response.getEntity())` before the
`try-with-resources` closes the client; otherwise the underlying TCP connection
cannot be returned to the pool.
```suggestion
HttpHead httpMethod = new HttpHead(url.toString());
```
##########
maven-plugin-tools-api/src/main/java/org/apache/maven/tools/plugin/javadoc/JavadocSite.java:
##########
@@ -200,6 +204,29 @@ static Map<String, String> getPackageListWithModules(final
URI url, final Settin
}
}
+ static URI getRedirectedUrl(final URI url, final Settings settings) throws
IOException {
Review Comment:
💡 **No `SocketTimeoutException` retry, unlike `getReader()`**
The existing `getReader()` method retries on `SocketTimeoutException` to
tolerate transient network failures. `getRedirectedUrl` has no equivalent
handling — a single timeout aborts the build. For consistency, consider
wrapping the HTTP call in the same retry logic used by `getReader()`.
--
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]
