From f0be979541e8ffa9515b54ee24ff2078784a4ca2 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Thu, 19 Apr 2018 15:40:19 -0400 Subject: [PATCH] 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@4a15a23b18ee5613894e935a3ac054bdda029535 --- docs/en/sql/endpoints/rest.asciidoc | 25 +++---- docs/en/sql/getting-started.asciidoc | 2 +- docs/en/sql/language/syntax.asciidoc | 6 +- .../xpack/sql/plugin/RestSqlQueryAction.java | 66 +++++++++++++------ .../xpack/sql/plugin/TextFormat.java | 26 +++----- .../xpack/sql/plugin/TextFormatTests.java | 29 +++----- .../resources/rest-api-spec/test/sql/sql.yml | 3 +- qa/sql/no-security/build.gradle | 1 + .../xpack/qa/sql/rest/RestSqlTestCase.java | 52 ++++++++------- 9 files changed, 114 insertions(+), 96 deletions(-) diff --git a/docs/en/sql/endpoints/rest.asciidoc b/docs/en/sql/endpoints/rest.asciidoc index 383a420bc2f..d31b03d3e77 100644 --- a/docs/en/sql/endpoints/rest.asciidoc +++ b/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: -- text (default) -- json -- smile -- yaml -- cbor - -Alternatively you can set the Accept HTTP header to the appropriate media format. -All formats above are supported, the GET parameter takes precedence over the header. +While the `text/plain` format is nice for humans, computers prefer something +more structured. You can replace the value of `format` with: +- `json` aka `application/json` +- `yaml` aka `application/yaml` +- `smile` aka `application/smile` +- `cbor` aka `application/cbor` +- `txt` aka `text/plain` +- `csv` aka `text/csv` +- `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": { diff --git a/docs/en/sql/getting-started.asciidoc b/docs/en/sql/getting-started.asciidoc index 802d3b9d757..8860fbc2050 100644 --- a/docs/en/sql/getting-started.asciidoc +++ b/docs/en/sql/getting-started.asciidoc @@ -20,7 +20,7 @@ And now you can execute SQL using the <> right away: [source,js] -------------------------------------------------- -POST /_xpack/sql +POST /_xpack/sql?format=txt { "query": "SELECT * FROM library WHERE release_date < '2000-01-01'" } diff --git a/docs/en/sql/language/syntax.asciidoc b/docs/en/sql/language/syntax.asciidoc index 38a2cba7765..5b837c91db2 100644 --- a/docs/en/sql/language/syntax.asciidoc +++ b/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" } diff --git a/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java b/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java index 332199e4541..9d043f855fd 100644 --- a/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/RestSqlQueryAction.java +++ b/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} and + * {@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.executeLocally(SqlQueryAction.INSTANCE, sqlRequest, - new RestToXContentListener(channel) { - @Override - public RestResponse buildResponse(SqlQueryResponse response, XContentBuilder builder) throws Exception { - // Make sure we only display JDBC-related data if JDBC is enabled - response.toXContent(builder, request); - return new BytesRestResponse(getStatus(response), builder); - } - }); + return channel -> client.execute(SqlQueryAction.INSTANCE, sqlRequest, new RestResponseListener(channel) { + @Override + public RestResponse buildResponse(SqlQueryResponse response) throws Exception { + XContentBuilder builder = XContentBuilder.builder(xContentType.xContent()); + response.toXContent(builder, request); + return new BytesRestResponse(RestStatus.OK, builder); + } + }); } - // The client accepts a text format - TextFormat text = TextFormat.fromMediaTypeOrFormat(request); - long startNanos = System.nanoTime(); + TextFormat textFormat = TextFormat.fromMediaTypeOrFormat(accept); + long startNanos = System.nanoTime(); return channel -> client.execute(SqlQueryAction.INSTANCE, sqlRequest, new RestResponseListener(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"; } -} \ No newline at end of file +} diff --git a/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java b/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java index 27258efc334..349a481cf66 100644 --- a/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/plugin/TextFormat.java +++ b/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) { - String format = request.param("format", request.header("Accept")); - - if (format == null) { - return PLAIN_TEXT; - } - + static TextFormat fromMediaTypeOrFormat(String accept) { for (TextFormat text : values()) { String contentType = text.contentType(); - if (contentType.equalsIgnoreCase(format) - || format.toLowerCase(Locale.ROOT).startsWith(contentType + ";") - || text.shortName().equalsIgnoreCase(format)) { + if (contentType.equalsIgnoreCase(accept) + || accept.toLowerCase(Locale.ROOT).startsWith(contentType + ";") + || 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; } -} \ No newline at end of file +} diff --git a/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java b/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java index 50f117d2665..1c6bbfa69e8 100644 --- a/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plugin/TextFormatTests.java +++ b/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 headers = new ArrayList<>(); headers.add(new ColumnInfo("index", "string", "keyword")); headers.add(new ColumnInfo("index", "number", "integer")); - + // values List> 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(); - } -} \ No newline at end of file +} diff --git a/plugin/src/test/resources/rest-api-spec/test/sql/sql.yml b/plugin/src/test/resources/rest-api-spec/test/sql/sql.yml index 033d3223002..551866b3b1e 100644 --- a/plugin/src/test/resources/rest-api-spec/test/sql/sql.yml +++ b/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 } - diff --git a/qa/sql/no-security/build.gradle b/qa/sql/no-security/build.gradle index 1452960548c..3a8b0ffde0a 100644 --- a/qa/sql/no-security/build.gradle +++ b/qa/sql/no-security/build.gradle @@ -5,4 +5,5 @@ integTestCluster { runqa { setting 'xpack.security.enabled', 'false' + setting 'xpack.license.self_generated.type', 'trial' } diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java index 32f9d5caeb4..e970fcaa88a 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java +++ b/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 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 (Strings.hasText(mode)) { - params.put("mode", mode); // JDBC or PLAIN mode + if (randomBoolean()) { + // We default to JSON but we force it randomly for extra coverage + 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 response = runSqlAsText("SELECT * FROM test"); + Tuple 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 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 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 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 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 runSqlAsText(String sql) throws IOException { - return runSqlAsText("", new StringEntity("{\"query\":\"" + sql + "\"}", ContentType.APPLICATION_JSON)); + private Tuple runSqlAsText(String sql, String accept) throws IOException { + return runSqlAsText("", new StringEntity("{\"query\":\"" + sql + "\"}", ContentType.APPLICATION_JSON), accept); } - private Tuple runSqlAsText(String sql, String acceptHeader) throws IOException { - return runSqlAsText("", new StringEntity("{\"query\":\"" + sql + "\"}", ContentType.APPLICATION_JSON), - new BasicHeader("Accept", acceptHeader)); - } - - private Tuple runSqlAsText(String suffix, HttpEntity sql, Header... headers) throws IOException { - Response response = client().performRequest("POST", "/_xpack/sql" + suffix, singletonMap("error_trace", "true"), sql, headers); + private Tuple runSqlAsText(String suffix, HttpEntity entity, String accept) throws IOException { + Response response = client().performRequest("POST", "/_xpack/sql" + suffix, singletonMap("error_trace", "true"), + entity, new BasicHeader("Accept", accept)); 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 runSqlAsTextFormat(String sql, String format) throws IOException { StringEntity entity = new StringEntity("{\"query\":\"" + sql + "\"}", ContentType.APPLICATION_JSON); - + Map 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 expected, Map 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"); } -} \ No newline at end of file +}