From 5af3e69dc48d05dbbceb1e4e12a4cd49b30131a9 Mon Sep 17 00:00:00 2001 From: Andy Webb Date: Fri, 10 May 2024 15:35:16 +0100 Subject: [PATCH 1/5] Update tests to include curly quote --- .../solrj/impl/Http2SolrClientTest.java | 8 ++--- .../solrj/impl/HttpJdkSolrClientTest.java | 6 ++-- .../solrj/impl/HttpSolrClientTestBase.java | 33 ++++++++++++------- 3 files changed, 29 insertions(+), 18 deletions(-) diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java index b6642955c4a..384908a0547 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java +++ b/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); } } diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java index 789ff090dd1..9821a8ec849 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java +++ b/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); } } diff --git a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java b/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java index 2c8caec0fdf..ec14e871e67 100644 --- a/solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpSolrClientTestBase.java +++ b/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 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); From 493a32920da2fa5084d7f98990d290bc485411c7 Mon Sep 17 00:00:00 2001 From: Andy Webb Date: Fri, 10 May 2024 15:26:12 +0100 Subject: [PATCH 2/5] Use toQueryString() for GET --- .../org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java index 034e16ac679..99e920a1074 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java +++ b/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); } From e53af16bd85f1fdae84cc7b942b05f6b14406b2c Mon Sep 17 00:00:00 2001 From: Andy Webb Date: Fri, 10 May 2024 15:26:31 +0100 Subject: [PATCH 3/5] Use toQueryString() for PUT/POST --- .../org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java index 99e920a1074..e3c40118d62 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java @@ -313,7 +313,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); From 987cb92f466923bb8fdd86459873befabbe5d060 Mon Sep 17 00:00:00 2001 From: Andy Webb Date: Fri, 10 May 2024 16:04:05 +0100 Subject: [PATCH 4/5] Use toQueryString() for PUT/POST body --- .../org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java index e3c40118d62..8fe0697878b 100644 --- a/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java +++ b/solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java @@ -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(); } From 928e1f424b06a290a15110bf374d3165983eddd0 Mon Sep 17 00:00:00 2001 From: Andy Webb Date: Sat, 11 May 2024 16:53:22 +0100 Subject: [PATCH 5/5] Update CHANGES.txt --- solr/CHANGES.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 30d340d11a4..02d1a40ac42 100644 --- a/solr/CHANGES.txt +++ b/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)