From 1852802502d07d9815cd40272e9db588e046308f Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Tue, 16 Jul 2013 16:08:55 +0200 Subject: [PATCH] SONAR-4506 Java WS Client - do not use URL query parameters for POST requests --- .../wsclient/internal/HttpRequestFactory.java | 10 ++-- .../org/sonar/wsclient/MockHttpServer.java | 27 +++++---- .../wsclient/MockHttpServerInterceptor.java | 4 ++ .../internal/DefaultActionPlanClientTest.java | 37 ++++++++++-- .../internal/DefaultIssueClientTest.java | 60 +++++++++++++++---- .../DefaultPermissionClientTest.java | 38 +++++++----- .../wsclient/user/DefaultUserClientTest.java | 27 ++++++--- 7 files changed, 148 insertions(+), 55 deletions(-) diff --git a/sonar-ws-client/src/main/java/org/sonar/wsclient/internal/HttpRequestFactory.java b/sonar-ws-client/src/main/java/org/sonar/wsclient/internal/HttpRequestFactory.java index 19b604884c0..184559fcde4 100644 --- a/sonar-ws-client/src/main/java/org/sonar/wsclient/internal/HttpRequestFactory.java +++ b/sonar-ws-client/src/main/java/org/sonar/wsclient/internal/HttpRequestFactory.java @@ -117,20 +117,19 @@ public class HttpRequestFactory { } public String get(String wsUrl, Map queryParams) { - HttpRequest request = HttpRequest.get(baseUrl + wsUrl, queryParams, true); + HttpRequest request = prepare(HttpRequest.get(baseUrl + wsUrl, queryParams, true)); return execute(request); } public String post(String wsUrl, Map queryParams) { - HttpRequest request = HttpRequest.post(baseUrl + wsUrl, queryParams, true); + HttpRequest request = prepare(HttpRequest.post(baseUrl + wsUrl, true)).form(queryParams, HttpRequest.CHARSET_UTF8); return execute(request); } private String execute(HttpRequest request) { try { - prepare(request); if (request.ok()) { - return request.body("UTF-8"); + return request.body(HttpRequest.CHARSET_UTF8); } // TODO handle error messages throw new HttpException(request.url().toString(), request.code()); @@ -140,7 +139,7 @@ public class HttpRequestFactory { } } - private void prepare(HttpRequest request) { + private HttpRequest prepare(HttpRequest request) { if (proxyHost != null) { request.useProxy(proxyHost, proxyPort); if (proxyLogin != null) { @@ -159,5 +158,6 @@ public class HttpRequestFactory { if (login != null) { request.basic(login, password); } + return request; } } diff --git a/sonar-ws-client/src/test/java/org/sonar/wsclient/MockHttpServer.java b/sonar-ws-client/src/test/java/org/sonar/wsclient/MockHttpServer.java index 0c2ab225b78..b66b3e0014c 100644 --- a/sonar-ws-client/src/test/java/org/sonar/wsclient/MockHttpServer.java +++ b/sonar-ws-client/src/test/java/org/sonar/wsclient/MockHttpServer.java @@ -19,7 +19,6 @@ */ package org.sonar.wsclient; -import org.apache.commons.io.IOUtils; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.Server; @@ -28,7 +27,6 @@ import org.eclipse.jetty.server.handler.AbstractHandler; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; - import java.io.IOException; import java.util.Enumeration; import java.util.HashMap; @@ -41,8 +39,8 @@ public class MockHttpServer { private Server server; private String responseBody; private int responseStatus = SC_OK; - private String requestBody, requestPath; - private Map requestHeaders = new HashMap(); + private String requestPath; + private Map requestHeaders = new HashMap(), requestParams = new HashMap(); public void start() throws Exception { // 0 is random available port @@ -56,13 +54,18 @@ public class MockHttpServer { @Override public void handle(String target, Request baseRequest, HttpServletRequest httpServletRequest, HttpServletResponse response) throws IOException, ServletException { requestPath = baseRequest.getUri().toString(); - requestBody = IOUtils.toString(baseRequest.getInputStream()); requestHeaders.clear(); - Enumeration headerNames = baseRequest.getHeaderNames(); - while (headerNames.hasMoreElements()) { - String headerName = (String)headerNames.nextElement(); + Enumeration names = baseRequest.getHeaderNames(); + while (names.hasMoreElements()) { + String headerName = (String)names.nextElement(); requestHeaders.put(headerName, baseRequest.getHeader(headerName)); } + requestParams.clear(); + names = baseRequest.getParameterNames(); + while (names.hasMoreElements()) { + String headerName = (String)names.nextElement(); + requestParams.put(headerName, baseRequest.getParameter(headerName)); + } response.setStatus(responseStatus); response.setContentType("application/json;charset=utf-8"); write(responseBody, response.getOutputStream()); @@ -92,10 +95,6 @@ public class MockHttpServer { return this; } - public String requestBody() { - return requestBody; - } - public String requestPath() { return requestPath; } @@ -104,6 +103,10 @@ public class MockHttpServer { return requestHeaders; } + public Map requestParams() { + return requestParams; + } + public int getPort() { return server.getConnectors()[0].getLocalPort(); } diff --git a/sonar-ws-client/src/test/java/org/sonar/wsclient/MockHttpServerInterceptor.java b/sonar-ws-client/src/test/java/org/sonar/wsclient/MockHttpServerInterceptor.java index 01d700d8c70..5e870c599e1 100644 --- a/sonar-ws-client/src/test/java/org/sonar/wsclient/MockHttpServerInterceptor.java +++ b/sonar-ws-client/src/test/java/org/sonar/wsclient/MockHttpServerInterceptor.java @@ -57,6 +57,10 @@ public final class MockHttpServerInterceptor extends ExternalResource { return server.requestHeaders(); } + public Map requestParams() { + return server.requestParams(); + } + public int port() { return server.getPort(); } diff --git a/sonar-ws-client/src/test/java/org/sonar/wsclient/issue/internal/DefaultActionPlanClientTest.java b/sonar-ws-client/src/test/java/org/sonar/wsclient/issue/internal/DefaultActionPlanClientTest.java index 3e51e4245d5..78a5959f328 100644 --- a/sonar-ws-client/src/test/java/org/sonar/wsclient/issue/internal/DefaultActionPlanClientTest.java +++ b/sonar-ws-client/src/test/java/org/sonar/wsclient/issue/internal/DefaultActionPlanClientTest.java @@ -37,6 +37,7 @@ import java.util.List; import static org.fest.assertions.Assertions.assertThat; import static org.fest.assertions.Fail.fail; +import static org.fest.assertions.MapAssert.entry; public class DefaultActionPlanClientTest { @@ -86,7 +87,13 @@ public class DefaultActionPlanClientTest { ActionPlan result = client.create( NewActionPlan.create().name("Short term").project("org.sonar.Sample").description("Short term issues").deadLine(stringToDate("2014-01-01"))); - assertThat(httpServer.requestedPath()).isEqualTo("/api/action_plans/create?project=org.sonar.Sample&description=Short%20term%20issues&name=Short%20term&deadLine=2014-01-01"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/action_plans/create"); + assertThat(httpServer.requestParams()).includes( + entry("project", "org.sonar.Sample"), + entry("description", "Short term issues"), + entry("name", "Short term"), + entry("deadLine", "2014-01-01") + ); assertThat(result).isNotNull(); } @@ -99,7 +106,13 @@ public class DefaultActionPlanClientTest { ActionPlan result = client.update( UpdateActionPlan.create().key("382f6f2e-ad9d-424a-b973-9b065e04348a").name("Short term").description("Short term issues").deadLine(stringToDate("2014-01-01"))); - assertThat(httpServer.requestedPath()).isEqualTo("/api/action_plans/update?description=Short%20term%20issues&name=Short%20term&deadLine=2014-01-01&key=382f6f2e-ad9d-424a-b973-9b065e04348a"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/action_plans/update"); + assertThat(httpServer.requestParams()).includes( + entry("key", "382f6f2e-ad9d-424a-b973-9b065e04348a"), + entry("description", "Short term issues"), + entry("name", "Short term"), + entry("deadLine", "2014-01-01") + ); assertThat(result).isNotNull(); } @@ -110,7 +123,10 @@ public class DefaultActionPlanClientTest { ActionPlanClient client = new DefaultActionPlanClient(requestFactory); client.delete("382f6f2e-ad9d-424a-b973-9b065e04348a"); - assertThat(httpServer.requestedPath()).isEqualTo("/api/action_plans/delete?key=382f6f2e-ad9d-424a-b973-9b065e04348a"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/action_plans/delete"); + assertThat(httpServer.requestParams()).includes( + entry("key", "382f6f2e-ad9d-424a-b973-9b065e04348a") + ); } @Test @@ -125,7 +141,10 @@ public class DefaultActionPlanClientTest { } catch (HttpException e) { assertThat(e.status()).isEqualTo(500); assertThat(e.url()).startsWith("http://localhost"); - assertThat(e.url()).endsWith("/api/action_plans/delete?key=382f6f2e-ad9d-424a-b973-9b065e04348a"); + assertThat(e.url()).endsWith("/api/action_plans/delete"); + assertThat(httpServer.requestParams()).includes( + entry("key", "382f6f2e-ad9d-424a-b973-9b065e04348a") + ); } } @@ -137,7 +156,10 @@ public class DefaultActionPlanClientTest { ActionPlanClient client = new DefaultActionPlanClient(requestFactory); ActionPlan result = client.open("382f6f2e-ad9d-424a-b973-9b065e04348a"); - assertThat(httpServer.requestedPath()).isEqualTo("/api/action_plans/open?key=382f6f2e-ad9d-424a-b973-9b065e04348a"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/action_plans/open"); + assertThat(httpServer.requestParams()).includes( + entry("key", "382f6f2e-ad9d-424a-b973-9b065e04348a") + ); assertThat(result).isNotNull(); } @@ -149,7 +171,10 @@ public class DefaultActionPlanClientTest { ActionPlanClient client = new DefaultActionPlanClient(requestFactory); ActionPlan result = client.close("382f6f2e-ad9d-424a-b973-9b065e04348a"); - assertThat(httpServer.requestedPath()).isEqualTo("/api/action_plans/close?key=382f6f2e-ad9d-424a-b973-9b065e04348a"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/action_plans/close"); + assertThat(httpServer.requestParams()).includes( + entry("key", "382f6f2e-ad9d-424a-b973-9b065e04348a") + ); assertThat(result).isNotNull(); } diff --git a/sonar-ws-client/src/test/java/org/sonar/wsclient/issue/internal/DefaultIssueClientTest.java b/sonar-ws-client/src/test/java/org/sonar/wsclient/issue/internal/DefaultIssueClientTest.java index 9befff8294f..96140bd593a 100644 --- a/sonar-ws-client/src/test/java/org/sonar/wsclient/issue/internal/DefaultIssueClientTest.java +++ b/sonar-ws-client/src/test/java/org/sonar/wsclient/issue/internal/DefaultIssueClientTest.java @@ -31,6 +31,7 @@ import java.util.List; import static org.fest.assertions.Assertions.assertThat; import static org.fest.assertions.Fail.fail; +import static org.fest.assertions.MapAssert.entry; public class DefaultIssueClientTest { @Rule @@ -74,7 +75,11 @@ public class DefaultIssueClientTest { IssueClient client = new DefaultIssueClient(requestFactory); Issue result = client.setSeverity("ABCDE", "BLOCKER"); - assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/set_severity?issue=ABCDE&severity=BLOCKER"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/set_severity"); + assertThat(httpServer.requestParams()).includes( + entry("issue", "ABCDE"), + entry("severity", "BLOCKER") + ); assertThat(result).isNotNull(); } @@ -86,7 +91,11 @@ public class DefaultIssueClientTest { IssueClient client = new DefaultIssueClient(requestFactory); Issue result = client.assign("ABCDE", "emmerik"); - assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/assign?issue=ABCDE&assignee=emmerik"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/assign"); + assertThat(httpServer.requestParams()).includes( + entry("issue", "ABCDE"), + entry("assignee", "emmerik") + ); assertThat(result).isNotNull(); } @@ -98,7 +107,10 @@ public class DefaultIssueClientTest { IssueClient client = new DefaultIssueClient(requestFactory); Issue result = client.assign("ABCDE", null); - assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/assign?issue=ABCDE"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/assign"); + assertThat(httpServer.requestParams()).includes( + entry("issue", "ABCDE") + ); assertThat(result).isNotNull(); } @@ -110,7 +122,11 @@ public class DefaultIssueClientTest { IssueClient client = new DefaultIssueClient(requestFactory); Issue result = client.plan("ABCDE", "DEFGH"); - assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/plan?issue=ABCDE&plan=DEFGH"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/plan"); + assertThat(httpServer.requestParams()).includes( + entry("issue", "ABCDE"), + entry("plan", "DEFGH") + ); assertThat(result).isNotNull(); } @@ -122,7 +138,10 @@ public class DefaultIssueClientTest { IssueClient client = new DefaultIssueClient(requestFactory); Issue result = client.plan("ABCDE", null); - assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/plan?issue=ABCDE"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/plan"); + assertThat(httpServer.requestParams()).includes( + entry("issue", "ABCDE") + ); assertThat(result).isNotNull(); } @@ -134,7 +153,11 @@ public class DefaultIssueClientTest { IssueClient client = new DefaultIssueClient(requestFactory); Issue result = client.create(NewIssue.create().component("Action.java").rule("squid:AvoidCycle")); - assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/create?component=Action.java&rule=squid:AvoidCycle"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/create"); + assertThat(httpServer.requestParams()).includes( + entry("component", "Action.java"), + entry("rule", "squid:AvoidCycle") + ); assertThat(result).isNotNull(); } @@ -164,7 +187,11 @@ public class DefaultIssueClientTest { IssueClient client = new DefaultIssueClient(requestFactory); Issue result = client.doTransition("ABCDE", "resolve"); - assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/do_transition?issue=ABCDE&transition=resolve"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/do_transition"); + assertThat(httpServer.requestParams()).includes( + entry("issue", "ABCDE"), + entry("transition", "resolve") + ); assertThat(result).isNotNull(); } @@ -176,7 +203,11 @@ public class DefaultIssueClientTest { IssueClient client = new DefaultIssueClient(requestFactory); IssueComment comment = client.addComment("ISSUE-1", "this is my comment"); - assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/add_comment?issue=ISSUE-1&text=this%20is%20my%20comment"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/add_comment"); + assertThat(httpServer.requestParams()).includes( + entry("issue", "ISSUE-1"), + entry("text", "this is my comment") + ); assertThat(comment).isNotNull(); assertThat(comment.key()).isEqualTo("COMMENT-123"); assertThat(comment.htmlText()).isEqualTo("this is my comment"); @@ -210,7 +241,11 @@ public class DefaultIssueClientTest { IssueClient client = new DefaultIssueClient(requestFactory); Issue result = client.doAction("ABCDE", "tweet"); - assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/do_action?issue=ABCDE&actionKey=tweet"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/do_action"); + assertThat(httpServer.requestParams()).includes( + entry("issue", "ABCDE"), + entry("actionKey", "tweet") + ); assertThat(result).isNotNull(); } @@ -227,7 +262,12 @@ public class DefaultIssueClientTest { IssueClient client = new DefaultIssueClient(requestFactory); BulkChange result = client.bulkChange(query); - assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/bulk_change?assign.assignee=geoffrey&issues=ABCD,EFGH&actions=assign"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/issues/bulk_change"); + assertThat(httpServer.requestParams()).includes( + entry("assign.assignee", "geoffrey"), + entry("issues", "ABCD,EFGH"), + entry("actions", "assign") + ); assertThat(result).isNotNull(); } } diff --git a/sonar-ws-client/src/test/java/org/sonar/wsclient/permissions/DefaultPermissionClientTest.java b/sonar-ws-client/src/test/java/org/sonar/wsclient/permissions/DefaultPermissionClientTest.java index c850665b145..b3a6a572f72 100644 --- a/sonar-ws-client/src/test/java/org/sonar/wsclient/permissions/DefaultPermissionClientTest.java +++ b/sonar-ws-client/src/test/java/org/sonar/wsclient/permissions/DefaultPermissionClientTest.java @@ -26,15 +26,13 @@ import org.junit.Test; import org.sonar.wsclient.MockHttpServerInterceptor; import org.sonar.wsclient.internal.HttpRequestFactory; -import java.util.Arrays; -import java.util.List; - import static org.fest.assertions.Assertions.assertThat; +import static org.fest.assertions.MapAssert.entry; public class DefaultPermissionClientTest { - private HttpRequestFactory requestFactory; - private DefaultPermissionClient client; + HttpRequestFactory requestFactory; + DefaultPermissionClient client; @Rule public MockHttpServerInterceptor httpServer = new MockHttpServerInterceptor(); @@ -52,7 +50,11 @@ public class DefaultPermissionClientTest { PermissionParameters params = PermissionParameters.create().user("daveloper").permission("admin"); client.addPermission(params); - assertThatRequestUrlContains("/api/permissions/add?", "user=daveloper", "permission=admin"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/permissions/add"); + assertThat(httpServer.requestParams()).includes( + entry("user", "daveloper"), + entry("permission", "admin") + ); } @Test @@ -62,7 +64,11 @@ public class DefaultPermissionClientTest { PermissionParameters params = PermissionParameters.create().group("my_group").permission("admin"); client.addPermission(params); - assertThatRequestUrlContains("/api/permissions/add?", "group=my_group", "permission=admin"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/permissions/add"); + assertThat(httpServer.requestParams()).includes( + entry("group", "my_group"), + entry("permission", "admin") + ); } @Test @@ -72,7 +78,11 @@ public class DefaultPermissionClientTest { PermissionParameters params = PermissionParameters.create().user("daveloper").permission("admin"); client.removePermission(params); - assertThatRequestUrlContains("/api/permissions/remove?", "user=daveloper", "permission=admin"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/permissions/remove"); + assertThat(httpServer.requestParams()).includes( + entry("user", "daveloper"), + entry("permission", "admin") + ); } @Test @@ -82,12 +92,10 @@ public class DefaultPermissionClientTest { PermissionParameters params = PermissionParameters.create().group("my_group").permission("admin"); client.removePermission(params); - assertThatRequestUrlContains("/api/permissions/remove?", "group=my_group", "permission=admin"); - } - - private void assertThatRequestUrlContains(String baseUrl, String... parameters) { - assertThat(httpServer.requestedPath()).startsWith(baseUrl); - List requestParameters = Arrays.asList(httpServer.requestedPath().substring(baseUrl.length()).split("&")); - assertThat(requestParameters).containsOnly(parameters); + assertThat(httpServer.requestedPath()).isEqualTo("/api/permissions/remove"); + assertThat(httpServer.requestParams()).includes( + entry("group", "my_group"), + entry("permission", "admin") + ); } } diff --git a/sonar-ws-client/src/test/java/org/sonar/wsclient/user/DefaultUserClientTest.java b/sonar-ws-client/src/test/java/org/sonar/wsclient/user/DefaultUserClientTest.java index 90e6865f28b..7056a2d32e7 100644 --- a/sonar-ws-client/src/test/java/org/sonar/wsclient/user/DefaultUserClientTest.java +++ b/sonar-ws-client/src/test/java/org/sonar/wsclient/user/DefaultUserClientTest.java @@ -29,11 +29,12 @@ import java.util.Arrays; import java.util.List; import static org.fest.assertions.Assertions.assertThat; +import static org.fest.assertions.MapAssert.entry; public class DefaultUserClientTest { - private HttpRequestFactory requestFactory; - private DefaultUserClient client; + HttpRequestFactory requestFactory; + DefaultUserClient client; @Rule public MockHttpServerInterceptor httpServer = new MockHttpServerInterceptor(); @@ -51,7 +52,7 @@ public class DefaultUserClientTest { UserQuery query = UserQuery.create().logins("simon", "loic"); List users = client.find(query); - assertThatRequestUrlContains("/api/users/search?", "logins=simon,loic"); + assertThatGetRequestUrlContains("/api/users/search?", "logins=simon,loic"); assertThat(users).hasSize(1); User simon = users.get(0); assertThat(simon.login()).isEqualTo("simon"); @@ -67,7 +68,12 @@ public class DefaultUserClientTest { UserParameters params = UserParameters.create().login("daveloper").password("pass1").passwordConfirmation("pass1"); User createdUser = client.create(params); - assertThatRequestUrlContains("/api/users/create?", "login=daveloper", "password=pass1", "password_confirmation=pass1"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/users/create"); + assertThat(httpServer.requestParams()).includes( + entry("login", "daveloper"), + entry("password", "pass1"), + entry("password_confirmation", "pass1") + ); assertThat(createdUser).isNotNull(); assertThat(createdUser.login()).isEqualTo("daveloper"); assertThat(createdUser.name()).isEqualTo("daveloper"); @@ -81,7 +87,11 @@ public class DefaultUserClientTest { UserParameters params = UserParameters.create().login("daveloper").email("new_email"); User updatedUser = client.update(params); - assertThatRequestUrlContains("/api/users/update?", "login=daveloper", "email=new_email"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/users/update"); + assertThat(httpServer.requestParams()).includes( + entry("login", "daveloper"), + entry("email", "new_email") + ); assertThat(updatedUser).isNotNull(); assertThat(updatedUser.login()).isEqualTo("daveloper"); assertThat(updatedUser.name()).isEqualTo("daveloper"); @@ -94,10 +104,13 @@ public class DefaultUserClientTest { client.deactivate("daveloper"); - assertThatRequestUrlContains("/api/users/deactivate?", "login=daveloper"); + assertThat(httpServer.requestedPath()).isEqualTo("/api/users/deactivate"); + assertThat(httpServer.requestParams()).includes( + entry("login", "daveloper") + ); } - private void assertThatRequestUrlContains(String baseUrl, String... parameters) { + private void assertThatGetRequestUrlContains(String baseUrl, String... parameters) { assertThat(httpServer.requestedPath()).startsWith(baseUrl); List requestParameters = Arrays.asList(httpServer.requestedPath().substring(baseUrl.length()).split("&")); assertThat(requestParameters).containsOnly(parameters); -- 2.39.5