Skip to content

Commit

Permalink
Restore support for escaped '/' as part of document id
Browse files Browse the repository at this point in the history
With elastic#13691 we introduced some custom logic to make sure that date math expressions like <logstash-{now/D}> don't get broken up into two where the slash appears in the expression. That said the only correct way to provide such a date math expression as part of the uri would be to properly escape the '/' instead. This fix also introduced a regression, as it would make sure that unescaped '/' are handled only in the case of date math expressions, but it removed support for properly escaped slashes anywhere else. The solution is to keep supporting escaped slashes only and require client libraries to properly escape them.

This commit reverts 93ad696 and makes sure that our REST tests runner supports escaping of path parts, which was more involving than expected as each single part of the path needs to be properly escaped. I am not too happy with the current solution but it's the best I could do for now, maybe not that concerning anyway given that it's just test code. I do find uri encoding quite frustrating in java.

Relates to elastic#13691
Relates to elastic#13665

Closes elastic#14177
Closes elastic#14216
  • Loading branch information
javanna committed Oct 21, 2015
1 parent d9ac96e commit b4d52fe
Show file tree
Hide file tree
Showing 7 changed files with 176 additions and 157 deletions.
65 changes: 3 additions & 62 deletions core/src/main/java/org/elasticsearch/common/path/PathTrie.java
Expand Up @@ -22,8 +22,6 @@
import com.google.common.collect.ImmutableMap;
import org.elasticsearch.common.Strings;

import java.util.ArrayList;
import java.util.List;
import java.util.Map;

