SOLR-17263: HttpJdkSolrClient doesn't encode curly braces etc

solr/CHANGES.txt

@@ -130,6 +130,8 @@ Bug Fixes
 * SOLR-17049: Actually mark all replicas down at startup and truly wait for them.
   This includes replicas that might not exist anymore locally. (Houston Putman, Vincent Primault)
 
+* SOLR-17263: Fix 'Illegal character in query' exception in HttpJdkSolrClient (Andy Webb)
+
 Dependency Upgrades
 ---------------------
 (No changes)

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

@@ -238,7 +238,7 @@ private PreparedRequest prepareGet(
     validateGetRequest(solrRequest);
     reqb.GET();
     decorateRequest(reqb, solrRequest);
-    reqb.uri(new URI(url + "?" + queryParams));
+    reqb.uri(new URI(url + queryParams.toQueryString()));
     return new PreparedRequest(reqb, null);
   }
 
@@ -302,7 +302,9 @@ private PreparedRequest preparePutOrPost(
       ModifiableSolrParams requestParams = queryParams;
       queryParams = calculateQueryParams(urlParamNames, requestParams);
       queryParams.add(calculateQueryParams(solrRequest.getQueryParams(), requestParams));
-      bodyPublisher = HttpRequest.BodyPublishers.ofString(requestParams.toString());
+      // note the toQueryString() method adds a leading question mark which needs to be removed here
+      bodyPublisher =
+          HttpRequest.BodyPublishers.ofString(requestParams.toQueryString().substring(1));
     } else {
       bodyPublisher = HttpRequest.BodyPublishers.noBody();
     }
@@ -313,7 +315,7 @@ private PreparedRequest preparePutOrPost(
     } else {
       reqb.method("POST", bodyPublisher);
     }
-    URI uriWithQueryParams = new URI(url + "?" + queryParams);
+    URI uriWithQueryParams = new URI(url + queryParams.toQueryString());
     reqb.uri(uriWithQueryParams);
 
     return new PreparedRequest(reqb, contentWritingFuture);

solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java

@@ -130,7 +130,7 @@ protected void testQuerySetup(SolrRequest.METHOD method, ResponseParser rp) thro
     DebugServlet.clear();
     String url = getBaseUrl() + DEBUG_SERVLET_PATH;
     SolrQuery q = new SolrQuery("foo");
-    q.setParam("a", "\u1234");
+    q.setParam("a", MUST_ENCODE);
     Http2SolrClient.Builder b =
         new Http2SolrClient.Builder(url).withDefaultCollection(DEFAULT_CORE);
     if (rp != null) {
@@ -233,7 +233,7 @@ public void testUpdateDefault() throws Exception {
     String url = getBaseUrl() + DEBUG_SERVLET_PATH;
     try (Http2SolrClient client =
         new Http2SolrClient.Builder(url).withDefaultCollection(DEFAULT_CORE).build()) {
-      testUpdate(client, WT.JAVABIN, "application/javabin", "\u1234");
+      testUpdate(client, WT.JAVABIN, "application/javabin", MUST_ENCODE);
     }
   }
 
@@ -246,7 +246,7 @@ public void testUpdateXml() throws Exception {
             .withRequestWriter(new RequestWriter())
             .withResponseParser(new XMLResponseParser())
             .build()) {
-      testUpdate(client, WT.XML, "application/xml; charset=UTF-8", "\u1234");
+      testUpdate(client, WT.XML, "application/xml; charset=UTF-8", MUST_ENCODE);
     }
   }
 
@@ -259,7 +259,7 @@ public void testUpdateJavabin() throws Exception {
             .withRequestWriter(new BinaryRequestWriter())
             .withResponseParser(new BinaryResponseParser())
             .build()) {
-      testUpdate(client, WT.JAVABIN, "application/javabin", "\u1234");
+      testUpdate(client, WT.JAVABIN, "application/javabin", MUST_ENCODE);
     }
   }
 

solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java

@@ -172,7 +172,7 @@ protected void testQuerySetup(SolrRequest.METHOD method, ResponseParser rp) thro
     }
     String url = getBaseUrl() + DEBUG_SERVLET_PATH;
     SolrQuery q = new SolrQuery("foo");
