Details
Improvement
Minor
Description
HttpSolrClient provides a facility to add default parameters for every request via the invariantParams attribute. Currently HttpSolrClient.Builder does not provide any option to configure this attribute. This jira is to add this functionality.
Issue Links
- links to
Activity
- All
- Comments
- Work Log
- History
- Activity
- Transitions
Mark Miller Ishan Chattopadhyaya Can you please take a look?
The main idea here is to expose invariantParams in a more generic fashion. I also wonder if this can be expanded so that we don't require custom logic in following places,
https://github.com/apache/lucene-solr/blob/8c79ab2649437c8c7ca275f6481c058c67626660/solr/solrj/src/java/org/apache/solr/client/solrj/impl/DelegationTokenHttpSolrClient.java#L54
https://github.com/apache/lucene-solr/blob/8c79ab2649437c8c7ca275f6481c058c67626660/solr/solrj/src/java/org/apache/solr/client/solrj/impl/DelegationTokenHttpSolrClient.java#L63
+1, LGTM.
Looks good.
Just a couple of thoughts:
In the HSC,public Builder withInvariantParams(ModifiableSolrParams params) { Objects.requireNonNull(params, "params must be non null!"); for (String name : params.getParameterNames()) { if (this.invariantParams.get(name) != null) { throw new IllegalStateException("parameter " + name + " is redefined."); } } this.invariantParams.add(params); return this; }Shouldn't it be possible for a user to, say, have multiple "fq" parameters? I think trying this here would trigger the "parameter fq is redefined." exception.Edit: sorry, I misread the code and it seems this is not an issue.- Wondering if we can add a test that uses the withInvariantParams method?
Wondering if we can add a test that uses the withInvariantParams method?
Since I refactored delegation token related stuff to use this mechanism, I was hoping to reuse existing tests. Would this be sufficient or should I add an explicit test for this?
I think something like this would just suffice.
diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderTest.java index a42e820..9b97378 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientBuilderTest.java @@ -22,6 +22,7 @@ import java.io.IOException; import org.apache.http.client.HttpClient; import org.apache.http.impl.client.HttpClientBuilder; import org.apache.lucene.util.LuceneTestCase; +import org.apache.solr.SolrTestCaseJ4; import org.apache.solr.client.solrj.ResponseParser; import org.apache.solr.client.solrj.impl.HttpSolrClient.Builder; import org.junit.Test; @@ -73,4 +74,38 @@ public class HttpSolrClientBuilderTest extends LuceneTestCase { assertTrue(usedParser instanceof BinaryResponseParser); } } + + @Test + public void testInvariantParams() throws IOException { + try(HttpSolrClient createdClient = new Builder(ANY_BASE_SOLR_URL) + .withHttpClient(ANY_HTTP_CLIENT) + .withInvariantParams(SolrTestCaseJ4.params("param", "value")) + .build()) { + assertTrue(createdClient.getHttpClient().equals(ANY_HTTP_CLIENT)); + assertEquals("value", createdClient.getInvariantParams().get("param")); + } + + try(HttpSolrClient createdClient = new Builder(ANY_BASE_SOLR_URL) + .withHttpClient(ANY_HTTP_CLIENT) + .withInvariantParams(SolrTestCaseJ4.params("fq", "fq1", "fq", "fq2")) + .build()) { + assertTrue(createdClient.getHttpClient().equals(ANY_HTTP_CLIENT)); + assertEquals(2, createdClient.getInvariantParams().getParams("fq").length); + } + + try(HttpSolrClient createdClient = new Builder(ANY_BASE_SOLR_URL) + .withHttpClient(ANY_HTTP_CLIENT) + .withDelegationToken("mydt") + .withInvariantParams(SolrTestCaseJ4.params(DelegationTokenHttpSolrClient.DELEGATION_TOKEN_PARAM, "mydt")) + .build()) { + fail(); + } catch(Exception ex) { + if (ex.getMessage().equals("parameter "+ DelegationTokenHttpSolrClient.DELEGATION_TOKEN_PARAM +" is redefined.")) { + // we're good + } else { + throw ex; + } + } + } + }
Ishan Chattopadhyaya I have updated the PR. Can you please take a look?
Adding the patch here for reference. Planning to commit this shortly.
Commit a3521174307ec716361260a77c928cc7264e8b59 in lucene-solr's branch refs/heads/branch_6x from Ishan Chattopadhyaya
[ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a352117 ]
SOLR-9860: Enable configuring invariantParams via HttpSolrClient.Builder
Commit 2c4c5040eecdf7ab63d81e6e5a2e519891684d59 in lucene-solr's branch refs/heads/master from Ishan Chattopadhyaya
[ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2c4c504 ]
SOLR-9860: Enable configuring invariantParams via HttpSolrClient.Builder
Thanks Hrishikesh Gadre.
Github user hgadre closed the pull request at:
GitHub user hgadre opened a pull request:
https://github.com/apache/lucene-solr/pull/123
SOLR-9860Enable configuring invariantParams via HttpSolrClient.BuilderYou can merge this pull request into a Git repository by running:
$ git pull https://github.com/hgadre/lucene-solr
SOLR-9860_fixAlternatively you can review and apply these changes as the patch at:
https://github.com/apache/lucene-solr/pull/123.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #123
commit 182bd4f3eb409495dda097cc2f3d5e2420c683f5
Author: Hrishikesh Gadre <hgadre@cloudera.com>
Date: 2016-12-13T19:35:20Z
SOLR-9860Enable configuring invariantParams via HttpSolrClient.Builder