From 63629d8a62b30f1172e28798ccaf13b73b53b17e Mon Sep 17 00:00:00 2001 From: Yohann Callea Date: Wed, 17 Jan 2024 15:36:32 +0100 Subject: [PATCH 1/3] Fix an undesired exception swallowing in ContainerPluginsApi.validateConfig(...) --- .../handler/admin/ContainerPluginsApi.java | 39 ++++++--------- .../solr/handler/TestContainerPlugin.java | 50 +++++++++++++++++++ 2 files changed, 66 insertions(+), 23 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/handler/admin/ContainerPluginsApi.java b/solr/core/src/java/org/apache/solr/handler/admin/ContainerPluginsApi.java index 910964783f9..0de5abc86a5 100644 --- a/solr/core/src/java/org/apache/solr/handler/admin/ContainerPluginsApi.java +++ b/solr/core/src/java/org/apache/solr/handler/admin/ContainerPluginsApi.java @@ -17,15 +17,12 @@ package org.apache.solr.handler.admin; -import static org.apache.lucene.util.IOUtils.closeWhileHandlingException; - import java.io.IOException; import java.lang.invoke.MethodHandles; import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.Objects; -import org.apache.solr.api.AnnotatedApi; import org.apache.solr.api.ClusterPluginsSource; import org.apache.solr.api.Command; import org.apache.solr.api.ContainerPluginsRegistry; @@ -130,28 +127,24 @@ public void update(PayloadObj payload) throws IOException { } private void validateConfig(PayloadObj payload, PluginMeta info) throws IOException { - if (info.klass.indexOf(':') > 0) { - if (info.version == null) { - payload.addError("Using package. must provide a packageVersion"); - return; - } - } - List errs = new ArrayList<>(); - ContainerPluginsRegistry.ApiInfo apiInfo = - coreContainer.getContainerPluginsRegistry().createInfo(payload.getDataMap(), errs); - if (!errs.isEmpty()) { - for (String err : errs) payload.addError(err); + if (info.klass.indexOf(':') > 0 && info.version == null) { + payload.addError("Using package. must provide a packageVersion"); return; } - AnnotatedApi api = null; - try { - apiInfo.init(); - } catch (Exception e) { - log.error("Error instantiating plugin ", e); - errs.add(e.getMessage()); - return; - } finally { - closeWhileHandlingException(api); + + final List errs = new ArrayList<>(); + final ContainerPluginsRegistry.ApiInfo apiInfo = + coreContainer.getContainerPluginsRegistry().createInfo(payload.getDataMap(), errs); + + if (errs.isEmpty()) { + try { + apiInfo.init(); + } catch (Exception e) { + log.error("Error instantiating plugin ", e); + errs.add(e.getMessage()); + } } + + errs.forEach(payload::addError); } } diff --git a/solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java b/solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java index fab149129f2..a45d6263d4e 100644 --- a/solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java +++ b/solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java @@ -47,6 +47,7 @@ import org.apache.solr.client.solrj.response.V2Response; import org.apache.solr.cloud.ClusterSingleton; import org.apache.solr.cloud.SolrCloudTestCase; +import org.apache.solr.common.SolrException; import org.apache.solr.common.annotation.JsonProperty; import org.apache.solr.common.util.ReflectMapWriter; import org.apache.solr.core.CoreContainer; @@ -377,6 +378,37 @@ public void testApiFromPackage() throws Exception { assertEquals(1452, C5.classData.limit()); } + @Test + public void testPluginConfigValidation() throws Exception { + final Callable readPluginState = getPlugin("/cluster/plugin"); + + // Register a plugin with an invalid configuration + final ValidatableConfig config = new ValidatableConfig(); + config.willPassValidation = false; + + final PluginMeta plugin = new PluginMeta(); + plugin.name = "validatableplugin"; + plugin.klass = ConfigurablePluginWithValidation.class.getName(); + plugin.config = config; + + final V2Request addPlugin = postPlugin(singletonMap("add", plugin)); + + // Verify that the expected error is thrown and the plugin is not registered + expectError(addPlugin, "invalid config"); + V2Response response = readPluginState.call(); + assertNull(response._getStr("/plugin/validatableplugin/class", null)); + + // Now register it with a valid configuration + config.willPassValidation = true; + addPlugin.process(cluster.getSolrClient()); + + // Verify that the plugin is properly registered + response = readPluginState.call(); + assertEquals( + ConfigurablePluginWithValidation.class.getName(), + response._getStr("/plugin/validatableplugin/class", null)); + } + public static class CC1 extends CC {} public static class CC2 extends CC1 {} @@ -509,6 +541,24 @@ public void m2(SolrQueryRequest req, SolrQueryResponse rsp) { } } + public static class ConfigurablePluginWithValidation + implements ConfigurablePlugin { + @Override + public void configure(ValidatableConfig cfg) { + cfg.validate(); + } + } + + public static class ValidatableConfig implements ReflectMapWriter { + @JsonProperty public boolean willPassValidation; + + public void validate() { + if (!willPassValidation) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "invalid config"); + } + } + } + private Callable getPlugin(String path) { V2Request req = new V2Request.Builder(path).forceV2(forceV2).GET().build(); return () -> { From a5b7088a5dd2075afb7a28ada58f936127a72a04 Mon Sep 17 00:00:00 2001 From: Yohann Callea Date: Fri, 19 Jan 2024 11:13:57 +0100 Subject: [PATCH 2/3] Minor update following review to improve tests readability --- .../org/apache/solr/handler/TestContainerPlugin.java | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java b/solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java index a45d6263d4e..b311886c5fd 100644 --- a/solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java +++ b/solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java @@ -545,18 +545,14 @@ public static class ConfigurablePluginWithValidation implements ConfigurablePlugin { @Override public void configure(ValidatableConfig cfg) { - cfg.validate(); + if (!cfg.willPassValidation) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "invalid config"); + } } } public static class ValidatableConfig implements ReflectMapWriter { @JsonProperty public boolean willPassValidation; - - public void validate() { - if (!willPassValidation) { - throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "invalid config"); - } - } } private Callable getPlugin(String path) { From 02460764ccb269462486bf63198288517e2c4fd2 Mon Sep 17 00:00:00 2001 From: Yohann Callea Date: Tue, 23 Jan 2024 15:12:04 +0100 Subject: [PATCH 3/3] Update CHANGES.txt --- solr/CHANGES.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 33446ed23e0..b2c00b14911 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -83,7 +83,8 @@ New Features Improvements --------------------- -(No changes) +* SOLR-17119: When registering or updating a ConfigurablePlugin through the `/cluster/plugin` API, + config validation exceptions are now propagated to the callers. (Yohann Callea) Optimizations ---------------------