Skip to content

Commit 375caf4

Browse files
astefanastefan
committed
Adressing review comments
1 parent 6c69d77 commit 375caf4

File tree

11 files changed

+160
-157
lines changed

11 files changed

+160
-157
lines changed

‎x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -60,32 +60,32 @@ public AbstractSqlQueryRequest(String query, List<SqlTypedParamValue> params, Qu
6060
protected static <R extends AbstractSqlQueryRequest> ObjectParser<R, Void> objectParser(Supplier<R> supplier) {
6161
// TODO: convert this into ConstructingObjectParser
6262
ObjectParser<R, Void> parser = new ObjectParser<>("sql/query", false, supplier);
63-
parser.declareString(AbstractSqlQueryRequest::query, Field.QUERY);
63+
parser.declareString(AbstractSqlQueryRequest::query, SqlRequestField.QUERY);
6464
parser.declareString((request, mode) -> {
6565
Mode m = Mode.fromString(mode);
6666
if (request.requestInfo() != null) {
6767
request.requestInfo().mode(m);
6868
} else {
6969
request.requestInfo(new RequestInfo(m));
7070
}
71-
}, Field.MODE);
71+
}, SqlRequestField.MODE);
7272
parser.declareString((request, clientId) -> {
7373
if (request.requestInfo() != null) {
7474
request.requestInfo().clientId(clientId);
7575
} else {
7676
request.requestInfo(new RequestInfo(Mode.PLAIN, clientId));
7777
}
78-
}, Field.CLIENT_ID);
79-
parser.declareObjectArray(AbstractSqlQueryRequest::params, (p, c) -> SqlTypedParamValue.fromXContent(p), Field.PARAMS);
80-
parser.declareString((request, zoneId) -> request.timeZone(TimeZone.getTimeZone(zoneId)), Field.TIME_ZONE);
81-
parser.declareInt(AbstractSqlQueryRequest::fetchSize, Field.FETCH_SIZE);
78+
}, SqlRequestField.CLIENT_ID);
79+
parser.declareObjectArray(AbstractSqlQueryRequest::params, (p, c) -> SqlTypedParamValue.fromXContent(p), SqlRequestField.PARAMS);
80+
parser.declareString((request, zoneId) -> request.timeZone(TimeZone.getTimeZone(zoneId)), SqlRequestField.TIME_ZONE);
81+
parser.declareInt(AbstractSqlQueryRequest::fetchSize, SqlRequestField.FETCH_SIZE);
8282
parser.declareString((request, timeout) -> request.requestTimeout(TimeValue.parseTimeValue(timeout, Protocol.REQUEST_TIMEOUT,
83-
"request_timeout")), Field.REQUEST_TIMEOUT);
83+
"request_timeout")), SqlRequestField.REQUEST_TIMEOUT);
8484
parser.declareString(
8585
(request, timeout) -> request.pageTimeout(TimeValue.parseTimeValue(timeout, Protocol.PAGE_TIMEOUT, "page_timeout")),
86-
Field.PAGE_TIMEOUT);
86+
SqlRequestField.PAGE_TIMEOUT);
8787
parser.declareObject(AbstractSqlQueryRequest::filter,
88-
(p, c) -> AbstractQueryBuilder.parseInnerQueryBuilder(p), Field.FILTER);
88+
(p, c) -> AbstractQueryBuilder.parseInnerQueryBuilder(p), SqlRequestField.FILTER);
8989
return parser;
9090
}
9191

@@ -252,7 +252,7 @@ public int hashCode() {
252252
return Objects.hash(super.hashCode(), query, timeZone, fetchSize, requestTimeout, pageTimeout, filter);
253253
}
254254

