Skip to content

Commit

Permalink
SOLR-17044: Consolidate SolrJ URL-building logic (#2455)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@gerlowskija
gerlowskija authored May 29, 2024
1 parent 8c5ae1b commit 437e279
Show file tree
Hide file tree
Showing 8 changed files with 140 additions and 72 deletions.
28 changes: 15 additions & 13 deletions solr/core/src/test/org/apache/solr/handler/admin/HealthCheckHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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());
Expand All @@ -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();
Expand All @@ -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());
Expand All @@ -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());
}
}

Expand Down
4 changes: 2 additions & 2 deletions solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@ public CompletableFuture<NamedList<Object>> 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);
Expand Down Expand Up @@ -493,7 +493,7 @@ public NamedList<Object> 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 {
Expand Down
2 changes: 1 addition & 1 deletion solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
37 changes: 8 additions & 29 deletions solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<ContentStream> 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) {
Expand All @@ -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) {
Expand All @@ -416,7 +395,7 @@ protected HttpRequestBase createMethod(SolrRequest<?> request, String collection
List<NameValuePair> 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)
Expand All @@ -443,15 +422,15 @@ 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);
return postOrPut;
}
// 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)
Expand Down
29 changes: 4 additions & 25 deletions solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrClientBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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<String> errPath = Arrays.asList("metadata", "error-class");

Expand Down Expand Up @@ -111,30 +111,9 @@ protected HttpSolrClientBase(String serverBaseUrl, HttpSolrClientBuilderBase<?,
}
}

protected String getRequestPath(SolrRequest<?> 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) {
Expand Down
4 changes: 2 additions & 2 deletions solr/solrj/src/java/org/apache/solr/client/solrj/impl/LBSolrClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down
52 changes: 52 additions & 0 deletions solr/solrj/src/java/org/apache/solr/client/solrj/util/ClientUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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<ContentStream> toContentStreams(
final String str, final String contentType) {
Expand All @@ -56,6 +63,51 @@ public static Collection<ContentStream> 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();
}

// ------------------------------------------------------------------------
// ------------------------------------------------------------------------

Expand Down
Loading

0 comments on commit 437e279

Please sign in to comment.