Adressing review comments

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

@@ -60,32 +60,32 @@ public AbstractSqlQueryRequest(String query, List<SqlTypedParamValue> params, Qu
     protected static <R extends AbstractSqlQueryRequest> ObjectParser<R, Void> objectParser(Supplier<R> supplier) {
         // TODO: convert this into ConstructingObjectParser
         ObjectParser<R, Void> 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) {
                 request.requestInfo().mode(m);
             } 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");

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;

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

@@ -6,15 +6,13 @@
 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;
 import org.elasticsearch.common.unit.TimeValue;
 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,20 +23,16 @@
 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
  */
 public class SqlQueryRequest extends AbstractSqlQueryRequest {
     private static final ObjectParser<SqlQueryRequest, Void> 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);
     }
 }

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();

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<SqlQueryResponse> {
 
@@ -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());
         }
     }
 

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<SqlTypedParamValue> list = new ArrayList<SqlTypedParamValue>(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 extends AbstractSqlRequest> R generateRequest(String json, Function<XContentParser, R> fromXContent)
+            throws IOException {
+        XContentParser parser = parser(json);
+        return fromXContent.apply(parser);
+    }
+
+    private void assertParsingErrorMessage(String json, String errorMessage, Consumer<XContentParser> 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(

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);
+    }
+
+}