Skip to content

Commit 437e279

Browse files
gerlowskijagerlowskija
authored
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.
1 parent 8c5ae1b commit 437e279

File tree

8 files changed

+140
-72
lines changed

8 files changed

+140
-72
lines changed

‎solr/core/src/test/org/apache/solr/handler/admin/HealthCheckHandlerTest.java‎

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@
2525
import java.util.Properties;
2626
import org.apache.solr.client.solrj.SolrClient;
2727
import org.apache.solr.client.solrj.SolrRequest;
28-
import org.apache.solr.client.solrj.SolrResponse;
2928
import org.apache.solr.client.solrj.SolrServerException;
3029
import org.apache.solr.client.solrj.impl.BaseHttpSolrClient;
3130
import org.apache.solr.client.solrj.request.CollectionAdminRequest;
@@ -54,24 +53,27 @@ public static void setupCluster() throws Exception {
5453
configureCluster(1).addConfig("conf", configset("cloud-minimal")).configure();
5554
}
5655

56+
private HealthCheckResponse runHealthcheckWithClient(SolrClient client) throws Exception {
57+
return new HealthCheckRequest().process(client);
58+
}
59+
5760
@Test
5861
public void testHealthCheckHandler() throws Exception {
59-
GenericSolrRequest req =
60-
new GenericSolrRequest(SolrRequest.METHOD.GET, HEALTH_CHECK_HANDLER_PATH);
6162

6263
// positive check that our only existing "healthy" node works with cloud client
6364
// NOTE: this is using GenericSolrRequest, not HealthCheckRequest which is why it passes
6465
// as compared with testHealthCheckHandlerWithCloudClient
6566
// (Not sure if that's actually a good thing -- but it's how the existing test worked)
67+
final var genericHealthcheck =
68+
new GenericSolrRequest(SolrRequest.METHOD.GET, HEALTH_CHECK_HANDLER_PATH);
6669
assertEquals(
6770
CommonParams.OK,
68-
req.process(cluster.getSolrClient()).getResponse().get(CommonParams.STATUS));
71+
genericHealthcheck.process(cluster.getSolrClient()).getResponse().get(CommonParams.STATUS));
6972

7073
// positive check that our exiting "healthy" node works with direct http client
7174
try (SolrClient solrClient =
7275
getHttpSolrClient(cluster.getJettySolrRunner(0).getBaseUrl().toString())) {
73-
SolrResponse response = req.process(solrClient);
74-
assertEquals(CommonParams.OK, response.getResponse().get(CommonParams.STATUS));
76+
assertEquals(CommonParams.OK, runHealthcheckWithClient(solrClient).getNodeStatus());
7577
}
7678

