Skip to content

Commit aafdb1b

Browse files
andywebb1975dsmiley
andywebb1975
authored and
dsmiley
committed
SOLR-17263: HttpJdkSolrClient doesn't encode curly braces etc (apache#2433)
(cherry picked from commit 4c439d0)
1 parent 08e6054 commit aafdb1b

File tree

5 files changed

+36
-21
lines changed

5 files changed

+36
-21
lines changed

‎solr/CHANGES.txt

+2
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ Bug Fixes
1212
* SOLR-17049: Actually mark all replicas down at startup and truly wait for them.
1313
This includes replicas that might not exist anymore locally. (Houston Putman, Vincent Primault)
1414

15+
* SOLR-17263: Fix 'Illegal character in query' exception in HttpJdkSolrClient (Andy Webb)
16+
1517
Dependency Upgrades
1618
---------------------
1719
(No changes)

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

+5-3
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,7 @@ private PreparedRequest prepareGet(
260260
validateGetRequest(solrRequest);
261261
reqb.GET();
262262
decorateRequest(reqb, solrRequest);
263-
reqb.uri(new URI(url + "?" + queryParams));
263+
reqb.uri(new URI(url + queryParams.toQueryString()));
264264
return new PreparedRequest(reqb, null);
265265
}
266266

@@ -324,7 +324,9 @@ private PreparedRequest preparePutOrPost(
324324
ModifiableSolrParams requestParams = queryParams;
325325
queryParams = calculateQueryParams(urlParamNames, requestParams);
326326
queryParams.add(calculateQueryParams(solrRequest.getQueryParams(), requestParams));
327-
bodyPublisher = HttpRequest.BodyPublishers.ofString(requestParams.toString());
327+
// note the toQueryString() method adds a leading question mark which needs to be removed here
328+
bodyPublisher =
329+
HttpRequest.BodyPublishers.ofString(requestParams.toQueryString().substring(1));
328330
} else {
329331
bodyPublisher = HttpRequest.BodyPublishers.noBody();
330332
}
@@ -335,7 +337,7 @@ private PreparedRequest preparePutOrPost(
335337
} else {
336338
reqb.method("POST", bodyPublisher);
337339
}
338-
URI uriWithQueryParams = new URI(url + "?" + queryParams);
340+
URI uriWithQueryParams = new URI(url + queryParams.toQueryString());
339341
reqb.uri(uriWithQueryParams);
340342

341343
return new PreparedRequest(reqb, contentWritingFuture);

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

+4-4
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ protected void testQuerySetup(SolrRequest.METHOD method, ResponseParser rp) thro
130130
DebugServlet.clear();
131131
String url = getBaseUrl() + DEBUG_SERVLET_PATH;
132132
SolrQuery q = new SolrQuery("foo");
133-
q.setParam("a", "\u1234");
133+
q.setParam("a", MUST_ENCODE);
134134
Http2SolrClient.Builder b =
135135
new Http2SolrClient.Builder(url).withDefaultCollection(DEFAULT_CORE);
136136
if (rp != null) {
@@ -233,7 +233,7 @@ public void testUpdateDefault() throws Exception {
233233
String url = getBaseUrl() + DEBUG_SERVLET_PATH;
234234
try (Http2SolrClient client =
235235
new Http2SolrClient.Builder(url).withDefaultCollection(DEFAULT_CORE).build()) {
236-
testUpdate(client, WT.JAVABIN, "application/javabin", "\u1234");
236+
testUpdate(client, WT.JAVABIN, "application/javabin", MUST_ENCODE);
237237
}
238238
}
239239

@@ -246,7 +246,7 @@ public void testUpdateXml() throws Exception {
246246
.withRequestWriter(new RequestWriter())
247247
.withResponseParser(new XMLResponseParser())
248248
.build()) {
249-
testUpdate(client, WT.XML, "application/xml; charset=UTF-8", "\u1234");
249+
testUpdate(client, WT.XML, "application/xml; charset=UTF-8", MUST_ENCODE);
250250
}
251251
}
252252

@@ -259,7 +259,7 @@ public void testUpdateJavabin() throws Exception {
259259
.withRequestWriter(new BinaryRequestWriter())
260260
.withResponseParser(new BinaryResponseParser())
261261
.build()) {
262-
testUpdate(client, WT.JAVABIN, "application/javabin", "\u1234");
262+
testUpdate(client, WT.JAVABIN, "application/javabin", MUST_ENCODE);
263263
}
264264
}
265265

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

