Re: Fwd: RFR 8163798: Add a versionedStream method to JarFile

[Alan Bateman]

On 26/08/2016 20:27, Steve Drach wrote:


:

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 versioned section of a modular MR JAR may contain .class files 
corresponding to types that are not in exported packages. In that 
scenario then it doesn't matter if the types are public or not. In any 
case, the JarFile code shouldn't be concerned this, it can leave any 
checking to packaging time and the jar tool.


-Alan.

Fwd: RFR 8163798: Add a versionedStream method to JarFile

[Steve Drach]

Sorry, forget to Cc core-libs on my response to Peter’s questions

> Begin forwarded message:
> 
> From: Steve Drach 
> Subject: Re: RFR 8163798: Add a versionedStream method to JarFile
> Date: August 26, 2016 at 9:44:39 AM PDT
> To: Peter Levart 
> 
> 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 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(&#

Re: RFR 8163798: Add a versionedStream method to JarFile

[Alan Bateman]

On 26/08/2016 00:30, Steve Drach wrote:


Hi,

Please review this changeset that adds a versionedStream method to JarFile.

webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.00/ 

issue: https://bugs.openjdk.java.net/browse/JDK-8163798 


I see Peter and others are commenting on the implementation so I'll 
focus on the javadoc for now. In general then I think the javadoc will 
need a bit of work as I don't think it's clear to the reader what is 
returned. It might help to include a few simple examples to show what 
elements will be in the stream. One example might be where the runtime 
version is 10 and there are resources in the base and versions/9 
sections. Also assume I've got resources in versions/10/** that are not 
in other sections then it would be useful to quickly understand if they 
are included are not.


Terminology-wise then I'm not sure if "accessible" should be used, the 
reason is that it will be confused with other types of access, 
particularly access control or even security permissions.


The javadoc also mentions non-public classes and being accessed by 
public classes. I know what you mean but the javadoc shouldn't be 
concerned with that. Clearly the API should be the same on all versions 
but that is something for the introducing in the class description 
rather than this method. Also with modular JARs then you may have public 
types in non-exported packages. I might even have public types in 
non-exported packages in versioned sections that don't override anything 
in the base section.


Another point to include in the javadoc is to make it clear that the 
method behaves the same as stream() then the JAR file is not a 
multi-release JAR.


A minor comment is that there are at least 4 x @{code ...}, it should be 
{@code ...}.


-Alan.

Re: 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 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// 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 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 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 t

Re: RFR 8163798: Add a versionedStream method to JarFile

[Andrej Golovnin]

Hi Steve,

in the line

597 Map> versionsMap = stream()

I miss space after the comma.

In the line

626 return finalNames.stream().map(nm -> getJarEntry(nm));

You can use a method reference instead of lambda:

626 return finalNames.stream().map(this::getJarEntry);

And if you add a method #getName() to the Name class,
then you can use method references in the lines 604 and 614 too.


+1 to Tagir's suggestions.


Best regards,
Andrej Golovnin

Re: RFR 8163798: Add a versionedStream method to JarFile

[Tagir Valeev]

Hello!

Small nitpick:

versionsMap.keySet().forEach(v -> {
Stream 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 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  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
>
>
>

RFR 8163798: Add a versionedStream method to JarFile

[Steve Drach]

Hi,

Please review this changeset that adds a versionedStream method to JarFile.

webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.00/ 

issue: https://bugs.openjdk.java.net/browse/JDK-8163798 


Thanks
Steve