From bf9b0f2d640aee805f9f3b097573f95753d344c5 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Fri, 29 Jul 2016 21:10:21 +0100 Subject: [PATCH] Ensure that query string is not duplicated when parameters overlap Closes gh-286 --- .../restdocs/cli/CliOperationRequest.java | 28 ------- .../restdocs/cli/CurlRequestSnippet.java | 14 ++-- .../restdocs/cli/HttpieRequestSnippet.java | 24 +++--- .../restdocs/http/HttpRequestSnippet.java | 9 ++- .../restdocs/operation/Parameters.java | 35 ++++++++ .../restdocs/cli/CurlRequestSnippetTests.java | 72 +++++++++++++++-- .../cli/HttpieRequestSnippetTests.java | 72 +++++++++++++++-- .../http/HttpRequestSnippetTests.java | 81 +++++++++++++++++++ 8 files changed, 276 insertions(+), 59 deletions(-) diff --git a/spring-restdocs-core/src/main/java/org/springframework/restdocs/cli/CliOperationRequest.java b/spring-restdocs-core/src/main/java/org/springframework/restdocs/cli/CliOperationRequest.java index ce305278..b8899410 100644 --- a/spring-restdocs-core/src/main/java/org/springframework/restdocs/cli/CliOperationRequest.java +++ b/spring-restdocs-core/src/main/java/org/springframework/restdocs/cli/CliOperationRequest.java @@ -52,34 +52,6 @@ final class CliOperationRequest implements OperationRequest { new BasicAuthHeaderFilter(), new HostHeaderFilter(delegate.getUri()))); } - Parameters getUniqueParameters() { - Parameters queryStringParameters = new QueryStringParser() - .parse(this.delegate.getUri()); - Parameters uniqueParameters = new Parameters(); - - for (Map.Entry> parameter : this.delegate.getParameters() - .entrySet()) { - addIfUnique(parameter, queryStringParameters, uniqueParameters); - } - return uniqueParameters; - } - - private void addIfUnique(Map.Entry> parameter, - Parameters queryStringParameters, Parameters uniqueParameters) { - if (!queryStringParameters.containsKey(parameter.getKey())) { - uniqueParameters.put(parameter.getKey(), parameter.getValue()); - } - else { - List candidates = parameter.getValue(); - List existing = queryStringParameters.get(parameter.getKey()); - for (String candidate : candidates) { - if (!existing.contains(candidate)) { - uniqueParameters.add(parameter.getKey(), candidate); - } - } - } - } - boolean isPutOrPost() { return HttpMethod.PUT.equals(this.delegate.getMethod()) || HttpMethod.POST.equals(this.delegate.getMethod()); diff --git a/spring-restdocs-core/src/main/java/org/springframework/restdocs/cli/CurlRequestSnippet.java b/spring-restdocs-core/src/main/java/org/springframework/restdocs/cli/CurlRequestSnippet.java index 31127c3c..4f8d603c 100644 --- a/spring-restdocs-core/src/main/java/org/springframework/restdocs/cli/CurlRequestSnippet.java +++ b/spring-restdocs-core/src/main/java/org/springframework/restdocs/cli/CurlRequestSnippet.java @@ -70,9 +70,12 @@ public class CurlRequestSnippet extends TemplatedSnippet { private String getUrl(Operation operation) { OperationRequest request = operation.getRequest(); - if (!request.getParameters().isEmpty() && includeParametersInUri(request)) { - return String.format("'%s?%s'", request.getUri(), - request.getParameters().toQueryString()); + Parameters uniqueParameters = request.getParameters() + .getUniqueParameters(operation.getRequest().getUri()); + if (!uniqueParameters.isEmpty() && includeParametersInUri(request)) { + return String.format("'%s%s%s'", request.getUri(), + StringUtils.hasText(request.getUri().getRawQuery()) ? "&" : "?", + uniqueParameters.toQueryString()); } return String.format("'%s'", request.getUri()); } @@ -157,9 +160,10 @@ public class CurlRequestSnippet extends TemplatedSnippet { } } - private void writeContentUsingParameters(CliOperationRequest request, + private void writeContentUsingParameters(OperationRequest request, PrintWriter writer) { - Parameters uniqueParameters = request.getUniqueParameters(); + Parameters uniqueParameters = request.getParameters() + .getUniqueParameters(request.getUri()); String queryString = uniqueParameters.toQueryString(); if (StringUtils.hasText(queryString)) { writer.print(String.format(" -d '%s'", queryString)); diff --git a/spring-restdocs-core/src/main/java/org/springframework/restdocs/cli/HttpieRequestSnippet.java b/spring-restdocs-core/src/main/java/org/springframework/restdocs/cli/HttpieRequestSnippet.java index 4c95c04d..10b96fac 100644 --- a/spring-restdocs-core/src/main/java/org/springframework/restdocs/cli/HttpieRequestSnippet.java +++ b/spring-restdocs-core/src/main/java/org/springframework/restdocs/cli/HttpieRequestSnippet.java @@ -90,10 +90,13 @@ public class HttpieRequestSnippet extends TemplatedSnippet { return options.toString(); } - private String getUrl(CliOperationRequest request) { - if (!request.getUniqueParameters().isEmpty() && includeParametersInUri(request)) { - return String.format("'%s?%s'", request.getUri(), - request.getParameters().toQueryString()); + private String getUrl(OperationRequest request) { + Parameters uniqueParameters = request.getParameters() + .getUniqueParameters(request.getUri()); + if (!uniqueParameters.isEmpty() && includeParametersInUri(request)) { + return String.format("'%s%s%s'", request.getUri(), + StringUtils.hasText(request.getUri().getRawQuery()) ? "&" : "?", + uniqueParameters.toQueryString()); } return String.format("'%s'", request.getUri()); } @@ -107,14 +110,15 @@ public class HttpieRequestSnippet extends TemplatedSnippet { return requestItems.toString(); } - private void writeOptions(CliOperationRequest request, PrintWriter writer) { - if (!request.getParts().isEmpty() || (!request.getUniqueParameters().isEmpty() - && !includeParametersInUri(request))) { + private void writeOptions(OperationRequest request, PrintWriter writer) { + if (!request.getParts().isEmpty() + || (!request.getParameters().getUniqueParameters(request.getUri()) + .isEmpty() && !includeParametersInUri(request))) { writer.print("--form "); } } - private boolean includeParametersInUri(CliOperationRequest request) { + private boolean includeParametersInUri(OperationRequest request) { return request.getMethod() == HttpMethod.GET || request.getContent().length > 0; } @@ -167,7 +171,9 @@ public class HttpieRequestSnippet extends TemplatedSnippet { writeContentUsingParameters(request.getParameters(), writer); } else if (request.isPutOrPost()) { - writeContentUsingParameters(request.getUniqueParameters(), writer); + writeContentUsingParameters( + request.getParameters().getUniqueParameters(request.getUri()), + writer); } } diff --git a/spring-restdocs-core/src/main/java/org/springframework/restdocs/http/HttpRequestSnippet.java b/spring-restdocs-core/src/main/java/org/springframework/restdocs/http/HttpRequestSnippet.java index c966f137..0110dd1b 100644 --- a/spring-restdocs-core/src/main/java/org/springframework/restdocs/http/HttpRequestSnippet.java +++ b/spring-restdocs-core/src/main/java/org/springframework/restdocs/http/HttpRequestSnippet.java @@ -30,6 +30,7 @@ import org.springframework.http.MediaType; import org.springframework.restdocs.operation.Operation; import org.springframework.restdocs.operation.OperationRequest; import org.springframework.restdocs.operation.OperationRequestPart; +import org.springframework.restdocs.operation.Parameters; import org.springframework.restdocs.snippet.Snippet; import org.springframework.restdocs.snippet.TemplatedSnippet; import org.springframework.util.StringUtils; @@ -75,12 +76,14 @@ public class HttpRequestSnippet extends TemplatedSnippet { private String getPath(OperationRequest request) { String path = request.getUri().getRawPath(); String queryString = request.getUri().getRawQuery(); - if (!request.getParameters().isEmpty() && includeParametersInUri(request)) { + Parameters uniqueParameters = request.getParameters() + .getUniqueParameters(request.getUri()); + if (!uniqueParameters.isEmpty() && includeParametersInUri(request)) { if (StringUtils.hasText(queryString)) { - queryString = queryString + "&" + request.getParameters().toQueryString(); + queryString = queryString + "&" + uniqueParameters.toQueryString(); } else { - queryString = request.getParameters().toQueryString(); + queryString = uniqueParameters.toQueryString(); } } if (StringUtils.hasText(queryString)) { diff --git a/spring-restdocs-core/src/main/java/org/springframework/restdocs/operation/Parameters.java b/spring-restdocs-core/src/main/java/org/springframework/restdocs/operation/Parameters.java index b696c292..bd05f421 100644 --- a/spring-restdocs-core/src/main/java/org/springframework/restdocs/operation/Parameters.java +++ b/spring-restdocs-core/src/main/java/org/springframework/restdocs/operation/Parameters.java @@ -17,10 +17,12 @@ package org.springframework.restdocs.operation; import java.io.UnsupportedEncodingException; +import java.net.URI; import java.net.URLEncoder; import java.util.List; import java.util.Map; +import org.springframework.restdocs.cli.QueryStringParser; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.StringUtils; @@ -53,6 +55,39 @@ public class Parameters extends LinkedMultiValueMap { return sb.toString(); } + /** + * Returns a new {@code Parameters} containing only the parameters that do no appear + * in the query string of the given {@code uri}. + * + * @param uri the uri + * @return the unique parameters + */ + public Parameters getUniqueParameters(URI uri) { + Parameters queryStringParameters = new QueryStringParser().parse(uri); + Parameters uniqueParameters = new Parameters(); + + for (Map.Entry> parameter : entrySet()) { + addIfUnique(parameter, queryStringParameters, uniqueParameters); + } + return uniqueParameters; + } + + private void addIfUnique(Map.Entry> parameter, + Parameters queryStringParameters, Parameters uniqueParameters) { + if (!queryStringParameters.containsKey(parameter.getKey())) { + uniqueParameters.put(parameter.getKey(), parameter.getValue()); + } + else { + List candidates = parameter.getValue(); + List existing = queryStringParameters.get(parameter.getKey()); + for (String candidate : candidates) { + if (!existing.contains(candidate)) { + uniqueParameters.add(parameter.getKey(), candidate); + } + } + } + } + private static void append(StringBuilder sb, String key) { append(sb, key, ""); } diff --git a/spring-restdocs-core/src/test/java/org/springframework/restdocs/cli/CurlRequestSnippetTests.java b/spring-restdocs-core/src/test/java/org/springframework/restdocs/cli/CurlRequestSnippetTests.java index e26ca217..314234b8 100644 --- a/spring-restdocs-core/src/test/java/org/springframework/restdocs/cli/CurlRequestSnippetTests.java +++ b/spring-restdocs-core/src/test/java/org/springframework/restdocs/cli/CurlRequestSnippetTests.java @@ -95,6 +95,47 @@ public class CurlRequestSnippetTests extends AbstractSnippetTests { .request("http://localhost/foo?param=value").build()); } + @Test + public void getRequestWithTotallyOverlappingQueryStringAndParameters() + throws IOException { + this.snippet + .expectCurlRequest( + "request-with-totally-overlapping-query-string-and-parameters") + .withContents(codeBlock("bash") + .content("$ curl 'http://localhost/foo?param=value' -i")); + new CurlRequestSnippet().document(operationBuilder( + "request-with-totally-overlapping-query-string-and-parameters") + .request("http://localhost/foo?param=value") + .param("param", "value").build()); + } + + @Test + public void getRequestWithPartiallyOverlappingQueryStringAndParameters() + throws IOException { + this.snippet + .expectCurlRequest( + "request-with-partially-overlapping-query-string-and-parameters") + .withContents(codeBlock("bash") + .content("$ curl 'http://localhost/foo?a=alpha&b=bravo' -i")); + new CurlRequestSnippet().document(operationBuilder( + "request-with-partially-overlapping-query-string-and-parameters") + .request("http://localhost/foo?a=alpha").param("a", "alpha") + .param("b", "bravo").build()); + } + + @Test + public void getRequestWithDisjointQueryStringAndParameters() throws IOException { + this.snippet + .expectCurlRequest( + "request-with-partially-overlapping-query-string-and-parameters") + .withContents(codeBlock("bash") + .content("$ curl 'http://localhost/foo?a=alpha&b=bravo' -i")); + new CurlRequestSnippet().document(operationBuilder( + "request-with-partially-overlapping-query-string-and-parameters") + .request("http://localhost/foo?a=alpha").param("b", "bravo") + .build()); + } + @Test public void getRequestWithQueryStringWithNoValue() throws IOException { this.snippet.expectCurlRequest("request-with-query-string-with-no-value") @@ -172,25 +213,42 @@ public class CurlRequestSnippetTests extends AbstractSnippetTests { } @Test - public void postRequestWithQueryStringAndParameter() throws IOException { - this.snippet.expectCurlRequest("post-request-with-query-string-and-parameter") + public void postRequestWithDisjointQueryStringAndParameter() throws IOException { + this.snippet + .expectCurlRequest( + "post-request-with-disjoint-query-string-and-parameter") .withContents(codeBlock("bash").content( "$ curl 'http://localhost/foo?a=alpha' -i -X POST -d 'b=bravo'")); - new CurlRequestSnippet() - .document(operationBuilder("post-request-with-query-string-and-parameter") + new CurlRequestSnippet().document( + operationBuilder("post-request-with-disjoint-query-string-and-parameter") .request("http://localhost/foo?a=alpha").method("POST") .param("b", "bravo").build()); } @Test - public void postRequestWithOverlappingQueryStringAndParameters() throws IOException { + public void postRequestWithTotallyOverlappingQueryStringAndParameters() + throws IOException { this.snippet .expectCurlRequest( - "post-request-with-overlapping-query-string-and-parameters") + "post-request-with-totally-overlapping-query-string-and-parameters") + .withContents(codeBlock("bash").content( + "$ curl 'http://localhost/foo?a=alpha&b=bravo' -i -X POST")); + new CurlRequestSnippet().document(operationBuilder( + "post-request-with-totally-overlapping-query-string-and-parameters") + .request("http://localhost/foo?a=alpha&b=bravo").method("POST") + .param("a", "alpha").param("b", "bravo").build()); + } + + @Test + public void postRequestWithPartiallyOverlappingQueryStringAndParameters() + throws IOException { + this.snippet + .expectCurlRequest( + "post-request-with-partially-overlapping-query-string-and-parameters") .withContents(codeBlock("bash").content( "$ curl 'http://localhost/foo?a=alpha' -i -X POST -d 'b=bravo'")); new CurlRequestSnippet().document(operationBuilder( - "post-request-with-overlapping-query-string-and-parameters") + "post-request-with-partially-overlapping-query-string-and-parameters") .request("http://localhost/foo?a=alpha").method("POST") .param("a", "alpha").param("b", "bravo").build()); } diff --git a/spring-restdocs-core/src/test/java/org/springframework/restdocs/cli/HttpieRequestSnippetTests.java b/spring-restdocs-core/src/test/java/org/springframework/restdocs/cli/HttpieRequestSnippetTests.java index 9f8164b3..9dd297c6 100644 --- a/spring-restdocs-core/src/test/java/org/springframework/restdocs/cli/HttpieRequestSnippetTests.java +++ b/spring-restdocs-core/src/test/java/org/springframework/restdocs/cli/HttpieRequestSnippetTests.java @@ -96,6 +96,47 @@ public class HttpieRequestSnippetTests extends AbstractSnippetTests { .request("http://localhost/foo?param=value").build()); } + @Test + public void getRequestWithTotallyOverlappingQueryStringAndParameters() + throws IOException { + this.snippet + .expectHttpieRequest( + "request-with-totally-overlapping-query-string-and-parameters") + .withContents(codeBlock("bash") + .content("$ http GET 'http://localhost/foo?param=value'")); + new HttpieRequestSnippet().document(operationBuilder( + "request-with-totally-overlapping-query-string-and-parameters") + .request("http://localhost/foo?param=value") + .param("param", "value").build()); + } + + @Test + public void getRequestWithPartiallyOverlappingQueryStringAndParameters() + throws IOException { + this.snippet + .expectHttpieRequest( + "request-with-partially-overlapping-query-string-and-parameters") + .withContents(codeBlock("bash") + .content("$ http GET 'http://localhost/foo?a=alpha&b=bravo'")); + new HttpieRequestSnippet().document(operationBuilder( + "request-with-partially-overlapping-query-string-and-parameters") + .request("http://localhost/foo?a=alpha").param("a", "alpha") + .param("b", "bravo").build()); + } + + @Test + public void getRequestWithDisjointQueryStringAndParameters() throws IOException { + this.snippet + .expectHttpieRequest( + "request-with-partially-overlapping-query-string-and-parameters") + .withContents(codeBlock("bash") + .content("$ http GET 'http://localhost/foo?a=alpha&b=bravo'")); + new HttpieRequestSnippet().document(operationBuilder( + "request-with-partially-overlapping-query-string-and-parameters") + .request("http://localhost/foo?a=alpha").param("b", "bravo") + .build()); + } + @Test public void getRequestWithQueryStringWithNoValue() throws IOException { this.snippet.expectHttpieRequest("request-with-query-string-with-no-value") @@ -173,25 +214,42 @@ public class HttpieRequestSnippetTests extends AbstractSnippetTests { } @Test - public void postRequestWithQueryStringAndParameter() throws IOException { - this.snippet.expectHttpieRequest("post-request-with-query-string-and-parameter") + public void postRequestWithDisjointQueryStringAndParameter() throws IOException { + this.snippet + .expectHttpieRequest( + "post-request-with-disjoint-query-string-and-parameter") .withContents(codeBlock("bash").content( "$ http --form POST 'http://localhost/foo?a=alpha' 'b=bravo'")); - new HttpieRequestSnippet() - .document(operationBuilder("post-request-with-query-string-and-parameter") + new HttpieRequestSnippet().document( + operationBuilder("post-request-with-disjoint-query-string-and-parameter") .request("http://localhost/foo?a=alpha").method("POST") .param("b", "bravo").build()); } @Test - public void postRequestWithOverlappingQueryStringAndParameters() throws IOException { + public void postRequestWithTotallyOverlappingQueryStringAndParameters() + throws IOException { this.snippet .expectHttpieRequest( - "post-request-with-overlapping-query-string-and-parameters") + "post-request-with-totally-overlapping-query-string-and-parameters") + .withContents(codeBlock("bash") + .content("$ http POST 'http://localhost/foo?a=alpha&b=bravo'")); + new HttpieRequestSnippet().document(operationBuilder( + "post-request-with-totally-overlapping-query-string-and-parameters") + .request("http://localhost/foo?a=alpha&b=bravo").method("POST") + .param("a", "alpha").param("b", "bravo").build()); + } + + @Test + public void postRequestWithPartiallyOverlappingQueryStringAndParameters() + throws IOException { + this.snippet + .expectHttpieRequest( + "post-request-with-partially-overlapping-query-string-and-parameters") .withContents(codeBlock("bash").content( "$ http --form POST 'http://localhost/foo?a=alpha' 'b=bravo'")); new HttpieRequestSnippet().document(operationBuilder( - "post-request-with-overlapping-query-string-and-parameters") + "post-request-with-partially-overlapping-query-string-and-parameters") .request("http://localhost/foo?a=alpha").method("POST") .param("a", "alpha").param("b", "bravo").build()); } diff --git a/spring-restdocs-core/src/test/java/org/springframework/restdocs/http/HttpRequestSnippetTests.java b/spring-restdocs-core/src/test/java/org/springframework/restdocs/http/HttpRequestSnippetTests.java index 6f7057b3..e66e6b5b 100644 --- a/spring-restdocs-core/src/test/java/org/springframework/restdocs/http/HttpRequestSnippetTests.java +++ b/spring-restdocs-core/src/test/java/org/springframework/restdocs/http/HttpRequestSnippetTests.java @@ -102,6 +102,34 @@ public class HttpRequestSnippetTests extends AbstractSnippetTests { .request("http://localhost/foo?bar").build()); } + @Test + public void getWithPartiallyOverlappingQueryStringAndParameters() throws IOException { + this.snippet + .expectHttpRequest( + "get-with-partially-overlapping-query-string-and-parameters") + .withContents(httpRequest(RequestMethod.GET, "/foo?a=alpha&b=bravo") + .header(HttpHeaders.HOST, "localhost")); + + new HttpRequestSnippet().document(operationBuilder( + "get-with-partially-overlapping-query-string-and-parameters") + .request("http://localhost/foo?a=alpha").param("a", "alpha") + .param("b", "bravo").build()); + } + + @Test + public void getWithTotallyOverlappingQueryStringAndParameters() throws IOException { + this.snippet + .expectHttpRequest( + "get-with-totally-overlapping-query-string-and-parameters") + .withContents(httpRequest(RequestMethod.GET, "/foo?a=alpha&b=bravo") + .header(HttpHeaders.HOST, "localhost")); + + new HttpRequestSnippet().document(operationBuilder( + "get-with-totally-overlapping-query-string-and-parameters") + .request("http://localhost/foo?a=alpha&b=bravo") + .param("a", "alpha").param("b", "bravo").build()); + } + @Test public void postRequestWithContent() throws IOException { String content = "Hello, world"; @@ -128,6 +156,59 @@ public class HttpRequestSnippetTests extends AbstractSnippetTests { .param("a", "alpha").content(content).build()); } + @Test + public void postRequestWithContentAndDisjointQueryStringAndParameters() + throws IOException { + String content = "Hello, world"; + this.snippet + .expectHttpRequest( + "post-request-with-content-and-disjoint-query-string-and-parameters") + .withContents(httpRequest(RequestMethod.POST, "/foo?b=bravo&a=alpha") + .header(HttpHeaders.HOST, "localhost").content(content) + .header(HttpHeaders.CONTENT_LENGTH, content.getBytes().length)); + + new HttpRequestSnippet().document(operationBuilder( + "post-request-with-content-and-disjoint-query-string-and-parameters") + .request("http://localhost/foo?b=bravo").method("POST") + .param("a", "alpha").content(content).build()); + } + + @Test + public void postRequestWithContentAndPartiallyOverlappingQueryStringAndParameters() + throws IOException { + String content = "Hello, world"; + this.snippet + .expectHttpRequest( + "post-request-with-content-and-partially-overlapping-query-string-and-parameters") + .withContents(httpRequest(RequestMethod.POST, "/foo?b=bravo&a=alpha") + .header(HttpHeaders.HOST, "localhost").content(content) + .header(HttpHeaders.CONTENT_LENGTH, content.getBytes().length)); + + new HttpRequestSnippet().document(operationBuilder( + "post-request-with-content-and-partially-overlapping-query-string-and-parameters") + .request("http://localhost/foo?b=bravo").method("POST") + .param("a", "alpha").param("b", "bravo").content(content) + .build()); + } + + @Test + public void postRequestWithContentAndTotallyOverlappingQueryStringAndParameters() + throws IOException { + String content = "Hello, world"; + this.snippet + .expectHttpRequest( + "post-request-with-content-and-totally-overlapping-query-string-and-parameters") + .withContents(httpRequest(RequestMethod.POST, "/foo?b=bravo&a=alpha") + .header(HttpHeaders.HOST, "localhost").content(content) + .header(HttpHeaders.CONTENT_LENGTH, content.getBytes().length)); + + new HttpRequestSnippet().document(operationBuilder( + "post-request-with-content-and-totally-overlapping-query-string-and-parameters") + .request("http://localhost/foo?b=bravo&a=alpha").method("POST") + .param("a", "alpha").param("b", "bravo").content(content) + .build()); + } + @Test public void postRequestWithCharset() throws IOException { String japaneseContent = "\u30b3\u30f3\u30c6\u30f3\u30c4";