+3-3
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ protected void testQuerySetup(SolrRequest.METHOD method, ResponseParser rp) thro
178178
}
179179
String url = getBaseUrl() + DEBUG_SERVLET_PATH;
180180
SolrQuery q = new SolrQuery("foo");
181-
q.setParam("a", "\u1234");
181+
q.setParam("a", MUST_ENCODE);
182182
HttpJdkSolrClient.Builder b = builder(url);
183183
if (rp != null) {
184184
b.withResponseParser(rp);
@@ -382,7 +382,7 @@ public void testSolrExceptionWithNullBaseurl() throws IOException, SolrServerExc
382382
public void testUpdateDefault() throws Exception {
383383
String url = getBaseUrl() + DEBUG_SERVLET_PATH;
384384
try (HttpJdkSolrClient client = builder(url).build()) {
385-
testUpdate(client, WT.JAVABIN, "application/javabin", "\u1234");
385+
testUpdate(client, WT.JAVABIN, "application/javabin", MUST_ENCODE);
386386
}
387387
}
388388

@@ -433,7 +433,7 @@ public void testUpdateJavabin() throws Exception {
433433
.withRequestWriter(new BinaryRequestWriter())
434434
.withResponseParser(new BinaryResponseParser())
435435
.build()) {
436-
testUpdate(client, WT.JAVABIN, "application/javabin", "\u1234");
436+
testUpdate(client, WT.JAVABIN, "application/javabin", MUST_ENCODE);
437437
assertNoHeadRequestWithSsl(client);
438438
}
439439
}

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

