kfaraz commented on code in PR #18752: URL: https://github.com/apache/druid/pull/18752#discussion_r2540430094
########## processing/src/main/java/org/apache/druid/query/http/ClientSqlQuery.java: ########## @@ -39,6 +39,9 @@ public class ClientSqlQuery @JsonProperty private final String resultFormat; + /** + * Indicates whether the query result should include a header. + */ Review Comment: Alternative to this, we may also just rename the getter method to `includeHeader` similar to how it is done in class `SqlQuery` (which is the Broker-side equivalent of this POJO). ########## embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/ITSqlQueryTest.java: ########## @@ -0,0 +1,379 @@ +/* + * 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.testing.embedded.query; + +import org.apache.druid.java.util.common.ISE; +import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.java.util.http.client.HttpClient; +import org.apache.druid.java.util.http.client.Request; +import org.apache.druid.java.util.http.client.response.StatusResponseHandler; +import org.apache.druid.java.util.http.client.response.StatusResponseHolder; +import org.apache.druid.server.DruidNode; +import org.apache.druid.testing.embedded.EmbeddedBroker; +import org.apache.druid.testing.embedded.EmbeddedCoordinator; +import org.apache.druid.testing.embedded.EmbeddedDruidCluster; +import org.apache.druid.testing.embedded.EmbeddedDruidServer; +import org.apache.druid.testing.embedded.EmbeddedIndexer; +import org.apache.druid.testing.embedded.EmbeddedOverlord; +import org.apache.druid.testing.embedded.EmbeddedRouter; +import org.apache.druid.testing.embedded.junit5.EmbeddedClusterTestBase; +import org.jboss.netty.handler.codec.http.HttpMethod; +import org.jboss.netty.handler.codec.http.HttpResponseStatus; +import org.junit.Assert; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.FieldSource; + +import javax.ws.rs.core.MediaType; +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.function.Function; + +public class ITSqlQueryTest extends EmbeddedClusterTestBase +{ + private static final String SQL_QUERY_ROUTE = "%s/druid/v2/sql/"; + + public static List<Boolean> flags = List.of(true, false); + + private final EmbeddedBroker broker = new EmbeddedBroker(); + private final EmbeddedRouter router = new EmbeddedRouter(); + + private HttpClient httpClientRef; + private String brokerEndpoint; + private String routerEndpoint; + + interface OnRequest + { + void on(Request request); + } + + interface OnResponse + { + void on(int statusCode, String response); + } + + @Override + protected EmbeddedDruidCluster createCluster() + { + return EmbeddedDruidCluster.withEmbeddedDerbyAndZookeeper() + .useLatchableEmitter() + .addServer(new EmbeddedOverlord()) + .addServer(new EmbeddedCoordinator()) + .addServer(broker) + .addServer(router) + .addServer(new EmbeddedIndexer()); + } + + @BeforeEach + void setUp() + { + httpClientRef = router.bindings().globalHttpClient(); + brokerEndpoint = String.format(SQL_QUERY_ROUTE, getServerUrl(broker)); Review Comment: This variant of `String.format` has been marked as a forbidden API, I think since it does not explicitly specify a locale. ```suggestion brokerEndpoint = StringUtils.format(SQL_QUERY_ROUTE, getServerUrl(broker)); ``` ########## embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/ITSqlQueryTest.java: ########## @@ -0,0 +1,379 @@ +/* + * 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.testing.embedded.query; + +import org.apache.druid.java.util.common.ISE; +import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.java.util.http.client.HttpClient; +import org.apache.druid.java.util.http.client.Request; +import org.apache.druid.java.util.http.client.response.StatusResponseHandler; +import org.apache.druid.java.util.http.client.response.StatusResponseHolder; +import org.apache.druid.server.DruidNode; +import org.apache.druid.testing.embedded.EmbeddedBroker; +import org.apache.druid.testing.embedded.EmbeddedCoordinator; +import org.apache.druid.testing.embedded.EmbeddedDruidCluster; +import org.apache.druid.testing.embedded.EmbeddedDruidServer; +import org.apache.druid.testing.embedded.EmbeddedIndexer; +import org.apache.druid.testing.embedded.EmbeddedOverlord; +import org.apache.druid.testing.embedded.EmbeddedRouter; +import org.apache.druid.testing.embedded.junit5.EmbeddedClusterTestBase; +import org.jboss.netty.handler.codec.http.HttpMethod; +import org.jboss.netty.handler.codec.http.HttpResponseStatus; +import org.junit.Assert; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.FieldSource; + +import javax.ws.rs.core.MediaType; +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.function.Function; + +public class ITSqlQueryTest extends EmbeddedClusterTestBase +{ + private static final String SQL_QUERY_ROUTE = "%s/druid/v2/sql/"; + + public static List<Boolean> flags = List.of(true, false); + + private final EmbeddedBroker broker = new EmbeddedBroker(); + private final EmbeddedRouter router = new EmbeddedRouter(); + + private HttpClient httpClientRef; + private String brokerEndpoint; + private String routerEndpoint; + + interface OnRequest + { + void on(Request request); + } + + interface OnResponse + { + void on(int statusCode, String response); + } + + @Override + protected EmbeddedDruidCluster createCluster() + { + return EmbeddedDruidCluster.withEmbeddedDerbyAndZookeeper() + .useLatchableEmitter() + .addServer(new EmbeddedOverlord()) + .addServer(new EmbeddedCoordinator()) + .addServer(broker) + .addServer(router) + .addServer(new EmbeddedIndexer()); + } + + @BeforeEach + void setUp() + { + httpClientRef = router.bindings().globalHttpClient(); + brokerEndpoint = String.format(SQL_QUERY_ROUTE, getServerUrl(broker)); + routerEndpoint = String.format(SQL_QUERY_ROUTE, getServerUrl(router)); + } + + protected void assertStringCompare(String expected, String actual, Function<String, Boolean> predicate) + { + if (!predicate.apply(expected)) { + throw new ISE("Expected: [%s] but got [%s]", expected, actual); + } + } + + private static String getServerUrl(EmbeddedDruidServer<?> server) + { + final DruidNode node = server.bindings().selfNode(); + return StringUtils.format( + "http://%s:%s";, + node.getHost(), + node.getPlaintextPort() + ); + } + + private void executeQuery( + String endpoint, + String contentType, + String query, + OnRequest onRequest, + OnResponse onResponse + ) + { + URL url; + try { + url = new URL(endpoint); + } + catch (MalformedURLException e) { + throw new AssertionError("Malformed URL"); + } + + // TODO: instead of pure use of HttpClient, try to use onAnyBroker and create onAnyRouter instead. + Request request = new Request(HttpMethod.POST, url); + if (contentType != null) { + request.addHeader("Content-Type", contentType); + } + + if (query != null) { + request.setContent(query.getBytes(StandardCharsets.UTF_8)); + } + + if (onRequest != null) { + onRequest.on(request); + } + + StatusResponseHolder response; + try { + response = httpClientRef.go(request, StatusResponseHandler.getInstance()) + .get(); + } + catch (InterruptedException | ExecutionException e) { + throw new AssertionError("Failed to execute a request", e); + } + + Assertions.assertNotNull(response); + + onResponse.on( + response.getStatus().getCode(), + response.getContent().trim() + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testNullContentType(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + null, + "select 1", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(statusCode, HttpResponseStatus.BAD_REQUEST.getCode()); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testUnsupportedContentType(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + "application/xml", + "select 1", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(statusCode, HttpResponseStatus.UNSUPPORTED_MEDIA_TYPE.getCode()); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testTextPlain(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.TEXT_PLAIN, + "select \n1", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(200, statusCode); + Assertions.assertEquals("[{\"EXPR$0\":1}]", responseBody); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testFormURLEncoded(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.APPLICATION_FORM_URLENCODED, + URLEncoder.encode("select 'x % y'", StandardCharsets.UTF_8), + (request) -> { + }, + (statusCode, responseBody) -> { + Assert.assertEquals(200, statusCode); + Assert.assertEquals("[{\"EXPR$0\":\"x % y\"}]", responseBody); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testFormURLEncoded_InvalidEncoding(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.APPLICATION_FORM_URLENCODED, + "select 'x % y'", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(400, statusCode); + assertStringCompare("Unable to decode", responseBody, responseBody::contains); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testJSON(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.APPLICATION_JSON, + "{\"query\":\"select 567\"}", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(200, statusCode); + Assertions.assertEquals("[{\"EXPR$0\":567}]", responseBody); + } + ); + + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + "application/json; charset=UTF-8", + "{\"query\":\"select 567\"}", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(200, statusCode); + Assertions.assertEquals("[{\"EXPR$0\":567}]", responseBody); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testInvalidJSONFormat(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.APPLICATION_JSON, + "{\"query\":select 567}", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(400, statusCode); + assertStringCompare("Malformed SQL query", responseBody, responseBody::contains); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testEmptyQuery_TextPlain(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.TEXT_PLAIN, + null, + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(400, statusCode); + assertStringCompare("Empty query", responseBody, responseBody::contains); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testEmptyQuery_UrlEncoded(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.APPLICATION_FORM_URLENCODED, + null, + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(400, statusCode); + assertStringCompare("Empty query", responseBody, responseBody::contains); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testBlankQuery_TextPlain(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.TEXT_PLAIN, + " ", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(400, statusCode); + assertStringCompare("Empty query", responseBody, responseBody::contains); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testEmptyQuery_JSON(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.APPLICATION_JSON, + null, + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(400, statusCode); + assertStringCompare("Empty query", responseBody, responseBody::contains); + } + ); + } + + /** + * When multiple Content-Type headers are set, the first one (in this case, it's the text format) should be used. + */ + @ParameterizedTest + @FieldSource("flags") + public void testMultipleContentType(boolean shouldQueryBroker) Review Comment: ```suggestion public void testMultipleContentType_usesFirstOne(boolean shouldQueryBroker) ``` ########## embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/ITSqlQueryTest.java: ########## @@ -0,0 +1,379 @@ +/* + * 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.testing.embedded.query; + +import org.apache.druid.java.util.common.ISE; +import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.java.util.http.client.HttpClient; +import org.apache.druid.java.util.http.client.Request; +import org.apache.druid.java.util.http.client.response.StatusResponseHandler; +import org.apache.druid.java.util.http.client.response.StatusResponseHolder; +import org.apache.druid.server.DruidNode; +import org.apache.druid.testing.embedded.EmbeddedBroker; +import org.apache.druid.testing.embedded.EmbeddedCoordinator; +import org.apache.druid.testing.embedded.EmbeddedDruidCluster; +import org.apache.druid.testing.embedded.EmbeddedDruidServer; +import org.apache.druid.testing.embedded.EmbeddedIndexer; +import org.apache.druid.testing.embedded.EmbeddedOverlord; +import org.apache.druid.testing.embedded.EmbeddedRouter; +import org.apache.druid.testing.embedded.junit5.EmbeddedClusterTestBase; +import org.jboss.netty.handler.codec.http.HttpMethod; +import org.jboss.netty.handler.codec.http.HttpResponseStatus; +import org.junit.Assert; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.FieldSource; + +import javax.ws.rs.core.MediaType; +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.function.Function; + +public class ITSqlQueryTest extends EmbeddedClusterTestBase +{ + private static final String SQL_QUERY_ROUTE = "%s/druid/v2/sql/"; + + public static List<Boolean> flags = List.of(true, false); + + private final EmbeddedBroker broker = new EmbeddedBroker(); + private final EmbeddedRouter router = new EmbeddedRouter(); + + private HttpClient httpClientRef; + private String brokerEndpoint; + private String routerEndpoint; + + interface OnRequest + { + void on(Request request); + } + + interface OnResponse + { + void on(int statusCode, String response); + } + + @Override + protected EmbeddedDruidCluster createCluster() + { + return EmbeddedDruidCluster.withEmbeddedDerbyAndZookeeper() + .useLatchableEmitter() + .addServer(new EmbeddedOverlord()) + .addServer(new EmbeddedCoordinator()) + .addServer(broker) + .addServer(router) + .addServer(new EmbeddedIndexer()); + } + + @BeforeEach + void setUp() + { + httpClientRef = router.bindings().globalHttpClient(); + brokerEndpoint = String.format(SQL_QUERY_ROUTE, getServerUrl(broker)); + routerEndpoint = String.format(SQL_QUERY_ROUTE, getServerUrl(router)); + } + + protected void assertStringCompare(String expected, String actual, Function<String, Boolean> predicate) + { + if (!predicate.apply(expected)) { + throw new ISE("Expected: [%s] but got [%s]", expected, actual); + } + } + + private static String getServerUrl(EmbeddedDruidServer<?> server) + { + final DruidNode node = server.bindings().selfNode(); + return StringUtils.format( + "http://%s:%s";, + node.getHost(), + node.getPlaintextPort() + ); + } + + private void executeQuery( + String endpoint, + String contentType, + String query, + OnRequest onRequest, + OnResponse onResponse + ) + { + URL url; + try { + url = new URL(endpoint); + } + catch (MalformedURLException e) { + throw new AssertionError("Malformed URL"); + } + + // TODO: instead of pure use of HttpClient, try to use onAnyBroker and create onAnyRouter instead. + Request request = new Request(HttpMethod.POST, url); + if (contentType != null) { + request.addHeader("Content-Type", contentType); + } + + if (query != null) { + request.setContent(query.getBytes(StandardCharsets.UTF_8)); + } + + if (onRequest != null) { + onRequest.on(request); + } + + StatusResponseHolder response; + try { + response = httpClientRef.go(request, StatusResponseHandler.getInstance()) + .get(); + } + catch (InterruptedException | ExecutionException e) { + throw new AssertionError("Failed to execute a request", e); + } + + Assertions.assertNotNull(response); + + onResponse.on( + response.getStatus().getCode(), + response.getContent().trim() + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testNullContentType(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + null, + "select 1", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(statusCode, HttpResponseStatus.BAD_REQUEST.getCode()); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testUnsupportedContentType(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + "application/xml", + "select 1", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(statusCode, HttpResponseStatus.UNSUPPORTED_MEDIA_TYPE.getCode()); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testTextPlain(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.TEXT_PLAIN, + "select \n1", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(200, statusCode); + Assertions.assertEquals("[{\"EXPR$0\":1}]", responseBody); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testFormURLEncoded(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.APPLICATION_FORM_URLENCODED, + URLEncoder.encode("select 'x % y'", StandardCharsets.UTF_8), + (request) -> { + }, + (statusCode, responseBody) -> { + Assert.assertEquals(200, statusCode); + Assert.assertEquals("[{\"EXPR$0\":\"x % y\"}]", responseBody); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testFormURLEncoded_InvalidEncoding(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.APPLICATION_FORM_URLENCODED, + "select 'x % y'", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(400, statusCode); + assertStringCompare("Unable to decode", responseBody, responseBody::contains); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testJSON(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.APPLICATION_JSON, + "{\"query\":\"select 567\"}", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(200, statusCode); + Assertions.assertEquals("[{\"EXPR$0\":567}]", responseBody); + } + ); + + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + "application/json; charset=UTF-8", + "{\"query\":\"select 567\"}", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(200, statusCode); + Assertions.assertEquals("[{\"EXPR$0\":567}]", responseBody); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testInvalidJSONFormat(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.APPLICATION_JSON, + "{\"query\":select 567}", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(400, statusCode); + assertStringCompare("Malformed SQL query", responseBody, responseBody::contains); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testEmptyQuery_TextPlain(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.TEXT_PLAIN, + null, + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(400, statusCode); + assertStringCompare("Empty query", responseBody, responseBody::contains); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testEmptyQuery_UrlEncoded(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.APPLICATION_FORM_URLENCODED, + null, + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(400, statusCode); + assertStringCompare("Empty query", responseBody, responseBody::contains); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testBlankQuery_TextPlain(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.TEXT_PLAIN, + " ", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(400, statusCode); + assertStringCompare("Empty query", responseBody, responseBody::contains); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testEmptyQuery_JSON(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.APPLICATION_JSON, + null, + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(400, statusCode); + assertStringCompare("Empty query", responseBody, responseBody::contains); + } + ); + } + + /** + * When multiple Content-Type headers are set, the first one (in this case, it's the text format) should be used. + */ + @ParameterizedTest + @FieldSource("flags") Review Comment: Rather than have all the tests be parameterized, I wonder if it would be cleaner to: - write the main test class from point-of-view of Broker. - then add a separate test class (using junit5 `@Nested` or even a separate class) which does testing form PoV of Router. ########## embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/ITSqlQueryTest.java: ########## @@ -0,0 +1,379 @@ +/* + * 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.testing.embedded.query; + +import org.apache.druid.java.util.common.ISE; +import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.java.util.http.client.HttpClient; +import org.apache.druid.java.util.http.client.Request; +import org.apache.druid.java.util.http.client.response.StatusResponseHandler; +import org.apache.druid.java.util.http.client.response.StatusResponseHolder; +import org.apache.druid.server.DruidNode; +import org.apache.druid.testing.embedded.EmbeddedBroker; +import org.apache.druid.testing.embedded.EmbeddedCoordinator; +import org.apache.druid.testing.embedded.EmbeddedDruidCluster; +import org.apache.druid.testing.embedded.EmbeddedDruidServer; +import org.apache.druid.testing.embedded.EmbeddedIndexer; +import org.apache.druid.testing.embedded.EmbeddedOverlord; +import org.apache.druid.testing.embedded.EmbeddedRouter; +import org.apache.druid.testing.embedded.junit5.EmbeddedClusterTestBase; +import org.jboss.netty.handler.codec.http.HttpMethod; +import org.jboss.netty.handler.codec.http.HttpResponseStatus; +import org.junit.Assert; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.FieldSource; + +import javax.ws.rs.core.MediaType; +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.function.Function; + +public class ITSqlQueryTest extends EmbeddedClusterTestBase +{ + private static final String SQL_QUERY_ROUTE = "%s/druid/v2/sql/"; + + public static List<Boolean> flags = List.of(true, false); + + private final EmbeddedBroker broker = new EmbeddedBroker(); + private final EmbeddedRouter router = new EmbeddedRouter(); + + private HttpClient httpClientRef; + private String brokerEndpoint; + private String routerEndpoint; + + interface OnRequest + { + void on(Request request); + } + + interface OnResponse + { + void on(int statusCode, String response); + } + + @Override + protected EmbeddedDruidCluster createCluster() + { + return EmbeddedDruidCluster.withEmbeddedDerbyAndZookeeper() + .useLatchableEmitter() + .addServer(new EmbeddedOverlord()) + .addServer(new EmbeddedCoordinator()) + .addServer(broker) + .addServer(router) + .addServer(new EmbeddedIndexer()); + } + + @BeforeEach + void setUp() + { + httpClientRef = router.bindings().globalHttpClient(); + brokerEndpoint = String.format(SQL_QUERY_ROUTE, getServerUrl(broker)); + routerEndpoint = String.format(SQL_QUERY_ROUTE, getServerUrl(router)); + } + + protected void assertStringCompare(String expected, String actual, Function<String, Boolean> predicate) + { + if (!predicate.apply(expected)) { + throw new ISE("Expected: [%s] but got [%s]", expected, actual); + } + } + + private static String getServerUrl(EmbeddedDruidServer<?> server) + { + final DruidNode node = server.bindings().selfNode(); + return StringUtils.format( + "http://%s:%s";, + node.getHost(), + node.getPlaintextPort() + ); + } + + private void executeQuery( + String endpoint, + String contentType, + String query, + OnRequest onRequest, + OnResponse onResponse + ) + { + URL url; + try { + url = new URL(endpoint); + } + catch (MalformedURLException e) { + throw new AssertionError("Malformed URL"); + } + + // TODO: instead of pure use of HttpClient, try to use onAnyBroker and create onAnyRouter instead. Review Comment: We can remove this TODO and add the `onAnyRouter` method if needed in the future. ########## embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/ITSqlQueryTest.java: ########## @@ -0,0 +1,379 @@ +/* + * 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.testing.embedded.query; + +import org.apache.druid.java.util.common.ISE; +import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.java.util.http.client.HttpClient; +import org.apache.druid.java.util.http.client.Request; +import org.apache.druid.java.util.http.client.response.StatusResponseHandler; +import org.apache.druid.java.util.http.client.response.StatusResponseHolder; +import org.apache.druid.server.DruidNode; +import org.apache.druid.testing.embedded.EmbeddedBroker; +import org.apache.druid.testing.embedded.EmbeddedCoordinator; +import org.apache.druid.testing.embedded.EmbeddedDruidCluster; +import org.apache.druid.testing.embedded.EmbeddedDruidServer; +import org.apache.druid.testing.embedded.EmbeddedIndexer; +import org.apache.druid.testing.embedded.EmbeddedOverlord; +import org.apache.druid.testing.embedded.EmbeddedRouter; +import org.apache.druid.testing.embedded.junit5.EmbeddedClusterTestBase; +import org.jboss.netty.handler.codec.http.HttpMethod; +import org.jboss.netty.handler.codec.http.HttpResponseStatus; +import org.junit.Assert; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.FieldSource; + +import javax.ws.rs.core.MediaType; +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.function.Function; + +public class ITSqlQueryTest extends EmbeddedClusterTestBase Review Comment: @Fly-Style , please feel free to rename this class to something more appropriate since GitHub is still treating is a separate file. Also, maybe add a 1-line javadoc on what the test is about. ########## embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/ITSqlQueryTest.java: ########## @@ -0,0 +1,379 @@ +/* + * 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.testing.embedded.query; + +import org.apache.druid.java.util.common.ISE; +import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.java.util.http.client.HttpClient; +import org.apache.druid.java.util.http.client.Request; +import org.apache.druid.java.util.http.client.response.StatusResponseHandler; +import org.apache.druid.java.util.http.client.response.StatusResponseHolder; +import org.apache.druid.server.DruidNode; +import org.apache.druid.testing.embedded.EmbeddedBroker; +import org.apache.druid.testing.embedded.EmbeddedCoordinator; +import org.apache.druid.testing.embedded.EmbeddedDruidCluster; +import org.apache.druid.testing.embedded.EmbeddedDruidServer; +import org.apache.druid.testing.embedded.EmbeddedIndexer; +import org.apache.druid.testing.embedded.EmbeddedOverlord; +import org.apache.druid.testing.embedded.EmbeddedRouter; +import org.apache.druid.testing.embedded.junit5.EmbeddedClusterTestBase; +import org.jboss.netty.handler.codec.http.HttpMethod; +import org.jboss.netty.handler.codec.http.HttpResponseStatus; +import org.junit.Assert; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.FieldSource; + +import javax.ws.rs.core.MediaType; +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.function.Function; + +public class ITSqlQueryTest extends EmbeddedClusterTestBase +{ + private static final String SQL_QUERY_ROUTE = "%s/druid/v2/sql/"; + + public static List<Boolean> flags = List.of(true, false); + + private final EmbeddedBroker broker = new EmbeddedBroker(); + private final EmbeddedRouter router = new EmbeddedRouter(); + + private HttpClient httpClientRef; + private String brokerEndpoint; + private String routerEndpoint; + + interface OnRequest Review Comment: I think we can remove this now and just use a `Consumer<Request>`. ########## embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/ITSqlQueryTest.java: ########## @@ -0,0 +1,379 @@ +/* + * 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.testing.embedded.query; + +import org.apache.druid.java.util.common.ISE; +import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.java.util.http.client.HttpClient; +import org.apache.druid.java.util.http.client.Request; +import org.apache.druid.java.util.http.client.response.StatusResponseHandler; +import org.apache.druid.java.util.http.client.response.StatusResponseHolder; +import org.apache.druid.server.DruidNode; +import org.apache.druid.testing.embedded.EmbeddedBroker; +import org.apache.druid.testing.embedded.EmbeddedCoordinator; +import org.apache.druid.testing.embedded.EmbeddedDruidCluster; +import org.apache.druid.testing.embedded.EmbeddedDruidServer; +import org.apache.druid.testing.embedded.EmbeddedIndexer; +import org.apache.druid.testing.embedded.EmbeddedOverlord; +import org.apache.druid.testing.embedded.EmbeddedRouter; +import org.apache.druid.testing.embedded.junit5.EmbeddedClusterTestBase; +import org.jboss.netty.handler.codec.http.HttpMethod; +import org.jboss.netty.handler.codec.http.HttpResponseStatus; +import org.junit.Assert; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.FieldSource; + +import javax.ws.rs.core.MediaType; +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.function.Function; + +public class ITSqlQueryTest extends EmbeddedClusterTestBase +{ + private static final String SQL_QUERY_ROUTE = "%s/druid/v2/sql/"; + + public static List<Boolean> flags = List.of(true, false); + + private final EmbeddedBroker broker = new EmbeddedBroker(); + private final EmbeddedRouter router = new EmbeddedRouter(); + + private HttpClient httpClientRef; + private String brokerEndpoint; + private String routerEndpoint; + + interface OnRequest + { + void on(Request request); + } + + interface OnResponse Review Comment: We can use a `BiConsumer<Integer, String>` instead. What do you think? ########## embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/ITSqlQueryTest.java: ########## @@ -0,0 +1,379 @@ +/* + * 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.testing.embedded.query; + +import org.apache.druid.java.util.common.ISE; +import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.java.util.http.client.HttpClient; +import org.apache.druid.java.util.http.client.Request; +import org.apache.druid.java.util.http.client.response.StatusResponseHandler; +import org.apache.druid.java.util.http.client.response.StatusResponseHolder; +import org.apache.druid.server.DruidNode; +import org.apache.druid.testing.embedded.EmbeddedBroker; +import org.apache.druid.testing.embedded.EmbeddedCoordinator; +import org.apache.druid.testing.embedded.EmbeddedDruidCluster; +import org.apache.druid.testing.embedded.EmbeddedDruidServer; +import org.apache.druid.testing.embedded.EmbeddedIndexer; +import org.apache.druid.testing.embedded.EmbeddedOverlord; +import org.apache.druid.testing.embedded.EmbeddedRouter; +import org.apache.druid.testing.embedded.junit5.EmbeddedClusterTestBase; +import org.jboss.netty.handler.codec.http.HttpMethod; +import org.jboss.netty.handler.codec.http.HttpResponseStatus; +import org.junit.Assert; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.FieldSource; + +import javax.ws.rs.core.MediaType; +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.function.Function; + +public class ITSqlQueryTest extends EmbeddedClusterTestBase +{ + private static final String SQL_QUERY_ROUTE = "%s/druid/v2/sql/"; + + public static List<Boolean> flags = List.of(true, false); + + private final EmbeddedBroker broker = new EmbeddedBroker(); + private final EmbeddedRouter router = new EmbeddedRouter(); + + private HttpClient httpClientRef; + private String brokerEndpoint; + private String routerEndpoint; + + interface OnRequest + { + void on(Request request); + } + + interface OnResponse + { + void on(int statusCode, String response); + } + + @Override + protected EmbeddedDruidCluster createCluster() + { + return EmbeddedDruidCluster.withEmbeddedDerbyAndZookeeper() + .useLatchableEmitter() + .addServer(new EmbeddedOverlord()) + .addServer(new EmbeddedCoordinator()) + .addServer(broker) + .addServer(router) + .addServer(new EmbeddedIndexer()); + } + + @BeforeEach + void setUp() + { + httpClientRef = router.bindings().globalHttpClient(); + brokerEndpoint = String.format(SQL_QUERY_ROUTE, getServerUrl(broker)); + routerEndpoint = String.format(SQL_QUERY_ROUTE, getServerUrl(router)); + } + + protected void assertStringCompare(String expected, String actual, Function<String, Boolean> predicate) + { + if (!predicate.apply(expected)) { + throw new ISE("Expected: [%s] but got [%s]", expected, actual); + } + } + + private static String getServerUrl(EmbeddedDruidServer<?> server) + { + final DruidNode node = server.bindings().selfNode(); + return StringUtils.format( + "http://%s:%s";, + node.getHost(), + node.getPlaintextPort() + ); + } + + private void executeQuery( + String endpoint, + String contentType, + String query, + OnRequest onRequest, + OnResponse onResponse + ) + { + URL url; + try { + url = new URL(endpoint); + } + catch (MalformedURLException e) { + throw new AssertionError("Malformed URL"); + } + + // TODO: instead of pure use of HttpClient, try to use onAnyBroker and create onAnyRouter instead. + Request request = new Request(HttpMethod.POST, url); + if (contentType != null) { + request.addHeader("Content-Type", contentType); + } + + if (query != null) { + request.setContent(query.getBytes(StandardCharsets.UTF_8)); + } + + if (onRequest != null) { + onRequest.on(request); + } + + StatusResponseHolder response; + try { + response = httpClientRef.go(request, StatusResponseHandler.getInstance()) + .get(); + } + catch (InterruptedException | ExecutionException e) { + throw new AssertionError("Failed to execute a request", e); + } + + Assertions.assertNotNull(response); + + onResponse.on( + response.getStatus().getCode(), + response.getContent().trim() + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testNullContentType(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + null, + "select 1", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(statusCode, HttpResponseStatus.BAD_REQUEST.getCode()); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testUnsupportedContentType(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + "application/xml", + "select 1", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(statusCode, HttpResponseStatus.UNSUPPORTED_MEDIA_TYPE.getCode()); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testTextPlain(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.TEXT_PLAIN, + "select \n1", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(200, statusCode); + Assertions.assertEquals("[{\"EXPR$0\":1}]", responseBody); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testFormURLEncoded(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.APPLICATION_FORM_URLENCODED, + URLEncoder.encode("select 'x % y'", StandardCharsets.UTF_8), + (request) -> { + }, + (statusCode, responseBody) -> { + Assert.assertEquals(200, statusCode); + Assert.assertEquals("[{\"EXPR$0\":\"x % y\"}]", responseBody); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testFormURLEncoded_InvalidEncoding(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.APPLICATION_FORM_URLENCODED, + "select 'x % y'", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(400, statusCode); + assertStringCompare("Unable to decode", responseBody, responseBody::contains); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testJSON(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.APPLICATION_JSON, + "{\"query\":\"select 567\"}", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(200, statusCode); + Assertions.assertEquals("[{\"EXPR$0\":567}]", responseBody); + } + ); + + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + "application/json; charset=UTF-8", + "{\"query\":\"select 567\"}", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(200, statusCode); + Assertions.assertEquals("[{\"EXPR$0\":567}]", responseBody); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testInvalidJSONFormat(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.APPLICATION_JSON, + "{\"query\":select 567}", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(400, statusCode); + assertStringCompare("Malformed SQL query", responseBody, responseBody::contains); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testEmptyQuery_TextPlain(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.TEXT_PLAIN, + null, + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(400, statusCode); + assertStringCompare("Empty query", responseBody, responseBody::contains); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testEmptyQuery_UrlEncoded(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.APPLICATION_FORM_URLENCODED, + null, + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(400, statusCode); + assertStringCompare("Empty query", responseBody, responseBody::contains); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testBlankQuery_TextPlain(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.TEXT_PLAIN, + " ", + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(400, statusCode); + assertStringCompare("Empty query", responseBody, responseBody::contains); + } + ); + } + + @ParameterizedTest + @FieldSource("flags") + public void testEmptyQuery_JSON(boolean shouldQueryBroker) + { + executeQuery( + shouldQueryBroker ? brokerEndpoint : routerEndpoint, + MediaType.APPLICATION_JSON, + null, + (request) -> { + }, + (statusCode, responseBody) -> { + Assertions.assertEquals(400, statusCode); + assertStringCompare("Empty query", responseBody, responseBody::contains); + } + ); + } + + /** + * When multiple Content-Type headers are set, the first one (in this case, it's the text format) should be used. + */ Review Comment: we can remove this comment and update the test method name to capture this info. ########## embedded-tests/src/test/java/org/apache/druid/testing/embedded/query/ITSqlQueryTest.java: ########## @@ -0,0 +1,379 @@ +/* + * 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.testing.embedded.query; + +import org.apache.druid.java.util.common.ISE; +import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.java.util.http.client.HttpClient; +import org.apache.druid.java.util.http.client.Request; +import org.apache.druid.java.util.http.client.response.StatusResponseHandler; +import org.apache.druid.java.util.http.client.response.StatusResponseHolder; +import org.apache.druid.server.DruidNode; +import org.apache.druid.testing.embedded.EmbeddedBroker; +import org.apache.druid.testing.embedded.EmbeddedCoordinator; +import org.apache.druid.testing.embedded.EmbeddedDruidCluster; +import org.apache.druid.testing.embedded.EmbeddedDruidServer; +import org.apache.druid.testing.embedded.EmbeddedIndexer; +import org.apache.druid.testing.embedded.EmbeddedOverlord; +import org.apache.druid.testing.embedded.EmbeddedRouter; +import org.apache.druid.testing.embedded.junit5.EmbeddedClusterTestBase; +import org.jboss.netty.handler.codec.http.HttpMethod; +import org.jboss.netty.handler.codec.http.HttpResponseStatus; +import org.junit.Assert; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.FieldSource; + +import javax.ws.rs.core.MediaType; +import java.net.MalformedURLException; +import java.net.URL; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.concurrent.ExecutionException; +import java.util.function.Function; + +public class ITSqlQueryTest extends EmbeddedClusterTestBase +{ + private static final String SQL_QUERY_ROUTE = "%s/druid/v2/sql/"; + + public static List<Boolean> flags = List.of(true, false); + + private final EmbeddedBroker broker = new EmbeddedBroker(); + private final EmbeddedRouter router = new EmbeddedRouter(); + + private HttpClient httpClientRef; + private String brokerEndpoint; + private String routerEndpoint; + + interface OnRequest + { + void on(Request request); + } + + interface OnResponse + { + void on(int statusCode, String response); + } + + @Override + protected EmbeddedDruidCluster createCluster() + { + return EmbeddedDruidCluster.withEmbeddedDerbyAndZookeeper() + .useLatchableEmitter() + .addServer(new EmbeddedOverlord()) + .addServer(new EmbeddedCoordinator()) + .addServer(broker) + .addServer(router) + .addServer(new EmbeddedIndexer()); + } + + @BeforeEach + void setUp() + { + httpClientRef = router.bindings().globalHttpClient(); + brokerEndpoint = String.format(SQL_QUERY_ROUTE, getServerUrl(broker)); + routerEndpoint = String.format(SQL_QUERY_ROUTE, getServerUrl(router)); + } + + protected void assertStringCompare(String expected, String actual, Function<String, Boolean> predicate) Review Comment: It seems that this method is always used to check if `responseBody.contains(testString)`. We can remove this method and just inline the call where applicable. -- 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]
