RFR 8163798: Add a versionedStream method to JarFile

[Peter Levart]

Hi Steve,

Here are my observations. I haven't followed the development of 
multi-release jar files very closely, so I might be missing some quirks, 
but I speak here from the knowledge I get from the javadocs...

Javadoc for the new method mentions public and non-public classes:

"@{code JarEntries}
      * containing non-public classes in the
      * versioned directory that corresponds to the version returned by
      * {@link JarFile#getVersion()} are included in the stream because 
they are
      * typically accessed by the public classes."

Why? The logic of multi-release jar files is indifferent towards the 
access attributes of class files. It never parses the class file to 
interpret the access attributes of the containing class. The class-level 
javadoc of JarFile does mention "public classes", but I view this just 
as a recommendation how they should be constructed.

The logic of multi-release jar files is indifferent towards the types of 
resources contained in the jar entries too, as far as I can see. Your 
logic in versionedStream() is not. The proposed new method is the only 
place in JarFile and ZipFile where the check for ".class" suffix of the 
entry name is performed. Why this special treatement of class files? I 
also don't understand why the versioned stream should contain the 
untranslated base versioned directories. For example, the following 
entries for the v10 stream in the test:

"META-INF/versions/9/"
"META-INF/versions/10/"

All other resources and sub-directories in the META-INF/versions/XX/... 
directories are "normalized" (meaning that the versioned prefix is 
stripped from the names in the returned entries). Normalizing above 
directories would translate them to "root" directory, which is never 
included as an entry of a normal jar or zip file. So I think they could 
simply be skipped in the resulting versioned stream. Couldn't they?

Considering all of the above, I think versionedStream() method could be 
much simpler. For example, something like the following:


     public Stream<JarEntry> versionedStream() {
         return
             !isMultiRelease()
             ? stream()
             : stream()
                 .flatMap(je -> {
                     String name = je.getName();
                     if (name.startsWith(META_INF_VERSIONS)) {
                         // versioned entry
                         if (name.length() > META_INF_VERSIONS.length()) {
                             // potentially real entry
                             String vStr = 
name.substring(META_INF_VERSIONS.length());
                             int offset = vStr.indexOf('/');
                             int version = 0;
                             if (offset >= 0) try {
                                 version = 
Integer.parseInt(vStr.substring(0, offset));
                             } catch (NumberFormatException e) { /* 
ignore */ }
                             if (version <= 0) {
                                 throw new RuntimeException(
                                     "Can't obtain version from 
multi-release jar entry: "
                                     + name);
                             }
                             return (version <= versionMajor && 
vStr.length() > offset + 1)
                                    // normalized name of real entry
                                    ? Stream.of(vStr.substring(offset + 1))
                                    // version > versionMajor or
                                    // META-INF/versions/<version>/ dir 
- ignored
                                    : Stream.empty();
                         } else {
                             // META-INF/versions/ dir - ignored
                             return Stream.empty();
                         }
                     } else { // base entry
                         return Stream.of(name);
                     }
                 })
                 .distinct()
                 .map(this::getJarEntry);
     }


What do you think?

Regards, Peter


On 08/26/2016 07:16 AM, Tagir Valeev wrote:
> Hello!
>
> Small nitpick:
>
> versionsMap.keySet().forEach(v -> {
>      Stream<String> names = versionsMap.get(v).stream().map(nm -> nm.name);
>      if (v == versionMajor) {
>          // add all entries of the version we are interested in
>          finalNames.addAll(names.collect(Collectors.toSet()));
>      } else {
>          // add resource entries found in lower versioned directories
>          finalNames.addAll(names.filter(nm -> nm.endsWith(".class") ? false
> : true)
>                  .collect(Collectors.toSet()));
>      }
> });
>
> I suggest to replace with
>
> versionsMap.forEach((v, list) -> {
>    Stream<String> names = list.stream().map(nm -> nm.name);
>    if (v != versionMajor) {
>      names = names.filter(nm -> !nm.endsWith(".class"));
>    }
>    names.forEach(finalNames::add);
> });
>
> This is cleaner and somewhat faster to use Map.forEach as you don't need to
> lookup every map entry.
>
> Also I don't see why "nm -> nm.endsWith(".class") ? false : true" could be
> better than
> "nm -> !nm.endsWith(".class")". Probably a matter of style though.
>
> Finally there's no need to collect into intermediate set just to add this
> set into finalNames.
> Instead you can drain the stream directly to finalNames via forEach.
>
> Probably it should be explicitly noted in spec that the resulting stream is
> unordered (or at least may
> be unordered) as it's created from the Set.
>
> With best regards,
> Tagir Valeev
>
> On Fri, Aug 26, 2016 at 2:30 AM, Steve Drach <steve.drach at oracle.com> wrote:
>
>> Hi,
>>
>> Please review this changeset that adds a versionedStream method to JarFile.
>>
>> webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.00/ <
>> http://cr.openjdk.java.net/~sdrach/8163798/webrev.00/>
>> issue: https://bugs.openjdk.java.net/browse/JDK-8163798 <
>> https://bugs.openjdk.java.net/browse/JDK-8163798>
>>
>> Thanks
>> Steve
>>
>>
>>