ycallea commented on code in PR #2202:
URL: https://github.com/apache/solr/pull/2202#discussion_r1457446395
##########
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:
The exception was swallowed here because the error message was mistakenly
only added to the local `errs` list (line 155) but never included in the
response's payload.
The removal of the call to `closeWhileHandlingException(...)` is a minor
code clean-up.
In this context, the purpose of this method was to close the `AnnotatedApi`
declared above (line 150), but as it is never instantiated this finally block
always resulted in a no-op.
I therefore removed both this unused local variable declaration and the
finally block which was dedicated to closing this resource.
##########
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:
On a side note, the way the `createInfo(...)` method (and by extension the
`ApiInfo` constructor) is currently implemented feels a bit unintuitive to be
honest, you have to pass it a reference to a list that will be populated with
some error messages, should any validation error occur in here.
I wanted to keep the changes from this pull request small enough to only
remediate the identified bug, but I feel it would be worth refactoring as part
of another pull request.
In my opinion, we should just throw exceptions instead of relying on this
errors' list (see
[ContainerPluginsRegistry.java#L329-L413](https://github.com/apache/solr/blob/main/solr/core/src/java/org/apache/solr/api/ContainerPluginsRegistry.java#L329-L413)),
it would simplify the code and ease errors management when constructing
`ApiInfo` objects.
##########
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:
Sure, it can definitely be implemented this way :
```java
public static class ConfigurablePluginWithValidation implements
ConfigurablePlugin<ValidatableConfig> {
@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;
}
```
Is it what you have in mind ?
##########
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:
I chose to implement it this way so we are only adding errors in the
response's payload (`payload.addError(...)`) at a single place.
We could however also do it as such, to keep the same flow as before my
change if it improves readability :
```java
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);
payload.addError(e.getMessage());
}
} else {
errs.forEach(payload::addError);
}
```
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
