Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-9860

Enable configuring invariantParams via HttpSolrClient.Builder

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 6.3
    • Fix Version/s: 6.4, 7.0
    • Component/s: None
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      None

      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.

      1. SOLR-9860.patch
        11 kB
        Ishan Chattopadhyaya

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user hgadre opened a pull request:

          https://github.com/apache/lucene-solr/pull/123

          SOLR-9860 Enable configuring invariantParams via HttpSolrClient.Builder

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/hgadre/lucene-solr SOLR-9860_fix

          Alternatively 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-9860 Enable configuring invariantParams via HttpSolrClient.Builder


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user hgadre opened a pull request: https://github.com/apache/lucene-solr/pull/123 SOLR-9860 Enable configuring invariantParams via HttpSolrClient.Builder You can merge this pull request into a Git repository by running: $ git pull https://github.com/hgadre/lucene-solr SOLR-9860 _fix Alternatively 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-9860 Enable configuring invariantParams via HttpSolrClient.Builder
          Show
          hgadre Hrishikesh Gadre added a comment - 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
          Hide
          markrmiller@gmail.com Mark Miller added a comment -

          +1, LGTM.

          Show
          markrmiller@gmail.com Mark Miller added a comment - +1, LGTM.
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment - - edited

          Looks good.
          Just a couple of thoughts:

          1. 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.

          2. Wondering if we can add a test that uses the withInvariantParams method?
          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - - edited 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?
          Hide
          hgadre Hrishikesh Gadre added a comment -

          Ishan Chattopadhyaya

          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?

          Show
          hgadre Hrishikesh Gadre added a comment - Ishan Chattopadhyaya 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?
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -

          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;
          +      }
          +    }
          +  }
          +
           }
          
          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - 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; + } + } + } + }
          Hide
          hgadre Hrishikesh Gadre added a comment -

          Ishan Chattopadhyaya I have updated the PR. Can you please take a look?

          Show
          hgadre Hrishikesh Gadre added a comment - Ishan Chattopadhyaya I have updated the PR. Can you please take a look?
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -

          Adding the patch here for reference. Planning to commit this shortly.

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - Adding the patch here for reference. Planning to commit this shortly.
          Hide
          jira-bot ASF subversion and git services added a comment -

          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

          Show
          jira-bot ASF subversion and git services added a comment - 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
          Hide
          jira-bot ASF subversion and git services added a comment -

          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

          Show
          jira-bot ASF subversion and git services added a comment - 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
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -
          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - Thanks Hrishikesh Gadre .
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user hgadre closed the pull request at:

          https://github.com/apache/lucene-solr/pull/123

          Show
          githubbot ASF GitHub Bot added a comment - Github user hgadre closed the pull request at: https://github.com/apache/lucene-solr/pull/123

            People

            • Assignee:
              ichattopadhyaya Ishan Chattopadhyaya
              Reporter:
              hgadre Hrishikesh Gadre
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development