dsmiley commented on code in PR #4266:
URL: https://github.com/apache/solr/pull/4266#discussion_r3038104598
##########
solr/core/src/test/org/apache/solr/cloud/HttpPartitionOnCommitTest.java:
##########
@@ -199,7 +199,7 @@ protected void sendCommitWithRetry(Replica replica) throws
Exception {
}
} catch (Exception exc) {
Throwable rootCause = SolrException.getRootCause(exc);
- if (rootCause instanceof NoHttpResponseException) {
+ if (rootCause instanceof IOException) {
Review Comment:
in tests we trust... :pray:
NoHttpResponseException is Apache HttpClient specific
##########
solr/core/src/java/org/apache/solr/security/SimplePrincipal.java:
##########
@@ -18,37 +18,12 @@
package org.apache.solr.security;
import java.security.Principal;
-import java.util.Objects;
/** A basic name-only {@link Principal}. */
-public final class SimplePrincipal implements Principal {
- // TODO use a record. But javadoc tooling complained as it was confused
(Java version issue?)
Review Comment:
this mystery was solved previously, unblocking this nice conversion
##########
solr/solrj/src/java/org/apache/solr/common/util/URLUtil.java:
##########
Review Comment:
nice new utility... I definitely learned some edge cases
##########
solr/solrj-jetty/src/test/org/apache/solr/client/solrj/jetty/HttpJettySolrClientCompatibilityTest.java:
##########
Review Comment:
will revert
##########
solr/core/src/test/org/apache/solr/security/MultiAuthPluginTest.java:
##########
@@ -532,6 +501,57 @@ private void verifyWWWAuthenticateHeaders(HttpClient
httpClient, String baseUrl)
actualSchemes.stream().map(s ->
s.toLowerCase(Locale.ROOT)).collect(Collectors.toList()));
}
Review Comment:
I'll see if I can reduce what was added here. seems too much.
Ah; it was using utilities in another class. I restored that pattern.
Pushing a fix.
##########
solr/core/src/test/org/apache/solr/cloud/TestTlogReplica.java:
##########
@@ -409,28 +399,19 @@ public void testRealTimeGet()
activeReplicaCount(numNrtReplicas, numReplicas, 0));
DocCollection docCollection =
assertNumberOfReplicas(numNrtReplicas, numReplicas, 0, false, true);
- HttpClient httpClient = getHttpClient();
int id = 0;
Slice slice = docCollection.getSlice("shard1");
List<String> ids = new ArrayList<>(slice.getReplicas().size());
for (Replica rAdd : slice.getReplicas()) {
- try (SolrClient client =
- new HttpSolrClient.Builder(rAdd.getBaseUrl())
- .withDefaultCollection(rAdd.getCoreName())
- .withHttpClient(httpClient)
- .build()) {
+ try (SolrClient client =
cluster.getReplicaJetty(rAdd).newSolrClient(rAdd.getCoreName())) {
Review Comment:
using new convenience methods on JettySolrRunner
##########
solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java:
##########
@@ -60,15 +51,17 @@
import org.apache.solr.embedded.JettySolrRunner;
import org.apache.solr.util.TimeOut;
import org.apache.zookeeper.KeeperException;
+import org.eclipse.jetty.client.ContentResponse;
+import org.eclipse.jetty.client.HttpClient;
+import org.eclipse.jetty.client.StringRequestContent;
+import org.eclipse.jetty.http.HttpMethod;
import org.junit.After;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
public class AliasIntegrationTest extends SolrCloudTestCase {
- private CloseableHttpClient httpClient;
Review Comment:
usually no field is needed since it's easy to grab a managed Jetty
HttpClient from places
##########
solr/core/src/test/org/apache/solr/rest/TestRestManager.java:
##########
@@ -120,6 +120,18 @@ public void testRestManagerEndpoints() throws Exception {
// assertJQ("/config/managed", "/managedResources==[]");
}
+ /** Helper method to verify HEAD request returns expected status code */
Review Comment:
because I removed a "head" method from RestTestBase & RestTestHarness or
something like that -- underused.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrExceptionTest.java:
##########
Review Comment:
ported/moved
##########
solr/test-framework/src/java/org/apache/solr/cloud/api/collections/AbstractCloudBackupRestoreTestCase.java:
##########
Review Comment:
I'll revert for now
##########
solr/test-framework/src/java/org/apache/solr/handler/BackupRestoreUtils.java:
##########
@@ -77,42 +78,25 @@ public static void verifyDocs(int nDocs, SolrClient
leaderClient, String collect
public static void runCoreAdminCommand(
String baseUrl, String coreName, String action, Map<String, String>
params)
- throws IOException, URISyntaxException {
- final URI uri = new URI(baseUrl);
- final var oldPath = uri.getPath() != null ? uri.getPath().substring(1) :
"";
- final var newPath = "admin/cores";
- final var finalPath = oldPath.isEmpty() ? newPath : oldPath + "/" +
newPath;
-
- final URIBuilder builder =
Review Comment:
replaced URIBuilder with a combo of SolrParams and the new URLUtil.buildURI
##########
solr/test-framework/build.gradle:
##########
@@ -103,8 +105,6 @@ dependencies {
exclude group: "commons-codec", module: "commons-codec"
exclude group: "commons-logging", module: "commons-logging"
})
- implementation libs.apache.httpcomponents.httpclient
Review Comment:
not actually removing since these lines were duplicated
##########
solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java:
##########
@@ -238,49 +228,53 @@ public void testProperties() throws Exception {
public void testModifyPropertiesV2() throws Exception {
final String aliasName = getSaferTestName();
ZkStateReader zkStateReader = createColectionsAndAlias(aliasName);
- final String baseUrl =
cluster.getRandomJetty(random()).getBaseURLV2().toString();
+ final JettySolrRunner runner = cluster.getRandomJetty(random());
+ final String baseUrl = runner.getBaseURLV2().toString();
+ final HttpClient httpClient = runner.getSolrClient().getHttpClient();
String aliasApi = String.format(Locale.ENGLISH, "/aliases/%s/properties",
aliasName);
- HttpPut withoutBody = new HttpPut(baseUrl + aliasApi);
- assertEquals(400,
httpClient.execute(withoutBody).getStatusLine().getStatusCode());
-
- HttpPut update = new HttpPut(baseUrl + aliasApi);
- update.setEntity(
- new StringEntity(
- "{\n"
- + " \"properties\":\n"
- + " {\n"
- + " \"foo\": \"baz\",\n"
- + " \"bar\": \"bam\"\n"
- + " }\n"
- + "}",
- ContentType.APPLICATION_JSON));
- assertSuccess(update);
+ assertEquals(
+ 400, httpClient.newRequest(baseUrl +
aliasApi).method(HttpMethod.PUT).send().getStatus());
+
+ String body =
+ "{\n"
+ + " \"properties\":\n"
+ + " {\n"
+ + " \"foo\": \"baz\",\n"
+ + " \"bar\": \"bam\"\n"
+ + " }\n"
+ + "}";
+ assertSuccess(
+ httpClient
+ .newRequest(baseUrl + aliasApi)
+ .method(HttpMethod.PUT)
+ .body(new StringRequestContent("application/json", body,
StandardCharsets.UTF_8))
+ .send());
Review Comment:
I preferred this "fluent" style whenever possible as I thinks it helps
readability
##########
solr/test-framework/src/java/org/apache/solr/util/RestTestHarness.java:
##########
@@ -179,31 +161,40 @@ public void reload() throws Exception {
@Override
public String update(String xml) {
try {
- HttpPost httpPost = new HttpPost(getBaseURL() + "/update");
- httpPost.setEntity(
- new StringEntity(xml, ContentType.create("application/xml",
StandardCharsets.UTF_8)));
- return getResponse(httpPost);
+ return getResponse(
+ getHttpClient()
+ .newRequest(URLUtil.buildURI(getBaseURI(), "/update"))
+ .method("POST")
+ .body(new StringRequestContent("application/xml", xml,
StandardCharsets.UTF_8)));
+ } catch (RuntimeException e) {
+ throw e;
} catch (Exception e) {
throw new RuntimeException(e);
}
}
/** Executes the given request and returns the response. */
- private String getResponse(HttpUriRequest request) throws IOException {
- HttpEntity entity = null;
+ private String getResponse(Request request) throws IOException {
try {
- entity =
- httpClient
- .execute(request,
HttpClientUtil.createNewHttpClientRequestContext())
- .getEntity();
- return EntityUtils.toString(entity, StandardCharsets.UTF_8);
- } finally {
- EntityUtils.consumeQuietly(entity);
+ ContentResponse rsp = request.send();
+ // if (rsp.getStatus() >= 300 && rsp.getStatus()!=) {
Review Comment:
partial attempt at validation but that not all callers expect status 200 so
I left this commented... I should remove this
##########
solr/test-framework/src/java/org/apache/solr/util/RestTestHarness.java:
##########
@@ -69,25 +74,13 @@ public String getAdminURL() {
* @exception IOException any exception in the response.
*/
public String query(String request) throws IOException {
- return getResponse(new HttpGet(getBaseURL() + request));
+ return getResponse(
+ getHttpClient().newRequest(URLUtil.buildURI(getBaseURI(),
request)).method("GET"));
}
public String adminQuery(String request) throws IOException {
- return getResponse(new HttpGet(getAdminURL() + request));
- }
-
- /**
- * Processes a HEAD request using a URL path (with no context path) +
optional query params and
- * returns the response content.
- *
- * @param request The URL path and optional query params
- * @return The response to the HEAD request
- */
- public String head(String request) throws IOException {
Review Comment:
didn't seam worthy -- only one test was using it and seems to test the test
infra maybe. So that one does the HEAD directly with Jetty HttpClient, not
through here.
##########
solr/modules/jwt-auth/src/test/org/apache/solr/security/jwt/JWTAuthPluginIntegrationTest.java:
##########
@@ -537,18 +533,18 @@ private void executeCommand(String url, HttpClient cl,
String payload, JsonWebSi
final Set<Map.Entry<String, Object>> initialPlugins =
getAuthPluginsInUseForCluster(url).entrySet();
- HttpPost httpPost;
- HttpResponse r;
- httpPost = new HttpPost(url);
- if (jws != null) setAuthorizationHeader(httpPost, "Bearer " +
jws.getCompactSerialization());
- httpPost.setEntity(new ByteArrayEntity(payload.getBytes(UTF_8)));
- httpPost.addHeader("Content-Type", "application/json; charset=UTF-8");
- r = cl.execute(httpPost);
- String response = new String(r.getEntity().getContent().readAllBytes(),
StandardCharsets.UTF_8);
- assertEquals(
- "Non-200 response code. Response was " + response, 200,
r.getStatusLine().getStatusCode());
+ var req = httpClient.POST(url);
+ if (jws != null) {
Review Comment:
i'll follow-up to streamline this
##########
solr/core/src/test/org/apache/solr/cloud/TestAuthenticationFramework.java:
##########
Review Comment:
just some leftover cruft in here from something
##########
solr/core/src/test/org/apache/solr/security/MultiAuthPluginTest.java:
##########
@@ -475,52 +447,49 @@ public void testMultiAuthWithBasicPluginAndAjax() throws
Exception {
securityConfHandler.securityConfEdited();
// Pretend to send unauthorized AJAX request
- HttpGet httpGet = new HttpGet(baseUrl + CommonParams.SYSTEM_INFO_PATH);
- httpGet.addHeader(new BasicHeader("X-Requested-With", "XMLHttpRequest"));
-
- HttpResponse response = httpClient.execute(httpGet);
- assertEquals(
- "Unauthorized response was expected", 401,
response.getStatusLine().getStatusCode());
-
- // Only first plugin is expected as response, which is also xBasic if
BasicAuthPlugin
- Header[] headers = response.getHeaders(HttpHeaders.WWW_AUTHENTICATE);
- List<String> actualSchemes =
Arrays.stream(headers).map(Header::getValue).toList();
-
- // Only the first scheme is expected for AJAX-Requests
- assertEquals("Only one scheme was expected", 1, actualSchemes.size());
-
- // In case of BasicAuthPlugin, xBasic should be returned if AJAX request
sent and handled by
- // BasicAuthPlugin
- String expectedScheme = "xBasic realm=\"solr\"";
- assertEquals(
- "Mapped xBasic challenge expected from first plugin which is
BasicAuthPlugin",
- expectedScheme,
- actualSchemes.getFirst());
- } finally {
- if (httpClient != null) {
- HttpClientUtil.close(httpClient);
+ try {
+ var response =
+ httpClient
+ .newRequest(baseUrl + CommonParams.SYSTEM_INFO_PATH)
+ .headers(h -> h.add("X-Requested-With", "XMLHttpRequest"))
+ .send();
+ assertEquals("Unauthorized response was expected", 401,
response.getStatus());
+
+ // Only first plugin is expected as response, which is also xBasic if
BasicAuthPlugin
+ List<String> actualSchemes =
response.getHeaders().getValuesList("WWW-Authenticate");
+
+ // Only the first scheme is expected for AJAX-Requests
+ assertEquals("Only one scheme was expected", 1, actualSchemes.size());
+
+ // In case of BasicAuthPlugin, xBasic should be returned if AJAX
request sent and handled by
+ // BasicAuthPlugin
+ String expectedScheme = "xBasic realm=\"solr\"";
+ assertEquals(
+ "Mapped xBasic challenge expected from first plugin which is
BasicAuthPlugin",
+ expectedScheme,
+ actualSchemes.getFirst());
+ } catch (Exception e) {
+ throw new RuntimeException(e);
}
+ } finally {
if (solrClient != null) {
solrClient.close();
}
}
}
- private int doHttpGetAnonymous(HttpClient cl, String url) throws IOException
{
Review Comment:
I hate the naming choice of `cl` for client. I insist on `httpClient`.
##########
solr/solrj/src/java/org/apache/solr/common/util/URLUtil.java:
##########
@@ -157,4 +158,96 @@ public static String getNodeNameForBaseUrl(String solrUrl)
URL url = new URI(solrUrl).toURL();
return url.getAuthority() + url.getPath().replace('/', '_');
}
+
+ /**
+ * Constructs a properly encoded URI by combining a base URI with a request
path. This ensures
+ * special characters (like umlauts) are properly percent-encoded.
+ *
+ * @param baseUri the base URI (e.g., URI for "http://localhost:8983/solr";)
+ * @param path the path to append (e.g., "/config/overlay" or "admin/cores")
+ * @return a properly encoded URI
+ * @throws IllegalArgumentException if the baseUri or path are invalid
+ */
+ public static URI buildURI(URI baseUri, String path) {
+ return buildURI(baseUri, path, null);
+ }
+
+ /**
+ * Constructs a properly encoded URI by combining a base URI with a request
path and optional
+ * query parameters. This ensures special characters are properly
percent-encoded.
+ *
+ * @param baseUri the base URI (e.g., URI for "http://localhost:8983/solr";)
+ * @param path the path to append (e.g., "/config/overlay" or
"admin/cores"). May include a query
+ * string (e.g., "/select?q=test")
+ * @param queryParams optional query parameters to append (may be null). If
the path already
+ * contains a query string, these params will be added to it.
+ * @return a properly encoded URI
+ * @throws IllegalArgumentException if the baseUri or path are invalid
+ */
+ public static URI buildURI(URI baseUri, String path, SolrParams queryParams)
{
+ if (baseUri == null) {
+ throw new IllegalArgumentException("baseUri cannot be null");
+ }
+ if (path == null || path.isEmpty()) {
+ throw new IllegalArgumentException("path cannot be null or empty");
+ }
+
+ try {
+ // The path may contain a query string (e.g., "/path?param=value")
+ // Split on the first '?' to separate path from query
+ String pathOnly;
+ String queryFromPath = null;
+ int queryIndex = path.indexOf('?');
+ if (queryIndex != -1) {
+ pathOnly = path.substring(0, queryIndex);
+ queryFromPath = path.substring(queryIndex + 1);
+ } else {
+ pathOnly = path;
+ }
+
+ // Normalize path: remove leading slash if present
+ String normalizedPath = pathOnly.startsWith("/") ? pathOnly.substring(1)
: pathOnly;
+
+ // Construct the full path by combining base path with the additional
path
+ String basePath = baseUri.getPath();
+ if (basePath == null) {
+ basePath = "";
+ }
+ // Ensure base path ends with '/' for proper concatenation
+ if (!basePath.isEmpty() && !basePath.endsWith("/")) {
+ basePath += "/";
+ }
+ String fullPath = basePath + normalizedPath;
+
+ // Build the URI using the multi-parameter constructor which properly
encodes the path
+ URI uri =
+ new URI(
+ baseUri.getScheme(),
+ baseUri.getUserInfo(),
+ baseUri.getHost(),
+ baseUri.getPort(),
+ fullPath,
+ queryFromPath, // pass query from path (null if not present)
+ baseUri.getFragment());
+
+ // If no additional query params, return early
+ if (queryParams == null) {
+ return uri;
+ }
+
+ // Build the final URL combining query from path with additional params
+ String encodedQuery = queryParams.toQueryString(); // starts with '?'
Review Comment:
I'll likely push a fix to ensure we create just one URI in this method when
queryParams is passed.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java:
##########
@@ -16,89 +16,19 @@
*/
package org.apache.solr.client.solrj.embedded;
-import static org.apache.solr.common.util.Utils.fromJSONString;
-
-import java.io.ByteArrayInputStream;
-import java.nio.charset.StandardCharsets;
-import java.util.Map;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
-import org.apache.http.HttpResponse;
-import org.apache.http.client.HttpClient;
-import org.apache.http.client.methods.HttpPost;
-import org.apache.http.entity.InputStreamEntity;
import org.apache.solr.SolrTestCaseJ4.SuppressSSL;
import org.apache.solr.client.solrj.SolrClient;
import org.apache.solr.client.solrj.SolrExampleTests;
-import org.apache.solr.client.solrj.apache.HttpClientUtil;
-import org.apache.solr.client.solrj.apache.HttpSolrClient;
import org.apache.solr.client.solrj.request.SolrQuery;
import org.apache.solr.client.solrj.response.QueryResponse;
-import org.apache.solr.common.SolrDocument;
import org.apache.solr.common.SolrInputDocument;
import org.junit.Test;
-/**
- * TODO? perhaps use: http://docs.codehaus.org/display/JETTY/ServletTester
rather then open a real
- * connection?
- */
@SuppressSSL(bugUrl = "https://issues.apache.org/jira/browse/SOLR-5776";)
public class SolrExampleJettyTest extends SolrExampleTests {
- @Test
- public void testBadSetup() {
- String url = "http" + (isSSLMode() ? "s" : "") + "://127.0.0.1/?core=xxx";
- // This test does NOT fail for HttpJettySolrClient
- expectThrows(Exception.class, () -> new
HttpSolrClient.Builder(url).build());
- }
-
- @Test
- public void testArbitraryJsonIndexing() throws Exception {
Review Comment:
moved to superclass by using SolrClient APIs without referencing any
HttpClient
##########
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java:
##########
@@ -16,89 +16,19 @@
*/
package org.apache.solr.client.solrj.embedded;
-import static org.apache.solr.common.util.Utils.fromJSONString;
-
-import java.io.ByteArrayInputStream;
-import java.nio.charset.StandardCharsets;
-import java.util.Map;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
-import org.apache.http.HttpResponse;
-import org.apache.http.client.HttpClient;
-import org.apache.http.client.methods.HttpPost;
-import org.apache.http.entity.InputStreamEntity;
import org.apache.solr.SolrTestCaseJ4.SuppressSSL;
import org.apache.solr.client.solrj.SolrClient;
import org.apache.solr.client.solrj.SolrExampleTests;
-import org.apache.solr.client.solrj.apache.HttpClientUtil;
-import org.apache.solr.client.solrj.apache.HttpSolrClient;
import org.apache.solr.client.solrj.request.SolrQuery;
import org.apache.solr.client.solrj.response.QueryResponse;
-import org.apache.solr.common.SolrDocument;
import org.apache.solr.common.SolrInputDocument;
import org.junit.Test;
-/**
- * TODO? perhaps use: http://docs.codehaus.org/display/JETTY/ServletTester
rather then open a real
- * connection?
- */
@SuppressSSL(bugUrl = "https://issues.apache.org/jira/browse/SOLR-5776";)
public class SolrExampleJettyTest extends SolrExampleTests {
Review Comment:
I suspect we'll soon be able to remove SolrExampleJettyTest as it has
nothing interesting
##########
solr/core/src/test/org/apache/solr/cloud/MigrateReplicasTest.java:
##########
@@ -316,34 +310,31 @@ public void testFailOnSingleNode() throws Exception {
public Map<?, ?> callMigrateReplicas(CloudSolrClient cloudClient,
MigrateReplicasRequestBody body)
throws IOException {
- HttpEntityEnclosingRequestBase httpRequest = null;
- HttpEntity entity;
String response = null;
Map<?, ?> r = null;
String uri =
cluster.getJettySolrRunners().get(0).getBaseURLV2().toString()
+ "/cluster/replicas/migrate";
+ HttpClient httpClient =
cluster.getRandomJetty(random()).getSolrClient().getHttpClient();
try {
- httpRequest = new HttpPost(uri);
-
- httpRequest.setEntity(
- new ByteArrayEntity(
- Utils.toJSON(Utils.getReflectWriter(body)),
ContentType.APPLICATION_JSON));
- httpRequest.setHeader("Accept", "application/json");
- entity =
- ((CloudLegacySolrClient)
cloudClient).getHttpClient().execute(httpRequest).getEntity();
- try {
- response = EntityUtils.toString(entity, UTF_8);
- r = (Map<?, ?>) Utils.fromJSONString(response);
- assertNotNull("No response given from MigrateReplicas API", r);
- assertNotNull("No responseHeader given from MigrateReplicas API",
r.get("responseHeader"));
- } catch (JSONParser.ParseException e) {
- log.error("err response: {}", response);
- throw new AssertionError(e);
- }
- } finally {
- httpRequest.releaseConnection();
+ var resp =
+ httpClient
+ .newRequest(uri)
+ .method(HttpMethod.POST)
+ .body(
+ new BytesRequestContent(
+ "application/json",
Utils.toJSON(Utils.getReflectWriter(body))))
+ .send();
+ response = resp.getContentAsString();
+ r = (Map<?, ?>) Utils.fromJSONString(response);
+ assertNotNull("No response given from MigrateReplicas API", r);
+ assertNotNull("No responseHeader given from MigrateReplicas API",
r.get("responseHeader"));
+ } catch (JSONParser.ParseException e) {
+ log.error("err response: {}", response);
+ throw new AssertionError(e);
+ } catch (Exception e) {
+ throw new IOException(e);
Review Comment:
in a number of tests, the LLM I used rewraps exceptions like this, and
admittedly I don't like this pattern -- catching Exception (very broad) to
throw an IOException. This is a needless wrap if the error turns out otherwise
be a RuntimeException. But in its defense, Jetty HttpClient.send() doesn't
throw IOException, it throws 3 other things, and one of those is
ExecutionException which is a general wrapper for anything. So... I'm also
sympathetic do this as it's very simple and we probably need not be so
pendantic in tests.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientTest.java:
##########
@@ -923,18 +921,16 @@ public void testWrongZkChrootTest() throws IOException {
}
@Test
- public void customHttpClientTest() throws IOException {
- CloseableHttpClient client = HttpClientUtil.createClient(null);
- try (CloudSolrClient solrClient =
- new RandomizingCloudSolrClientBuilder(
-
Collections.singletonList(cluster.getZkServer().getZkAddress()),
Optional.empty())
- .withHttpClient(client)
- .build()) {
-
- assertSame(((CloudLegacySolrClient)
solrClient).getLbClient().getHttpClient(), client);
-
- } finally {
- HttpClientUtil.close(client);
+ public void customHttpClientTest() throws Exception {
Review Comment:
expanding test coverage of our new clients here
##########
solr/core/build.gradle:
##########
@@ -203,7 +203,6 @@ dependencies {
exclude group: "net.bytebuddy", module: "byte-buddy-agent"
})
testImplementation libs.apache.httpcomponents.httpclient
- testImplementation libs.apache.httpcomponents.httpcore
Review Comment:
progress
##########
solr/core/src/test/org/apache/solr/cloud/TestTlogReplica.java:
##########
@@ -176,9 +170,7 @@ public void testCreateDelete() throws Exception {
"conf",
2, // numShards
4); // tlogReplicas
- HttpGet createCollectionGet = new HttpGet(url);
- HttpResponse httpResponse =
getHttpClient().execute(createCollectionGet);
- assertEquals(200, httpResponse.getStatusLine().getStatusCode());
+ getHttpClient().newRequest(url).send();
Review Comment:
I may change these to .GET(url) which is slightly clearer.
##########
solr/modules/jwt-auth/src/test/org/apache/solr/security/jwt/JWTAuthPluginIntegrationTest.java:
##########
@@ -491,7 +487,7 @@ private Pair<String, Integer> post(String url, String json,
String token) throws
URL createUrl = URI.create(url).toURL();
HttpURLConnection con = (HttpURLConnection) createUrl.openConnection();
Review Comment:
Hmm; perhaps someday we shouldn't use JDK HttpURLConnection but I have no
appetite for that transition now.
##########
solr/core/src/test/org/apache/solr/cloud/TestConfigSetsAPI.java:
##########
@@ -1165,14 +1125,14 @@ private void assertConfigsetFiles(String configSetName,
String suffix, SolrZkCli
readFile("solr/configsets/upload/" + configSetName +
"/solrconfig.xml"));
}
- private long uploadConfigSet(
+ private int uploadConfigSet(
Review Comment:
in this test I was displeased seeing `long` where `int` should have been
used (for HTTP status), so I changed return types and assertions accordingly.
##########
solr/core/src/test/org/apache/solr/cloud/TestMiniSolrCloudClusterSSL.java:
##########
@@ -384,37 +384,35 @@ private static void checkClusterJettys(
* Trivial helper method for doing a HEAD request of the specified URL using
the specified client
* and getting the HTTP statusCode from the response
*/
- private static int doHeadRequest(final CloseableHttpClient client, final
String url)
- throws Exception {
- return client.execute(new HttpHead(url)).getStatusLine().getStatusCode();
+ private static int doHeadRequest(final HttpClient client, final String url)
throws Exception {
+ return client.newRequest(url).method("HEAD").send().getStatus();
}
/**
* Returns a new HttpClient that supports both HTTP and HTTPS (with the
default test truststore),
* but has no keystore -- so servers requiring client authentication should
fail.
*/
- private static CloseableHttpClient getSslAwareClientWithNoClientCerts() {
+ private static HttpClient newSslAwareClientWithNoClientCerts() throws
Exception {
Review Comment:
some non-trivial translation done here, taking inspiration from
HttpJettySolrClient which does similar
##########
solr/core/src/test/org/apache/solr/core/TestSolrConfigHandler.java:
##########
@@ -690,9 +689,8 @@ public static LinkedHashMapWriter testForResponseElement(
try {
m =
testServerBaseUrl == null
- ? getRespMap(uri, harness)
- : TestSolrConfigHandlerConcurrent.getAsMap(
- testServerBaseUrl + uri, cloudSolrClient);
+ ? getRespMap(harness, uri)
+ : getRespMap(testServerBaseUrl, uri);
Review Comment:
this was a little tricky to figure out when testServerBaseUrl != null...
can't use harness in this case... but I didn't want to use cloudSolrClient as
before since, at the moment, it might be Apache HttpClient based. So I
overloaded getRespMap for this case.
##########
solr/core/src/test/org/apache/solr/cloud/TestTlogReplica.java:
##########
@@ -374,15 +365,14 @@ private void addReplicaToShard(String shardName,
Replica.Type type)
collectionName,
shardName);
String requestBody = String.format(Locale.ROOT, "{\"type\": \"%s\"}",
type);
- HttpPost addReplicaPost = new HttpPost(url);
- addReplicaPost.setHeader("Content-type", "application/json");
- addReplicaPost.setEntity(new StringEntity(requestBody));
- httpResponse = getHttpClient().execute(addReplicaPost);
- if (httpResponse.getStatusLine().getStatusCode() == 400) {
- final String entity = EntityUtils.toString(httpResponse.getEntity());
- System.out.println(entity);
- }
- assertEquals(200, httpResponse.getStatusLine().getStatusCode());
+ var httpResponse2 =
+ getHttpClient()
+ .newRequest(url)
+ .method(HttpMethod.POST)
Review Comment:
similarly, I'll replace these with a shorter version
##########
solr/core/src/test/org/apache/solr/filestore/TestDistribFileStore.java:
##########
@@ -334,15 +330,12 @@ public static <T extends NavigableObject> T
assertResponseValues(
public static void uploadKey(byte[] bytes, String path, MiniSolrCloudCluster
cluster)
throws Exception {
JettySolrRunner jetty = cluster.getRandomJetty(random());
- try (HttpSolrClient client = (HttpSolrClient) jetty.newClient()) {
- PackageUtils.uploadKey(bytes, path,
jetty.getCoreContainer().getSolrHome());
+ PackageUtils.uploadKey(bytes, path,
jetty.getCoreContainer().getSolrHome());
+
+ final var syncReq = new FileStoreApi.SyncFile(path);
+ final var syncRsp = syncReq.process(jetty.getSolrClient());
Review Comment:
as in many places now, just using this new simple API providing a shared
SolrClient instead of creating a new client
##########
solr/core/src/test/org/apache/solr/core/TestSolrConfigHandler.java:
##########
@@ -119,7 +118,7 @@ public void after() throws Exception {
public void testProperty() throws Exception {
RestTestHarness harness = restTestHarness;
- MapWriter confMap = getRespMap("/config", harness);
+ MapWriter confMap = getRespMap(harness, "/config");
Review Comment:
a matter of taste but I *really* want to see the first arg be a
client/harness/recipient-of-remaining-args when we have method with such.
##########
solr/core/src/test/org/apache/solr/handler/admin/api/ClusterPropsAPITest.java:
##########
@@ -88,109 +81,121 @@ public void tearDown() throws Exception {
@Test
public void testClusterPropertyOpsAllGood() throws Exception {
- try (HttpSolrClient client = new
HttpSolrClient.Builder(baseUrl.toString()).build()) {
Review Comment:
mostly indentation because we no longer need to create a HttpSolrClient
here. Tip: When viewing diffs in GH; it's good to disable whitespace
##########
solr/core/src/test/org/apache/solr/security/BasicAuthStandaloneTest.java:
##########
@@ -155,42 +147,36 @@ static void doHttpPost(
}
static void doHttpPost(
- HttpClient cl,
+ HttpClient httpClient,
String url,
String jsonCommand,
String basicUser,
String basicPass,
int expectStatusCode)
throws IOException {
- doHttpPostWithHeader(
- cl, url, jsonCommand, getBasicAuthHeader(basicUser, basicPass),
expectStatusCode);
- }
-
- static void doHttpPostWithHeader(
- HttpClient cl, String url, String jsonCommand, Header header, int
expectStatusCode)
- throws IOException {
- HttpPost httpPost = new HttpPost(url);
- httpPost.setHeader(header);
- httpPost.setEntity(new ByteArrayEntity(jsonCommand.replace("'",
"\"").getBytes(UTF_8)));
- httpPost.addHeader("Content-Type", "application/json; charset=UTF-8");
- HttpResponse r = cl.execute(httpPost);
- int statusCode = r.getStatusLine().getStatusCode();
- HttpClientUtil.consumeFully(r.getEntity());
- assertEquals("proper_cred sent, but access denied", expectStatusCode,
statusCode);
- }
-
- private static Header getBasicAuthHeader(String user, String pwd) {
- String userPass = user + ":" + pwd;
- String encoded =
Base64.getEncoder().encodeToString(userPass.getBytes(UTF_8));
- return new BasicHeader("Authorization", "Basic " + encoded);
+ try {
+ var rsp =
+ httpClient
+ .POST(url)
+ .headers(
+ h -> h.add("Authorization",
encodeBasicAuthHeaderIfNotNull(basicUser, basicPass)))
Review Comment:
small refactoring to take advantage of HttpClient header.add doing nothing
if the value is null. So added null-returning header value method
encodeBasicAuthHeaderIfNotNull. Ultimately allows to use the fluent style
instead of if-conditions to add headers. Maybe this specific spot doesn't show
this well but it does elsewhere.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientCacheTest.java:
##########
@@ -120,7 +119,7 @@ public DocCollection get() {
return new SocketException("TEST");
}
if (i == 3) {
- return new NoHttpResponseException("TEST");
+ return new ConnectException("TEST");
Review Comment:
exception differences between both clients can be tricky when migrating...
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleTests.java:
##########
@@ -1023,6 +1025,39 @@ public void testMultiContentStreamRequest() throws
Exception {
assertEquals(5, rsp.getResults().getNumFound());
}
+ @Test
+ public void testArbitraryJsonIndexing() throws Exception {
Review Comment:
again, the one I ported/moved
##########
solr/test-framework/src/java/org/apache/solr/util/RestTestHarness.java:
##########
Review Comment:
ground zero for a chunk of the PR
##########
solr/solrj/src/test/org/apache/solr/common/util/URLUtilTest.java:
##########
Review Comment:
new tests for new code
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
