Repository: square/retrofit
Issue: retrofit2.BuildInConverters$StringConverter let me confused
Labels: Enhancement

I am so confused,when send request like this

 public interface API {
       @FormUrlEncoded
       @POST("login.do?os=" + ConstantValue.OS + "&version=" + ConstantValue.VERSION)
       Observable<LoginInfo> login(@Field("cmd") String params);
  } 
the method of my custom Converter stringConverter method do not work.. (to encrypt request)
retrofit2.BuildInConvert$StringConverter is first before mine execute.. help~please~
retrofit version 2.1.0 @


Issue comments:
i found the default BuildInConverters,its stringConverter will intercept the request which type is String.

```
@Override public Converter<?, String> stringConverter(Type type, Annotation[] annotations,Retrofit retrofit) {
    if (type == String.class) {
        return StringConverter.INSTANCE;
    }
    return null;
}
```

so i change the type of the parameter,like this

public interface API {
   @FormUrlEncoded
   @POST("login.do?os=" + ConstantValue.OS + "&version=" + ConstantValue.VERSION)
   Observable<LoginInfo> login(@Field("cmd") JsonObject params);
}

then my custom Converter work.

note:
i do not know weather the way I encrypt the request  is ok..  
can I change the default Converter ,not change my type to let it can use my custom converter . 

The built-in converter is added last so any user-supplied converters should always take precedence.

Looks like we don't call the string converter when the type is already string. This is potentially problematic if you have annotations on the parameter with which you want to modify the value. Let me look at relaxing this restriction.

thank you very much @JakeWharton @swankjesse 

@iacky 后来你是怎么处理的呢？求解

Pull request review comments:
This is unrelated... it just looks nicer.

Did the IDE tell you to re-format this and you just accidentally pressed yes :curious_face: ?

🍩


Patch that resolved the issue:
diff --git a/retrofit/src/main/java/retrofit2/BuiltInConverters.java b/retrofit/src/main/java/retrofit2/BuiltInConverters.java
index d580c00fb1..f729383a31 100644
--- a/retrofit/src/main/java/retrofit2/BuiltInConverters.java
+++ b/retrofit/src/main/java/retrofit2/BuiltInConverters.java
@@ -27,10 +27,9 @@ final class BuiltInConverters extends Converter.Factory {
   public Converter<ResponseBody, ?> responseBodyConverter(Type type, Annotation[] annotations,
       Retrofit retrofit) {
     if (type == ResponseBody.class) {
-      if (Utils.isAnnotationPresent(annotations, Streaming.class)) {
-        return StreamingResponseBodyConverter.INSTANCE;
-      }
-      return BufferingResponseBodyConverter.INSTANCE;
+      return Utils.isAnnotationPresent(annotations, Streaming.class)
+          ? StreamingResponseBodyConverter.INSTANCE
+          : BufferingResponseBodyConverter.INSTANCE;
     }
     if (type == Void.class) {
       return VoidResponseBodyConverter.INSTANCE;
@@ -47,22 +46,6 @@ public Converter<?, RequestBody> requestBodyConverter(Type type,
     return null;
   }
 
-  @Override public Converter<?, String> stringConverter(Type type, Annotation[] annotations,
-      Retrofit retrofit) {
-    if (type == String.class) {
-      return StringConverter.INSTANCE;
-    }
-    return null;
-  }
-
-  static final class StringConverter implements Converter<String, String> {
-    static final StringConverter INSTANCE = new StringConverter();
-
-    @Override public String convert(String value) throws IOException {
-      return value;
-    }
-  }
-
   static final class VoidResponseBodyConverter implements Converter<ResponseBody, Void> {
     static final VoidResponseBodyConverter INSTANCE = new VoidResponseBodyConverter();
 


Test patch:
diff --git a/retrofit/src/test/java/retrofit2/RetrofitTest.java b/retrofit/src/test/java/retrofit2/RetrofitTest.java
index 9c17cab429..41036c9877 100644
--- a/retrofit/src/test/java/retrofit2/RetrofitTest.java
+++ b/retrofit/src/test/java/retrofit2/RetrofitTest.java
@@ -399,11 +399,13 @@ class MyConverterFactory extends Converter.Factory {
     assertThat(annotations).hasAtLeastOneElementOfType(Annotated.Foo.class);
   }
 
-  @Test public void stringConverterNotCalledForString() {
+  @Test public void stringConverterCalledForString() {
+    final AtomicBoolean factoryCalled = new AtomicBoolean();
     class MyConverterFactory extends Converter.Factory {
       @Override public Converter<?, String> stringConverter(Type type, Annotation[] annotations,
           Retrofit retrofit) {
-        throw new AssertionError();
+        factoryCalled.set(true);
+        return null;
       }
     }
     Retrofit retrofit = new Retrofit.Builder()
@@ -413,7 +415,7 @@ class MyConverterFactory extends Converter.Factory {
     CallMethod service = retrofit.create(CallMethod.class);
     Call<ResponseBody> call = service.queryString(null);
     assertThat(call).isNotNull();
-    // We also implicitly assert the above factory was not called as it would have thrown.
+    assertThat(factoryCalled.get()).isTrue();
   }
 
   @Test public void stringConverterReturningNullResultsInDefault() {