7779
// successfully create a dummy collection
@@ -82,8 +84,7 @@ public void testHealthCheckHandler() throws Exception {
8284
.withProperty("solr.directoryFactory", "solr.StandardDirectoryFactory")
8385
.process(solrClient);
8486
assertEquals(0, collectionAdminResponse.getStatus());
85-
SolrResponse response = req.process(solrClient);
86-
assertEquals(CommonParams.OK, response.getResponse().get(CommonParams.STATUS));
87+
assertEquals(CommonParams.OK, runHealthcheckWithClient(solrClient).getNodeStatus());
8788
} finally {
8889
cluster.deleteAllCollections();
8990
cluster.deleteAllConfigSets();
@@ -94,7 +95,8 @@ public void testHealthCheckHandler() throws Exception {
9495
try (SolrClient solrClient = getHttpSolrClient(newJetty.getBaseUrl().toString())) {
9596

9697
// positive check that our (new) "healthy" node works with direct http client
97-
assertEquals(CommonParams.OK, req.process(solrClient).getResponse().get(CommonParams.STATUS));
98+
final var response = runHealthcheckWithClient(solrClient);
99+
assertEquals(CommonParams.OK, response.getNodeStatus());
98100

99101
// now "break" our (new) node
100102
newJetty.getCoreContainer().getZkController().getZkClient().close();
@@ -104,7 +106,7 @@ public void testHealthCheckHandler() throws Exception {
104106
expectThrows(
105107
BaseHttpSolrClient.RemoteSolrException.class,
106108
() -> {
107-
req.process(solrClient);
109+
runHealthcheckWithClient(solrClient);
108110
});
109111
assertTrue(e.getMessage(), e.getMessage().contains("Host Unavailable"));
110112
assertEquals(SolrException.ErrorCode.SERVICE_UNAVAILABLE.code, e.code());
@@ -118,7 +120,7 @@ public void testHealthCheckHandler() throws Exception {
118120
try (SolrClient solrClient = getHttpSolrClient(newJetty.getBaseUrl().toString())) {
119121

120122
// positive check that our (new) "healthy" node works with direct http client
121-
assertEquals(CommonParams.OK, req.process(solrClient).getResponse().get(CommonParams.STATUS));
123+
assertEquals(CommonParams.OK, runHealthcheckWithClient(solrClient).getNodeStatus());
122124

123125
// shutdown the core container of new node
124126
newJetty.getCoreContainer().shutdown();
@@ -128,7 +130,7 @@ public void testHealthCheckHandler() throws Exception {
128130
expectThrows(
129131
SolrException.class,
130132
() -> {
131-
req.process(solrClient).getResponse().get(CommonParams.STATUS);
133+
runHealthcheckWithClient(solrClient).getNodeStatus();
132134
fail("API shouldn't be available, and fail at above request");
133135
});
134136
assertEquals("Exception code should be 404", 404, thrown.code());
@@ -147,7 +149,7 @@ public void testHealthCheckHandler() throws Exception {
147149
try (SolrClient solrClient =
148150
getHttpSolrClient(cluster.getJettySolrRunner(0).getBaseUrl().toString())) {
149151

150-
assertEquals(CommonParams.OK, req.process(solrClient).getResponse().get(CommonParams.STATUS));
152+
assertEquals(CommonParams.OK, runHealthcheckWithClient(solrClient).getNodeStatus());
151153
}
152154
}
153155

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -426,7 +426,7 @@ public CompletableFuture<NamedList<Object>> requestAsync(
426426
final MakeRequestReturnValue mrrv;
427427
final String url;
428428
try {
429-
url = getRequestPath(solrRequest, collection);
429+
url = getRequestUrl(solrRequest, collection);
430430
mrrv = makeRequest(solrRequest, url, true);
431431
} catch (SolrServerException | IOException e) {
432432
future.completeExceptionally(e);
@@ -493,7 +493,7 @@ public NamedList<Object> request(SolrRequest<?> solrRequest, String collection)
493493
if (ClientUtils.shouldApplyDefaultCollection(collection, solrRequest)) {
494494
collection = defaultCollection;
495495
}
496-
String url = getRequestPath(solrRequest, collection);
496+
String url = getRequestUrl(solrRequest, collection);
497497
Throwable abortCause = null;
498498
Request req = null;
499499
try {

‎solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,7 @@ private PreparedRequest prepareRequest(SolrRequest<?> solrRequest, String collec
198198
if (ClientUtils.shouldApplyDefaultCollection(collection, solrRequest)) {
199199
collection = defaultCollection;
200200
}
201-
String url = getRequestPath(solrRequest, collection);
201+
String url = getRequestUrl(solrRequest, collection);
202202
ResponseParser parserToUse = responseParser(solrRequest);
203203
ModifiableSolrParams queryParams = initalizeSolrParams(solrRequest, parserToUse);
204204
var reqb = HttpRequest.newBuilder();

‎solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java‎

Lines changed: 8 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,7 @@
2525
import java.io.UnsupportedEncodingException;
2626
import java.lang.invoke.MethodHandles;
2727
import java.net.ConnectException;
28-
import java.net.MalformedURLException;
2928
import java.net.SocketTimeoutException;
30-
import java.net.URL;
3129
import java.nio.charset.Charset;
3230
import java.nio.charset.StandardCharsets;
3331
import java.security.Principal;
@@ -334,22 +332,15 @@ protected ModifiableSolrParams calculateQueryParams(
334332
return queryModParams;
335333
}
336334

337-
static String changeV2RequestEndpoint(String basePath) throws MalformedURLException {
338-
URL oldURL = new URL(basePath);
339-
String newPath = oldURL.getPath().replaceFirst("/solr", "/api");
340-
return new URL(oldURL.getProtocol(), oldURL.getHost(), oldURL.getPort(), newPath).toString();
341-
}
342-
343335
protected HttpRequestBase createMethod(SolrRequest<?> request, String collection)
344336
throws IOException, SolrServerException {
345337
SolrParams params = request.getParams();
346338
RequestWriter.ContentWriter contentWriter = requestWriter.getContentWriter(request);
347339
Collection<ContentStream> streams =
348340
contentWriter == null ? requestWriter.getContentStreams(request) : null;
349-
String path = requestWriter.getPath(request);
350-
if (path == null || !path.startsWith("/")) {
351-
path = DEFAULT_PATH;
352-
}
341+
342+
final String requestUrlBeforeParams =
343+
ClientUtils.buildRequestUrl(request, requestWriter, baseUrl, collection);
353344

354345
ResponseParser parser = request.getResponseParser();
355346
if (parser == null) {
@@ -369,36 +360,24 @@ protected HttpRequestBase createMethod(SolrRequest<?> request, String collection
369360
wparams.add(invariantParams);
370361
}
371362

372-
String basePath = baseUrl;
373-
if (collection != null) basePath += "/" + collection;
374-
375-
if (request instanceof V2Request) {
376-
if (System.getProperty("solr.v2RealPath") == null || ((V2Request) request).isForceV2()) {
377-
basePath = changeV2RequestEndpoint(baseUrl);
378-
} else {
379-
basePath = baseUrl + "/____v2";
380-
}
381-
}
382-
383363
if (SolrRequest.METHOD.GET == request.getMethod()) {
384364
if (streams != null || contentWriter != null) {
385365
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "GET can't send streams!");
386366
}
387367

388-
HttpGet result = new HttpGet(basePath + path + wparams.toQueryString());
368+
HttpGet result = new HttpGet(requestUrlBeforeParams + wparams.toQueryString());
389369

390370
populateHeaders(result, contextHeaders);
391371
return result;
392372
}
393373

394374
if (SolrRequest.METHOD.DELETE == request.getMethod()) {
395-
return new HttpDelete(basePath + path + wparams.toQueryString());
375+
return new HttpDelete(requestUrlBeforeParams + wparams.toQueryString());
396376
}
397377

398378
if (SolrRequest.METHOD.POST == request.getMethod()
399379
|| SolrRequest.METHOD.PUT == request.getMethod()) {
400380

401-
String url = basePath + path;
402381
boolean hasNullStreamName = false;
403382
if (streams != null) {
404383
for (ContentStream cs : streams) {
@@ -416,7 +395,7 @@ protected HttpRequestBase createMethod(SolrRequest<?> request, String collection
416395
List<NameValuePair> postOrPutParams = new ArrayList<>();
417396

418397
if (contentWriter != null) {
419-
String fullQueryUrl = url + wparams.toQueryString();
398+
String fullQueryUrl = requestUrlBeforeParams + wparams.toQueryString();
420399
HttpEntityEnclosingRequestBase postOrPut =
421400
SolrRequest.METHOD.POST == request.getMethod()
422401
? new HttpPost(fullQueryUrl)
@@ -443,15 +422,15 @@ public void writeTo(OutputStream outstream) throws IOException {
443422
// send server list and request list as query string params
444423
ModifiableSolrParams queryParams = calculateQueryParams(this.urlParamNames, wparams);
445424
queryParams.add(calculateQueryParams(request.getQueryParams(), wparams));
446-
String fullQueryUrl = url + queryParams.toQueryString();
425+
String fullQueryUrl = requestUrlBeforeParams + queryParams.toQueryString();
447426
HttpEntityEnclosingRequestBase postOrPut =
448427
fillContentStream(
449428
request, streams, wparams, isMultipart, postOrPutParams, fullQueryUrl);
450429
return postOrPut;
451430
}
452431
// It is has one stream, it is the post body, put the params in the URL
453432
else {
454-
String fullQueryUrl = url + wparams.toQueryString();
433+
String fullQueryUrl = requestUrlBeforeParams + wparams.toQueryString();
455434
HttpEntityEnclosingRequestBase postOrPut =
456435
SolrRequest.METHOD.POST == request.getMethod()
457436
? new HttpPost(fullQueryUrl)

‎solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java‎

Lines changed: 4 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
import java.io.IOException;
2424
import java.io.InputStream;
2525
import java.net.MalformedURLException;
26-
import java.net.URL;
2726
import java.nio.charset.Charset;
2827
import java.nio.charset.StandardCharsets;
2928
import java.util.Arrays;
@@ -42,6 +41,7 @@
4241
import org.apache.solr.client.solrj.SolrServerException;
4342
import org.apache.solr.client.solrj.request.RequestWriter;
4443
import org.apache.solr.client.solrj.request.V2Request;
44+
import org.apache.solr.client.solrj.util.ClientUtils;
4545
import org.apache.solr.common.SolrException;
4646
import org.apache.solr.common.params.CommonParams;
4747
import org.apache.solr.common.params.ModifiableSolrParams;
@@ -51,7 +51,7 @@
5151

5252
public abstract class HttpSolrClientBase extends SolrClient {
5353

54-
protected static final String DEFAULT_PATH = "/select";
54+
protected static final String DEFAULT_PATH = ClientUtils.DEFAULT_PATH;
5555
protected static final Charset FALLBACK_CHARSET = StandardCharsets.UTF_8;
5656
private static final List<String> errPath = Arrays.asList("metadata", "error-class");
5757

@@ -111,30 +111,9 @@ protected HttpSolrClientBase(String serverBaseUrl, HttpSolrClientBuilderBase<?,
111111
}
112112
}
113113

114-
protected String getRequestPath(SolrRequest<?> solrRequest, String collection)
114+
protected String getRequestUrl(SolrRequest<?> solrRequest, String collection)
115115
throws MalformedURLException {
116-
String basePath = solrRequest.getBasePath() == null ? serverBaseUrl : solrRequest.getBasePath();
117-
if (collection != null) basePath += "/" + collection;
118-
119-
if (solrRequest instanceof V2Request) {
120-
if (System.getProperty("solr.v2RealPath") == null) {
121-
basePath = changeV2RequestEndpoint(basePath);
122-
} else {
123-
basePath = serverBaseUrl + "/____v2";
124-
}
125-
}
126-
String path = requestWriter.getPath(solrRequest);
127-
if (path == null || !path.startsWith("/")) {
128-
path = DEFAULT_PATH;
129-
}
130-
131-
return basePath + path;
132-
}
133-
134-
protected String changeV2RequestEndpoint(String basePath) throws MalformedURLException {
135-
URL oldURL = new URL(basePath);
136-
String newPath = oldURL.getPath().replaceFirst("/solr", "/api");
137-
return new URL(oldURL.getProtocol(), oldURL.getHost(), oldURL.getPort(), newPath).toString();
116+
return ClientUtils.buildRequestUrl(solrRequest, requestWriter, serverBaseUrl, collection);
138117
}
139118

140119
protected ResponseParser responseParser(SolrRequest<?> solrRequest) {

‎solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -484,8 +484,8 @@ protected Exception doRequest(
484484
Exception ex = null;
485485
try {
486486
rsp.server = baseUrl.toString();
487-
req.getRequest().setBasePath(baseUrl.toString());
488-
rsp.rsp = getClient(baseUrl).request(req.getRequest(), (String) null);
487+
req.getRequest().setBasePath(baseUrl.getBaseUrl());
488+
rsp.rsp = getClient(baseUrl).request(req.getRequest(), baseUrl.getCore());
489489
if (isZombie) {
490490
zombieServers.remove(baseUrl.toString());
491491
}

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

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919
import java.io.IOException;
2020
import java.io.StringWriter;
2121
import java.io.Writer;
22+
import java.net.MalformedURLException;
23+
import java.net.URL;
2224
import java.nio.ByteBuffer;
2325
import java.nio.charset.StandardCharsets;
2426
import java.util.ArrayList;
@@ -27,8 +29,11 @@
2729
import java.util.Date;
2830
import java.util.Map;
2931
import java.util.Map.Entry;
32+
import org.apache.solr.client.solrj.SolrClient;
3033
import org.apache.solr.client.solrj.SolrRequest;
3134
import org.apache.solr.client.solrj.SolrResponse;
35+
import org.apache.solr.client.solrj.request.RequestWriter;
36+
import org.apache.solr.client.solrj.request.V2Request;
3237
import org.apache.solr.common.SolrInputDocument;
3338
import org.apache.solr.common.SolrInputField;
3439
import org.apache.solr.common.cloud.Slice;
@@ -44,6 +49,8 @@ public class ClientUtils {
4449
public static final String TEXT_XML = "application/xml; charset=UTF-8";
4550
public static final String TEXT_JSON = "application/json; charset=UTF-8";
4651

52+
public static final String DEFAULT_PATH = "/select";
53+
4754
/** Take a string and make it an iterable ContentStream */
4855
public static Collection<ContentStream> toContentStreams(
4956
final String str, final String contentType) {
@@ -56,6 +63,51 @@ public static Collection<ContentStream> toContentStreams(
5663
return streams;
5764
}
5865

66+
/**
67+
* Create the full URL for a SolrRequest (excepting query parameters) as a String
68+
*
69+
* @param solrRequest the {@link SolrRequest} to build the URL for
70+
* @param requestWriter a {@link RequestWriter} from the {@link SolrClient} that will be sending
71+
* the request
72+
* @param serverRootUrl the root URL of the Solr server being targeted. May by overridden {@link
73+
* SolrRequest#getBasePath()}, if present.
74+
* @param collection the collection to send the request to. May be null if no collection is
75+
* needed.
76+
* @throws MalformedURLException if {@code serverRootUrl} or {@link SolrRequest#getBasePath()}
77+
* contain a malformed URL string
78+
*/
79+
public static String buildRequestUrl(
80+
SolrRequest<?> solrRequest,
81+
RequestWriter requestWriter,
82+
String serverRootUrl,
83+
String collection)
84+
throws MalformedURLException {
85+
String basePath = solrRequest.getBasePath() == null ? serverRootUrl : solrRequest.getBasePath();
86+
87+
if (solrRequest instanceof V2Request) {
88+
if (System.getProperty("solr.v2RealPath") == null) {
89+
basePath = changeV2RequestEndpoint(basePath);
90+
} else {
91+
basePath = serverRootUrl + "/____v2";
92+
}
93+
}
94+
95+
if (solrRequest.requiresCollection() && collection != null) basePath += "/" + collection;
96+
97+
String path = requestWriter.getPath(solrRequest);
98+
if (path == null || !path.startsWith("/")) {
99+
path = DEFAULT_PATH;
100+
}
101+
102+
return basePath + path;
103+
}
104+
105+
private static String changeV2RequestEndpoint(String basePath) throws MalformedURLException {
106+
URL oldURL = new URL(basePath);
107+
String newPath = oldURL.getPath().replaceFirst("/solr", "/api");
108+
return new URL(oldURL.getProtocol(), oldURL.getHost(), oldURL.getPort(), newPath).toString();
109+
}
110+
59111
// ------------------------------------------------------------------------
60112
// ------------------------------------------------------------------------
61113

0 commit comments

Comments
 (0)