Skip to content
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

Jump to bottom

REST high-level client: encode path parts #28663

Merged
merged 5 commits into from
Feb 15, 2018
Merged

REST high-level client: encode path parts #28663

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3faa032
REST high-level client: encode path parts
javanna Feb 13, 2018
46d05d1
add tests for params encoding
javanna Feb 13, 2018
39b2490
fix ccs test failure
javanna Feb 14, 2018
04dbbea
fix test failures
javanna Feb 14, 2018
e1b3efc
Merge branch 'master' into fix/rest-hl-client-encode-parts
javanna Feb 15, 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
11 changes: 10 additions & 1 deletion client/rest-high-level/src/main/java/org/elasticsearch/client/Request.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.apache.http.client.methods.HttpHead;
import org.apache.http.client.methods.HttpPost;
import org.apache.http.client.methods.HttpPut;
import org.apache.http.client.utils.URIBuilder;
import org.apache.http.entity.ByteArrayEntity;
import org.apache.http.entity.ContentType;
import org.apache.lucene.util.BytesRef;
Expand Down Expand Up @@ -73,6 +74,7 @@

import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.net.URISyntaxException;
import java.nio.charset.Charset;
import java.util.Collections;
import java.util.HashMap;
Expand Down Expand Up @@ -568,7 +570,14 @@ static String buildEndpoint(String... parts) {
StringJoiner joiner = new StringJoiner("/", "/", "");
for (String part : parts) {
if (Strings.hasLength(part)) {
joiner.add(part);
try {
//encode each part (e.g. index, type and id) separately before merging them into the path
URIBuilder uriBuilder = new URIBuilder().setPath(part);
//make sure that "/" in each part are properly encoded too
joiner.add(uriBuilder.build().getRawPath().replaceAll("/", "%2F"));
Copy link
Member

Choose a reason for hiding this comment

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

Could we use a static encode() method for this? We'll have to reuse it in some URL parameters like routing or parent, right?

Copy link
Member Author

@javanna javanna Feb 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

it's complicated :) querystring params should already be encoded by the rest low-level client. but the low-level client expects paths to be externally encoded, as they are provided altogether and we can't encode each part safely anymore. In the high-level client though, it makes sense to encode each part separately as they are provided separately. I think what we do here is all we need.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add tests for this.

Copy link
Member

Choose a reason for hiding this comment

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

Of course, I just forgot that the low-level client already encodes parameters. Thanks.

} catch (URISyntaxException e) {
throw new IllegalArgumentException("Path part [" + part + "] couldn't be encoded", e);
}
}
}
return joiner.toString();
Expand Down
47 changes: 46 additions & 1 deletion client/rest-high-level/src/test/java/org/elasticsearch/client/CrudIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.elasticsearch.ElasticsearchStatusException;
import org.elasticsearch.action.DocWriteRequest;
import org.elasticsearch.action.DocWriteResponse;
import org.elasticsearch.action.admin.indices.get.GetIndexRequest;
import org.elasticsearch.action.bulk.BulkItemResponse;
import org.elasticsearch.action.bulk.BulkProcessor;
import org.elasticsearch.action.bulk.BulkRequest;
Expand All @@ -52,6 +53,9 @@
import org.elasticsearch.script.Script;
import org.elasticsearch.script.ScriptType;
import org.elasticsearch.search.fetch.subphase.FetchSourceContext;
import org.joda.time.DateTime;
import org.joda.time.DateTimeZone;
import org.joda.time.format.DateTimeFormat;

import java.io.IOException;
import java.util.Collections;
Expand Down Expand Up @@ -648,7 +652,7 @@ public void testBulk() throws IOException {
validateBulkResponses(nbItems, errors, bulkResponse, bulkRequest);
}

public void testBulkProcessorIntegration() throws IOException, InterruptedException {
public void testBulkProcessorIntegration() throws IOException {
int nbItems = randomIntBetween(10, 100);
boolean[] errors = new boolean[nbItems];

Expand Down Expand Up @@ -762,4 +766,45 @@ private void validateBulkResponses(int nbItems, boolean[] errors, BulkResponse b
}
}
}

public void testUrlEncode() throws IOException {
String indexPattern = "<logstash-{now/M}>";
String expectedIndex = "logstash-" +
DateTimeFormat.forPattern("YYYY.MM.dd").print(new DateTime(DateTimeZone.UTC).monthOfYear().roundFloorCopy());
{
IndexRequest indexRequest = new IndexRequest(indexPattern, "type", "id#1");
indexRequest.source("field", "value");
IndexResponse indexResponse = highLevelClient().index(indexRequest);
assertEquals(expectedIndex, indexResponse.getIndex());
assertEquals("type", indexResponse.getType());
assertEquals("id#1", indexResponse.getId());
}
{
GetRequest getRequest = new GetRequest(indexPattern, "type", "id#1");
GetResponse getResponse = highLevelClient().get(getRequest);
assertTrue(getResponse.isExists());
assertEquals(expectedIndex, getResponse.getIndex());
assertEquals("type", getResponse.getType());
assertEquals("id#1", getResponse.getId());
}

{
IndexRequest indexRequest = new IndexRequest("index", "type", "this/is/the/id");
indexRequest.source("field", "value");
IndexResponse indexResponse = highLevelClient().index(indexRequest);
assertEquals("index", indexResponse.getIndex());
assertEquals("type", indexResponse.getType());
assertEquals("this/is/the/id", indexResponse.getId());
}
{
GetRequest getRequest = new GetRequest("index", "type", "this/is/the/id");
GetResponse getResponse = highLevelClient().get(getRequest);
assertTrue(getResponse.isExists());
assertEquals("index", getResponse.getIndex());
assertEquals("type", getResponse.getType());
assertEquals("this/is/the/id", getResponse.getId());
}

assertTrue(highLevelClient().indices().exists(new GetIndexRequest().indices(indexPattern, "index")));
}
}
14 changes: 14 additions & 0 deletions client/rest-high-level/src/test/java/org/elasticsearch/client/RequestTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1178,6 +1178,20 @@ public void testBuildEndpoint() {
assertEquals("/a/_create", Request.buildEndpoint("a", null, null, "_create"));
}

