abhishekrb19 commented on code in PR #20308:
URL: https://github.com/apache/druid/pull/20308#discussion_r3974932626
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -160,7 +160,7 @@ public Sequence<T> run(final QueryPlus<T> queryPlus, final
ResponseContext conte
final ListenableFuture<InputStream> future;
final String url = scheme + "://" + host + "/druid/v2/";
- final String cancelUrl = url + query.getId();
+ final String cancelUrl = url + StringUtils.urlEncode(query.getId());
Review Comment:
Here's a unit test in `DirectDruidClientTest` that fails without this
encoding fix:
```java
@Test
public void testCancelUrlEncodesQueryId()
{
// A queryId with path-unsafe characters.
final String queryId = "some query/id#needing?encoding";
QueryPlus queryPlus = QueryPlus.wrap(
Druids.newTimeBoundaryQueryBuilder()
.dataSource("test")
.context(Map.of(
DirectDruidClient.QUERY_FAIL_TIME, Long.MAX_VALUE,
BaseQuery.QUERY_ID, queryId
))
.build()
);
QueuedTestHttpClient queuedHttpClient = new QueuedTestHttpClient();
// First response: for the main POST query request. Cancelled, so
DirectDruidClient reacts by
// issuing a cancel request.
queuedHttpClient.enqueue(Futures.immediateCancelledFuture());
// Second response: for the DELETE cancel request itself.
queuedHttpClient.enqueue(Futures.immediateCancelledFuture());
DirectDruidClient client = makeDirectDruidClient(queuedHttpClient);
Sequence results = client.run(queryPlus, responseContext);
Assertions.assertEquals(0, client.getNumOpenConnections());
QueryInterruptedException actualException =
Assertions.assertThrows(QueryInterruptedException.class, () ->
results.toList());
Assertions.assertEquals(hostName, actualException.getHost());
Assertions.assertEquals("Query cancelled",
actualException.getErrorCode());
Assertions.assertEquals("Task was cancelled.",
actualException.getCause().getMessage());
Assertions.assertTrue(blockingExecutorService.hasPendingTasks());
blockingExecutorService.finishNextPendingTask();
Assertions.assertTrue(blockingExecutorService.hasPendingTasks());
ISE observedException = Assertions.assertThrows(ISE.class, () ->
blockingExecutorService.finishNextPendingTask());
Assertions.assertTrue(observedException.getCause() instanceof
CancellationException);
// Verify the actual outgoing cancel request.
List<Request> requests = queuedHttpClient.getRequests();
Assertions.assertEquals(2, requests.size());
Request cancelRequest = requests.get(1);
Assertions.assertEquals(HttpMethod.DELETE, cancelRequest.getMethod());
Assertions.assertEquals(
"/druid/v2/" + queryId,
URLDecoder.decode(cancelRequest.getUrl().getPath(),
StandardCharsets.UTF_8)
);
}
```
--
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]
