Fix for bug in Ribbon-Module, where query strings are not properly encoded

julian.duniec · julian.duniec · commit 9a7e84e158bd · 2014-01-30T14:38:24.000+01:00
diff --git a/core/src/main/java/feign/RequestTemplate.java b/core/src/main/java/feign/RequestTemplate.java
@@ -463,13 +463,38 @@ private StringBuilder pullAnyQueriesOutOfUrl(StringBuilder url) {
         firstQueries.putAll(queries);
         queries.clear();
       }
-      queries.putAll(firstQueries);
+      //Since we decode all queries, we want to use the
+	  //query()-method to re-add them to ensure that all
+	  //logic (such as url-encoding) are executed, giving
+	  //a valid queryLine()
+      for(String key : firstQueries.keySet()) {
+		  Collection<String> values = firstQueries.get(key);
+		  if(allValuesAreNull(values)) {
+			  //Queryies where all values are null will
+			  //be ignored by the query(key, value)-method
+			  //So we manually avoid this case here, to ensure that
+			  //we still fulfill the contract (ex. parameters without values)
+			  queries.put(urlEncode(key), values);
+		  }
+		  else {
+			  query(key, values);
+		  }
+
+	  }
       return new StringBuilder(url.substring(0, queryIndex));
     }
     return url;
   }
 
-  private static Map<String, Collection<String>> parseAndDecodeQueries(String queryLine) {
+	private boolean allValuesAreNull(Collection<String> values) {
+		if(values.isEmpty()) return true;
+		for(String val : values) {
+			if(val != null) return false;
+		}
+		return true;
+	}
+
+	private static Map<String, Collection<String>> parseAndDecodeQueries(String queryLine) {
     Map<String, Collection<String>> map = new LinkedHashMap<String, Collection<String>>();
     if (emptyToNull(queryLine) == null)
       return map;
diff --git a/ribbon/src/test/java/feign/ribbon/RibbonClientTest.java b/ribbon/src/test/java/feign/ribbon/RibbonClientTest.java
@@ -32,10 +32,13 @@
 import static feign.Util.UTF_8;
 import static org.testng.Assert.assertEquals;
 
+import javax.inject.Named;
+
 @Test
 public class RibbonClientTest {
   interface TestInterface {
     @RequestLine("POST /") void post();
+	@RequestLine("GET /?a={a}") void getWithQueryParameters(@Named("a") String a);
 
     @dagger.Module(injects = Feign.class, overrides = true, addsTo = Feign.Defaults.class)
     static class Module {
@@ -108,6 +111,43 @@ public void ioExceptionRetry() throws IOException, InterruptedException {
     }
   }
 
+	/*
+		This test-case replicates a bug that occurs when using RibbonRequest with a query string.
+
+		The querystrings would not be URL-encoded, leading to invalid HTTP-requests if the query string contained
+		invalid characters (ex. space).
+	 */
+	@Test public void urlEncodeQueryStringParameters () throws IOException, InterruptedException {
+		String client = "RibbonClientTest-urlEncodeQueryStringParameters";
+		String serverListKey = client + ".ribbon.listOfServers";
+
+		String queryStringValue = "some string with space";
+		String expectedQueryStringValue = "some+string+with+space";
+		String expectedRequestLine = String.format("GET /?a=%s HTTP/1.1", expectedQueryStringValue);
+
+		MockWebServer server = new MockWebServer();
+		server.enqueue(new MockResponse().setBody("success!".getBytes(UTF_8)));
+		server.play();
+
+		getConfigInstance().setProperty(serverListKey, hostAndPort(server.getUrl("")));
+
+		try {
+
+			TestInterface api = Feign.create(TestInterface.class, "http://" + client, new TestInterface.Module(), new RibbonModule());
+
+			api.getWithQueryParameters(queryStringValue);
+
+			final String recordedRequestLine = server.takeRequest().getRequestLine();
+
+			assertEquals(recordedRequestLine, expectedRequestLine);
+		} finally {
+			server.shutdown();
+			getConfigInstance().clearProperty(serverListKey);
+		}
+	}
+
+
+
   static String hostAndPort(URL url) {
     // our build slaves have underscores in their hostnames which aren't permitted by ribbon
     return "localhost:" + url.getPort();
