From 126e8e4aeef9866c96e938d84ab8aedac1a63a1c Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 19 Aug 2015 21:32:15 -0400 Subject: [PATCH] Improve Java version comparison in JarHell MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit improves Java version comparison in JarHell. The first improvement is the addition of a method to check the version format of a target version string. This method will reject target version strings that are not a sequence of nonnegative decimal integers separated by “.”s, possibly with leading zeros (0*[0-9]+(\.[0-9]+)?). This version format is the version format used for Java specification versioning (cf. Java Product Versioning, 1.5.1 Specification Versioning and the Javadocs for java.lang.Package.) The second improvement is a clean method for checking that a target version is compatible with the runtime version of the JVM. This is done using the system property java.specification.version and comparing the versions lexicograpically. This method of comparison has been tested on JDK 9 builds that include JEP-220 (the Project Jigsaw JEP concerning modular runtime images) and JEP-223 (the version string JEP). The class that encapsulates the methods for parsing and comparing versions is written in a way that can easily be converted to use the Version class from JEP-223 if that class is ultimately incorporated into mainline JDK 9. Closes #12441 --- .../org/elasticsearch/bootstrap/JarHell.java | 46 ++++++---- .../elasticsearch/bootstrap/JavaVersion.java | 87 +++++++++++++++++++ .../elasticsearch/bootstrap/JarHellTests.java | 49 +++++++++-- .../bootstrap/JavaVersionTests.java | 79 +++++++++++++++++ 4 files changed, 234 insertions(+), 27 deletions(-) create mode 100644 core/src/main/java/org/elasticsearch/bootstrap/JavaVersion.java create mode 100644 core/src/test/java/org/elasticsearch/bootstrap/JavaVersionTests.java diff --git a/core/src/main/java/org/elasticsearch/bootstrap/JarHell.java b/core/src/main/java/org/elasticsearch/bootstrap/JarHell.java index 1f6e3345c38..a3c6366fe32 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/JarHell.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/JarHell.java @@ -25,6 +25,7 @@ import org.elasticsearch.common.io.PathUtils; import org.elasticsearch.common.logging.ESLogger; import org.elasticsearch.common.logging.Loggers; +import java.io.ByteArrayInputStream; import java.io.IOException; import java.net.URL; import java.net.URLClassLoader; @@ -33,12 +34,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.SimpleFileVisitor; import java.nio.file.attribute.BasicFileAttributes; -import java.util.Arrays; -import java.util.Enumeration; -import java.util.HashMap; -import java.util.HashSet; -import java.util.Map; -import java.util.Set; +import java.util.*; import java.util.jar.JarEntry; import java.util.jar.JarFile; import java.util.jar.Manifest; @@ -69,7 +65,7 @@ public class JarHell { logger.debug("sun.boot.class.path: {}", System.getProperty("sun.boot.class.path")); logger.debug("classloader urls: {}", Arrays.toString(((URLClassLoader)loader).getURLs())); } - checkJarHell(((URLClassLoader)loader).getURLs()); + checkJarHell(((URLClassLoader) loader).getURLs()); } /** @@ -141,6 +137,7 @@ public class JarHell { // give a nice error if jar requires a newer java version String targetVersion = manifest.getMainAttributes().getValue("X-Compile-Target-JDK"); if (targetVersion != null) { + checkVersionFormat(targetVersion); checkJavaVersion(jar.toString(), targetVersion); } @@ -153,23 +150,34 @@ public class JarHell { } } + public static void checkVersionFormat(String targetVersion) { + if (!JavaVersion.isValid(targetVersion)) { + throw new IllegalStateException( + String.format( + Locale.ROOT, + "version string must be a sequence of nonnegative decimal integers separated by \".\"'s and may have leading zeros but was %s", + targetVersion + ) + ); + } + } + /** * Checks that the java specification version {@code targetVersion} * required by {@code resource} is compatible with the current installation. */ public static void checkJavaVersion(String resource, String targetVersion) { - String systemVersion = System.getProperty("java.specification.version"); - float current = Float.POSITIVE_INFINITY; - float target = Float.NEGATIVE_INFINITY; - try { - current = Float.parseFloat(systemVersion); - target = Float.parseFloat(targetVersion); - } catch (NumberFormatException e) { - // some spec changed, time for a more complex parser - } - if (current < target) { - throw new IllegalStateException(resource + " requires Java " + targetVersion - + ", your system: " + systemVersion); + JavaVersion version = JavaVersion.parse(targetVersion); + if (JavaVersion.current().compareTo(version) < 0) { + throw new IllegalStateException( + String.format( + Locale.ROOT, + "%s requires Java %s:, your system: %s", + resource, + targetVersion, + JavaVersion.current().toString() + ) + ); } } diff --git a/core/src/main/java/org/elasticsearch/bootstrap/JavaVersion.java b/core/src/main/java/org/elasticsearch/bootstrap/JavaVersion.java new file mode 100644 index 00000000000..f88469702a5 --- /dev/null +++ b/core/src/main/java/org/elasticsearch/bootstrap/JavaVersion.java @@ -0,0 +1,87 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.bootstrap; + +import org.elasticsearch.common.Strings; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +class JavaVersion implements Comparable { + private final List version; + + public List getVersion() { + return Collections.unmodifiableList(version); + } + + private JavaVersion(List version) { + this.version = version; + } + + public static JavaVersion parse(String value) { + if (value == null) { + throw new NullPointerException("value"); + } + if ("".equals(value)) { + throw new IllegalArgumentException("value"); + } + + List version = new ArrayList<>(); + String[] components = value.split("\\."); + for (String component : components) { + version.add(Integer.valueOf(component)); + } + + return new JavaVersion(version); + } + + public static boolean isValid(String value) { + if (!value.matches("^0*[0-9]+(\\.[0-9]+)*$")) { + return false; + } + return true; + } + + private final static JavaVersion CURRENT = parse(System.getProperty("java.specification.version")); + + public static JavaVersion current() { + return CURRENT; + } + + @Override + public int compareTo(JavaVersion o) { + int len = Math.max(version.size(), o.version.size()); + for (int i = 0; i < len; i++) { + int d = (i < version.size() ? version.get(i) : 0); + int s = (i < o.version.size() ? o.version.get(i) : 0); + if (s < d) + return 1; + if (s > d) + return -1; + } + return 0; + } + + @Override + public String toString() { + return Strings.collectionToDelimitedString(version, "."); + } +} diff --git a/core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java b/core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java index 50c68eba281..2d825674dbe 100644 --- a/core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java +++ b/core/src/test/java/org/elasticsearch/bootstrap/JarHellTests.java @@ -20,6 +20,7 @@ package org.elasticsearch.bootstrap; import org.elasticsearch.Version; +import org.elasticsearch.common.Strings; import org.elasticsearch.test.ESTestCase; import java.io.IOException; @@ -27,6 +28,8 @@ import java.net.URL; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardOpenOption; +import java.util.ArrayList; +import java.util.List; import java.util.jar.Attributes; import java.util.jar.JarOutputStream; import java.util.jar.Manifest; @@ -153,22 +156,25 @@ public class JarHellTests extends ESTestCase { public void testRequiredJDKVersionTooOld() throws Exception { Path dir = createTempDir(); - String previousJavaVersion = System.getProperty("java.specification.version"); - System.setProperty("java.specification.version", "1.7"); + List current = JavaVersion.current().getVersion(); + List target = new ArrayList<>(current.size()); + for (int i = 0; i < current.size(); i++) { + target.add(current.get(i) + 1); + } + JavaVersion targetVersion = JavaVersion.parse(Strings.collectionToDelimitedString(target, ".")); + Manifest manifest = new Manifest(); Attributes attributes = manifest.getMainAttributes(); attributes.put(Attributes.Name.MANIFEST_VERSION, "1.0.0"); - attributes.put(new Attributes.Name("X-Compile-Target-JDK"), "1.8"); + attributes.put(new Attributes.Name("X-Compile-Target-JDK"), targetVersion.toString()); URL[] jars = {makeJar(dir, "foo.jar", manifest, "Foo.class")}; try { JarHell.checkJarHell(jars); fail("did not get expected exception"); } catch (IllegalStateException e) { - assertTrue(e.getMessage().contains("requires Java 1.8")); - assertTrue(e.getMessage().contains("your system: 1.7")); - } finally { - System.setProperty("java.specification.version", previousJavaVersion); + assertTrue(e.getMessage().contains("requires Java " + targetVersion.toString())); + assertTrue(e.getMessage().contains("your system: " + JavaVersion.current().toString())); } } @@ -213,7 +219,12 @@ public class JarHellTests extends ESTestCase { attributes.put(Attributes.Name.MANIFEST_VERSION, "1.0.0"); attributes.put(new Attributes.Name("X-Compile-Target-JDK"), "bogus"); URL[] jars = {makeJar(dir, "foo.jar", manifest, "Foo.class")}; - JarHell.checkJarHell(jars); + try { + JarHell.checkJarHell(jars); + fail("did not get expected exception"); + } catch (IllegalStateException e) { + assertTrue(e.getMessage().equals("version string must be a sequence of nonnegative decimal integers separated by \".\"'s and may have leading zeros but was bogus")); + } } /** make sure if a plugin is compiled against the same ES version, it works */ @@ -242,4 +253,26 @@ public class JarHellTests extends ESTestCase { assertTrue(e.getMessage().contains("requires Elasticsearch 1.0-bogus")); } } + + public void testValidVersions() { + String[] versions = new String[]{"1.7", "1.7.0", "0.1.7", "1.7.0.80"}; + for (String version : versions) { + try { + JarHell.checkVersionFormat(version); + } catch (IllegalStateException e) { + fail(version + " should be accepted as a valid version format"); + } + } + } + + public void testInvalidVersions() { + String[] versions = new String[]{"", "1.7.0_80", "1.7."}; + for (String version : versions) { + try { + JarHell.checkVersionFormat(version); + fail("\"" + version + "\"" + " should be rejected as an invalid version format"); + } catch (IllegalStateException e) { + } + } + } } diff --git a/core/src/test/java/org/elasticsearch/bootstrap/JavaVersionTests.java b/core/src/test/java/org/elasticsearch/bootstrap/JavaVersionTests.java new file mode 100644 index 00000000000..851e0fd9f85 --- /dev/null +++ b/core/src/test/java/org/elasticsearch/bootstrap/JavaVersionTests.java @@ -0,0 +1,79 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.bootstrap; + +import org.elasticsearch.test.ESTestCase; +import org.junit.Test; + +import java.util.List; + +import static org.hamcrest.CoreMatchers.is; + +public class JavaVersionTests extends ESTestCase { + @Test + public void testParse() { + JavaVersion javaVersion = JavaVersion.parse("1.7.0"); + List version = javaVersion.getVersion(); + assertThat(3, is(version.size())); + assertThat(1, is(version.get(0))); + assertThat(7, is(version.get(1))); + assertThat(0, is(version.get(2))); + } + + @Test + public void testToString() { + JavaVersion javaVersion = JavaVersion.parse("1.7.0"); + assertThat("1.7.0", is(javaVersion.toString())); + } + + @Test + public void testCompare() { + JavaVersion onePointSix = JavaVersion.parse("1.6"); + JavaVersion onePointSeven = JavaVersion.parse("1.7"); + JavaVersion onePointSevenPointZero = JavaVersion.parse("1.7.0"); + JavaVersion onePointSevenPointOne = JavaVersion.parse("1.7.1"); + JavaVersion onePointSevenPointTwo = JavaVersion.parse("1.7.2"); + JavaVersion onePointSevenPointOnePointOne = JavaVersion.parse("1.7.1.1"); + JavaVersion onePointSevenPointTwoPointOne = JavaVersion.parse("1.7.2.1"); + + assertTrue(onePointSix.compareTo(onePointSeven) < 0); + assertTrue(onePointSeven.compareTo(onePointSix) > 0); + assertTrue(onePointSix.compareTo(onePointSix) == 0); + assertTrue(onePointSeven.compareTo(onePointSevenPointZero) == 0); + assertTrue(onePointSevenPointOnePointOne.compareTo(onePointSevenPointOne) > 0); + assertTrue(onePointSevenPointTwo.compareTo(onePointSevenPointTwoPointOne) < 0); + } + + @Test + public void testValidVersions() { + String[] versions = new String[]{"1.7", "1.7.0", "0.1.7", "1.7.0.80"}; + for (String version : versions) { + assertTrue(JavaVersion.isValid(version)); + } + } + + @Test + public void testInvalidVersions() { + String[] versions = new String[]{"", "1.7.0_80", "1.7."}; + for (String version : versions) { + assertFalse(JavaVersion.isValid(version)); + } + } +} \ No newline at end of file