public void testBuildEndPointEncodeParts() {
assertEquals("/-%23index1,index%232/type/id", Request.buildEndpoint("-#index1,index#2", "type", "id"));
assertEquals("/index/type%232/id", Request.buildEndpoint("index", "type#2", "id"));
assertEquals("/index/type/this%2Fis%2Fthe%2Fid", Request.buildEndpoint("index", "type", "this/is/the/id"));
assertEquals("/index/type/this%7Cis%7Cthe%7Cid", Request.buildEndpoint("index", "type", "this|is|the|id"));
assertEquals("/index/type/id%231", Request.buildEndpoint("index", "type", "id#1"));
assertEquals("/%3Clogstash-%7Bnow%2FM%7D%3E/_search", Request.buildEndpoint("<logstash-{now/M}>", "_search"));
assertEquals("/%E4%B8%AD%E6%96%87", Request.buildEndpoint("中文"));
assertEquals("/foo%20bar", Request.buildEndpoint("foo bar"));
assertEquals("/foo%2Fbar", Request.buildEndpoint("foo/bar"));
assertEquals("/foo%5Ebar", Request.buildEndpoint("foo^bar"));
assertEquals("/*", Request.buildEndpoint("*"));
}

public void testEndpoint() {
assertEquals("/index/type/id", Request.endpoint("index", "type", "id"));
assertEquals("/index/type/id/_endpoint", Request.endpoint("index", "type", "id", "_endpoint"));
Expand Down
7 changes: 3 additions & 4 deletions test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
package org.elasticsearch.test.rest.yaml;

import com.carrotsearch.randomizedtesting.RandomizedTest;

import org.apache.http.Header;
import org.apache.http.HttpEntity;
import org.apache.http.HttpHost;
Expand Down Expand Up @@ -85,9 +84,9 @@ public ClientYamlTestResponse callApi(String apiName, Map<String, String> params
Map<String, String> pathParts = new HashMap<>();
Map<String, String> queryStringParams = new HashMap<>();

Set<String> apiRequiredPathParts = restApi.getPathParts().entrySet().stream().filter(e -> e.getValue() == true).map(Entry::getKey)
Set<String> apiRequiredPathParts = restApi.getPathParts().entrySet().stream().filter(Entry::getValue).map(Entry::getKey)
.collect(Collectors.toSet());
Set<String> apiRequiredParameters = restApi.getParams().entrySet().stream().filter(e -> e.getValue() == true).map(Entry::getKey)
Set<String> apiRequiredParameters = restApi.getParams().entrySet().stream().filter(Entry::getValue).map(Entry::getKey)
.collect(Collectors.toSet());

for (Map.Entry<String, String> entry : params.entrySet()) {
Expand Down Expand Up @@ -151,7 +150,7 @@ public ClientYamlTestResponse callApi(String apiName, Map<String, String> params
for (String pathPart : restPath.getPathParts()) {
try {
finalPath.append('/');
// We append "/" to the path part to handle parts that start with - or other invalid characters
// We prepend "/" to the path part to handle parts that start with - or other invalid characters
URI uri = new URI(null, null, null, -1, "/" + pathPart, null, null);
//manually escape any slash that each part may contain
finalPath.append(uri.getRawPath().substring(1).replaceAll("/", "%2F"));
Expand Down