import static org.elasticsearch.common.collect.MapBuilder.newMapBuilder;
Expand All @@ -33,26 +31,15 @@
*/
public class PathTrie<T> {

public static interface Decoder {
public interface Decoder {
String decode(String value);
}

public static final Decoder NO_DECODER = new Decoder() {
@Override
public String decode(String value) {
return value;
}
};

private final Decoder decoder;
private final TrieNode<T> root;
private final char separator;
private T rootValue;

public PathTrie() {
this('/', "*", NO_DECODER);
}

public PathTrie(Decoder decoder) {
this('/', "*", decoder);
}
Expand Down Expand Up @@ -197,7 +184,7 @@ public T retrieve(String[] path, int index, Map<String, String> params) {

private void put(Map<String, String> params, TrieNode<T> node, String value) {
if (params != null && node.isNamedWildcard()) {
params.put(node.namedWildcard(), value);
params.put(node.namedWildcard(), decoder.decode(value));
}
}
}
Expand All @@ -224,7 +211,7 @@ public T retrieve(String path, Map<String, String> params) {
if (path.length() == 0) {
return rootValue;
}
String[] strings = splitPath(decoder.decode(path));
String[] strings = Strings.splitStringToArray(path, separator);
if (strings.length == 0) {
return rootValue;
}
Expand All @@ -235,50 +222,4 @@ public T retrieve(String path, Map<String, String> params) {
}
return root.retrieve(strings, index, params);
}

/*
Splits up the url path up by '/' and is aware of
index name expressions that appear between '<' and '>'.
*/
String[] splitPath(final String path) {
if (path == null || path.length() == 0) {
return Strings.EMPTY_ARRAY;
}
int count = 1;
boolean splitAllowed = true;
for (int i = 0; i < path.length(); i++) {
final char currentC = path.charAt(i);
if ('<' == currentC) {
splitAllowed = false;
} else if (currentC == '>') {
splitAllowed = true;
} else if (splitAllowed && currentC == separator) {
count++;
}
}

final List<String> result = new ArrayList<>(count);
final StringBuilder builder = new StringBuilder();

splitAllowed = true;
for (int i = 0; i < path.length(); i++) {
final char currentC = path.charAt(i);
if ('<' == currentC) {
splitAllowed = false;
} else if (currentC == '>') {
splitAllowed = true;
} else if (splitAllowed && currentC == separator) {
if (builder.length() > 0) {
result.add(builder.toString());
builder.setLength(0);
}
continue;
}
builder.append(currentC);
}
if (builder.length() > 0) {
result.add(builder.toString());
}
return result.toArray(new String[result.size()]);
}
}
57 changes: 30 additions & 27 deletions core/src/test/java/org/elasticsearch/common/path/PathTrieTests.java
Expand Up @@ -19,13 +19,13 @@

package org.elasticsearch.common.path;

import org.elasticsearch.rest.support.RestUtils;
import org.elasticsearch.test.ESTestCase;
import org.junit.Test;

import java.util.HashMap;
import java.util.Map;

import static org.hamcrest.Matchers.arrayContaining;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.nullValue;

Expand All @@ -34,8 +34,15 @@
*/
public class PathTrieTests extends ESTestCase {

public static final PathTrie.Decoder NO_DECODER = new PathTrie.Decoder() {
@Override
public String decode(String value) {
return value;
}
};

public void testPath() {
PathTrie<String> trie = new PathTrie<>();
PathTrie<String> trie = new PathTrie<>(NO_DECODER);
trie.insert("/a/b/c", "walla");
trie.insert("a/d/g", "kuku");
trie.insert("x/b/c", "lala");
Expand All @@ -62,13 +69,13 @@ public void testPath() {
}

public void testEmptyPath() {
PathTrie<String> trie = new PathTrie<>();
PathTrie<String> trie = new PathTrie<>(NO_DECODER);
trie.insert("/", "walla");
assertThat(trie.retrieve(""), equalTo("walla"));
}

public void testDifferentNamesOnDifferentPath() {
PathTrie<String> trie = new PathTrie<>();
PathTrie<String> trie = new PathTrie<>(NO_DECODER);
trie.insert("/a/{type}", "test1");
trie.insert("/b/{name}", "test2");

Expand All @@ -82,7 +89,7 @@ public void testDifferentNamesOnDifferentPath() {
}

public void testSameNameOnDifferentPath() {
PathTrie<String> trie = new PathTrie<>();
PathTrie<String> trie = new PathTrie<>(NO_DECODER);
trie.insert("/a/c/{name}", "test1");
trie.insert("/b/{name}", "test2");

Expand All @@ -96,7 +103,7 @@ public void testSameNameOnDifferentPath() {
}

public void testPreferNonWildcardExecution() {
PathTrie<String> trie = new PathTrie<>();
PathTrie<String> trie = new PathTrie<>(NO_DECODER);
trie.insert("{test}", "test1");
trie.insert("b", "test2");
trie.insert("{test}/a", "test3");
Expand All @@ -112,7 +119,7 @@ public void testPreferNonWildcardExecution() {
}

public void testSamePathConcreteResolution() {
PathTrie<String> trie = new PathTrie<>();
PathTrie<String> trie = new PathTrie<>(NO_DECODER);
trie.insert("{x}/{y}/{z}", "test1");
trie.insert("{x}/_y/{k}", "test2");

Expand All @@ -128,7 +135,7 @@ public void testSamePathConcreteResolution() {
}

public void testNamedWildcardAndLookupWithWildcard() {
PathTrie<String> trie = new PathTrie<>();
PathTrie<String> trie = new PathTrie<>(NO_DECODER);
trie.insert("x/{test}", "test1");
trie.insert("{test}/a", "test2");
trie.insert("/{test}", "test3");
Expand Down Expand Up @@ -156,24 +163,20 @@ public void testNamedWildcardAndLookupWithWildcard() {
assertThat(params.get("test"), equalTo("*"));
}

public void testSplitPath() {
PathTrie<String> trie = new PathTrie<>();
assertThat(trie.splitPath("/a/"), arrayContaining("a"));
assertThat(trie.splitPath("/a/b"),arrayContaining("a", "b"));
assertThat(trie.splitPath("/a/b/c"), arrayContaining("a", "b", "c"));
assertThat(trie.splitPath("/a/b/<c/d>"), arrayContaining("a", "b", "<c/d>"));
assertThat(trie.splitPath("/a/b/<c/d>/d"), arrayContaining("a", "b", "<c/d>", "d"));

assertThat(trie.splitPath("/<logstash-{now}>/_search"), arrayContaining("<logstash-{now}>", "_search"));
assertThat(trie.splitPath("/<logstash-{now/d}>/_search"), arrayContaining("<logstash-{now/d}>", "_search"));
assertThat(trie.splitPath("/<logstash-{now/M{YYYY.MM}}>/_search"), arrayContaining("<logstash-{now/M{YYYY.MM}}>", "_search"));
assertThat(trie.splitPath("/<logstash-{now/M{YYYY.MM}}>/_search"), arrayContaining("<logstash-{now/M{YYYY.MM}}>", "_search"));
assertThat(trie.splitPath("/<logstash-{now/M{YYYY.MM|UTC}}>/log/_search"), arrayContaining("<logstash-{now/M{YYYY.MM|UTC}}>", "log", "_search"));

assertThat(trie.splitPath("/<logstash-{now/M}>,<logstash-{now/M-1M}>/_search"), arrayContaining("<logstash-{now/M}>,<logstash-{now/M-1M}>", "_search"));
assertThat(trie.splitPath("/<logstash-{now/M}>,<logstash-{now/M-1M}>/_search"), arrayContaining("<logstash-{now/M}>,<logstash-{now/M-1M}>", "_search"));
assertThat(trie.splitPath("/<logstash-{now/M{YYYY.MM}}>,<logstash-{now/M-1M{YYYY.MM}}>/_search"), arrayContaining("<logstash-{now/M{YYYY.MM}}>,<logstash-{now/M-1M{YYYY.MM}}>", "_search"));
assertThat(trie.splitPath("/<logstash-{now/M{YYYY.MM|UTC}}>,<logstash-{now/M-1M{YYYY.MM|UTC}}>/_search"), arrayContaining("<logstash-{now/M{YYYY.MM|UTC}}>,<logstash-{now/M-1M{YYYY.MM|UTC}}>", "_search"));
//https://github.com/elastic/elasticsearch/issues/14177
//https://github.com/elastic/elasticsearch/issues/13665
public void testEscapedSlashWithinUrl() {
PathTrie<String> pathTrie = new PathTrie<>(RestUtils.REST_DECODER);
pathTrie.insert("/{index}/{type}/{id}", "test");
HashMap<String, String> params = new HashMap<>();
assertThat(pathTrie.retrieve("/index/type/a%2Fe", params), equalTo("test"));
assertThat(params.get("index"), equalTo("index"));
assertThat(params.get("type"), equalTo("type"));
assertThat(params.get("id"), equalTo("a/e"));
params.clear();
assertThat(pathTrie.retrieve("/<logstash-{now%2Fd}>/type/id", params), equalTo("test"));
assertThat(params.get("index"), equalTo("<logstash-{now/d}>"));
assertThat(params.get("type"), equalTo("type"));
assertThat(params.get("id"), equalTo("id"));
}

}
Expand Up @@ -100,7 +100,7 @@ public void testThatPathsAreNormalized() throws Exception {
notFoundUris.add("/_plugin/dummy/%2e%2e/%2e%2e/%2e%2e/%2e%2e/index.html");
notFoundUris.add("/_plugin/dummy/%2e%2e%2f%2e%2e%2f%2e%2e%2f%2e%2e%2findex.html");
notFoundUris.add("/_plugin/dummy/%2E%2E/%2E%2E/%2E%2E/%2E%2E/index.html");
notFoundUris.add("/_plugin/dummy/..\\..\\..\\..\\..\\log4j.properties");
notFoundUris.add("/_plugin/dummy/..%5C..%5C..%5C..%5C..%5Clog4j.properties");

for (String uri : notFoundUris) {
HttpResponse response = httpClient().path(uri).execute();
Expand Down
Expand Up @@ -232,8 +232,9 @@ private HttpRequestBuilder callApiBuilder(String apiName, Map<String, String> pa
httpRequestBuilder.method(RandomizedTest.randomFrom(supportedMethods));
}

//the http method is randomized (out of the available ones with the chosen api)
return httpRequestBuilder.path(RandomizedTest.randomFrom(restApi.getFinalPaths(pathParts)));
//the rest path to use is randomized out of the matching ones (if more than one)
RestPath restPath = RandomizedTest.randomFrom(restApi.getFinalPaths(pathParts));
return httpRequestBuilder.pathParts(restPath.getPathParts());
}

private RestApi restApi(String apiName) {
Expand Down
@@ -0,0 +1,97 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.test.rest.client;

import java.util.*;

public class RestPath {
private final List<PathPart> parts;
private final List<String> placeholders;

public RestPath(List<String> parts) {
List<PathPart> pathParts = new ArrayList<>(parts.size());
for (String part : parts) {
pathParts.add(new PathPart(part, false));
}
this.parts = pathParts;
this.placeholders = Collections.emptyList();
}

public RestPath(String path) {
String[] pathParts = path.split("/");
List<String> placeholders = new ArrayList<>();
List<PathPart> parts = new ArrayList<>();
for (String pathPart : pathParts) {
if (pathPart.length() > 0) {
if (pathPart.startsWith("{")) {
if (pathPart.indexOf('}') != pathPart.length() - 1) {
throw new IllegalArgumentException("more than one parameter found in the same path part: [" + pathPart + "]");
}
String placeholder = pathPart.substring(1, pathPart.length() - 1);
parts.add(new PathPart(placeholder, true));
placeholders.add(placeholder);
} else {
parts.add(new PathPart(pathPart, false));
}
}
}
this.placeholders = placeholders;
this.parts = parts;
}

public String[] getPathParts() {
String[] parts = new String[this.parts.size()];
int i = 0;
for (PathPart part : this.parts) {
parts[i++] = part.pathPart;
}
return parts;
}

public boolean matches(Set<String> params) {
return placeholders.size() == params.size() && placeholders.containsAll(params);
}

public RestPath replacePlaceholders(Map<String,String> params) {
List<String> finalPathParts = new ArrayList<>(parts.size());
for (PathPart pathPart : parts) {
if (pathPart.isPlaceholder) {
String value = params.get(pathPart.pathPart);
if (value == null) {
throw new IllegalArgumentException("parameter [" + pathPart.pathPart + "] missing");
}
finalPathParts.add(value);
} else {
finalPathParts.add(pathPart.pathPart);
}
}
return new RestPath(finalPathParts);
}

private static class PathPart {
private final boolean isPlaceholder;
private final String pathPart;

private PathPart(String pathPart, boolean isPlaceholder) {
this.isPlaceholder = isPlaceholder;
this.pathPart = pathPart;
}
}
}

0 comments on commit b4d52fe

Please sign in to comment.