Skip to content

SQL: move requests' parameters to requests JSON body #36149

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.

Sign up for GitHub

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

Merged
merged 15 commits into from
Dec 11, 2018
Merged

SQL: move requests' parameters to requests JSON body #36149

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
bda5654
Move SQL request parameters from http url params to JSON body
astefan Nov 29, 2018
6c69d77
Removed leftover commented code
astefan Dec 3, 2018
375caf4
Adressing review comments
astefan Dec 4, 2018
b32f545
Merge branch 'master' of https://github.com/elastic/elasticsearch int…
astefan Dec 4, 2018
5720e44
Addressed reviews
astefan Dec 5, 2018
9e0b05d
Merge branch 'master' of https://github.com/elastic/elasticsearch int…
astefan Dec 5, 2018
e8298c5
Further polishing
astefan Dec 5, 2018
697f836
Merge branch 'master' of https://github.com/elastic/elasticsearch int…
astefan Dec 5, 2018
5fc74eb
Merge branch 'master' of https://github.com/elastic/elasticsearch int…
astefan Dec 6, 2018
358a944
Merge branch 'master' of https://github.com/elastic/elasticsearch int…
astefan Dec 6, 2018
945bab2
Cosmetic changes
astefan Dec 10, 2018
51dee0d
Merge branch 'master' of https://github.com/elastic/elasticsearch int…
astefan Dec 10, 2018
5a1bc38
Revert back Strings class usage
astefan Dec 10, 2018
1a1f84b
Handle request body in `source` parameter as well
astefan Dec 11, 2018
1dc2a64
Merge branch 'master' of https://github.com/elastic/elasticsearch int…
astefan Dec 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions ...ulti-node/src/test/java/org/elasticsearch/xpack/sql/qa/multi_node/RestSqlMultinodeIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

import static java.util.Collections.singletonList;
import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.columnInfo;
import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.mode;
import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.randomMode;

