Refactor PluginsLoader to better support tests (#117522)

This refactors the way PluginsLoader is created to better support
various types of testing.

Author: jdconrad

benchmarks/src/main/java/org/elasticsearch/benchmark/script/ScriptScoreBenchmark.java

@@ -77,7 +77,7 @@ public class ScriptScoreBenchmark {
     private final PluginsService pluginsService = new PluginsService(
         Settings.EMPTY,
         null,
-        new PluginsLoader(null, Path.of(System.getProperty("plugins.dir")))
+        PluginsLoader.createPluginsLoader(null, Path.of(System.getProperty("plugins.dir")))
     );
     private final ScriptModule scriptModule = new ScriptModule(Settings.EMPTY, pluginsService.filterPlugins(ScriptPlugin.class).toList());
 

server/src/main/java/org/elasticsearch/bootstrap/Elasticsearch.java

@@ -206,7 +206,7 @@ private static void initPhase2(Bootstrap bootstrap) throws IOException {
         );
 
         // load the plugin Java modules and layers now for use in entitlements
-        var pluginsLoader = new PluginsLoader(nodeEnv.modulesFile(), nodeEnv.pluginsFile());
+        var pluginsLoader = PluginsLoader.createPluginsLoader(nodeEnv.modulesFile(), nodeEnv.pluginsFile());
         bootstrap.setPluginsLoader(pluginsLoader);
 
         if (Boolean.parseBoolean(System.getProperty("es.entitlements.enabled"))) {

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

@@ -118,15 +118,30 @@ public static LayerAndLoader ofLoader(ClassLoader loader) {
      * @param modulesDirectory The directory modules exist in, or null if modules should not be loaded from the filesystem
      * @param pluginsDirectory The directory plugins exist in, or null if plugins should not be loaded from the filesystem
      */
-    @SuppressWarnings("this-escape")
-    public PluginsLoader(Path modulesDirectory, Path pluginsDirectory) {
+    public static PluginsLoader createPluginsLoader(Path modulesDirectory, Path pluginsDirectory) {
+        return createPluginsLoader(modulesDirectory, pluginsDirectory, true);
+    }
 
-        Map<String, List<ModuleQualifiedExportsService>> qualifiedExports = new HashMap<>(ModuleQualifiedExportsService.getBootServices());
-        addServerExportsService(qualifiedExports);
+    /**
+     * Constructs a new PluginsLoader
+     *
+     * @param modulesDirectory The directory modules exist in, or null if modules should not be loaded from the filesystem
+     * @param pluginsDirectory The directory plugins exist in, or null if plugins should not be loaded from the filesystem
+     * @param withServerExports {@code true} to add server module exports
+     */
+    public static PluginsLoader createPluginsLoader(Path modulesDirectory, Path pluginsDirectory, boolean withServerExports) {
+        Map<String, List<ModuleQualifiedExportsService>> qualifiedExports;
+        if (withServerExports) {
+            qualifiedExports = new HashMap<>(ModuleQualifiedExportsService.getBootServices());
+            addServerExportsService(qualifiedExports);
+        } else {
+            qualifiedExports = Collections.emptyMap();
+        }
 
         Set<PluginBundle> seenBundles = new LinkedHashSet<>();
 
         // load (elasticsearch) module layers
+        List<PluginDescriptor> moduleDescriptors;
         if (modulesDirectory != null) {
             try {
                 Set<PluginBundle> modules = PluginsUtils.getModuleBundles(modulesDirectory);
@@ -140,6 +155,7 @@ public PluginsLoader(Path modulesDirectory, Path pluginsDirectory) {
         }
 
         // load plugin layers
+        List<PluginDescriptor> pluginDescriptors;
         if (pluginsDirectory != null) {
             try {
                 // TODO: remove this leniency, but tests bogusly rely on it
@@ -158,7 +174,28 @@ public PluginsLoader(Path modulesDirectory, Path pluginsDirectory) {
             pluginDescriptors = Collections.emptyList();
         }
 
-        this.loadedPluginLayers = Collections.unmodifiableMap(loadPluginLayers(seenBundles, qualifiedExports));
+        Map<String, LoadedPluginLayer> loadedPluginLayers = new LinkedHashMap<>();
+        Map<String, Set<URL>> transitiveUrls = new HashMap<>();
+        List<PluginBundle> sortedBundles = PluginsUtils.sortBundles(seenBundles);
+        if (sortedBundles.isEmpty() == false) {
+            Set<URL> systemLoaderURLs = JarHell.parseModulesAndClassPath();
+            for (PluginBundle bundle : sortedBundles) {
+                PluginsUtils.checkBundleJarHell(systemLoaderURLs, bundle, transitiveUrls);
+                loadPluginLayer(bundle, loadedPluginLayers, qualifiedExports);
+            }
+        }
+
+        return new PluginsLoader(moduleDescriptors, pluginDescriptors, loadedPluginLayers);
+    }
+
+    PluginsLoader(
+        List<PluginDescriptor> moduleDescriptors,
+        List<PluginDescriptor> pluginDescriptors,
+        Map<String, LoadedPluginLayer> loadedPluginLayers
+    ) {
+        this.moduleDescriptors = moduleDescriptors;
+        this.pluginDescriptors = pluginDescriptors;
+        this.loadedPluginLayers = loadedPluginLayers;
     }
 
     public List<PluginDescriptor> moduleDescriptors() {
@@ -173,25 +210,7 @@ public Stream<PluginLayer> pluginLayers() {
         return loadedPluginLayers.values().stream().map(Function.identity());
     }
 
-    private Map<String, LoadedPluginLayer> loadPluginLayers(
-        Set<PluginBundle> bundles,
-        Map<String, List<ModuleQualifiedExportsService>> qualifiedExports
-    ) {
-        Map<String, LoadedPluginLayer> loaded = new LinkedHashMap<>();
-        Map<String, Set<URL>> transitiveUrls = new HashMap<>();
-        List<PluginBundle> sortedBundles = PluginsUtils.sortBundles(bundles);
-        if (sortedBundles.isEmpty() == false) {
-            Set<URL> systemLoaderURLs = JarHell.parseModulesAndClassPath();
-            for (PluginBundle bundle : sortedBundles) {
-                PluginsUtils.checkBundleJarHell(systemLoaderURLs, bundle, transitiveUrls);
-                loadPluginLayer(bundle, loaded, qualifiedExports);
-            }
-        }
-
-        return loaded;
-    }
-
-    private void loadPluginLayer(
+    private static void loadPluginLayer(
         PluginBundle bundle,
         Map<String, LoadedPluginLayer> loaded,
         Map<String, List<ModuleQualifiedExportsService>> qualifiedExports
@@ -211,7 +230,7 @@ private void loadPluginLayer(
         }
 
         final ClassLoader parentLoader = ExtendedPluginsClassLoader.create(
-            getClass().getClassLoader(),
+            PluginsLoader.class.getClassLoader(),
             extendedPlugins.stream().map(LoadedPluginLayer::spiClassLoader).toList()
         );
         LayerAndLoader spiLayerAndLoader = null;
@@ -427,7 +446,7 @@ private static List<ModuleLayer> parentLayersOrBoot(List<ModuleLayer> parentLaye
         }
     }
 
-    protected void addServerExportsService(Map<String, List<ModuleQualifiedExportsService>> qualifiedExports) {
+    private static void addServerExportsService(Map<String, List<ModuleQualifiedExportsService>> qualifiedExports) {
         var exportsService = new ModuleQualifiedExportsService(serverModule) {
             @Override
             protected void addExports(String pkg, Module target) {

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

@@ -18,7 +18,6 @@
 import org.elasticsearch.env.Environment;
 import org.elasticsearch.env.TestEnvironment;
 import org.elasticsearch.index.IndexModule;
-import org.elasticsearch.jdk.ModuleQualifiedExportsService;
 import org.elasticsearch.plugin.analysis.CharFilterFactory;
 import org.elasticsearch.plugins.scanners.PluginInfo;
 import org.elasticsearch.plugins.spi.BarPlugin;
@@ -66,12 +65,11 @@ public class PluginsServiceTests extends ESTestCase {
     public static class FilterablePlugin extends Plugin implements ScriptPlugin {}
 
     static PluginsService newPluginsService(Settings settings) {
-        return new PluginsService(settings, null, new PluginsLoader(null, TestEnvironment.newEnvironment(settings).pluginsFile()) {
-            @Override
-            protected void addServerExportsService(Map<String, List<ModuleQualifiedExportsService>> qualifiedExports) {
-                // tests don't run modular
-            }
-        });
+        return new PluginsService(
+            settings,
+            null,
+            PluginsLoader.createPluginsLoader(null, TestEnvironment.newEnvironment(settings).pluginsFile(), false)
+        );
     }
 
     static PluginsService newMockPluginsService(List<Class<? extends Plugin>> classpathPlugins) {

test/framework/src/main/java/org/elasticsearch/plugins/MockPluginsService.java

@@ -16,7 +16,6 @@
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.util.concurrent.ConcurrentCollections;
 import org.elasticsearch.env.Environment;
-import org.elasticsearch.jdk.ModuleQualifiedExportsService;
 import org.elasticsearch.plugins.spi.SPIClassIterator;
 
 import java.lang.reflect.Constructor;
@@ -43,13 +42,11 @@ public class MockPluginsService extends PluginsService {
      * @param classpathPlugins Plugins that exist in the classpath which should be loaded
      */
     public MockPluginsService(Settings settings, Environment environment, Collection<Class<? extends Plugin>> classpathPlugins) {
-        super(settings, environment.configFile(), new PluginsLoader(environment.modulesFile(), environment.pluginsFile()) {
-
-            @Override
-            protected void addServerExportsService(Map<String, List<ModuleQualifiedExportsService>> qualifiedExports) {
-                // tests don't run modular
-            }
-        });
+        super(
+            settings,
+            environment.configFile(),
+            new PluginsLoader(Collections.emptyList(), Collections.emptyList(), Collections.emptyMap())
+        );
 
         List<LoadedPlugin> pluginsLoaded = new ArrayList<>();
 

x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/test/bench/WatcherScheduleEngineBenchmark.java

@@ -109,7 +109,10 @@ public static void main(String[] args) throws Exception {
 
         // First clean everything and index the watcher (but not via put alert api!)
         try (
-            Node node = new Node(internalNodeEnv, new PluginsLoader(internalNodeEnv.modulesFile(), internalNodeEnv.pluginsFile())).start()
+            Node node = new Node(
+                internalNodeEnv,
+                PluginsLoader.createPluginsLoader(internalNodeEnv.modulesFile(), internalNodeEnv.pluginsFile())
+            ).start()
         ) {
             final Client client = node.client();
             ClusterHealthResponse response = client.admin().cluster().prepareHealth(TimeValue.THIRTY_SECONDS).setWaitForNodes("2").get();