You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "ycallea (via GitHub)" <gi...@apache.org> on 2024/01/17 14:38:55 UTC
[PR] Exception swallowing in ContainerPluginsApi causes some config validation errors to be ignored [solr]
ycallea opened a new pull request, #2202:
URL: https://github.com/apache/solr/pull/2202
https://issues.apache.org/jira/browse/SOLR-XXXXX
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Solr:
* https://issues.apache.org/jira/projects/SOLR
For something minor (i.e. that wouldn't be worth putting in release notes), you can skip JIRA.
To create a Jira issue, you will need to create an account there first.
The title of the PR should reference the Jira issue number in the form:
* SOLR-####: <short description of problem or changes>
SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# Description
Please provide a short description of the changes you're making with this pull request.
# Solution
Please provide a short description of the approach taken to implement your solution.
# Tests
Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.
# Checklist
Please review the following and check all that apply:
- [ ] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17119: Exception swallowing in ContainerPluginsApi causes some config validation errors to be ignored [solr]
Posted by "pvcnt (via GitHub)" <gi...@apache.org>.
pvcnt commented on code in PR #2202:
URL: https://github.com/apache/solr/pull/2202#discussion_r1458657389
##########
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:
Oh, I didn't get it that only appending to `errs` had no effect (which is what you are fixing). I take back my comment then, it's fine as it is!
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17119: Exception swallowing in ContainerPluginsApi causes some config validation errors to be ignored [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2202:
URL: https://github.com/apache/solr/pull/2202#issuecomment-1900824614
Can a CHANGES.txt entry be proposed here? I suppose this is an "Improvement", though I can sympathize why you filed the JIRA issue as a bug.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17119: Exception swallowing in ContainerPluginsApi causes some config validation errors to be ignored [solr]
Posted by "ycallea (via GitHub)" <gi...@apache.org>.
ycallea commented on code in PR #2202:
URL: https://github.com/apache/solr/pull/2202#discussion_r1458725903
##########
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:
Thanks, I updated the code accordingly in https://github.com/apache/solr/pull/2202/commits/729b14b191f79fc99759f94cdd3a45a09e7ffa2c
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17119: Exception swallowing in ContainerPluginsApi causes some config validation errors to be ignored [solr]
Posted by "ycallea (via GitHub)" <gi...@apache.org>.
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 151) 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 146), 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.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17119: Exception swallowing in ContainerPluginsApi causes some config validation errors to be ignored [solr]
Posted by "ycallea (via GitHub)" <gi...@apache.org>.
ycallea commented on code in PR #2202:
URL: https://github.com/apache/solr/pull/2202#discussion_r1464959226
##########
solr/CHANGES.txt:
##########
@@ -140,6 +140,9 @@ Improvements
* SOLR-17096: solr.xml now supports declaring clusterSingleton plugins (Paul McArthur, David Smiley)
+* SOLR-17119: When registering a ConfigurablePlugin through the `/cluster/plugin` API using the
+ V2Request client, config validation exceptions are now propagated to the callers (Yohann Callea)
Review Comment:
It would indeed also affect users querying the API in plain HTTP without SolrJ.
I agree that the proposed description seems to imply that the issue only exist when calling the API through SolrJ, I updated it to be less specific.
I also moved the change to 9.6.0 as requested in the previous comment.
Thanks again for your guidance 🙂
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17119: Exception swallowing in ContainerPluginsApi causes some config validation errors to be ignored [solr]
Posted by "ycallea (via GitHub)" <gi...@apache.org>.
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17119: Exception swallowing in ContainerPluginsApi causes some config validation errors to be ignored [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2202:
URL: https://github.com/apache/solr/pull/2202#discussion_r1463627890
##########
solr/CHANGES.txt:
##########
@@ -140,6 +140,9 @@ Improvements
* SOLR-17096: solr.xml now supports declaring clusterSingleton plugins (Paul McArthur, David Smiley)
+* SOLR-17119: When registering a ConfigurablePlugin through the `/cluster/plugin` API using the
Review Comment:
Please move to 9.6
##########
solr/CHANGES.txt:
##########
@@ -140,6 +140,9 @@ Improvements
* SOLR-17096: solr.xml now supports declaring clusterSingleton plugins (Paul McArthur, David Smiley)
+* SOLR-17119: When registering a ConfigurablePlugin through the `/cluster/plugin` API using the
+ V2Request client, config validation exceptions are now propagated to the callers (Yohann Callea)
Review Comment:
Suggest rewording "the V2Request client" to "SolrJ with V2Request". (people know SolrJ is a client and this word sets better context to users who have no clue what V2Request even is).
Curious; is"V2Request" actually pertinent to the bug? Would someone using plain HTTP see the problem?
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17119: Exception swallowing in ContainerPluginsApi causes some config validation errors to be ignored [solr]
Posted by "pvcnt (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17119: Exception swallowing in ContainerPluginsApi causes some config validation errors to be ignored [solr]
Posted by "pvcnt (via GitHub)" <gi...@apache.org>.
pvcnt commented on code in PR #2202:
URL: https://github.com/apache/solr/pull/2202#discussion_r1458658401
##########
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:
Yes. Very minor comment, I just thought it would slightly improve the readability of the test. ;)
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17119: Exception swallowing in ContainerPluginsApi causes some config validation errors to be ignored [solr]
Posted by "ycallea (via GitHub)" <gi...@apache.org>.
ycallea commented on PR #2202:
URL: https://github.com/apache/solr/pull/2202#issuecomment-1906160411
> Can a CHANGES.txt entry be proposed here? I suppose this is an "Improvement", though I can sympathize why you filed the JIRA issue as a bug.
I agree classifying it as a bug could be debatable.
I don't have much history on the initial implementation so I can't tell if this behavior was intended or not, thus I'm fine classifying it as an improvement if it makes more sense.
I added a `CHANGES.txt` entry proposal in https://github.com/apache/solr/pull/2202/commits/ad6c5958326971b2e77d1ff0fa3211ca448c3998, for now it has been added as an improvement for the 9.5.0 release.
Let me know if you need me to make some changes to the proposed description, category and/or release 🙂
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17119: Exception swallowing in ContainerPluginsApi causes some config validation errors to be ignored [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley merged PR #2202:
URL: https://github.com/apache/solr/pull/2202
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org