Discussion:
RFR: 8190312: javadoc -link doesn't work with http: -> https: URL redirects
Jonathan Gibbons
2018-11-20 00:13:09 UTC
Permalink
Please review a fix for javadoc such that it will follow redirected
URLs, including http: to https:, for URLs given on the command line with
-link.

An unconditional warning is given if it is found that a URL is redirected.

The change is in Extern.java, with the primary change being the addition
of a new method `open(URL)` that allows redirection and generates the
warning message.  The other changes are cosmetic cleanup, partly to
aggregate some URL handling methods together at the end of the file, and
partly to follow IDE suggestions.

The bigger part of the work is a new test, with two test cases.

The first test case is a "real world" test case that uses
docs.oracle.com if it is available.  Running this test case may require
proxies to be set in order to work as intended. The test checks if the
site can be accessed, and skips the test case if not.

The second case sets up two transient webservers, with an HttpServer
that redirects all requests to an HttpsServer. The test case uses SSL
credentials used in similar tests in the main test/jdk test suite. This
test case is always enabled. The test case verifies that the warning
message is generated as expected and that the generated files do -not-
use the redirected URLs. This is to avoid baking in an assumption that
all the files will be redirected. In other words, the redirection is
only used for reading the element-list or package-list files.

JBS: https://bugs.openjdk.java.net/browse/JDK-8190312
Webrev: http://cr.openjdk.java.net/~jjg/8190312/webrev.00/

-- Jon
Hannes Wallnöfer
2018-11-21 11:34:41 UTC
Permalink
I don’t think interpreting any 3xx HTTP response is a good idea.

After some research consulting the Mozilla documentation[1] on HTTP status code it seems like:

304 (not modified) has different meaning and shouldn’t be returned unless we sent a conditional request.
305 (use proxy) has been deprecated since HTTP 1.0 and Location would be the proxy server, not the requested document.

On the other hand, there are new status codes 307 and 308 that are kind of „fixed“ versions of 302 and 301.

So we probably should consider the following status codes as redirects:

300, 301, 302, 303, 307, 308

[1]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status

Otherwise looks good.

Hannes
Please review a fix for javadoc such that it will follow redirected URLs, including http: to https:, for URLs given on the command line with -link.
An unconditional warning is given if it is found that a URL is redirected.
The change is in Extern.java, with the primary change being the addition of a new method `open(URL)` that allows redirection and generates the warning message. The other changes are cosmetic cleanup, partly to aggregate some URL handling methods together at the end of the file, and partly to follow IDE suggestions.
The bigger part of the work is a new test, with two test cases.
The first test case is a "real world" test case that uses docs.oracle.com if it is available. Running this test case may require proxies to be set in order to work as intended. The test checks if the site can be accessed, and skips the test case if not.
The second case sets up two transient webservers, with an HttpServer that redirects all requests to an HttpsServer. The test case uses SSL credentials used in similar tests in the main test/jdk test suite. This test case is always enabled. The test case verifies that the warning message is generated as expected and that the generated files do -not- use the redirected URLs. This is to avoid baking in an assumption that all the files will be redirected. In other words, the redirection is only used for reading the element-list or package-list files.
JBS: https://bugs.openjdk.java.net/browse/JDK-8190312
Webrev: http://cr.openjdk.java.net/~jjg/8190312/webrev.00/
-- Jon
Jonathan Gibbons
2018-11-21 20:47:55 UTC
Permalink
Thanks for the research.

I did cut-n-paste part of that code from mainline JDK code, but I'm also
OK to use your more refined suggestion.  I'll post another webrev.

