[GitHub] [lucene-solr] anshumg commented on a change in pull request #2445: SOLR-15154: Let Http2SolrClient pass Basic Auth credentials to all requests

[GitBox <gi...@apache.org>]

anshumg commented on a change in pull request #2445:
URL: https://github.com/apache/lucene-solr/pull/2445#discussion_r586731718



##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java
##########
@@ -875,6 +917,17 @@ public Builder withSSLConfig(SSLConfig sslConfig) {
       return this;
     }
 
+    public Builder withBasicAuthCredentials(String user, String pass) {
+      if (user != null || pass != null) {
+        if (user == null || pass == null) {
+          throw new IllegalStateException("Invalid Authentication credentials. Either both or none must be provided");

Review comment:
       both -> both user(name) and password ?

##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java
##########
@@ -854,7 +871,32 @@ public Builder(String baseSolrUrl) {
     }
 
     public Http2SolrClient build() {
-      return new Http2SolrClient(baseSolrUrl, this);
+      Http2SolrClient client = new Http2SolrClient(baseSolrUrl, this);
+      try {
+        httpClientBuilderSetup(client);
+      } catch (RuntimeException e) {
+        try {
+          client.close();
+        } catch (Exception exceptionOnClose) {
+          e.addSuppressed(exceptionOnClose);
+        }
+        throw e;
+      }
+      return client;
+    }
+
+    private void httpClientBuilderSetup(Http2SolrClient client) {
+      String factoryClassName = System.getProperty(HttpClientUtil.SYS_PROP_HTTP_CLIENT_BUILDER_FACTORY);
+      if (factoryClassName != null) {
+        log.debug ("Using Builder Factory: {}", factoryClassName);

Review comment:
       Using HTTP Client Builder Factory?

##########
File path: solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java
##########
@@ -616,6 +645,113 @@ public void testGetDefaultSslContextFactory() {
     System.clearProperty("javax.net.ssl.trustStoreType");
   }
 
+  protected void expectThrowsAndMessage(Class<? extends Exception> expectedType, ThrowingRunnable executable, String expectedMessage) {
+    Exception e = expectThrows(expectedType, executable);
+    assertTrue("Expecting message to contain \"" + expectedMessage + "\" but was: " + e.getMessage(), e.getMessage().contains(expectedMessage));
+  }
+
+  @Test
+  public void testBadExplicitCredentials() {
+    expectThrowsAndMessage(IllegalStateException.class, () -> new Http2SolrClient.Builder()
+            .withBasicAuthCredentials("foo", null), "Invalid Authentication credentials");
+    expectThrowsAndMessage(IllegalStateException.class, () -> new Http2SolrClient.Builder()
+            .withBasicAuthCredentials(null, "foo"), "Invalid Authentication credentials");
+  }
+
+  @Test
+  public void testSetCredentialsExplicitly() {
+    try (Http2SolrClient client = new Http2SolrClient.Builder(jetty.getBaseUrl().toString() + "/debug/foo")
+            .withBasicAuthCredentials("foo", "explicit")
+            .build();) {
+      QueryRequest r = new QueryRequest(new SolrQuery("quick brown fox"));
+      try {
+        ignoreException("Error from server");
+        client.request(r);
+      } catch (Exception e) {
+        // expected
+      }
+      unIgnoreException("Error from server");
+      assertTrue(DebugServlet.headers.size() > 0);
+      String authorizationHeader = DebugServlet.headers.get("authorization");
+      assertNotNull("Expecting authorization header but got: " + DebugServlet.headers, authorizationHeader);

Review comment:
       Does it make sense to reframe the test failure message as `No authorization information in the header found. Header : `
   Feel free to ignore it, but as you were printing the entire header, I thought specifying that made it more obvious.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org