-    q.setParam("a", "\u1234");
+    q.setParam("a", MUST_ENCODE);
     HttpJdkSolrClient.Builder b = builder(url);
     if (rp != null) {
       b.withResponseParser(rp);
@@ -313,7 +313,7 @@ public void testSolrExceptionWithNullBaseurl() throws IOException, SolrServerExc
   public void testUpdateDefault() throws Exception {
     String url = getBaseUrl() + DEBUG_SERVLET_PATH;
     try (HttpJdkSolrClient client = builder(url).build()) {
-      testUpdate(client, WT.JAVABIN, "application/javabin", "\u1234");
+      testUpdate(client, WT.JAVABIN, "application/javabin", MUST_ENCODE);
     }
   }
 
@@ -364,7 +364,7 @@ public void testUpdateJavabin() throws Exception {
             .withRequestWriter(new BinaryRequestWriter())
             .withResponseParser(new BinaryResponseParser())
             .build()) {
-      testUpdate(client, WT.JAVABIN, "application/javabin", "\u1234");
+      testUpdate(client, WT.JAVABIN, "application/javabin", MUST_ENCODE);
       assertNoHeadRequestWithSsl(client);
     }
   }

solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java

@@ -21,6 +21,7 @@
 
 import java.io.IOException;
 import java.io.InputStream;
