From 2908b7e5fcdbee099494f7af8a7192d17aa6f238 Mon Sep 17 00:00:00 2001 From: Andrei Stefan Date: Mon, 20 Jan 2020 16:40:19 +0200 Subject: [PATCH] SQL: add support for passing query parameters in REST API calls (#51029) (#51222) * REST PreparedStatement-like query parameters are now supported in the form of an array of non-object, non-array values where ES SQL parser will try to infer the data type of the value being passed as parameter. (cherry picked from commit 45b8bf619aecb1c03d7bc0cf06928dcc36005a66) --- docs/reference/sql/endpoints/rest.asciidoc | 37 +++- .../xpack/sql/qa/rest/RestSqlTestCase.java | 5 +- .../sql/action/AbstractSqlQueryRequest.java | 96 +++++++++- .../xpack/sql/action/SqlQueryRequest.java | 4 +- .../xpack/sql/action/SqlTranslateRequest.java | 1 + .../sql/action/SqlQueryRequestTests.java | 170 ++++++++++++++---- .../sql/action/SqlRequestParsersTests.java | 133 +++++++++++++- .../xpack/sql/proto/SqlTypedParamValue.java | 32 +++- 8 files changed, 427 insertions(+), 51 deletions(-) diff --git a/docs/reference/sql/endpoints/rest.asciidoc b/docs/reference/sql/endpoints/rest.asciidoc index 6001005f3de..2f9be5da17d 100644 --- a/docs/reference/sql/endpoints/rest.asciidoc +++ b/docs/reference/sql/endpoints/rest.asciidoc @@ -8,6 +8,7 @@ * <> * <> * <> +* <> * <> [[sql-rest-overview]] @@ -337,7 +338,7 @@ Which will like return the [[sql-rest-filtering]] === Filtering using {es} query DSL -You can filter the results that SQL will run on using a standard +One can filter the results that SQL will run on using a standard {es} query DSL by specifying the query in the filter parameter. @@ -442,6 +443,36 @@ Which looks like: -------------------------------------------------- // TESTRESPONSE[s/46ToAwFzQERYRjFaWEo1UVc1a1JtVjBZMmdCQUFBQUFBQUFBQUVXWjBaNlFXbzNOV0pVY21Wa1NUZDJhV2t3V2xwblp3PT3\/\/\/\/\/DwQBZgZhdXRob3IBBHRleHQAAAFmBG5hbWUBBHRleHQAAAFmCnBhZ2VfY291bnQBBGxvbmcBAAFmDHJlbGVhc2VfZGF0ZQEIZGF0ZXRpbWUBAAEP/$body.cursor/] +[[sql-rest-params]] +=== Passing parameters to a query + +Using values in a query condition, for example, or in a `HAVING` statement can be done "inline", +by integrating the value in the query string itself: + +[source,console] +-------------------------------------------------- +POST /_sql?format=txt +{ + "query": "SELECT YEAR(release_date) AS year FROM library WHERE page_count > 300 AND author = 'Frank Herbert' GROUP BY year HAVING COUNT(*) > 0" +} +-------------------------------------------------- +// TEST[setup:library] + +or it can be done by extracting the values in a separate list of parameters and using question mark placeholders (`?`) in the query string: + +[source,console] +-------------------------------------------------- +POST /_sql?format=txt +{ + "query": "SELECT YEAR(release_date) AS year FROM library WHERE page_count > ? AND author = ? GROUP BY year HAVING COUNT(*) > ?", + "params": [300, "Frank Herbert", 0] +} +-------------------------------------------------- +// TEST[setup:library] + +[IMPORTANT] +The recommended way of passing values to a query is with question mark placeholders, to avoid any attempts of hacking or SQL injection. + [[sql-rest-fields]] === Supported REST parameters @@ -495,6 +526,10 @@ More information available https://docs.oracle.com/javase/8/docs/api/java/time/Z |false |Whether to include <> in the query execution or not (default). +|params +|none +|Optional list of parameters to replace question mark (`?`) placeholders inside the query. + |=== Do note that most parameters (outside the timeout and `columnar` ones) make sense only during the initial query - any follow-up pagination request only requires the `cursor` parameter as explained in the <> chapter. diff --git a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java index 46c5bd9d406..efffb069f22 100644 --- a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java +++ b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java @@ -544,8 +544,11 @@ public abstract class RestSqlTestCase extends BaseRestSqlTestCase implements Err } else { expected.put("rows", Arrays.asList(Arrays.asList("foo", 10))); } + + String params = mode.equals("jdbc") ? "{\"type\": \"integer\", \"value\": 10}, {\"type\": \"keyword\", \"value\": \"foo\"}" : + "10, \"foo\""; assertResponse(expected, runSql(new StringEntity("{\"query\":\"SELECT test, ? param FROM test WHERE test = ?\", " + - "\"params\":[{\"type\": \"integer\", \"value\": 10}, {\"type\": \"keyword\", \"value\": \"foo\"}]" + "\"params\":[" + params + "]" + mode(mode) + columnarParameter(columnar) + "}", ContentType.APPLICATION_JSON), StringUtils.EMPTY, mode)); } diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java index 776f64f2962..14857c95b3d 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/AbstractSqlQueryRequest.java @@ -12,7 +12,12 @@ 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.ObjectParser.ValueType; import org.elasticsearch.common.xcontent.ToXContentFragment; +import org.elasticsearch.common.xcontent.XContentLocation; +import org.elasticsearch.common.xcontent.XContentParseException; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentParser.Token; import org.elasticsearch.index.query.AbstractQueryBuilder; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.xpack.sql.proto.Mode; @@ -22,6 +27,7 @@ import org.elasticsearch.xpack.sql.proto.SqlTypedParamValue; import java.io.IOException; import java.time.ZoneId; +import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Objects; @@ -75,11 +81,11 @@ public abstract class AbstractSqlQueryRequest extends AbstractSqlRequest impleme parser.declareString(AbstractSqlQueryRequest::query, QUERY); parser.declareString((request, mode) -> request.mode(Mode.fromString(mode)), MODE); parser.declareString((request, clientId) -> request.clientId(clientId), CLIENT_ID); - parser.declareObjectArray(AbstractSqlQueryRequest::params, (p, c) -> SqlTypedParamValue.fromXContent(p), PARAMS); + parser.declareField(AbstractSqlQueryRequest::params, AbstractSqlQueryRequest::parseParams, PARAMS, ValueType.VALUE_ARRAY); parser.declareString((request, zoneId) -> request.zoneId(ZoneId.of(zoneId)), TIME_ZONE); parser.declareInt(AbstractSqlQueryRequest::fetchSize, FETCH_SIZE); parser.declareString((request, timeout) -> request.requestTimeout(TimeValue.parseTimeValue(timeout, Protocol.REQUEST_TIMEOUT, - "request_timeout")), REQUEST_TIMEOUT); + "request_timeout")), REQUEST_TIMEOUT); parser.declareString( (request, timeout) -> request.pageTimeout(TimeValue.parseTimeValue(timeout, Protocol.PAGE_TIMEOUT, "page_timeout")), PAGE_TIMEOUT); @@ -117,6 +123,87 @@ public abstract class AbstractSqlQueryRequest extends AbstractSqlRequest impleme this.params = params; return this; } + + private static List parseParams(XContentParser p) throws IOException { + List result = new ArrayList<>(); + Token token = p.currentToken(); + + if (token == Token.START_ARRAY) { + Object value = null; + String type = null; + SqlTypedParamValue previousParam = null; + SqlTypedParamValue currentParam = null; + + while ((token = p.nextToken()) != Token.END_ARRAY) { + XContentLocation loc = p.getTokenLocation(); + + if (token == Token.START_OBJECT) { + // we are at the start of a value/type pair... hopefully + currentParam = SqlTypedParamValue.fromXContent(p); + /* + * Always set the xcontentlocation for the first param just in case the first one happens to not meet the parsing rules + * that are checked later in validateParams method. + * Also, set the xcontentlocation of the param that is different from the previous param in list when it comes to + * its type being explicitly set or inferred. + */ + if ((previousParam != null && previousParam.hasExplicitType() == false) || result.isEmpty()) { + currentParam.tokenLocation(loc); + } + } else { + if (token == Token.VALUE_STRING) { + value = p.text(); + type = "keyword"; + } else if (token == Token.VALUE_NUMBER) { + XContentParser.NumberType numberType = p.numberType(); + if (numberType == XContentParser.NumberType.INT) { + value = p.intValue(); + type = "integer"; + } else if (numberType == XContentParser.NumberType.LONG) { + value = p.longValue(); + type = "long"; + } else if (numberType == XContentParser.NumberType.FLOAT) { + value = p.floatValue(); + type = "float"; + } else if (numberType == XContentParser.NumberType.DOUBLE) { + value = p.doubleValue(); + type = "double"; + } + } else if (token == Token.VALUE_BOOLEAN) { + value = p.booleanValue(); + type = "boolean"; + } else if (token == Token.VALUE_NULL) { + value = null; + type = "null"; + } else { + throw new XContentParseException(loc, "Failed to parse object: unexpected token [" + token + "] found"); + } + + currentParam = new SqlTypedParamValue(type, value, false); + if ((previousParam != null && previousParam.hasExplicitType() == true) || result.isEmpty()) { + currentParam.tokenLocation(loc); + } + } + + result.add(currentParam); + previousParam = currentParam; + } + } + + return result; + } + + protected static void validateParams(List params, Mode mode) { + for(SqlTypedParamValue param : params) { + if (Mode.isDriver(mode) && param.hasExplicitType() == false) { + throw new XContentParseException(param.tokenLocation(), "[params] must be an array where each entry is an object with a " + + "value/type pair"); + } + if (Mode.isDriver(mode) == false && param.hasExplicitType() == true) { + throw new XContentParseException(param.tokenLocation(), "[params] must be an array where each entry is a single field (no " + + "objects supported)"); + } + } + } /** * The client's time zone @@ -204,10 +291,11 @@ public abstract class AbstractSqlQueryRequest extends AbstractSqlRequest impleme public static void writeSqlTypedParamValue(StreamOutput out, SqlTypedParamValue value) throws IOException { out.writeString(value.type); out.writeGenericValue(value.value); + out.writeBoolean(value.hasExplicitType()); } public static SqlTypedParamValue readSqlTypedParamValue(StreamInput in) throws IOException { - return new SqlTypedParamValue(in.readString(), in.readGenericValue()); + return new SqlTypedParamValue(in.readString(), in.readGenericValue(), in.readBoolean()); } @Override @@ -248,6 +336,6 @@ public abstract class AbstractSqlQueryRequest extends AbstractSqlRequest impleme @Override public int hashCode() { - return Objects.hash(super.hashCode(), query, zoneId, fetchSize, requestTimeout, pageTimeout, filter); + return Objects.hash(super.hashCode(), query, params, zoneId, fetchSize, requestTimeout, pageTimeout, filter); } } diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java index 933cccb686a..a338410b727 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlQueryRequest.java @@ -188,6 +188,8 @@ public class SqlQueryRequest extends AbstractSqlQueryRequest { } public static SqlQueryRequest fromXContent(XContentParser parser) { - return PARSER.apply(parser, null); + SqlQueryRequest request = PARSER.apply(parser, null); + validateParams(request.params(), request.mode()); + return request; } } \ No newline at end of file diff --git a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlTranslateRequest.java b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlTranslateRequest.java index 9bf6f0db194..708779ec6e3 100644 --- a/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlTranslateRequest.java +++ b/x-pack/plugin/sql/sql-action/src/main/java/org/elasticsearch/xpack/sql/action/SqlTranslateRequest.java @@ -58,6 +58,7 @@ public class SqlTranslateRequest extends AbstractSqlQueryRequest { public static SqlTranslateRequest fromXContent(XContentParser parser) { SqlTranslateRequest request = PARSER.apply(parser, null); + validateParams(request.params(), request.mode()); return request; } diff --git a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryRequestTests.java b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryRequestTests.java index 992f55e5a3b..e58154a2e08 100644 --- a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryRequestTests.java +++ b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlQueryRequestTests.java @@ -5,31 +5,37 @@ */ package org.elasticsearch.xpack.sql.action; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.search.SearchModule; -import org.elasticsearch.test.AbstractSerializingTestCase; +import org.elasticsearch.test.AbstractWireSerializingTestCase; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.sql.proto.Mode; +import org.elasticsearch.xpack.sql.proto.Protocol; import org.elasticsearch.xpack.sql.proto.RequestInfo; import org.elasticsearch.xpack.sql.proto.SqlTypedParamValue; import org.junit.Before; +import java.io.IOException; import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.function.Consumer; import java.util.function.Supplier; +import static org.elasticsearch.test.AbstractXContentTestCase.xContentTester; import static org.elasticsearch.xpack.sql.action.SqlTestUtils.randomFilter; import static org.elasticsearch.xpack.sql.action.SqlTestUtils.randomFilterOrNull; import static org.elasticsearch.xpack.sql.proto.RequestInfo.CLIENT_IDS; -public class SqlQueryRequestTests extends AbstractSerializingTestCase { +public class SqlQueryRequestTests extends AbstractWireSerializingTestCase { public RequestInfo requestInfo; @@ -60,49 +66,16 @@ public class SqlQueryRequestTests extends AbstractSerializingTestCase randomParameters() { - if (randomBoolean()) { - return Collections.emptyList(); - } else { - int len = randomIntBetween(1, 10); - List arr = new ArrayList<>(len); - for (int i = 0; i < len; i++) { - @SuppressWarnings("unchecked") Supplier supplier = randomFrom( - () -> new SqlTypedParamValue("boolean", randomBoolean()), - () -> new SqlTypedParamValue("long", randomLong()), - () -> new SqlTypedParamValue("double", randomDouble()), - () -> new SqlTypedParamValue("null", null), - () -> new SqlTypedParamValue("keyword", randomAlphaOfLength(10)) - ); - arr.add(supplier.get()); - } - return Collections.unmodifiableList(arr); - } - } - @Override protected Writeable.Reader instanceReader() { return SqlQueryRequest::new; } - private TimeValue randomTV() { - return TimeValue.parseTimeValue(randomTimeValue(), null, "test"); - } - - @Override - protected SqlQueryRequest doParseInstance(XContentParser parser) { - return SqlQueryRequest.fromXContent(parser); - } - @Override protected SqlQueryRequest mutateInstance(SqlQueryRequest instance) { @SuppressWarnings("unchecked") Consumer mutator = randomFrom( - request -> request.requestInfo(randomValueOtherThan(request.requestInfo(), this::randomRequestInfo)), + request -> mutateRequestInfo(instance, request), request -> request.query(randomValueOtherThan(request.query(), () -> randomAlphaOfLength(5))), request -> request.params(randomValueOtherThan(request.params(), this::randomParameters)), request -> request.zoneId(randomValueOtherThan(request.zoneId(), ESTestCase::randomZone)), @@ -119,10 +92,135 @@ public class SqlQueryRequestTests extends AbstractSerializingTestCase false) + .assertEqualsConsumer(this::assertEqualInstances) + .assertToXContentEquivalence(true) + .test(); + } public void testTimeZoneNullException() { final SqlQueryRequest sqlQueryRequest = createTestInstance(); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> sqlQueryRequest.zoneId(null)); assertEquals("time zone may not be null.", e.getMessage()); } + + private RequestInfo randomRequestInfo() { + return new RequestInfo(randomFrom(Mode.values()), randomFrom(randomFrom(CLIENT_IDS), requestInfo.clientId())); + } + + private TimeValue randomTV() { + return TimeValue.parseTimeValue(randomTimeValue(), null, "test"); + } + + public List randomParameters() { + if (randomBoolean()) { + return Collections.emptyList(); + } else { + int len = randomIntBetween(1, 10); + List arr = new ArrayList<>(len); + boolean hasExplicitType = Mode.isDriver(this.requestInfo.mode()); + for (int i = 0; i < len; i++) { + @SuppressWarnings("unchecked") Supplier supplier = randomFrom( + () -> new SqlTypedParamValue("boolean", randomBoolean(), hasExplicitType), + () -> new SqlTypedParamValue("long", randomLong(), hasExplicitType), + () -> new SqlTypedParamValue("double", randomDouble(), hasExplicitType), + () -> new SqlTypedParamValue("null", null, hasExplicitType), + () -> new SqlTypedParamValue("keyword", randomAlphaOfLength(10), hasExplicitType) + ); + arr.add(supplier.get()); + } + return Collections.unmodifiableList(arr); + } + } + + private SqlQueryRequest doParseInstance(XContentParser parser) { + return SqlQueryRequest.fromXContent(parser); + } + + /** + * This is needed because {@link SqlQueryRequest#toXContent(XContentBuilder, org.elasticsearch.common.xcontent.ToXContent.Params)} + * is not serializing {@link SqlTypedParamValue} according to the request's {@link Mode} and it shouldn't, in fact. + * For testing purposes, different serializing methods for {@link SqlTypedParamValue} are necessary so that + * {@link SqlQueryRequest#fromXContent(XContentParser)} populates {@link SqlTypedParamValue#hasExplicitType()} + * properly. + */ + private static void toXContent(SqlQueryRequest request, XContentBuilder builder) throws IOException { + builder.startObject(); + if (request.query() != null) { + builder.field("query", request.query()); + } + builder.field("mode", request.mode().toString()); + if (request.clientId() != null) { + builder.field("client_id", request.clientId()); + } + if (request.params() != null && request.params().isEmpty() == false) { + builder.startArray("params"); + for (SqlTypedParamValue val : request.params()) { + if (Mode.isDriver(request.mode())) { + builder.startObject(); + builder.field("type", val.type); + builder.field("value", val.value); + builder.endObject(); + } else { + builder.value(val.value); + } + } + builder.endArray(); + } + if (request.zoneId() != null) { + builder.field("time_zone", request.zoneId().getId()); + } + if (request.fetchSize() != Protocol.FETCH_SIZE) { + builder.field("fetch_size", request.fetchSize()); + } + if (request.requestTimeout() != Protocol.REQUEST_TIMEOUT) { + builder.field("request_timeout", request.requestTimeout().getStringRep()); + } + if (request.pageTimeout() != Protocol.PAGE_TIMEOUT) { + builder.field("page_timeout", request.pageTimeout().getStringRep()); + } + if (request.filter() != null) { + builder.field("filter"); + request.filter().toXContent(builder, ToXContent.EMPTY_PARAMS); + } + if (request.columnar() != null) { + builder.field("columnar", request.columnar()); + } + if (request.fieldMultiValueLeniency()) { + builder.field("field_multi_value_leniency", request.fieldMultiValueLeniency()); + } + if (request.indexIncludeFrozen()) { + builder.field("index_include_frozen", request.indexIncludeFrozen()); + } + if (request.binaryCommunication() != null) { + builder.field("binary_format", request.binaryCommunication()); + } + if (request.cursor() != null) { + builder.field("cursor", request.cursor()); + } + builder.endObject(); + } } diff --git a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java index c9bdc8d670c..34245945e50 100644 --- a/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java +++ b/x-pack/plugin/sql/sql-action/src/test/java/org/elasticsearch/xpack/sql/action/SqlRequestParsersTests.java @@ -9,6 +9,7 @@ package org.elasticsearch.xpack.sql.action; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.DeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.common.xcontent.XContentParseException; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.test.ESTestCase; @@ -21,7 +22,7 @@ import java.util.List; import java.util.function.Consumer; import java.util.function.Function; -import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.CoreMatchers.containsString; public class SqlRequestParsersTests extends ESTestCase { @@ -105,23 +106,139 @@ public class SqlRequestParsersTests extends ESTestCase { SqlQueryRequest::fromXContent); Mode randomMode = randomFrom(Mode.values()); + String params; + List list = new ArrayList<>(1); + + if (Mode.isDriver(randomMode)) { + params = "{\"value\":123, \"type\":\"whatever\"}"; + list.add(new SqlTypedParamValue("whatever", 123, true)); + } else { + params = "123"; + list.add(new SqlTypedParamValue("integer", 123, false)); + } + SqlQueryRequest request = generateRequest("{\"cursor\" : \"whatever\", \"mode\" : \"" + randomMode.toString() + "\", \"client_id\" : \"bla\"," - + "\"query\":\"select\",\"params\":[{\"value\":123, \"type\":\"whatever\"}], \"time_zone\":\"UTC\"," + + "\"query\":\"select\"," + + "\"params\":[" + params + "]," + + " \"time_zone\":\"UTC\"," + "\"request_timeout\":\"5s\",\"page_timeout\":\"10s\"}", SqlQueryRequest::fromXContent); assertNull(request.clientId()); assertEquals(randomMode, request.mode()); assertEquals("whatever", request.cursor()); assertEquals("select", request.query()); - - List list = new ArrayList<>(1); - list.add(new SqlTypedParamValue("whatever", 123)); + assertEquals(list, request.params()); assertEquals("UTC", request.zoneId().getId()); assertEquals(TimeValue.parseTimeValue("5s", "request_timeout"), request.requestTimeout()); assertEquals(TimeValue.parseTimeValue("10s", "page_timeout"), request.pageTimeout()); } + public void testParamsSuccessfulParsingInDriverMode() throws IOException { + Mode driverMode = randomValueOtherThanMany((m) -> Mode.isDriver(m) == false, () -> randomFrom(Mode.values())); + String json = "{" + + " \"params\":[{\"type\":\"integer\",\"value\":35000}," + + " {\"type\":\"date\",\"value\":\"1960-01-01\"}," + + " {\"type\":\"boolean\",\"value\":false}," + + " {\"type\":\"keyword\",\"value\":\"foo\"}]," + + " \"mode\": \"" + driverMode.toString() + "\"" + + "}"; + SqlQueryRequest request = generateRequest(json, SqlQueryRequest::fromXContent); + List params = request.params(); + assertEquals(4, params.size()); + + assertEquals(35000, params.get(0).value); + assertEquals("integer", params.get(0).type); + assertTrue(params.get(0).hasExplicitType()); + + assertEquals("1960-01-01", params.get(1).value); + assertEquals("date", params.get(1).type); + assertTrue(params.get(1).hasExplicitType()); + + assertEquals(false, params.get(2).value); + assertEquals("boolean", params.get(2).type); + assertTrue(params.get(2).hasExplicitType()); + + assertEquals("foo", params.get(3).value); + assertEquals("keyword", params.get(3).type); + assertTrue(params.get(3).hasExplicitType()); + } + + public void testParamsSuccessfulParsingInNonDriverMode() throws IOException { + Mode nonDriverMode = randomValueOtherThanMany(Mode::isDriver, () -> randomFrom(Mode.values())); + String json = "{" + + " \"params\":[35000,\"1960-01-01\",false,\"foo\"]," + + " \"mode\": \"" + nonDriverMode.toString() + "\"" + + "}"; + SqlQueryRequest request = generateRequest(json, SqlQueryRequest::fromXContent); + List params = request.params(); + assertEquals(4, params.size()); + + assertEquals(35000, params.get(0).value); + assertEquals("integer", params.get(0).type); + assertFalse(params.get(0).hasExplicitType()); + + assertEquals("1960-01-01", params.get(1).value); + assertEquals("keyword", params.get(1).type); + assertFalse(params.get(1).hasExplicitType()); + + assertEquals(false, params.get(2).value); + assertEquals("boolean", params.get(2).type); + assertFalse(params.get(2).hasExplicitType()); + + assertEquals("foo", params.get(3).value); + assertEquals("keyword", params.get(3).type); + assertFalse(params.get(3).hasExplicitType()); + } + + public void testParamsParsingFailure_QueryRequest_NonDriver() throws IOException { + Mode m = randomValueOtherThanMany(Mode::isDriver, () -> randomFrom(Mode.values())); + assertXContentParsingErrorMessage("{\"params\":[{\"whatever\":35000},\"1960-01-01\",false,\"foo\"],\"mode\": \"" + + m.toString() + "\"}", + "[sql/query] failed to parse field [params]", + SqlQueryRequest::fromXContent); + assertXContentParsingErrorMessage("{\"params\":[350.123,\"1960-01-01\",{\"foobar\":false},\"foo\"],\"mode\": \"}" + + m.toString() + "\"}", + "[sql/query] failed to parse field [params]", + SqlQueryRequest::fromXContent); + assertXContentParsingErrorMessage("{\"mode\": \"" + m.toString() + "\",\"params\":[350.123,\"1960-01-01\",false," + + "{\"type\":\"keyword\",\"value\":\"foo\"}]}", + "[params] must be an array where each entry is a single field (no objects supported)", + SqlQueryRequest::fromXContent); + } + + public void testParamsParsingFailure_TranslateRequest_NonDriver() throws IOException { + Mode m = randomValueOtherThanMany(Mode::isDriver, () -> randomFrom(Mode.values())); + assertXContentParsingErrorMessage("{\"params\":[{\"whatever\":35000},\"1960-01-01\",false,\"foo\"],\"mode\": \"" + + m.toString() + "\"}", + "[sql/query] failed to parse field [params]", + SqlTranslateRequest::fromXContent); + assertXContentParsingErrorMessage("{\"params\":[350.123,\"1960-01-01\",{\"foobar\":false},\"foo\"],\"mode\": \"}" + + m.toString() + "\"}", + "[sql/query] failed to parse field [params]", + SqlTranslateRequest::fromXContent); + assertXContentParsingErrorMessage("{\"mode\": \"" + m.toString() + "\",\"params\":[350.123,\"1960-01-01\",false," + + "{\"type\":\"keyword\",\"value\":\"foo\"}]}", + "[params] must be an array where each entry is a single field (no objects supported)", + SqlTranslateRequest::fromXContent); + } + + public void testParamsParsingFailure_Driver() throws IOException { + Mode m = randomValueOtherThanMany((t) -> Mode.isDriver(t) == false, () -> randomFrom(Mode.values())); + assertXContentParsingErrorMessage("{\"params\":[35000,{\"value\":\"1960-01-01\",\"type\":\"date\"},{\"value\":\"foo\"," + + "\"type\":\"keyword\"}],\"mode\": \"" + m.toString() + "\"}", + "[params] must be an array where each entry is an object with a value/type pair", + SqlQueryRequest::fromXContent); + assertXContentParsingErrorMessage("{\"params\":[{\"value\":10,\"type\":\"integer\"},{\"value\":\"1960-01-01\",\"type\":\"date\"}," + + "false,\"foo\"],\"mode\": \"" + m.toString() + "\"}", + "[params] must be an array where each entry is an object with a value/type pair", + SqlQueryRequest::fromXContent); + assertXContentParsingErrorMessage("{\"mode\": \"" + m.toString() + "\",\"params\":[{\"value\":10,\"type\":\"integer\"}," + + "{\"value\":\"1960-01-01\",\"type\":\"date\"},{\"foo\":\"bar\"}]}", + "[sql/query] failed to parse field [params]", + SqlQueryRequest::fromXContent); + } + private R generateRequest(String json, Function fromXContent) throws IOException { XContentParser parser = parser(json); @@ -140,6 +257,12 @@ public class SqlRequestParsersTests extends ESTestCase { assertThat(e.getCause().getMessage(), containsString(errorMessage)); } + private void assertXContentParsingErrorMessage(String json, String errorMessage, Consumer consumer) throws IOException { + XContentParser parser = parser(json); + Exception e = expectThrows(XContentParseException.class, () -> consumer.accept(parser)); + assertThat(e.getMessage(), containsString(errorMessage)); + } + private XContentParser parser(String content) throws IOException { XContentType xContentType = XContentType.JSON; return xContentType.xContent().createParser( diff --git a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlTypedParamValue.java b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlTypedParamValue.java index caa230d0635..89cf13eb52e 100644 --- a/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlTypedParamValue.java +++ b/x-pack/plugin/sql/sql-proto/src/main/java/org/elasticsearch/xpack/sql/proto/SqlTypedParamValue.java @@ -10,6 +10,7 @@ import org.elasticsearch.common.xcontent.ConstructingObjectParser; import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentLocation; import org.elasticsearch.common.xcontent.XContentParser; import java.io.IOException; @@ -37,10 +38,33 @@ public class SqlTypedParamValue implements ToXContentObject { public final Object value; public final String type; + private boolean hasExplicitType; // the type is explicitly set in the request or inferred by the parser + private XContentLocation tokenLocation; // location of the token failing the parsing rules public SqlTypedParamValue(String type, Object value) { + this(type, value, true); + } + + public SqlTypedParamValue(String type, Object value, boolean hasExplicitType) { this.value = value; this.type = type; + this.hasExplicitType = hasExplicitType; + } + + public boolean hasExplicitType() { + return hasExplicitType; + } + + public void hasExplicitType(boolean hasExplicitType) { + this.hasExplicitType = hasExplicitType; + } + + public XContentLocation tokenLocation() { + return tokenLocation; + } + + public void tokenLocation(XContentLocation tokenLocation) { + this.tokenLocation = tokenLocation; } @Override @@ -65,16 +89,18 @@ public class SqlTypedParamValue implements ToXContentObject { return false; } SqlTypedParamValue that = (SqlTypedParamValue) o; - return Objects.equals(value, that.value) && Objects.equals(type, that.type); + return Objects.equals(value, that.value) + && Objects.equals(type, that.type) + && Objects.equals(hasExplicitType, that.hasExplicitType); } @Override public int hashCode() { - return Objects.hash(value, type); + return Objects.hash(value, type, hasExplicitType); } @Override public String toString() { - return String.valueOf(value) + "[" + type + "]"; + return String.valueOf(value) + " [" + type + "][" + hasExplicitType + "][" + tokenLocation + "]"; } } \ No newline at end of file