gianm commented on code in PR #17937:
URL: https://github.com/apache/druid/pull/17937#discussion_r2064988285
##########
docs/api-reference/sql-api.md:
##########
@@ -99,6 +103,23 @@ The request body takes the following properties:
}
```
+##### Text Format Request body
+
+Druid also allows you to submit SQL queries in text format which is simpler
than above JSON format.
+To do this, just set the `Content-Type` request header to `text/plain` or
`application/x-www-form-urlencoded`, and pass SQL via the HTTP Body.
Review Comment:
Should mention that for `application/x-www-form-urlencoded`, the payload the
user sends must be url-encoded.
##########
docs/api-reference/sql-api.md:
##########
@@ -99,6 +103,23 @@ The request body takes the following properties:
}
```
+##### Text Format Request body
+
+Druid also allows you to submit SQL queries in text format which is simpler
than above JSON format.
+To do this, just set the `Content-Type` request header to `text/plain` or
`application/x-www-form-urlencoded`, and pass SQL via the HTTP Body.
+
+If there are multiple `Content-Type` headers, the **first** one is used.
+
+For response, the `resultFormat` is always `object` with the HTTP response
header `Content-Type: application/json`.
+If you want more control over the query context or response format, use the
above JSON format request body instead.
+
+For example:
+
+```commandline
+echo 'SELECT 1' | curl http://ROUTER_IP:ROUTER_PORT/druid/v2/sql --data @-
Review Comment:
This example line I think needs to be `--data-urlencode`. A query with more
special characters would demonstrate it. This command returns an error
`URLDecoder: Illegal hex characters in escape (%) pattern`:
```
echo "SELECT 'x % y'" | curl http://localhost:8082/druid/v2/sql/ --data @-
```
This one runs fine:
```
echo "SELECT 'x % y'" | curl http://localhost:8082/druid/v2/sql/
--data-urlencode @-
```
##########
integration-tests/src/test/java/org/apache/druid/tests/query/ITSqlQueryTest.java:
##########
@@ -0,0 +1,259 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.tests.query;
+
+import com.google.inject.Inject;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.testing.IntegrationTestingConfig;
+import org.apache.druid.testing.guice.DruidTestModuleFactory;
+import org.apache.druid.tests.TestNGGroup;
+import org.apache.http.HttpEntity;
+import org.apache.http.HttpStatus;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpPost;
+import org.apache.http.entity.StringEntity;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClientBuilder;
+import org.apache.http.util.EntityUtils;
+import org.testng.annotations.Guice;
+import org.testng.annotations.Test;
+
+import java.io.IOException;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+
+/**
+ * Test the SQL endpoint with different Content-Type
+ */
+@Test(groups = {TestNGGroup.QUERY, TestNGGroup.CENTRALIZED_DATASOURCE_SCHEMA})
+@Guice(moduleFactory = DruidTestModuleFactory.class)
+public class ITSqlQueryTest
+{
+ private static final Logger LOG = new Logger(ITSqlQueryTest.class);
+
+ @Inject
+ IntegrationTestingConfig config;
+
+ interface IExecutable
+ {
+ void execute(String endpoint) throws IOException;
+ }
+
+ private void executeWithRetry(String contentType, IExecutable executable)
+ {
+ executeWithRetry(StringUtils.format("%s/druid/v2/sql/",
config.getBrokerUrl()), contentType, executable);
+ executeWithRetry(StringUtils.format("%s/druid/v2/sql/",
config.getRouterUrl()), contentType, executable);
+ }
+
+ private void executeWithRetry(String endpoint, String contentType,
IExecutable executable)
+ {
+ Throwable lastException = null;
+ for (int i = 1; i <= 5; i++) {
+ LOG.info("Query to %s with Content-Type = %s, tries = %s", endpoint,
contentType, i);
+ try {
+ executable.execute(endpoint);
+ return;
+ }
+ catch (IOException e) {
+ // Only catch IOException
+ lastException = e;
+ }
+ try {
+ Thread.sleep(200);
+ }
+ catch (InterruptedException ignored) {
+ break;
+ }
+ }
+ throw new ISE(contentType + " failed after 5 tries, last exception: " +
lastException);
+ }
+
+ @Test
+ public void testUnsupportedMediaType()
+ {
+ executeWithRetry(
+ "<EMPTY>",
+ (endpoint) -> {
+ try (CloseableHttpClient client =
HttpClientBuilder.create().build()) {
+ HttpPost request = new HttpPost(endpoint);
+ request.addHeader("Content-Type", "application/xml");
+ request.setEntity(new StringEntity("select 1"));
+
+ try (CloseableHttpResponse response = client.execute(request)) {
+ assertEquals(HttpStatus.SC_UNSUPPORTED_MEDIA_TYPE,
response.getStatusLine().getStatusCode());
+ }
+ }
+ }
+ );
+ }
+
+ @Test
+ public void testTextPlain()
+ {
+ executeWithRetry(
+ "text/plain",
+ (endpoint) -> {
+ try (CloseableHttpClient client =
HttpClientBuilder.create().build()) {
+ HttpPost request = new HttpPost(endpoint);
+ request.addHeader("Content-Type", "text/plain");
+ request.setEntity(new StringEntity("select 1"));
+
+ try (CloseableHttpResponse response = client.execute(request)) {
+ assertEquals(200, response.getStatusLine().getStatusCode());
+
+ HttpEntity entity = response.getEntity();
+ assertNotNull(entity);
+ String responseBody = EntityUtils.toString(entity).trim();
+ assertEquals("[{\"EXPR$0\":1}]", responseBody);
+ }
+ }
+ }
+ );
+ }
+
+ @Test
+ public void testFormURLEncoded()
+ {
+ executeWithRetry(
+ "application/x-www-form-urlencoded",
+ (endpoint) -> {
+ try (CloseableHttpClient client =
HttpClientBuilder.create().build()) {
+ HttpPost request = new HttpPost(endpoint);
+ request.addHeader("Content-Type",
"application/x-www-form-urlencoded");
Review Comment:
For this test please use something that must be urldecoded. For example
`SELECT%20%31` would decode to `SELECT 1`.
##########
sql/src/main/java/org/apache/druid/sql/http/SqlQuery.java:
##########
@@ -200,4 +211,94 @@ public SqlQuery withQueryContext(Map<String, Object>
newContext)
{
return new SqlQuery(query, resultFormat, header, typesHeader,
sqlTypesHeader, newContext, parameters);
}
+
+ /**
+ * Extract SQL query object or SQL text from an HTTP Request
+ */
+ @FunctionalInterface
+ interface ISqlQueryExtractor<T>
+ {
+ T extract() throws IOException;
+ }
+
+ /**
+ * For BROKERs to use.
+ * <p>
+ * Brokers use com.sun.jersey upon Jetty for RESTful API, however jersey
internally has special handling for x-www-form-urlencoded,
+ * it's not able to get the data from the stream of HttpServletRequest for
such content type.
+ * So we use HttpContext to get the request entity/string instead of using
HttpServletRequest.
+ *
+ * @throws HttpException if the content type is not supported
+ * @throws BadRequestException if the SQL query is malformed or fail to read
from the request
+ */
+ public static SqlQuery from(HttpContext httpContext)
+ {
+ MediaType mediaType = httpContext.getRequest().getMediaType();
+ if (mediaType == null) {
+ throw new HttpException(
+ Response.Status.UNSUPPORTED_MEDIA_TYPE,
+ "Unsupported Content-Type: null"
+ );
+ }
+
+ try {
Review Comment:
This `try` is missing from the other `from` method, and should be broader
(so it can catch `IllegalArgumentException` from failed url decoding, for
example). Currently, some parsing errors in some cases can fall through as
unfriendly errors.
Suggestion on how to make the error handling more friendly:
- move the `try` / `catch` from this method to the `private static` version
of `from`, so both `public static` versions of `from` can benefit.
- have the `private static` version of `from` catch all exceptions from
parsing and decoding, and re-throw them as `BadRequestException`.
- modify `AsyncQueryForwardingServlet` so the call to `SqlQuery.from` is
wrapped in its own `try` / `catch`, which catches any `Exception`, and sends
that `Exception` to `handleQueryParseException`. This is needed because parse
exceptions would no longer be `IOException`s.
--
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]
