SQL: Mimick Elasticsearch defaults for sql action (elastic/x-pack-elasticsearch#4398)

The SQL action supports several text outputs and used to default to an
output that looked like the SQL CLI. It is a lovely output format but
this changes output selection behavior to mimick Elasticsearch's
standard behavior: it'll now default to the same format as the request.

That means that if you want the pretty text format then you need to ask
for it. The way to do that is:

```
POST /_xpack/sql?format=text/plain
{
    "query": "SELECT * FROM library ORDER BY page_count DESC LIMIT 5"
}
```

Original commit: elastic/x-pack-elasticsearch@4a15a23b18

docs/en/sql/endpoints/rest.asciidoc

@@ -7,7 +7,7 @@ and returns the results. For example:
 
 [source,js]
 --------------------------------------------------
-POST /_xpack/sql
+POST /_xpack/sql?format=txt
 {
     "query": "SELECT * FROM library ORDER BY page_count DESC LIMIT 5"
 }
@@ -30,16 +30,19 @@ James S.A. Corey |Leviathan Wakes     |561            |2011-06-02T00:00:00.000Z
 // TESTRESPONSE[s/\|/\\|/ s/\+/\\+/]
 // TESTRESPONSE[_cat]
 
-You can also choose to get results in a structured format by adding the `format` parameter. Currently supported formats:
+While the `text/plain` format is nice for humans, computers prefer something
-- text (default)
+more structured. You can replace the value of `format` with:
-- json
+- `json` aka `application/json`
-- smile
+- `yaml` aka `application/yaml`
-- yaml
+- `smile` aka `application/smile`
-- cbor
+- `cbor` aka `application/cbor`
-
+- `txt` aka `text/plain`
-Alternatively you can set the Accept HTTP header to the appropriate media format.
+- `csv` aka `text/csv`
-All formats above are supported, the GET parameter takes precedence over the header.
+- `tsv` aka `text/tab-separated-values`
 
+Alternatively you can set the `Accept` HTTP header to the appropriate media
+format. The GET parameter takes precedence over the header. If neither is
+specified then the response is returned in the same format as the request.
 
 [source,js]
 --------------------------------------------------
@@ -147,7 +150,7 @@ parameter.
 
 [source,js]
 --------------------------------------------------
-POST /_xpack/sql
+POST /_xpack/sql?format=txt
 {
     "query": "SELECT * FROM library ORDER BY page_count DESC",
     "filter": {

docs/en/sql/getting-started.asciidoc

@@ -20,7 +20,7 @@ And now you can execute SQL using the <<sql-rest>> right away:
 
 [source,js]
 --------------------------------------------------
-POST /_xpack/sql
+POST /_xpack/sql?format=txt
 {
     "query": "SELECT * FROM library WHERE release_date < '2000-01-01'"
 }

docs/en/sql/language/syntax.asciidoc

@@ -23,7 +23,7 @@ So sorting by a field looks like:
 
 [source,js]
 --------------------------------------------------
-POST /_xpack/sql
+POST /_xpack/sql?format=txt
 {
     "query": "SELECT * FROM library ORDER BY page_count DESC LIMIT 5"
 }
@@ -55,7 +55,7 @@ combined using the same rules as Elasticsearch's
 
 [source,js]
 --------------------------------------------------
-POST /_xpack/sql
+POST /_xpack/sql?format=txt
 {
     "query": "SELECT SCORE(), * FROM library WHERE match(name, 'dune') ORDER BY SCORE() DESC"
 }
@@ -82,7 +82,7 @@ is possible even if you are not sorting by `SCORE()`:
 
 [source,js]
 --------------------------------------------------
-POST /_xpack/sql
+POST /_xpack/sql?format=txt
 {
     "query": "SELECT SCORE(), * FROM library WHERE match(name, 'dune') ORDER BY page_count DESC"
 }

plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java

@@ -18,7 +18,6 @@ import org.elasticsearch.rest.RestRequest;
 import org.elasticsearch.rest.RestResponse;
 import org.elasticsearch.rest.RestStatus;
 import org.elasticsearch.rest.action.RestResponseListener;
-import org.elasticsearch.rest.action.RestToXContentListener;
 import org.elasticsearch.xpack.sql.session.Cursor;
 import org.elasticsearch.xpack.sql.session.Cursors;
 
@@ -43,34 +42,63 @@ public class RestSqlQueryAction extends BaseRestHandler {
             sqlRequest = SqlQueryRequest.fromXContent(parser, AbstractSqlRequest.Mode.fromString(request.param("mode")));
         }
 
-        XContentType xContentType = XContentType.fromMediaTypeOrFormat(request.param("format", request.header("Accept")));
+        /*
+         * Since we support {@link TextFormat} <strong>and</strong>
+         * {@link XContent} outputs we can't use {@link RestToXContentListener}
+         * like everything else. We want to stick as closely as possible to
+         * Elasticsearch's defaults though, while still layering in ways to
+         * control the output more easilly.
+         *
+         * First we find the string that the user used to specify the response
+         * format. If there is a {@code format} paramter we use that. If there
+         * isn't but there is a {@code Accept} header then we use that. If there
+         * isn't then we use the {@code Content-Type} header which is required.
+         */
+        String accept = request.param("format");
+        if (accept == null) {
+            accept = request.header("Accept");
+            if ("*/*".equals(accept)) {
+                // */* means "I don't care" which we should treat like not specifying the header
+                accept = null;
+            }
+        }
+        if (accept == null) {
+            accept = request.header("Content-Type");
+        }
+        assert accept != null : "The Content-Type header is required";
+
+        /*
+         * Second, we pick the actual content type to use by first parsing the
+         * string from the previous step as an {@linkplain XContent} value. If
+         * that doesn't parse we parse it as a {@linkplain TextFormat} value. If
+         * that doesn't parse it'll throw an {@link IllegalArgumentException}
+         * which we turn into a 400 error.
+         */
+        XContentType xContentType = accept == null ? XContentType.JSON : XContentType.fromMediaTypeOrFormat(accept);
         if (xContentType != null) {
-            // The client expects us to send back results in a XContent format
+            return channel -> client.execute(SqlQueryAction.INSTANCE, sqlRequest, new RestResponseListener<SqlQueryResponse>(channel) {
-            return channel -> client.executeLocally(SqlQueryAction.INSTANCE, sqlRequest,
+                @Override
-                    new RestToXContentListener<SqlQueryResponse>(channel) {
+                public RestResponse buildResponse(SqlQueryResponse response) throws Exception {
-                        @Override
+                    XContentBuilder builder = XContentBuilder.builder(xContentType.xContent());
-                        public RestResponse buildResponse(SqlQueryResponse response, XContentBuilder builder) throws Exception {
+                    response.toXContent(builder, request);
-                            // Make sure we only display JDBC-related data if JDBC is enabled
+                    return new BytesRestResponse(RestStatus.OK, builder);
-                            response.toXContent(builder, request);
+                }
-                            return new BytesRestResponse(getStatus(response), builder);
+            });
-                        }
-                    });
         }
 
-        // The client accepts a text format
+        TextFormat textFormat = TextFormat.fromMediaTypeOrFormat(accept);
-        TextFormat text = TextFormat.fromMediaTypeOrFormat(request);
-        long startNanos = System.nanoTime();
 
+        long startNanos = System.nanoTime();
         return channel -> client.execute(SqlQueryAction.INSTANCE, sqlRequest, new RestResponseListener<SqlQueryResponse>(channel) {
             @Override
             public RestResponse buildResponse(SqlQueryResponse response) throws Exception {
                 Cursor cursor = Cursors.decodeFromString(sqlRequest.cursor());
-                final String data = text.format(cursor, request, response);
+                final String data = textFormat.format(cursor, request, response);
 
-                RestResponse restResponse = new BytesRestResponse(RestStatus.OK, text.contentType(request),
+                RestResponse restResponse = new BytesRestResponse(RestStatus.OK, textFormat.contentType(request),
                         data.getBytes(StandardCharsets.UTF_8));
 
-                Cursor responseCursor = text.wrapCursor(cursor, response);
+                Cursor responseCursor = textFormat.wrapCursor(cursor, response);
 
                 if (responseCursor != Cursor.EMPTY) {
                     restResponse.addHeader("Cursor", Cursors.encodeToString(Version.CURRENT, responseCursor));
@@ -86,4 +114,4 @@ public class RestSqlQueryAction extends BaseRestHandler {
     public String getName() {
         return "xpack_sql_action";
     }
-}
+}

plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java

@@ -26,7 +26,7 @@ enum TextFormat {
 
     /**
      * Default text writer.
-     * 
+     *
      * The implementation is a bit weird since state needs to be passed around, namely the formatter
      * since it is initialized based on the first page of data.
      * To avoid leaking the formatter, it gets discovered again in the wrapping method to attach it
@@ -75,15 +75,15 @@ enum TextFormat {
 
     /**
      * Comma Separated Values implementation.
-     * 
+     *
      * Based on:
      * https://tools.ietf.org/html/rfc4180
      * https://www.iana.org/assignments/media-types/text/csv
      * https://www.w3.org/TR/sparql11-results-csv-tsv/
-     * 
+     *
      */
     CSV() {
-        
+
         @Override
         protected String delimiter() {
             return ",";
@@ -236,23 +236,17 @@ enum TextFormat {
         return Cursors.decodeFromString(response.cursor());
     }
 
-    static TextFormat fromMediaTypeOrFormat(RestRequest request) {
+    static TextFormat fromMediaTypeOrFormat(String accept) {
-        String format = request.param("format", request.header("Accept"));
-
-        if (format == null) {
-            return PLAIN_TEXT;
-        }
-
         for (TextFormat text : values()) {
             String contentType = text.contentType();
-            if (contentType.equalsIgnoreCase(format)
+            if (contentType.equalsIgnoreCase(accept)
-                    || format.toLowerCase(Locale.ROOT).startsWith(contentType + ";")
+                    || accept.toLowerCase(Locale.ROOT).startsWith(contentType + ";")
-                    || text.shortName().equalsIgnoreCase(format)) {
+                    || text.shortName().equalsIgnoreCase(accept)) {
                 return text;
             }
         }
 
-        return PLAIN_TEXT;
+        throw new IllegalArgumentException("invalid format [" + accept + "]");
     }
 
     /**
@@ -303,4 +297,4 @@ enum TextFormat {
     String maybeEscape(String value) {
         return value;
     }
-}
+}

plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java

@@ -25,29 +25,25 @@ import static org.hamcrest.CoreMatchers.is;
 public class TextFormatTests extends ESTestCase {
 
     public void testPlainTextDetection() {
-        TextFormat text = TextFormat.fromMediaTypeOrFormat(withHeader("Accept", "text/plain"));
+        TextFormat text = TextFormat.fromMediaTypeOrFormat("text/plain");
         assertThat(text, is(TextFormat.PLAIN_TEXT));
     }
 
-    public void testTextFallbackDetection() {
-        TextFormat text = TextFormat.fromMediaTypeOrFormat(withHeader("Accept", "text/*"));
-        assertThat(text, is(TextFormat.PLAIN_TEXT));
-    }
-
-    public void testTextFallbackNoHeader() {
-        assertThat(TextFormat.fromMediaTypeOrFormat(req()), is(TextFormat.PLAIN_TEXT));
-    }
-
     public void testCsvDetection() {
-        TextFormat text = TextFormat.fromMediaTypeOrFormat(withHeader("Accept", "text/csv"));
+        TextFormat text = TextFormat.fromMediaTypeOrFormat("text/csv");
         assertThat(text, is(CSV));
     }
 
     public void testTsvDetection() {
-        TextFormat text = TextFormat.fromMediaTypeOrFormat(withHeader("Accept", "text/tab-separated-values"));
+        TextFormat text = TextFormat.fromMediaTypeOrFormat("text/tab-separated-values");
         assertThat(text, is(TSV));
     }
 
+    public void testInvalidFormat() {
+        Exception e = expectThrows(IllegalArgumentException.class, () -> TextFormat.fromMediaTypeOrFormat("text/garbage"));
+        assertEquals("invalid format [text/garbage]", e.getMessage());
+    }
+
     public void testCsvContentType() {
         assertEquals("text/csv; charset=utf-8; header=present", CSV.contentType(req()));
     }
@@ -127,7 +123,7 @@ public class TextFormatTests extends ESTestCase {
         List<ColumnInfo> headers = new ArrayList<>();
         headers.add(new ColumnInfo("index", "string", "keyword"));
         headers.add(new ColumnInfo("index", "number", "integer"));
-        
+
         // values
         List<List<Object>> values = new ArrayList<>();
         values.add(asList("Along The River Bank", 11 * 60 + 48));
@@ -157,9 +153,4 @@ public class TextFormatTests extends ESTestCase {
     private static RestRequest reqNoHeader() {
         return new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withParams(singletonMap("header", "absent")).build();
     }
-
+}
-
-    private static RestRequest withHeader(String key, String value) {
-        return new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withHeaders(singletonMap(key, singletonList(value))).build();
-    }
-}

plugin/src/test/resources/rest-api-spec/test/sql/sql.yml

@@ -80,7 +80,7 @@ setup:
 "Getting textual representation":
   - do:
       xpack.sql.query:
-        format: text
+        format: txt
         body:
           query: "SELECT * FROM test ORDER BY int asc"
   - match:
@@ -117,4 +117,3 @@ setup:
       indices.stats: { index: 'test' }
 
   - match: { indices.test.total.search.open_contexts: 0 }
-

qa/sql/no-security/build.gradle

@@ -5,4 +5,5 @@ integTestCluster {
 
 runqa {
   setting 'xpack.security.enabled', 'false'
+  setting 'xpack.license.self_generated.type', 'trial'
 }

qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java

@@ -343,10 +343,17 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe
         Map<String, String> params = new TreeMap<>();
         params.put("error_trace", "true");   // Helps with debugging in case something crazy happens on the server.
         params.put("pretty", "true");        // Improves error reporting readability
-        params.put("format", "json");        // JSON is easier to parse then a table
+        if (randomBoolean()) {
-        if (Strings.hasText(mode)) {
+            // We default to JSON but we force it randomly for extra coverage
-            params.put("mode", mode);            // JDBC or PLAIN mode
+            params.put("format", "json");
         }
+        if (Strings.hasText(mode)) {
+            params.put("mode", mode);        // JDBC or PLAIN mode
+        }
+        Header[] headers = randomFrom(
+            new Header[] {},
+            new Header[] {new BasicHeader("Accept", "*/*")},
+            new Header[] {new BasicHeader("Accpet", "application/json")});
         Response response = client().performRequest("POST", "/_xpack/sql" + suffix, params, sql);
         try (InputStream content = response.getEntity().getContent()) {
             return XContentHelper.convertToMap(JsonXContent.jsonXContent, content, false);
@@ -447,7 +454,7 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe
                 "---------------\n" +
                 "test           \n" +
                 "test           \n";
-        Tuple<String, String> response = runSqlAsText("SELECT * FROM test");
+        Tuple<String, String> response = runSqlAsText("SELECT * FROM test", "text/plain");
         assertEquals(expected, response.v1());
     }
 
@@ -466,9 +473,10 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe
         for (int i = 0; i < 20; i += 2) {
             Tuple<String, String> response;
             if (i == 0) {
-                response = runSqlAsText("", new StringEntity(request, ContentType.APPLICATION_JSON));
+                response = runSqlAsText("", new StringEntity(request, ContentType.APPLICATION_JSON), "text/plain");
             } else {
-                response = runSqlAsText("", new StringEntity("{\"cursor\":\"" + cursor + "\"}", ContentType.APPLICATION_JSON));
+                response = runSqlAsText("", new StringEntity("{\"cursor\":\"" + cursor + "\"}", ContentType.APPLICATION_JSON),
+                        "text/plain");
             }
 
             StringBuilder expected = new StringBuilder();
@@ -494,7 +502,7 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe
     }
 
     // CSV/TSV tests
-    
+
     private static String toJson(String value) {
         return "\"" + new String(JsonStringEncoder.getInstance().quoteAsString(value)) + "\"";
     }
@@ -509,11 +517,11 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe
                 "first,1\r\n" +
                 "second\t,2\r\n" +
                 "\"\"\"third,\"\"\",3\r\n";
-        
+
         String query = "SELECT * FROM test ORDER BY number";
         Tuple<String, String> response = runSqlAsText(query, "text/csv");
         assertEquals(expected, response.v1());
-        
+
         response = runSqlAsTextFormat(query, "csv");
         assertEquals(expected, response.v1());
     }
@@ -527,7 +535,7 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe
                 "first,1\r\n" +
                 "second\t,2\r\n" +
                 "\"\"\"third,\"\"\",3\r\n";
-        
+
         String query = "SELECT * FROM test ORDER BY number";
         Tuple<String, String> response = runSqlAsText(query, "text/csv; header=absent");
         assertEquals(expected, response.v1());
@@ -543,7 +551,7 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe
                 "first\t1\n" +
                 "second\\t\t2\n" +
                 "\"third,\"\t3\n";
-        
+
         String query = "SELECT * FROM test ORDER BY number";
         Tuple<String, String> response = runSqlAsText(query, "text/tab-separated-values");
         assertEquals(expected, response.v1());
@@ -551,17 +559,13 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe
         assertEquals(expected, response.v1());
     }
 
-    private Tuple<String, String> runSqlAsText(String sql) throws IOException {
+    private Tuple<String, String> runSqlAsText(String sql, String accept) throws IOException {
-        return runSqlAsText("", new StringEntity("{\"query\":\"" + sql + "\"}", ContentType.APPLICATION_JSON));
+        return runSqlAsText("", new StringEntity("{\"query\":\"" + sql + "\"}", ContentType.APPLICATION_JSON), accept);
     }
 
-    private Tuple<String, String> runSqlAsText(String sql, String acceptHeader) throws IOException {
+    private Tuple<String, String> runSqlAsText(String suffix, HttpEntity entity, String accept) throws IOException {
-        return runSqlAsText("", new StringEntity("{\"query\":\"" + sql + "\"}", ContentType.APPLICATION_JSON),
+        Response response = client().performRequest("POST", "/_xpack/sql" + suffix, singletonMap("error_trace", "true"),
-                new BasicHeader("Accept", acceptHeader));
+                entity, new BasicHeader("Accept", accept));
-    }
-
-    private Tuple<String, String> runSqlAsText(String suffix, HttpEntity sql, Header... headers) throws IOException {
-        Response response = client().performRequest("POST", "/_xpack/sql" + suffix, singletonMap("error_trace", "true"), sql, headers);
         return new Tuple<>(
                 Streams.copyToString(new InputStreamReader(response.getEntity().getContent(), StandardCharsets.UTF_8)),
                 response.getHeader("Cursor")
@@ -570,20 +574,18 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe
 
     private Tuple<String, String> runSqlAsTextFormat(String sql, String format) throws IOException {
         StringEntity entity = new StringEntity("{\"query\":\"" + sql + "\"}", ContentType.APPLICATION_JSON);
-        
+
         Map<String, String> params = new HashMap<>();
         params.put("error_trace", "true");
         params.put("format", format);
-                
+
         Response response = client().performRequest("POST", "/_xpack/sql", params, entity);
         return new Tuple<>(
                 Streams.copyToString(new InputStreamReader(response.getEntity().getContent(), StandardCharsets.UTF_8)),
                 response.getHeader("Cursor")
         );
-
     }
 
-
     private void assertResponse(Map<String, Object> expected, Map<String, Object> actual) {
         if (false == expected.equals(actual)) {
             NotEqualMessageBuilder message = new NotEqualMessageBuilder();
@@ -625,4 +627,4 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe
     public static String randomMode() {
         return randomFrom("", "jdbc", "plain");
     }
-}
+}