SOLR-17119: Exception swallowing in ContainerPluginsApi causes some config validation errors to be ignored

[ycallea]

https://issues.apache.org/jira/browse/SOLR-17119

Description

When registering or updating a ConfigurablePlugin through the /cluster/plugin API, exceptions raised within the void configure(T cfg) method are swallowed in ContainerPluginsApi.

This behavior can be troublesome as the exceptions raised in the configure(...) method are usually validation errors on the configuration to be applied, which are therefore ignored and allow for invalid plugin configuration to become effective.

Solution

A small change has been implemented in ContainerPluginsApi to properly include the exceptions raised while configuring a configurable cluster plugin in the response's payload errors.

A minor code clean-up on the modified method have also been included to this pull request.

Tests

• A new test has been written to illustrate the priorly failing scenario and validate the changes.
• Existing tests continue to pass.

Checklist

Please review the following and check all that apply:

• I have reviewed the guidelines for How to Contribute 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 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

[pvcnt]

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.

[ycallea]

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 :

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);
}

[ycallea]

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), it would simplify the code and ease errors management when constructing ApiInfo objects.

[pvcnt]

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!

[pvcnt]

Can this be merged into ConfigurablePluginWithValidation#configure? Again, just to make the code easier to follow by avoiding one jump.

[ycallea]

Sure, it can definitely be implemented this way :

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 ?

[pvcnt]

Yes. Very minor comment, I just thought it would slightly improve the readability of the test. ;)

[ycallea]

Thanks, I updated the code accordingly in 729b14b

[pvcnt]

Is this method (missing in the modified version) what was causing the exception to be swallowed?

[ycallea]

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.

[dsmiley]

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.

[bruno-roustant]

Thanks @ycallea

[ycallea]

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 ad6c595, 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 🙂

[dsmiley]

Please move to 9.6

[dsmiley]

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?

[ycallea]

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 🙂