Introduce path encoding (fixes #1561)

HttpRequestBuilder.java - replaced armeria QueryParamsBuilder with apache http `URIBuilder`. - minor generics cleanup - `URIBuilder` now handles path segment encoding and query parameters. - we now check if any paths are prefixed with `/` and issue a warning log, that this is probably a mistake encoding.feature - added more demo/test cases for path encoding HttpUtils.java - minor generics cleanup - final is redundant on static methods karate-demo/pom.xml - scope = import only works in the dependencyManagement section, this fixes the maven warning. Request.java - minor generics cleanup - use computeIfAbsent() HttpClientFactory.java - public static final are redundant on interface fields - also use method reference KarateMockHandlerTest.java KarateHttpMockHandlerTest.java - removed all `/` path prefixes README.md - updated with details on how to correctly use path - changes any erroneous examples of path usage Introduce path encoding (fixes #1561) HttpRequestBuilder.java - replaced armeria QueryParamsBuilder with apache http URIBuilder. - minor generics cleanup - introduced support for fragments. - URIBuilder now handles path segment encoding, query parameters and any fragment. - care was taken to ensure that path's prefixed with / are accepted, but a warning is printed letting people know they should remove the prefixing / encoding.feature - added more demo/test cases for path encoding HttpUtils.java - minor generics cleanup - final is redundant on static methods karate-demo/pom.xml - scope = import only works in the dependencyManagement section, this fixes the maven warning. Request.java - minor generics cleanup - use computeIfAbsent() - cleanup the setUrl() method, no longer need to hack around Spring's decoding issue, at least in this class.. HttpClientFactory.java - public static final are redundant on interface fields - also use method reference EncodingController.java - because this controller is used in two different context's 1) spring's mock mvc environment 2) spring boot + tomcat - it needs to account for the different quirks of those environments. MockHttpClient.java - spring is somehow messing up the decoding of the pathInfo from the supplied url, using ISO 8859-1 instead of UTF-8, so here we explicitly set the pathInfo variable using the result of the trusted URI class :)
karatelabs · May 28, 2021 · becd0e9 · becd0e9
1 parent bbecea4
commit becd0e9
Show file tree

Hide file tree

Showing 14 changed files with 176 additions and 150 deletions.
diff --git a/README.md b/README.md
@@ -1589,17 +1589,23 @@ Given url 'https://' + e2eHostName + '/v1/api'
 If you are trying to build dynamic URLs including query-string parameters in the form: `http://myhost/some/path?foo=bar&search=true` - please refer to the [`param`](#param) keyword.
 
 ## `path`
-REST-style path parameters.  Can be expressions that will be evaluated.  Comma delimited values are supported which can be more convenient, and takes care of URL-encoding and appending '/' where needed.
+REST-style path parameters.  Can be expressions that will be evaluated.  Comma delimited values are supported which can be more convenient, and takes care of URL-encoding and appending '/' between path segments as needed.
 ```cucumber
+# this is invalid and will result in / being encoded as %2F when sent to the remote server
+# eg. given a documentId of 1234 the path will be: /documents%2F1234%2Fdownload
 Given path 'documents/' + documentId + '/download'
 
-# this is equivalent to the above
+# this is the correct way to specify multiple paths
 Given path 'documents', documentId, 'download'
 
 # or you can do the same on multiple lines if you wish
 Given path 'documents'
 And path documentId
 And path 'download'
+
+# you can also ensure that the constructed url has a trailing / by appending an empty path segment
+# eg. given a documentId of 1234 the path will be: /documents/1234/download/
+Given path 'documents', documentId, 'download', ''
 ```
 Note that the `path` 'resets' after any HTTP request is made but not the `url`. The [Hello World](#hello-world) is a great example of 'REST-ful' use of the `url` when the test focuses on a single REST 'resource'. Look at how the `path` did not need to be specified for the second HTTP `get` call since `/cats` is part of the `url`.
 
@@ -1820,7 +1826,7 @@ Use this for multipart content items that don't have field-names.  Here below is
 also demonstrates using the [`multipart/related`](https://tools.ietf.org/html/rfc2387) content-type.
 
 ```cucumber
-Given path '/v2/documents'
+Given path 'v2', 'documents'
 And multipart entity read('foo.json')
 And multipart field image = read('bar.jpg')
 And header Content-Type = 'multipart/related'

diff --git a/karate-core/src/main/java/com/intuit/karate/http/HttpClientFactory.java b/karate-core/src/main/java/com/intuit/karate/http/HttpClientFactory.java
@@ -34,6 +34,6 @@ public interface HttpClientFactory {
 
     HttpClient create(ScenarioEngine engine);
 
-    public static final HttpClientFactory DEFAULT = engine -> new ApacheHttpClient(engine);
+    HttpClientFactory DEFAULT = ApacheHttpClient::new;
 
 }
diff --git a/karate-core/src/main/java/com/intuit/karate/http/HttpRequestBuilder.java b/karate-core/src/main/java/com/intuit/karate/http/HttpRequestBuilder.java
@@ -28,11 +28,11 @@
 import com.intuit.karate.graal.JsArray;
 import com.intuit.karate.graal.JsValue;
 import com.intuit.karate.graal.Methods;
-import com.linecorp.armeria.common.QueryParams;
-import com.linecorp.armeria.common.QueryParamsBuilder;
 import io.netty.handler.codec.http.cookie.ClientCookieEncoder;
 import io.netty.handler.codec.http.cookie.Cookie;
 import io.netty.handler.codec.http.cookie.DefaultCookie;
+import java.net.URI;
+import java.net.URISyntaxException;
 import java.nio.charset.Charset;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -47,7 +47,9 @@
 import java.util.function.Function;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
+import org.apache.http.client.utils.URIBuilder;
 import org.graalvm.polyglot.Value;
 import org.graalvm.polyglot.proxy.ProxyObject;
 import org.slf4j.Logger;
@@ -84,7 +86,7 @@ public class HttpRequestBuilder implements ProxyObject {
         URL, METHOD, PATH, PARAM, PARAMS, HEADER, HEADERS, BODY, INVOKE,
         GET, POST, PUT, DELETE, PATCH, HEAD, CONNECT, OPTIONS, TRACE
     };
-    private static final Set<String> KEY_SET = new HashSet(Arrays.asList(KEYS));
+    private static final Set<String> KEY_SET = new HashSet<>(Arrays.asList(KEYS));
     private static final JsArray KEY_ARRAY = new JsArray(KEYS);
 
     private String url;
@@ -159,14 +161,7 @@ public HttpRequest build() {
             }
             multiPart = null;
         }
-        String urlAndPath = getUrlAndPath();
-        if (params != null) {
-            QueryParamsBuilder qpb = QueryParams.builder();
-            params.forEach((k, v) -> qpb.add(k, v));
-            String append = urlAndPath.indexOf('?') == -1 ? "?" : "&";
-            urlAndPath = urlAndPath + append + qpb.toQueryString();
-        }
-        request.setUrl(urlAndPath);
+        request.setUrl(getUri().toASCIIString());
         if (multiPart != null) {
             if (body == null) { // this is not-null only for a re-try, don't rebuild multi-part
                 body = multiPart.build();
@@ -183,7 +178,7 @@ public HttpRequest build() {
             request.setBodyForDisplay(multiPart.getBodyForDisplay());
         }
         if (cookies != null && !cookies.isEmpty()) {
-            List<String> cookieValues = new ArrayList(cookies.size());
+            List<String> cookieValues = new ArrayList<>(cookies.size());
             for (Cookie c : cookies) {
                 String cookieValue = ClientCookieEncoder.LAX.encode(c);
                 cookieValues.add(cookieValue);
@@ -263,32 +258,29 @@ public HttpRequestBuilder path(String path) {
         return this;
     }
 
-    private String getPath() {
-        String temp = "";
-        if (paths == null) {
-            return temp;
-        }
-        for (String path : paths) {
-            if (path.startsWith("/")) {
-                path = path.substring(1);
+    private URI getUri() {
+        try {
+            URIBuilder builder = url == null ? new URIBuilder() : new URIBuilder(url);
+            if (params != null) {
+                params.forEach((key, values) -> values.forEach(value -> builder.addParameter(key, value)));
             }
-            if (!temp.isEmpty() && !temp.endsWith("/")) {
-                temp = temp + "/";
+            if (paths != null) {
+                paths.forEach(path -> {
+                    if (path.startsWith("/")) {
+                        logger.warn("Path segment: '{}' starts with a '/', this is probably a mistake. The '/' character will be escaped and sent to the remote server as '%2F'. " +
+                                "If you want to include multiple paths please separate them using commas. Ie. 'hello', 'world' instead of '/hello/world'.", path);
+                    }
+                });
+                // merge paths from the supplied url with additional paths supplied to this builder
+                List<String> merged = Stream.of(builder.getPathSegments(), paths)
+                        .flatMap(List::stream)
+                        .collect(Collectors.toList());
+                builder.setPathSegments(merged);
             }
-            temp = temp + path;
-        }
-        return temp;
-    }
-
-    public String getUrlAndPath() {
-        if (url == null) {
-            url = "";
-        }
-        String path = getPath();
-        if (path.isEmpty()) {
-            return url;
+            return builder.build();
+        } catch (URISyntaxException e) {
+            throw new RuntimeException(e);
         }
-        return url.endsWith("/") ? url + path : url + "/" + path;
     }
 
     public HttpRequestBuilder body(Object body) {
@@ -331,7 +323,7 @@ public HttpRequestBuilder header(String name, String... values) {
 
     public HttpRequestBuilder header(String name, List<String> values) {
         if (headers == null) {
-            headers = new LinkedHashMap();
+            headers = new LinkedHashMap<>();
         }
         for (String key : headers.keySet()) {
             if (key.equalsIgnoreCase(name)) {
@@ -390,7 +382,7 @@ public HttpRequestBuilder param(String name, String... values) {
 
     public HttpRequestBuilder param(String name, List<String> values) {
         if (params == null) {
-            params = new HashMap();
+            params = new HashMap<>();
         }
         List<String> notNullValues = values.stream().filter(v -> v != null).collect(Collectors.toList());
         if (!notNullValues.isEmpty()) {
@@ -417,7 +409,7 @@ public HttpRequestBuilder cookie(Map<String, Object> map) {
 
     public HttpRequestBuilder cookie(Cookie cookie) {
         if (cookies == null) {
-            cookies = new HashSet();
+            cookies = new HashSet<>();
         }
         cookies.add(cookie);
         return this;
@@ -451,7 +443,7 @@ public HttpRequestBuilder multiPart(Map<String, Object> map) {
     //
     private final Methods.FunVar PATH_FUNCTION = args -> {
         if (args.length == 0) {
-            return getPath();
+            return getUri().getPath();
         } else {
             for (Object o : args) {
                 if (o != null) {
@@ -596,7 +588,7 @@ public boolean hasMember(String key) {
 
     @Override
     public String toString() {
-        return getUrlAndPath();
+        return getUri().toASCIIString();
     }
 
 }
diff --git a/karate-core/src/main/java/com/intuit/karate/http/HttpUtils.java b/karate-core/src/main/java/com/intuit/karate/http/HttpUtils.java
@@ -64,7 +64,7 @@ public static Map<String, String> parseContentTypeParams(String mimeType) {
         if (count <= 1) {
             return null;
         }
-        Map<String, String> map = new LinkedHashMap(count - 1);
+        Map<String, String> map = new LinkedHashMap<>(count - 1);
         for (int i = 1; i < count; i++) {
             String item = items.get(i);
             int pos = item.indexOf('=');
@@ -90,7 +90,7 @@ public static Map<String, String> parseUriPattern(String pattern, String url) {
         if (rightSize != leftSize) {
             return null;
         }
-        Map<String, String> map = new LinkedHashMap(leftSize);
+        Map<String, String> map = new LinkedHashMap<>(leftSize);
         for (int i = 0; i < leftSize; i++) {
             String left = leftList.get(i);
             String right = rightList.get(i);
@@ -107,7 +107,7 @@ public static Map<String, String> parseUriPattern(String pattern, String url) {
         return map;
     }
 
-    public static final String normaliseUriPath(String uri) {
+    public static String normaliseUriPath(String uri) {
         uri = uri.indexOf('?') == -1 ? uri : uri.substring(0, uri.indexOf('?'));
         if (uri.endsWith("/")) {
             uri = uri.substring(0, uri.length() - 1);
@@ -224,7 +224,7 @@ public static FullHttpResponse transform(FullHttpResponse original, String body)
 
     private static final HttpResponseStatus CONNECTION_ESTABLISHED = new HttpResponseStatus(200, "Connection established");
 
-    public static final FullHttpResponse connectionEstablished() {
+    public static FullHttpResponse connectionEstablished() {
         return new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, CONNECTION_ESTABLISHED);
     }
 
@@ -243,7 +243,7 @@ public static void addViaHeader(HttpMessage msg, String alias) {
         List<String> list;
         if (msg.headers().contains(HttpHeaderNames.VIA)) {
             List<String> existing = msg.headers().getAll(HttpHeaderNames.VIA);
-            list = new ArrayList(existing);
+            list = new ArrayList<>(existing);
             list.add(sb.toString());
         } else {
             list = Collections.singletonList(sb.toString());

diff --git a/karate-core/src/main/java/com/intuit/karate/http/Request.java b/karate-core/src/main/java/com/intuit/karate/http/Request.java
@@ -92,7 +92,7 @@ public class Request implements ProxyObject {
         PATH, METHOD, PARAM, PARAMS, HEADER, HEADERS, PATH_PARAM, PATH_PARAMS, BODY, MULTI_PART, MULTI_PARTS, JSON, AJAX,
         GET, POST, PUT, DELETE, PATCH, HEAD, CONNECT, OPTIONS, TRACE
     };
-    private static final Set<String> KEY_SET = new HashSet(Arrays.asList(KEYS));
+    private static final Set<String> KEY_SET = new HashSet<>(Arrays.asList(KEYS));
     private static final JsArray KEY_ARRAY = new JsArray(KEYS);
 
     private String urlAndPath;
@@ -106,7 +106,7 @@ public class Request implements ProxyObject {
     private ResourceType resourceType;
     private String resourcePath;
     private String pathParam;
-    private List pathParams = Collections.EMPTY_LIST;
+    private List pathParams = Collections.emptyList();
     private RequestContext requestContext;
 
     public RequestContext getRequestContext() {
@@ -149,7 +149,7 @@ public String getContentType() {
     public List<Cookie> getCookies() {
          List<String> cookieValues = getHeaderValues(HttpConstants.HDR_COOKIE);
          if (cookieValues == null) {
-             return Collections.EMPTY_LIST;
+             return Collections.emptyList();
          }
          return cookieValues.stream().map(ClientCookieDecoder.STRICT::decode).collect(toList());
     }
@@ -178,17 +178,8 @@ public void setUrl(String url) {
         StringUtils.Pair pair = HttpUtils.parseUriIntoUrlBaseAndPath(url);
         urlBase = pair.left;
         QueryStringDecoder qsd = new QueryStringDecoder(pair.right);
-        String path = qsd.path();
-        Map<String, List<String>> queryParams = qsd.parameters();
-        if (queryParams.size() == 1) {
-            List<String> list = queryParams.values().iterator().next();
-            if (!list.isEmpty() && "".equals(list.get(0))) {
-                // annoying edge case where url had encoded characters
-                path = pair.right.replace('?', '�');
-            }
-        }
-        setPath(path);
-        setParams(queryParams);
+        setPath(qsd.path());
+        setParams(qsd.parameters());
     }
 
     public String getUrlAndPath() {
@@ -231,7 +222,7 @@ public void setMethod(String method) {
     }
 
     public Map<String, List<String>> getParams() {
-        return params == null ? Collections.EMPTY_MAP : params;
+        return params == null ? Collections.emptyMap() : params;
     }
 
     public void setParams(Map<String, List<String>> params) {
@@ -255,7 +246,7 @@ public void setPathParams(List pathParams) {
     }
 
     public Map<String, List<String>> getHeaders() {
-        return headers == null ? Collections.EMPTY_MAP : headers;
+        return headers == null ? Collections.emptyMap() : headers;
     }
 
     public void setHeaders(Map<String, List<String>> headers) {
@@ -282,7 +273,7 @@ public Object getBodyConverted() {
         try {
             return JsValue.fromBytes(body, false, rt);
         } catch (Exception e) {
-            logger.trace("failed to auto-convert response: {}", e);
+            logger.trace("failed to auto-convert response", e);
             return getBodyAsString();
         }
     }
@@ -335,27 +326,23 @@ public void processBody() {
         boolean multipart;
         if (contentType.startsWith("multipart")) {
             multipart = true;
-            multiParts = new HashMap();
+            multiParts = new HashMap<>();
         } else if (contentType.contains("form-urlencoded")) {
             multipart = false;
         } else {
             return;
         }
         logger.trace("decoding content-type: {}", contentType);
-        params = (params == null || params.isEmpty()) ? new HashMap() : new HashMap(params); // since it may be immutable
+        params = (params == null || params.isEmpty()) ? new HashMap<>() : new HashMap<>(params); // since it may be immutable
         DefaultFullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.valueOf(method), path, Unpooled.wrappedBuffer(body));
         request.headers().add(HttpConstants.HDR_CONTENT_TYPE, contentType);
         InterfaceHttpPostRequestDecoder decoder = multipart ? new HttpPostMultipartRequestDecoder(request) : new HttpPostStandardRequestDecoder(request);
         try {
             for (InterfaceHttpData part : decoder.getBodyHttpDatas()) {
                 String name = part.getName();
                 if (multipart && part instanceof FileUpload) {
-                    List<Map<String, Object>> list = multiParts.get(name);
-                    if (list == null) {
-                        list = new ArrayList();
-                        multiParts.put(name, list);
-                    }
-                    Map<String, Object> map = new HashMap();
+                    List<Map<String, Object>> list = multiParts.computeIfAbsent(name, k -> new ArrayList<>());
+                    Map<String, Object> map = new HashMap<>();
                     list.add(map);
                     FileUpload fup = (FileUpload) part;
                     map.put("name", name);
@@ -373,11 +360,7 @@ public void processBody() {
                     }
                 } else { // form-field, url-encoded if not multipart
                     Attribute attribute = (Attribute) part;
-                    List<String> list = params.get(name);
-                    if (list == null) {
-                        list = new ArrayList();
-                        params.put(name, list);
-                    }
+                    List<String> list = params.computeIfAbsent(name, k -> new ArrayList<>());
                     list.add(attribute.getValue());
                 }
             }