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 --------------------- 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..b311886c5fd 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,20 @@ public void m2(SolrQueryRequest req, SolrQueryResponse rsp) { } } + public static class ConfigurablePluginWithValidation + implements ConfigurablePlugin { + @Override + public void configure(ValidatableConfig cfg) { + if (!cfg.willPassValidation) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "invalid config"); + } + } + } + + public static class ValidatableConfig implements ReflectMapWriter { + @JsonProperty public boolean willPassValidation; + } + private Callable getPlugin(String path) { V2Request req = new V2Request.Builder(path).forceV2(forceV2).GET().build(); return () -> {