Repository: square/retrofit
Issue: Additional parameter annotations not ignored.

Hey, I'm using Retrofit 2.0.0-SNAPSHOT.

My REST service interface is written in Kotlin and Kotlin seems to always add an extends-wildcard to generics.

Retrofit complains with

> Exception in thread "main" java.lang.IllegalArgumentException: Method return type must not include a type variable or wildcard: retrofit.Call&lt;java.util.List&lt;? extends retrofit.Repo>>

My Kotlin code is

``` kotlin
interface GitHubService {
    @GET("/users/{user}/repos?per_page=1")
    fun listRepos(@Path("user") user: String,
                  @Query("page") page: Int): Call<List<Repo>>

    @GET("/users/{user}/repos?per_page=1")
    fun listRepos(@Path("user") user: String,
                  @Query("page") page: Int,
                  callback: Callback<List<Repo>>)
}
```

I wonder whether the check for the wildcards is necessary.


Issue comments:
That's completely broken. a `Call<List<? extends Repo>>` is definitely not the same thing as a `Call<List<Repo>>`. The former `List` cannot receive a call to `add()` of anything but null; the latter can.

Is there a Kotlin bug for this?

I've found this: https://youtrack.jetbrains.com/issue/KT-5792

It suggest to change `List` to `MutableList`. Retrofit will complain with this error:

> Exception in thread "main" java.lang.IllegalArgumentException: Method response type is java.util.List&lt;retrofit.example.Repo> but no converter factory registered. Either add a converter factory to the Retrofit instance or use ResponseBody.

I guess this is unrelated and the new Retrofit API requires me to explicitely define a `Converter`.

Yup.

Right. We no longer use any converter as a default. You can add a dependency to `converter-gson` and just do `.converterFactory(GsonConverterFactory.create())` when you create the Retrofit instance.

As to the Kotlin stuff, I guess this will just serve as a recommended practice.

Already got it, thanks! I use `compile "com.squareup.retrofit:converter-moshi:2.0.0-SNAPSHOT"` and `.converterFactory(MoshiConverterFactory.create())`.

Another error is