/**
Expand Down Expand Up @@ -111,10 +112,7 @@ private void assertCount(RestClient client, int count) throws IOException {
expected.put("rows", singletonList(singletonList(count)));

Request request = new Request("POST", "/_sql");
if (false == mode.isEmpty()) {
request.addParameter("mode", mode);
}
request.setJsonEntity("{\"query\": \"SELECT COUNT(*) FROM test\"}");
request.setJsonEntity("{\"query\": \"SELECT COUNT(*) FROM test\"" + mode(mode) + "}");
Map<String, Object> actual = responseToMap(client.performRequest(request));

if (false == expected.equals(actual)) {
Expand Down
35 changes: 18 additions & 17 deletions .../qa/security/src/test/java/org/elasticsearch/xpack/sql/qa/security/RestSqlSecurityIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.util.stream.Collectors;

import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.columnInfo;
import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.mode;
import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.randomMode;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.empty;
Expand Down Expand Up @@ -66,21 +67,23 @@ public void expectMatchesAdmin(String adminSql, String user, String userSql) thr
@Override
public void expectScrollMatchesAdmin(String adminSql, String user, String userSql) throws Exception {
String mode = randomMode();
Map<String, Object> adminResponse = runSql(null, mode,
new StringEntity("{\"query\": \"" + adminSql + "\", \"fetch_size\": 1}", ContentType.APPLICATION_JSON));
Map<String, Object> otherResponse = runSql(user, mode,
new StringEntity("{\"query\": \"" + adminSql + "\", \"fetch_size\": 1}", ContentType.APPLICATION_JSON));
Map<String, Object> adminResponse = runSql(null,
new StringEntity("{\"query\": \"" + adminSql + "\", \"fetch_size\": 1" + mode(mode) + "}",
ContentType.APPLICATION_JSON));
Map<String, Object> otherResponse = runSql(user,
new StringEntity("{\"query\": \"" + adminSql + "\", \"fetch_size\": 1" + mode(mode) + "}",
ContentType.APPLICATION_JSON));

String adminCursor = (String) adminResponse.remove("cursor");
String otherCursor = (String) otherResponse.remove("cursor");
assertNotNull(adminCursor);
assertNotNull(otherCursor);
assertResponse(adminResponse, otherResponse);
while (true) {
adminResponse = runSql(null, mode,
new StringEntity("{\"cursor\": \"" + adminCursor + "\"}", ContentType.APPLICATION_JSON));
otherResponse = runSql(user, mode,
new StringEntity("{\"cursor\": \"" + otherCursor + "\"}", ContentType.APPLICATION_JSON));
adminResponse = runSql(null,
new StringEntity("{\"cursor\": \"" + adminCursor + "\"" + mode(mode) + "}", ContentType.APPLICATION_JSON));
otherResponse = runSql(user,
new StringEntity("{\"cursor\": \"" + otherCursor + "\"" + mode(mode) + "}", ContentType.APPLICATION_JSON));
adminCursor = (String) adminResponse.remove("cursor");
otherCursor = (String) otherResponse.remove("cursor");
assertResponse(adminResponse, otherResponse);
Expand Down Expand Up @@ -173,14 +176,11 @@ public void checkNoMonitorMain(String user) throws Exception {
}

private static Map<String, Object> runSql(@Nullable String asUser, String mode, String sql) throws IOException {
return runSql(asUser, mode, new StringEntity("{\"query\": \"" + sql + "\"}", ContentType.APPLICATION_JSON));
return runSql(asUser, new StringEntity("{\"query\": \"" + sql + "\"" + mode(mode) + "}", ContentType.APPLICATION_JSON));
}

private static Map<String, Object> runSql(@Nullable String asUser, String mode, HttpEntity entity) throws IOException {
private static Map<String, Object> runSql(@Nullable String asUser, HttpEntity entity) throws IOException {
Request request = new Request("POST", "/_sql");
if (false == mode.isEmpty()) {
request.addParameter("mode", mode);
}
if (asUser != null) {
RequestOptions.Builder options = request.getOptions().toBuilder();
options.addHeader("es-security-runas-user", asUser);
Expand Down Expand Up @@ -223,14 +223,15 @@ protected AuditLogAsserter createAuditLogAsserter() {
public void testHijackScrollFails() throws Exception {
createUser("full_access", "rest_minimal");

Map<String, Object> adminResponse = RestActions.runSql(null, randomMode(),
new StringEntity("{\"query\": \"SELECT * FROM test\", \"fetch_size\": 1}", ContentType.APPLICATION_JSON));
Map<String, Object> adminResponse = RestActions.runSql(null,
new StringEntity("{\"query\": \"SELECT * FROM test\", \"fetch_size\": 1" + mode(randomMode()) + "}",
ContentType.APPLICATION_JSON));

String cursor = (String) adminResponse.remove("cursor");
assertNotNull(cursor);

ResponseException e = expectThrows(ResponseException.class, () -> RestActions.runSql("full_access", randomMode(),
new StringEntity("{\"cursor\":\"" + cursor + "\"}", ContentType.APPLICATION_JSON)));
ResponseException e = expectThrows(ResponseException.class, () -> RestActions.runSql("full_access",
new StringEntity("{\"cursor\":\"" + cursor + "\"" + mode(randomMode()) + "}", ContentType.APPLICATION_JSON)));
// TODO return a better error message for bad scrolls
assertThat(e.getMessage(), containsString("No search context found for id"));
assertEquals(404, e.getResponse().getStatusLine().getStatusCode());
Expand Down
13 changes: 4 additions & 9 deletions ...sql/qa/security/src/test/java/org/elasticsearch/xpack/sql/qa/security/UserFunctionIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
import java.util.Map;

import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.columnInfo;
import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.mode;
import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.randomMode;

public class UserFunctionIT extends ESRestTestCase {

Expand Down Expand Up @@ -172,14 +174,11 @@ private void deleteUser(String name) throws IOException {
}

private Map<String, Object> runSql(String asUser, String mode, String sql) throws IOException {
return runSql(asUser, mode, new StringEntity("{\"query\": \"" + sql + "\"}", ContentType.APPLICATION_JSON));
return runSql(asUser, new StringEntity("{\"query\": \"" + sql + "\"" + mode(mode) + "}", ContentType.APPLICATION_JSON));
}

private Map<String, Object> runSql(String asUser, String mode, HttpEntity entity) throws IOException {
private Map<String, Object> runSql(String asUser, HttpEntity entity) throws IOException {
Request request = new Request("POST", "/_sql");
if (false == mode.isEmpty()) {
request.addParameter("mode", mode);
}
if (asUser != null) {
RequestOptions.Builder options = request.getOptions().toBuilder();
options.addHeader("es-security-runas-user", asUser);
Expand All @@ -203,10 +202,6 @@ private static Map<String, Object> toMap(Response response) throws IOException {
}
}

private String randomMode() {
return randomFrom("plain", "jdbc", "");
}

private void index(String... docs) throws IOException {
Request request = new Request("POST", "/test/test/_bulk");
request.addParameter("refresh", "true");
Expand Down
49 changes: 23 additions & 26 deletions x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,10 @@ public void testNextPage() throws IOException {
for (int i = 0; i < 20; i += 2) {
Map<String, Object> response;
if (i == 0) {
response = runSql(mode, new StringEntity(sqlRequest, ContentType.APPLICATION_JSON));
response = runSql(new StringEntity(sqlRequest, ContentType.APPLICATION_JSON), "");
} else {
response = runSql(mode, new StringEntity("{\"cursor\":\"" + cursor + "\"}",
ContentType.APPLICATION_JSON));
response = runSql(new StringEntity("{\"cursor\":\"" + cursor + "\"" + mode(mode) + "}",
ContentType.APPLICATION_JSON), "");
Copy link
Member

@costin costin Dec 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"" could be StringUtils/Strings.EMPTY

Copy link
Contributor Author

@astefan astefan Dec 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added an EMPTY constant to org.elasticsearch.xpack.sql.proto.StringUtils. It seems there is no other class visible to qa that has this constant defined.

}

Map<String, Object> expected = new HashMap<>();
Expand All @@ -120,8 +120,8 @@ public void testNextPage() throws IOException {
}
Map<String, Object> expected = new HashMap<>();
expected.put("rows", emptyList());
assertResponse(expected, runSql(mode, new StringEntity("{ \"cursor\":\"" + cursor + "\"}",
ContentType.APPLICATION_JSON)));
assertResponse(expected, runSql(new StringEntity("{ \"cursor\":\"" + cursor + "\"" + mode(mode) + "}",
ContentType.APPLICATION_JSON), ""));
}

@AwaitsFix(bugUrl = "Unclear status, https://github.com/elastic/x-pack-elasticsearch/issues/2074")
Expand All @@ -136,8 +136,7 @@ public void testTimeZone() throws IOException {
expected.put("size", 2);

// Default TimeZone is UTC
assertResponse(expected, runSql(mode, new StringEntity("{\"query\":\"SELECT DAY_OF_YEAR(test), COUNT(*) FROM test\"}",
ContentType.APPLICATION_JSON)));
assertResponse(expected, runSql(mode, "SELECT DAY_OF_YEAR(test), COUNT(*) FROM test"));
}

public void testScoreWithFieldNamedScore() throws IOException {
Expand Down Expand Up @@ -306,31 +305,25 @@ private Map<String, Object> runSql(String mode, String sql) throws IOException {
}

private Map<String, Object> runSql(String mode, String sql, String suffix) throws IOException {
return runSql(mode, new StringEntity("{\"query\":\"" + sql + "\"}", ContentType.APPLICATION_JSON), suffix);
return runSql(new StringEntity("{\"query\":\"" + sql + "\"" + mode(mode) + "}", ContentType.APPLICATION_JSON), suffix);
}

private Map<String, Object> runSql(String mode, HttpEntity sql) throws IOException {
return runSql(mode, sql, "");
}

private Map<String, Object> runSql(String mode, HttpEntity sql, String suffix) throws IOException {
private Map<String, Object> runSql(HttpEntity sql, String suffix) throws IOException {
Request request = new Request("POST", "/_sql" + suffix);
request.addParameter("error_trace", "true"); // Helps with debugging in case something crazy happens on the server.
request.addParameter("pretty", "true"); // Improves error reporting readability
if (randomBoolean()) {
// We default to JSON but we force it randomly for extra coverage
request.addParameter("format", "json");
}
if (false == mode.isEmpty()) {
request.addParameter("mode", mode); // JDBC or PLAIN mode
}
if (randomBoolean()) {
// JSON is the default but randomly set it sometime for extra coverage
RequestOptions.Builder options = request.getOptions().toBuilder();
options.addHeader("Accept", randomFrom("*/*", "application/json"));
request.setOptions(options);
}
request.setEntity(sql);

