SOLR-17256: Introduce SolrRequest.setBasePath alternative (#2714)

Mutating SolrRequest objects as setBasePath does is a bad practice, as
the modification isn't documented and many users expect to reuse these
request objects across multiple requests.

This commit adds an alternative approach which allows both clients and
SolrRequest instances to remain immutable: a
`Http2SolrClient.requestWithBaseUrl` method.

setBasePath usages (and the method itself) will be removed in a
subsequent commit.

---------

Co-authored-by: David Smiley <[email protected]>

solr/CHANGES.txt

@@ -133,6 +133,10 @@ Improvements
 
 * SOLR-17442: Resolve -v flag conflict (version, value, verbose) in bin/solr. (Eric Pugh, Christos Malliaridis)
 
+* SOLR-17256: Deprecate SolrRequest `setBasePath` and `getBasePath` methods.  SolrJ users wishing to temporarily
+  override an HTTP client's base URL may use `Http2SolrClient.requestWithBaseUrl` instead. (Jason Gerlowski,
+  Sanjay Dutt, David Smiley)
+
 Optimizations
 ---------------------
 * SOLR-14985: Solrj CloudSolrClient with Solr URLs had serious performance regressions (since the

solr/core/src/java/org/apache/solr/handler/IndexFetcher.java

@@ -94,7 +94,6 @@
 import org.apache.lucene.store.IOContext;
 import org.apache.lucene.store.IndexInput;
 import org.apache.lucene.store.IndexOutput;
-import org.apache.solr.client.solrj.SolrClient;
 import org.apache.solr.client.solrj.SolrServerException;
 import org.apache.solr.client.solrj.impl.Http2SolrClient;
 import org.apache.solr.client.solrj.impl.HttpClientUtil;
@@ -185,7 +184,7 @@ public class IndexFetcher {
 
   boolean fetchFromLeader = false;
 
-  private final SolrClient solrClient;
+  private final Http2SolrClient solrClient;
 
   private Integer connTimeout;
 
@@ -263,17 +262,15 @@ public String getMessage() {
   // It's crucial not to remove the authentication credentials as they are essential for User
   // managed replication.
   // GitHub PR #2276
-  private SolrClient createSolrClient(
+  private Http2SolrClient createSolrClient(
       SolrCore core, String httpBasicAuthUser, String httpBasicAuthPassword, String leaderBaseUrl) {
     final UpdateShardHandler updateShardHandler = core.getCoreContainer().getUpdateShardHandler();
-    Http2SolrClient httpClient =
-        new Http2SolrClient.Builder(leaderBaseUrl)
-            .withHttpClient(updateShardHandler.getRecoveryOnlyHttpClient())
-            .withBasicAuthCredentials(httpBasicAuthUser, httpBasicAuthPassword)
-            .withIdleTimeout(soTimeout, TimeUnit.MILLISECONDS)
-            .withConnectionTimeout(connTimeout, TimeUnit.MILLISECONDS)
-            .build();
-    return httpClient;
+    return new Http2SolrClient.Builder(leaderBaseUrl)
+        .withHttpClient(updateShardHandler.getRecoveryOnlyHttpClient())
+        .withBasicAuthCredentials(httpBasicAuthUser, httpBasicAuthPassword)
+        .withIdleTimeout(soTimeout, TimeUnit.MILLISECONDS)
+        .withConnectionTimeout(connTimeout, TimeUnit.MILLISECONDS)
+        .build();
   }
 
   public IndexFetcher(
@@ -376,10 +373,8 @@ public NamedList<Object> getLatestVersion() throws IOException {
     params.set(CommonParams.WT, JAVABIN);
     params.set(CommonParams.QT, ReplicationHandler.PATH);
     QueryRequest req = new QueryRequest(params);
-    req.setBasePath(leaderBaseUrl);
-    // TODO modify to use shardhandler
     try {
-      return solrClient.request(req, leaderCoreName);
+      return solrClient.requestWithBaseUrl(leaderBaseUrl, leaderCoreName, req).getResponse();
     } catch (SolrServerException e) {
       throw new SolrException(ErrorCode.SERVER_ERROR, e.getMessage(), e);
     }
@@ -397,10 +392,9 @@ private void fetchFileList(long gen) throws IOException {
     params.set(CommonParams.WT, JAVABIN);
     params.set(CommonParams.QT, ReplicationHandler.PATH);
     QueryRequest req = new QueryRequest(params);
-    req.setBasePath(leaderBaseUrl);
-    // TODO modify to use shardhandler
     try {
-      NamedList<?> response = solrClient.request(req, leaderCoreName);
+      NamedList<?> response =
+          solrClient.requestWithBaseUrl(leaderBaseUrl, leaderCoreName, req).getResponse();
 
       List<Map<String, Object>> files = (List<Map<String, Object>>) response.get(CMD_GET_FILE_LIST);
       if (files != null) filesToDownload = Collections.synchronizedList(files);
@@ -2132,9 +2126,8 @@ NamedList<Object> getDetails() throws IOException, SolrServerException {
     params.set(CommonParams.QT, ReplicationHandler.PATH);
 
     QueryRequest request = new QueryRequest(params);
-    request.setBasePath(leaderBaseUrl);
     // TODO use shardhandler
-    return solrClient.request(request, leaderCoreName);
+    return solrClient.requestWithBaseUrl(leaderBaseUrl, leaderCoreName, request).getResponse();
   }
 
   public void destroy() {

solr/solrj/src/java/org/apache/solr/client/solrj/SolrClientFunction.java

@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.client.solrj;
+
+import java.io.IOException;
+
+/** A lambda intended for invoking SolrClient operations */
+@FunctionalInterface
+public interface SolrClientFunction<C extends SolrClient, R> {
+  R apply(C c) throws IOException, SolrServerException;
+}

solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java

@@ -41,7 +41,9 @@
 import java.util.stream.Collectors;
 import org.apache.solr.client.solrj.ResponseParser;
 import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.SolrClientFunction;
 import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.solr.client.solrj.SolrResponse;
 import org.apache.solr.client.solrj.SolrServerException;
 import org.apache.solr.client.solrj.embedded.SSLConfig;
 import org.apache.solr.client.solrj.impl.HttpListenerFactory.RequestResponseListener;
@@ -113,7 +115,7 @@ public class Http2SolrClient extends HttpSolrClientBase {
   private final HttpClient httpClient;
 
   private List<HttpListenerFactory> listenerFactory = new ArrayList<>();
-  private final AsyncTracker asyncTracker = new AsyncTracker();
+  protected AsyncTracker asyncTracker = new AsyncTracker();
 
   private final boolean closeClient;
   private ExecutorService executor;
@@ -536,6 +538,41 @@ public NamedList<Object> request(SolrRequest<?> solrRequest, String collection)
     }
   }
 
+  /**
+   * Executes a SolrRequest using the provided URL to temporarily override any "base URL" currently
+   * used by this client
+   *
+   * @param baseUrl a URL to a root Solr path (i.e. "/solr") that should be used for this request
+   * @param collection an optional collection or core name used to override the client's "default
+   *     collection". May be 'null' for any requests that don't require a collection or wish to rely
+   *     on the client's default
+   * @param req the SolrRequest to send
+   */
+  public final <R extends SolrResponse> R requestWithBaseUrl(
+      String baseUrl, String collection, SolrRequest<R> req)
+      throws SolrServerException, IOException {
+    return requestWithBaseUrl(baseUrl, (c) -> req.process(c, collection));
+  }
+
+  /**
+   * Temporarily modifies the client to use a different base URL and runs the provided lambda
+   *
+   * @param baseUrl the base URL to use on any requests made within the 'clientFunction' lambda
+   * @param clientFunction a Function that consumes a Http2SolrClient and returns an arbitrary value
+   * @return the value returned after invoking 'clientFunction'
+   * @param <R> the type returned by the provided function (and by this method)
+   */
+  public <R> R requestWithBaseUrl(
+      String baseUrl, SolrClientFunction<Http2SolrClient, R> clientFunction)
+      throws SolrServerException, IOException {
+
+    // Despite the name, try-with-resources used here to avoid IDE and ObjectReleaseTracker
+    // complaints
+    try (final var derivedClient = new NoCloseHttp2SolrClient(baseUrl, this)) {
+      return clientFunction.apply(derivedClient);
+    }
+  }
+
   private NamedList<Object> processErrorsAndResponse(
       SolrRequest<?> solrRequest, Response response, InputStream is, String urlExceptionMessage)
       throws SolrServerException {
@@ -788,6 +825,29 @@ protected RequestWriter getRequestWriter() {
     return requestWriter;
   }
 
+  /**
+   * An Http2SolrClient that doesn't close or cleanup any resources
+   *
+   * <p>Only safe to use as a derived copy of an existing instance which retains responsibility for
+   * closing all involved resources.
+   *
+   * @see #requestWithBaseUrl(String, SolrClientFunction)
+   */
+  private static class NoCloseHttp2SolrClient extends Http2SolrClient {
+
+    public NoCloseHttp2SolrClient(String baseUrl, Http2SolrClient parentClient) {
+      super(baseUrl, new Http2SolrClient.Builder(baseUrl).withHttpClient(parentClient));
+
+      this.asyncTracker = parentClient.asyncTracker;
+    }
+
+    @Override
+    public void close() {
+      /* Intentional no-op */
+      ObjectReleaseTracker.release(this);
+    }
+  }
+
   private static class AsyncTracker {
     private static final int MAX_OUTSTANDING_REQUESTS = 1000;
 

solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java

@@ -81,6 +81,8 @@ public static String buildRequestUrl(
       String serverRootUrl,
       String collection)
       throws MalformedURLException {
+
+    // TODO remove getBasePath support here prior to closing SOLR-17256
     String basePath = solrRequest.getBasePath() == null ? serverRootUrl : solrRequest.getBasePath();
 
     if (solrRequest.getApiVersion() == SolrRequest.ApiVersion.V2) {

solr/solrj/src/test/org/apache/solr/client/solrj/impl/DebugServlet.java

@@ -34,6 +34,7 @@
 public class DebugServlet extends HttpServlet {
   public static void clear() {
     lastMethod = null;
+    url = null;
     headers = null;
     parameters = null;
     errorCode = null;
@@ -45,6 +46,7 @@ public static void clear() {
 
   public static Integer errorCode = null;
   public static String lastMethod = null;
+  public static String url = null;
   public static HashMap<String, String> headers = null;
   public static Map<String, String[]> parameters = null;
   public static String queryString = null;
@@ -123,6 +125,7 @@ protected void doPut(HttpServletRequest req, HttpServletResponse resp)
   }
 
   private void recordRequest(HttpServletRequest req, HttpServletResponse resp) {
+    url = req.getRequestURL().toString();
     setHeaders(req);
     setParameters(req);
     setQueryString(req);

solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java

@@ -32,6 +32,7 @@
 import org.apache.solr.client.solrj.request.RequestWriter;
 import org.apache.solr.common.params.CommonParams;
 import org.apache.solr.common.params.MapSolrParams;
+import org.apache.solr.common.params.ModifiableSolrParams;
 import org.eclipse.jetty.client.WWWAuthenticationProtocolHandler;
 import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.util.ssl.SslContextFactory;
@@ -181,6 +182,38 @@ public void testQueryXmlPut() throws Exception {
     super.testQueryXmlPut();
   }
 
+  @Test
+  public void testOverrideBaseUrl() throws Exception {
+    DebugServlet.clear();
+    final var defaultUrl =
+        "http://not-a-real-url:8983/solr"; // Would result in an exception if used
+    final var urlToUse = getBaseUrl() + DEBUG_SERVLET_PATH;
+    final var queryParams = new ModifiableSolrParams();
+    queryParams.add("q", "*:*");
+
+    // Ensure the correct URL is used by the lambda-based requestWithBaseUrl method
+    try (Http2SolrClient client =
+        new Http2SolrClient.Builder(defaultUrl).withDefaultCollection(DEFAULT_CORE).build()) {
+      try {
+        client.requestWithBaseUrl(urlToUse, (c) -> c.query(queryParams));
+      } catch (BaseHttpSolrClient.RemoteSolrException rse) {
+      }
+
+      assertEquals(urlToUse + "/select", DebugServlet.url);
+    }
+
+    // Ensure the correct URL is used by the SolrRequest-based requestWithBaseUrl method
+    try (Http2SolrClient client =
+        new Http2SolrClient.Builder(defaultUrl).withDefaultCollection(DEFAULT_CORE).build()) {
+      try {
+        client.requestWithBaseUrl(urlToUse, null, new QueryRequest(queryParams));
+      } catch (BaseHttpSolrClient.RemoteSolrException rse) {
+      }
+
+      assertEquals(urlToUse + "/select", DebugServlet.url);
+    }
+  }
+
   @Test
   public void testDelete() throws Exception {
     DebugServlet.clear();