diff --git a/README.md b/README.md index 87d5caf85..30188dc5c 100644 --- a/README.md +++ b/README.md @@ -113,6 +113,14 @@ See [Advanced Usage](#advanced-usage) for more examples. > > `@RequestLine` and `@QueryMap` templates do not encode slash `/` characters by default. To change this behavior, set the `decodeSlash` property on the `@RequestLine` to `false`. +> **What about plus? `+`** +> +> Per the URI specification, a `+` sign is allowed in both the path and query segments of a URI, however, handling of +> the symbol on the query can be inconsistent. In some legacy systems, the `+` is equivalent to the a space. Feign takes the approach of modern systems, where a +> `+` symbol should not represent a space and is explicitly encoded as `%2B` when found on a query string. +> +> If you wish to use `+` as a space, then use the literal ` ` character or encode the value directly as `%20` + ##### Custom Expansion The `@Param` annotation has an optional property `expander` allowing for complete control over the individual parameter's expansion. diff --git a/core/src/main/java/feign/template/Expressions.java b/core/src/main/java/feign/template/Expressions.java index d3088ceea..558766a6d 100644 --- a/core/src/main/java/feign/template/Expressions.java +++ b/core/src/main/java/feign/template/Expressions.java @@ -1,5 +1,5 @@ /** - * Copyright 2012-2018 The Feign Authors + * Copyright 2012-2019 The Feign Authors * * Licensed 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 @@ -11,10 +11,10 @@ * or implied. See the License for the specific language governing permissions and limitations under * the License. */ - package feign.template; import feign.Util; +import feign.template.UriUtils.FragmentType; import java.util.ArrayList; import java.util.LinkedHashMap; import java.util.List; @@ -29,11 +29,10 @@ public final class Expressions { static { expressions = new LinkedHashMap<>(); - expressions.put(Pattern.compile("\\+(\\w[-\\w.]*[ ]*)(:(.+))?"), ReservedExpression.class); expressions.put(Pattern.compile("(\\w[-\\w.]*[ ]*)(:(.+))?"), SimpleExpression.class); } - public static Expression create(final String value) { + public static Expression create(final String value, final FragmentType type) { /* remove the start and end braces */ final String expression = stripBraces(value); @@ -54,7 +53,6 @@ public static Expression create(final String value) { Entry> matchedExpression = matchedExpressionEntry.get(); Pattern expressionPattern = matchedExpression.getKey(); - Class expressionType = matchedExpression.getValue(); /* create a new regular expression matcher for the expression */ String variableName = null; @@ -69,7 +67,7 @@ public static Expression create(final String value) { } } - return new SimpleExpression(variableName, variablePattern); + return new SimpleExpression(variableName, variablePattern, type); } private static String stripBraces(String expression) { @@ -82,36 +80,21 @@ private static String stripBraces(String expression) { return expression; } - /** - * Expression that does not encode reserved characters. This expression adheres to RFC 6570 - * Reserved Expansion (Level 2) - * specification. - */ - public static class ReservedExpression extends SimpleExpression { - private final String RESERVED_CHARACTERS = ":/?#[]@!$&\'()*+,;="; - - ReservedExpression(String expression, String pattern) { - super(expression, pattern); - } - - @Override - String encode(Object value) { - return UriUtils.encodeReserved(value.toString(), RESERVED_CHARACTERS, Util.UTF_8); - } - } - /** * Expression that adheres to Simple String Expansion as outlined in variables) { Object value = variables.get(expression.getName()); if (value != null) { String expanded = expression.expand(value, this.encode.isEncodingRequired()); - if (!this.encodeSlash) { + if (this.encodeSlash) { logger.fine("Explicit slash decoding specified, decoding all slashes in uri"); - expanded = expanded.replaceAll("\\%2F", "/"); + expanded = expanded.replaceAll("/", "%2F"); } resolved.append(expanded); } else { @@ -200,7 +201,9 @@ private void parseFragment(String fragment, boolean query) { if (chunk.startsWith("{")) { /* it's an expression, defer encoding until resolution */ - Expression expression = Expressions.create(chunk); + FragmentType type = (query) ? FragmentType.QUERY : FragmentType.PATH_SEGMENT; + + Expression expression = Expressions.create(chunk, type); if (expression == null) { this.templateChunks.add(Literal.create(encode(chunk, query))); } else { diff --git a/core/src/main/java/feign/template/UriUtils.java b/core/src/main/java/feign/template/UriUtils.java index da8e6c029..1fd17dcc8 100644 --- a/core/src/main/java/feign/template/UriUtils.java +++ b/core/src/main/java/feign/template/UriUtils.java @@ -1,5 +1,5 @@ /** - * Copyright 2012-2018 The Feign Authors + * Copyright 2012-2019 The Feign Authors * * Licensed 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 @@ -14,19 +14,18 @@ package feign.template; import feign.Util; +import java.io.ByteArrayOutputStream; import java.io.UnsupportedEncodingException; -import java.net.URI; -import java.net.URL; import java.net.URLDecoder; -import java.net.URLEncoder; import java.nio.charset.Charset; import java.util.regex.Matcher; import java.util.regex.Pattern; public class UriUtils { - private static final String QUERY_RESERVED_CHARACTERS = "?/,="; - private static final String PATH_RESERVED_CHARACTERS = "/=@:!$&\'(),;~"; + + // private static final String QUERY_RESERVED_CHARACTERS = "="; + // private static final String PATH_RESERVED_CHARACTERS = "/=@:!$&\'(),;~"; private static final Pattern PCT_ENCODED_PATTERN = Pattern.compile("%[0-9A-Fa-f][0-9A-Fa-f]"); /** @@ -46,7 +45,7 @@ public static boolean isEncoded(String value) { * @return the encoded value. */ public static String encode(String value) { - return encodeReserved(value, "", Util.UTF_8); + return encodeReserved(value, FragmentType.URI, Util.UTF_8); } /** @@ -57,7 +56,7 @@ public static String encode(String value) { * @return the encoded value. */ public static String encode(String value, Charset charset) { - return encodeReserved(value, "", charset); + return encodeReserved(value, FragmentType.URI, charset); } /** @@ -85,7 +84,13 @@ public static String decode(String value, Charset charset) { * @return the encoded path fragment. */ public static String pathEncode(String path, Charset charset) { - return encodeReserved(path, PATH_RESERVED_CHARACTERS, charset); + return encodeReserved(path, FragmentType.PATH_SEGMENT, charset); + + /* + * path encoding is not equivalent to query encoding, there are few differences, namely dealing + * with spaces, !, ', (, ), and ~ characters. we will need to manually process those values. + */ + // return encoded.replaceAll("\\+", "%20"); } /** @@ -96,46 +101,31 @@ public static String pathEncode(String path, Charset charset) { * @return the encoded query fragment. */ public static String queryEncode(String query, Charset charset) { - return encodeReserved(query, QUERY_RESERVED_CHARACTERS, charset); + return encodeReserved(query, FragmentType.QUERY, charset); + + /* spaces will be encoded as 'plus' symbols here, we want them pct-encoded */ + // return encoded.replaceAll("\\+", "%20"); } /** - * Determines if the provided uri is an absolute uri. + * Uri Encode a Query Parameter name or value. * - * @param uri to evaluate. - * @return true if the uri is absolute. + * @param queryParam containing the query parameter. + * @param charset to use. + * @return the encoded query fragment. */ - public static boolean isAbsolute(String uri) { - return uri != null && !uri.isEmpty() && uri.startsWith("http"); + public static String queryParamEncode(String queryParam, Charset charset) { + return encodeReserved(queryParam, FragmentType.QUERY_PARAM, charset); } /** - * Uri Encode a String using the provided charset. + * Determines if the provided uri is an absolute uri. * - * @param value to encode. - * @param charset to use. - * @return the encoded value. + * @param uri to evaluate. + * @return true if the uri is absolute. */ - private static String urlEncode(String value, Charset charset) { - try { - String encoded = URLEncoder.encode(value, charset.toString()); - - /* - * url encoding is not equivalent to URI encoding, there are few differences, namely dealing - * with spaces, !, ', (, ), and ~ characters. we will need to manually process those values. - */ - return encoded.replaceAll("\\+", "%20") - .replaceAll("\\%21", "!") - .replaceAll("\\%27", "'") - .replaceAll("\\%28", "(") - .replaceAll("\\%29", ")") - .replaceAll("\\%7E", "~") - .replaceAll("\\%2B", "+"); - - } catch (UnsupportedEncodingException uee) { - /* since the encoding is not supported, return the original value */ - return value; - } + public static boolean isAbsolute(String uri) { + return uri != null && !uri.isEmpty() && uri.startsWith("http"); } @@ -144,16 +134,16 @@ private static String urlEncode(String value, Charset charset) { * ignored. * * @param value inspect. - * @param reserved characters to preserve. + * @param type identifying which uri fragment rules to apply. * @param charset to use. * @return a new String with the reserved characters preserved. */ - public static String encodeReserved(String value, String reserved, Charset charset) { + public static String encodeReserved(String value, FragmentType type, Charset charset) { /* value is encoded, we need to split it up and skip the parts that are already encoded */ Matcher matcher = PCT_ENCODED_PATTERN.matcher(value); if (!matcher.find()) { - return encodeChunk(value, reserved, charset); + return encodeChunk(value, type, charset); } int length = value.length(); @@ -164,7 +154,7 @@ public static String encodeReserved(String value, String reserved, Charset chars String before = value.substring(index, matcher.start()); /* encode it */ - encoded.append(encodeChunk(before, reserved, charset)); + encoded.append(encodeChunk(before, type, charset)); /* append the encoded value */ encoded.append(matcher.group()); @@ -175,7 +165,7 @@ public static String encodeReserved(String value, String reserved, Charset chars /* append the rest of the string */ String tail = value.substring(index, length); - encoded.append(encodeChunk(tail, reserved, charset)); + encoded.append(encodeChunk(tail, type, charset)); return encoded.toString(); } @@ -183,40 +173,112 @@ public static String encodeReserved(String value, String reserved, Charset chars * Encode a Uri Chunk, ensuring that all reserved characters are also encoded. * * @param value to encode. - * @param reserved characters to evaluate. + * @param type identifying which uri fragment rules to apply. * @param charset to use. * @return an encoded uri chunk. */ - private static String encodeChunk(String value, String reserved, Charset charset) { - StringBuilder encoded = null; - int length = value.length(); - int index = 0; - for (int i = 0; i < length; i++) { - char character = value.charAt(i); - if (reserved.indexOf(character) != -1) { - if (encoded == null) { - encoded = new StringBuilder(length + 8); + private static String encodeChunk(String value, FragmentType type, Charset charset) { + byte[] data = value.getBytes(charset); + ByteArrayOutputStream encoded = new ByteArrayOutputStream(); + + for (byte b : data) { + if (type.isAllowed(b)) { + encoded.write(b); + } else { + /* percent encode the byte */ + pctEncode(b, encoded); + } + } + return new String(encoded.toByteArray()); + } + + /** + * Percent Encode the provided byte. + * + * @param data to encode + * @param bos with the output stream to use. + */ + private static void pctEncode(byte data, ByteArrayOutputStream bos) { + bos.write('%'); + char hex1 = Character.toUpperCase(Character.forDigit((data >> 4) & 0xF, 16)); + char hex2 = Character.toUpperCase(Character.forDigit(data & 0xF, 16)); + bos.write(hex1); + bos.write(hex2); + } + + enum FragmentType { + + URI { + @Override + boolean isAllowed(int c) { + return isUnreserved(c); + } + }, + RESERVED { + @Override + boolean isAllowed(int c) { + return isUnreserved(c) || isReserved(c); + } + }, + PATH_SEGMENT { + @Override + boolean isAllowed(int c) { + return this.isPchar(c) || (c == '/'); + } + }, + QUERY { + @Override + boolean isAllowed(int c) { + /* although plus signs are allowed, their use is inconsistent. force encoding */ + if (c == '+') { + return false; } - if (i != index) { - /* we are in the middle of the value, so we need to encode mid string */ - encoded.append(urlEncode(value.substring(index, i), charset)); + return this.isPchar(c) || c == '/' || c == '?'; + } + }, + QUERY_PARAM { + @Override + boolean isAllowed(int c) { + /* explicitly encode equals, ampersands, questions */ + if (c == '=' || c == '&' || c == '?') { + return false; } - encoded.append(character); - index = i + 1; + return QUERY.isAllowed(c); } + }; + + abstract boolean isAllowed(int c); + + protected boolean isAlpha(int c) { + return (c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z'); } - /* if there are no reserved characters, encode the original value */ - if (encoded == null) { - return urlEncode(value, charset); + protected boolean isDigit(int c) { + return (c >= '0' && c <= '9'); } - /* encode the rest of the string */ - if (index < length) { - encoded.append(urlEncode(value.substring(index, length), charset)); + protected boolean isGenericDelimiter(int c) { + return (c == ':') || (c == '/') || (c == '?') || (c == '#') || (c == '[') || (c == ']') + || (c == '@'); + } + + protected boolean isSubDelimiter(int c) { + return (c == '!') || (c == '$') || (c == '&') || (c == '\'') || (c == '(') || (c == ')') + || (c == '*') || (c == '+') || (c == ',') || (c == ';') || (c == '='); + } + + protected boolean isUnreserved(int c) { + return this.isAlpha(c) || this.isDigit(c) || c == '-' || c == '.' || c == '_' || c == '~'; + } + + protected boolean isReserved(int c) { + return this.isGenericDelimiter(c) || this.isSubDelimiter(c); + } + + protected boolean isPchar(int c) { + return this.isUnreserved(c) || this.isSubDelimiter(c) || c == ':' || c == '@'; } - return encoded.toString(); } } diff --git a/core/src/test/java/feign/FeignTest.java b/core/src/test/java/feign/FeignTest.java index 3f474312e..d3795bce5 100644 --- a/core/src/test/java/feign/FeignTest.java +++ b/core/src/test/java/feign/FeignTest.java @@ -1,5 +1,5 @@ /** - * Copyright 2012-2018 The Feign Authors + * Copyright 2012-2019 The Feign Authors * * Licensed 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 @@ -768,7 +768,7 @@ public void encodedQueryParam() throws Exception { api.encodedQueryParam("5.2FSi+"); assertThat(server.takeRequest()) - .hasPath("/?trim=5.2FSi+"); + .hasPath("/?trim=5.2FSi%2B"); } @Test diff --git a/core/src/test/java/feign/template/UriTemplateTest.java b/core/src/test/java/feign/template/UriTemplateTest.java index 3845b9851..9176b855d 100644 --- a/core/src/test/java/feign/template/UriTemplateTest.java +++ b/core/src/test/java/feign/template/UriTemplateTest.java @@ -1,5 +1,5 @@ /** - * Copyright 2012-2018 The Feign Authors + * Copyright 2012-2019 The Feign Authors * * Licensed 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 @@ -222,6 +222,15 @@ public void ensurePlusIsSupportedOnPath() { assertThat(expanded).isEqualToIgnoringCase("https://www.example.com/sam+adams/beer/"); } + @Test + public void ensurePlusInEncodedAs2BOnQuery() { + String template = "https://www.example.com/beer?type={type}"; + UriTemplate uriTemplate = UriTemplate.create(template, Util.UTF_8); + Map parameters = Collections.singletonMap("type", "sam+adams"); + String expanded = uriTemplate.expand(parameters); + assertThat(expanded).isEqualToIgnoringCase("https://www.example.com/beer?type=sam%2Badams"); + } + @Test public void incompleteTemplateIsALiteral() { String template = "https://www.example.com/testing/foo}}"; diff --git a/core/src/test/java/feign/template/UriUtilsTest.java b/core/src/test/java/feign/template/UriUtilsTest.java index c0fca8811..353d19935 100644 --- a/core/src/test/java/feign/template/UriUtilsTest.java +++ b/core/src/test/java/feign/template/UriUtilsTest.java @@ -1,5 +1,5 @@ /** - * Copyright 2012-2018 The Feign Authors + * Copyright 2012-2019 The Feign Authors * * Licensed 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 @@ -14,23 +14,80 @@ package feign.template; +import static java.nio.charset.StandardCharsets.UTF_8; import static org.assertj.core.api.Assertions.assertThat; -import java.nio.charset.StandardCharsets; +import java.nio.charset.Charset; import org.junit.Test; public class UriUtilsTest { + + /** + * Verify that values outside of the allowed characters in a path segment are pct-encoded. The + * list of approved characters used are those listed in