Response response = client().performRequest(request);
try (InputStream content = response.getEntity().getContent()) {
return XContentHelper.convertToMap(JsonXContent.jsonXContent, content, false);
Expand All @@ -357,9 +350,9 @@ public void testBasicQueryWithFilter() throws IOException {
Map<String, Object> expected = new HashMap<>();
expected.put("columns", singletonList(columnInfo(mode, "test", "text", JDBCType.VARCHAR, 0)));
expected.put("rows", singletonList(singletonList("foo")));
assertResponse(expected, runSql(mode, new StringEntity("{\"query\":\"SELECT * FROM test\", " +
"\"filter\":{\"match\": {\"test\": \"foo\"}}}",
ContentType.APPLICATION_JSON)));
assertResponse(expected, runSql(new StringEntity("{\"query\":\"SELECT * FROM test\", " +
"\"filter\":{\"match\": {\"test\": \"foo\"}}" + mode(mode) + "}",
ContentType.APPLICATION_JSON), ""));
}

public void testBasicQueryWithParameters() throws IOException {
Expand All @@ -373,16 +366,16 @@ public void testBasicQueryWithParameters() throws IOException {
columnInfo(mode, "param", "integer", JDBCType.INTEGER, 11)
));
expected.put("rows", singletonList(Arrays.asList("foo", 10)));
assertResponse(expected, runSql(mode, new StringEntity("{\"query\":\"SELECT test, ? param FROM test WHERE test = ?\", " +
"\"params\":[{\"type\": \"integer\", \"value\": 10}, {\"type\": \"keyword\", \"value\": \"foo\"}]}",
ContentType.APPLICATION_JSON)));
assertResponse(expected, runSql(new StringEntity("{\"query\":\"SELECT test, ? param FROM test WHERE test = ?\", " +
"\"params\":[{\"type\": \"integer\", \"value\": 10}, {\"type\": \"keyword\", \"value\": \"foo\"}]"
+ mode(mode) + "}", ContentType.APPLICATION_JSON), ""));
}

