Fwd: RFR 8163798: Add a versionedStream method to JarFile
Steve Drach
steve.drach at oracle.com
Fri Aug 26 19:27:21 UTC 2016
Sorry, forget to Cc core-libs on my response to Peter’s questions
> Begin forwarded message:
>
> From: Steve Drach <steve.drach at oracle.com>
> Subject: Re: RFR 8163798: Add a versionedStream method to JarFile
> Date: August 26, 2016 at 9:44:39 AM PDT
> To: Peter Levart <peter.levart at gmail.com>
>
> Hi Peter,
>
>> 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 public “interface” for a multi-release jar is the set of public or protected classes in the base (or root) section of the jar. By definition, no new (i.e. without a corresponding base class) public or protected classes can be contained in a versioned directory. Versioned directories can contain new package-private classes. In practice these package-private classes are dependencies for the public/protected classes in
> in the same versioned directory.
>
>> 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.
>
> I know. I thought about doing that but rejected it as too heavyweight and just decided to use the heuristic that real class files have the suffix “.class” and that if there was not a corresponding base class, then the class has to be package-private according to the “rules” of multi-release jar files. The jar tool enforces these rules by actually parsing the class files.
>
>> 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 hope the explanation above is sufficient. The versionedStream looks at the jar file in an inverted or backwards way. In the usual case, a
> multi-release jar file entry is obtained by JarFile.getJarEntry(base-entry-name). In this case if the jar file was opened for version 10, we’d not be able to see or get package-private classes in the version 9 directory. Oh, one thing I should mention, Mr. Jar’s can be sparsely populated in the versioned directories, So if a version 10 public class was not in the version 10 directory, but was in the version 9 directory, then the version 9 one would be returned.
>
>> 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/“
>
> I just did it because they are returned by JarFile::stream. I’m ambivalent about it, so I wanted to see the comments if any.
>
>>
>> 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?
>
> Yes.
>
>>
>> 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?
>
> It’s certainly more sophisticated than the way I did it. If we could constrain the getJarEntry only to base names we might be able to handle the package-private classes too. I need to think about it a bit.
>
> Steve
>
>>
>> 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
>>>>
>>>>
>>>>
>>
>
More information about the core-libs-dev
mailing list