255-
public interface Field {
255+
public interface SqlRequestField {
256256
ParseField QUERY = new ParseField("query");
257257
ParseField CURSOR = new ParseField("cursor");
258258
ParseField PARAMS = new ParseField("params");

‎x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,9 @@
1919

2020
import static org.elasticsearch.action.ValidateActions.addValidationError;
2121
import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
22+
import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.CURSOR;
23+
import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.MODE;
24+
import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.CLIENT_ID;
2225

2326
/**
2427
* Request to clean all SQL resources associated with the cursor
@@ -35,9 +38,9 @@ public class SqlClearCursorRequest extends AbstractSqlRequest {
3538

3639
static {
3740
// "cursor" is required constructor parameter
38-
PARSER.declareString(constructorArg(), AbstractSqlQueryRequest.Field.CURSOR);
39-
PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), AbstractSqlQueryRequest.Field.MODE);
40-
PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), AbstractSqlQueryRequest.Field.CLIENT_ID);
41+
PARSER.declareString(constructorArg(), CURSOR);
42+
PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), MODE);
43+
PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), CLIENT_ID);
4144
}
4245

4346
private String cursor;

‎x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java‎

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,13 @@
66
package org.elasticsearch.xpack.sql.action;
77

88
import org.elasticsearch.action.ActionRequestValidationException;
9-
import org.elasticsearch.common.ParseField;
109
import org.elasticsearch.common.Strings;
1110
import org.elasticsearch.common.io.stream.StreamInput;
1211
import org.elasticsearch.common.io.stream.StreamOutput;
1312
import org.elasticsearch.common.unit.TimeValue;
1413
import org.elasticsearch.common.xcontent.ObjectParser;
1514
import org.elasticsearch.common.xcontent.XContentBuilder;
1615
import org.elasticsearch.common.xcontent.XContentParser;
17-
import org.elasticsearch.index.query.AbstractQueryBuilder;
1816
import org.elasticsearch.index.query.QueryBuilder;
1917
import org.elasticsearch.xpack.sql.proto.RequestInfo;
2018
import org.elasticsearch.xpack.sql.proto.SqlTypedParamValue;
@@ -25,20 +23,16 @@
2523
import java.util.TimeZone;
2624

2725
import static org.elasticsearch.action.ValidateActions.addValidationError;
26+
import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.CURSOR;
2827

2928
/**
3029
* Request to perform an sql query
3130
*/
3231
public class SqlQueryRequest extends AbstractSqlQueryRequest {
3332
private static final ObjectParser<SqlQueryRequest, Void> PARSER = objectParser(SqlQueryRequest::new);
3433

35-
public static final ParseField CURSOR = new ParseField("cursor");
36-
public static final ParseField FILTER = new ParseField("filter");
37-
3834
static {
3935
PARSER.declareString(SqlQueryRequest::cursor, CURSOR);
40-
PARSER.declareObject(SqlQueryRequest::filter,
41-
(p, c) -> AbstractQueryBuilder.parseInnerQueryBuilder(p), FILTER);
4236
}
4337

4438
private String cursor = "";
@@ -115,7 +109,6 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
115109
}
116110

117111
public static SqlQueryRequest fromXContent(XContentParser parser) {
118-
SqlQueryRequest request = PARSER.apply(parser, null);
119-
return request;
112+
return PARSER.apply(parser, null);
120113
}
121114
}

‎x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryResponse.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import java.util.Objects;
2424

2525
import static java.util.Collections.unmodifiableList;
26+
import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.CURSOR;
2627

2728
/**
2829
* Response to perform an sql query
@@ -158,7 +159,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
158159
builder.endArray();
159160

160161
if (cursor.equals("") == false) {
161-
builder.field(AbstractSqlQueryRequest.Field.CURSOR.getPreferredName(), cursor);
162+
builder.field(CURSOR.getPreferredName(), cursor);
162163
}
163164
}
164165
return builder.endObject();

‎x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryResponseTests.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626

2727
import static org.elasticsearch.common.xcontent.ToXContent.EMPTY_PARAMS;
2828
import static org.hamcrest.Matchers.hasSize;
29+
import static org.elasticsearch.xpack.sql.action.AbstractSqlQueryRequest.SqlRequestField.CURSOR;
2930

3031
public class SqlQueryResponseTests extends AbstractStreamableXContentTestCase<SqlQueryResponse> {
3132

@@ -109,7 +110,7 @@ public void testToXContent() throws IOException {
109110
}
110111

111112
if (testInstance.cursor().equals("") == false) {
112-
assertEquals(rootMap.get(AbstractSqlQueryRequest.Field.CURSOR.getPreferredName()), testInstance.cursor());
113+
assertEquals(rootMap.get(CURSOR.getPreferredName()), testInstance.cursor());
113114
}
114115
}
115116

‎x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java‎

Lines changed: 42 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -17,82 +17,62 @@
1717
import java.io.IOException;
1818
import java.util.ArrayList;
1919
import java.util.List;
20+
import java.util.function.Consumer;
21+
import java.util.function.Function;
2022

2123
import static org.hamcrest.Matchers.containsString;
2224

2325
public class SqlRequestParsersTests extends ESTestCase {
2426

2527
public void testUnknownFieldParsingErrors() throws IOException {
26-
XContentParser parser = parser("{\"key\" : \"value\"}");
27-
Exception e = expectThrows(IllegalArgumentException.class, () -> SqlClearCursorRequest.fromXContent(parser));
28-
assertThat(e.getMessage(), containsString("unknown field [key]"));
29-
30-
XContentParser parser2 = parser("{\"key\" : \"value\"}");
31-
e = expectThrows(IllegalArgumentException.class, () -> SqlQueryRequest.fromXContent(parser2));
32-
assertThat(e.getMessage(), containsString("unknown field [key]"));
33-
34-
XContentParser parser3 = parser("{\"key\" : \"value\"}");
35-
e = expectThrows(IllegalArgumentException.class, () -> SqlTranslateRequest.fromXContent(parser3));
36-
assertThat(e.getMessage(), containsString("unknown field [key]"));
28+
assertParsingErrorMessage("{\"key\" : \"value\"}", "unknown field [key]", SqlClearCursorRequest::fromXContent);
29+
assertParsingErrorMessage("{\"key\" : \"value\"}", "unknown field [key]", SqlQueryRequest::fromXContent);
30+
assertParsingErrorMessage("{\"key\" : \"value\"}", "unknown field [key]", SqlTranslateRequest::fromXContent);
3731
}
3832

3933
public void testClearCursorRequestParser() throws IOException {
40-
XContentParser parser = parser("{\"mode\" : \"jdbc\"}");
41-
Exception e = expectThrows(IllegalArgumentException.class, () -> SqlClearCursorRequest.fromXContent(parser));
42-
assertThat(e.getMessage(), containsString("Required [cursor]"));
34+
assertParsingErrorMessage("{\"mode\" : \"jdbc\"}", "Required [cursor]", SqlClearCursorRequest::fromXContent);
35+
assertParsingErrorMessage("{\"cursor\" : \"whatever\", \"fetch_size\":123}", "unknown field [fetch_size]",
36+
SqlClearCursorRequest::fromXContent);
4337

44-
XContentParser parser2 = parser("{\"cursor\" : \"whatever\", \"fetch_size\":123}");
45-
e = expectThrows(IllegalArgumentException.class, () -> SqlClearCursorRequest.fromXContent(parser2));
46-
assertThat(e.getMessage(), containsString("unknown field [fetch_size]"));
47-
48-
XContentParser parser3 = parser("{\"cursor\" : \"whatever\", \"mode\" : \"jdbc\", \"client.id\" : \"bla\"}");
49-
SqlClearCursorRequest request = SqlClearCursorRequest.fromXContent(parser3);
38+
SqlClearCursorRequest request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"jdbc\", \"client.id\" : \"bla\"}",
39+
SqlClearCursorRequest::fromXContent);
5040
assertEquals("bla", request.clientId());
5141
assertEquals("jdbc", request.mode().toString());
5242
assertEquals("whatever", request.getCursor());
5343

54-
XContentParser parser4 = parser("{\"cursor\" : \"whatever\", \"mode\" : \"some foo mode\", \"client.id\" : \"bla\"}");
55-
request = SqlClearCursorRequest.fromXContent(parser4);
44+
request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"some foo mode\", \"client.id\" : \"bla\"}",
45+
SqlClearCursorRequest::fromXContent);
5646
assertEquals("bla", request.clientId());
5747
assertEquals("plain", request.mode().toString());
5848
assertEquals("whatever", request.getCursor());
49+
50+
request = generateRequest("{\"cursor\" : \"whatever\"}", SqlClearCursorRequest::fromXContent);
51+
assertNull(request.clientId());
52+
assertEquals("plain", request.mode().toString());
53+
assertEquals("whatever", request.getCursor());
5954
}
6055

6156
public void testTranslateRequestParser() throws IOException {
62-
XContentParser parser = parser("{\"qquery\" : \"select * from bla\"}");
63-
Exception e = expectThrows(IllegalArgumentException.class, () -> SqlTranslateRequest.fromXContent(parser));
64-
assertThat(e.getMessage(), containsString("unknown field [qquery]"));
57+
assertParsingErrorMessage("{\"qquery\" : \"select * from bla\"}", "unknown field [qquery]", SqlTranslateRequest::fromXContent);
6558

66-
XContentParser parser3 = parser("{\"query\" : \"select * from foo\"}");
67-
SqlTranslateRequest request = SqlTranslateRequest.fromXContent(parser3);
59+
SqlTranslateRequest request = generateRequest("{\"query\" : \"select * from foo\"}", SqlTranslateRequest::fromXContent);
6860
assertEquals("select * from foo", request.query());
6961
}
7062

7163
public void testQueryRequestParser() throws IOException {
72-
XContentParser parser = parser("{\"mode\" : 123}");
73-
Exception e = expectThrows(IllegalArgumentException.class, () -> SqlQueryRequest.fromXContent(parser));
74-
assertThat(e.getMessage(), containsString("mode doesn't support values of type: VALUE_NUMBER"));
75-
76-
XContentParser parser2 = parser("{\"cursor\" : \"whatever\", \"fetch_size\":\"abc\"}");
77-
e = expectThrows(IllegalArgumentException.class, () -> SqlQueryRequest.fromXContent(parser2));
78-
assertThat(e.getMessage(), containsString("failed to parse field [fetch_size]"));
79-
80-
XContentParser parser3 = parser("{\"client.id\":123}");
81-
e = expectThrows(IllegalArgumentException.class, () -> SqlQueryRequest.fromXContent(parser3));
82-
assertThat(e.getMessage(), containsString("client.id doesn't support values of type: VALUE_NUMBER"));
64+
assertParsingErrorMessage("{\"mode\" : 123}", "mode doesn't support values of type: VALUE_NUMBER", SqlQueryRequest::fromXContent);
65+
assertParsingErrorMessage("{\"cursor\" : \"whatever\", \"fetch_size\":\"abc\"}", "failed to parse field [fetch_size]",
66+
SqlQueryRequest::fromXContent);
67+
assertParsingErrorMessage("{\"client.id\":123}", "client.id doesn't support values of type: VALUE_NUMBER",
68+
SqlQueryRequest::fromXContent);
69+
assertParsingErrorMessage("{\"params\":[{\"value\":123}]}", "failed to parse field [params]", SqlQueryRequest::fromXContent);
70+
assertParsingErrorMessage("{\"time_zone\":12}", "time_zone doesn't support values of type: VALUE_NUMBER",
71+
SqlQueryRequest::fromXContent);
8372

84-
XContentParser parser4 = parser("{\"params\":[{\"value\":123}]}");
85-
e = expectThrows(IllegalArgumentException.class, () -> SqlQueryRequest.fromXContent(parser4));
86-
assertThat(e.getMessage(), containsString("failed to parse field [params]"));
87-
88-
XContentParser parser5 = parser("{\"time_zone\":12}");
89-
e = expectThrows(IllegalArgumentException.class, () -> SqlQueryRequest.fromXContent(parser5));
90-
assertThat(e.getMessage(), containsString("time_zone doesn't support values of type: VALUE_NUMBER"));
91-
92-
XContentParser parser6 = parser("{\"cursor\" : \"whatever\", \"mode\" : \"jdbc\", \"client.id\" : \"bla\", \"query\":\"select\","
93-
+ "\"params\":[{\"value\":123, \"type\":\"whatever\"}], \"time_zone\":\"UTC\", \"request_timeout\":\"5s\","
94-
+ "\"page_timeout\":\"10s\"}");
95-
SqlQueryRequest request = SqlQueryRequest.fromXContent(parser6);
73+
SqlQueryRequest request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"jdbc\", \"client.id\" : \"bla\","
74+
+ "\"query\":\"select\",\"params\":[{\"value\":123, \"type\":\"whatever\"}], \"time_zone\":\"UTC\","
75+
+ "\"request_timeout\":\"5s\",\"page_timeout\":\"10s\"}", SqlQueryRequest::fromXContent);
9676
assertEquals("bla", request.clientId());
9777
assertEquals("jdbc", request.mode().toString());
9878
assertEquals("whatever", request.cursor());
@@ -101,12 +81,23 @@ public void testQueryRequestParser() throws IOException {
10181
List<SqlTypedParamValue> list = new ArrayList<SqlTypedParamValue>(1);
10282
list.add(new SqlTypedParamValue("whatever", 123));
10383
assertEquals(list, request.params());
104-
10584
assertEquals("UTC", request.timeZone().getID());
10685
assertEquals(TimeValue.parseTimeValue("5s", "request_timeout"), request.requestTimeout());
10786
assertEquals(TimeValue.parseTimeValue("10s", "page_timeout"), request.pageTimeout());
10887
}
10988

89+
private <R extends AbstractSqlRequest> R generateRequest(String json, Function<XContentParser, R> fromXContent)
90+
throws IOException {
91+
XContentParser parser = parser(json);
92+
return fromXContent.apply(parser);
93+
}
94+
95+
private void assertParsingErrorMessage(String json, String errorMessage, Consumer<XContentParser> consumer) throws IOException {
96+
XContentParser parser = parser(json);
97+
Exception e = expectThrows(IllegalArgumentException.class, () -> consumer.accept(parser));
98+
assertThat(e.getMessage(), containsString(errorMessage));
99+
}
100+
110101
private XContentParser parser(String content) throws IOException {
111102
XContentType xContentType = XContentType.JSON;
112103
return xContentType.xContent().createParser(

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/AbstractSqlAction.java‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License;
4+
* you may not use this file except in compliance with the Elastic License.
5+
*/
6+
7+
package org.elasticsearch.xpack.sql.plugin;
8+
9+
import org.elasticsearch.client.node.NodeClient;
10+
import org.elasticsearch.common.settings.Settings;
11+
import org.elasticsearch.rest.BaseRestHandler;
12+
import org.elasticsearch.rest.RestRequest;
13+
import org.elasticsearch.xpack.sql.action.AbstractSqlRequest;
14+
import org.elasticsearch.xpack.sql.proto.Mode;
15+
import org.elasticsearch.xpack.sql.proto.RequestInfo;
16+
17+
import java.io.IOException;
18+
import java.util.Locale;
19+
20+
import static org.elasticsearch.xpack.sql.proto.RequestInfo.CANVAS;
21+
import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLI;
22+
23+
public abstract class AbstractSqlAction extends BaseRestHandler {
24+
25+
protected AbstractSqlAction(Settings settings) {
26+
super(settings);
27+
}
28+
29+
@Override
30+
public abstract String getName();
31+
32+
/*
33+
* Parse the rest request and create a sql request out of it.
34+
*/
35+
protected abstract AbstractSqlRequest initializeSqlRequest(RestRequest request) throws IOException;
36+
37+
/*
38+
* Perform additional steps after the default initialization.
39+
*/
40+
protected abstract RestChannelConsumer doPrepareRequest(AbstractSqlRequest sqlRequest, RestRequest request,
41+
NodeClient client) throws IOException;
42+
43+
@Override
44+
protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) throws IOException {
45+
AbstractSqlRequest sqlRequest = initializeSqlRequest(request);
46+
47+
// no mode specified, default to "PLAIN"
48+
if (sqlRequest.requestInfo() == null) {
49+
sqlRequest.requestInfo(new RequestInfo(Mode.PLAIN));
50+
}
51+
52+
// default to no client id, unless it's CLI or CANVAS
53+
if (sqlRequest.requestInfo().clientId() != null) {
54+
String clientId = sqlRequest.requestInfo().clientId().toLowerCase(Locale.ROOT);
55+
if (!clientId.equals(CLI) && !clientId.equals(CANVAS)) {
56+
clientId = null;
57+
}
58+
sqlRequest.requestInfo().clientId(clientId);
59+
}
60+
61+
return doPrepareRequest(sqlRequest, request, client);
62+
}
63+
64+
}

0 commit comments

Comments
 (0)