+import java.net.URLEncoder;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.Base64;
@@ -63,6 +64,8 @@ public abstract class HttpSolrClientTestBase extends SolrJettyTestBase {
   protected static final String REDIRECT_SERVLET_PATH = "/redirect";
   protected static final String REDIRECT_SERVLET_REGEX = REDIRECT_SERVLET_PATH + "/*";
   protected static final String COLLECTION_1 = "collection1";
+  // example chars that must be URI encoded - non-ASCII and curly quote
+  protected static final String MUST_ENCODE = "\u1234\u007B";
 
   @BeforeClass
   public static void beforeTest() throws Exception {
@@ -112,7 +115,7 @@ public void testQueryGet() throws Exception {
     assertNull(DebugServlet.headers.get("content-type"));
     // param encoding
     assertEquals(1, DebugServlet.parameters.get("a").length);
-    assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
+    assertEquals(MUST_ENCODE, DebugServlet.parameters.get("a")[0]);
   }
 
   public void testQueryPost() throws Exception {
@@ -124,9 +127,11 @@ public void testQueryPost() throws Exception {
     assertEquals("javabin", DebugServlet.parameters.get(CommonParams.WT)[0]);
     assertEquals(1, DebugServlet.parameters.get(CommonParams.VERSION).length);
     assertEquals(1, DebugServlet.parameters.get("a").length);
-    assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
+    assertEquals(MUST_ENCODE, DebugServlet.parameters.get("a")[0]);
     assertEquals(expectedUserAgent(), DebugServlet.headers.get("user-agent"));
     assertEquals("application/x-www-form-urlencoded", DebugServlet.headers.get("content-type"));
+    // this validates that URI encoding has been applied - the content-length is smaller if not
+    assertEquals("41", DebugServlet.headers.get("content-length"));
   }
 
   public void testQueryPut() throws Exception {
@@ -138,9 +143,10 @@ public void testQueryPut() throws Exception {
     assertEquals("javabin", DebugServlet.parameters.get(CommonParams.WT)[0]);
     assertEquals(1, DebugServlet.parameters.get(CommonParams.VERSION).length);
     assertEquals(1, DebugServlet.parameters.get("a").length);
-    assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
+    assertEquals(MUST_ENCODE, DebugServlet.parameters.get("a")[0]);
     assertEquals(expectedUserAgent(), DebugServlet.headers.get("user-agent"));
     assertEquals("application/x-www-form-urlencoded", DebugServlet.headers.get("content-type"));
+    assertEquals("41", DebugServlet.headers.get("content-length"));
   }
 
   public void testQueryXmlGet() throws Exception {
@@ -152,7 +158,7 @@ public void testQueryXmlGet() throws Exception {
     assertEquals("xml", DebugServlet.parameters.get(CommonParams.WT)[0]);
     assertEquals(1, DebugServlet.parameters.get(CommonParams.VERSION).length);
     assertEquals(1, DebugServlet.parameters.get("a").length);
-    assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
+    assertEquals(MUST_ENCODE, DebugServlet.parameters.get("a")[0]);
     assertEquals(expectedUserAgent(), DebugServlet.headers.get("user-agent"));
   }
 
@@ -165,7 +171,7 @@ public void testQueryXmlPost() throws Exception {
     assertEquals("xml", DebugServlet.parameters.get(CommonParams.WT)[0]);
     assertEquals(1, DebugServlet.parameters.get(CommonParams.VERSION).length);
     assertEquals(1, DebugServlet.parameters.get("a").length);
-    assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
+    assertEquals(MUST_ENCODE, DebugServlet.parameters.get("a")[0]);
     assertEquals(expectedUserAgent(), DebugServlet.headers.get("user-agent"));
     assertEquals("application/x-www-form-urlencoded", DebugServlet.headers.get("content-type"));
   }
@@ -179,7 +185,7 @@ public void testQueryXmlPut() throws Exception {
     assertEquals("xml", DebugServlet.parameters.get(CommonParams.WT)[0]);
     assertEquals(1, DebugServlet.parameters.get(CommonParams.VERSION).length);
     assertEquals(1, DebugServlet.parameters.get("a").length);
-    assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
+    assertEquals(MUST_ENCODE, DebugServlet.parameters.get("a")[0]);
     assertEquals(expectedUserAgent(), DebugServlet.headers.get("user-agent"));
     assertEquals("application/x-www-form-urlencoded", DebugServlet.headers.get("content-type"));
   }
@@ -283,7 +289,8 @@ protected void testUpdate(HttpSolrClientBase client, WT wt, String contentType,
     SolrInputDocument doc = new SolrInputDocument();
     doc.addField("id", docIdValue);
     req.add(doc);
-    req.setParam("a", "\u1234");
+    // non-ASCII characters and curly quotes should be URI-encoded
+    req.setParam("a", MUST_ENCODE);
 
     try {
       client.request(req);
@@ -300,7 +307,7 @@ protected void testUpdate(HttpSolrClientBase client, WT wt, String contentType,
         client.getParser().getVersion(), DebugServlet.parameters.get(CommonParams.VERSION)[0]);
     assertEquals(contentType, DebugServlet.headers.get("content-type"));
     assertEquals(1, DebugServlet.parameters.get("a").length);
-    assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
+    assertEquals(MUST_ENCODE, DebugServlet.parameters.get("a")[0]);
 
     if (wt == WT.XML) {
       String requestBody = new String(DebugServlet.requestBody, StandardCharsets.UTF_8);
@@ -337,12 +344,14 @@ protected void testCollectionParameters(
   protected void setReqParamsOf(UpdateRequest req, String... keys) {
     if (keys != null) {
       for (String k : keys) {
-        req.setParam(k, k + "Value");
+        // note inclusion of non-ASCII character, and curly quotes which should be URI encoded
+        req.setParam(k, k + "Value" + MUST_ENCODE);
       }
     }
   }
 
-  protected void verifyServletState(HttpSolrClientBase client, SolrRequest<?> request) {
+  protected void verifyServletState(HttpSolrClientBase client, SolrRequest<?> request)
+      throws Exception {
     // check query String
     Iterator<String> paramNames = request.getParams().getParameterNamesIterator();
     while (paramNames.hasNext()) {
@@ -354,7 +363,9 @@ protected void verifyServletState(HttpSolrClientBase client, SolrRequest<?> requ
               client.getUrlParamNames().contains(name)
                   || (request.getQueryParams() != null && request.getQueryParams().contains(name));
           assertEquals(
-              shouldBeInQueryString, DebugServlet.queryString.contains(name + "=" + value));
+              shouldBeInQueryString,
+              DebugServlet.queryString.contains(
+                  name + "=" + URLEncoder.encode(value, StandardCharsets.UTF_8.name())));
           // in either case, it should be in the parameters
           assertNotNull(DebugServlet.parameters.get(name));
           assertEquals(1, DebugServlet.parameters.get(name).length);