-
Notifications
You must be signed in to change notification settings - Fork 25.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SQL: add support for passing query parameters in REST API calls #51029
Changes from 2 commits
f3284a2
2ef9a55
595f505
08b74d0
6eac38b
09570d6
a81d8ef
a059a0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,12 @@ | |
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 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 @@ protected static <R extends AbstractSqlQueryRequest> ObjectParser<R, Void> objec | |
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could the second arg also be a method reference? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed |
||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. was the extra space intended? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, to have a uniform formatting for the field parsers' declaration. See here the initial formatting. |
||
parser.declareString( | ||
(request, timeout) -> request.pageTimeout(TimeValue.parseTimeValue(timeout, Protocol.PAGE_TIMEOUT, "page_timeout")), | ||
PAGE_TIMEOUT); | ||
|
@@ -117,6 +123,86 @@ public AbstractSqlQueryRequest params(List<SqlTypedParamValue> params) { | |
this.params = params; | ||
return this; | ||
} | ||
|
||
private static List<SqlTypedParamValue> parseParams(XContentParser p) throws IOException { | ||
List<SqlTypedParamValue> result = new ArrayList<>(); | ||
Token token = p.currentToken(); | ||
|
||
if (token == Token.START_ARRAY) { | ||
Object value = null; | ||
String type = null; | ||
SqlTypedParamValue previousParam = 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 | ||
SqlTypedParamValue s = SqlTypedParamValue.fromXContent(p); | ||
/* | ||
* Set the xcontentlocation for the first param just in case the first one doesn't meet the parsing rules | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was wondering if this was meant as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I rephrased this section for, hopefully, better clarity. |
||
* that are checked later in validateParams method and, also, 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()) { | ||
s.tokenLocation(loc); | ||
} | ||
result.add(s); | ||
previousParam = s; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code executes in both if/else branches, it might be worth considering taking it out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've extracted the common code, but I kept the if/else approach. I find it a bit more clear than a |
||
} 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"); | ||
} | ||
|
||
SqlTypedParamValue s = new SqlTypedParamValue(type, value, false); | ||
if ((previousParam != null && previousParam.hasExplicitType() == true) || result.isEmpty()) { | ||
s.tokenLocation(loc); | ||
} | ||
result.add(s); | ||
previousParam = s; | ||
} | ||
} | ||
} | ||
|
||
return result; | ||
} | ||
|
||
protected static void validateParams(List<SqlTypedParamValue> 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 +290,11 @@ public AbstractSqlQueryRequest(StreamInput in) throws IOException { | |
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 +335,6 @@ public boolean equals(Object o) { | |
|
||
@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); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this formatting change is wrong? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll change it. |
||
String cursor, RequestInfo requestInfo, boolean fieldMultiValueLeniency, boolean indexIncludeFrozen) { | ||
super(query, params, filter, zoneId, fetchSize, requestTimeout, pageTimeout, requestInfo); | ||
this.cursor = cursor; | ||
this.columnar = columnar; | ||
|
@@ -188,6 +188,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws | |
} | ||
|
||
public static SqlQueryRequest fromXContent(XContentParser parser) { | ||
return PARSER.apply(parser, null); | ||
SqlQueryRequest request = PARSER.apply(parser, null); | ||
validateParams(request.params(), request.mode()); | ||
return request; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).