Respond to PR feedback

williamrandolph · Oct 19, 2023 · a3a217d · a3a217d
1 parent aafc6a8
commit a3a217d
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 42 deletions.
diff --git a/server/src/main/java/org/elasticsearch/plugins/PluginsUtils.java b/server/src/main/java/org/elasticsearch/plugins/PluginsUtils.java
@@ -79,22 +79,37 @@ public static List<Path> findPluginDirs(final Path rootPath) throws IOException
      * Verify the given plugin is compatible with the current Elasticsearch installation.
      */
     public static void verifyCompatibility(PluginDescriptor info) {
-        boolean hasSemanticVersion = SemanticVersion.semanticPattern.matcher(Build.current().version()).matches();
+        Matcher buildVersionMatcher = SemanticVersion.semanticPattern.matcher(Build.current().version());
         // If we're not on a semantic version, assume plugins are compatible
-        if (hasSemanticVersion) {
-            SemanticVersion currentElasticsearchVersion = SemanticVersion.create(Build.current().version());
+        if (buildVersionMatcher.matches()) {
+            SemanticVersion currentElasticsearchVersion;
+            try {
+                currentElasticsearchVersion = new SemanticVersion(
+                    Integer.parseInt(buildVersionMatcher.group(1)),
+                    Integer.parseInt(buildVersionMatcher.group(2)),
+                    Integer.parseInt(buildVersionMatcher.group(3))
+                );
+            } catch (NumberFormatException e) {
+                throw new IllegalArgumentException("Couldn't parse integers from build version [" + Build.current().version() + "]", e);
+            }
             if (info.isStable()) {
+                Matcher pluginEsVersionMatcher = SemanticVersion.semanticPattern.matcher(info.getElasticsearchVersion());
+                if (pluginEsVersionMatcher.matches() == false) {
+                    throw new IllegalArgumentException(
+                        "Expected semantic version for plugin [" + info.getName() + "] but was [" + info.getElasticsearchVersion() + "]"
+                    );
+                }
                 SemanticVersion pluginElasticsearchVersion;
                 try {
-                    pluginElasticsearchVersion = SemanticVersion.create(info.getElasticsearchVersion());
-                } catch (IllegalArgumentException e) {
+                    pluginElasticsearchVersion = new SemanticVersion(
+                        Integer.parseInt(pluginEsVersionMatcher.group(1)),
+                        Integer.parseInt(pluginEsVersionMatcher.group(2)),
+                        Integer.parseInt(pluginEsVersionMatcher.group(3))
+                    );
+                } catch (NumberFormatException e) {
                     throw new IllegalArgumentException(
-                        "Stable Plugin ["
-                            + info.getName()
-                            + "] was built for non-semantic Elasticsearch version "
-                            + info.getElasticsearchVersion()
-                            + " and cannot run on version "
-                            + Build.current().version()
+                        "Expected integer version for plugin [" + info.getName() + "] but found [" + info.getElasticsearchVersion() + "]",
+                        e
                     );
                 }
 
@@ -138,25 +153,9 @@ public static void verifyCompatibility(PluginDescriptor info) {
         JarHell.checkJavaVersion(info.getName(), info.getJavaVersion());
     }
 
-    private record SemanticVersion(int major, int minor, int bugfix, String suffix) {
+    private record SemanticVersion(int major, int minor, int bugfix) {
 
-        static final Pattern semanticPattern = Pattern.compile("^(\\d+)\\.(\\d+)\\.(\\d+)(\\D?.*)$");
-        static SemanticVersion create(String version) {
-            Matcher matcher = semanticPattern.matcher(version);
-            if (matcher.matches() == false) {
-                throw new IllegalArgumentException("Cannot be parsed as a semantic version: " + version);
-            }
-            try {
-                return new SemanticVersion(
-                    Integer.parseInt(matcher.group(1)),
-                    Integer.parseInt(matcher.group(2)),
-                    Integer.parseInt(matcher.group(3)),
-                    matcher.group(4)
-                );
-            } catch (NumberFormatException e) {
-                throw new IllegalArgumentException("Cannot be parsed as a semantic version: " + version, e);
-            }
-        }
+        static final Pattern semanticPattern = Pattern.compile("^(\\d+)\\.(\\d+)\\.(\\d+)$");
 
         // does not compare anything after the semantic version
         boolean after(SemanticVersion other) {

diff --git a/server/src/test/java/org/elasticsearch/plugins/PluginsUtilsTests.java b/server/src/test/java/org/elasticsearch/plugins/PluginsUtilsTests.java
@@ -34,8 +34,6 @@
 
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.when;
 
 @LuceneTestCase.SuppressFileSystems(value = "ExtrasFS")
 public class PluginsUtilsTests extends ESTestCase {
@@ -384,19 +382,16 @@ public void testJarHellSpiConflict() throws Exception {
         assertThat(e.getCause().getMessage(), containsString("DummyClass1"));
     }
 
+    public void testInternalNonSemanticVersions() throws Exception {
+        PluginDescriptor info = getPluginDescriptorForVersion(randomAlphaOfLengthBetween(6, 32), "1.8", false);
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> PluginsUtils.verifyCompatibility(info));
+        assertThat(e.getMessage(), containsString("Plugin [my_plugin] was built for Elasticsearch version"));
+    }
+
     public void testStableNonSemanticVersions() throws Exception {
-        // TODO[wrb]: stop using mock once https://github.com/elastic/elasticsearch/pull/100713 is merged
-        PluginDescriptor info = mock(PluginDescriptor.class);
-        String pluginEsBuildVersion = randomAlphaOfLength(10);
-        when(info.getElasticsearchVersion()).thenReturn(pluginEsBuildVersion);
-        when(info.isStable()).thenReturn(true);
+        PluginDescriptor info = getPluginDescriptorForVersion(randomAlphaOfLengthBetween(6, 32), "1.8", true);
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> PluginsUtils.verifyCompatibility(info));
-        assertThat(
-            e.getMessage(),
-            containsString(
-                "was built for non-semantic Elasticsearch version " + pluginEsBuildVersion + " and cannot run on version " + Version.CURRENT
-            )
-        );
+        assertThat(e.getMessage(), containsString("Expected semantic version for plugin [my_plugin] but was"));
     }
 
     public void testStableEarlierElasticsearchVersion() throws Exception {