SOLR-18188: Tests, switch from Apache HttpClient to Jetty HttpClient by dsmiley · Pull Request #4266 · apache/solr

dsmiley · 2026-04-06T05:45:00Z

https://issues.apache.org/jira/browse/SOLR-18188

This PR switches nearly all tests that were using Apache HttpClient directly to instead use Jetty HttpClient (or in some cases a plain SolrClient given we can do a lot with them). Also any tests with import org.apache.http.

As of this writing, I deferred ~8 tests that casted a cloud client to CloudLegacySolrClient to then call getHttpClient merely to create an HttpSolrClient with the same backing client -- maybe for efficiency. TBD on their fate.

SimplePrinciple was made a record and used in places where an Apache variant was used.

In URLUtils, I added a new method: buildURI which is needed in RestTestHarness, BackupRestoreUtils, and I think eventually more places. It handles URI encoding matters. Apparently Apache HttpClient was doing things implicitly that Jetty doesn't.

This is another step in a journey that continues

(follow-up note: scope was reduced. see #4270 for the rest).

…ient etc. is still allowed. Don't import org.apache.http classes. Mostly done via Google Jules.

Introduce URLUtil.buildURI

epugh

14 out of 67 reviewed, this is all looking great! Really excited to see this big lift done, and the tests are reading much nicer.

solr/core/src/java/org/apache/solr/security/SimplePrincipal.java

solr/core/src/test/org/apache/solr/cloud/api/collections/CollectionReloadTest.java

epugh · 2026-04-06T07:23:22Z

solr/core/src/test/org/apache/solr/cloud/api/collections/CollectionReloadTest.java

-        createCollectionPost.setEntity(new StringEntity(requestBody));
-        HttpResponse httpResponse =
-            ((CloudLegacySolrClient) cluster.getSolrClient())
+        var response2 =


This is much nicer, much less clunky plumbing!

even thoguh we set all the same things.

...re/src/test/org/apache/solr/cloud/api/collections/TestCollectionsAPIViaSolrCloudCluster.java

epugh · 2026-04-06T07:29:13Z

solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java

+        400, httpClient.newRequest(baseUrl + aliasApi).method(HttpMethod.PUT).send().getStatus());
+
+    String body =
+        "{\n"


