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

[Claes Redestad]

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

>
> 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