-- Jon
Post by Hannes Wallnöfer
I don’t think interpreting any 3xx HTTP response is a good idea.
304 (not modified) has different meaning and shouldn’t be returned unless we sent a conditional request.
305 (use proxy) has been deprecated since HTTP 1.0 and Location would be the proxy server, not the requested document.
On the other hand, there are new status codes 307 and 308 that are kind of „fixed“ versions of 302 and 301.
300, 301, 302, 303, 307, 308
[1]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status
Otherwise looks good.
Hannes
Please review a fix for javadoc such that it will follow redirected URLs, including http: to https:, for URLs given on the command line with -link.
An unconditional warning is given if it is found that a URL is redirected.
The change is in Extern.java, with the primary change being the addition of a new method `open(URL)` that allows redirection and generates the warning message. The other changes are cosmetic cleanup, partly to aggregate some URL handling methods together at the end of the file, and partly to follow IDE suggestions.
The bigger part of the work is a new test, with two test cases.
The first test case is a "real world" test case that uses docs.oracle.com if it is available. Running this test case may require proxies to be set in order to work as intended. The test checks if the site can be accessed, and skips the test case if not.
The second case sets up two transient webservers, with an HttpServer that redirects all requests to an HttpsServer. The test case uses SSL credentials used in similar tests in the main test/jdk test suite. This test case is always enabled. The test case verifies that the warning message is generated as expected and that the generated files do -not- use the redirected URLs. This is to avoid baking in an assumption that all the files will be redirected. In other words, the redirection is only used for reading the element-list or package-list files.
JBS: https://bugs.openjdk.java.net/browse/JDK-8190312
Webrev: http://cr.openjdk.java.net/~jjg/8190312/webrev.00/
-- Jon
Jonathan Gibbons
2018-11-21 21:15:25 UTC
Permalink
New webrev posted:
http://cr.openjdk.java.net/~jjg/8190312/webrev.01/

The new webrev is based on the latest jdk/jdk. After resolving minor
merge conflicts, the significant change is this one, which replaces the
"if" with a "switch" for selected values.

