Repository: square/retrofit
Issue: Path Params Are Incorrectly Encoded As Query Params

``` java
@GET("/foo/{bar}/")
Response a(@Name("bar") String bar)
```

``` java
a("{baz}");
```

is incorrectly encoded to `/foo/%7Bpbaz%7D/` instead of `/foo/{baz}/`.


Issue comments:
I was going to take a stab at this, but according to http://stackoverflow.com/questions/4669692/valid-characters-for-directory-part-of-a-url-for-short-links and the referenced RFCs, this appears to be appropriate behavior for a URL path. Maybe there is something I'm missing, but it appears that { and } require percent escaping.

The percent escaping is insignificant for the { character. But it is important that we get the behavior we want for the `?`, `&` and `#` characters, which have very different semantics depending on whether they're escaped or not.

I wrote some failing test cases and results [here](https://gist.github.com/campnic/5024459). 

It appears that the path parameters are being escaped correctly but that query parameters are not. If I could get confirmation on that, I'll get a patch pulled together.


Pull request review comments:
You can just `return` here and drop the first and last line.

All the changes in this file need to be two-space indentation instead of four.

Sorry, do you guys have a format template for eclipse? I'm fighting my own
template to try to keep things styled like the rest of the project.
On Feb 24, 2013 4:31 PM, "Jake Wharton" notifications@github.com wrote:

> In retrofit/src/test/java/retrofit/http/RequestBuilderTest.java:
> 
> > @@ -76,6 +76,48 @@
> >      assertThat(request.getUrl()).isEqualTo("http://example.com/foo/bar/pong/?kit=kat&riff=raff");
> >      assertThat(request.getBody()).isNull();
> >    }
> > +
> > -  @Test public void getWithPathAndQueryQuestionMarkParam() throws Exception {
> > -      Request request = new Helper() //
> 
> Needs to be two-space indentation instead of four.
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/square/retrofit/pull/158/files#r3129647.

No, sorry. None of us use Eclipse. I realize it can be a challenge since there are wildly different styles. I can fix it after merging, if you'd like.

Naw, I'll fix it. Better to have it be correct so i am reminded in the future.


Patch that resolved the issue:
diff --git a/retrofit/src/main/java/retrofit/http/RequestBuilder.java b/retrofit/src/main/java/retrofit/http/RequestBuilder.java
index 2fe5394137..d73adcdd8b 100644
--- a/retrofit/src/main/java/retrofit/http/RequestBuilder.java
+++ b/retrofit/src/main/java/retrofit/http/RequestBuilder.java
@@ -88,12 +88,7 @@ Request build() {
         }
       }
       if (found != null) {
-        String value;
-        try {
-          value = URLEncoder.encode(String.valueOf(found.getValue()), UTF_8);
-        } catch (UnsupportedEncodingException e) {
-          throw new AssertionError(e);
-        }
+        String value = getUrlEncodedValue(found);
         replacedPath = replacedPath.replace("{" + found.getName() + "}", value);
         paramList.remove(found);
       } else {
@@ -127,7 +122,8 @@ Request build() {
       for (int i = 0, count = paramList.size(); i < count; i++) {
         url.append((i == 0) ? '?' : '&');
         Parameter nonPathParam = paramList.get(i);
-        url.append(nonPathParam.getName()).append("=").append(nonPathParam.getValue());
+        String value = getUrlEncodedValue(nonPathParam);
+        url.append(nonPathParam.getName()).append("=").append(value);
       }
     } else if (!paramList.isEmpty()) {
       if (methodInfo.isMultipart) {
@@ -157,4 +153,12 @@ Request build() {
 
     return new Request(methodInfo.restMethod.value(), url.toString(), headers, body);
   }
+
+  private static String getUrlEncodedValue(Parameter found) {
+    try {
+      return URLEncoder.encode(String.valueOf(found.getValue()), UTF_8);
+    } catch (UnsupportedEncodingException e) {
+      throw new AssertionError(e);
+    }
+  }
 }
\ No newline at end of file


Test patch:
diff --git a/retrofit/src/test/java/retrofit/http/RequestBuilderTest.java b/retrofit/src/test/java/retrofit/http/RequestBuilderTest.java
index 7615893a2d..7de05d4f12 100644
--- a/retrofit/src/test/java/retrofit/http/RequestBuilderTest.java
+++ b/retrofit/src/test/java/retrofit/http/RequestBuilderTest.java
@@ -76,6 +76,48 @@ public class RequestBuilderTest {
     assertThat(request.getUrl()).isEqualTo("http://example.com/foo/bar/pong/?kit=kat&riff=raff");
     assertThat(request.getBody()).isNull();
   }
+  
+  @Test public void getWithPathAndQueryQuestionMarkParam() throws Exception {
+    Request request = new Helper() //
+        .setMethod("GET") //
+        .setUrl("http://example.com") //
+        .setPath("/foo/bar/{ping}/") //
+        .addNamedParam("ping", "pong?") //
+        .addNamedParam("kit", "kat?") //
+        .build();
+    assertThat(request.getMethod()).isEqualTo("GET");
+    assertThat(request.getHeaders()).isEmpty();
+    assertThat(request.getUrl()).isEqualTo("http://example.com/foo/bar/pong%3F/?kit=kat%3F");
+    assertThat(request.getBody()).isNull();
+  }
+  
+  @Test public void getWithPathAndQueryAmpersandParam() throws Exception {
+    Request request = new Helper() //
+        .setMethod("GET") //
+        .setUrl("http://example.com") //
+        .setPath("/foo/bar/{ping}/") //
+        .addNamedParam("ping", "pong&") //
+        .addNamedParam("kit", "kat&") //
+        .build();
+    assertThat(request.getMethod()).isEqualTo("GET");
+    assertThat(request.getHeaders()).isEmpty();
+    assertThat(request.getUrl()).isEqualTo("http://example.com/foo/bar/pong%26/?kit=kat%26");
+    assertThat(request.getBody()).isNull();
+  }
+  
+  @Test public void getWithPathAndQueryHashParam() throws Exception {
+    Request request = new Helper() //
+        .setMethod("GET") //
+        .setUrl("http://example.com") //
+        .setPath("/foo/bar/{ping}/") //
+        .addNamedParam("ping", "pong#") //
+        .addNamedParam("kit", "kat#") //
+        .build();
+    assertThat(request.getMethod()).isEqualTo("GET");
+    assertThat(request.getHeaders()).isEmpty();
+    assertThat(request.getUrl()).isEqualTo("http://example.com/foo/bar/pong%23/?kit=kat%23");
+    assertThat(request.getBody()).isNull();
+  }
 
   @Test public void getWithPathAndQueryParamAsync() throws Exception {
     Request request = new Helper() //