> Exception in thread "main" java.lang.IllegalArgumentException: Multiple Retrofit annotations found, only one allowed. (parameter #1) for method GitHubService.listRepos

for this interface

``` kotlin
interface GitHubService {
    @GET("/users/{user}/repos?per_page=1")
    fun listRepos(@Path("user") user: String,
                  @Query("page") page: Int): Call<MutableList<Repo>>
}
```

Sorry for posting this unrelated error. But I don't understand the error message, found no solution in the changeset that introduced this error check and it even complains with only a single Retrofit annotation.

It somehow sees two annotations on a single parameter. I'll have to try it tomorrow unless you can run `javap -c` on the class file and post it.

`javap -c GitHubService.class`:

```
Compiled from "retrobug.kt"
public interface retrobug.GitHubService {
  public static final kotlin.reflect.KClass $kotlinClass;

  static {};
    Code:
       0: ldc           #2                  // class retrobug/GitHubService
       2: invokestatic  #38                 // Method kotlin/jvm/internal/Reflection.createKotlinClass:(Ljava/lang/Class;)Lkotlin/reflect/KClass;
       5: putstatic     #40                 // Field $kotlinClass:Lkotlin/reflect/KClass;
       8: return

  public abstract retrofit.Call<java.util.List<retrobug.Repo>> listRepos(java.lang.String, int);
}
```

`javap -c GitHubService$$TImpl.class`:

```
Compiled from "retrobug.kt"
public final class retrobug.GitHubService$$TImpl {
}
```

It definately worked with the same interface in Kotlin (minus the usage of `Call`) and Retrofit 1.9.0.

I adapted the `SimpleService` sample [1] to Kotlin and set a breakpoint to a line in `RequestFactoryParser#parseParameters()` [2].

![retrofit-bug-01](https://cloud.githubusercontent.com/assets/496255/8691233/1768e636-2ac0-11e5-8a94-bcea656eb543.png)

Seems that the `IllegalArgumentException` is due to the additional Kotlin annotation `
@JetValueParameter`.

[1] https://github.com/square/retrofit/blob/904bf94b7c/samples/src/main/java/com/example/retrofit/SimpleService.java#L25
[2] https://github.com/square/retrofit/blob/bf02efdf30/retrofit/src/main/java/retrofit/RequestFactoryParser.java#L208

---

``` kotlin
import retrofit.Call
import retrofit.MoshiConverterFactory
import retrofit.Retrofit
import retrofit.http.GET
import retrofit.http.Path

data class Contributor(val login: String,
                       val contributions: Int)

interface GitHubService {
    @GET("/repos/{owner}/{repo}/contributors")
    fun contributors(@Path("owner") owner: String,
                     @Path("repo") repo: String): Call<MutableList<Contributor>>
}

fun main(args: Array<String>) {
    val retrofit = Retrofit.Builder()
        .baseUrl("https://api.github.com")
        .converterFactory(MoshiConverterFactory.create())
        .build()

    val service = retrofit.create(javaClass<GitHubService>())

    val response = service.contributors("square", "retrofit").execute()
    response.body().forEach { println(it) }
}
```

Looks like a simple bug. We should be ignoring annotations we don't recognize.

Updated to the newest snapshot. Everything works fine. Thanks a lot, great work!

Small note: I got an error that "service methods cannot return void". Obviously the approach with `Callback` as method argument is superseded by `Call` that also allows `Callback` using `enqueue()`. This seems to be a great API change in order to improve testability.


Pull request review comments:

Patch that resolved the issue:
diff --git a/retrofit/src/main/java/retrofit/RequestFactory.java b/retrofit/src/main/java/retrofit/RequestFactory.java
index 99f0891e80..63527aabeb 100644
--- a/retrofit/src/main/java/retrofit/RequestFactory.java
+++ b/retrofit/src/main/java/retrofit/RequestFactory.java
@@ -51,6 +51,13 @@ Request create(Object... args) {
 
     if (args != null) {
       RequestBuilderAction[] actions = requestBuilderActions;
+      if (actions.length != args.length) {
+        throw new IllegalArgumentException("Argument count ("
+            + args.length
+            + ") doesn't match action count ("
+            + actions.length
+            + ")");
+      }
       for (int i = 0, count = args.length; i < count; i++) {
         actions[i].perform(requestBuilder, args[i]);
       }
diff --git a/retrofit/src/main/java/retrofit/RequestFactoryParser.java b/retrofit/src/main/java/retrofit/RequestFactoryParser.java
index bdac016cd8..79337ee2c2 100644
--- a/retrofit/src/main/java/retrofit/RequestFactoryParser.java
+++ b/retrofit/src/main/java/retrofit/RequestFactoryParser.java
@@ -48,7 +48,6 @@
 
 import static retrofit.Utils.methodError;
 
-/** Request metadata about a service interface declaration. */
 final class RequestFactoryParser {
   // Upper and lower characters, digits, underscores, and hyphens, starting with a character.
   private static final String PARAM = "[a-zA-Z][a-zA-Z0-9_-]*";
@@ -205,10 +204,7 @@ private void parseParameters(Converter.Factory converterFactory) {
       Annotation[] methodParameterAnnotations = methodParameterAnnotationArrays[i];
       if (methodParameterAnnotations != null) {
         for (Annotation methodParameterAnnotation : methodParameterAnnotations) {
-          if (requestBuilderActions[i] != null) {
-            throw parameterError(i, "Multiple Retrofit annotations found, only one allowed.");
-          }
-
+          RequestBuilderAction action = null;
           if (methodParameterAnnotation instanceof Url) {
             if (gotUrl) {
               throw parameterError(i, "Multiple @Url method annotations found.");
@@ -226,7 +222,7 @@ private void parseParameters(Converter.Factory converterFactory) {
               throw parameterError(i, "@Url cannot be used with @%s URL", httpMethod);
             }
             gotUrl = true;
-            requestBuilderActions[i] = new RequestBuilderAction.Url();
+            action = new RequestBuilderAction.Url();
 
           } else if (methodParameterAnnotation instanceof Path) {
             if (gotQuery) {
@@ -244,12 +240,11 @@ private void parseParameters(Converter.Factory converterFactory) {
             Path path = (Path) methodParameterAnnotation;
             String name = path.value();
             validatePathName(i, name);
-            requestBuilderActions[i] = new RequestBuilderAction.Path(name, path.encoded());
+            action = new RequestBuilderAction.Path(name, path.encoded());
 
           } else if (methodParameterAnnotation instanceof Query) {
             Query query = (Query) methodParameterAnnotation;
-            requestBuilderActions[i] =
-                new RequestBuilderAction.Query(query.value(), query.encoded());
+            action = new RequestBuilderAction.Query(query.value(), query.encoded());
             gotQuery = true;
 
           } else if (methodParameterAnnotation instanceof QueryMap) {
@@ -257,19 +252,18 @@ private void parseParameters(Converter.Factory converterFactory) {
               throw parameterError(i, "@QueryMap parameter type must be Map.");
             }
             QueryMap queryMap = (QueryMap) methodParameterAnnotation;
-            requestBuilderActions[i] = new RequestBuilderAction.QueryMap(queryMap.encoded());
+            action = new RequestBuilderAction.QueryMap(queryMap.encoded());
 
           } else if (methodParameterAnnotation instanceof Header) {
             Header header = (Header) methodParameterAnnotation;
-            requestBuilderActions[i] = new RequestBuilderAction.Header(header.value());
+            action = new RequestBuilderAction.Header(header.value());
 
           } else if (methodParameterAnnotation instanceof Field) {
             if (!isFormEncoded) {
               throw parameterError(i, "@Field parameters can only be used with form encoding.");
             }
             Field field = (Field) methodParameterAnnotation;
-            requestBuilderActions[i] =
-                new RequestBuilderAction.Field(field.value(), field.encoded());
+            action = new RequestBuilderAction.Field(field.value(), field.encoded());
             gotField = true;
 
           } else if (methodParameterAnnotation instanceof FieldMap) {
@@ -280,7 +274,7 @@ private void parseParameters(Converter.Factory converterFactory) {
               throw parameterError(i, "@FieldMap parameter type must be Map.");
             }
             FieldMap fieldMap = (FieldMap) methodParameterAnnotation;
-            requestBuilderActions[i] = new RequestBuilderAction.FieldMap(fieldMap.encoded());
+            action = new RequestBuilderAction.FieldMap(fieldMap.encoded());
             gotField = true;
 
           } else if (methodParameterAnnotation instanceof Part) {
@@ -303,7 +297,7 @@ private void parseParameters(Converter.Factory converterFactory) {
               }
               converter = converterFactory.get(methodParameterType);
             }
-            requestBuilderActions[i] = new RequestBuilderAction.Part<>(headers, converter);
+            action = new RequestBuilderAction.Part<>(headers, converter);
             gotPart = true;
 
           } else if (methodParameterAnnotation instanceof PartMap) {
@@ -315,8 +309,7 @@ private void parseParameters(Converter.Factory converterFactory) {
               throw parameterError(i, "@PartMap parameter type must be Map.");
             }
             PartMap partMap = (PartMap) methodParameterAnnotation;
-            requestBuilderActions[i] =
-                new RequestBuilderAction.PartMap(converterFactory, partMap.encoding());
+            action = new RequestBuilderAction.PartMap(converterFactory, partMap.encoding());
             gotPart = true;
 
           } else if (methodParameterAnnotation instanceof Body) {
@@ -341,9 +334,16 @@ private void parseParameters(Converter.Factory converterFactory) {
               converter = converterFactory.get(methodParameterType);
             }
 
-            requestBuilderActions[i] = new RequestBuilderAction.Body<>(converter);
+            action = new RequestBuilderAction.Body<>(converter);
             gotBody = true;
           }
+
+          if (action != null) {
+            if (requestBuilderActions[i] != null) {
+              throw parameterError(i, "Multiple Retrofit annotations found, only one allowed.");
+            }
+            requestBuilderActions[i] = action;
+          }
         }
       }
 


Test patch:
diff --git a/retrofit/src/test/java/retrofit/RequestBuilderTest.java b/retrofit/src/test/java/retrofit/RequestBuilderTest.java
index d097628f6c..6fc52cb7d7 100644
--- a/retrofit/src/test/java/retrofit/RequestBuilderTest.java
+++ b/retrofit/src/test/java/retrofit/RequestBuilderTest.java
@@ -162,6 +162,19 @@ Call<Object> method(@Body @Query("nope") Object o) {
     }
   }
 
+  @interface NonNull {}
+
+  @Test public void multipleParameterAnnotationsOnlyOneRetrofitAllowed() throws Exception {
+    class Example {
+      @GET("/") //
+      Call<Object> method(@Query("maybe") @NonNull Object o) {
+        return null;
+      }
+    }
+    Request request = buildRequest(Example.class, "yep");
+    assertThat(request.urlString()).isEqualTo("http://example.com/?maybe=yep");
+  }
+
   @Test public void twoMethodsFail() {
     class Example {
       @PATCH("/foo") //