From 437e27970b2ab7bbe2680e3c37d78ad410209a63 Mon Sep 17 00:00:00 2001 From: Jason Gerlowski Date: Wed, 29 May 2024 08:10:00 -0400 Subject: [PATCH] SOLR-17044: Consolidate SolrJ URL-building logic (#2455) Prior to this commit, SolrJ had near-duplicate logic for building request paths in two different places: HttpSolrClient and HttpSolrClientBase (used by the JDK and Jetty HSC's). This commit consolidates the path-building logic for all of SolrJ, so that it can live in only one place. --- .../handler/admin/HealthCheckHandlerTest.java | 28 +++++----- .../client/solrj/impl/Http2SolrClient.java | 4 +- .../client/solrj/impl/HttpJdkSolrClient.java | 2 +- .../client/solrj/impl/HttpSolrClient.java | 37 +++--------- .../client/solrj/impl/HttpSolrClientBase.java | 29 ++-------- .../solr/client/solrj/impl/LBSolrClient.java | 4 +- .../solr/client/solrj/util/ClientUtils.java | 52 +++++++++++++++++ .../client/solrj/util/ClientUtilsTest.java | 56 +++++++++++++++++++ 8 files changed, 140 insertions(+), 72 deletions(-) diff --git a/solr/core/src/test/org/apache/solr/handler/admin/HealthCheckHandlerTest.java b/solr/core/src/test/org/apache/solr/handler/admin/HealthCheckHandlerTest.java index f56359f95bd..67c0e445f1b 100644 --- a/solr/core/src/test/org/apache/solr/handler/admin/HealthCheckHandlerTest.java +++ b/solr/core/src/test/org/apache/solr/handler/admin/HealthCheckHandlerTest.java @@ -25,7 +25,6 @@ import java.util.Properties; import org.apache.solr.client.solrj.SolrClient; 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.impl.BaseHttpSolrClient; import org.apache.solr.client.solrj.request.CollectionAdminRequest; @@ -54,24 +53,27 @@ public static void setupCluster() throws Exception { configureCluster(1).addConfig("conf", configset("cloud-minimal")).configure(); } + private HealthCheckResponse runHealthcheckWithClient(SolrClient client) throws Exception { + return new HealthCheckRequest().process(client); + } + @Test public void testHealthCheckHandler() throws Exception { - GenericSolrRequest req = - new GenericSolrRequest(SolrRequest.METHOD.GET, HEALTH_CHECK_HANDLER_PATH); // positive check that our only existing "healthy" node works with cloud client // NOTE: this is using GenericSolrRequest, not HealthCheckRequest which is why it passes // as compared with testHealthCheckHandlerWithCloudClient // (Not sure if that's actually a good thing -- but it's how the existing test worked) + final var genericHealthcheck = + new GenericSolrRequest(SolrRequest.METHOD.GET, HEALTH_CHECK_HANDLER_PATH); assertEquals( CommonParams.OK, - req.process(cluster.getSolrClient()).getResponse().get(CommonParams.STATUS)); + genericHealthcheck.process(cluster.getSolrClient()).getResponse().get(CommonParams.STATUS)); // positive check that our exiting "healthy" node works with direct http client try (SolrClient solrClient = getHttpSolrClient(cluster.getJettySolrRunner(0).getBaseUrl().toString())) { - SolrResponse response = req.process(solrClient); - assertEquals(CommonParams.OK, response.getResponse().get(CommonParams.STATUS)); + assertEquals(CommonParams.OK, runHealthcheckWithClient(solrClient).getNodeStatus()); } // successfully create a dummy collection @@ -82,8 +84,7 @@ public void testHealthCheckHandler() throws Exception { .withProperty("solr.directoryFactory", "solr.StandardDirectoryFactory") .process(solrClient); assertEquals(0, collectionAdminResponse.getStatus()); - SolrResponse response = req.process(solrClient); - assertEquals(CommonParams.OK, response.getResponse().get(CommonParams.STATUS)); + assertEquals(CommonParams.OK, runHealthcheckWithClient(solrClient).getNodeStatus()); } finally { cluster.deleteAllCollections(); cluster.deleteAllConfigSets(); @@ -94,7 +95,8 @@ public void testHealthCheckHandler() throws Exception { try (SolrClient solrClient = getHttpSolrClient(newJetty.getBaseUrl().toString())) { // positive check that our (new) "healthy" node works with direct http client - assertEquals(CommonParams.OK, req.process(solrClient).getResponse().get(CommonParams.STATUS)); + final var response = runHealthcheckWithClient(solrClient); + assertEquals(CommonParams.OK, response.getNodeStatus()); // now "break" our (new) node newJetty.getCoreContainer().getZkController().getZkClient().close(); @@ -104,7 +106,7 @@ public void testHealthCheckHandler() throws Exception { expectThrows( BaseHttpSolrClient.RemoteSolrException.class, () -> { - req.process(solrClient); + runHealthcheckWithClient(solrClient); }); assertTrue(e.getMessage(), e.getMessage().contains("Host Unavailable")); assertEquals(SolrException.ErrorCode.SERVICE_UNAVAILABLE.code, e.code()); @@ -118,7 +120,7 @@ public void testHealthCheckHandler() throws Exception { try (SolrClient solrClient = getHttpSolrClient(newJetty.getBaseUrl().toString())) { // positive check that our (new) "healthy" node works with direct http client - assertEquals(CommonParams.OK, req.process(solrClient).getResponse().get(CommonParams.STATUS)); + assertEquals(CommonParams.OK, runHealthcheckWithClient(solrClient).getNodeStatus()); // shutdown the core container of new node newJetty.getCoreContainer().shutdown(); @@ -128,7 +130,7 @@ public void testHealthCheckHandler() throws Exception { expectThrows( SolrException.class, () -> { - req.process(solrClient).getResponse().get(CommonParams.STATUS); + runHealthcheckWithClient(solrClient).getNodeStatus(); fail("API shouldn't be available, and fail at above request"); }); assertEquals("Exception code should be 404", 404, thrown.code()); @@ -147,7 +149,7 @@ public void testHealthCheckHandler() throws Exception { try (SolrClient solrClient = getHttpSolrClient(cluster.getJettySolrRunner(0).getBaseUrl().toString())) { - assertEquals(CommonParams.OK, req.process(solrClient).getResponse().get(CommonParams.STATUS)); + assertEquals(CommonParams.OK, runHealthcheckWithClient(solrClient).getNodeStatus()); } } diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java index 74fa2b2aafd..0164c42e63c 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java @@ -426,7 +426,7 @@ public CompletableFuture> requestAsync( final MakeRequestReturnValue mrrv; final String url; try { - url = getRequestPath(solrRequest, collection); + url = getRequestUrl(solrRequest, collection); mrrv = makeRequest(solrRequest, url, true); } catch (SolrServerException | IOException e) { future.completeExceptionally(e); @@ -493,7 +493,7 @@ public NamedList request(SolrRequest solrRequest, String collection) if (ClientUtils.shouldApplyDefaultCollection(collection, solrRequest)) { collection = defaultCollection; } - String url = getRequestPath(solrRequest, collection); + String url = getRequestUrl(solrRequest, collection); Throwable abortCause = null; Request req = null; try { diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java index 5a5b72e87c9..32f988ddaa3 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java @@ -198,7 +198,7 @@ private PreparedRequest prepareRequest(SolrRequest solrRequest, String collec if (ClientUtils.shouldApplyDefaultCollection(collection, solrRequest)) { collection = defaultCollection; } - String url = getRequestPath(solrRequest, collection); + String url = getRequestUrl(solrRequest, collection); ResponseParser parserToUse = responseParser(solrRequest); ModifiableSolrParams queryParams = initalizeSolrParams(solrRequest, parserToUse); var reqb = HttpRequest.newBuilder(); diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java index b2a08a7912b..41eb07207a9 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java @@ -25,9 +25,7 @@ import java.io.UnsupportedEncodingException; import java.lang.invoke.MethodHandles; import java.net.ConnectException; -import java.net.MalformedURLException; import java.net.SocketTimeoutException; -import java.net.URL; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.security.Principal; @@ -334,22 +332,15 @@ protected ModifiableSolrParams calculateQueryParams( return queryModParams; } - static String changeV2RequestEndpoint(String basePath) throws MalformedURLException { - URL oldURL = new URL(basePath); - String newPath = oldURL.getPath().replaceFirst("/solr", "/api"); - return new URL(oldURL.getProtocol(), oldURL.getHost(), oldURL.getPort(), newPath).toString(); - } - protected HttpRequestBase createMethod(SolrRequest request, String collection) throws IOException, SolrServerException { SolrParams params = request.getParams(); RequestWriter.ContentWriter contentWriter = requestWriter.getContentWriter(request); Collection streams = contentWriter == null ? requestWriter.getContentStreams(request) : null; - String path = requestWriter.getPath(request); - if (path == null || !path.startsWith("/")) { - path = DEFAULT_PATH; - } + + final String requestUrlBeforeParams = + ClientUtils.buildRequestUrl(request, requestWriter, baseUrl, collection); ResponseParser parser = request.getResponseParser(); if (parser == null) { @@ -369,36 +360,24 @@ protected HttpRequestBase createMethod(SolrRequest request, String collection wparams.add(invariantParams); } - String basePath = baseUrl; - if (collection != null) basePath += "/" + collection; - - if (request instanceof V2Request) { - if (System.getProperty("solr.v2RealPath") == null || ((V2Request) request).isForceV2()) { - basePath = changeV2RequestEndpoint(baseUrl); - } else { - basePath = baseUrl + "/____v2"; - } - } - if (SolrRequest.METHOD.GET == request.getMethod()) { if (streams != null || contentWriter != null) { throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "GET can't send streams!"); } - HttpGet result = new HttpGet(basePath + path + wparams.toQueryString()); + HttpGet result = new HttpGet(requestUrlBeforeParams + wparams.toQueryString()); populateHeaders(result, contextHeaders); return result; } if (SolrRequest.METHOD.DELETE == request.getMethod()) { - return new HttpDelete(basePath + path + wparams.toQueryString()); + return new HttpDelete(requestUrlBeforeParams + wparams.toQueryString()); } if (SolrRequest.METHOD.POST == request.getMethod() || SolrRequest.METHOD.PUT == request.getMethod()) { - String url = basePath + path; boolean hasNullStreamName = false; if (streams != null) { for (ContentStream cs : streams) { @@ -416,7 +395,7 @@ protected HttpRequestBase createMethod(SolrRequest request, String collection List postOrPutParams = new ArrayList<>(); if (contentWriter != null) { - String fullQueryUrl = url + wparams.toQueryString(); + String fullQueryUrl = requestUrlBeforeParams + wparams.toQueryString(); HttpEntityEnclosingRequestBase postOrPut = SolrRequest.METHOD.POST == request.getMethod() ? new HttpPost(fullQueryUrl) @@ -443,7 +422,7 @@ public void writeTo(OutputStream outstream) throws IOException { // send server list and request list as query string params ModifiableSolrParams queryParams = calculateQueryParams(this.urlParamNames, wparams); queryParams.add(calculateQueryParams(request.getQueryParams(), wparams)); - String fullQueryUrl = url + queryParams.toQueryString(); + String fullQueryUrl = requestUrlBeforeParams + queryParams.toQueryString(); HttpEntityEnclosingRequestBase postOrPut = fillContentStream( request, streams, wparams, isMultipart, postOrPutParams, fullQueryUrl); @@ -451,7 +430,7 @@ public void writeTo(OutputStream outstream) throws IOException { } // It is has one stream, it is the post body, put the params in the URL else { - String fullQueryUrl = url + wparams.toQueryString(); + String fullQueryUrl = requestUrlBeforeParams + wparams.toQueryString(); HttpEntityEnclosingRequestBase postOrPut = SolrRequest.METHOD.POST == request.getMethod() ? new HttpPost(fullQueryUrl) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java index 56df7cafe61..6645e4f7cae 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java @@ -23,7 +23,6 @@ import java.io.IOException; import java.io.InputStream; import java.net.MalformedURLException; -import java.net.URL; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.util.Arrays; @@ -42,6 +41,7 @@ import org.apache.solr.client.solrj.SolrServerException; import org.apache.solr.client.solrj.request.RequestWriter; import org.apache.solr.client.solrj.request.V2Request; +import org.apache.solr.client.solrj.util.ClientUtils; import org.apache.solr.common.SolrException; import org.apache.solr.common.params.CommonParams; import org.apache.solr.common.params.ModifiableSolrParams; @@ -51,7 +51,7 @@ public abstract class HttpSolrClientBase extends SolrClient { - protected static final String DEFAULT_PATH = "/select"; + protected static final String DEFAULT_PATH = ClientUtils.DEFAULT_PATH; protected static final Charset FALLBACK_CHARSET = StandardCharsets.UTF_8; private static final List errPath = Arrays.asList("metadata", "error-class"); @@ -111,30 +111,9 @@ protected HttpSolrClientBase(String serverBaseUrl, HttpSolrClientBuilderBase solrRequest, String collection) + protected String getRequestUrl(SolrRequest solrRequest, String collection) throws MalformedURLException { - String basePath = solrRequest.getBasePath() == null ? serverBaseUrl : solrRequest.getBasePath(); - if (collection != null) basePath += "/" + collection; - - if (solrRequest instanceof V2Request) { - if (System.getProperty("solr.v2RealPath") == null) { - basePath = changeV2RequestEndpoint(basePath); - } else { - basePath = serverBaseUrl + "/____v2"; - } - } - String path = requestWriter.getPath(solrRequest); - if (path == null || !path.startsWith("/")) { - path = DEFAULT_PATH; - } - - return basePath + path; - } - - protected String changeV2RequestEndpoint(String basePath) throws MalformedURLException { - URL oldURL = new URL(basePath); - String newPath = oldURL.getPath().replaceFirst("/solr", "/api"); - return new URL(oldURL.getProtocol(), oldURL.getHost(), oldURL.getPort(), newPath).toString(); + return ClientUtils.buildRequestUrl(solrRequest, requestWriter, serverBaseUrl, collection); } protected ResponseParser responseParser(SolrRequest solrRequest) { diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java index 8bb0ccdd7db..1effc3b8a0a 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java @@ -484,8 +484,8 @@ protected Exception doRequest( Exception ex = null; try { rsp.server = baseUrl.toString(); - req.getRequest().setBasePath(baseUrl.toString()); - rsp.rsp = getClient(baseUrl).request(req.getRequest(), (String) null); + req.getRequest().setBasePath(baseUrl.getBaseUrl()); + rsp.rsp = getClient(baseUrl).request(req.getRequest(), baseUrl.getCore()); if (isZombie) { zombieServers.remove(baseUrl.toString()); } diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java b/solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java index be6b02977ea..16f27410400 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java @@ -19,6 +19,8 @@ import java.io.IOException; import java.io.StringWriter; import java.io.Writer; +import java.net.MalformedURLException; +import java.net.URL; import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; import java.util.ArrayList; @@ -27,8 +29,11 @@ import java.util.Date; import java.util.Map; import java.util.Map.Entry; +import org.apache.solr.client.solrj.SolrClient; import org.apache.solr.client.solrj.SolrRequest; import org.apache.solr.client.solrj.SolrResponse; +import org.apache.solr.client.solrj.request.RequestWriter; +import org.apache.solr.client.solrj.request.V2Request; import org.apache.solr.common.SolrInputDocument; import org.apache.solr.common.SolrInputField; import org.apache.solr.common.cloud.Slice; @@ -44,6 +49,8 @@ public class ClientUtils { public static final String TEXT_XML = "application/xml; charset=UTF-8"; public static final String TEXT_JSON = "application/json; charset=UTF-8"; + public static final String DEFAULT_PATH = "/select"; + /** Take a string and make it an iterable ContentStream */ public static Collection toContentStreams( final String str, final String contentType) { @@ -56,6 +63,51 @@ public static Collection toContentStreams( return streams; } + /** + * Create the full URL for a SolrRequest (excepting query parameters) as a String + * + * @param solrRequest the {@link SolrRequest} to build the URL for + * @param requestWriter a {@link RequestWriter} from the {@link SolrClient} that will be sending + * the request + * @param serverRootUrl the root URL of the Solr server being targeted. May by overridden {@link + * SolrRequest#getBasePath()}, if present. + * @param collection the collection to send the request to. May be null if no collection is + * needed. + * @throws MalformedURLException if {@code serverRootUrl} or {@link SolrRequest#getBasePath()} + * contain a malformed URL string + */ + public static String buildRequestUrl( + SolrRequest solrRequest, + RequestWriter requestWriter, + String serverRootUrl, + String collection) + throws MalformedURLException { + String basePath = solrRequest.getBasePath() == null ? serverRootUrl : solrRequest.getBasePath(); + + if (solrRequest instanceof V2Request) { + if (System.getProperty("solr.v2RealPath") == null) { + basePath = changeV2RequestEndpoint(basePath); + } else { + basePath = serverRootUrl + "/____v2"; + } + } + + if (solrRequest.requiresCollection() && collection != null) basePath += "/" + collection; + + String path = requestWriter.getPath(solrRequest); + if (path == null || !path.startsWith("/")) { + path = DEFAULT_PATH; + } + + return basePath + path; + } + + private static String changeV2RequestEndpoint(String basePath) throws MalformedURLException { + URL oldURL = new URL(basePath); + String newPath = oldURL.getPath().replaceFirst("/solr", "/api"); + return new URL(oldURL.getProtocol(), oldURL.getHost(), oldURL.getPort(), newPath).toString(); + } + // ------------------------------------------------------------------------ // ------------------------------------------------------------------------ diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/util/ClientUtilsTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/util/ClientUtilsTest.java index 0d078eeec4b..407fffa2454 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/util/ClientUtilsTest.java +++ b/solr/solrj/src/test/org/apache/solr/client/solrj/util/ClientUtilsTest.java @@ -18,7 +18,11 @@ import org.apache.solr.SolrTestCase; import org.apache.solr.client.solrj.request.CollectionAdminRequest; +import org.apache.solr.client.solrj.request.HealthCheckRequest; +import org.apache.solr.client.solrj.request.QueryRequest; +import org.apache.solr.client.solrj.request.RequestWriter; import org.apache.solr.client.solrj.request.UpdateRequest; +import org.apache.solr.client.solrj.request.V2Request; import org.junit.Test; /** @@ -49,4 +53,56 @@ public void testDeterminesWhenToUseDefaultCollection() { "Expected default-coll to be skipped when a collection is explicitly provided", ClientUtils.shouldApplyDefaultCollection("someCollection", defaultNeededRequest)); } + + @Test + public void testUrlBuilding() throws Exception { + final var rw = new RequestWriter(); + // Simple case, non-collection request + { + final var request = new HealthCheckRequest(); + final var url = ClientUtils.buildRequestUrl(request, rw, "http://localhost:8983/solr", null); + assertEquals("http://localhost:8983/solr/admin/info/health", url); + } + + // Simple case, collection request + { + final var request = new QueryRequest(); + final var url = + ClientUtils.buildRequestUrl(request, rw, "http://localhost:8983/solr", "someColl"); + assertEquals("http://localhost:8983/solr/someColl/select", url); + } + + // Uses SolrRequest.getBasePath() to override baseUrl + { + final var request = new HealthCheckRequest(); + request.setBasePath("http://alternate-url:7574/solr"); + final var url = ClientUtils.buildRequestUrl(request, rw, "http://localhost:8983/solr", null); + assertEquals("http://alternate-url:7574/solr/admin/info/health", url); + } + + // V2 path is correct when solr.v2RealPath sysprop set + { + System.setProperty("solr.v2RealPath", "true"); + final var request = new V2Request.Builder("/collections").build(); + final var url = ClientUtils.buildRequestUrl(request, rw, "http://localhost:8983/solr", null); + assertEquals("http://localhost:8983/solr/____v2/collections", url); + } + + // V2 path is correct when solr.v2RealPath sysprop NOT set + { + System.clearProperty("solr.v2RealPath"); + final var request = new V2Request.Builder("/collections").build(); + final var url = ClientUtils.buildRequestUrl(request, rw, "http://localhost:8983/solr", null); + assertEquals("http://localhost:8983/api/collections", url); + } + + // Ignores collection when not needed (i.e. obeys SolrRequest.requiresCollection) + { + final var request = new HealthCheckRequest(); + final var url = + ClientUtils.buildRequestUrl( + request, rw, "http://localhost:8983/solr", "unneededCollection"); + assertEquals("http://localhost:8983/solr/admin/info/health", url); + } + } }