Jar plugin loading refactor as generic ServerPlugin

Now that we have a fully pluggable ServerPlugin mechanism
with registered providers, there is no reason why the jar
plugins cannot be loaded using the same interface.

Removing all the custom code managing the jar plugins
and creating a new pre-defined JarPluginProvider.

NOTE: A limitation still exists on init-steps as they
can only be provided by jar plugins. This is due to
the fact that Gerrit init runs without having a fully
configured plugin loader and full set of Guice injectors.

Change-Id: I793570735376be3a40f3204af6e82820191caac6

gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitPluginStepsLoader.java

@@ -18,6 +18,7 @@ import com.google.common.base.Objects;
 import com.google.gerrit.extensions.annotations.PluginName;
 import com.google.gerrit.pgm.util.ConsoleUI;
 import com.google.gerrit.server.config.SitePaths;
+import com.google.gerrit.server.plugins.JarPluginProvider;
 import com.google.gerrit.server.plugins.PluginLoader;
 import com.google.inject.AbstractModule;
 import com.google.inject.Inject;
@@ -95,9 +96,9 @@ public class InitPluginStepsLoader {
     }
   }
 
-  private Injector getPluginInjector(File jarFile) throws IOException {
+  private Injector getPluginInjector(final File jarFile) throws IOException {
     final String pluginName =
-        Objects.firstNonNull(PluginLoader.getGerritJarPluginName(jarFile),
+        Objects.firstNonNull(JarPluginProvider.getJarPluginName(jarFile),
             PluginLoader.nameOf(jarFile));
     return initInjector.createChildInjector(new AbstractModule() {
       @Override

gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitPlugins.java

@@ -18,7 +18,7 @@ import com.google.common.collect.Lists;
 import com.google.gerrit.common.PluginData;
 import com.google.gerrit.pgm.util.ConsoleUI;
 import com.google.gerrit.server.config.SitePaths;
-import com.google.gerrit.server.plugins.PluginLoader;
+import com.google.gerrit.server.plugins.JarPluginProvider;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 
@@ -52,7 +52,7 @@ public class InitPlugins implements InitStep {
     pluginsDistribution.foreach(new PluginsDistribution.Processor() {
       @Override
       public void process(String pluginName, InputStream in) throws IOException {
-        File tmpPlugin = PluginLoader.storeInTemp(pluginName, in, site);
+        File tmpPlugin = JarPluginProvider.storeInTemp(pluginName, in, site);
         String pluginVersion = getVersion(tmpPlugin);
         if (deleteTempPluginFile) {
           tmpPlugin.delete();

gerrit-server/src/main/java/com/google/gerrit/server/plugins/JarPluginProvider.java

@@ -0,0 +1,159 @@
+// Copyright (C) 2014 The Android Open Source Project
+//
+// Licensed 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 com.google.gerrit.server.plugins;
+
+import static com.google.gerrit.server.plugins.PluginLoader.asTemp;
+
+import com.google.common.base.Objects;
+import com.google.gerrit.server.config.SitePaths;
+import com.google.inject.Inject;
+
+import org.eclipse.jgit.internal.storage.file.FileSnapshot;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.text.SimpleDateFormat;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.List;
+import java.util.jar.JarFile;
+import java.util.jar.Manifest;
+
+public class JarPluginProvider implements ServerPluginProvider {
+  static final String PLUGIN_TMP_PREFIX = "plugin_";
+  static final String JAR_EXTENSION = ".jar";
+  static final Logger log = LoggerFactory.getLogger(JarPluginProvider.class);
+
+  private final File tmpDir;
+
+  @Inject
+  JarPluginProvider(SitePaths sitePaths) {
+    tmpDir = sitePaths.tmp_dir;
+  }
+
+  @Override
+  public boolean handles(File srcFile) {
+    String fileName = srcFile.getName();
+    return fileName.endsWith(JAR_EXTENSION)
+        || fileName.endsWith(JAR_EXTENSION + ".disabled");
+  }
+
+  @Override
+  public String getPluginName(File srcFile) {
+    try {
+      return Objects.firstNonNull(getJarPluginName(srcFile),
+          PluginLoader.nameOf(srcFile));
+    } catch (IOException e) {
+      throw new IllegalArgumentException("Invalid plugin file " + srcFile
+          + ": cannot get plugin name", e);
+    }
+  }
+
+  public static String getJarPluginName(File srcFile) throws IOException {
+    try (JarFile jarFile = new JarFile(srcFile)) {
+      return jarFile.getManifest().getMainAttributes()
+          .getValue("Gerrit-PluginName");
+    }
+  }
+
+  @Override
+  public ServerPlugin get(File srcFile, FileSnapshot snapshot,
+      PluginDescription description) throws InvalidPluginException {
+    try {
+      String name = getPluginName(srcFile);
+      String extension = getExtension(srcFile);
+      try (FileInputStream in = new FileInputStream(srcFile)) {
+        File tmp = asTemp(in, tempNameFor(name), extension, tmpDir);
+        return loadJarPlugin(name, srcFile, snapshot, tmp, description);
+      }
+    } catch (IOException | ClassNotFoundException e) {
+      throw new InvalidPluginException("Cannot load Jar plugin " + srcFile, e);
+    }
+  }
+
+  @Override
+  public String getProviderPluginName() {
+    return "gerrit";
+  }
+
+  private static String getExtension(File file) {
+    return getExtension(file.getName());
+  }
+
+  private static String getExtension(String name) {
+    int ext = name.lastIndexOf('.');
+    return 0 < ext ? name.substring(ext) : "";
+  }
+
+  private static String tempNameFor(String name) {
+    SimpleDateFormat fmt = new SimpleDateFormat("yyMMdd_HHmm");
+    return PLUGIN_TMP_PREFIX + name + "_" + fmt.format(new Date()) + "_";
+  }
+
+  public static File storeInTemp(String pluginName, InputStream in,
+      SitePaths sitePaths) throws IOException {
+    if (!sitePaths.tmp_dir.exists()) {
+      sitePaths.tmp_dir.mkdirs();
+    }
+    return asTemp(in, tempNameFor(pluginName), ".jar", sitePaths.tmp_dir);
+  }
+
+  private ServerPlugin loadJarPlugin(String name, File srcJar,
+      FileSnapshot snapshot, File tmp, PluginDescription description)
+      throws IOException, InvalidPluginException, MalformedURLException,
+      ClassNotFoundException {
+    JarFile jarFile = new JarFile(tmp);
+    boolean keep = false;
+    try {
+      Manifest manifest = jarFile.getManifest();
+      Plugin.ApiType type = Plugin.getApiType(manifest);
+
+      List<URL> urls = new ArrayList<>(2);
+      String overlay = System.getProperty("gerrit.plugin-classes");
+      if (overlay != null) {
+        File classes = new File(new File(new File(overlay), name), "main");
+        if (classes.isDirectory()) {
+          log.info(String.format("plugin %s: including %s", name,
+              classes.getPath()));
+          urls.add(classes.toURI().toURL());
+        }
+      }
+      urls.add(tmp.toURI().toURL());
+
+      ClassLoader pluginLoader =
+          new URLClassLoader(urls.toArray(new URL[urls.size()]),
+              PluginLoader.parentFor(type));
+
+      ServerPlugin plugin =
+          new ServerPlugin(name, description.canonicalUrl, description.user,
+              srcJar, snapshot, new JarScanner(srcJar), description.dataDir,
+              pluginLoader);
+      plugin.setCleanupHandle(new CleanupHandle(tmp, jarFile));
+      keep = true;
+      return plugin;
+    } finally {
+      if (!keep) {
+        jarFile.close();
+      }
+    }
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/plugins/Plugin.java

@@ -73,6 +73,7 @@ public abstract class Plugin {
   private final CacheKey cacheKey;
   private final PluginUser pluginUser;
   private final FileSnapshot snapshot;
+  private CleanupHandle cleanupHandle;
 
   protected LifecycleManager manager;
 
@@ -92,6 +93,14 @@ public abstract class Plugin {
     this.disabled = srcFile.getName().endsWith(".disabled");
   }
 
+  public CleanupHandle getCleanupHandle() {
+    return cleanupHandle;
+  }
+
+  public void setCleanupHandle(CleanupHandle cleanupHandle) {
+    this.cleanupHandle = cleanupHandle;
+  }
+
   PluginUser getPluginUser() {
     return pluginUser;
   }

gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginLoader.java

@@ -47,20 +47,14 @@ import org.slf4j.LoggerFactory;
 
 import java.io.File;
 import java.io.FileFilter;
-import java.io.FileInputStream;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
-import java.net.MalformedURLException;
-import java.net.URL;
-import java.net.URLClassLoader;
-import java.text.SimpleDateFormat;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
-import java.util.Date;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
@@ -70,8 +64,6 @@ import java.util.Set;
 import java.util.TreeSet;
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.TimeUnit;
-import java.util.jar.JarFile;
-import java.util.jar.Manifest;
 
 @Singleton
 public class PluginLoader implements LifecycleListener {
@@ -84,7 +76,6 @@ public class PluginLoader implements LifecycleListener {
 
   private final File pluginsDir;
   private final File dataDir;
-  private final File tmpDir;
   private final PluginGuiceEnvironment env;
   private final ServerInformationImpl srvInfoImpl;
   private final PluginUser.Factory pluginUserFactory;
@@ -98,7 +89,7 @@ public class PluginLoader implements LifecycleListener {
   private final Provider<String> urlProvider;
   private final PersistentCacheFactory persistentCacheFactory;
   private final boolean remoteAdmin;
-  private final UniversalServerPluginProvider externalPluginFactory;
+  private final UniversalServerPluginProvider serverPluginFactory;
 
   @Inject
   public PluginLoader(SitePaths sitePaths,
@@ -109,10 +100,9 @@ public class PluginLoader implements LifecycleListener {
       @GerritServerConfig Config cfg,
       @CanonicalWebUrl Provider<String> provider,
       PersistentCacheFactory cacheFactory,
-      UniversalServerPluginProvider extPluginFactory) {
+      UniversalServerPluginProvider pluginFactory) {
     pluginsDir = sitePaths.plugins_dir;
     dataDir = sitePaths.data_dir;
-    tmpDir = sitePaths.tmp_dir;
     env = pe;
     srvInfoImpl = sii;
     pluginUserFactory = puf;
@@ -124,7 +114,7 @@ public class PluginLoader implements LifecycleListener {
     cleaner = pct;
     urlProvider = provider;
     persistentCacheFactory = cacheFactory;
-    externalPluginFactory = extPluginFactory;
+    serverPluginFactory = pluginFactory;
 
     remoteAdmin =
         cfg.getBoolean("plugins", null, "allowRemoteAdmin", false);
@@ -203,32 +193,18 @@ public class PluginLoader implements LifecycleListener {
     }
   }
 
-  public static File storeInTemp(String pluginName, InputStream in,
-      SitePaths sitePaths) throws IOException {
-    if (!sitePaths.tmp_dir.exists()) {
-      sitePaths.tmp_dir.mkdirs();
-    }
-    return asTemp(in, tempNameFor(pluginName), ".jar", sitePaths.tmp_dir);
-  }
-
-  private static File asTemp(InputStream in,
-      String prefix, String suffix,
-      File dir) throws IOException {
+  static File asTemp(InputStream in, String prefix, String suffix, File dir)
+      throws IOException {
     File tmp = File.createTempFile(prefix, suffix, dir);
     boolean keep = false;
-    try {
-      FileOutputStream out = new FileOutputStream(tmp);
-      try {
-        byte[] data = new byte[8192];
-        int n;
-        while ((n = in.read(data)) > 0) {
-          out.write(data, 0, n);
-        }
-        keep = true;
-        return tmp;
-      } finally {
-        out.close();
+    try (FileOutputStream out = new FileOutputStream(tmp)) {
+      byte[] data = new byte[8192];
+      int n;
+      while ((n = in.read(data)) > 0) {
+        out.write(data, 0, n);
       }
+      keep = true;
+      return tmp;
     } finally {
       if (!keep) {
         tmp.delete();
@@ -388,10 +364,8 @@ public class PluginLoader implements LifecycleListener {
       String name = entry.getKey();
       File file = entry.getValue();
       String fileName = file.getName();
-      if (!isJarPlugin(fileName) && !isJsPlugin(fileName)
-          && !externalPluginFactory.handles(file)) {
-        log.warn("File is not a JAR nor JS plugin and "
-            + "no Plugin provider was found that handles it: {}", fileName);
+      if (!isJsPlugin(fileName) && !serverPluginFactory.handles(file)) {
+        log.warn("No Plugin provider was found that handles this file format: {}", fileName);
         continue;
       }
 
@@ -455,11 +429,15 @@ public class PluginLoader implements LifecycleListener {
     FileSnapshot snapshot = FileSnapshot.save(plugin);
     try {
       Plugin newPlugin = loadPlugin(name, plugin, snapshot);
-      name = newPlugin.getName(); // Pluggable plugin provider may have assigned
-                                  // a plugin name that could be actually
-                                  // different from the initial
-                                  // one assigned during scan. It is safer then
-                                  // to reassign it.
+      if (newPlugin.getCleanupHandle() != null) {
+        cleanupHandles.put(newPlugin, newPlugin.getCleanupHandle());
+      }
+      /*
+       * Pluggable plugin provider may have assigned a plugin name that could be
+       * actually different from the initial one assigned during scan. It is
+       * safer then to reassign it.
+       */
+      name = newPlugin.getName();
       boolean reload = oldPlugin != null
           && oldPlugin.canReload()
           && newPlugin.canReload();
@@ -549,10 +527,6 @@ public class PluginLoader implements LifecycleListener {
     return 0 < ext ? name.substring(0, ext) : name;
   }
 
-  private static String getExtension(File file) {
-    return getExtension(file.getName());
-  }
-
   private static String getExtension(String name) {
     int ext = name.lastIndexOf('.');
     return 0 < ext ? name.substring(ext) : "";
@@ -561,67 +535,16 @@ public class PluginLoader implements LifecycleListener {
   private Plugin loadPlugin(String name, File srcPlugin, FileSnapshot snapshot)
       throws IOException, ClassNotFoundException, InvalidPluginException {
     String pluginName = srcPlugin.getName();
-    if (isJarPlugin(pluginName)) {
-      File tmp;
-      FileInputStream in = new FileInputStream(srcPlugin);
-      String extension = getExtension(srcPlugin);
-      try {
-        tmp = asTemp(in, tempNameFor(name), extension, tmpDir);
-      } finally {
-        in.close();
-      }
-      return loadJarPlugin(name, srcPlugin, snapshot, tmp);
-    } else if (isJsPlugin(pluginName)) {
+    if (isJsPlugin(pluginName)) {
       return loadJsPlugin(name, srcPlugin, snapshot);
-    } else if (externalPluginFactory.handles(srcPlugin)) {
-      return loadExternalPlugin(srcPlugin, snapshot);
+    } else if (serverPluginFactory.handles(srcPlugin)) {
+      return loadServerPlugin(srcPlugin, snapshot);
     } else {
       throw new InvalidPluginException(String.format(
           "Unsupported plugin type: %s", srcPlugin.getName()));
     }
   }
 
-  private Plugin loadJarPlugin(String name, File srcJar, FileSnapshot snapshot,
-      File tmp) throws IOException, InvalidPluginException,
-      MalformedURLException, ClassNotFoundException {
-    JarFile jarFile = new JarFile(tmp);
-    boolean keep = false;
-    try {
-      Manifest manifest = jarFile.getManifest();
-      Plugin.ApiType type = Plugin.getApiType(manifest);
-
-      List<URL> urls = new ArrayList<>(2);
-      String overlay = System.getProperty("gerrit.plugin-classes");
-      if (overlay != null) {
-        File classes = new File(new File(new File(overlay), name), "main");
-        if (classes.isDirectory()) {
-          log.info(String.format(
-              "plugin %s: including %s",
-              name, classes.getPath()));
-          urls.add(classes.toURI().toURL());
-        }
-      }
-      urls.add(tmp.toURI().toURL());
-
-      ClassLoader pluginLoader = new URLClassLoader(
-          urls.toArray(new URL[urls.size()]),
-          parentFor(type));
-
-      Plugin plugin = new ServerPlugin(name, getPluginCanonicalWebUrl(name),
-          pluginUserFactory.create(name),
-          srcJar, snapshot,
-          new JarScanner(srcJar),
-          getPluginDataDir(name), pluginLoader);
-      cleanupHandles.put(plugin, new CleanupHandle(tmp, jarFile));
-      keep = true;
-      return plugin;
-    } finally {
-      if (!keep) {
-        jarFile.close();
-      }
-    }
-  }
-
   private File getPluginDataDir(String name) {
     return new File(dataDir, name);
   }
@@ -637,15 +560,15 @@ public class PluginLoader implements LifecycleListener {
     return new JsPlugin(name, srcJar, pluginUserFactory.create(name), snapshot);
   }
 
-  private ServerPlugin loadExternalPlugin(File scriptFile, FileSnapshot snapshot)
-      throws InvalidPluginException {
-    String name = externalPluginFactory.getPluginName(scriptFile);
-    return externalPluginFactory.get(scriptFile, snapshot,
-        new PluginDescription(pluginUserFactory.create(name),
-            getPluginCanonicalWebUrl(name), getPluginDataDir(name)));
+  private ServerPlugin loadServerPlugin(File scriptFile,
+      FileSnapshot snapshot) throws InvalidPluginException {
+    String name = serverPluginFactory.getPluginName(scriptFile);
+    return serverPluginFactory.get(scriptFile, snapshot, new PluginDescription(
+        pluginUserFactory.create(name), getPluginCanonicalWebUrl(name),
+        getPluginDataDir(name)));
   }
 
-  private static ClassLoader parentFor(Plugin.ApiType type)
+  static ClassLoader parentFor(Plugin.ApiType type)
       throws InvalidPluginException {
     switch (type) {
       case EXTENSION:
@@ -659,22 +582,17 @@ public class PluginLoader implements LifecycleListener {
     }
   }
 
-  private static String tempNameFor(String name) {
-    SimpleDateFormat fmt = new SimpleDateFormat("yyMMdd_HHmm");
-    return PLUGIN_TMP_PREFIX + name + "_" + fmt.format(new Date()) + "_";
-  }
-
   // Only one active plugin per plugin name can exist for each plugin name.
   // Filter out disabled plugins and transform the multimap to a map
   private static Map<String, File> filterDisabled(
-      Multimap<String, File> jars) {
+      Multimap<String, File> pluginFiles) {
     Map<String, File> activePlugins = Maps.newHashMapWithExpectedSize(
-        jars.keys().size());
-    for (String name : jars.keys()) {
-      for (File jar : jars.asMap().get(name)) {
-        if (!jar.getName().endsWith(".disabled")) {
+        pluginFiles.keys().size());
+    for (String name : pluginFiles.keys()) {
+      for (File pluginFile : pluginFiles.asMap().get(name)) {
+        if (!pluginFile.getName().endsWith(".disabled")) {
           assert(!activePlugins.containsKey(name));
-          activePlugins.put(name, jar);
+          activePlugins.put(name, pluginFile);
         }
       }
     }
@@ -763,23 +681,13 @@ public class PluginLoader implements LifecycleListener {
     });
   }
 
-  public static String getGerritJarPluginName(File srcFile) throws IOException {
-    try (JarFile jarFile = new JarFile(srcFile)) {
-      return jarFile.getManifest().getMainAttributes()
-          .getValue("Gerrit-PluginName");
-    }
-  }
-
   public String getGerritPluginName(File srcFile) throws IOException {
     String fileName = srcFile.getName();
-    if (isJarPlugin(fileName)) {
-      return getGerritJarPluginName(srcFile);
-    }
     if (isJsPlugin(fileName)) {
       return fileName.substring(0, fileName.length() - 3);
     }
-    if (externalPluginFactory.handles(srcFile)) {
-      return externalPluginFactory.getPluginName(srcFile);
+    if (serverPluginFactory.handles(srcFile)) {
+      return serverPluginFactory.getPluginName(srcFile);
     }
     return null;
   }
@@ -793,10 +701,6 @@ public class PluginLoader implements LifecycleListener {
     return map;
   }
 
-  private static boolean isJarPlugin(String name) {
-    return isPlugin(name, "jar");
-  }
-
   private static boolean isJsPlugin(String name) {
     return isPlugin(name, "js");
   }

gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginModule.java

@@ -31,6 +31,8 @@ public class PluginModule extends LifecycleModule {
     listener().to(PluginLoader.class);
 
     DynamicSet.setOf(binder(), ServerPluginProvider.class);
+    DynamicSet.bind(binder(), ServerPluginProvider.class).to(
+        JarPluginProvider.class);
     bind(UniversalServerPluginProvider.class);
   }
 }