RFR: JDK-8161230 ClassLoader: add resource methods returning java.util.stream.Stream

[Mandy Chung]

> On Sep 3, 2016, at 12:55 PM, Patrick Reinhart <patrick at reini.net> wrote:
> 
> Hi Mandy,
> 
> The current changes can be found here:
> 
> http://cr.openjdk.java.net/~reinhapa/reviews/8161230/webrev.02
> 
> 
> On 02.09.2016 23:11, Mandy Chung wrote:
>>> On Sep 2, 2016, at 2:50 AM, Patrick Reinhart <patrick at reini.net>
>>>  wrote:
>>> 
>>> Updated the existing 
>>> http://cr.openjdk.java.net/~reinhapa/reviews/8161230/webrev.01
>> ClassLoader::resources returning Stream<URL> is a good addition. 
>>  
>> 1386      * {@code IOException} occur getting the next resource element, it must be 
>> 1387      * wrapped into an {@code UncheckedIOException}.
>> 
>> This has been mentioned in the paragraph above. This can be dropped from @apiNote. And add:
>> 
>> @throws UncheckedIOException if I/O errors occur
>> 
> There was already an discussion about this with Alan Bateman:
> 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-August/042792.html
> 

I sample a few existing APIs and they document UncheckedIOException in the method description rather than @throws as Alan described.

> So I do not know is the best solution for this…

Your method description already covers it.  It’s fine.  line 1386-1387 in @apiNote is redundant that I still think can be taken out.

>> 
>> I have reservation in adding ClassLoader::systemResources static method.
>> 
>> ClassLoader::getSystemResources is a convenient method that is equivalent to calling ClassLoader::getSystemClassLoader().getResources(), since the system class loader is never null.  This method includes the resources in the application’s classpath.
>> 
>> With the new ClassLoader::getPlatformClassLoader() method, a better way to get the “system” resources from JDK (not the ones from the classpath), it can call:
>>    ClassLoader::getPlatformClassLoader().resources();
>> 
>> To get a Stream of URL of the resources visible to system class loader, it can call the new resources method:
>>    ClassLoader::getSystemClassLoader().resources();
>> 
>> So it doesn’t seem that the proposed systemResources static method is necessary.
>> 
> 
> Hmm... We may have overlooked this, when we started by just looking into the API changes in general. I did those with Paul Sandoz in more deep in the 
> following thread: 
> 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-August/042752.html
> 

> and this proposal is based on the "go" from Paul here:
> 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-August/043012.html
> 

I file a bug to update the spec ClassLoader::getSystemClassLoader to return non-null:
   https://bugs.openjdk.java.net/browse/JDK-8165563

>> I suggest to combine the two tests in a single test, ResourcesStreamTest and add @bug JBS-id
>> 
>> ResourcesSuccessCase
>> 
>>   46         List<URL> resources = stream.collect(Collectors.toList());
>>   52         URL url = resources.get(0);
>> 
>> You can check the size first.  This can be simplified to use filter/findFirst to file the expected URL (yes need to call cl.resources again that is not an issue).
>> 
> 
> I have done this also in the new revision

testFailure method can be simplified to
    long count = cl.resources(“the name”).count();
    if (count != 1)
        throw new Exception("expected resource is null or empty");

    cl.resources("the name”)
      .filter(url -> "file:/somefile".equals(url.toExternalForm())
      .findFirst()
      .orElseThrow(…)

Mandy