public void testBasicTranslateQueryWithFilter() throws IOException {
index("{\"test\":\"foo\"}",
"{\"test\":\"bar\"}");

Map<String, Object> response = runSql("",
Map<String, Object> response = runSql(
new StringEntity("{\"query\":\"SELECT * FROM test\", \"filter\":{\"match\": {\"test\": \"foo\"}}}",
ContentType.APPLICATION_JSON), "/translate/"
);
Expand Down Expand Up @@ -424,7 +417,7 @@ public void testTranslateQueryWithGroupByAndHaving() throws IOException {
index("{\"salary\":100}",
"{\"age\":20}");

Map<String, Object> response = runSql("",
Map<String, Object> response = runSql(
new StringEntity("{\"query\":\"SELECT avg(salary) FROM test GROUP BY abs(age) HAVING avg(salary) > 50 LIMIT 10\"}",
ContentType.APPLICATION_JSON), "/translate/"
);
Expand Down Expand Up @@ -570,9 +563,9 @@ public void testNextPageText() throws IOException {
}
Map<String, Object> expected = new HashMap<>();
expected.put("rows", emptyList());
assertResponse(expected, runSql("", new StringEntity("{\"cursor\":\"" + cursor + "\"}", ContentType.APPLICATION_JSON)));
assertResponse(expected, runSql(new StringEntity("{\"cursor\":\"" + cursor + "\"}", ContentType.APPLICATION_JSON), ""));

Map<String, Object> response = runSql("", new StringEntity("{\"cursor\":\"" + cursor + "\"}", ContentType.APPLICATION_JSON),
Map<String, Object> response = runSql(new StringEntity("{\"cursor\":\"" + cursor + "\"}", ContentType.APPLICATION_JSON),
"/close");
assertEquals(true, response.get("succeeded"));

Expand Down Expand Up @@ -718,6 +711,10 @@ private static Map<String, Object> searchStats() throws IOException {
public static String randomMode() {
return randomFrom("", "jdbc", "plain");
}

public static String mode(String mode) {
return mode.isEmpty() ? "" : ",\"mode\":\"" + mode + "\"";
Copy link
Member

@costin costin Dec 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strings.isEmpty is a better choice as it checks if it's also null.

}

private void index(String... docs) throws IOException {
Request request = new Request("POST", "/test/test/_bulk");
Expand Down
13 changes: 5 additions & 8 deletions ...plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/rest/RestSqlUsageTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import java.util.Locale;
import java.util.Map;

import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.mode;

public abstract class RestSqlUsageTestCase extends ESRestTestCase {
private List<IndexDocument> testData = Arrays.asList(
new IndexDocument("used", "Don Quixote", 1072),
Expand Down Expand Up @@ -274,20 +276,15 @@ private void runSql(String mode, String restClient, String sql) throws IOExcepti
// We default to JSON but we force it randomly for extra coverage
request.addParameter("format", "json");
}
if (false == mode.isEmpty()) {
request.addParameter("mode", mode); // JDBC or PLAIN mode
}
// randomly use the "client.id" parameter or not
if (false == ignoreClientType) {
request.addParameter("client.id", restClient);
}
if (randomBoolean()) {
// JSON is the default but randomly set it sometime for extra coverage
RequestOptions.Builder options = request.getOptions().toBuilder();
options.addHeader("Accept", randomFrom("*/*", "application/json"));
request.setOptions(options);
}
request.setEntity(new StringEntity("{\"query\":\"" + sql + "\"}", ContentType.APPLICATION_JSON));
request.setEntity(new StringEntity("{\"query\":\"" + sql + "\"" + mode(mode) +
(ignoreClientType ? "" : ",\"client.id\":\"" + restClient + "\"") + "}",
ContentType.APPLICATION_JSON));
client().performRequest(request);
}

Expand Down
Loading