i had good luck with the """ tags to simplify json formatted strings. Since you are reformatting a lot in this PR, might be worth embracing it for the JSON?

I may change some of these where I add new code but otherwise don't want to change more lines in a massive PR.

solr/core/src/test/org/apache/solr/cloud/TestMiniSolrCloudClusterSSL.java

epugh · 2026-04-06T07:53:31Z

solr/core/src/test/org/apache/solr/cloud/TestPullReplica.java

-          url = url + pickRandom("", "&nrtReplicas=1", "&replicationFactor=1");
-          HttpGet createCollectionGet = new HttpGet(url);
-          getHttpClient().execute(createCollectionGet);
+          url1 = url1 + pickRandom("", "&nrtReplicas=1", "&replicationFactor=1");


ah my old friend, you are back again numbered variable..

I'm able to address most/all of those issues via simply using switch-expressions, which creates blocks of isolated scope

solr/core/src/test/org/apache/solr/core/TestSolrConfigHandler.java

epugh · 2026-04-06T07:59:10Z

solr/core/src/test/org/apache/solr/core/TestSolrConfigHandler.java


+  @SuppressWarnings({"rawtypes"})
+  private static LinkedHashMapWriter getRespMap(String baseUrl, String path) throws Exception {
+    // a hack... if possible pass in an httpClient and also be able to init RestTestHarness with it


is this a note to yourself for future work in this PR, or for a future developer? if it's for a future dev, maybe elaborate a bit more and add that tag we use? todo or something?

"hack" is possibly overstated. It's not ideal to create a new harness (which creates a client) when I'd rather somehow re-use a client. I don't think I feel strongly enough now to bother with TODO here.

I did a little... trick/hack enabling use of TestHarness created for a different baseUrl, and thus this method is now removed.

epugh · 2026-04-06T08:00:50Z

solr/core/src/test/org/apache/solr/filestore/TestDistribFileStore.java

+                .newRequest(baseUrl + "/cluster/filestore/files" + path)
+                .send();
+        assertEquals(200, resp.getStatus());
+        ByteBuffer buf = ByteBuffer.wrap(resp.getContent());


reads so much nicer!

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

dsmiley

This PR is huge...
Honestly review-wise... if tests pass...
I'll try and draw attention to "interesting" things.

dsmiley · 2026-04-06T05:45:46Z

solr/core/src/java/org/apache/solr/security/SimplePrincipal.java


 /** 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?)


this mystery was solved previously, unblocking this nice conversion

dsmiley · 2026-04-06T05:47:22Z

solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java


 public class AliasIntegrationTest extends SolrCloudTestCase {

-  private CloseableHttpClient httpClient;


usually no field is needed since it's easy to grab a managed Jetty HttpClient from places

dsmiley · 2026-04-06T05:48:10Z

solr/core/src/test/org/apache/solr/cloud/AliasIntegrationTest.java

+        httpClient
+            .newRequest(baseUrl + aliasApi)
+            .method(HttpMethod.PUT)
+            .body(new StringRequestContent("application/json", body, StandardCharsets.UTF_8))
+            .send());


I preferred this "fluent" style whenever possible as I thinks it helps readability

dsmiley · 2026-04-06T05:51:54Z

solr/core/src/test/org/apache/solr/cloud/HttpPartitionOnCommitTest.java

      } catch (Exception exc) {
        Throwable rootCause = SolrException.getRootCause(exc);
-        if (rootCause instanceof NoHttpResponseException) {
+        if (rootCause instanceof IOException) {


in tests we trust... 🙏
NoHttpResponseException is Apache HttpClient specific

solr/solrj/src/java/org/apache/solr/common/util/URLUtil.java

dsmiley · 2026-04-06T16:37:52Z

solr/solrj/src/test/org/apache/solr/client/solrj/SolrExceptionTest.java

ported/moved

solr/solrj/src/test/org/apache/solr/common/util/URLUtilTest.java

...ework/src/java/org/apache/solr/cloud/api/collections/AbstractCloudBackupRestoreTestCase.java

solr/test-framework/src/java/org/apache/solr/handler/BackupRestoreUtils.java

solr/test-framework/src/java/org/apache/solr/util/RestTestHarness.java

…88-migrateToJettyHttpClient # Conflicts: # solr/solrj/src/test/org/apache/solr/client/solrj/embedded/JettyWebappTest.java

Didn't need version suffixes on vars since they are now scoped.

* better exception handling. * allow passing an absolute URL thus overriding the base

…ettyHttpClient # Conflicts: # solr/test-framework/build.gradle

…onstruct RestTestHarness is now immutable and no longer owns resources, making it cheap to create on-the-fly for different URLs without complex lifecycle management. Changes to RestTestHarness: - Removed Closeable interface - RTH no longer owns the HttpClient - Removed RESTfulServerProvider interface and setServerProvider() method - Removed the "://" hack for detecting absolute URLs in buildUri() - New constructor: RestTestHarness(HttpClient, String baseUrl) - Convenience constructor: RestTestHarness(JettySolrRunner, String coreOrCollection) Gets HttpClient from jetty.getSolrClient().getHttpClient() - Added newWithUrl(String) helper to create new instance with different URL sharing the same HttpClient Updated test infrastructure: - Simplified testForResponseElement() by removing testServerBaseUrl and cloudSolrClient parameters; now requires non-null harness - Simplified reqhandlertests() to take only RestTestHarness parameter - Removed all close() calls and try-with-resources blocks - Updated tests to create RTH instances on-the-fly instead of mutating via setServerProvider() - TestReqParamsAPI, TestSolrConfigHandlerCloud, and others now use harness.newWithUrl() to query specific replica URLs - TestSolrConfigHandlerConcurrent.getAsMap() now takes RestTestHarness instead of HttpClient + URI

RestTestHarness: URI based RestTestHarness.head restored; different signature

RestTestHarness: URI based RestTestHarness.head restored; different signature Fixed a few bugs

dsmiley · 2026-04-07T14:56:23Z

I made a significant change to make RestTestHarness cheap to create without any need to close it, and also making it immutable.

I think I should split this big PR, a new one centered on RestTestHarness and most of its users and thus also including the URLUtils.buildURI addition. WDYT @epugh ? I could do that ~quickly and then we come back to this PR.

dsmiley · 2026-04-07T21:47:58Z

RestTestHarness & URLUtils stuff is now reverted from this PR; see #4270 for them.

Now this PR is smaller, albeit still pretty big.

epugh · 2026-04-08T08:19:26Z

I think that was a good decision. I will review and run the tests locally

dsmiley added 2 commits April 5, 2026 12:14

Scope: don't use Apache HttpClient directly; use of it via HttpSolrCl…

0989327

…ient etc. is still allowed. Don't import org.apache.http classes. Mostly done via Google Jules.

Finish-ish.

00e89df

Introduce URLUtil.buildURI

dsmiley requested a review from epugh April 6, 2026 05:45

dsmiley added the no-changelog label Apr 6, 2026

github-actions bot added dependencies Dependency upgrades test-framework jetty-server client:solrj module:jwt-auth tool:build tests cat:search cat:cloud cat:schema cat:security cat:packagemanager cat:metrics labels Apr 6, 2026

epugh reviewed Apr 6, 2026

View reviewed changes

dsmiley and others added 5 commits April 6, 2026 09:37

Use clearer GET(uri) method

09f3bfb

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Use convenient POST method

ed8b57b

more HttpMethod use

3f37f1c

dedupe/simplify doHttpPost/doHttpPostWithHeader

5c7bf2e

pass headers better

548abfd

dsmiley commented Apr 6, 2026

View reviewed changes

dsmiley added 6 commits April 6, 2026 13:14

reverting some things

40b9991

optimize buildURI

e77d8db

Merge remote-tracking branch 'refs/remotes/apache/main' into SOLR-181…

0dd173a

…88-migrateToJettyHttpClient # Conflicts: # solr/solrj/src/test/org/apache/solr/client/solrj/embedded/JettyWebappTest.java

fix merge

2683172

enhanced switch statements.

ad3f730

Didn't need version suffixes on vars since they are now scoped.

CreateRoutedAliasTest: align comments

11ee2f2

dsmiley added 7 commits April 6, 2026 16:52

multi-line string

3eee082

callMigrateReplicas var naming

c6b32c5

RestTestHarness:

cd72b3f

* better exception handling. * allow passing an absolute URL thus overriding the base

define logger correctly

28d1faf

@SuppressWarnings("try")

30597c6

Merge remote-tracking branch 'apache/main' into SOLR-18188-migrateToJ…

2b02520

…ettyHttpClient # Conflicts: # solr/test-framework/build.gradle

github-actions bot added the module:ltr label Apr 7, 2026

dsmiley added 2 commits April 7, 2026 09:45

JettySolrRunner.getRestClient & getBaseUri.

8ae9852

RestTestHarness: URI based RestTestHarness.head restored; different signature

JettySolrRunner.getRestClient & getBaseUri.

49fbdcb

RestTestHarness: URI based RestTestHarness.head restored; different signature Fixed a few bugs

head() should return the status code

b96cfe1

dsmiley mentioned this pull request Apr 7, 2026

SOLR-18188: RestTestHarness overhaul, add URLUtil.buildURI #4270

Open

Revert changes split to another PR

ad62248

github-actions bot removed the module:ltr label Apr 7, 2026


		public class AliasIntegrationTest extends SolrCloudTestCase {

		private CloseableHttpClient httpClient;

Conversation

dsmiley commented Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epugh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dsmiley commented Apr 7, 2026

Uh oh!

dsmiley commented Apr 7, 2026

Uh oh!

epugh commented Apr 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dsmiley commented Apr 6, 2026 •

edited

Loading