Improved logic that removes top-level folder from archives when needed

Whether we remove the top-level folder from the archive depends now on the zip itself and not on where it was downloaded from. That makes it work installing local files too.

Closes #3582

src/main/java/org/elasticsearch/plugins/PluginManager.java

@@ -37,6 +37,8 @@ import java.io.FileOutputStream;
 import java.io.IOException;
 import java.net.URL;
 import java.util.Enumeration;
+import java.util.HashSet;
+import java.util.Set;
 import java.util.zip.ZipEntry;
 import java.util.zip.ZipFile;
 
@@ -115,7 +117,6 @@ public class PluginManager {
         }
 
         // now, try as a path name...
-        String filterZipName = null;
         if (!downloaded) {
             if (name.indexOf('/') != -1) {
                 // github repo
@@ -126,7 +127,6 @@ public class PluginManager {
                 if (elements.length > 2) {
                     version = elements[2];
                 }
-                filterZipName = repoName;
                 // the installation file should not include the userName, just the repoName
                 name = repoName;
                 if (name.startsWith("elasticsearch-")) {
@@ -219,17 +219,18 @@ public class PluginManager {
         ZipFile zipFile = null;
         try {
             zipFile = new ZipFile(pluginFile);
-            Enumeration<? extends ZipEntry> zipEntries = zipFile.entries();
+            //we check whether we need to remove the top-level folder while extracting
+            //sometimes (e.g. github) the downloaded archive contains a top-level folder which needs to be removed
+            boolean removeTopLevelDir = topLevelDirInExcess(zipFile);
+            Enumeration <? extends ZipEntry> zipEntries = zipFile.entries();
             while (zipEntries.hasMoreElements()) {
                 ZipEntry zipEntry = zipEntries.nextElement();
                 if (zipEntry.isDirectory()) {
                     continue;
                 }
                 String zipEntryName = zipEntry.getName().replace('\\', '/');
-                if (filterZipName != null) {
-                    if (zipEntryName.startsWith(filterZipName)) {
-                        zipEntryName = zipEntryName.substring(zipEntryName.indexOf('/'));
-                    }
+                if (removeTopLevelDir) {
+                    zipEntryName = zipEntryName.substring(zipEntryName.indexOf('/'));
                 }
                 File target = new File(extractLocation, zipEntryName);
                 FileSystemUtils.mkdirs(target.getParentFile());
@@ -306,7 +307,31 @@ public class PluginManager {
             }
         }
     }
-    
+
+    private boolean topLevelDirInExcess(ZipFile zipFile) {
+        //We don't rely on ZipEntry#isDirectory because it might be that there is no explicit dir
+        //but the files path do contain dirs, thus they are going to be extracted on sub-folders anyway
+        Enumeration<? extends ZipEntry> zipEntries = zipFile.entries();
+        Set<String> topLevelDirNames = new HashSet<String>();
+        while (zipEntries.hasMoreElements()) {
+            ZipEntry zipEntry = zipEntries.nextElement();
+            String zipEntryName = zipEntry.getName().replace('\\', '/');
+
+            int slash = zipEntryName.indexOf('/');
+            //if there isn't a slash in the entry name it means that we have a file in the top-level
+            if (slash == -1 ) {
+                return false;
+            }
+
+            topLevelDirNames.add(zipEntryName.substring(0, slash));
+            //if we have more than one top-level folder
+            if (topLevelDirNames.size() > 1) {
+                return false;
+            }
+        }
+        return topLevelDirNames.size() == 1 && !"_site".equals(topLevelDirNames.iterator().next());
+    }
+
     private static final int EXIT_CODE_OK = 0;
     private static final int EXIT_CODE_CMD_USAGE = 64;
     private static final int EXIT_CODE_IO_ERROR = 74;

src/test/java/org/elasticsearch/test/integration/plugin/PluginManagerTests.java

@@ -0,0 +1,146 @@
+/*
+ * Licensed to ElasticSearch and Shay Banon 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.test.integration.plugin;
+
+import org.elasticsearch.action.admin.cluster.node.info.NodesInfoResponse;
+import org.elasticsearch.common.collect.Tuple;
+import org.elasticsearch.common.io.FileSystemUtils;
+import org.elasticsearch.common.settings.ImmutableSettings;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.env.Environment;
+import org.elasticsearch.node.internal.InternalSettingsPerparer;
+import org.elasticsearch.plugins.PluginManager;
+import org.elasticsearch.rest.RestStatus;
+import org.elasticsearch.test.integration.AbstractNodesTests;
+import org.elasticsearch.test.integration.rest.helper.HttpClient;
+import org.elasticsearch.test.integration.rest.helper.HttpClientResponse;
+import org.hamcrest.Matchers;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.File;
+import java.net.URL;
+
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.notNullValue;
+
+public class PluginManagerTests extends AbstractNodesTests {
+
+    private static final String PLUGIN_DIR = "target/plugins";
+
+    @Test
+    public void testLocalPluginInstallSingleFolder() throws Exception {
+        //When we have only a folder in top-level (no files either) we remove that folder while extracting
+        String pluginName = "plugin-test";
+        URL url = PluginManagerTests.class.getResource("plugin_single_folder.zip");
+        downloadAndExtract(pluginName, "file://" + url.getFile());
+
+        startNode();
+
+        assertPluginLoaded(pluginName);
+        assertPluginAvailable(pluginName);
+    }
+
+    @Test
+    public void testLocalPluginInstallSiteFolder() throws Exception {
+        //When we have only a folder in top-level (no files either) but it's called _site, we make it work
+        //we can either remove the folder while extracting and then re-add it manually or just leave it as it is
+        String pluginName = "plugin-test";
+        URL url = PluginManagerTests.class.getResource("plugin_folder_site.zip");
+        downloadAndExtract(pluginName, "file://" + url.getFile());
+
+        startNode();
+
+        assertPluginLoaded(pluginName);
+        assertPluginAvailable(pluginName);
+    }
+
+    @Test
+    public void testLocalPluginWithoutFolders() throws Exception {
+        //When we don't have folders at all in the top-level, but only files, we don't modify anything
+        String pluginName = "plugin-test";
+        URL url = PluginManagerTests.class.getResource("plugin_without_folders.zip");
+        downloadAndExtract(pluginName, "file://" + url.getFile());
+
+        startNode();
+
+        assertPluginLoaded(pluginName);
+        assertPluginAvailable(pluginName);
+    }
+
+    @Test
+    public void testLocalPluginFolderAndFile() throws Exception {
+        //When we have a single top-level folder but also files in the top-level, we don't modify anything
+        String pluginName = "plugin-test";
+        URL url = PluginManagerTests.class.getResource("plugin_folder_file.zip");
+        downloadAndExtract(pluginName, "file://" + url.getFile());
+
+        startNode();
+
+        assertPluginLoaded(pluginName);
+        assertPluginAvailable(pluginName);
+    }
+
+    private static void downloadAndExtract(String pluginName, String pluginUrl) throws Exception {
+        Tuple<Settings, Environment> initialSettings = InternalSettingsPerparer.prepareSettings(
+                ImmutableSettings.settingsBuilder().put("path.plugins", PLUGIN_DIR).build(), false);
+        if (!initialSettings.v2().pluginsFile().exists()) {
+            FileSystemUtils.mkdirs(initialSettings.v2().pluginsFile());
+        }
+        PluginManager pluginManager = new PluginManager(initialSettings.v2(), pluginUrl);
+        pluginManager.downloadAndExtract(pluginName, false);
+    }
+
+    private void startNode() {
+        startNode("plugin-test-node", ImmutableSettings.settingsBuilder()
+                .put("discovery.zen.ping.multicast.enabled", false)
+                .put("path.plugins", PLUGIN_DIR));
+    }
+
+    private void assertPluginLoaded(String pluginName) {
+        NodesInfoResponse nodesInfoResponse = client().admin().cluster().prepareNodesInfo().clear().setPlugin(true).get();
+        assertThat(nodesInfoResponse.getNodes().length, equalTo(1));
+        assertThat(nodesInfoResponse.getNodes()[0].getPlugins().getInfos(), notNullValue());
+        assertThat(nodesInfoResponse.getNodes()[0].getPlugins().getInfos().size(), equalTo(1));
+        assertThat(nodesInfoResponse.getNodes()[0].getPlugins().getInfos().get(0).getName(), equalTo(pluginName));
+        assertThat(nodesInfoResponse.getNodes()[0].getPlugins().getInfos().get(0).isSite(), equalTo(true));
+    }
+
+    private void assertPluginAvailable(String pluginName) {
+        HttpClient httpClient = new HttpClient("http://localhost:9200/");
+        HttpClientResponse response = httpClient.request("_plugin/" + pluginName + "/");
+        assertThat(response.errorCode(), Matchers.equalTo(RestStatus.OK.getStatus()));
+    }
+
+    @Before
+    public void before() {
+        deletePluginsFolder();
+    }
+
+    @After
+    public void after() {
+        deletePluginsFolder();
+        closeAllNodes();
+    }
+
+    private void deletePluginsFolder() {
+        FileSystemUtils.deleteRecursively(new File(PLUGIN_DIR));
+    }
+}