From bda56540b5361e5825386b7f609c70912ca8dc2a Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Thu, 29 Nov 2018 09:54:13 +0200 Subject: [PATCH 1/8] Move SQL request parameters from http url params to JSON body --- .../sql/qa/multi_node/RestSqlMultinodeIT.java | 6 +- .../sql/qa/security/RestSqlSecurityIT.java | 35 +++--- .../xpack/sql/qa/security/UserFunctionIT.java | 13 +- .../xpack/sql/qa/rest/RestSqlTestCase.java | 49 ++++---- .../sql/qa/rest/RestSqlUsageTestCase.java | 13 +- .../sql/action/AbstractSqlQueryRequest.java | 45 +++++-- .../sql/action/SqlClearCursorRequest.java | 27 ++-- .../xpack/sql/action/SqlQueryRequest.java | 3 +- .../xpack/sql/action/SqlQueryResponse.java | 7 +- .../xpack/sql/action/SqlTranslateRequest.java | 6 +- .../action/SqlClearCursorRequestTests.java | 24 ++-- .../sql/action/SqlQueryRequestTests.java | 2 +- .../sql/action/SqlQueryResponseTests.java | 11 +- .../sql/action/SqlRequestParsersTests.java | 115 ++++++++++++++++++ .../sql/action/SqlTranslateRequestTests.java | 2 +- .../xpack/sql/client/HttpClient.java | 7 +- .../elasticsearch/xpack/sql/proto/Mode.java | 9 +- .../sql/proto/SqlClearCursorRequest.java | 8 ++ .../xpack/sql/proto/SqlQueryRequest.java | 8 ++ .../sql/plugin/RestSqlClearCursorAction.java | 18 ++- .../xpack/sql/plugin/RestSqlQueryAction.java | 26 ++-- .../sql/plugin/RestSqlTranslateAction.java | 20 ++- .../xpack/sql/plugin/SqlField.java | 16 +++ .../sql/plugin/TransportSqlQueryAction.java | 7 +- .../xpack/sql/action/CliFormatterTests.java | 3 +- .../sql/execution/search/CursorTests.java | 3 +- .../xpack/sql/plugin/TextFormatTests.java | 7 +- 27 files changed, 354 insertions(+), 136 deletions(-) create mode 100644 x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java create mode 100644 x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlField.java diff --git a/x-pack/plugin/sql/qa/multi-node/src/test/java/org/elasticsearch/xpack/sql/qa/multi_node/RestSqlMultinodeIT.java b/x-pack/plugin/sql/qa/multi-node/src/test/java/org/elasticsearch/xpack/sql/qa/multi_node/RestSqlMultinodeIT.java index 69b33767fa9c5..2a4dd79001d1c 100644 --- a/x-pack/plugin/sql/qa/multi-node/src/test/java/org/elasticsearch/xpack/sql/qa/multi_node/RestSqlMultinodeIT.java +++ b/x-pack/plugin/sql/qa/multi-node/src/test/java/org/elasticsearch/xpack/sql/qa/multi_node/RestSqlMultinodeIT.java @@ -26,6 +26,7 @@ import static java.util.Collections.singletonList; import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.columnInfo; +import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.mode; import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.randomMode; /** @@ -111,10 +112,7 @@ private void assertCount(RestClient client, int count) throws IOException { expected.put("rows", singletonList(singletonList(count))); Request request = new Request("POST", "/_sql"); - if (false == mode.isEmpty()) { - request.addParameter("mode", mode); - } - request.setJsonEntity("{\"query\": \"SELECT COUNT(*) FROM test\"}"); + request.setJsonEntity("{\"query\": \"SELECT COUNT(*) FROM test\"" + mode(mode) + "}"); Map actual = responseToMap(client.performRequest(request)); if (false == expected.equals(actual)) { diff --git a/x-pack/plugin/sql/qa/security/src/test/java/org/elasticsearch/xpack/sql/qa/security/RestSqlSecurityIT.java b/x-pack/plugin/sql/qa/security/src/test/java/org/elasticsearch/xpack/sql/qa/security/RestSqlSecurityIT.java index b6dd5efdfef93..86772a49372ba 100644 --- a/x-pack/plugin/sql/qa/security/src/test/java/org/elasticsearch/xpack/sql/qa/security/RestSqlSecurityIT.java +++ b/x-pack/plugin/sql/qa/security/src/test/java/org/elasticsearch/xpack/sql/qa/security/RestSqlSecurityIT.java @@ -30,6 +30,7 @@ import java.util.stream.Collectors; import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.columnInfo; +import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.mode; import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.randomMode; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.empty; @@ -66,10 +67,12 @@ public void expectMatchesAdmin(String adminSql, String user, String userSql) thr @Override public void expectScrollMatchesAdmin(String adminSql, String user, String userSql) throws Exception { String mode = randomMode(); - Map adminResponse = runSql(null, mode, - new StringEntity("{\"query\": \"" + adminSql + "\", \"fetch_size\": 1}", ContentType.APPLICATION_JSON)); - Map otherResponse = runSql(user, mode, - new StringEntity("{\"query\": \"" + adminSql + "\", \"fetch_size\": 1}", ContentType.APPLICATION_JSON)); + Map adminResponse = runSql(null, + new StringEntity("{\"query\": \"" + adminSql + "\", \"fetch_size\": 1" + mode(mode) + "}", + ContentType.APPLICATION_JSON)); + Map otherResponse = runSql(user, + new StringEntity("{\"query\": \"" + adminSql + "\", \"fetch_size\": 1" + mode(mode) + "}", + ContentType.APPLICATION_JSON)); String adminCursor = (String) adminResponse.remove("cursor"); String otherCursor = (String) otherResponse.remove("cursor"); @@ -77,10 +80,10 @@ public void expectScrollMatchesAdmin(String adminSql, String user, String userSq assertNotNull(otherCursor); assertResponse(adminResponse, otherResponse); while (true) { - adminResponse = runSql(null, mode, - new StringEntity("{\"cursor\": \"" + adminCursor + "\"}", ContentType.APPLICATION_JSON)); - otherResponse = runSql(user, mode, - new StringEntity("{\"cursor\": \"" + otherCursor + "\"}", ContentType.APPLICATION_JSON)); + adminResponse = runSql(null, + new StringEntity("{\"cursor\": \"" + adminCursor + "\"" + mode(mode) + "}", ContentType.APPLICATION_JSON)); + otherResponse = runSql(user, + new StringEntity("{\"cursor\": \"" + otherCursor + "\"" + mode(mode) + "}", ContentType.APPLICATION_JSON)); adminCursor = (String) adminResponse.remove("cursor"); otherCursor = (String) otherResponse.remove("cursor"); assertResponse(adminResponse, otherResponse); @@ -173,14 +176,11 @@ public void checkNoMonitorMain(String user) throws Exception { } private static Map runSql(@Nullable String asUser, String mode, String sql) throws IOException { - return runSql(asUser, mode, new StringEntity("{\"query\": \"" + sql + "\"}", ContentType.APPLICATION_JSON)); + return runSql(asUser, new StringEntity("{\"query\": \"" + sql + "\"" + mode(mode) + "}", ContentType.APPLICATION_JSON)); } - private static Map runSql(@Nullable String asUser, String mode, HttpEntity entity) throws IOException { + private static Map runSql(@Nullable String asUser, HttpEntity entity) throws IOException { Request request = new Request("POST", "/_sql"); - if (false == mode.isEmpty()) { - request.addParameter("mode", mode); - } if (asUser != null) { RequestOptions.Builder options = request.getOptions().toBuilder(); options.addHeader("es-security-runas-user", asUser); @@ -223,14 +223,15 @@ protected AuditLogAsserter createAuditLogAsserter() { public void testHijackScrollFails() throws Exception { createUser("full_access", "rest_minimal"); - Map adminResponse = RestActions.runSql(null, randomMode(), - new StringEntity("{\"query\": \"SELECT * FROM test\", \"fetch_size\": 1}", ContentType.APPLICATION_JSON)); + Map adminResponse = RestActions.runSql(null, + new StringEntity("{\"query\": \"SELECT * FROM test\", \"fetch_size\": 1" + mode(randomMode()) + "}", + ContentType.APPLICATION_JSON)); String cursor = (String) adminResponse.remove("cursor"); assertNotNull(cursor); - ResponseException e = expectThrows(ResponseException.class, () -> RestActions.runSql("full_access", randomMode(), - new StringEntity("{\"cursor\":\"" + cursor + "\"}", ContentType.APPLICATION_JSON))); + ResponseException e = expectThrows(ResponseException.class, () -> RestActions.runSql("full_access", + new StringEntity("{\"cursor\":\"" + cursor + "\"" + mode(randomMode()) + "}", ContentType.APPLICATION_JSON))); // TODO return a better error message for bad scrolls assertThat(e.getMessage(), containsString("No search context found for id")); assertEquals(404, e.getResponse().getStatusLine().getStatusCode()); diff --git a/x-pack/plugin/sql/qa/security/src/test/java/org/elasticsearch/xpack/sql/qa/security/UserFunctionIT.java b/x-pack/plugin/sql/qa/security/src/test/java/org/elasticsearch/xpack/sql/qa/security/UserFunctionIT.java index 4ba3875cac016..816bae4d925dc 100644 --- a/x-pack/plugin/sql/qa/security/src/test/java/org/elasticsearch/xpack/sql/qa/security/UserFunctionIT.java +++ b/x-pack/plugin/sql/qa/security/src/test/java/org/elasticsearch/xpack/sql/qa/security/UserFunctionIT.java @@ -35,6 +35,8 @@ import java.util.Map; import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.columnInfo; +import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.mode; +import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.randomMode; public class UserFunctionIT extends ESRestTestCase { @@ -174,14 +176,11 @@ private void deleteUser(String name) throws IOException { } private Map runSql(String asUser, String mode, String sql) throws IOException { - return runSql(asUser, mode, new StringEntity("{\"query\": \"" + sql + "\"}", ContentType.APPLICATION_JSON)); + return runSql(asUser, new StringEntity("{\"query\": \"" + sql + "\"" + mode(mode) + "}", ContentType.APPLICATION_JSON)); } - private Map runSql(String asUser, String mode, HttpEntity entity) throws IOException { + private Map runSql(String asUser, HttpEntity entity) throws IOException { Request request = new Request("POST", "/_sql"); - if (false == mode.isEmpty()) { - request.addParameter("mode", mode); - } if (asUser != null) { RequestOptions.Builder options = request.getOptions().toBuilder(); options.addHeader("es-security-runas-user", asUser); @@ -205,10 +204,6 @@ private static Map toMap(Response response) throws IOException { } } - private String randomMode() { - return randomFrom("plain", "jdbc", ""); - } - private void index(String... docs) throws IOException { Request request = new Request("POST", "/test/test/_bulk"); request.addParameter("refresh", "true"); diff --git a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java index 1b8160146cd88..a90ac79d9e93e 100644 --- a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java +++ b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java @@ -97,10 +97,10 @@ public void testNextPage() throws IOException { for (int i = 0; i < 20; i += 2) { Map response; if (i == 0) { - response = runSql(mode, new StringEntity(sqlRequest, ContentType.APPLICATION_JSON)); + response = runSql(new StringEntity(sqlRequest, ContentType.APPLICATION_JSON), ""); } else { - response = runSql(mode, new StringEntity("{\"cursor\":\"" + cursor + "\"}", - ContentType.APPLICATION_JSON)); + response = runSql(new StringEntity("{\"cursor\":\"" + cursor + "\"" + mode(mode) + "}", + ContentType.APPLICATION_JSON), ""); } Map expected = new HashMap<>(); @@ -120,8 +120,8 @@ public void testNextPage() throws IOException { } Map expected = new HashMap<>(); expected.put("rows", emptyList()); - assertResponse(expected, runSql(mode, new StringEntity("{ \"cursor\":\"" + cursor + "\"}", - ContentType.APPLICATION_JSON))); + assertResponse(expected, runSql(new StringEntity("{ \"cursor\":\"" + cursor + "\"" + mode(mode) + "}", + ContentType.APPLICATION_JSON), "")); } @AwaitsFix(bugUrl = "Unclear status, https://github.com/elastic/x-pack-elasticsearch/issues/2074") @@ -136,8 +136,7 @@ public void testTimeZone() throws IOException { expected.put("size", 2); // Default TimeZone is UTC - assertResponse(expected, runSql(mode, new StringEntity("{\"query\":\"SELECT DAY_OF_YEAR(test), COUNT(*) FROM test\"}", - ContentType.APPLICATION_JSON))); + assertResponse(expected, runSql(mode, "SELECT DAY_OF_YEAR(test), COUNT(*) FROM test")); } public void testScoreWithFieldNamedScore() throws IOException { @@ -306,14 +305,10 @@ private Map runSql(String mode, String sql) throws IOException { } private Map runSql(String mode, String sql, String suffix) throws IOException { - return runSql(mode, new StringEntity("{\"query\":\"" + sql + "\"}", ContentType.APPLICATION_JSON), suffix); + return runSql(new StringEntity("{\"query\":\"" + sql + "\"" + mode(mode) + "}", ContentType.APPLICATION_JSON), suffix); } - private Map runSql(String mode, HttpEntity sql) throws IOException { - return runSql(mode, sql, ""); - } - - private Map runSql(String mode, HttpEntity sql, String suffix) throws IOException { + private Map runSql(HttpEntity sql, String suffix) throws IOException { Request request = new Request("POST", "/_sql" + suffix); request.addParameter("error_trace", "true"); // Helps with debugging in case something crazy happens on the server. request.addParameter("pretty", "true"); // Improves error reporting readability @@ -321,9 +316,6 @@ private Map runSql(String mode, HttpEntity sql, String suffix) t // We default to JSON but we force it randomly for extra coverage request.addParameter("format", "json"); } - if (false == mode.isEmpty()) { - request.addParameter("mode", mode); // JDBC or PLAIN mode - } if (randomBoolean()) { // JSON is the default but randomly set it sometime for extra coverage RequestOptions.Builder options = request.getOptions().toBuilder(); @@ -331,6 +323,7 @@ private Map runSql(String mode, HttpEntity sql, String suffix) t request.setOptions(options); } request.setEntity(sql); + Response response = client().performRequest(request); try (InputStream content = response.getEntity().getContent()) { return XContentHelper.convertToMap(JsonXContent.jsonXContent, content, false); @@ -357,9 +350,9 @@ public void testBasicQueryWithFilter() throws IOException { Map expected = new HashMap<>(); expected.put("columns", singletonList(columnInfo(mode, "test", "text", JDBCType.VARCHAR, 0))); expected.put("rows", singletonList(singletonList("foo"))); - assertResponse(expected, runSql(mode, new StringEntity("{\"query\":\"SELECT * FROM test\", " + - "\"filter\":{\"match\": {\"test\": \"foo\"}}}", - ContentType.APPLICATION_JSON))); + assertResponse(expected, runSql(new StringEntity("{\"query\":\"SELECT * FROM test\", " + + "\"filter\":{\"match\": {\"test\": \"foo\"}}" + mode(mode) + "}", + ContentType.APPLICATION_JSON), "")); } public void testBasicQueryWithParameters() throws IOException { @@ -373,16 +366,16 @@ public void testBasicQueryWithParameters() throws IOException { columnInfo(mode, "param", "integer", JDBCType.INTEGER, 11) )); expected.put("rows", singletonList(Arrays.asList("foo", 10))); - assertResponse(expected, runSql(mode, new StringEntity("{\"query\":\"SELECT test, ? param FROM test WHERE test = ?\", " + - "\"params\":[{\"type\": \"integer\", \"value\": 10}, {\"type\": \"keyword\", \"value\": \"foo\"}]}", - ContentType.APPLICATION_JSON))); + assertResponse(expected, runSql(new StringEntity("{\"query\":\"SELECT test, ? param FROM test WHERE test = ?\", " + + "\"params\":[{\"type\": \"integer\", \"value\": 10}, {\"type\": \"keyword\", \"value\": \"foo\"}]" + + mode(mode) + "}", ContentType.APPLICATION_JSON), "")); } public void testBasicTranslateQueryWithFilter() throws IOException { index("{\"test\":\"foo\"}", "{\"test\":\"bar\"}"); - Map response = runSql("", + Map response = runSql( new StringEntity("{\"query\":\"SELECT * FROM test\", \"filter\":{\"match\": {\"test\": \"foo\"}}}", ContentType.APPLICATION_JSON), "/translate/" ); @@ -424,7 +417,7 @@ public void testTranslateQueryWithGroupByAndHaving() throws IOException { index("{\"salary\":100}", "{\"age\":20}"); - Map response = runSql("", + Map response = runSql( new StringEntity("{\"query\":\"SELECT avg(salary) FROM test GROUP BY abs(age) HAVING avg(salary) > 50 LIMIT 10\"}", ContentType.APPLICATION_JSON), "/translate/" ); @@ -570,9 +563,9 @@ public void testNextPageText() throws IOException { } Map expected = new HashMap<>(); expected.put("rows", emptyList()); - assertResponse(expected, runSql("", new StringEntity("{\"cursor\":\"" + cursor + "\"}", ContentType.APPLICATION_JSON))); + assertResponse(expected, runSql(new StringEntity("{\"cursor\":\"" + cursor + "\"}", ContentType.APPLICATION_JSON), "")); - Map response = runSql("", new StringEntity("{\"cursor\":\"" + cursor + "\"}", ContentType.APPLICATION_JSON), + Map response = runSql(new StringEntity("{\"cursor\":\"" + cursor + "\"}", ContentType.APPLICATION_JSON), "/close"); assertEquals(true, response.get("succeeded")); @@ -718,6 +711,10 @@ private static Map searchStats() throws IOException { public static String randomMode() { return randomFrom("", "jdbc", "plain"); } + + public static String mode(String mode) { + return mode.isEmpty() ? "" : ",\"mode\":\"" + mode + "\""; + } private void index(String... docs) throws IOException { Request request = new Request("POST", "/test/test/_bulk"); diff --git a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlUsageTestCase.java b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlUsageTestCase.java index a874296e15576..a5ec59aba26f0 100644 --- a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlUsageTestCase.java +++ b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlUsageTestCase.java @@ -25,6 +25,8 @@ import java.util.Locale; import java.util.Map; +import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.mode; + public abstract class RestSqlUsageTestCase extends ESRestTestCase { private List testData = Arrays.asList( new IndexDocument("used", "Don Quixote", 1072), @@ -274,20 +276,15 @@ private void runSql(String mode, String restClient, String sql) throws IOExcepti // We default to JSON but we force it randomly for extra coverage request.addParameter("format", "json"); } - if (false == mode.isEmpty()) { - request.addParameter("mode", mode); // JDBC or PLAIN mode - } - // randomly use the "client.id" parameter or not - if (false == ignoreClientType) { - request.addParameter("client.id", restClient); - } if (randomBoolean()) { // JSON is the default but randomly set it sometime for extra coverage RequestOptions.Builder options = request.getOptions().toBuilder(); options.addHeader("Accept", randomFrom("*/*", "application/json")); request.setOptions(options); } - request.setEntity(new StringEntity("{\"query\":\"" + sql + "\"}", ContentType.APPLICATION_JSON)); + request.setEntity(new StringEntity("{\"query\":\"" + sql + "\"" + mode(mode) + + (ignoreClientType ? "" : ",\"client.id\":\"" + restClient + "\"") + "}", + ContentType.APPLICATION_JSON)); client().performRequest(request); } diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java index 8892346c1b83e..e03221623e46e 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java @@ -15,6 +15,7 @@ import org.elasticsearch.common.xcontent.ToXContentFragment; import org.elasticsearch.index.query.AbstractQueryBuilder; import org.elasticsearch.index.query.QueryBuilder; +import org.elasticsearch.xpack.sql.proto.Mode; import org.elasticsearch.xpack.sql.proto.Protocol; import org.elasticsearch.xpack.sql.proto.RequestInfo; import org.elasticsearch.xpack.sql.proto.SqlTypedParamValue; @@ -58,18 +59,33 @@ public AbstractSqlQueryRequest(String query, List params, Qu protected static ObjectParser objectParser(Supplier supplier) { // TODO: convert this into ConstructingObjectParser - ObjectParser parser = new ObjectParser<>("sql/query", true, supplier); - parser.declareString(AbstractSqlQueryRequest::query, new ParseField("query")); - parser.declareObjectArray(AbstractSqlQueryRequest::params, (p, c) -> SqlTypedParamValue.fromXContent(p), new ParseField("params")); - parser.declareString((request, zoneId) -> request.timeZone(TimeZone.getTimeZone(zoneId)), new ParseField("time_zone")); - parser.declareInt(AbstractSqlQueryRequest::fetchSize, new ParseField("fetch_size")); + ObjectParser parser = new ObjectParser<>("sql/query", false, supplier); + parser.declareString(AbstractSqlQueryRequest::query, Field.QUERY); + parser.declareString((request, mode) -> { + Mode m = Mode.fromString(mode); + if (request.requestInfo() != null) { + request.requestInfo().mode(m); + } else { + request.requestInfo(new RequestInfo(m)); + } + }, Field.MODE); + parser.declareString((request, clientId) -> { + if (request.requestInfo() != null) { + request.requestInfo().clientId(clientId); + } else { + request.requestInfo(new RequestInfo(Mode.PLAIN, clientId)); + } + }, Field.CLIENT_ID); + parser.declareObjectArray(AbstractSqlQueryRequest::params, (p, c) -> SqlTypedParamValue.fromXContent(p), Field.PARAMS); + parser.declareString((request, zoneId) -> request.timeZone(TimeZone.getTimeZone(zoneId)), Field.TIME_ZONE); + parser.declareInt(AbstractSqlQueryRequest::fetchSize, Field.FETCH_SIZE); parser.declareString((request, timeout) -> request.requestTimeout(TimeValue.parseTimeValue(timeout, Protocol.REQUEST_TIMEOUT, - "request_timeout")), new ParseField("request_timeout")); + "request_timeout")), Field.REQUEST_TIMEOUT); parser.declareString( (request, timeout) -> request.pageTimeout(TimeValue.parseTimeValue(timeout, Protocol.PAGE_TIMEOUT, "page_timeout")), - new ParseField("page_timeout")); + Field.PAGE_TIMEOUT); parser.declareObject(AbstractSqlQueryRequest::filter, - (p, c) -> AbstractQueryBuilder.parseInnerQueryBuilder(p), new ParseField("filter")); + (p, c) -> AbstractQueryBuilder.parseInnerQueryBuilder(p), Field.FILTER); return parser; } @@ -235,4 +251,17 @@ public boolean equals(Object o) { public int hashCode() { return Objects.hash(super.hashCode(), query, timeZone, fetchSize, requestTimeout, pageTimeout, filter); } + + public interface Field { + ParseField QUERY = new ParseField("query"); + ParseField CURSOR = new ParseField("cursor"); + ParseField PARAMS = new ParseField("params"); + ParseField TIME_ZONE = new ParseField("time_zone"); + ParseField FETCH_SIZE = new ParseField("fetch_size"); + ParseField REQUEST_TIMEOUT = new ParseField("request_timeout"); + ParseField PAGE_TIMEOUT = new ParseField("page_timeout"); + ParseField FILTER = new ParseField("filter"); + ParseField MODE = new ParseField("mode"); + ParseField CLIENT_ID = new ParseField("client.id"); + } } diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java index 778ab4a613aa7..d6122d2b6633e 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.sql.action; import org.elasticsearch.action.ActionRequestValidationException; -import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.ConstructingObjectParser; @@ -26,24 +25,28 @@ */ public class SqlClearCursorRequest extends AbstractSqlRequest { - private static final ConstructingObjectParser PARSER = - new ConstructingObjectParser<>(SqlClearCursorAction.NAME, true, (objects, mode) -> new SqlClearCursorRequest( - mode, - (String) objects[0] - )); + private static final ConstructingObjectParser PARSER = + // here the position in "objects" is the same as the fields parser declarations below + new ConstructingObjectParser<>(SqlClearCursorAction.NAME, objects -> { + RequestInfo requestInfo = new RequestInfo(Mode.fromString((String) objects[1]), + (String) objects[2]); + return new SqlClearCursorRequest(requestInfo, (String) objects[0]); + }); static { - PARSER.declareString(constructorArg(), new ParseField("cursor")); + // "cursor" is required constructor parameter + PARSER.declareString(constructorArg(), AbstractSqlQueryRequest.Field.CURSOR); + PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), AbstractSqlQueryRequest.Field.MODE); + PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), AbstractSqlQueryRequest.Field.CLIENT_ID); } private String cursor; public SqlClearCursorRequest() { - } - public SqlClearCursorRequest(Mode mode, String cursor) { - super(new RequestInfo(mode)); + public SqlClearCursorRequest(RequestInfo requestInfo, String cursor) { + super(requestInfo); this.cursor = cursor; } @@ -101,7 +104,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws return new org.elasticsearch.xpack.sql.proto.SqlClearCursorRequest(cursor, requestInfo()).toXContent(builder, params); } - public static SqlClearCursorRequest fromXContent(XContentParser parser, Mode mode) { - return PARSER.apply(parser, mode); + public static SqlClearCursorRequest fromXContent(XContentParser parser) { + return PARSER.apply(parser, null); } } diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java index eeda0b9d8bfee..88e4a22c50bdc 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java @@ -114,9 +114,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws pageTimeout(), filter(), cursor(), requestInfo()).toXContent(builder, params); } - public static SqlQueryRequest fromXContent(XContentParser parser, RequestInfo requestInfo) { + public static SqlQueryRequest fromXContent(XContentParser parser) { SqlQueryRequest request = PARSER.apply(parser, null); - request.requestInfo(requestInfo); return request; } } \ No newline at end of file diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java index bafd4581f1fa5..8555776dc8964 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java @@ -31,6 +31,7 @@ public class SqlQueryResponse extends ActionResponse implements ToXContentObject // TODO: Simplify cursor handling private String cursor; + private Mode mode; private List columns; // TODO investigate reusing Page here - it probably is much more efficient private List> rows; @@ -38,8 +39,9 @@ public class SqlQueryResponse extends ActionResponse implements ToXContentObject public SqlQueryResponse() { } - public SqlQueryResponse(String cursor, @Nullable List columns, List> rows) { + public SqlQueryResponse(String cursor, Mode mode, @Nullable List columns, List> rows) { this.cursor = cursor; + this.mode = mode; this.columns = columns; this.rows = rows; } @@ -134,7 +136,6 @@ public void writeTo(StreamOutput out) throws IOException { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - Mode mode = Mode.fromString(params.param("mode")); builder.startObject(); { if (columns != null) { @@ -157,7 +158,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.endArray(); if (cursor.equals("") == false) { - builder.field(SqlQueryRequest.CURSOR.getPreferredName(), cursor); + builder.field(AbstractSqlQueryRequest.Field.CURSOR.getPreferredName(), cursor); } } return builder.endObject(); diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlTranslateRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlTranslateRequest.java index a25a1ca5c951a..c8015be462358 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlTranslateRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlTranslateRequest.java @@ -13,9 +13,8 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.query.QueryBuilder; -import org.elasticsearch.xpack.sql.proto.Mode; -import org.elasticsearch.xpack.sql.proto.SqlQueryRequest; import org.elasticsearch.xpack.sql.proto.RequestInfo; +import org.elasticsearch.xpack.sql.proto.SqlQueryRequest; import org.elasticsearch.xpack.sql.proto.SqlTypedParamValue; import java.io.IOException; @@ -56,9 +55,8 @@ public String getDescription() { return "SQL Translate [" + query() + "][" + filter() + "]"; } - public static SqlTranslateRequest fromXContent(XContentParser parser, Mode mode) { + public static SqlTranslateRequest fromXContent(XContentParser parser) { SqlTranslateRequest request = PARSER.apply(parser, null); - request.requestInfo(new RequestInfo(mode)); return request; } diff --git a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequestTests.java b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequestTests.java index e9c7043519b4c..72f0f4f8dc359 100644 --- a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequestTests.java +++ b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequestTests.java @@ -9,22 +9,29 @@ import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.test.AbstractSerializingTestCase; import org.elasticsearch.xpack.sql.proto.Mode; +import org.elasticsearch.xpack.sql.proto.RequestInfo; import org.junit.Before; import java.io.IOException; import java.util.function.Consumer; +import static org.elasticsearch.xpack.sql.proto.RequestInfo.CANVAS; +import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLI; + public class SqlClearCursorRequestTests extends AbstractSerializingTestCase { - public Mode testMode; + + public RequestInfo requestInfo; + public String clientId; @Before public void setup() { - testMode = randomFrom(Mode.values()); + clientId = randomFrom(CLI, CANVAS, randomAlphaOfLengthBetween(10, 20)); + requestInfo = new RequestInfo(randomFrom(Mode.values()), clientId); } @Override protected SqlClearCursorRequest createTestInstance() { - return new SqlClearCursorRequest(testMode, randomAlphaOfLength(100)); + return new SqlClearCursorRequest(requestInfo, randomAlphaOfLength(100)); } @Override @@ -34,20 +41,23 @@ protected Writeable.Reader instanceReader() { @Override protected SqlClearCursorRequest doParseInstance(XContentParser parser) { - return SqlClearCursorRequest.fromXContent(parser, testMode); + return SqlClearCursorRequest.fromXContent(parser); + } + + private RequestInfo randomRequestInfo() { + return new RequestInfo(randomFrom(Mode.values()), randomFrom(CLI, CANVAS, clientId)); } @Override protected SqlClearCursorRequest mutateInstance(SqlClearCursorRequest instance) throws IOException { @SuppressWarnings("unchecked") Consumer mutator = randomFrom( - request -> request.mode(randomValueOtherThan(request.mode(), () -> randomFrom(Mode.values()))), + request -> request.requestInfo(randomValueOtherThan(request.requestInfo(), this::randomRequestInfo)), request -> request.setCursor(randomValueOtherThan(request.getCursor(), SqlQueryResponseTests::randomStringCursor)) ); - SqlClearCursorRequest newRequest = new SqlClearCursorRequest(instance.mode(), instance.getCursor()); + SqlClearCursorRequest newRequest = new SqlClearCursorRequest(instance.requestInfo(), instance.getCursor()); mutator.accept(newRequest); return newRequest; - } } diff --git a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryRequestTests.java b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryRequestTests.java index beb206b2dbcbd..526c24cd868f6 100644 --- a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryRequestTests.java +++ b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryRequestTests.java @@ -96,7 +96,7 @@ private TimeValue randomTV() { @Override protected SqlQueryRequest doParseInstance(XContentParser parser) { - return SqlQueryRequest.fromXContent(parser, requestInfo); + return SqlQueryRequest.fromXContent(parser); } @Override diff --git a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryResponseTests.java b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryResponseTests.java index 7d9fa40a416ef..2833b4b1b38a8 100644 --- a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryResponseTests.java +++ b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryResponseTests.java @@ -14,6 +14,7 @@ import org.elasticsearch.test.AbstractStreamableXContentTestCase; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.sql.proto.ColumnInfo; +import org.elasticsearch.xpack.sql.proto.Mode; import java.io.IOException; import java.util.ArrayList; @@ -34,10 +35,10 @@ static String randomStringCursor() { @Override protected SqlQueryResponse createTestInstance() { - return createRandomInstance(randomStringCursor()); + return createRandomInstance(randomStringCursor(), randomFrom(Mode.values())); } - public static SqlQueryResponse createRandomInstance(String cursor) { + public static SqlQueryResponse createRandomInstance(String cursor, Mode mode) { int columnCount = between(1, 10); List columns = null; @@ -69,7 +70,7 @@ public static SqlQueryResponse createRandomInstance(String cursor) { rows.add(row); } } - return new SqlQueryResponse(cursor, columns, rows); + return new SqlQueryResponse(cursor, mode, columns, rows); } @Override @@ -108,7 +109,7 @@ public void testToXContent() throws IOException { } if (testInstance.cursor().equals("") == false) { - assertEquals(rootMap.get(SqlQueryRequest.CURSOR.getPreferredName()), testInstance.cursor()); + assertEquals(rootMap.get(AbstractSqlQueryRequest.Field.CURSOR.getPreferredName()), testInstance.cursor()); } } @@ -116,6 +117,6 @@ public void testToXContent() throws IOException { protected SqlQueryResponse doParseInstance(XContentParser parser) { org.elasticsearch.xpack.sql.proto.SqlQueryResponse response = org.elasticsearch.xpack.sql.proto.SqlQueryResponse.fromXContent(parser); - return new SqlQueryResponse(response.cursor(), response.columns(), response.rows()); + return new SqlQueryResponse(response.cursor(), Mode.JDBC, response.columns(), response.rows()); } } diff --git a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java new file mode 100644 index 0000000000000..8f05be83b9820 --- /dev/null +++ b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java @@ -0,0 +1,115 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.sql.action; + +import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.common.xcontent.DeprecationHandler; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.sql.proto.SqlTypedParamValue; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; + +import static org.hamcrest.Matchers.containsString; + +public class SqlRequestParsersTests extends ESTestCase { + + public void testUnknownFieldParsingErrors() throws IOException { + XContentParser parser = parser("{\"key\" : \"value\"}"); + Exception e = expectThrows(IllegalArgumentException.class, () -> SqlClearCursorRequest.fromXContent(parser)); + assertThat(e.getMessage(), containsString("unknown field [key]")); + + XContentParser parser2 = parser("{\"key\" : \"value\"}"); + e = expectThrows(IllegalArgumentException.class, () -> SqlQueryRequest.fromXContent(parser2)); + assertThat(e.getMessage(), containsString("unknown field [key]")); + + XContentParser parser3 = parser("{\"key\" : \"value\"}"); + e = expectThrows(IllegalArgumentException.class, () -> SqlTranslateRequest.fromXContent(parser3)); + assertThat(e.getMessage(), containsString("unknown field [key]")); + } + + public void testClearCursorRequestParser() throws IOException { + XContentParser parser = parser("{\"mode\" : \"jdbc\"}"); + Exception e = expectThrows(IllegalArgumentException.class, () -> SqlClearCursorRequest.fromXContent(parser)); + assertThat(e.getMessage(), containsString("Required [cursor]")); + + XContentParser parser2 = parser("{\"cursor\" : \"whatever\", \"fetch_size\":123}"); + e = expectThrows(IllegalArgumentException.class, () -> SqlClearCursorRequest.fromXContent(parser2)); + assertThat(e.getMessage(), containsString("unknown field [fetch_size]")); + + XContentParser parser3 = parser("{\"cursor\" : \"whatever\", \"mode\" : \"jdbc\", \"client.id\" : \"bla\"}"); + SqlClearCursorRequest request = SqlClearCursorRequest.fromXContent(parser3); + assertEquals("bla", request.clientId()); + assertEquals("jdbc", request.mode().toString()); + assertEquals("whatever", request.getCursor()); + + XContentParser parser4 = parser("{\"cursor\" : \"whatever\", \"mode\" : \"some foo mode\", \"client.id\" : \"bla\"}"); + request = SqlClearCursorRequest.fromXContent(parser4); + assertEquals("bla", request.clientId()); + assertEquals("plain", request.mode().toString()); + assertEquals("whatever", request.getCursor()); + } + + public void testTranslateRequestParser() throws IOException { + XContentParser parser = parser("{\"qquery\" : \"select * from bla\"}"); + Exception e = expectThrows(IllegalArgumentException.class, () -> SqlTranslateRequest.fromXContent(parser)); + assertThat(e.getMessage(), containsString("unknown field [qquery]")); + + XContentParser parser3 = parser("{\"query\" : \"select * from foo\"}"); + SqlTranslateRequest request = SqlTranslateRequest.fromXContent(parser3); + assertEquals("select * from foo", request.query()); + } + + public void testQueryRequestParser() throws IOException { + XContentParser parser = parser("{\"mode\" : 123}"); + Exception e = expectThrows(IllegalArgumentException.class, () -> SqlQueryRequest.fromXContent(parser)); + assertThat(e.getMessage(), containsString("mode doesn't support values of type: VALUE_NUMBER")); + + XContentParser parser2 = parser("{\"cursor\" : \"whatever\", \"fetch_size\":\"abc\"}"); + e = expectThrows(IllegalArgumentException.class, () -> SqlQueryRequest.fromXContent(parser2)); + assertThat(e.getMessage(), containsString("failed to parse field [fetch_size]")); + + XContentParser parser3 = parser("{\"client.id\":123}"); + e = expectThrows(IllegalArgumentException.class, () -> SqlQueryRequest.fromXContent(parser3)); + assertThat(e.getMessage(), containsString("client.id doesn't support values of type: VALUE_NUMBER")); + + XContentParser parser4 = parser("{\"params\":[{\"value\":123}]}"); + e = expectThrows(IllegalArgumentException.class, () -> SqlQueryRequest.fromXContent(parser4)); + assertThat(e.getMessage(), containsString("failed to parse field [params]")); + + XContentParser parser5 = parser("{\"time_zone\":12}"); + e = expectThrows(IllegalArgumentException.class, () -> SqlQueryRequest.fromXContent(parser5)); + assertThat(e.getMessage(), containsString("time_zone doesn't support values of type: VALUE_NUMBER")); + + XContentParser parser6 = parser("{\"cursor\" : \"whatever\", \"mode\" : \"jdbc\", \"client.id\" : \"bla\", \"query\":\"select\"," + + "\"params\":[{\"value\":123, \"type\":\"whatever\"}], \"time_zone\":\"UTC\", \"request_timeout\":\"5s\"," + + "\"page_timeout\":\"10s\"}"); + SqlQueryRequest request = SqlQueryRequest.fromXContent(parser6); + assertEquals("bla", request.clientId()); + assertEquals("jdbc", request.mode().toString()); + assertEquals("whatever", request.cursor()); + assertEquals("select", request.query()); + + List list = new ArrayList(1); + list.add(new SqlTypedParamValue("whatever", 123)); + assertEquals(list, request.params()); + + assertEquals("UTC", request.timeZone().getID()); + assertEquals(TimeValue.parseTimeValue("5s", "request_timeout"), request.requestTimeout()); + assertEquals(TimeValue.parseTimeValue("10s", "page_timeout"), request.pageTimeout()); + } + + private XContentParser parser(String content) throws IOException { + XContentType xContentType = XContentType.JSON; + return xContentType.xContent().createParser( + NamedXContentRegistry.EMPTY, DeprecationHandler.THROW_UNSUPPORTED_OPERATION, content); + } +} diff --git a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlTranslateRequestTests.java b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlTranslateRequestTests.java index fbc282a0a96be..3d48f7fc7a428 100644 --- a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlTranslateRequestTests.java +++ b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlTranslateRequestTests.java @@ -63,7 +63,7 @@ protected NamedXContentRegistry xContentRegistry() { @Override protected SqlTranslateRequest doParseInstance(XContentParser parser) { - return SqlTranslateRequest.fromXContent(parser, testMode); + return SqlTranslateRequest.fromXContent(parser); } @Override diff --git a/x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/HttpClient.java b/x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/HttpClient.java index e408027c286ec..3d48fee51f2ff 100644 --- a/x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/HttpClient.java +++ b/x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/HttpClient.java @@ -19,11 +19,11 @@ import org.elasticsearch.xpack.sql.proto.MainResponse; import org.elasticsearch.xpack.sql.proto.Mode; import org.elasticsearch.xpack.sql.proto.Protocol; +import org.elasticsearch.xpack.sql.proto.RequestInfo; import org.elasticsearch.xpack.sql.proto.SqlClearCursorRequest; import org.elasticsearch.xpack.sql.proto.SqlClearCursorResponse; import org.elasticsearch.xpack.sql.proto.SqlQueryRequest; import org.elasticsearch.xpack.sql.proto.SqlQueryResponse; -import org.elasticsearch.xpack.sql.proto.RequestInfo; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; @@ -37,7 +37,6 @@ import java.util.function.Function; import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLI; -import static org.elasticsearch.xpack.sql.client.ConnectionConfiguration.CLIENT_ID; /** * A specialized high-level REST client with support for SQL-related functions. @@ -95,9 +94,7 @@ private Response post(String path CheckedFunction responseParser) throws SQLException { byte[] requestBytes = toXContent(request); - String query = "error_trace&mode=" + - request.mode() + - (request.clientId() != null ? "&" + CLIENT_ID + "=" + request.clientId() : ""); + String query = "error_trace"; Tuple response = AccessController.doPrivileged((PrivilegedAction>>) () -> JreHttpUrlConnection.http(path, query, cfg, con -> diff --git a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java index 598c52a91797b..b1903ebc44e26 100644 --- a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java +++ b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java @@ -17,10 +17,15 @@ public enum Mode { ODBC; public static Mode fromString(String mode) { - if (mode == null) { + /*if (mode == null) { + return PLAIN; + } + return Mode.valueOf(mode.toUpperCase(Locale.ROOT));*/ + try { + return Mode.valueOf(mode.toUpperCase(Locale.ROOT)); + } catch (Exception e) { return PLAIN; } - return Mode.valueOf(mode.toUpperCase(Locale.ROOT)); } diff --git a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlClearCursorRequest.java b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlClearCursorRequest.java index 2881c0657aa19..a3a8ef186e599 100644 --- a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlClearCursorRequest.java +++ b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlClearCursorRequest.java @@ -43,6 +43,14 @@ public int hashCode() { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.field("cursor", cursor); + if (requestInfo() != null) { + if (mode() != null) { + builder.field("mode", mode().toString()); + } + if (clientId() != null) { + builder.field("client.id", clientId()); + } + } return builder; } } diff --git a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlQueryRequest.java b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlQueryRequest.java index 93a8f1f5e712c..98ddcedb86ce4 100644 --- a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlQueryRequest.java +++ b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlQueryRequest.java @@ -140,6 +140,14 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws if (query != null) { builder.field("query", query); } + if (requestInfo() != null) { + if (mode() != null) { + builder.field("mode", mode().toString()); + } + if (clientId() != null) { + builder.field("client.id", clientId()); + } + } if (this.params.isEmpty() == false) { builder.startArray("params"); for (SqlTypedParamValue val : this.params) { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlClearCursorAction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlClearCursorAction.java index cb2ffa3e107f6..b6a8592af185f 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlClearCursorAction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlClearCursorAction.java @@ -19,10 +19,14 @@ import org.elasticsearch.xpack.sql.action.SqlClearCursorRequest; import org.elasticsearch.xpack.sql.proto.Mode; import org.elasticsearch.xpack.sql.proto.Protocol; +import org.elasticsearch.xpack.sql.proto.RequestInfo; import java.io.IOException; +import java.util.Locale; import static org.elasticsearch.rest.RestRequest.Method.POST; +import static org.elasticsearch.xpack.sql.proto.RequestInfo.CANVAS; +import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLI; public class RestSqlClearCursorAction extends BaseRestHandler { @@ -40,7 +44,19 @@ public class RestSqlClearCursorAction extends BaseRestHandler { protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { SqlClearCursorRequest sqlRequest; try (XContentParser parser = request.contentOrSourceParamParser()) { - sqlRequest = SqlClearCursorRequest.fromXContent(parser, Mode.fromString(request.param("mode"))); + sqlRequest = SqlClearCursorRequest.fromXContent(parser); + } + + // no mode specified, default to "PLAIN" + if (sqlRequest.requestInfo() == null) { + sqlRequest.requestInfo(new RequestInfo(Mode.PLAIN)); + } + if (sqlRequest.requestInfo().clientId() != null) { + String clientId = sqlRequest.requestInfo().clientId().toLowerCase(Locale.ROOT); + if (!clientId.equals(CLI) && !clientId.equals(CANVAS)) { + clientId = null; + } + sqlRequest.requestInfo().clientId(clientId); } return channel -> client.executeLocally(SqlClearCursorAction.INSTANCE, sqlRequest, new RestToXContentListener<>(channel)); } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java index b8bf7cb858374..7dcf1df08e9f0 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java @@ -43,8 +43,6 @@ public class RestSqlQueryAction extends BaseRestHandler { private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(RestSqlQueryAction.class)); - private static String CLIENT_ID = "client.id"; - RestSqlQueryAction(Settings settings, RestController controller) { super(settings); // TODO: remove deprecated endpoint in 8.0.0 @@ -60,17 +58,21 @@ public class RestSqlQueryAction extends BaseRestHandler { @Override protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { SqlQueryRequest sqlRequest; - try (XContentParser parser = request.contentOrSourceParamParser()) { - String clientId = request.param(CLIENT_ID); - if (clientId != null) { - clientId = clientId.toLowerCase(Locale.ROOT); - if (!clientId.equals(CLI) && !clientId.equals(CANVAS)) { - clientId = null; - } + try (XContentParser parser = request.contentParser()) { + sqlRequest = SqlQueryRequest.fromXContent(parser); + } + + // no mode specified, default to "PLAIN" + if (sqlRequest.requestInfo() == null) { + sqlRequest.requestInfo(new RequestInfo(Mode.PLAIN)); + } + + if (sqlRequest.requestInfo().clientId() != null) { + String clientId = sqlRequest.requestInfo().clientId().toLowerCase(Locale.ROOT); + if (!clientId.equals(CLI) && !clientId.equals(CANVAS)) { + clientId = null; } - - sqlRequest = SqlQueryRequest.fromXContent(parser, - new RequestInfo(Mode.fromString(request.param("mode")), clientId)); + sqlRequest.requestInfo().clientId(clientId); } /* diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java index 5929ca5aadd35..ec055cc9a0509 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java @@ -18,11 +18,15 @@ import org.elasticsearch.xpack.sql.action.SqlTranslateRequest; import org.elasticsearch.xpack.sql.proto.Mode; import org.elasticsearch.xpack.sql.proto.Protocol; +import org.elasticsearch.xpack.sql.proto.RequestInfo; import java.io.IOException; +import java.util.Locale; import static org.elasticsearch.rest.RestRequest.Method.GET; import static org.elasticsearch.rest.RestRequest.Method.POST; +import static org.elasticsearch.xpack.sql.proto.RequestInfo.CANVAS; +import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLI; /** * REST action for translating SQL queries into ES requests @@ -47,8 +51,22 @@ public RestSqlTranslateAction(Settings settings, RestController controller) { protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { SqlTranslateRequest sqlRequest; try (XContentParser parser = request.contentOrSourceParamParser()) { - sqlRequest = SqlTranslateRequest.fromXContent(parser, Mode.fromString(request.param("mode"))); + sqlRequest = SqlTranslateRequest.fromXContent(parser); } + + // no mode specified, default to "PLAIN" + if (sqlRequest.requestInfo() == null) { + sqlRequest.requestInfo(new RequestInfo(Mode.PLAIN)); + } + + if (sqlRequest.requestInfo().clientId() != null) { + String clientId = sqlRequest.requestInfo().clientId().toLowerCase(Locale.ROOT); + if (!clientId.equals(CLI) && !clientId.equals(CANVAS)) { + clientId = null; + } + sqlRequest.requestInfo().clientId(clientId); + } + return channel -> client.executeLocally(SqlTranslateAction.INSTANCE, sqlRequest, new RestToXContentListener<>(channel)); } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlField.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlField.java new file mode 100644 index 0000000000000..58c25c101aebe --- /dev/null +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlField.java @@ -0,0 +1,16 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.sql.plugin; + +import org.elasticsearch.common.ParseField; + +final class SqlField { + public static final ParseField MODE = new ParseField("mode"); + public static final ParseField CLIENT_ID = new ParseField("client.id"); + + private SqlField() {} +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TransportSqlQueryAction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TransportSqlQueryAction.java index 94c7965ee95c7..5a794572b909e 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TransportSqlQueryAction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TransportSqlQueryAction.java @@ -88,7 +88,7 @@ public static void operation(PlanExecutor planExecutor, SqlQueryRequest request, } else { planExecutor.metrics().paging(metric); planExecutor.nextPage(cfg, Cursors.decodeFromString(request.cursor()), - ActionListener.wrap(rowSet -> listener.onResponse(createResponse(rowSet, null)), + ActionListener.wrap(rowSet -> listener.onResponse(createResponse(request.mode(), rowSet, null)), e -> { planExecutor.metrics().failed(metric); listener.onFailure(e); @@ -107,10 +107,10 @@ static SqlQueryResponse createResponse(SqlQueryRequest request, SchemaRowSet row } } columns = unmodifiableList(columns); - return createResponse(rowSet, columns); + return createResponse(request.mode(), rowSet, columns); } - static SqlQueryResponse createResponse(RowSet rowSet, List columns) { + static SqlQueryResponse createResponse(Mode mode, RowSet rowSet, List columns) { List> rows = new ArrayList<>(); rowSet.forEachRow(rowView -> { List row = new ArrayList<>(rowView.columnCount()); @@ -120,6 +120,7 @@ static SqlQueryResponse createResponse(RowSet rowSet, List columns) return new SqlQueryResponse( Cursors.encodeToString(Version.CURRENT, rowSet.nextPageCursor()), + mode, columns, rows); } diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/action/CliFormatterTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/action/CliFormatterTests.java index 29f371e007a98..ad4f1c998bbc8 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/action/CliFormatterTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/action/CliFormatterTests.java @@ -7,6 +7,7 @@ import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.sql.proto.ColumnInfo; +import org.elasticsearch.xpack.sql.proto.Mode; import java.sql.Types; import java.util.Arrays; @@ -14,7 +15,7 @@ import static org.hamcrest.Matchers.arrayWithSize; public class CliFormatterTests extends ESTestCase { - private final SqlQueryResponse firstResponse = new SqlQueryResponse("", + private final SqlQueryResponse firstResponse = new SqlQueryResponse("", Mode.PLAIN, Arrays.asList( new ColumnInfo("", "foo", "string", Types.VARCHAR, 0), new ColumnInfo("", "bar", "long", Types.BIGINT, 15), diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/CursorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/CursorTests.java index 2c1ef04c863ba..68fd6aa0bb415 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/CursorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/execution/search/CursorTests.java @@ -16,6 +16,7 @@ import org.elasticsearch.xpack.sql.action.SqlQueryResponse; import org.elasticsearch.xpack.sql.plugin.CliFormatterCursor; import org.elasticsearch.xpack.sql.proto.ColumnInfo; +import org.elasticsearch.xpack.sql.proto.Mode; import org.elasticsearch.xpack.sql.session.Configuration; import org.elasticsearch.xpack.sql.session.Cursor; import org.elasticsearch.xpack.sql.session.Cursors; @@ -69,7 +70,7 @@ private static SqlQueryResponse createRandomSqlResponse() { randomInt(), randomInt(25))); } } - return new SqlQueryResponse("", columns, Collections.emptyList()); + return new SqlQueryResponse("", randomFrom(Mode.values()), columns, Collections.emptyList()); } @SuppressWarnings("unchecked") diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java index 7a0f6dbca9d13..f1210ad6137db 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java @@ -12,6 +12,7 @@ import org.elasticsearch.test.rest.FakeRestRequest; import org.elasticsearch.xpack.sql.action.SqlQueryResponse; import org.elasticsearch.xpack.sql.proto.ColumnInfo; +import org.elasticsearch.xpack.sql.proto.Mode; import java.util.ArrayList; import java.util.List; @@ -117,7 +118,7 @@ public void testTsvFormatWithEscapedData() { } private static SqlQueryResponse emptyData() { - return new SqlQueryResponse(null, singletonList(new ColumnInfo("index", "name", "keyword")), emptyList()); + return new SqlQueryResponse(null, Mode.JDBC, singletonList(new ColumnInfo("index", "name", "keyword")), emptyList()); } private static SqlQueryResponse regularData() { @@ -131,7 +132,7 @@ private static SqlQueryResponse regularData() { values.add(asList("Along The River Bank", 11 * 60 + 48)); values.add(asList("Mind Train", 4 * 60 + 40)); - return new SqlQueryResponse(null, headers, values); + return new SqlQueryResponse(null, Mode.JDBC, headers, values); } private static SqlQueryResponse escapedData() { @@ -145,7 +146,7 @@ private static SqlQueryResponse escapedData() { values.add(asList("normal", "\"quo\"ted\",\n")); values.add(asList("commas", "a,b,c,\n,d,e,\t\n")); - return new SqlQueryResponse(null, headers, values); + return new SqlQueryResponse(null, Mode.JDBC, headers, values); } private static RestRequest req() { From 6c69d77517a0585e355f56f04aa20d19d450c706 Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Mon, 3 Dec 2018 12:02:37 +0200 Subject: [PATCH 2/8] Removed leftover commented code --- .../src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java index b1903ebc44e26..940515f64cfbc 100644 --- a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java +++ b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java @@ -17,10 +17,6 @@ public enum Mode { ODBC; public static Mode fromString(String mode) { - /*if (mode == null) { - return PLAIN; - } - return Mode.valueOf(mode.toUpperCase(Locale.ROOT));*/ try { return Mode.valueOf(mode.toUpperCase(Locale.ROOT)); } catch (Exception e) { From 375caf4e4be31131153880d97fb298c148f5a284 Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Tue, 4 Dec 2018 04:34:14 +0200 Subject: [PATCH 3/8] Adressing review comments --- .../sql/action/AbstractSqlQueryRequest.java | 20 ++-- .../sql/action/SqlClearCursorRequest.java | 9 +- .../xpack/sql/action/SqlQueryRequest.java | 11 +-- .../xpack/sql/action/SqlQueryResponse.java | 3 +- .../sql/action/SqlQueryResponseTests.java | 3 +- .../sql/action/SqlRequestParsersTests.java | 93 +++++++++---------- .../xpack/sql/plugin/AbstractSqlAction.java | 64 +++++++++++++ .../sql/plugin/RestSqlClearCursorAction.java | 32 +++---- .../xpack/sql/plugin/RestSqlQueryAction.java | 32 ++----- .../sql/plugin/RestSqlTranslateAction.java | 34 +++---- .../xpack/sql/plugin/SqlField.java | 16 ---- 11 files changed, 160 insertions(+), 157 deletions(-) create mode 100644 x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/AbstractSqlAction.java delete mode 100644 x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlField.java diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java index e03221623e46e..56bd6910ea1d4 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java @@ -60,7 +60,7 @@ public AbstractSqlQueryRequest(String query, List params, Qu protected static ObjectParser objectParser(Supplier supplier) { // TODO: convert this into ConstructingObjectParser ObjectParser parser = new ObjectParser<>("sql/query", false, supplier); - parser.declareString(AbstractSqlQueryRequest::query, Field.QUERY); + parser.declareString(AbstractSqlQueryRequest::query, SqlRequestField.QUERY); parser.declareString((request, mode) -> { Mode m = Mode.fromString(mode); if (request.requestInfo() != null) { @@ -68,24 +68,24 @@ protected static ObjectParser objec } else { request.requestInfo(new RequestInfo(m)); } - }, Field.MODE); + }, SqlRequestField.MODE); parser.declareString((request, clientId) -> { if (request.requestInfo() != null) { request.requestInfo().clientId(clientId); } else { request.requestInfo(new RequestInfo(Mode.PLAIN, clientId)); } - }, Field.CLIENT_ID); - parser.declareObjectArray(AbstractSqlQueryRequest::params, (p, c) -> SqlTypedParamValue.fromXContent(p), Field.PARAMS); - parser.declareString((request, zoneId) -> request.timeZone(TimeZone.getTimeZone(zoneId)), Field.TIME_ZONE); - parser.declareInt(AbstractSqlQueryRequest::fetchSize, Field.FETCH_SIZE); + }, SqlRequestField.CLIENT_ID); + parser.declareObjectArray(AbstractSqlQueryRequest::params, (p, c) -> SqlTypedParamValue.fromXContent(p), SqlRequestField.PARAMS); + parser.declareString((request, zoneId) -> request.timeZone(TimeZone.getTimeZone(zoneId)), SqlRequestField.TIME_ZONE); + parser.declareInt(AbstractSqlQueryRequest::fetchSize, SqlRequestField.FETCH_SIZE); parser.declareString((request, timeout) -> request.requestTimeout(TimeValue.parseTimeValue(timeout, Protocol.REQUEST_TIMEOUT, - "request_timeout")), Field.REQUEST_TIMEOUT); + "request_timeout")), SqlRequestField.REQUEST_TIMEOUT); parser.declareString( (request, timeout) -> request.pageTimeout(TimeValue.parseTimeValue(timeout, Protocol.PAGE_TIMEOUT, "page_timeout")), - Field.PAGE_TIMEOUT); + SqlRequestField.PAGE_TIMEOUT); parser.declareObject(AbstractSqlQueryRequest::filter, - (p, c) -> AbstractQueryBuilder.parseInnerQueryBuilder(p), Field.FILTER); + (p, c) -> AbstractQueryBuilder.parseInnerQueryBuilder(p), SqlRequestField.FILTER); return parser; } @@ -252,7 +252,7 @@ public int hashCode() { return Objects.hash(super.hashCode(), query, timeZone, fetchSize, requestTimeout, pageTimeout, filter); } - public interface Field { + public interface SqlRequestField { ParseField QUERY = new ParseField("query"); ParseField CURSOR = new ParseField("cursor"); ParseField PARAMS = new ParseField("params"); diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java index d6122d2b6633e..84b69ef4691ca 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java @@ -19,6 +19,9 @@ import static org.elasticsearch.action.ValidateActions.addValidationError; import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg; +import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.CURSOR; +import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.MODE; +import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.CLIENT_ID; /** * Request to clean all SQL resources associated with the cursor @@ -35,9 +38,9 @@ public class SqlClearCursorRequest extends AbstractSqlRequest { static { // "cursor" is required constructor parameter - PARSER.declareString(constructorArg(), AbstractSqlQueryRequest.Field.CURSOR); - PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), AbstractSqlQueryRequest.Field.MODE); - PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), AbstractSqlQueryRequest.Field.CLIENT_ID); + PARSER.declareString(constructorArg(), CURSOR); + PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), MODE); + PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), CLIENT_ID); } private String cursor; diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java index 88e4a22c50bdc..ed40f0110f482 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.sql.action; import org.elasticsearch.action.ActionRequestValidationException; -import org.elasticsearch.common.ParseField; import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; @@ -14,7 +13,6 @@ import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.index.query.AbstractQueryBuilder; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.xpack.sql.proto.RequestInfo; import org.elasticsearch.xpack.sql.proto.SqlTypedParamValue; @@ -25,6 +23,7 @@ import java.util.TimeZone; import static org.elasticsearch.action.ValidateActions.addValidationError; +import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.CURSOR; /** * Request to perform an sql query @@ -32,13 +31,8 @@ public class SqlQueryRequest extends AbstractSqlQueryRequest { private static final ObjectParser PARSER = objectParser(SqlQueryRequest::new); - public static final ParseField CURSOR = new ParseField("cursor"); - public static final ParseField FILTER = new ParseField("filter"); - static { PARSER.declareString(SqlQueryRequest::cursor, CURSOR); - PARSER.declareObject(SqlQueryRequest::filter, - (p, c) -> AbstractQueryBuilder.parseInnerQueryBuilder(p), FILTER); } private String cursor = ""; @@ -115,7 +109,6 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws } public static SqlQueryRequest fromXContent(XContentParser parser) { - SqlQueryRequest request = PARSER.apply(parser, null); - return request; + return PARSER.apply(parser, null); } } \ No newline at end of file diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java index 8555776dc8964..d88b0c6640c69 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java @@ -23,6 +23,7 @@ import java.util.Objects; import static java.util.Collections.unmodifiableList; +import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.CURSOR; /** * Response to perform an sql query @@ -158,7 +159,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.endArray(); if (cursor.equals("") == false) { - builder.field(AbstractSqlQueryRequest.Field.CURSOR.getPreferredName(), cursor); + builder.field(CURSOR.getPreferredName(), cursor); } } return builder.endObject(); diff --git a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryResponseTests.java b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryResponseTests.java index 2833b4b1b38a8..1c4e5f42b32c0 100644 --- a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryResponseTests.java +++ b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryResponseTests.java @@ -26,6 +26,7 @@ import static org.elasticsearch.common.xcontent.ToXContent.EMPTY_PARAMS; import static org.hamcrest.Matchers.hasSize; +import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.CURSOR; public class SqlQueryResponseTests extends AbstractStreamableXContentTestCase { @@ -109,7 +110,7 @@ public void testToXContent() throws IOException { } if (testInstance.cursor().equals("") == false) { - assertEquals(rootMap.get(AbstractSqlQueryRequest.Field.CURSOR.getPreferredName()), testInstance.cursor()); + assertEquals(rootMap.get(CURSOR.getPreferredName()), testInstance.cursor()); } } diff --git a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java index 8f05be83b9820..defda7e9a1574 100644 --- a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java +++ b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java @@ -17,82 +17,62 @@ import java.io.IOException; import java.util.ArrayList; import java.util.List; +import java.util.function.Consumer; +import java.util.function.Function; import static org.hamcrest.Matchers.containsString; public class SqlRequestParsersTests extends ESTestCase { public void testUnknownFieldParsingErrors() throws IOException { - XContentParser parser = parser("{\"key\" : \"value\"}"); - Exception e = expectThrows(IllegalArgumentException.class, () -> SqlClearCursorRequest.fromXContent(parser)); - assertThat(e.getMessage(), containsString("unknown field [key]")); - - XContentParser parser2 = parser("{\"key\" : \"value\"}"); - e = expectThrows(IllegalArgumentException.class, () -> SqlQueryRequest.fromXContent(parser2)); - assertThat(e.getMessage(), containsString("unknown field [key]")); - - XContentParser parser3 = parser("{\"key\" : \"value\"}"); - e = expectThrows(IllegalArgumentException.class, () -> SqlTranslateRequest.fromXContent(parser3)); - assertThat(e.getMessage(), containsString("unknown field [key]")); + assertParsingErrorMessage("{\"key\" : \"value\"}", "unknown field [key]", SqlClearCursorRequest::fromXContent); + assertParsingErrorMessage("{\"key\" : \"value\"}", "unknown field [key]", SqlQueryRequest::fromXContent); + assertParsingErrorMessage("{\"key\" : \"value\"}", "unknown field [key]", SqlTranslateRequest::fromXContent); } public void testClearCursorRequestParser() throws IOException { - XContentParser parser = parser("{\"mode\" : \"jdbc\"}"); - Exception e = expectThrows(IllegalArgumentException.class, () -> SqlClearCursorRequest.fromXContent(parser)); - assertThat(e.getMessage(), containsString("Required [cursor]")); + assertParsingErrorMessage("{\"mode\" : \"jdbc\"}", "Required [cursor]", SqlClearCursorRequest::fromXContent); + assertParsingErrorMessage("{\"cursor\" : \"whatever\", \"fetch_size\":123}", "unknown field [fetch_size]", + SqlClearCursorRequest::fromXContent); - XContentParser parser2 = parser("{\"cursor\" : \"whatever\", \"fetch_size\":123}"); - e = expectThrows(IllegalArgumentException.class, () -> SqlClearCursorRequest.fromXContent(parser2)); - assertThat(e.getMessage(), containsString("unknown field [fetch_size]")); - - XContentParser parser3 = parser("{\"cursor\" : \"whatever\", \"mode\" : \"jdbc\", \"client.id\" : \"bla\"}"); - SqlClearCursorRequest request = SqlClearCursorRequest.fromXContent(parser3); + SqlClearCursorRequest request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"jdbc\", \"client.id\" : \"bla\"}", + SqlClearCursorRequest::fromXContent); assertEquals("bla", request.clientId()); assertEquals("jdbc", request.mode().toString()); assertEquals("whatever", request.getCursor()); - XContentParser parser4 = parser("{\"cursor\" : \"whatever\", \"mode\" : \"some foo mode\", \"client.id\" : \"bla\"}"); - request = SqlClearCursorRequest.fromXContent(parser4); + request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"some foo mode\", \"client.id\" : \"bla\"}", + SqlClearCursorRequest::fromXContent); assertEquals("bla", request.clientId()); assertEquals("plain", request.mode().toString()); assertEquals("whatever", request.getCursor()); + + request = generateRequest("{\"cursor\" : \"whatever\"}", SqlClearCursorRequest::fromXContent); + assertNull(request.clientId()); + assertEquals("plain", request.mode().toString()); + assertEquals("whatever", request.getCursor()); } public void testTranslateRequestParser() throws IOException { - XContentParser parser = parser("{\"qquery\" : \"select * from bla\"}"); - Exception e = expectThrows(IllegalArgumentException.class, () -> SqlTranslateRequest.fromXContent(parser)); - assertThat(e.getMessage(), containsString("unknown field [qquery]")); + assertParsingErrorMessage("{\"qquery\" : \"select * from bla\"}", "unknown field [qquery]", SqlTranslateRequest::fromXContent); - XContentParser parser3 = parser("{\"query\" : \"select * from foo\"}"); - SqlTranslateRequest request = SqlTranslateRequest.fromXContent(parser3); + SqlTranslateRequest request = generateRequest("{\"query\" : \"select * from foo\"}", SqlTranslateRequest::fromXContent); assertEquals("select * from foo", request.query()); } public void testQueryRequestParser() throws IOException { - XContentParser parser = parser("{\"mode\" : 123}"); - Exception e = expectThrows(IllegalArgumentException.class, () -> SqlQueryRequest.fromXContent(parser)); - assertThat(e.getMessage(), containsString("mode doesn't support values of type: VALUE_NUMBER")); - - XContentParser parser2 = parser("{\"cursor\" : \"whatever\", \"fetch_size\":\"abc\"}"); - e = expectThrows(IllegalArgumentException.class, () -> SqlQueryRequest.fromXContent(parser2)); - assertThat(e.getMessage(), containsString("failed to parse field [fetch_size]")); - - XContentParser parser3 = parser("{\"client.id\":123}"); - e = expectThrows(IllegalArgumentException.class, () -> SqlQueryRequest.fromXContent(parser3)); - assertThat(e.getMessage(), containsString("client.id doesn't support values of type: VALUE_NUMBER")); + assertParsingErrorMessage("{\"mode\" : 123}", "mode doesn't support values of type: VALUE_NUMBER", SqlQueryRequest::fromXContent); + assertParsingErrorMessage("{\"cursor\" : \"whatever\", \"fetch_size\":\"abc\"}", "failed to parse field [fetch_size]", + SqlQueryRequest::fromXContent); + assertParsingErrorMessage("{\"client.id\":123}", "client.id doesn't support values of type: VALUE_NUMBER", + SqlQueryRequest::fromXContent); + assertParsingErrorMessage("{\"params\":[{\"value\":123}]}", "failed to parse field [params]", SqlQueryRequest::fromXContent); + assertParsingErrorMessage("{\"time_zone\":12}", "time_zone doesn't support values of type: VALUE_NUMBER", + SqlQueryRequest::fromXContent); - XContentParser parser4 = parser("{\"params\":[{\"value\":123}]}"); - e = expectThrows(IllegalArgumentException.class, () -> SqlQueryRequest.fromXContent(parser4)); - assertThat(e.getMessage(), containsString("failed to parse field [params]")); - - XContentParser parser5 = parser("{\"time_zone\":12}"); - e = expectThrows(IllegalArgumentException.class, () -> SqlQueryRequest.fromXContent(parser5)); - assertThat(e.getMessage(), containsString("time_zone doesn't support values of type: VALUE_NUMBER")); - - XContentParser parser6 = parser("{\"cursor\" : \"whatever\", \"mode\" : \"jdbc\", \"client.id\" : \"bla\", \"query\":\"select\"," - + "\"params\":[{\"value\":123, \"type\":\"whatever\"}], \"time_zone\":\"UTC\", \"request_timeout\":\"5s\"," - + "\"page_timeout\":\"10s\"}"); - SqlQueryRequest request = SqlQueryRequest.fromXContent(parser6); + SqlQueryRequest request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"jdbc\", \"client.id\" : \"bla\"," + + "\"query\":\"select\",\"params\":[{\"value\":123, \"type\":\"whatever\"}], \"time_zone\":\"UTC\"," + + "\"request_timeout\":\"5s\",\"page_timeout\":\"10s\"}", SqlQueryRequest::fromXContent); assertEquals("bla", request.clientId()); assertEquals("jdbc", request.mode().toString()); assertEquals("whatever", request.cursor()); @@ -101,12 +81,23 @@ public void testQueryRequestParser() throws IOException { List list = new ArrayList(1); list.add(new SqlTypedParamValue("whatever", 123)); assertEquals(list, request.params()); - assertEquals("UTC", request.timeZone().getID()); assertEquals(TimeValue.parseTimeValue("5s", "request_timeout"), request.requestTimeout()); assertEquals(TimeValue.parseTimeValue("10s", "page_timeout"), request.pageTimeout()); } + private R generateRequest(String json, Function fromXContent) + throws IOException { + XContentParser parser = parser(json); + return fromXContent.apply(parser); + } + + private void assertParsingErrorMessage(String json, String errorMessage, Consumer consumer) throws IOException { + XContentParser parser = parser(json); + Exception e = expectThrows(IllegalArgumentException.class, () -> consumer.accept(parser)); + assertThat(e.getMessage(), containsString(errorMessage)); + } + private XContentParser parser(String content) throws IOException { XContentType xContentType = XContentType.JSON; return xContentType.xContent().createParser( diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/AbstractSqlAction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/AbstractSqlAction.java new file mode 100644 index 0000000000000..7c0e4e139c3e8 --- /dev/null +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/AbstractSqlAction.java @@ -0,0 +1,64 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.sql.plugin; + +import org.elasticsearch.client.node.NodeClient; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.rest.BaseRestHandler; +import org.elasticsearch.rest.RestRequest; +import org.elasticsearch.xpack.sql.action.AbstractSqlRequest; +import org.elasticsearch.xpack.sql.proto.Mode; +import org.elasticsearch.xpack.sql.proto.RequestInfo; + +import java.io.IOException; +import java.util.Locale; + +import static org.elasticsearch.xpack.sql.proto.RequestInfo.CANVAS; +import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLI; + +public abstract class AbstractSqlAction extends BaseRestHandler { + + protected AbstractSqlAction(Settings settings) { + super(settings); + } + + @Override + public abstract String getName(); + + /* + * Parse the rest request and create a sql request out of it. + */ + protected abstract AbstractSqlRequest initializeSqlRequest(RestRequest request) throws IOException; + + /* + * Perform additional steps after the default initialization. + */ + protected abstract RestChannelConsumer doPrepareRequest(AbstractSqlRequest sqlRequest, RestRequest request, + NodeClient client) throws IOException; + + @Override + protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { + AbstractSqlRequest sqlRequest = initializeSqlRequest(request); + + // no mode specified, default to "PLAIN" + if (sqlRequest.requestInfo() == null) { + sqlRequest.requestInfo(new RequestInfo(Mode.PLAIN)); + } + + // default to no client id, unless it's CLI or CANVAS + if (sqlRequest.requestInfo().clientId() != null) { + String clientId = sqlRequest.requestInfo().clientId().toLowerCase(Locale.ROOT); + if (!clientId.equals(CLI) && !clientId.equals(CANVAS)) { + clientId = null; + } + sqlRequest.requestInfo().clientId(clientId); + } + + return doPrepareRequest(sqlRequest, request, client); + } + +} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlClearCursorAction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlClearCursorAction.java index b6a8592af185f..4525b7378551b 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlClearCursorAction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlClearCursorAction.java @@ -11,24 +11,19 @@ import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.action.RestToXContentListener; +import org.elasticsearch.xpack.sql.action.AbstractSqlRequest; import org.elasticsearch.xpack.sql.action.SqlClearCursorAction; import org.elasticsearch.xpack.sql.action.SqlClearCursorRequest; -import org.elasticsearch.xpack.sql.proto.Mode; import org.elasticsearch.xpack.sql.proto.Protocol; -import org.elasticsearch.xpack.sql.proto.RequestInfo; import java.io.IOException; -import java.util.Locale; import static org.elasticsearch.rest.RestRequest.Method.POST; -import static org.elasticsearch.xpack.sql.proto.RequestInfo.CANVAS; -import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLI; -public class RestSqlClearCursorAction extends BaseRestHandler { +public class RestSqlClearCursorAction extends AbstractSqlAction { private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(RestSqlClearCursorAction.class)); @@ -39,25 +34,20 @@ public class RestSqlClearCursorAction extends BaseRestHandler { POST, Protocol.CLEAR_CURSOR_REST_ENDPOINT, this, POST, Protocol.CLEAR_CURSOR_DEPRECATED_REST_ENDPOINT, deprecationLogger); } - + @Override - protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { + protected AbstractSqlRequest initializeSqlRequest(RestRequest request) throws IOException { SqlClearCursorRequest sqlRequest; - try (XContentParser parser = request.contentOrSourceParamParser()) { + try (XContentParser parser = request.contentParser()) { sqlRequest = SqlClearCursorRequest.fromXContent(parser); } - // no mode specified, default to "PLAIN" - if (sqlRequest.requestInfo() == null) { - sqlRequest.requestInfo(new RequestInfo(Mode.PLAIN)); - } - if (sqlRequest.requestInfo().clientId() != null) { - String clientId = sqlRequest.requestInfo().clientId().toLowerCase(Locale.ROOT); - if (!clientId.equals(CLI) && !clientId.equals(CANVAS)) { - clientId = null; - } - sqlRequest.requestInfo().clientId(clientId); - } + return sqlRequest; + } + + @Override + protected RestChannelConsumer doPrepareRequest(AbstractSqlRequest sqlRequest, RestRequest request, NodeClient client) + throws IOException { return channel -> client.executeLocally(SqlClearCursorAction.INSTANCE, sqlRequest, new RestToXContentListener<>(channel)); } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java index 7dcf1df08e9f0..898c74660d615 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java @@ -14,32 +14,27 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; -import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.BytesRestResponse; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.RestResponse; import org.elasticsearch.rest.RestStatus; import org.elasticsearch.rest.action.RestResponseListener; +import org.elasticsearch.xpack.sql.action.AbstractSqlRequest; import org.elasticsearch.xpack.sql.action.SqlQueryAction; import org.elasticsearch.xpack.sql.action.SqlQueryRequest; import org.elasticsearch.xpack.sql.action.SqlQueryResponse; -import org.elasticsearch.xpack.sql.proto.Mode; import org.elasticsearch.xpack.sql.proto.Protocol; -import org.elasticsearch.xpack.sql.proto.RequestInfo; import org.elasticsearch.xpack.sql.session.Cursor; import org.elasticsearch.xpack.sql.session.Cursors; import java.io.IOException; import java.nio.charset.StandardCharsets; -import java.util.Locale; import static org.elasticsearch.rest.RestRequest.Method.GET; import static org.elasticsearch.rest.RestRequest.Method.POST; -import static org.elasticsearch.xpack.sql.proto.RequestInfo.CANVAS; -import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLI; -public class RestSqlQueryAction extends BaseRestHandler { +public class RestSqlQueryAction extends AbstractSqlAction { private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(RestSqlQueryAction.class)); @@ -54,27 +49,20 @@ public class RestSqlQueryAction extends BaseRestHandler { POST, Protocol.SQL_QUERY_REST_ENDPOINT, this, POST, Protocol.SQL_QUERY_DEPRECATED_REST_ENDPOINT, deprecationLogger); } - + @Override - protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { + protected AbstractSqlRequest initializeSqlRequest(RestRequest request) throws IOException { SqlQueryRequest sqlRequest; try (XContentParser parser = request.contentParser()) { sqlRequest = SqlQueryRequest.fromXContent(parser); } - // no mode specified, default to "PLAIN" - if (sqlRequest.requestInfo() == null) { - sqlRequest.requestInfo(new RequestInfo(Mode.PLAIN)); - } - - if (sqlRequest.requestInfo().clientId() != null) { - String clientId = sqlRequest.requestInfo().clientId().toLowerCase(Locale.ROOT); - if (!clientId.equals(CLI) && !clientId.equals(CANVAS)) { - clientId = null; - } - sqlRequest.requestInfo().clientId(clientId); - } + return sqlRequest; + } + @Override + protected RestChannelConsumer doPrepareRequest(AbstractSqlRequest sqlRequest, RestRequest request, NodeClient client) + throws IOException { /* * Since we support {@link TextFormat} and * {@link XContent} outputs we can't use {@link RestToXContentListener} @@ -125,7 +113,7 @@ public RestResponse buildResponse(SqlQueryResponse response) throws Exception { return channel -> client.execute(SqlQueryAction.INSTANCE, sqlRequest, new RestResponseListener(channel) { @Override public RestResponse buildResponse(SqlQueryResponse response) throws Exception { - Cursor cursor = Cursors.decodeFromString(sqlRequest.cursor()); + Cursor cursor = Cursors.decodeFromString(((SqlQueryRequest) sqlRequest).cursor()); final String data = textFormat.format(cursor, request, response); RestResponse restResponse = new BytesRestResponse(RestStatus.OK, textFormat.contentType(request), diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java index ec055cc9a0509..7abbd88d8ed41 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java @@ -10,28 +10,23 @@ import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.action.RestToXContentListener; +import org.elasticsearch.xpack.sql.action.AbstractSqlRequest; import org.elasticsearch.xpack.sql.action.SqlTranslateAction; import org.elasticsearch.xpack.sql.action.SqlTranslateRequest; -import org.elasticsearch.xpack.sql.proto.Mode; import org.elasticsearch.xpack.sql.proto.Protocol; -import org.elasticsearch.xpack.sql.proto.RequestInfo; import java.io.IOException; -import java.util.Locale; import static org.elasticsearch.rest.RestRequest.Method.GET; import static org.elasticsearch.rest.RestRequest.Method.POST; -import static org.elasticsearch.xpack.sql.proto.RequestInfo.CANVAS; -import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLI; /** * REST action for translating SQL queries into ES requests */ -public class RestSqlTranslateAction extends BaseRestHandler { +public class RestSqlTranslateAction extends AbstractSqlAction { private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(RestSqlTranslateAction.class)); @@ -46,27 +41,20 @@ public RestSqlTranslateAction(Settings settings, RestController controller) { POST, Protocol.SQL_TRANSLATE_REST_ENDPOINT, this, POST, Protocol.SQL_TRANSLATE_DEPRECATED_REST_ENDPOINT, deprecationLogger); } - + @Override - protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { + protected AbstractSqlRequest initializeSqlRequest(RestRequest request) throws IOException { SqlTranslateRequest sqlRequest; - try (XContentParser parser = request.contentOrSourceParamParser()) { + try (XContentParser parser = request.contentParser()) { sqlRequest = SqlTranslateRequest.fromXContent(parser); } - // no mode specified, default to "PLAIN" - if (sqlRequest.requestInfo() == null) { - sqlRequest.requestInfo(new RequestInfo(Mode.PLAIN)); - } - - if (sqlRequest.requestInfo().clientId() != null) { - String clientId = sqlRequest.requestInfo().clientId().toLowerCase(Locale.ROOT); - if (!clientId.equals(CLI) && !clientId.equals(CANVAS)) { - clientId = null; - } - sqlRequest.requestInfo().clientId(clientId); - } - + return sqlRequest; + } + + @Override + protected RestChannelConsumer doPrepareRequest(AbstractSqlRequest sqlRequest, RestRequest request, NodeClient client) + throws IOException { return channel -> client.executeLocally(SqlTranslateAction.INSTANCE, sqlRequest, new RestToXContentListener<>(channel)); } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlField.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlField.java deleted file mode 100644 index 58c25c101aebe..0000000000000 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/SqlField.java +++ /dev/null @@ -1,16 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ - -package org.elasticsearch.xpack.sql.plugin; - -import org.elasticsearch.common.ParseField; - -final class SqlField { - public static final ParseField MODE = new ParseField("mode"); - public static final ParseField CLIENT_ID = new ParseField("client.id"); - - private SqlField() {} -} From 5720e44ae3e96ae900fc31d0dcd7a6451970f041 Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Wed, 5 Dec 2018 09:46:36 +0200 Subject: [PATCH 4/8] Addressed reviews --- .../sql/qa/rest/RestSqlUsageTestCase.java | 6 +- .../sql/action/AbstractSqlQueryRequest.java | 19 +----- .../xpack/sql/action/AbstractSqlRequest.java | 2 +- .../sql/action/SqlClearCursorRequest.java | 5 +- .../xpack/sql/action/SqlQueryRequest.java | 1 + .../xpack/sql/action/SqlTranslateRequest.java | 1 + .../action/SqlClearCursorRequestTests.java | 7 +- .../sql/action/SqlRequestParsersTests.java | 26 ++++++-- .../sql/client/ConnectionConfiguration.java | 2 +- .../xpack/sql/client/HttpClient.java | 4 +- .../xpack/sql/proto/RequestInfo.java | 11 +++- .../sql/proto/SqlClearCursorRequest.java | 2 +- .../xpack/sql/proto/SqlQueryRequest.java | 2 +- .../xpack/sql/plugin/AbstractSqlAction.java | 64 ------------------- .../sql/plugin/RestSqlClearCursorAction.java | 15 ++--- .../xpack/sql/plugin/RestSqlQueryAction.java | 17 ++--- .../sql/plugin/RestSqlTranslateAction.java | 15 ++--- 17 files changed, 64 insertions(+), 135 deletions(-) delete mode 100644 x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/AbstractSqlAction.java diff --git a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlUsageTestCase.java b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlUsageTestCase.java index a5ec59aba26f0..5e0d06ff571b3 100644 --- a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlUsageTestCase.java +++ b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlUsageTestCase.java @@ -66,11 +66,11 @@ private void getBaseMetrics() throws UnsupportedOperationException, IOException Map baseStats = getStats(); List>> nodesListStats = (List) baseStats.get("stats"); - // used for "client.id" request parameter value, but also for getting the stats from ES + // used for "client_id" request parameter value, but also for getting the stats from ES clientType = randomFrom(ClientType.values()).toString(); ignoreClientType = randomBoolean(); - // "client.id" parameter will not be sent in the requests + // "client_id" parameter will not be sent in the requests // and "clientType" will only be used for getting the stats back from ES if (ignoreClientType) { clientType = ClientType.REST.toString(); @@ -283,7 +283,7 @@ private void runSql(String mode, String restClient, String sql) throws IOExcepti request.setOptions(options); } request.setEntity(new StringEntity("{\"query\":\"" + sql + "\"" + mode(mode) + - (ignoreClientType ? "" : ",\"client.id\":\"" + restClient + "\"") + "}", + (ignoreClientType ? "" : ",\"client_id\":\"" + restClient + "\"") + "}", ContentType.APPLICATION_JSON)); client().performRequest(request); } diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java index 56bd6910ea1d4..467cb65c6f488 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java @@ -61,21 +61,8 @@ protected static ObjectParser objec // TODO: convert this into ConstructingObjectParser ObjectParser parser = new ObjectParser<>("sql/query", false, supplier); parser.declareString(AbstractSqlQueryRequest::query, SqlRequestField.QUERY); - parser.declareString((request, mode) -> { - Mode m = Mode.fromString(mode); - if (request.requestInfo() != null) { - request.requestInfo().mode(m); - } else { - request.requestInfo(new RequestInfo(m)); - } - }, SqlRequestField.MODE); - parser.declareString((request, clientId) -> { - if (request.requestInfo() != null) { - request.requestInfo().clientId(clientId); - } else { - request.requestInfo(new RequestInfo(Mode.PLAIN, clientId)); - } - }, SqlRequestField.CLIENT_ID); + parser.declareString((request, mode) -> request.mode(Mode.fromString(mode)), SqlRequestField.MODE); + parser.declareString((request, clientId) -> request.clientId(clientId), SqlRequestField.CLIENT_ID); parser.declareObjectArray(AbstractSqlQueryRequest::params, (p, c) -> SqlTypedParamValue.fromXContent(p), SqlRequestField.PARAMS); parser.declareString((request, zoneId) -> request.timeZone(TimeZone.getTimeZone(zoneId)), SqlRequestField.TIME_ZONE); parser.declareInt(AbstractSqlQueryRequest::fetchSize, SqlRequestField.FETCH_SIZE); @@ -262,6 +249,6 @@ public interface SqlRequestField { ParseField PAGE_TIMEOUT = new ParseField("page_timeout"); ParseField FILTER = new ParseField("filter"); ParseField MODE = new ParseField("mode"); - ParseField CLIENT_ID = new ParseField("client.id"); + ParseField CLIENT_ID = new ParseField("client_id"); } } diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlRequest.java index 9dd9bf40bfa9d..ea5cce74ed392 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlRequest.java @@ -28,7 +28,7 @@ public abstract class AbstractSqlRequest extends ActionRequest implements ToXCon private RequestInfo requestInfo; protected AbstractSqlRequest() { - + this.requestInfo = new RequestInfo(Mode.PLAIN); } protected AbstractSqlRequest(RequestInfo requestInfo) { diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java index 84b69ef4691ca..909a3e76d9b65 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java @@ -19,9 +19,9 @@ import static org.elasticsearch.action.ValidateActions.addValidationError; import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg; +import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.CLIENT_ID; import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.CURSOR; import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.MODE; -import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.CLIENT_ID; /** * Request to clean all SQL resources associated with the cursor @@ -29,7 +29,7 @@ public class SqlClearCursorRequest extends AbstractSqlRequest { private static final ConstructingObjectParser PARSER = - // here the position in "objects" is the same as the fields parser declarations below + // here the position in "objects" is the same as the fields parser declarations below new ConstructingObjectParser<>(SqlClearCursorAction.NAME, objects -> { RequestInfo requestInfo = new RequestInfo(Mode.fromString((String) objects[1]), (String) objects[2]); @@ -46,6 +46,7 @@ public class SqlClearCursorRequest extends AbstractSqlRequest { private String cursor; public SqlClearCursorRequest() { + super(); } public SqlClearCursorRequest(RequestInfo requestInfo, String cursor) { diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java index ed40f0110f482..72030df51e0a9 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java @@ -38,6 +38,7 @@ public class SqlQueryRequest extends AbstractSqlQueryRequest { private String cursor = ""; public SqlQueryRequest() { + super(); } public SqlQueryRequest(String query, List params, QueryBuilder filter, TimeZone timeZone, diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlTranslateRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlTranslateRequest.java index c8015be462358..4522c938f8a5e 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlTranslateRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlTranslateRequest.java @@ -30,6 +30,7 @@ public class SqlTranslateRequest extends AbstractSqlQueryRequest { private static final ObjectParser PARSER = objectParser(SqlTranslateRequest::new); public SqlTranslateRequest() { + super(); } public SqlTranslateRequest(String query, List params, QueryBuilder filter, TimeZone timeZone, diff --git a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequestTests.java b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequestTests.java index 72f0f4f8dc359..ca9c450748e34 100644 --- a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequestTests.java +++ b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequestTests.java @@ -21,12 +21,11 @@ public class SqlClearCursorRequestTests extends AbstractSerializingTestCase { public RequestInfo requestInfo; - public String clientId; @Before public void setup() { - clientId = randomFrom(CLI, CANVAS, randomAlphaOfLengthBetween(10, 20)); - requestInfo = new RequestInfo(randomFrom(Mode.values()), clientId); + requestInfo = new RequestInfo(randomFrom(Mode.values()), + randomFrom(CLI, CANVAS, randomAlphaOfLengthBetween(10, 20))); } @Override @@ -45,7 +44,7 @@ protected SqlClearCursorRequest doParseInstance(XContentParser parser) { } private RequestInfo randomRequestInfo() { - return new RequestInfo(randomFrom(Mode.values()), randomFrom(CLI, CANVAS, clientId)); + return new RequestInfo(randomFrom(Mode.values()), randomFrom(CLI, CANVAS, requestInfo.clientId())); } @Override diff --git a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java index defda7e9a1574..ffde176d1e2e2 100644 --- a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java +++ b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java @@ -35,15 +35,15 @@ public void testClearCursorRequestParser() throws IOException { assertParsingErrorMessage("{\"cursor\" : \"whatever\", \"fetch_size\":123}", "unknown field [fetch_size]", SqlClearCursorRequest::fromXContent); - SqlClearCursorRequest request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"jdbc\", \"client.id\" : \"bla\"}", + SqlClearCursorRequest request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"jdbc\", \"client_id\" : \"bla\"}", SqlClearCursorRequest::fromXContent); - assertEquals("bla", request.clientId()); + assertNull(request.clientId()); assertEquals("jdbc", request.mode().toString()); assertEquals("whatever", request.getCursor()); - request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"some foo mode\", \"client.id\" : \"bla\"}", + request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"some foo mode\", \"client_id\" : \"bla\"}", SqlClearCursorRequest::fromXContent); - assertEquals("bla", request.clientId()); + assertNull(request.clientId()); assertEquals("plain", request.mode().toString()); assertEquals("whatever", request.getCursor()); @@ -51,6 +51,18 @@ public void testClearCursorRequestParser() throws IOException { assertNull(request.clientId()); assertEquals("plain", request.mode().toString()); assertEquals("whatever", request.getCursor()); + + request = generateRequest("{\"cursor\" : \"whatever\", \"client_id\" : \"CLI\"}", + SqlClearCursorRequest::fromXContent); + assertEquals("cli", request.clientId()); + assertEquals("plain", request.mode().toString()); + assertEquals("whatever", request.getCursor()); + + request = generateRequest("{\"cursor\" : \"whatever\", \"client_id\" : \"cANVAs\"}", + SqlClearCursorRequest::fromXContent); + assertEquals("canvas", request.clientId()); + assertEquals("plain", request.mode().toString()); + assertEquals("whatever", request.getCursor()); } public void testTranslateRequestParser() throws IOException { @@ -64,16 +76,16 @@ public void testQueryRequestParser() throws IOException { assertParsingErrorMessage("{\"mode\" : 123}", "mode doesn't support values of type: VALUE_NUMBER", SqlQueryRequest::fromXContent); assertParsingErrorMessage("{\"cursor\" : \"whatever\", \"fetch_size\":\"abc\"}", "failed to parse field [fetch_size]", SqlQueryRequest::fromXContent); - assertParsingErrorMessage("{\"client.id\":123}", "client.id doesn't support values of type: VALUE_NUMBER", + assertParsingErrorMessage("{\"client_id\":123}", "client_id doesn't support values of type: VALUE_NUMBER", SqlQueryRequest::fromXContent); assertParsingErrorMessage("{\"params\":[{\"value\":123}]}", "failed to parse field [params]", SqlQueryRequest::fromXContent); assertParsingErrorMessage("{\"time_zone\":12}", "time_zone doesn't support values of type: VALUE_NUMBER", SqlQueryRequest::fromXContent); - SqlQueryRequest request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"jdbc\", \"client.id\" : \"bla\"," + SqlQueryRequest request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"jdbc\", \"client_id\" : \"bla\"," + "\"query\":\"select\",\"params\":[{\"value\":123, \"type\":\"whatever\"}], \"time_zone\":\"UTC\"," + "\"request_timeout\":\"5s\",\"page_timeout\":\"10s\"}", SqlQueryRequest::fromXContent); - assertEquals("bla", request.clientId()); + assertNull(request.clientId()); assertEquals("jdbc", request.mode().toString()); assertEquals("whatever", request.cursor()); assertEquals("select", request.query()); diff --git a/x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/ConnectionConfiguration.java b/x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/ConnectionConfiguration.java index 00ecd8246221d..c50d1da820edb 100644 --- a/x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/ConnectionConfiguration.java +++ b/x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/ConnectionConfiguration.java @@ -50,7 +50,7 @@ public class ConnectionConfiguration { public static final String PAGE_SIZE = "page.size"; private static final String PAGE_SIZE_DEFAULT = "1000"; - public static final String CLIENT_ID = "client.id"; + public static final String CLIENT_ID = "client_id"; // Auth diff --git a/x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/HttpClient.java b/x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/HttpClient.java index 3d48fee51f2ff..096ebb64e52bf 100644 --- a/x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/HttpClient.java +++ b/x-pack/plugin/sql/sql-client/src/main/java/org/elasticsearch/xpack/sql/client/HttpClient.java @@ -65,7 +65,7 @@ public MainResponse serverInfo() throws SQLException { public SqlQueryResponse queryInit(String query, int fetchSize) throws SQLException { // TODO allow customizing the time zone - this is what session set/reset/get should be about - // method called only from CLI. "client.id" is set to "cli" + // method called only from CLI. "client_id" is set to "cli" SqlQueryRequest sqlRequest = new SqlQueryRequest(query, Collections.emptyList(), null, TimeZone.getTimeZone("UTC"), fetchSize, TimeValue.timeValueMillis(cfg.queryTimeout()), TimeValue.timeValueMillis(cfg.pageTimeout()), new RequestInfo(Mode.PLAIN, CLI)); @@ -77,7 +77,7 @@ public SqlQueryResponse query(SqlQueryRequest sqlRequest) throws SQLException { } public SqlQueryResponse nextPage(String cursor) throws SQLException { - // method called only from CLI. "client.id" is set to "cli" + // method called only from CLI. "client_id" is set to "cli" SqlQueryRequest sqlRequest = new SqlQueryRequest(cursor, TimeValue.timeValueMillis(cfg.queryTimeout()), TimeValue.timeValueMillis(cfg.pageTimeout()), new RequestInfo(Mode.PLAIN, CLI)); return post(Protocol.SQL_QUERY_REST_ENDPOINT, sqlRequest, SqlQueryResponse::fromXContent); diff --git a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/RequestInfo.java b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/RequestInfo.java index 03a388359d988..e22f3d82ec6a2 100644 --- a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/RequestInfo.java +++ b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/RequestInfo.java @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.sql.proto; +import java.util.Locale; import java.util.Objects; public class RequestInfo { @@ -20,8 +21,8 @@ public RequestInfo(Mode mode) { } public RequestInfo(Mode mode, String clientId) { - this.mode = mode; - this.clientId = clientId; + mode(mode); + clientId(clientId); } public Mode mode() { @@ -37,6 +38,12 @@ public String clientId() { } public void clientId(String clientId) { + if (clientId != null) { + clientId = clientId.toLowerCase(Locale.ROOT); + if (!clientId.equals(CLI) && !clientId.equals(CANVAS)) { + clientId = null; + } + } this.clientId = clientId; } diff --git a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlClearCursorRequest.java b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlClearCursorRequest.java index a3a8ef186e599..10ada2ca84638 100644 --- a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlClearCursorRequest.java +++ b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlClearCursorRequest.java @@ -48,7 +48,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.field("mode", mode().toString()); } if (clientId() != null) { - builder.field("client.id", clientId()); + builder.field("client_id", clientId()); } } return builder; diff --git a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlQueryRequest.java b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlQueryRequest.java index 98ddcedb86ce4..abb9430efd4cc 100644 --- a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlQueryRequest.java +++ b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlQueryRequest.java @@ -145,7 +145,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.field("mode", mode().toString()); } if (clientId() != null) { - builder.field("client.id", clientId()); + builder.field("client_id", clientId()); } } if (this.params.isEmpty() == false) { diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/AbstractSqlAction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/AbstractSqlAction.java deleted file mode 100644 index 7c0e4e139c3e8..0000000000000 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/AbstractSqlAction.java +++ /dev/null @@ -1,64 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ - -package org.elasticsearch.xpack.sql.plugin; - -import org.elasticsearch.client.node.NodeClient; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.rest.BaseRestHandler; -import org.elasticsearch.rest.RestRequest; -import org.elasticsearch.xpack.sql.action.AbstractSqlRequest; -import org.elasticsearch.xpack.sql.proto.Mode; -import org.elasticsearch.xpack.sql.proto.RequestInfo; - -import java.io.IOException; -import java.util.Locale; - -import static org.elasticsearch.xpack.sql.proto.RequestInfo.CANVAS; -import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLI; - -public abstract class AbstractSqlAction extends BaseRestHandler { - - protected AbstractSqlAction(Settings settings) { - super(settings); - } - - @Override - public abstract String getName(); - - /* - * Parse the rest request and create a sql request out of it. - */ - protected abstract AbstractSqlRequest initializeSqlRequest(RestRequest request) throws IOException; - - /* - * Perform additional steps after the default initialization. - */ - protected abstract RestChannelConsumer doPrepareRequest(AbstractSqlRequest sqlRequest, RestRequest request, - NodeClient client) throws IOException; - - @Override - protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { - AbstractSqlRequest sqlRequest = initializeSqlRequest(request); - - // no mode specified, default to "PLAIN" - if (sqlRequest.requestInfo() == null) { - sqlRequest.requestInfo(new RequestInfo(Mode.PLAIN)); - } - - // default to no client id, unless it's CLI or CANVAS - if (sqlRequest.requestInfo().clientId() != null) { - String clientId = sqlRequest.requestInfo().clientId().toLowerCase(Locale.ROOT); - if (!clientId.equals(CLI) && !clientId.equals(CANVAS)) { - clientId = null; - } - sqlRequest.requestInfo().clientId(clientId); - } - - return doPrepareRequest(sqlRequest, request, client); - } - -} diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlClearCursorAction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlClearCursorAction.java index 4525b7378551b..00e316ffabe94 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlClearCursorAction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlClearCursorAction.java @@ -11,10 +11,10 @@ import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.action.RestToXContentListener; -import org.elasticsearch.xpack.sql.action.AbstractSqlRequest; import org.elasticsearch.xpack.sql.action.SqlClearCursorAction; import org.elasticsearch.xpack.sql.action.SqlClearCursorRequest; import org.elasticsearch.xpack.sql.proto.Protocol; @@ -23,7 +23,7 @@ import static org.elasticsearch.rest.RestRequest.Method.POST; -public class RestSqlClearCursorAction extends AbstractSqlAction { +public class RestSqlClearCursorAction extends BaseRestHandler { private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(RestSqlClearCursorAction.class)); @@ -34,20 +34,15 @@ public class RestSqlClearCursorAction extends AbstractSqlAction { POST, Protocol.CLEAR_CURSOR_REST_ENDPOINT, this, POST, Protocol.CLEAR_CURSOR_DEPRECATED_REST_ENDPOINT, deprecationLogger); } - + @Override - protected AbstractSqlRequest initializeSqlRequest(RestRequest request) throws IOException { + protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) + throws IOException { SqlClearCursorRequest sqlRequest; try (XContentParser parser = request.contentParser()) { sqlRequest = SqlClearCursorRequest.fromXContent(parser); } - return sqlRequest; - } - - @Override - protected RestChannelConsumer doPrepareRequest(AbstractSqlRequest sqlRequest, RestRequest request, NodeClient client) - throws IOException { return channel -> client.executeLocally(SqlClearCursorAction.INSTANCE, sqlRequest, new RestToXContentListener<>(channel)); } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java index 898c74660d615..e96f3c960c473 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java @@ -14,13 +14,13 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.BytesRestResponse; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.RestResponse; import org.elasticsearch.rest.RestStatus; import org.elasticsearch.rest.action.RestResponseListener; -import org.elasticsearch.xpack.sql.action.AbstractSqlRequest; import org.elasticsearch.xpack.sql.action.SqlQueryAction; import org.elasticsearch.xpack.sql.action.SqlQueryRequest; import org.elasticsearch.xpack.sql.action.SqlQueryResponse; @@ -34,7 +34,7 @@ import static org.elasticsearch.rest.RestRequest.Method.GET; import static org.elasticsearch.rest.RestRequest.Method.POST; -public class RestSqlQueryAction extends AbstractSqlAction { +public class RestSqlQueryAction extends BaseRestHandler { private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(RestSqlQueryAction.class)); @@ -49,20 +49,15 @@ public class RestSqlQueryAction extends AbstractSqlAction { POST, Protocol.SQL_QUERY_REST_ENDPOINT, this, POST, Protocol.SQL_QUERY_DEPRECATED_REST_ENDPOINT, deprecationLogger); } - + @Override - protected AbstractSqlRequest initializeSqlRequest(RestRequest request) throws IOException { + protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) + throws IOException { SqlQueryRequest sqlRequest; try (XContentParser parser = request.contentParser()) { sqlRequest = SqlQueryRequest.fromXContent(parser); } - return sqlRequest; - } - - @Override - protected RestChannelConsumer doPrepareRequest(AbstractSqlRequest sqlRequest, RestRequest request, NodeClient client) - throws IOException { /* * Since we support {@link TextFormat} and * {@link XContent} outputs we can't use {@link RestToXContentListener} @@ -113,7 +108,7 @@ public RestResponse buildResponse(SqlQueryResponse response) throws Exception { return channel -> client.execute(SqlQueryAction.INSTANCE, sqlRequest, new RestResponseListener(channel) { @Override public RestResponse buildResponse(SqlQueryResponse response) throws Exception { - Cursor cursor = Cursors.decodeFromString(((SqlQueryRequest) sqlRequest).cursor()); + Cursor cursor = Cursors.decodeFromString(sqlRequest.cursor()); final String data = textFormat.format(cursor, request, response); RestResponse restResponse = new BytesRestResponse(RestStatus.OK, textFormat.contentType(request), diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java index 7abbd88d8ed41..6fb7e7d70602f 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java @@ -10,10 +10,10 @@ import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.rest.BaseRestHandler; import org.elasticsearch.rest.RestController; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.action.RestToXContentListener; -import org.elasticsearch.xpack.sql.action.AbstractSqlRequest; import org.elasticsearch.xpack.sql.action.SqlTranslateAction; import org.elasticsearch.xpack.sql.action.SqlTranslateRequest; import org.elasticsearch.xpack.sql.proto.Protocol; @@ -26,7 +26,7 @@ /** * REST action for translating SQL queries into ES requests */ -public class RestSqlTranslateAction extends AbstractSqlAction { +public class RestSqlTranslateAction extends BaseRestHandler { private static final DeprecationLogger deprecationLogger = new DeprecationLogger(LogManager.getLogger(RestSqlTranslateAction.class)); @@ -41,20 +41,15 @@ public RestSqlTranslateAction(Settings settings, RestController controller) { POST, Protocol.SQL_TRANSLATE_REST_ENDPOINT, this, POST, Protocol.SQL_TRANSLATE_DEPRECATED_REST_ENDPOINT, deprecationLogger); } - + @Override - protected AbstractSqlRequest initializeSqlRequest(RestRequest request) throws IOException { + protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) + throws IOException { SqlTranslateRequest sqlRequest; try (XContentParser parser = request.contentParser()) { sqlRequest = SqlTranslateRequest.fromXContent(parser); } - return sqlRequest; - } - - @Override - protected RestChannelConsumer doPrepareRequest(AbstractSqlRequest sqlRequest, RestRequest request, NodeClient client) - throws IOException { return channel -> client.executeLocally(SqlTranslateAction.INSTANCE, sqlRequest, new RestToXContentListener<>(channel)); } From e8298c582009c64f0e5dcf63d30ef6a51d459e25 Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Wed, 5 Dec 2018 16:38:29 +0200 Subject: [PATCH 5/8] Further polishing --- .../xpack/sql/qa/rest/RestSqlTestCase.java | 3 +- .../sql/action/AbstractSqlQueryRequest.java | 45 +++++++++-------- .../sql/action/SqlClearCursorRequest.java | 6 +-- .../xpack/sql/action/SqlQueryRequest.java | 1 - .../xpack/sql/action/SqlQueryResponse.java | 2 +- .../action/SqlClearCursorRequestTests.java | 7 ++- .../sql/action/SqlQueryRequestTests.java | 10 ++-- .../sql/action/SqlQueryResponseTests.java | 2 +- .../sql/action/SqlRequestParsersTests.java | 48 +++++++++++++++---- .../elasticsearch/xpack/sql/proto/Mode.java | 5 +- .../xpack/sql/proto/RequestInfo.java | 7 ++- .../sql/proto/SqlClearCursorRequest.java | 10 ++-- .../xpack/sql/proto/SqlQueryRequest.java | 10 ++-- 13 files changed, 88 insertions(+), 68 deletions(-) diff --git a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java index a90ac79d9e93e..e3b7a9d2a19a8 100644 --- a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java +++ b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java @@ -10,6 +10,7 @@ import org.apache.http.HttpEntity; import org.apache.http.entity.ContentType; import org.apache.http.entity.StringEntity; +import org.apache.logging.log4j.util.Strings; import org.elasticsearch.client.Request; import org.elasticsearch.client.RequestOptions; import org.elasticsearch.client.Response; @@ -713,7 +714,7 @@ public static String randomMode() { } public static String mode(String mode) { - return mode.isEmpty() ? "" : ",\"mode\":\"" + mode + "\""; + return Strings.isEmpty(mode) ? "" : ",\"mode\":\"" + mode + "\""; } private void index(String... docs) throws IOException { diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java index 467cb65c6f488..2b90a7d41fa2c 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java @@ -40,6 +40,17 @@ public abstract class AbstractSqlQueryRequest extends AbstractSqlRequest impleme @Nullable private QueryBuilder filter = null; private List params = Collections.emptyList(); + + static final ParseField QUERY = new ParseField("query"); + static final ParseField CURSOR = new ParseField("cursor"); + static final ParseField PARAMS = new ParseField("params"); + static final ParseField TIME_ZONE = new ParseField("time_zone"); + static final ParseField FETCH_SIZE = new ParseField("fetch_size"); + static final ParseField REQUEST_TIMEOUT = new ParseField("request_timeout"); + static final ParseField PAGE_TIMEOUT = new ParseField("page_timeout"); + static final ParseField FILTER = new ParseField("filter"); + static final ParseField MODE = new ParseField("mode"); + static final ParseField CLIENT_ID = new ParseField("client_id"); public AbstractSqlQueryRequest() { super(); @@ -58,21 +69,22 @@ public AbstractSqlQueryRequest(String query, List params, Qu } protected static ObjectParser objectParser(Supplier supplier) { - // TODO: convert this into ConstructingObjectParser + // Using an ObjectParser here (vs. ConstructingObjectParser) because the latter needs to instantiate a concrete class + // and we would duplicate the code from this class to its subclasses ObjectParser parser = new ObjectParser<>("sql/query", false, supplier); - parser.declareString(AbstractSqlQueryRequest::query, SqlRequestField.QUERY); - parser.declareString((request, mode) -> request.mode(Mode.fromString(mode)), SqlRequestField.MODE); - parser.declareString((request, clientId) -> request.clientId(clientId), SqlRequestField.CLIENT_ID); - parser.declareObjectArray(AbstractSqlQueryRequest::params, (p, c) -> SqlTypedParamValue.fromXContent(p), SqlRequestField.PARAMS); - parser.declareString((request, zoneId) -> request.timeZone(TimeZone.getTimeZone(zoneId)), SqlRequestField.TIME_ZONE); - parser.declareInt(AbstractSqlQueryRequest::fetchSize, SqlRequestField.FETCH_SIZE); + parser.declareString(AbstractSqlQueryRequest::query, QUERY); + parser.declareString((request, mode) -> request.mode(Mode.fromString(mode)), MODE); + parser.declareString((request, clientId) -> request.clientId(clientId), CLIENT_ID); + parser.declareObjectArray(AbstractSqlQueryRequest::params, (p, c) -> SqlTypedParamValue.fromXContent(p), PARAMS); + parser.declareString((request, zoneId) -> request.timeZone(TimeZone.getTimeZone(zoneId)), TIME_ZONE); + parser.declareInt(AbstractSqlQueryRequest::fetchSize, FETCH_SIZE); parser.declareString((request, timeout) -> request.requestTimeout(TimeValue.parseTimeValue(timeout, Protocol.REQUEST_TIMEOUT, - "request_timeout")), SqlRequestField.REQUEST_TIMEOUT); + "request_timeout")), REQUEST_TIMEOUT); parser.declareString( (request, timeout) -> request.pageTimeout(TimeValue.parseTimeValue(timeout, Protocol.PAGE_TIMEOUT, "page_timeout")), - SqlRequestField.PAGE_TIMEOUT); + PAGE_TIMEOUT); parser.declareObject(AbstractSqlQueryRequest::filter, - (p, c) -> AbstractQueryBuilder.parseInnerQueryBuilder(p), SqlRequestField.FILTER); + (p, c) -> AbstractQueryBuilder.parseInnerQueryBuilder(p), FILTER); return parser; } @@ -238,17 +250,4 @@ public boolean equals(Object o) { public int hashCode() { return Objects.hash(super.hashCode(), query, timeZone, fetchSize, requestTimeout, pageTimeout, filter); } - - public interface SqlRequestField { - ParseField QUERY = new ParseField("query"); - ParseField CURSOR = new ParseField("cursor"); - ParseField PARAMS = new ParseField("params"); - ParseField TIME_ZONE = new ParseField("time_zone"); - ParseField FETCH_SIZE = new ParseField("fetch_size"); - ParseField REQUEST_TIMEOUT = new ParseField("request_timeout"); - ParseField PAGE_TIMEOUT = new ParseField("page_timeout"); - ParseField FILTER = new ParseField("filter"); - ParseField MODE = new ParseField("mode"); - ParseField CLIENT_ID = new ParseField("client_id"); - } } diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java index 909a3e76d9b65..02358d58c1243 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java @@ -19,9 +19,9 @@ import static org.elasticsearch.action.ValidateActions.addValidationError; import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg; -import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.CLIENT_ID; -import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.CURSOR; -import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.MODE; +import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.CURSOR; +import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.MODE; +import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.CLIENT_ID; /** * Request to clean all SQL resources associated with the cursor diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java index 72030df51e0a9..ec3e2b331f075 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java @@ -23,7 +23,6 @@ import java.util.TimeZone; import static org.elasticsearch.action.ValidateActions.addValidationError; -import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.CURSOR; /** * Request to perform an sql query diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java index d88b0c6640c69..6206879487dd6 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java @@ -23,7 +23,7 @@ import java.util.Objects; import static java.util.Collections.unmodifiableList; -import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.CURSOR; +import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.CURSOR; /** * Response to perform an sql query diff --git a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequestTests.java b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequestTests.java index ca9c450748e34..a77ded7e63ae9 100644 --- a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequestTests.java +++ b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequestTests.java @@ -15,8 +15,7 @@ import java.io.IOException; import java.util.function.Consumer; -import static org.elasticsearch.xpack.sql.proto.RequestInfo.CANVAS; -import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLI; +import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLIENT_IDS; public class SqlClearCursorRequestTests extends AbstractSerializingTestCase { @@ -25,7 +24,7 @@ public class SqlClearCursorRequestTests extends AbstractSerializingTestCase { public RequestInfo requestInfo; - public String clientId; @Before public void setup() { - clientId = randomFrom(CLI, CANVAS, randomAlphaOfLengthBetween(10, 20)); - requestInfo = new RequestInfo(randomFrom(Mode.values()), clientId); + requestInfo = new RequestInfo(randomFrom(Mode.values()), + randomFrom(randomFrom(CLIENT_IDS), randomAlphaOfLengthBetween(10, 20))); } @Override @@ -62,7 +60,7 @@ protected SqlQueryRequest createTestInstance() { } private RequestInfo randomRequestInfo() { - return new RequestInfo(randomFrom(Mode.values()), randomFrom(CLI, CANVAS, clientId)); + return new RequestInfo(randomFrom(Mode.values()), randomFrom(randomFrom(CLIENT_IDS), requestInfo.clientId())); } public List randomParameters() { diff --git a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryResponseTests.java b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryResponseTests.java index 1c4e5f42b32c0..7379e5d35bf89 100644 --- a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryResponseTests.java +++ b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryResponseTests.java @@ -26,7 +26,7 @@ import static org.elasticsearch.common.xcontent.ToXContent.EMPTY_PARAMS; import static org.hamcrest.Matchers.hasSize; -import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.CURSOR; +import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.CURSOR; public class SqlQueryResponseTests extends AbstractStreamableXContentTestCase { diff --git a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java index ffde176d1e2e2..f2153065cbd6f 100644 --- a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java +++ b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java @@ -12,6 +12,7 @@ import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.sql.proto.Mode; import org.elasticsearch.xpack.sql.proto.SqlTypedParamValue; import java.io.IOException; @@ -30,38 +31,51 @@ public void testUnknownFieldParsingErrors() throws IOException { assertParsingErrorMessage("{\"key\" : \"value\"}", "unknown field [key]", SqlTranslateRequest::fromXContent); } + public void testUnknownModeFieldParsingErrors() throws IOException { + assertParsingErrorMessageReason("{\"cursor\":\"foo\",\"mode\" : \"value\"}", + "No enum constant org.elasticsearch.xpack.sql.proto.Mode.VALUE", SqlClearCursorRequest::fromXContent); + assertParsingErrorMessageReason("{\"cursor\":\"foo\",\"mode\" : \"value\"}", + "No enum constant org.elasticsearch.xpack.sql.proto.Mode.VALUE", SqlQueryRequest::fromXContent); + assertParsingErrorMessageReason("{\"mode\" : \"value\"}", + "No enum constant org.elasticsearch.xpack.sql.proto.Mode.VALUE", SqlTranslateRequest::fromXContent); + } + public void testClearCursorRequestParser() throws IOException { assertParsingErrorMessage("{\"mode\" : \"jdbc\"}", "Required [cursor]", SqlClearCursorRequest::fromXContent); assertParsingErrorMessage("{\"cursor\" : \"whatever\", \"fetch_size\":123}", "unknown field [fetch_size]", SqlClearCursorRequest::fromXContent); + Mode randomMode = randomFrom(Mode.values()); - SqlClearCursorRequest request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"jdbc\", \"client_id\" : \"bla\"}", + SqlClearCursorRequest request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"" + + randomMode.toString() + "\", \"client_id\" : \"bla\"}", SqlClearCursorRequest::fromXContent); assertNull(request.clientId()); - assertEquals("jdbc", request.mode().toString()); + assertEquals(randomMode, request.mode()); assertEquals("whatever", request.getCursor()); - request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"some foo mode\", \"client_id\" : \"bla\"}", + randomMode = randomFrom(Mode.values()); + request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"" + + randomMode.toString() + "\", \"client_id\" : \"bla\"}", SqlClearCursorRequest::fromXContent); assertNull(request.clientId()); - assertEquals("plain", request.mode().toString()); + assertEquals(randomMode, request.mode()); assertEquals("whatever", request.getCursor()); request = generateRequest("{\"cursor\" : \"whatever\"}", SqlClearCursorRequest::fromXContent); assertNull(request.clientId()); - assertEquals("plain", request.mode().toString()); + assertEquals(Mode.PLAIN, request.mode()); assertEquals("whatever", request.getCursor()); request = generateRequest("{\"cursor\" : \"whatever\", \"client_id\" : \"CLI\"}", SqlClearCursorRequest::fromXContent); assertEquals("cli", request.clientId()); - assertEquals("plain", request.mode().toString()); + assertEquals(Mode.PLAIN, request.mode()); assertEquals("whatever", request.getCursor()); request = generateRequest("{\"cursor\" : \"whatever\", \"client_id\" : \"cANVAs\"}", SqlClearCursorRequest::fromXContent); assertEquals("canvas", request.clientId()); - assertEquals("plain", request.mode().toString()); + assertEquals(Mode.PLAIN, request.mode()); assertEquals("whatever", request.getCursor()); } @@ -70,6 +84,14 @@ public void testTranslateRequestParser() throws IOException { SqlTranslateRequest request = generateRequest("{\"query\" : \"select * from foo\"}", SqlTranslateRequest::fromXContent); assertEquals("select * from foo", request.query()); + assertEquals(Mode.PLAIN, request.mode()); + + Mode randomMode = randomFrom(Mode.values()); + request = generateRequest("{\"query\" : \"whatever\", \"client_id\" : \"foo\", \"mode\":\"" + + randomMode.toString() + "\"}", + SqlTranslateRequest::fromXContent); + assertNull(request.clientId()); + assertEquals(randomMode, request.mode()); } public void testQueryRequestParser() throws IOException { @@ -82,11 +104,13 @@ public void testQueryRequestParser() throws IOException { assertParsingErrorMessage("{\"time_zone\":12}", "time_zone doesn't support values of type: VALUE_NUMBER", SqlQueryRequest::fromXContent); - SqlQueryRequest request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"jdbc\", \"client_id\" : \"bla\"," + Mode randomMode = randomFrom(Mode.values()); + SqlQueryRequest request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"" + + randomMode.toString() + "\", \"client_id\" : \"bla\"," + "\"query\":\"select\",\"params\":[{\"value\":123, \"type\":\"whatever\"}], \"time_zone\":\"UTC\"," + "\"request_timeout\":\"5s\",\"page_timeout\":\"10s\"}", SqlQueryRequest::fromXContent); assertNull(request.clientId()); - assertEquals("jdbc", request.mode().toString()); + assertEquals(randomMode, request.mode()); assertEquals("whatever", request.cursor()); assertEquals("select", request.query()); @@ -110,6 +134,12 @@ private void assertParsingErrorMessage(String json, String errorMessage, Consume assertThat(e.getMessage(), containsString(errorMessage)); } + private void assertParsingErrorMessageReason(String json, String errorMessage, Consumer consumer) throws IOException { + XContentParser parser = parser(json); + Exception e = expectThrows(IllegalArgumentException.class, () -> consumer.accept(parser)); + assertThat(e.getCause().getMessage(), containsString(errorMessage)); + } + private XContentParser parser(String content) throws IOException { XContentType xContentType = XContentType.JSON; return xContentType.xContent().createParser( diff --git a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java index 940515f64cfbc..a301b41d3887a 100644 --- a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java +++ b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java @@ -17,11 +17,10 @@ public enum Mode { ODBC; public static Mode fromString(String mode) { - try { - return Mode.valueOf(mode.toUpperCase(Locale.ROOT)); - } catch (Exception e) { + if (mode == null || mode.isEmpty()) { return PLAIN; } + return Mode.valueOf(mode.toUpperCase(Locale.ROOT)); } diff --git a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/RequestInfo.java b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/RequestInfo.java index e22f3d82ec6a2..be1f1a58adb3b 100644 --- a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/RequestInfo.java +++ b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/RequestInfo.java @@ -6,12 +6,15 @@ package org.elasticsearch.xpack.sql.proto; +import java.util.Arrays; +import java.util.List; import java.util.Locale; import java.util.Objects; public class RequestInfo { public static final String CLI = "cli"; - public static final String CANVAS = "canvas"; + private static final String CANVAS = "canvas"; + public static final List CLIENT_IDS = Arrays.asList(CLI, CANVAS); private Mode mode; private String clientId; @@ -40,7 +43,7 @@ public String clientId() { public void clientId(String clientId) { if (clientId != null) { clientId = clientId.toLowerCase(Locale.ROOT); - if (!clientId.equals(CLI) && !clientId.equals(CANVAS)) { + if (false == CLIENT_IDS.contains(clientId)) { clientId = null; } } diff --git a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlClearCursorRequest.java b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlClearCursorRequest.java index 10ada2ca84638..b1f374e74a8da 100644 --- a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlClearCursorRequest.java +++ b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlClearCursorRequest.java @@ -43,13 +43,9 @@ public int hashCode() { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.field("cursor", cursor); - if (requestInfo() != null) { - if (mode() != null) { - builder.field("mode", mode().toString()); - } - if (clientId() != null) { - builder.field("client_id", clientId()); - } + builder.field("mode", mode().toString()); + if (clientId() != null) { + builder.field("client_id", clientId()); } return builder; } diff --git a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlQueryRequest.java b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlQueryRequest.java index abb9430efd4cc..651dc468bb909 100644 --- a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlQueryRequest.java +++ b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlQueryRequest.java @@ -140,13 +140,9 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws if (query != null) { builder.field("query", query); } - if (requestInfo() != null) { - if (mode() != null) { - builder.field("mode", mode().toString()); - } - if (clientId() != null) { - builder.field("client_id", clientId()); - } + builder.field("mode", mode().toString()); + if (clientId() != null) { + builder.field("client_id", clientId()); } if (this.params.isEmpty() == false) { builder.startArray("params"); From 945bab264983d2e3c07e0e72b43c59fe281c2b6a Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Mon, 10 Dec 2018 12:51:45 +0200 Subject: [PATCH 6/8] Cosmetic changes --- .../xpack/sql/qa/rest/RestSqlTestCase.java | 28 ++++++++++--------- .../sql/qa/rest/RestSqlUsageTestCase.java | 3 +- .../sql/action/SqlClearCursorRequest.java | 1 - .../elasticsearch/xpack/sql/proto/Mode.java | 4 ++- .../xpack/sql/proto/StringUtils.java | 2 ++ 5 files changed, 22 insertions(+), 16 deletions(-) diff --git a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java index e3b7a9d2a19a8..062ab0c81b99c 100644 --- a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java +++ b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java @@ -10,18 +10,19 @@ import org.apache.http.HttpEntity; import org.apache.http.entity.ContentType; import org.apache.http.entity.StringEntity; -import org.apache.logging.log4j.util.Strings; import org.elasticsearch.client.Request; import org.elasticsearch.client.RequestOptions; import org.elasticsearch.client.Response; import org.elasticsearch.client.ResponseException; import org.elasticsearch.common.CheckedSupplier; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.io.Streams; import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.test.NotEqualMessageBuilder; import org.elasticsearch.test.rest.ESRestTestCase; +import org.elasticsearch.xpack.sql.proto.StringUtils; import org.elasticsearch.xpack.sql.qa.ErrorsTestCase; import org.hamcrest.Matcher; @@ -101,7 +102,7 @@ public void testNextPage() throws IOException { response = runSql(new StringEntity(sqlRequest, ContentType.APPLICATION_JSON), ""); } else { response = runSql(new StringEntity("{\"cursor\":\"" + cursor + "\"" + mode(mode) + "}", - ContentType.APPLICATION_JSON), ""); + ContentType.APPLICATION_JSON), StringUtils.EMPTY); } Map expected = new HashMap<>(); @@ -122,7 +123,7 @@ public void testNextPage() throws IOException { Map expected = new HashMap<>(); expected.put("rows", emptyList()); assertResponse(expected, runSql(new StringEntity("{ \"cursor\":\"" + cursor + "\"" + mode(mode) + "}", - ContentType.APPLICATION_JSON), "")); + ContentType.APPLICATION_JSON), StringUtils.EMPTY)); } @AwaitsFix(bugUrl = "Unclear status, https://github.com/elastic/x-pack-elasticsearch/issues/2074") @@ -302,7 +303,7 @@ private void expectBadRequest(CheckedSupplier, Exception> co } private Map runSql(String mode, String sql) throws IOException { - return runSql(mode, sql, ""); + return runSql(mode, sql, StringUtils.EMPTY); } private Map runSql(String mode, String sql, String suffix) throws IOException { @@ -353,7 +354,7 @@ public void testBasicQueryWithFilter() throws IOException { expected.put("rows", singletonList(singletonList("foo"))); assertResponse(expected, runSql(new StringEntity("{\"query\":\"SELECT * FROM test\", " + "\"filter\":{\"match\": {\"test\": \"foo\"}}" + mode(mode) + "}", - ContentType.APPLICATION_JSON), "")); + ContentType.APPLICATION_JSON), StringUtils.EMPTY)); } public void testBasicQueryWithParameters() throws IOException { @@ -369,7 +370,7 @@ public void testBasicQueryWithParameters() throws IOException { expected.put("rows", singletonList(Arrays.asList("foo", 10))); assertResponse(expected, runSql(new StringEntity("{\"query\":\"SELECT test, ? param FROM test WHERE test = ?\", " + "\"params\":[{\"type\": \"integer\", \"value\": 10}, {\"type\": \"keyword\", \"value\": \"foo\"}]" - + mode(mode) + "}", ContentType.APPLICATION_JSON), "")); + + mode(mode) + "}", ContentType.APPLICATION_JSON), StringUtils.EMPTY)); } public void testBasicTranslateQueryWithFilter() throws IOException { @@ -545,10 +546,10 @@ public void testNextPageText() throws IOException { for (int i = 0; i < 20; i += 2) { Tuple response; if (i == 0) { - response = runSqlAsText("", new StringEntity(request, ContentType.APPLICATION_JSON), "text/plain"); + response = runSqlAsText(StringUtils.EMPTY, new StringEntity(request, ContentType.APPLICATION_JSON), "text/plain"); } else { - response = runSqlAsText("", new StringEntity("{\"cursor\":\"" + cursor + "\"}", ContentType.APPLICATION_JSON), - "text/plain"); + response = runSqlAsText(StringUtils.EMPTY, new StringEntity("{\"cursor\":\"" + cursor + "\"}", + ContentType.APPLICATION_JSON), "text/plain"); } StringBuilder expected = new StringBuilder(); @@ -564,7 +565,8 @@ public void testNextPageText() throws IOException { } Map expected = new HashMap<>(); expected.put("rows", emptyList()); - assertResponse(expected, runSql(new StringEntity("{\"cursor\":\"" + cursor + "\"}", ContentType.APPLICATION_JSON), "")); + assertResponse(expected, runSql(new StringEntity("{\"cursor\":\"" + cursor + "\"}", ContentType.APPLICATION_JSON), + StringUtils.EMPTY)); Map response = runSql(new StringEntity("{\"cursor\":\"" + cursor + "\"}", ContentType.APPLICATION_JSON), "/close"); @@ -632,7 +634,7 @@ public void testQueryInTSV() throws IOException { } private Tuple runSqlAsText(String sql, String accept) throws IOException { - return runSqlAsText("", new StringEntity("{\"query\":\"" + sql + "\"}", ContentType.APPLICATION_JSON), accept); + return runSqlAsText(StringUtils.EMPTY, new StringEntity("{\"query\":\"" + sql + "\"}", ContentType.APPLICATION_JSON), accept); } /** @@ -710,11 +712,11 @@ private static Map searchStats() throws IOException { } public static String randomMode() { - return randomFrom("", "jdbc", "plain"); + return randomFrom(StringUtils.EMPTY, "jdbc", "plain"); } public static String mode(String mode) { - return Strings.isEmpty(mode) ? "" : ",\"mode\":\"" + mode + "\""; + return Strings.isEmpty(mode) ? StringUtils.EMPTY : ",\"mode\":\"" + mode + "\""; } private void index(String... docs) throws IOException { diff --git a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlUsageTestCase.java b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlUsageTestCase.java index 5e0d06ff571b3..a8c45fedf2a5e 100644 --- a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlUsageTestCase.java +++ b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlUsageTestCase.java @@ -14,6 +14,7 @@ import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.test.rest.ESRestTestCase; import org.elasticsearch.xpack.sql.proto.Mode; +import org.elasticsearch.xpack.sql.proto.StringUtils; import org.elasticsearch.xpack.sql.qa.FeatureMetric; import org.junit.Before; @@ -283,7 +284,7 @@ private void runSql(String mode, String restClient, String sql) throws IOExcepti request.setOptions(options); } request.setEntity(new StringEntity("{\"query\":\"" + sql + "\"" + mode(mode) + - (ignoreClientType ? "" : ",\"client_id\":\"" + restClient + "\"") + "}", + (ignoreClientType ? StringUtils.EMPTY : ",\"client_id\":\"" + restClient + "\"") + "}", ContentType.APPLICATION_JSON)); client().performRequest(request); } diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java index 02358d58c1243..4d5cc1734333f 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java @@ -46,7 +46,6 @@ public class SqlClearCursorRequest extends AbstractSqlRequest { private String cursor; public SqlClearCursorRequest() { - super(); } public SqlClearCursorRequest(RequestInfo requestInfo, String cursor) { diff --git a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java index a301b41d3887a..caa6702533af0 100644 --- a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java +++ b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java @@ -6,6 +6,8 @@ package org.elasticsearch.xpack.sql.proto; +import org.elasticsearch.common.Strings; + import java.util.Locale; /** @@ -17,7 +19,7 @@ public enum Mode { ODBC; public static Mode fromString(String mode) { - if (mode == null || mode.isEmpty()) { + if (Strings.isEmpty(mode)) { return PLAIN; } return Mode.valueOf(mode.toUpperCase(Locale.ROOT)); diff --git a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/StringUtils.java b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/StringUtils.java index 10ece6f5a0025..93c2e2f743c97 100644 --- a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/StringUtils.java +++ b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/StringUtils.java @@ -24,6 +24,8 @@ public final class StringUtils { + public static final String EMPTY = ""; + private static final DateTimeFormatter ISO_WITH_MILLIS = new DateTimeFormatterBuilder() .parseCaseInsensitive() .append(ISO_LOCAL_DATE) From 5a1bc381fa957f8d8966a42a093682a70c7af49f Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Mon, 10 Dec 2018 13:32:08 +0200 Subject: [PATCH 7/8] Revert back Strings class usage --- .../src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java index caa6702533af0..a301b41d3887a 100644 --- a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java +++ b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java @@ -6,8 +6,6 @@ package org.elasticsearch.xpack.sql.proto; -import org.elasticsearch.common.Strings; - import java.util.Locale; /** @@ -19,7 +17,7 @@ public enum Mode { ODBC; public static Mode fromString(String mode) { - if (Strings.isEmpty(mode)) { + if (mode == null || mode.isEmpty()) { return PLAIN; } return Mode.valueOf(mode.toUpperCase(Locale.ROOT)); From 1a1f84b6b455acbc90dece8a86fb537502ee316a Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Tue, 11 Dec 2018 02:13:18 +0200 Subject: [PATCH 8/8] Handle request body in `source` parameter as well --- .../org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java | 2 +- .../elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java index e96f3c960c473..2158d0a003764 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java @@ -54,7 +54,7 @@ public class RestSqlQueryAction extends BaseRestHandler { protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { SqlQueryRequest sqlRequest; - try (XContentParser parser = request.contentParser()) { + try (XContentParser parser = request.contentOrSourceParamParser()) { sqlRequest = SqlQueryRequest.fromXContent(parser); } diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java index 6fb7e7d70602f..c066b854593d9 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlTranslateAction.java @@ -46,7 +46,7 @@ public RestSqlTranslateAction(Settings settings, RestController controller) { protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException { SqlTranslateRequest sqlRequest; - try (XContentParser parser = request.contentParser()) { + try (XContentParser parser = request.contentOrSourceParamParser()) { sqlRequest = SqlTranslateRequest.fromXContent(parser); }