Repository: square/retrofit
Issue: Improper encoding of path segment

Here's a test case:

https://android.api.simple.com/atm-api/nearest/45.529801,-122.683633 (works)

Retrofit encodes as:

https://android.api.simple.com/atm-api/nearest/45.529801%2C-122.683633 (doesn't work)

According to [this guide](http://blog.lunatech.com/2009/02/03/what-every-web-developer-must-know-about-url-encoding) the `,` (comma) character should be allowed unencoded in the path.

Easily worked around with `encoded=true`.


Issue comments:
I'm assuming you are using something like `/nearest/{location}` and specifying the location as a parameter?

Yep, sorry. Here's the endpoint definition (with `encoded` workaround):

``` java
@GET("/atm-api/nearest/{coordinate}")
Call<AtmLocation> getNearestAtmLocation(@Path(value = "coordinate", encoded = true) Coordinate coordinate);
```

`Coordinate` is converted using this `toString` implementation:

``` java
return String.format("%.6f,%.6f", latitude, longitude)
```

Try with Retrofit 2 and OkHttp 2.5 ?

@swankjesse Yep this is with `-beta1`

This is because path encoding happens in Retrofit, not OkHttp's HttpUrl class.

``` java
String encodedValue = URLEncoder.encode(String.valueOf(value), "UTF-8");
// URLEncoder encodes for use as a query parameter. Path encoding uses %20 to
// encode spaces rather than +. Query encoding difference specified in HTML spec.
// Any remaining plus signs represent spaces as already URLEncoded.
encodedValue = encodedValue.replace("+", "%20");
relativeUrl = relativeUrl.replace("{" + name + "}", encodedValue);
```


Pull request review comments:

Patch that resolved the issue:
diff --git a/retrofit/src/main/java/retrofit/RequestBuilder.java b/retrofit/src/main/java/retrofit/RequestBuilder.java
index 36195f0c0f..65eed2c1a1 100644
--- a/retrofit/src/main/java/retrofit/RequestBuilder.java
+++ b/retrofit/src/main/java/retrofit/RequestBuilder.java
@@ -23,11 +23,14 @@
 import com.squareup.okhttp.Request;
 import com.squareup.okhttp.RequestBody;
 import java.io.IOException;
-import java.io.UnsupportedEncodingException;
-import java.net.URLEncoder;
+import okio.Buffer;
 import okio.BufferedSink;
 
 final class RequestBuilder {
+  private static final char[] HEX_DIGITS =
+      { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F' };
+  private static final String PATH_SEGMENT_ENCODE_SET = " \"<>^`{}|/\\?#";
+
   private final String method;
 
   private final HttpUrl baseUrl;
@@ -82,20 +85,55 @@ void addPathParam(String name, String value, boolean encoded) {
       // The relative URL is cleared when the first query parameter is set.
       throw new AssertionError();
     }
-    try {
-      if (!encoded) {
-        String encodedValue = URLEncoder.encode(String.valueOf(value), "UTF-8");
-        // URLEncoder encodes for use as a query parameter. Path encoding uses %20 to
-        // encode spaces rather than +. Query encoding difference specified in HTML spec.
-        // Any remaining plus signs represent spaces as already URLEncoded.
-        encodedValue = encodedValue.replace("+", "%20");
-        relativeUrl = relativeUrl.replace("{" + name + "}", encodedValue);
+    relativeUrl = relativeUrl.replace("{" + name + "}", canonicalize(value, encoded));
+  }
+
+  static String canonicalize(String input, boolean alreadyEncoded) {
+    int codePoint;
+    for (int i = 0, limit = input.length(); i < limit; i += Character.charCount(codePoint)) {
+      codePoint = input.codePointAt(i);
+      if (codePoint < 0x20 || codePoint >= 0x7f
+          || PATH_SEGMENT_ENCODE_SET.indexOf(codePoint) != -1
+          || (codePoint == '%' && !alreadyEncoded)) {
+        // Slow path: the character at i requires encoding!
+        Buffer out = new Buffer();
+        out.writeUtf8(input, 0, i);
+        canonicalize(out, input, i, limit, alreadyEncoded);
+        return out.readUtf8();
+      }
+    }
+
+    // Fast path: no characters required encoding.
+    return input;
+  }
+
+  static void canonicalize(Buffer out, String input, int pos, int limit, boolean alreadyEncoded) {
+    Buffer utf8Buffer = null; // Lazily allocated.
+    int codePoint;
+    for (int i = pos; i < limit; i += Character.charCount(codePoint)) {
+      codePoint = input.codePointAt(i);
+      if (alreadyEncoded
+          && (codePoint == '\t' || codePoint == '\n' || codePoint == '\f' || codePoint == '\r')) {
+        // Skip this character.
+      } else if (codePoint < 0x20
+          || codePoint >= 0x7f
+          || PATH_SEGMENT_ENCODE_SET.indexOf(codePoint) != -1
+          || (codePoint == '%' && !alreadyEncoded)) {
+        // Percent encode this character.
+        if (utf8Buffer == null) {
+          utf8Buffer = new Buffer();
+        }
+        utf8Buffer.writeUtf8CodePoint(codePoint);
+        while (!utf8Buffer.exhausted()) {
+          int b = utf8Buffer.readByte() & 0xff;
+          out.writeByte('%');
+          out.writeByte(HEX_DIGITS[(b >> 4) & 0xf]);
+          out.writeByte(HEX_DIGITS[b & 0xf]);
+        }
       } else {
-        relativeUrl = relativeUrl.replace("{" + name + "}", String.valueOf(value));
+        // This character doesn't need encoding. Just copy it over.
+        out.writeUtf8CodePoint(codePoint);
       }
-    } catch (UnsupportedEncodingException e) {
-      throw new RuntimeException(
-          "Unable to convert path parameter \"" + name + "\" value to UTF-8:" + value, e);
     }
   }
 


Test patch:
diff --git a/retrofit/src/test/java/retrofit/RequestBuilderTest.java b/retrofit/src/test/java/retrofit/RequestBuilderTest.java
index a0dd9de018..404e539154 100644
--- a/retrofit/src/test/java/retrofit/RequestBuilderTest.java
+++ b/retrofit/src/test/java/retrofit/RequestBuilderTest.java
@@ -770,7 +770,7 @@ Call<Object> method(@Path("ping") String ping, @Query("kit") String kit) {
     Request request = buildRequest(Example.class, "pong&", "kat&");
     assertThat(request.method()).isEqualTo("GET");
     assertThat(request.headers().size()).isZero();
-    assertThat(request.urlString()).isEqualTo("http://example.com/foo/bar/pong%26/?kit=kat%26");
+    assertThat(request.urlString()).isEqualTo("http://example.com/foo/bar/pong&/?kit=kat%26");
     assertThat(request.body()).isNull();
   }