Add tag to make extended plugin optional

Signed-off-by: Craig Perkins <[email protected]>
cwperks · Jan 3, 2025 · 7e75335 · 7e75335
1 parent 28d80e7
commit 7e75335
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 8 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -59,7 +59,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ### Changed
 - Indexed IP field supports `terms_query` with more than 1025 IP masks [#16391](https://github.com/opensearch-project/OpenSearch/pull/16391)
 - Make entries for dependencies from server/build.gradle to gradle version catalog ([#16707](https://github.com/opensearch-project/OpenSearch/pull/16707))
-- Make extended plugins optional ([#16909](https://github.com/opensearch-project/OpenSearch/pull/16909))
+- Allow extended plugins to be optional ([#16909](https://github.com/opensearch-project/OpenSearch/pull/16909))
 
 ### Deprecated
 - Performing update operation with default pipeline or final pipeline is deprecated ([#16712](https://github.com/opensearch-project/OpenSearch/pull/16712))

diff --git a/server/src/main/java/org/opensearch/plugins/PluginsService.java b/server/src/main/java/org/opensearch/plugins/PluginsService.java
@@ -497,6 +497,11 @@ static List<Bundle> sortBundles(Set<Bundle> bundles) {
         return new ArrayList<>(sortedBundles);
     }
 
+    static boolean isExtendedPluginOptional(String extendedPlugin) {
+        String[] dependency = extendedPlugin.split(";");
+        return dependency.length > 1 && "optional=true".equals(dependency[1]);
+    }
+
     // add the given bundle to the sorted bundles, first adding dependencies
     private static void addSortedBundle(
         Bundle bundle,
@@ -522,11 +527,16 @@ private static void addSortedBundle(
 
         dependencyStack.add(name);
         for (String dependency : bundle.plugin.getExtendedPlugins()) {
-            Bundle depBundle = bundles.get(dependency);
+            String dependencyName = dependency.split(";")[0];
+            Bundle depBundle = bundles.get(dependencyName);
             if (depBundle == null) {
-                logger.warn("Missing plugin [" + dependency + "], dependency of [" + name + "]");
-                logger.warn("Some features of this plugin may not function without the dependencies being installed.\n");
-                continue;
+                if (isExtendedPluginOptional(dependency)) {
+                    logger.warn("Missing plugin [" + dependencyName + "], dependency of [" + name + "]");
+                    logger.warn("Some features of this plugin may not function without the dependencies being installed.\n");
+                    continue;
+                } else {
+                    throw new IllegalArgumentException("Missing plugin [" + dependencyName + "], dependency of [" + name + "]");
+                }
             }
             addSortedBundle(depBundle, bundles, sortedBundles, dependencyStack);
             assert sortedBundles.contains(depBundle);
@@ -655,9 +665,10 @@ static void checkBundleJarHell(Set<URL> classpath, Bundle bundle, Map<String, Se
             Set<URL> urls = new HashSet<>();
             for (String extendedPlugin : exts) {
                 Set<URL> pluginUrls = transitiveUrls.get(extendedPlugin);
-                if (pluginUrls == null) {
+                if (pluginUrls == null && isExtendedPluginOptional(extendedPlugin)) {
                     continue;
                 }
+                assert pluginUrls != null : "transitive urls should have already been set for " + extendedPlugin;
 
                 Set<URL> intersection = new HashSet<>(urls);
                 intersection.retainAll(pluginUrls);

diff --git a/server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java b/server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java
@@ -75,6 +75,7 @@
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.hasToString;
 import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.sameInstance;
 
@@ -361,7 +362,24 @@ public void testSortBundlesNoDeps() throws Exception {
         assertThat(sortedBundles, Matchers.contains(bundle1, bundle2, bundle3));
     }
 
-    public void testSortBundlesMissingDep() throws Exception {
+    public void testIsOptionalExtendedPlugin() {
+        assertThat(PluginsService.isExtendedPluginOptional("plugin-dep"), is(false));
+        assertThat(PluginsService.isExtendedPluginOptional("plugin-dep;optional=true"), is(true));
+        assertThat(PluginsService.isExtendedPluginOptional("plugin-dep;optional=false"), is(false));
+    }
+
+    public void testSortBundlesMissingRequiredDep() throws Exception {
+        Path pluginDir = createTempDir();
+        PluginInfo info = new PluginInfo("foo", "desc", "1.0", Version.CURRENT, "1.8", "MyPlugin", Collections.singletonList("dne"), false);
+        PluginsService.Bundle bundle = new PluginsService.Bundle(info, pluginDir);
+        IllegalArgumentException e = expectThrows(
+            IllegalArgumentException.class,
+            () -> PluginsService.sortBundles(Collections.singleton(bundle))
+        );
+        assertEquals("Missing plugin [dne], dependency of [foo]", e.getMessage());
+    }
+
+    public void testSortBundlesMissingOptionalDep() throws Exception {
         try (MockLogAppender mockLogAppender = MockLogAppender.createForLoggers(LogManager.getLogger(PluginsService.class))) {
             mockLogAppender.addExpectation(
                 new MockLogAppender.SeenEventExpectation(
@@ -379,7 +397,7 @@ public void testSortBundlesMissingDep() throws Exception {
                 Version.CURRENT,
                 "1.8",
                 "MyPlugin",
-                Collections.singletonList("dne"),
+                Collections.singletonList("dne;optional=true"),
                 false
             );
             PluginsService.Bundle bundle = new PluginsService.Bundle(info, pluginDir);