Enhance extensible plugin (#58542)

Rather than let ExtensiblePlugins know extending plugins' classloaders,
we now pass along an explicit ExtensionLoader that loads the extensions
asked for. Extensions constructed that way can optionally receive their
own Plugin instance in the constructor.

modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessPlugin.java

@@ -54,7 +54,6 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.ServiceLoader;
 import java.util.function.Supplier;
 
 /**
@@ -113,14 +112,14 @@ public List<Setting<?>> getSettings() {
     }
 
     @Override
-    public void reloadSPI(ClassLoader loader) {
-        for (PainlessExtension extension : ServiceLoader.load(PainlessExtension.class, loader)) {
-            for (Map.Entry<ScriptContext<?>, List<Whitelist>> entry : extension.getContextWhitelists().entrySet()) {
+    public void loadExtensions(ExtensionLoader loader) {
+        loader.loadExtensions(PainlessExtension.class).stream()
+            .flatMap(extension -> extension.getContextWhitelists().entrySet().stream())
+            .forEach(entry -> {
                 List<Whitelist> existing = whitelists.computeIfAbsent(entry.getKey(),
                     c -> new ArrayList<>(Whitelist.BASE_WHITELISTS));
                 existing.addAll(entry.getValue());
-            }
-        }
+            });
     }
 
     @Override

server/src/main/java/org/elasticsearch/plugins/ExtensiblePlugin.java

@@ -19,6 +19,8 @@
 
 package org.elasticsearch.plugins;
 
+import java.util.List;
+
 /**
  * An extension point for {@link Plugin} implementations to be themselves extensible.
  *
@@ -27,8 +29,22 @@
  */
 public interface ExtensiblePlugin {
 
+    interface ExtensionLoader {
+        /**
+         * Load extensions of the type from all extending plugins. The concrete extensions must have either a no-arg constructor
+         * or a single-arg constructor accepting the specific plugin class.
+         * @param extensionPointType the extension point type
+         * @param <T> extension point type
+         * @return all implementing extensions.
+         */
+        <T> List<T> loadExtensions(Class<T> extensionPointType);
+    }
+
     /**
-     * Reload any SPI implementations from the given classloader.
+     * Allow this plugin to load extensions from other plugins.
+     *
+     * This method is called once only, after initializing this plugin and all plugins extending this plugin. It is called before
+     * any other methods on this Plugin instance are called.
      */
-    default void reloadSPI(ClassLoader loader) {}
+    default void loadExtensions(ExtensionLoader loader) {}
 }

server/src/main/java/org/elasticsearch/plugins/PluginsService.java

@@ -27,6 +27,7 @@
 import org.apache.lucene.codecs.Codec;
 import org.apache.lucene.codecs.DocValuesFormat;
 import org.apache.lucene.codecs.PostingsFormat;
+import org.apache.lucene.util.SPIClassIterator;
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.Version;
 import org.elasticsearch.action.admin.cluster.node.info.PluginsAndModules;
@@ -466,17 +467,99 @@ private List<Tuple<PluginInfo,Plugin>> loadBundles(Set<Bundle> bundles) {
         Map<String, Plugin> loaded = new HashMap<>();
         Map<String, Set<URL>> transitiveUrls = new HashMap<>();
         List<Bundle> sortedBundles = sortBundles(bundles);
-
         for (Bundle bundle : sortedBundles) {
             checkBundleJarHell(JarHell.parseClassPath(), bundle, transitiveUrls);
 
             final Plugin plugin = loadBundle(bundle, loaded);
             plugins.add(new Tuple<>(bundle.plugin, plugin));
         }
 
+        loadExtensions(plugins);
         return Collections.unmodifiableList(plugins);
     }
 
+    // package-private for test visibility
+    static void loadExtensions(List<Tuple<PluginInfo, Plugin>> plugins) {
+        Map<String, List<Plugin>> extendingPluginsByName = plugins.stream()
+            .flatMap(t -> t.v1().getExtendedPlugins().stream().map(extendedPlugin -> Tuple.tuple(extendedPlugin, t.v2())))
+            .collect(Collectors.groupingBy(Tuple::v1, Collectors.mapping(Tuple::v2, Collectors.toList())));
+        for (Tuple<PluginInfo, Plugin> pluginTuple : plugins) {
+            if (pluginTuple.v2() instanceof ExtensiblePlugin) {
+                loadExtensionsForPlugin((ExtensiblePlugin) pluginTuple.v2(),
+                    extendingPluginsByName.getOrDefault(pluginTuple.v1().getName(), Collections.emptyList()));
+            }
+        }
+    }
+
+    private static void loadExtensionsForPlugin(ExtensiblePlugin extensiblePlugin, List<Plugin> extendingPlugins) {
+        ExtensiblePlugin.ExtensionLoader extensionLoader = new ExtensiblePlugin.ExtensionLoader() {
+            @Override
+            public <T> List<T> loadExtensions(Class<T> extensionPointType) {
+                List<T> result = new ArrayList<>();
+                for (Plugin extendingPlugin : extendingPlugins) {
+                    result.addAll(createExtensions(extensionPointType, extendingPlugin));
+                }
+                return Collections.unmodifiableList(result);
+            }
+        };
+
+        extensiblePlugin.loadExtensions(extensionLoader);
+    }
+
+    private static <T> List<? extends T> createExtensions(Class<T> extensionPointType, Plugin plugin) {
+        SPIClassIterator<T> classIterator = SPIClassIterator.get(extensionPointType, plugin.getClass().getClassLoader());
+        List<T> extensions = new ArrayList<>();
+        while (classIterator.hasNext()) {
+            Class<? extends T> extensionClass = classIterator.next();
+            extensions.add(createExtension(extensionClass, extensionPointType, plugin));
+        }
+        return extensions;
+    }
+
+    // package-private for test visibility
+    static <T> T createExtension(Class<? extends T> extensionClass, Class<T> extensionPointType, Plugin plugin) {
+        //noinspection unchecked
+        Constructor<T>[] constructors = (Constructor<T>[]) extensionClass.getConstructors();
+        if (constructors.length == 0) {
+            throw new IllegalStateException("no public " + extensionConstructorMessage(extensionClass, extensionPointType));
+        }
+
+        if (constructors.length > 1) {
+            throw new IllegalStateException("no unique public " + extensionConstructorMessage(extensionClass, extensionPointType));
+        }
+
+        final Constructor<T> constructor = constructors[0];
+        if (constructor.getParameterCount() > 1) {
+            throw new IllegalStateException(extensionSignatureMessage(extensionClass, extensionPointType, plugin));
+        }
+
+        if (constructor.getParameterCount() == 1 && constructor.getParameterTypes()[0] != plugin.getClass()) {
+            throw new IllegalStateException(extensionSignatureMessage(extensionClass, extensionPointType, plugin) +
+                ", not (" + constructor.getParameterTypes()[0].getName() + ")");
+        }
+
+        try {
+            if (constructor.getParameterCount() == 0) {
+                return constructor.newInstance();
+            } else {
+                return constructor.newInstance(plugin);
+            }
+        } catch (ReflectiveOperationException e) {
+            throw new IllegalStateException(
+                "failed to create extension [" + extensionClass.getName() + "] of type [" + extensionPointType.getName() + "]", e
+            );
+        }
+    }
+
+    private static <T> String extensionSignatureMessage(Class<? extends T> extensionClass, Class<T> extensionPointType, Plugin plugin) {
+        return "signature of " + extensionConstructorMessage(extensionClass, extensionPointType) +
+            " must be either () or (" + plugin.getClass().getName() + ")";
+    }
+
+    private static <T> String extensionConstructorMessage(Class<? extends T> extensionClass, Class<T> extensionPointType) {
+        return "constructor for extension [" + extensionClass.getName() + "] of type [" + extensionPointType.getName() + "]";
+    }
+
     // jar-hell check the bundle against the parent classloader and extended plugins
     // the plugin cli does it, but we do it again, in case lusers mess with jar files manually
     static void checkBundleJarHell(Set<URL> classpath, Bundle bundle, Map<String, Set<URL>> transitiveUrls) {
@@ -549,12 +632,13 @@ private Plugin loadBundle(Bundle bundle, Map<String, Plugin> loaded) {
 
         // reload SPI with any new services from the plugin
         reloadLuceneSPI(loader);
-        for (String extendedPluginName : bundle.plugin.getExtendedPlugins()) {
-            // note: already asserted above that extended plugins are loaded and extensible
-            ExtensiblePlugin.class.cast(loaded.get(extendedPluginName)).reloadSPI(loader);
-        }
 
         Class<? extends Plugin> pluginClass = loadPluginClass(bundle.plugin.getClassname(), loader);
+        if (loader != pluginClass.getClassLoader()) {
+            throw new IllegalStateException("Plugin [" + name + "] must reference a class loader local Plugin class ["
+                + bundle.plugin.getClassname()
+                + "] (class loader [" + pluginClass.getClassLoader() + "])");
+        }
         Plugin plugin = loadPlugin(pluginClass, settings, configPath);
         loaded.put(name, plugin);
         return plugin;

server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java

@@ -24,6 +24,7 @@
 import org.apache.lucene.util.LuceneTestCase;
 import org.elasticsearch.Version;
 import org.elasticsearch.bootstrap.JarHell;
+import org.elasticsearch.common.collect.Tuple;
 import org.elasticsearch.common.io.PathUtils;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.env.Environment;
@@ -34,6 +35,7 @@
 
 import java.io.IOException;
 import java.io.InputStream;
+import java.lang.reflect.InvocationTargetException;
 import java.net.URL;
 import java.nio.file.FileSystemException;
 import java.nio.file.Files;
@@ -54,8 +56,11 @@
 
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.hasToString;
 import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.notNullValue;
+import static org.hamcrest.Matchers.sameInstance;
 
 @LuceneTestCase.SuppressFileSystems(value = "ExtrasFS")
 public class PluginsServiceTests extends ESTestCase {
@@ -690,4 +695,164 @@ public void testExistingMandatoryInstalledPlugin() throws IOException {
                         .build();
         newPluginsService(settings);
     }
+
+    public void testPluginFromParentClassLoader() throws IOException {
+        final Path pathHome = createTempDir();
+        final Path plugins = pathHome.resolve("plugins");
+        final Path fake = plugins.resolve("fake");
+
+        PluginTestUtil.writePluginProperties(
+            fake,
+            "description", "description",
+            "name", "fake",
+            "version", "1.0.0",
+            "elasticsearch.version", Version.CURRENT.toString(),
+            "java.version", System.getProperty("java.specification.version"),
+            "classname", TestPlugin.class.getName()); // set a class defined outside the bundle (in parent class-loader of plugin)
+
+        final Settings settings =
+            Settings.builder()
+                .put("path.home", pathHome)
+                .put("plugin.mandatory", "fake")
+                .build();
+        IllegalStateException exception = expectThrows(IllegalStateException.class, () -> newPluginsService(settings));
+        assertThat(exception, hasToString(containsString("Plugin [fake] must reference a class loader local Plugin class [" +
+            TestPlugin.class.getName() + "] (class loader [" + PluginsServiceTests.class.getClassLoader() + "])")));
+    }
+
+    public void testExtensiblePlugin() {
+        TestExtensiblePlugin extensiblePlugin = new TestExtensiblePlugin();
+        PluginsService.loadExtensions(Collections.singletonList(
+            Tuple.tuple(new PluginInfo("extensible", null, null, null, null, null, Collections.emptyList(), false), extensiblePlugin)
+        ));
+
+        assertThat(extensiblePlugin.extensions, notNullValue());
+        assertThat(extensiblePlugin.extensions, hasSize(0));
+
+        extensiblePlugin = new TestExtensiblePlugin();
+        TestPlugin testPlugin = new TestPlugin();
+        PluginsService.loadExtensions(Arrays.asList(
+            Tuple.tuple(new PluginInfo("extensible", null, null, null, null, null, Collections.emptyList(), false), extensiblePlugin),
+            Tuple.tuple(new PluginInfo("test", null, null, null, null, null, Collections.singletonList("extensible"), false), testPlugin)
+        ));
+
+        assertThat(extensiblePlugin.extensions, notNullValue());
+        assertThat(extensiblePlugin.extensions, hasSize(2));
+        assertThat(extensiblePlugin.extensions.get(0), instanceOf(TestExtension1.class));
+        assertThat(extensiblePlugin.extensions.get(1), instanceOf(TestExtension2.class));
+        assertThat(((TestExtension2) extensiblePlugin.extensions.get(1)).plugin, sameInstance(testPlugin));
+    }
+
+    public void testNoExtensionConstructors() {
+        TestPlugin plugin = new TestPlugin();
+        class TestExtension implements TestExtensionPoint {
+            private TestExtension() {
+            }
+        }
+        IllegalStateException e = expectThrows(IllegalStateException.class, () -> {
+            PluginsService.createExtension(TestExtension.class, TestExtensionPoint.class, plugin);
+        });
+
+        assertThat(e, hasToString(containsString("no public constructor for extension [" + TestExtension.class.getName() +
+            "] of type [" + TestExtensionPoint.class.getName() + "]")));
+    }
+
+    public void testMultipleExtensionConstructors() {
+        TestPlugin plugin = new TestPlugin();
+        class TestExtension implements TestExtensionPoint {
+            public TestExtension() {
+            }
+            public TestExtension(TestPlugin plugin) {
+
+            }
+        }
+        IllegalStateException e = expectThrows(IllegalStateException.class, () -> {
+            PluginsService.createExtension(TestExtension.class, TestExtensionPoint.class, plugin);
+        });
+
+        assertThat(e, hasToString(containsString("no unique public constructor for extension [" + TestExtension.class.getName() +
+            "] of type [" + TestExtensionPoint.class.getName() + "]")));
+    }
+
+    public void testBadSingleParameterConstructor() {
+        TestPlugin plugin = new TestPlugin();
+        IllegalStateException e = expectThrows(IllegalStateException.class, () -> {
+            PluginsService.createExtension(BadSingleParameterConstructorExtension.class, TestExtensionPoint.class, plugin);
+        });
+
+        assertThat(e,
+            hasToString(containsString("signature of constructor for extension [" + BadSingleParameterConstructorExtension.class.getName() +
+                "] of type [" + TestExtensionPoint.class.getName() + "] must be either () or (" + TestPlugin.class.getName() + "), not (" +
+                String.class.getName() + ")")));
+    }
+
+    public void testTooManyParametersExtensionConstructors() {
+        TestPlugin plugin = new TestPlugin();
+        IllegalStateException e = expectThrows(IllegalStateException.class, () -> {
+            PluginsService.createExtension(TooManyParametersConstructorExtension.class, TestExtensionPoint.class, plugin);
+        });
+
+        assertThat(e,
+            hasToString(containsString("signature of constructor for extension [" + TooManyParametersConstructorExtension.class.getName() +
+                "] of type [" + TestExtensionPoint.class.getName() + "] must be either () or (" + TestPlugin.class.getName() + ")")));
+    }
+
+    public void testThrowingConstructor() {
+        TestPlugin plugin = new TestPlugin();
+        IllegalStateException e = expectThrows(IllegalStateException.class, () -> {
+            PluginsService.createExtension(ThrowingConstructorExtension.class, TestExtensionPoint.class, plugin);
+        });
+
+        assertThat(e,
+            hasToString(containsString("failed to create extension [" + ThrowingConstructorExtension.class.getName() +
+                "] of type [" + TestExtensionPoint.class.getName() + "]")));
+        assertThat(e.getCause(), instanceOf(InvocationTargetException.class));
+        assertThat(e.getCause().getCause(), instanceOf(IllegalArgumentException.class));
+        assertThat(e.getCause().getCause(), hasToString(containsString("test constructor failure")));
+    }
+
+    private static class TestExtensiblePlugin extends Plugin implements ExtensiblePlugin {
+        private List<TestExtensionPoint> extensions;
+
+        @Override
+        public void loadExtensions(ExtensionLoader loader) {
+            assert extensions == null;
+            extensions = loader.loadExtensions(TestExtensionPoint.class);
+            // verify unmodifiable.
+            expectThrows(UnsupportedOperationException.class, () -> extensions.add(new TestExtension1()));
+        }
+    }
+
+    public static class TestPlugin extends Plugin {
+    }
+
+    public interface TestExtensionPoint {
+    }
+
+    public static class TestExtension1 implements TestExtensionPoint {
+    }
+
+    public static class TestExtension2 implements TestExtensionPoint {
+        public Plugin plugin;
+
+        public TestExtension2(TestPlugin plugin) {
+            this.plugin = plugin;
+        }
+    }
+
+    public static class BadSingleParameterConstructorExtension implements TestExtensionPoint {
+        public BadSingleParameterConstructorExtension(String bad) {
+        }
+    }
+
+    public static class TooManyParametersConstructorExtension implements TestExtensionPoint {
+        public TooManyParametersConstructorExtension(String bad) {
+        }
+    }
+
+    public static class ThrowingConstructorExtension implements TestExtensionPoint {
+        public ThrowingConstructorExtension() {
+            throw new IllegalArgumentException("test constructor failure");
+        }
+    }
 }