pvcnt commented on code in PR #2202:
URL: https://github.com/apache/solr/pull/2202#discussion_r1457106803
##########
solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java:
##########
@@ -509,6 +541,24 @@ public void m2(SolrQueryRequest req, SolrQueryResponse
rsp) {
}
}
+ public static class ConfigurablePluginWithValidation
+ implements ConfigurablePlugin<ValidatableConfig> {
+ @Override
+ public void configure(ValidatableConfig cfg) {
+ cfg.validate();
+ }
+ }
+
+ public static class ValidatableConfig implements ReflectMapWriter {
+ @JsonProperty public boolean willPassValidation;
+
+ public void validate() {
Review Comment:
Can this be merged into `ConfigurablePluginWithValidation#configure`? Again,
just to make the code easier to follow by avoiding one jump.
##########
solr/core/src/java/org/apache/solr/handler/admin/ContainerPluginsApi.java:
##########
@@ -134,29 +131,25 @@ public void update(PayloadObj<PluginMeta> payload) throws
IOException {
}
private void validateConfig(PayloadObj<PluginMeta> payload, PluginMeta info)
throws IOException {
- if (info.klass.indexOf(':') > 0) {
- if (info.version == null) {
- payload.addError("Using package. must provide a packageVersion");
- return;
- }
- }
- List<String> 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);
Review Comment:
Is this method (missing in the modified version) what was causing the
exception to be swallowed?
##########
solr/core/src/java/org/apache/solr/handler/admin/ContainerPluginsApi.java:
##########
@@ -134,29 +131,25 @@ public void update(PayloadObj<PluginMeta> payload) throws
IOException {
}
private void validateConfig(PayloadObj<PluginMeta> payload, PluginMeta info)
throws IOException {
- if (info.klass.indexOf(':') > 0) {
- if (info.version == null) {
- payload.addError("Using package. must provide a packageVersion");
- return;
- }
- }
- List<String> 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<String> 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);
Review Comment:
It doesn't change the outcome, but maybe it could be done in an `else`
block? It's difficult to explain why, but I find this statement difficult to
parse in my head in this current form.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
