Provides a nicer error when a user omits the http method

Adrian Cole · Adrian Cole · commit ae7e00e45cab · 2016-01-09T20:05:42.000+08:00
Before this change, if someone accidentally left out the HTTP method in a `@RequestLine` annotation, they'd get an undecipherable error like: "Body parameters cannot be used with form parameters." from Contract.java. This makes the omission easier to find by changing the message to: "RequestLine annotation didn't start with an HTTP verb..." Fixes OpenFeign#310
diff --git a/core/src/main/java/feign/Contract.java b/core/src/main/java/feign/Contract.java
@@ -189,6 +189,9 @@ protected void processAnnotationOnMethod(MethodMetadata data, Annotation methodA
         checkState(emptyToNull(requestLine) != null,
                    "RequestLine annotation was empty on method %s.", method.getName());
         if (requestLine.indexOf(' ') == -1) {
+          checkState(requestLine.indexOf('/') == -1,
+              "RequestLine annotation didn't start with an HTTP verb on method %s.",
+              method.getName());
           data.template().method(requestLine);
           return;
         }
diff --git a/core/src/main/java/feign/RequestTemplate.java b/core/src/main/java/feign/RequestTemplate.java
@@ -32,6 +32,7 @@
 
 import static feign.Util.CONTENT_LENGTH;
 import static feign.Util.UTF_8;
+import static feign.Util.checkArgument;
 import static feign.Util.checkNotNull;
 import static feign.Util.emptyToNull;
 import static feign.Util.toArray;
@@ -249,6 +250,7 @@ public Request request() {
   /* @see Request#method() */
   public RequestTemplate method(String method) {
     this.method = checkNotNull(method, "method");
+    checkArgument(method.matches("^[A-Z]+$"), "Invalid HTTP Method: %s", method);
     return this;
   }
   
diff --git a/core/src/test/java/feign/DefaultContractTest.java b/core/src/test/java/feign/DefaultContractTest.java
@@ -578,4 +578,18 @@ private MethodMetadata parseAndValidateMetadata(Class<?> targetType, String meth
     return contract.parseAndValidateMetadata(targetType,
                                              targetType.getMethod(method, parameterTypes));
   }
+
+  interface MissingMethod {
+    @RequestLine("/path?queryParam={queryParam}")
+    Response updateSharing(@Param("queryParam") long queryParam, String bodyParam);
+  }
+
+  /** Let's help folks not lose time when they mistake request line for a URI! */
+  @Test
+  public void missingMethod() throws Exception {
+    thrown.expect(IllegalStateException.class);
+    thrown.expectMessage("RequestLine annotation didn't start with an HTTP verb on method updateSharing");
+
+    contract.parseAndValidatateMetadata(MissingMethod.class);
+  }
 }
diff --git a/core/src/test/java/feign/RequestTemplateTest.java b/core/src/test/java/feign/RequestTemplateTest.java
@@ -15,11 +15,13 @@
  */
 package feign;
 
+import org.junit.Rule;
 import org.junit.Test;
 
 import java.util.Arrays;
 import java.util.LinkedHashMap;
 import java.util.Map;
+import org.junit.rules.ExpectedException;
 
 import static feign.RequestTemplate.expand;
 import static feign.assertj.FeignAssertions.assertThat;
@@ -28,6 +30,9 @@
 
 public class RequestTemplateTest {
 
+  @Rule
+  public final ExpectedException thrown = ExpectedException.none();
+
   /**
    * Avoid depending on guava solely for map literals.
    */
@@ -273,4 +278,13 @@ public void encodeSlashTest() throws Exception {
     assertThat(template)
         .hasUrl("/api/%2F");
   }
+
+  /** Implementations have a bug if they pass junk as the http method. */
+  @Test
+  public void uriStuffedIntoMethod() throws Exception {
+    thrown.expect(IllegalArgumentException.class);
+    thrown.expectMessage("Invalid HTTP Method: /path?queryParam={queryParam}");
+
+    new RequestTemplate().method("/path?queryParam={queryParam}");
+  }
 }
