Re: RFR: 8158295 Add a multi-release jar validation mechanism to jar tool

[Oleg G. Barbashov]

07.07.2016 23:32, Steve Drach пишет:

Hi,

Please review the following:

webrev: http://cr.openjdk.java.net/~sdrach/8158295/webrev.01/ 
<http://cr.openjdk.java.net/~sdrach/8158295/webrev.01/>
issue: https://bugs.openjdk.java.net/browse/JDK-8158295 
<https://bugs.openjdk.java.net/browse/JDK-8158295>

This changeset adds a multi-release jar validator to jar tool.  After the jar 
tool builds a multi-release jar, the potential resultant jar file is passed to 
the validator to assure that the jar file meets the minimal standards of a 
multi-release jar, in particular that versioned classes have the same api as 
base classes they override.  There are other checks, for example warning if two 
classes are identical.  If the jar file is invalid, it is not kept, so —create 
will not produce a jar file and —update will keep the input jar file.

Thanks
Steve

574     private boolean validate(String fname) {
 575         boolean valid = true;
 576         Validator validator = new Validator(this);
 577
 578         try (JarFile jf = new JarFile(fname)) {
 579             AtomicBoolean validHolder = new AtomicBoolean(valid);
 580             jf.stream()
 581                     .filter(e -> !e.isDirectory())
 582                     .filter(e -> !e.getName().equals(MANIFEST_NAME))
 583                     .filter(e -> !e.getName().endsWith(MODULE_INFO))
 584                     .sorted(entryComparator)
 585                     .forEachOrdered(je -> {
 586                         boolean b = validator.accept(je, jf);
 587                         if (validHolder.get()) validHolder.set(b);
 588                     });
 589             valid = validHolder.get();
 590         } catch (IOException e) {
 591             error(formatMsg2("error.validator.jarfile.exception", fname, 
e.getMessage()));
 592             valid = false;
 593         } catch (InvalidJarException e) {
 594             error(formatMsg("error.validator.bad.entry.name", 
e.getMessage()));
 595             valid = false;
 596         }
 597         return valid;
 598     }

(IMHO) Using of AtomicBoolean and forEachOrdered() for stateful validator here looks 
forced. It may be avoided by using regular iterator with "for" loop:

            Stream<JarEntry> sorted = jf.stream()
                    .filter(e -> !e.isDirectory())
                    .filter(e -> !e.getName().equals(MANIFEST_NAME))
                    .filter(e -> !e.getName().endsWith(MODULE_INFO))
                    .sorted(entryComparator);
            for (Iterator<JarEntry> iter = sorted.iterator(); iter.hasNext();) {
                if (!validator.accept(iter.next(), jf)) {
                    valid = false;
                }
            }

or even better by storing "valid" state inside the Validator after turning it to 
regular Consumer<JarEntry>:

        try (JarFile jf = new JarFile(fname)) {
            Validator validator = new Validator(this, jf);
            jf.stream()
                    .filter(e -> !e.isDirectory())
                    .filter(e -> !e.getName().equals(MANIFEST_NAME))
                    .filter(e -> !e.getName().endsWith(MODULE_INFO))
                    .sorted(entryComparator)
                    .forEachOrdered(validator);
            return validator.isValidated();
        } catch (IOException e) {
            error(formatMsg2("error.validator.jarfile.exception", fname, 
e.getMessage()));
            return false;
        } catch (NumberFormatException e) {
            error(formatMsg("error.validator.bad.entry.name", e.getMessage()));
            return false;
        }

Moreover what is the reason to have an external loop over the jar's entries? 
Even if we will use several different filter sets in future it would be better 
to use it as optional parameter for Validator.