RFR: 8190312: javadoc -link doesn't work with http: -> https: URL redirects

[Jonathan Gibbons]

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++;
---
 >                 // See:
 >                 // 
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


On 11/21/2018 12:47 PM, Jonathan Gibbons wrote:
> 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
>
>
> On 11/21/2018 03:34 AM, Hannes Wallnöfer wrote:
>> 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
>>
>>
>>
>>> Am 20.11.2018 um 01:13 schrieb Jonathan Gibbons 
>>> <jonathan.gibbons at oracle.com>:
>>>
>>> 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
>>>
>