SQL: move requests' parameters to requests JSON body by astefan · Pull Request #36149 · elastic/elasticsearch

astefan · 2018-12-03T10:05:48Z

Fixes #35992.

elasticmachine · 2018-12-03T10:05:49Z

Pinging @elastic/es-search

matriv

Looks good overall, left some comments mostly about repetition of code.

...sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java

matriv · 2018-12-03T11:15:39Z

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

+import org.elasticsearch.common.ParseField;
+
+final class SqlField {
+    public static final ParseField MODE = new ParseField("mode");


You have those here: https://github.com/elastic/elasticsearch/pull/36149/files#diff-87a0e05a586a10ecca6f36c0e82ef44aR255

Indeed, overlooked this class. Will remove it and use the interface inside AbstractSqlQueryRequest. Thanks for catching.

matriv · 2018-12-03T11:32:04Z

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

+
+import org.elasticsearch.common.ParseField;
+
+final class SqlField {


can be an interface, and also the name could be SqlRequestField

Will use the interface inside AbstractSqlQueryRequest. Renamed to SqlRequestField.

matriv · 2018-12-03T11:33:50Z

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

        SqlTranslateRequest sqlRequest;
        try (XContentParser parser = request.contentOrSourceParamParser()) {
-            sqlRequest = SqlTranslateRequest.fromXContent(parser, Mode.fromString(request.param("mode")));
+            sqlRequest = SqlTranslateRequest.fromXContent(parser);


This block is repeated here: https://github.com/elastic/elasticsearch/pull/36149/files#diff-47894e3a968e76739b45d73c3405ab8eR61 and here: https://github.com/elastic/elasticsearch/pull/36149/files#diff-33543baa55df79bc3941f885b6fcfa61R47 so maybe extract it to a method.

matriv · 2018-12-03T11:35:13Z

...ck/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlQueryRequest.java

        if (query != null) {
            builder.field("query", query);
        }
+        if (requestInfo() != null) {


code is repeated here: https://github.com/elastic/elasticsearch/pull/36149/files#diff-7e3b87bb842194e7f25b2a712cb821e6R46 please extract to a method.

Has this been resolved because it still shows up?

matriv · 2018-12-03T11:39:25Z

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

            builder.endArray();

            if (cursor.equals("") == false) {
-                builder.field(SqlQueryRequest.CURSOR.getPreferredName(), cursor);


Do we still need the CURSOR and FILTER in org.elasticsearch.xpack.sql.action.SqlQueryRequest?

matriv · 2018-12-03T11:45:05Z

.../sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java

+        assertThat(e.getMessage(), containsString("unknown field [key]"));
+    }
+
+    public void testClearCursorRequestParser() throws IOException {


Please also check for missing completely client.id and mode.
If you extract the parsing in a method then you could also unit test the method so we can avoid checking for all different types of requests.

…o 35992_fix

costin

Left some comments mainly around encapsulation.

costin · 2018-12-04T08:47:23Z

...sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java

+        ParseField PAGE_TIMEOUT = new ParseField("page_timeout");
+        ParseField FILTER = new ParseField("filter");
+        ParseField MODE = new ParseField("mode");
+        ParseField CLIENT_ID = new ParseField("client.id");


Should be client_id not client.id - helps with JSON and it is consistent with the other params.

I planned to change the parameter name after this PR, but I can do it now, as well.

costin · 2018-12-04T08:53:31Z

...sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java

+        parser.declareString(AbstractSqlQueryRequest::query, SqlRequestField.QUERY);
+        parser.declareString((request, mode) -> {
+            Mode m = Mode.fromString(mode);
+            if (request.requestInfo() != null) {


This would be better handled inside the request object so there are no null checks.
Either there's a default requestInfo so one can just call request.info().clientId() or do request.clientId(..) and internally set that up.

costin · 2018-12-04T08:53:40Z

...sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java

+            }
+        }, SqlRequestField.MODE);
+        parser.declareString((request, clientId) -> {
+            if (request.requestInfo() != null) {


costin · 2018-12-04T08:54:17Z

...n/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java

-            (String) objects[0]
-        ));
+    private static final ConstructingObjectParser<SqlClearCursorRequest, RequestInfo> PARSER =
+            // here the position in "objects" is the same as the fields parser declarations below 


Why is the comment misaligned with the object below?

costin · 2018-12-04T08:55:38Z

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


    // TODO: Simplify cursor handling
    private String cursor;
+    private Mode mode;


Why do we need the mode in the response? Do clients consume it in any way?

We use it to output dates in different formats, depending on mode (jdbc/odbc VS. plain). See

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

Line 174 in b32f545

if (Mode.isDriver(mode)) {

costin · 2018-12-04T08:56:27Z

.../sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequestTests.java

-    public Mode testMode;
+
+    public RequestInfo requestInfo;
+    public String clientId;


Why is there a clientId and RequestInfo; the former is contained in the latter.

Missed this one. Will fix.

costin · 2018-12-04T08:58:20Z

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java

-        if (mode == null) {
+        try {
+            return Mode.valueOf(mode.toUpperCase(Locale.ROOT));
+        } catch (Exception e) {


I think we should just thrown an exception stating the mode is invalid. I think it's good to be lenient but at the same time simply don't set up a mode instead of setting an incorrect one.

I've done this way because mode is a required parameter, even if a client can choose not to send one or send the wrong one. It's just required everywhere in the code and I kept the same behavior.

If it's a required param let's use a default value for it so in case the client doesn't send one, we default to PLAIN which is what we do anyway.
However if the client sends an incorrect value we should report this not silently ignore it.

costin · 2018-12-04T08:59:15Z

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

+        AbstractSqlRequest sqlRequest = initializeSqlRequest(request);
+
+        // no mode specified, default to "PLAIN"
+        if (sqlRequest.requestInfo() == null) {


This constant checking of requestInfo is trappy - simply set a default one for PLAIN and no client id so there's no risk of getting a NPE.

…o 35992_fix

costin

Some leftovers.

costin · 2018-12-05T09:07:29Z

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java

-        if (mode == null) {
+        try {
+            return Mode.valueOf(mode.toUpperCase(Locale.ROOT));
+        } catch (Exception e) {


If it's a required param let's use a default value for it so in case the client doesn't send one, we default to PLAIN which is what we do anyway.
However if the client sends an incorrect value we should report this not silently ignore it.

costin · 2018-12-05T09:09:46Z

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/RequestInfo.java

    public void clientId(String clientId) {
+        if (clientId != null) {
+            clientId = clientId.toLowerCase(Locale.ROOT);
+            if (!clientId.equals(CLI) && !clientId.equals(CANVAS)) {


Let's make this pluggable by checking clientId against a list of known values/whitelist.
I mention this since for odbc we're looking into potentially sending extra information (32 vs 64 bit, maybe some OS info) and this would make it easier - just add the item to the whitelist.

costin · 2018-12-05T09:12:24Z

...gin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlClearCursorRequest.java

    @Override
    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
        builder.field("cursor", cursor);
+        if (requestInfo() != null) {


This makes reading inconsistent since a field might appear or not depending on its value. How about using nullValue?
Also mode is always required so the check should not be performed for it no?

costin · 2018-12-05T09:13:15Z

...ck/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlQueryRequest.java

        if (query != null) {
            builder.field("query", query);
        }
+        if (requestInfo() != null) {


Has this been resolved because it still shows up?

costin · 2018-12-05T09:16:17Z

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/RequestInfo.java

    public RequestInfo(Mode mode, String clientId) {
-        this.mode = mode;
-        this.clientId = clientId;
+        mode(mode);


Do we expect the requestInfo to be modified after the data is passed through the constructor?
If not, the setters are not needed and the fields can be made final.

The ObjectParser here needs access to individual methods to set mode and client_id respectively.

costin · 2018-12-05T09:21:16Z

...sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java

        return Objects.hash(super.hashCode(), query, timeZone, fetchSize, requestTimeout, pageTimeout, filter);
    }
+
+    public interface SqlRequestField {


Let's move these fields inside the class itself, make them private/default and remove the interface.

costin · 2018-12-05T09:21:48Z

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java

    }
+
+    public static String mode(String mode) {
+        return mode.isEmpty() ? "" : ",\"mode\":\"" + mode + "\"";


Strings.isEmpty is a better choice as it checks if it's also null.

…o 35992_fix

astefan · 2018-12-05T15:51:16Z

run the gradle build tests 2

…o 35992_fix

astefan · 2018-12-06T02:20:47Z

run the gradle build tests 2

…o 35992_fix

astefan · 2018-12-06T13:04:47Z

run the gradle build tests 2

costin

LGTM

costin · 2018-12-10T07:48:18Z

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java

 import org.apache.http.HttpEntity;
 import org.apache.http.entity.ContentType;
 import org.apache.http.entity.StringEntity;
+import org.apache.logging.log4j.util.Strings;


Looks like an incorrect import.

costin · 2018-12-10T07:48:36Z

x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java

-                response = runSql(mode, new StringEntity("{\"cursor\":\"" + cursor + "\"}",
-                        ContentType.APPLICATION_JSON));
+                response = runSql(new StringEntity("{\"cursor\":\"" + cursor + "\"" + mode(mode) + "}",
+                        ContentType.APPLICATION_JSON), "");


"" could be StringUtils/Strings.EMPTY

I've added an EMPTY constant to org.elasticsearch.xpack.sql.proto.StringUtils. It seems there is no other class visible to qa that has this constant defined.

costin · 2018-12-10T07:50:16Z

...n/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlClearCursorRequest.java


    public SqlClearCursorRequest() {
-
+        super();


There's no value in adding super as this constructor is called implicitly anyway.

costin · 2018-12-10T07:52:19Z

x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/Mode.java


    public static Mode fromString(String mode) {
-        if (mode == null) {
+        if (mode == null || mode.isEmpty()) {


Potentially Strings.isEmpty

There is no utility class visible to this class that has an isEmpty() method useful here. Or I missed it... which is very likely.

matriv

LGTM

…o 35992_fix

astefan · 2018-12-11T01:29:42Z

run the gradle build tests 1

(cherry picked from commit eead8a1)

* elastic/master: (36 commits) Add check for minimum required Docker version (elastic#36497) Minor search controller changes (elastic#36479) Add default methods to DocValueFormat (elastic#36480) Fix the mixed cluster REST test explain/11_basic_with_types. Modify `BigArrays` to take name of circuit breaker (elastic#36461) Move LoggedExec to minimumRuntime source set (elastic#36453) added 6.5.4 version Add test logging for elastic#35644 Tests- added helper methods to ESRestTestCase for checking warnings (elastic#36443) SQL: move requests' parameters to requests JSON body (elastic#36149) [Zen2] Respect the no_master_block setting (elastic#36478) Require soft-deletes when access changes snapshot (elastic#36446) Switch more tests to zen2 (elastic#36367) [Painless] Add extensive tests for def to primitive casts (elastic#36455) Add setting to bypass Rollover action (elastic#36235) Try running CI against Zulu (elastic#36391) [DOCS] Reworked the shard allocation filtering info. (elastic#36456) Log [initial_master_nodes] on formation failure (elastic#36466) converting ForbiddenPatternsTask to .java (elastic#36194) fixed typo ...

astefan added 2 commits December 3, 2018 11:52

Move SQL request parameters from http url params to JSON body

bda5654

Removed leftover commented code

6c69d77

astefan added >enhancement v7.0.0 :Analytics/SQL SQL querying v6.6.0 labels Dec 3, 2018

astefan requested a review from costin December 3, 2018 10:05

astefan requested a review from matriv December 3, 2018 10:05

matriv reviewed Dec 3, 2018

View reviewed changes

astefan added 2 commits December 4, 2018 04:34

Adressing review comments

375caf4

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

b32f545

…o 35992_fix

costin requested changes Dec 4, 2018

View reviewed changes

astefan added 2 commits December 5, 2018 09:46

Addressed reviews

5720e44

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

9e0b05d

…o 35992_fix

costin reviewed Dec 5, 2018

View reviewed changes

astefan added 2 commits December 5, 2018 16:38

Further polishing

e8298c5

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

697f836

…o 35992_fix

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

5fc74eb

…o 35992_fix

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

358a944

…o 35992_fix

costin approved these changes Dec 10, 2018

View reviewed changes

matriv approved these changes Dec 10, 2018

View reviewed changes

astefan added 5 commits December 10, 2018 12:51

Cosmetic changes

945bab2

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

51dee0d

…o 35992_fix

Revert back Strings class usage

5a1bc38

Handle request body in source parameter as well

1a1f84b

Merge branch 'master' of https://github.com/elastic/elasticsearch int…

1dc2a64

…o 35992_fix

astefan merged commit eead8a1 into elastic:master Dec 11, 2018

astefan added a commit that referenced this pull request Dec 11, 2018

SQL: move requests' parameters to requests JSON body (#36149)

ea19d8f

(cherry picked from commit eead8a1)

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

astefan mentioned this pull request Jun 26, 2019

SQL: handle SQL not being available in a more graceful way #43665

Merged


		import org.elasticsearch.common.ParseField;

		final class SqlField {

Conversation

astefan commented Dec 3, 2018

Uh oh!

elasticmachine commented Dec 3, 2018

Uh oh!

matriv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

costin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astefan commented Dec 5, 2018

Uh oh!

astefan commented Dec 6, 2018

astefan Dec 5, 2018 •

edited

Loading