RFR: 8147462: URI.toURL could be more efficient for most non-opaque URIs

[Chris Hegarty]

> On 18 Jan 2016, at 16:20, Claes Redestad <claes.redestad at oracle.com> wrote:
> 
> On 2016-01-18 15:34, Chris Hegarty wrote:
>> On 18/01/16 14:09, Alan Bateman wrote:
>>> 
>>> On 17/01/2016 15:30, Claes Redestad wrote:
>>>> Hi,
>>>> 
>>>> please review this patch which might make URI.toURL more efficient
>>>> 
>>>> Webrev: http://cr.openjdk.java.net/~redestad/8147462
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8147462
>>>> 
>>>> This patch exploits the fact that URI will apply the same validation
>>>> to the URI/URL specification for any valid non-opaque URL, thus it's
>>>> safe to use the component-based URL constructor. Also, URIs
>>>> representing malformed URLs will throw an exception as specified both
>>>> before and after. A number of simple test cases to capture and
>>>> document this was added to the existing java/net/URI/URItoURL jtreg test.
>>> I think the change to URI looks okay but it would be good to include a
>>> brief comment as otherwise it will be difficult for future maintainers
>>> to understand.
>> 
>> +1.  For a long time we have been suggesting that anyone requiring
>> a URL should retrieve it from URI::toURL, so it is good to have a
>> fast(er) common path.
>> 
>> The handling of a null/empty host is a little esoteric, so a comment
>> would be good.
> 
> How about this?
> 
> http://cr.openjdk.java.net/~redestad/8147462/webrev.02 <http://cr.openjdk.java.net/~redestad/8147462/webrev.02>

Looks good Claes.

-Chris.



>> I do have a little discomfort about this change, since the toURL spec
>> specifically says it is "equivalent to evaluating the expression new URL(this.toString())". I wonder if your tests should cover this too,
>> i.e. the resulting URLs from toURL and URL(this.toString()) are equal?
> 
> The test already does this. I've just added cases that exercise some interesting cases like URIs containing /./ and quoted characters.
> 
> I restructured the exceptional tests slightly to differentiate the cases where both toURL and new URL throw MalformedURLException from cases where toURL throws IAE. It makes the test slightly more verbose but arguably a lot simpler.
> 
> However: the javadoc spec for URL(String) seems incomplete, and I might have stumbled into a trap...
> 
>     * @exception  MalformedURLException  if no protocol is specified, or an
>     *               unknown protocol is found, or {@code spec} is {@code null}.
> 
> Since this method delegates parsing to the URLStreamHandler associated with the protocol it might be overriden to throw exceptions (that will be wrapped in MalformedURLException) for a variety of reasons.
> 
> The ability for URLStreamHandler implementations to override the parseURL method seem to prevent this improvement unless we only do this for a subset of known, well-behaved URLStreamHandlers, which likely defeat the optimization by adding complexity.
> 
> Thanks!
> 
> /Claes

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/net-dev/attachments/20160118/f4d76f10/attachment-0001.html>