+22-11
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
import java.io.IOException;
2323
import java.io.InputStream;
24+
import java.net.URLEncoder;
2425
import java.nio.charset.StandardCharsets;
2526
import java.util.ArrayList;
2627
import java.util.Base64;
@@ -64,6 +65,8 @@ public abstract class HttpSolrClientTestBase extends SolrJettyTestBase {
6465
protected static final String REDIRECT_SERVLET_PATH = "/redirect";
6566
protected static final String REDIRECT_SERVLET_REGEX = REDIRECT_SERVLET_PATH + "/*";
6667
protected static final String COLLECTION_1 = "collection1";
68+
// example chars that must be URI encoded - non-ASCII and curly quote
69+
protected static final String MUST_ENCODE = "\u1234\u007B";
6770

6871
@BeforeClass
6972
public static void beforeTest() throws Exception {
@@ -113,7 +116,7 @@ public void testQueryGet() throws Exception {
113116
assertNull(DebugServlet.headers.get("content-type"));
114117
// param encoding
115118
assertEquals(1, DebugServlet.parameters.get("a").length);
116-
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
119+
assertEquals(MUST_ENCODE, DebugServlet.parameters.get("a")[0]);
117120
}
118121

119122
public void testQueryPost() throws Exception {
@@ -125,9 +128,11 @@ public void testQueryPost() throws Exception {
125128
assertEquals("javabin", DebugServlet.parameters.get(CommonParams.WT)[0]);
126129
assertEquals(1, DebugServlet.parameters.get(CommonParams.VERSION).length);
127130
assertEquals(1, DebugServlet.parameters.get("a").length);
128-
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
131+
assertEquals(MUST_ENCODE, DebugServlet.parameters.get("a")[0]);
129132
assertEquals(expectedUserAgent(), DebugServlet.headers.get("user-agent"));
130133
assertEquals("application/x-www-form-urlencoded", DebugServlet.headers.get("content-type"));
134+
// this validates that URI encoding has been applied - the content-length is smaller if not
135+
assertEquals("41", DebugServlet.headers.get("content-length"));
131136
}
132137

133138
public void testQueryPut() throws Exception {
@@ -139,9 +144,10 @@ public void testQueryPut() throws Exception {
139144
assertEquals("javabin", DebugServlet.parameters.get(CommonParams.WT)[0]);
140145
assertEquals(1, DebugServlet.parameters.get(CommonParams.VERSION).length);
141146
assertEquals(1, DebugServlet.parameters.get("a").length);
142-
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
147+
assertEquals(MUST_ENCODE, DebugServlet.parameters.get("a")[0]);
143148
assertEquals(expectedUserAgent(), DebugServlet.headers.get("user-agent"));
144149
assertEquals("application/x-www-form-urlencoded", DebugServlet.headers.get("content-type"));
150+
assertEquals("41", DebugServlet.headers.get("content-length"));
145151
}
146152

147153
public void testQueryXmlGet() throws Exception {
@@ -153,7 +159,7 @@ public void testQueryXmlGet() throws Exception {
153159
assertEquals("xml", DebugServlet.parameters.get(CommonParams.WT)[0]);
154160
assertEquals(1, DebugServlet.parameters.get(CommonParams.VERSION).length);
155161
assertEquals(1, DebugServlet.parameters.get("a").length);
156-
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
162+
assertEquals(MUST_ENCODE, DebugServlet.parameters.get("a")[0]);
157163
assertEquals(expectedUserAgent(), DebugServlet.headers.get("user-agent"));
158164
}
159165

@@ -166,7 +172,7 @@ public void testQueryXmlPost() throws Exception {
166172
assertEquals("xml", DebugServlet.parameters.get(CommonParams.WT)[0]);
167173
assertEquals(1, DebugServlet.parameters.get(CommonParams.VERSION).length);
168174
assertEquals(1, DebugServlet.parameters.get("a").length);
169-
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
175+
assertEquals(MUST_ENCODE, DebugServlet.parameters.get("a")[0]);
170176
assertEquals(expectedUserAgent(), DebugServlet.headers.get("user-agent"));
171177
assertEquals("application/x-www-form-urlencoded", DebugServlet.headers.get("content-type"));
172178
}
@@ -180,7 +186,7 @@ public void testQueryXmlPut() throws Exception {
180186
assertEquals("xml", DebugServlet.parameters.get(CommonParams.WT)[0]);
181187
assertEquals(1, DebugServlet.parameters.get(CommonParams.VERSION).length);
182188
assertEquals(1, DebugServlet.parameters.get("a").length);
183-
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
189+
assertEquals(MUST_ENCODE, DebugServlet.parameters.get("a")[0]);
184190
assertEquals(expectedUserAgent(), DebugServlet.headers.get("user-agent"));
185191
assertEquals("application/x-www-form-urlencoded", DebugServlet.headers.get("content-type"));
186192
}
@@ -284,7 +290,8 @@ protected void testUpdate(HttpSolrClientBase client, WT wt, String contentType,
284290
SolrInputDocument doc = new SolrInputDocument();
285291
doc.addField("id", docIdValue);
286292
req.add(doc);
287-
req.setParam("a", "\u1234");
293+
// non-ASCII characters and curly quotes should be URI-encoded
294+
req.setParam("a", MUST_ENCODE);
288295

289296
try {
290297
client.request(req);
@@ -301,7 +308,7 @@ protected void testUpdate(HttpSolrClientBase client, WT wt, String contentType,
301308
client.getParser().getVersion(), DebugServlet.parameters.get(CommonParams.VERSION)[0]);
302309
assertEquals(contentType, DebugServlet.headers.get("content-type"));
303310
assertEquals(1, DebugServlet.parameters.get("a").length);
304-
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
311+
assertEquals(MUST_ENCODE, DebugServlet.parameters.get("a")[0]);
305312

306313
if (wt == WT.XML) {
307314
String requestBody = new String(DebugServlet.requestBody, StandardCharsets.UTF_8);
@@ -338,12 +345,14 @@ protected void testCollectionParameters(
338345
protected void setReqParamsOf(UpdateRequest req, String... keys) {
339346
if (keys != null) {
340347
for (String k : keys) {
341-
req.setParam(k, k + "Value");
348+
// note inclusion of non-ASCII character, and curly quotes which should be URI encoded
349+
req.setParam(k, k + "Value" + MUST_ENCODE);
342350
}
343351
}
344352
}
345353

346-
protected void verifyServletState(HttpSolrClientBase client, SolrRequest<?> request) {
354+
protected void verifyServletState(HttpSolrClientBase client, SolrRequest<?> request)
355+
throws Exception {
347356
// check query String
348357
Iterator<String> paramNames = request.getParams().getParameterNamesIterator();
349358
while (paramNames.hasNext()) {
@@ -355,7 +364,9 @@ protected void verifyServletState(HttpSolrClientBase client, SolrRequest<?> requ
355364
client.getUrlParamNames().contains(name)
356365
|| (request.getQueryParams() != null && request.getQueryParams().contains(name));
357366
assertEquals(
358-
shouldBeInQueryString, DebugServlet.queryString.contains(name + "=" + value));
367+
shouldBeInQueryString,
368+
DebugServlet.queryString.contains(
369+
name + "=" + URLEncoder.encode(value, StandardCharsets.UTF_8.name())));
359370
// in either case, it should be in the parameters
360371
assertNotNull(DebugServlet.parameters.get(name));
361372
assertEquals(1, DebugServlet.parameters.get(name).length);

0 commit comments

Comments
 (0)