Looks good! Hannes
> Am 21.11.2018 um 22:15 schrieb Jonathan Gibbons <[email protected]>: > > 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 >>>> <[email protected]>: >>>> >>>> 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 >>>> >> >