465,479c472,494
<                 if (stat >= 300 && stat <= 307 && stat != 306 &&
<                         stat != HttpURLConnection.HTTP_NOT_MODIFIED) {
<                     URL base = http.getURL();
<                     String loc = http.getHeaderField("Location");
<                     URL target = null;
<                     if (loc != null) {
<                         target = new URL(base, loc);
<                     }
<                     http.disconnect();
<                     if (target == null || redirects >= 5) {
<                         throw new IOException("illegal URL redirect");
<                     }
<                     redir = true;
<                     conn = target.openConnection();
<                     redirects++;
---
                 //
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status
                 //
https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#3xx_Redirection
                 switch (stat) {
                     case 300: // Multiple Choices
                     case 301: // Moved Permanently
                     case 302: // Found (previously Moved Temporarily)
                     case 303: // See Other
                     case 307: // Temporary Redirect
                     case 308: // Permanent Redirect
                         URL base = http.getURL();
                         String loc = http.getHeaderField("Location");
                         URL target = null;
                         if (loc != null) {
                             target = new URL(base, loc);
                         }
                         http.disconnect();
                         if (target == null || redirects >= 5) {
                             throw new IOException("illegal URL
redirect");
                         }
                         redir = true;
                         conn = target.openConnection();
                         redirects++;
-- Jon
Thanks for the research.
I did cut-n-paste part of that code from mainline JDK code, but I'm
also OK to use your more refined suggestion.  I'll post another webrev.
-- Jon
Post by Hannes Wallnöfer
I don’t think interpreting any 3xx HTTP response is a good idea.
After some research consulting the Mozilla documentation[1] on HTTP
304 (not modified) has different meaning and shouldn’t be returned
unless we sent a conditional request.
305 (use proxy) has been deprecated since HTTP 1.0 and Location would
be the proxy server, not the requested document.
On the other hand, there are new status codes 307 and 308 that are
kind of „fixed“ versions of 302 and 301.
300, 301, 302, 303, 307, 308
[1]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status
Otherwise looks good.
Hannes
Am 20.11.2018 um 01:13 schrieb Jonathan Gibbons
Please review a fix for javadoc such that it will follow redirected
URLs, including http: to https:, for URLs given on the command line
with -link.
An unconditional warning is given if it is found that a URL is redirected.
The change is in Extern.java, with the primary change being the
addition of a new method `open(URL)` that allows redirection and
generates the warning message.  The other changes are cosmetic
cleanup, partly to aggregate some URL handling methods together at
the end of the file, and partly to follow IDE suggestions.
The bigger part of the work is a new test, with two test cases.
The first test case is a "real world" test case that uses
docs.oracle.com if it is available.  Running this test case may
require proxies to be set in order to work as intended. The test
checks if the site can be accessed, and skips the test case if not.
The second case sets up two transient webservers, with an HttpServer
that redirects all requests to an HttpsServer. The test case uses
SSL credentials used in similar tests in the main test/jdk test
suite. This test case is always enabled. The test case verifies that
the warning message is generated as expected and that the generated
files do -not- use the redirected URLs. This is to avoid baking in
an assumption that all the files will be redirected. In other words,
the redirection is only used for reading the element-list or
package-list files.
JBS: https://bugs.openjdk.java.net/browse/JDK-8190312
Webrev: http://cr.openjdk.java.net/~jjg/8190312/webrev.00/
-- Jon
Hannes Wallnöfer
2018-11-22 11:33:51 UTC
Permalink
Looks good!

Hannes
Post by Jonathan Gibbons
http://cr.openjdk.java.net/~jjg/8190312/webrev.01/
The new webrev is based on the latest jdk/jdk. After resolving minor merge conflicts, the significant change is this one, which replaces the "if" with a "switch" for selected values.
465,479c472,494
< if (stat >= 300 && stat <= 307 && stat != 306 &&
< stat != HttpURLConnection.HTTP_NOT_MODIFIED) {
< URL base = http.getURL();
< String loc = http.getHeaderField("Location");
< URL target = null;
< if (loc != null) {
< target = new URL(base, loc);
< }
< http.disconnect();
< if (target == null || redirects >= 5) {
< throw new IOException("illegal URL redirect");
< }
< redir = true;
< conn = target.openConnection();
< redirects++;
---
// https://developer.mozilla.org/en-US/docs/Web/HTTP/Status
// https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#3xx_Redirection
switch (stat) {
case 300: // Multiple Choices
case 301: // Moved Permanently
case 302: // Found (previously Moved Temporarily)
case 303: // See Other
case 307: // Temporary Redirect
case 308: // Permanent Redirect
URL base = http.getURL();
String loc = http.getHeaderField("Location");
URL target = null;
if (loc != null) {
target = new URL(base, loc);
}
http.disconnect();
if (target == null || redirects >= 5) {
throw new IOException("illegal URL redirect");
}
redir = true;
conn = target.openConnection();
redirects++;
-- Jon
Thanks for the research.
I did cut-n-paste part of that code from mainline JDK code, but I'm also OK to use your more refined suggestion. I'll post another webrev.
-- Jon
Post by Hannes Wallnöfer
I don’t think interpreting any 3xx HTTP response is a good idea.
304 (not modified) has different meaning and shouldn’t be returned unless we sent a conditional request.
305 (use proxy) has been deprecated since HTTP 1.0 and Location would be the proxy server, not the requested document.
On the other hand, there are new status codes 307 and 308 that are kind of „fixed“ versions of 302 and 301.
300, 301, 302, 303, 307, 308
[1]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status
Otherwise looks good.
Hannes
Please review a fix for javadoc such that it will follow redirected URLs, including http: to https:, for URLs given on the command line with -link.
An unconditional warning is given if it is found that a URL is redirected.
The change is in Extern.java, with the primary change being the addition of a new method `open(URL)` that allows redirection and generates the warning message. The other changes are cosmetic cleanup, partly to aggregate some URL handling methods together at the end of the file, and partly to follow IDE suggestions.
The bigger part of the work is a new test, with two test cases.
The first test case is a "real world" test case that uses docs.oracle.com if it is available. Running this test case may require proxies to be set in order to work as intended. The test checks if the site can be accessed, and skips the test case if not.
The second case sets up two transient webservers, with an HttpServer that redirects all requests to an HttpsServer. The test case uses SSL credentials used in similar tests in the main test/jdk test suite. This test case is always enabled. The test case verifies that the warning message is generated as expected and that the generated files do -not- use the redirected URLs. This is to avoid baking in an assumption that all the files will be redirected. In other words, the redirection is only used for reading the element-list or package-list files.
JBS: https://bugs.openjdk.java.net/browse/JDK-8190312
Webrev: http://cr.openjdk.java.net/~jjg/8190312/webrev.00/
-- Jon
Loading...