SQL: add support for passing query parameters in REST API calls #51029

astefan · 2020-01-15T10:43:22Z

Add support for PreparedStatement-like of passing values to a sql query.
The query string will use question mark placeholders (?) for any values that will be passed in through a new params request parameter:

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]
}

Addresses #42916.

…o 42916_impl

elasticmachine · 2020-01-15T10:43:25Z

Pinging @elastic/es-search (:Search/SQL)

costin

LGTM

costin · 2020-01-15T12:36:52Z

docs/reference/sql/endpoints/rest.asciidoc

+--------------------------------------------------
+// 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:


I would add a statement to mention that this is the recommended way of passing parameters to avoid hacking or SQL injection (which does not apply to us but nevertheless).

…o 42916_impl

matriv

LGTM. Left 2 comments

matriv · 2020-01-15T12:15:35Z

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

@@ -61,8 +61,8 @@ public SqlQueryRequest() {
    }

    public SqlQueryRequest(String query, List<SqlTypedParamValue> params, QueryBuilder filter, ZoneId zoneId,
-                           int fetchSize, TimeValue requestTimeout, TimeValue pageTimeout, Boolean columnar,
-                           String cursor, RequestInfo requestInfo, boolean fieldMultiValueLeniency, boolean indexIncludeFrozen) {
+            int fetchSize, TimeValue requestTimeout, TimeValue pageTimeout, Boolean columnar,


I think this formatting change is wrong?

I'll change it.

matriv · 2020-01-15T12:17:00Z

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

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


tokenLocation shouldn't be part of equals and hashCode?

bpintea · 2020-01-16T17:05:23Z

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

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


was the extra space intended?

Yes, to have a uniform formatting for the field parsers' declaration. See here the initial formatting.

bpintea · 2020-01-16T17:05:33Z

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

+                    // we are at the start of a value/type pair... hopefully
+                    SqlTypedParamValue s = SqlTypedParamValue.fromXContent(p);
+                    /*
+                     * Set the xcontentlocation for the first param just in case the first one doesn't meet the parsing rules


Was wondering if this was meant as ... for the first param that doesn't meet ..., since an object could occur at any point in the array.

I rephrased this section for, hopefully, better clarity.

bpintea · 2020-01-16T17:05:37Z

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

+                    if ((previousParam != null && previousParam.hasExplicitType() == false) || result.isEmpty()) {
+                        s.tokenLocation(loc);
+                    }
+                    result.add(s);
+                    previousParam = s;


This code executes in both if/else branches, it might be worth considering taking it out.
Another nit: a switch with all the top enums (object, string, number, bool, null) might also arguably simplify the code.

I've extracted the common code, but I kept the if/else approach. I find it a bit more clear than a switch.

bpintea · 2020-01-16T17:08:23Z

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

@@ -75,11 +81,11 @@ public AbstractSqlQueryRequest(String query, List<SqlTypedParamValue> params, Qu
        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, p -> AbstractSqlQueryRequest.parseParams(p), PARAMS, ValueType.VALUE_ARRAY);


Could the second arg also be a method reference?

bpintea

Looks great, only left a few questions and suggestions.

…o 42916_impl

the equality between TokenLocations is based on instance and not inner values.

…o 42916_impl

…tic#51029) * 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 45b8bf6)

…) (#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 45b8bf6)

…tic#51029) * 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.

astefan added 2 commits January 15, 2020 12:36

Add support for REST PreparedStatement-like query parameters

f3284a2

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

2ef9a55

…o 42916_impl

astefan added >feature :Analytics/SQL SQL querying v8.0.0 v7.6.0 labels Jan 15, 2020

astefan requested review from costin, bpintea and matriv January 15, 2020 10:43

costin approved these changes Jan 15, 2020

View reviewed changes

$@polyfractal$ polyfractal added v7.7.0 and removed v7.6.0 labels Jan 15, 2020

astefan added 2 commits January 16, 2020 10:12

Add statement about recommended way of passing values

595f505

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

08b74d0

…o 42916_impl

matriv approved these changes Jan 16, 2020

View reviewed changes

bpintea reviewed Jan 16, 2020

View reviewed changes

bpintea approved these changes Jan 16, 2020

View reviewed changes

astefan added 4 commits January 20, 2020 11:03

Address reviews

6eac38b

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

09570d6

…o 42916_impl

Revert adding tokenLocation to toString and hashCode methods since

a81d8ef

the equality between TokenLocations is based on instance and not inner values.

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

a059a0d

…o 42916_impl

astefan merged commit 45b8bf6 into elastic:master Jan 20, 2020

astefan mentioned this pull request Jan 20, 2020

SQL: add support for passing query parameters in REST API calls #51222

Merged

astefan deleted the 42916_impl branch January 20, 2020 14:45

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket elastic/elasticsearch-net#4525

Closed

38 tasks

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL: add support for passing query parameters in REST API calls #51029

SQL: add support for passing query parameters in REST API calls #51029

astefan commented Jan 15, 2020

elasticmachine commented Jan 15, 2020

costin left a comment

costin Jan 15, 2020

matriv left a comment

matriv Jan 15, 2020

astefan Jan 20, 2020

matriv Jan 15, 2020

bpintea Jan 16, 2020 •

edited

astefan Jan 20, 2020

bpintea Jan 16, 2020

astefan Jan 20, 2020

bpintea Jan 16, 2020

astefan Jan 20, 2020

bpintea Jan 16, 2020

astefan Jan 20, 2020

bpintea left a comment

SQL: add support for passing query parameters in REST API calls #51029

SQL: add support for passing query parameters in REST API calls #51029

Conversation

astefan commented Jan 15, 2020

elasticmachine commented Jan 15, 2020

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea Jan 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea left a comment

Choose a reason for hiding this comment

bpintea Jan 16, 2020 •

edited