Introduce path encoding (fixes karatelabs#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() HttpClientFactory.java - public static final are redundant on interface fields - also use method reference
aaronjwhiteside · Apr 19, 2021 · 759baca · 759baca
1 parent 789e8c7
commit 759baca
Show file tree

Hide file tree

Showing 7 changed files with 105 additions and 72 deletions.
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,13 +86,14 @@ 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;
     private String method;
     private List<String> paths;
     private Map<String, List<String>> params;
+    private String fragment;
     private Map<String, List<String>> headers;
     private MultiPartBuilder multiPart;
     private Object body;
@@ -159,14 +162,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 +179,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);
@@ -245,6 +241,11 @@ public HttpRequestBuilder method(String method) {
         return this;
     }
 
+    public HttpRequestBuilder fragment(String fragment) {
+        this.fragment = fragment;
+        return this;
+    }
+
     public HttpRequestBuilder paths(String... paths) {
         for (String path : paths) {
             path(path);
@@ -263,32 +264,39 @@ public HttpRequestBuilder path(String path) {
         return this;
     }
 
-    private String getPath() {
-        String temp = "";
+    private List<String> backwardsCompatiblePaths() {
         if (paths == null) {
-            return temp;
+            return Collections.emptyList();
         }
-        for (String path : paths) {
-            if (path.startsWith("/")) {
+
+        List<String> result = new ArrayList<>(paths.size());
+        for (int i = 0; i < paths.size(); i++) {
+            String path = paths.get(i);
+            if (i == 0 && path.startsWith("/")) {
                 path = path.substring(1);
+                logger.warn("the first path segment starts with a '/', this will be stripped off for now, but in the future this may be escaped and cause your request to fail.");
             }
-            if (!temp.isEmpty() && !temp.endsWith("/")) {
-                temp = temp + "/";
-            }
-            temp = temp + path;
+            result.add(path);
         }
-        return temp;
+        return result;
     }
 
-    public String getUrlAndPath() {
-        if (url == null) {
-            url = "";
-        }
-        String path = getPath();
-        if (path.isEmpty()) {
-            return url;
+    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)));
+            }
+            // merge paths from the base url with additional paths supplied to this builder
+            List<String> merged = Stream.of(builder.getPathSegments(), backwardsCompatiblePaths())
+                    .flatMap(List::stream)
+                    .collect(Collectors.toList());
+            return builder.setPathSegments(merged)
+                    .setFragment(fragment)
+                    .build();
+        } catch (URISyntaxException e) {
+            throw new RuntimeException(e);
         }
-        return url.endsWith("/") ? url + path : url + "/" + path;
     }
 
     public HttpRequestBuilder body(Object body) {
@@ -331,7 +339,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 +398,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 +425,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 +459,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 +604,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());
     }
@@ -231,7 +231,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 +255,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 +282,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 +335,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 +369,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());
                 }
             }

diff --git a/karate-core/src/test/java/com/intuit/karate/core/KarateHttpMockHandlerTest.java b/karate-core/src/test/java/com/intuit/karate/core/KarateHttpMockHandlerTest.java
@@ -159,7 +159,7 @@ void testKarateRemove() {
         startMockServer();
         run(
                 urlStep(),
-                "path '/hello/1'",
+                "path '/hello', '1'",
                 "method get"
         );
         matchVarContains("response", "{ '2': 'bar' }");

diff --git a/karate-demo/pom.xml b/karate-demo/pom.xml
@@ -18,14 +18,19 @@
 
     <!-- you will not need all these dependencies for a typical karate project !-->
     <!-- for a skeleton project, use the archetype: https://github.com/intuit/karate#quickstart !-->
-    <dependencies>        
-        <dependency>
-            <groupId>org.springframework.boot</groupId>
-            <artifactId>spring-boot-dependencies</artifactId>
-            <version>${spring.boot.version}</version>
-            <type>pom</type>
-            <scope>import</scope>
-        </dependency>        
+    <dependencyManagement>
+        <dependencies>
+            <dependency>
+                <groupId>org.springframework.boot</groupId>
+                <artifactId>spring-boot-dependencies</artifactId>
+                <version>${spring.boot.version}</version>
+                <type>pom</type>
+                <scope>import</scope>
+            </dependency>
+        </dependencies>
+    </dependencyManagement>
+
+    <dependencies>
         <dependency>
             <groupId>org.springframework.boot</groupId>
             <artifactId>spring-boot-starter-websocket</artifactId>
@@ -175,7 +180,7 @@
                                 <include>demo/DemoTestParallel.java</include>
                             </includes>
                             <!-- needed for jacoco to integrate properly -->
-                            <argLine>${argLine}</argLine>
+                            <argLine>-Dfile.encoding=UTF-8 ${argLine}</argLine>
                         </configuration>
                     </plugin>                     
                     <plugin>