Merge pull request #40 from palantir/bugfix/optional-headers

Disallow PathParam Optionals
palantir · Feb 9, 2016 · 4c5699b · 4c5699b
2 parents 31d72b4 + cf9b99d
commit 4c5699b
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 23 deletions.
diff --git a/http-clients/src/main/java/com/palantir/remoting/http/GuavaOptionalAwareContract.java b/http-clients/src/main/java/com/palantir/remoting/http/GuavaOptionalAwareContract.java
@@ -28,10 +28,15 @@
 import java.util.List;
 import java.util.Map;
 import javax.ws.rs.HeaderParam;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.QueryParam;
 
 /**
- * Decorates a Contract and uses {@link GuavaNullOptionalExpander} for any PathParam or QueryParam parameters
- * and {@link GuavaEmptyOptionalExpander} for any {@link HeaderParam} parameters.
+ * Decorates a {@link Contract} and uses {@link GuavaNullOptionalExpander} for any {@link QueryParam} parameters,
+ * {@link GuavaEmptyOptionalExpander} for any {@link HeaderParam} parameters, and throws a {@link RuntimeException}
+ * at first encounter of an {@link Optional} typed {@link PathParam}.
+ * <p>
+ * {@link PathParam}s require a value, and so we explicitly disallow use with {@link Optional}.
  */
 public final class GuavaOptionalAwareContract implements Contract {
 
@@ -61,10 +66,16 @@ public List<MethodMetadata> parseAndValidatateMetadata(Class<?> targetType) {
                 for (int i = 0; i < parameterTypes.length; i++) {
                     Class<?> cls = parameterTypes[i];
                     if (cls.equals(Optional.class)) {
-                        if (FluentIterable.of(annotations[i]).transform(EXTRACT_CLASS).contains(HeaderParam.class)) {
+                        FluentIterable<Class<?>> paramAnnotations =
+                                FluentIterable.of(annotations[i]).transform(EXTRACT_CLASS);
+                        if (paramAnnotations.contains(HeaderParam.class)) {
                             md.indexToExpanderClass().put(i, GuavaEmptyOptionalExpander.class);
-                        } else {
+                        } else if (paramAnnotations.contains(QueryParam.class)) {
                             md.indexToExpanderClass().put(i, GuavaNullOptionalExpander.class);
+                        } else if (paramAnnotations.contains(PathParam.class)) {
+                            throw new RuntimeException(String.format(
+                                    "Cannot use Guava Optionals with PathParams. (Class: %s, Method: %s, Param: arg%d)",
+                                    targetType.getName(), method.getName(), i));
                         }
                     }
                 }

diff --git a/http-clients/src/test/java/com/palantir/remoting/http/GuavaOptionalAwareContractTest.java b/http-clients/src/test/java/com/palantir/remoting/http/GuavaOptionalAwareContractTest.java
@@ -18,6 +18,7 @@
 
 import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.fail;
 
 import com.google.common.base.Optional;
 import com.google.common.collect.ImmutableSet;
@@ -50,10 +51,17 @@ public void before() {
     }
 
     @Path("/")
-    public interface FakeoInterface {
+    public interface CannotDecorateInterface {
         @GET
         @Path("{opt}/foo/{req}")
         String path(@PathParam("opt") Optional<String> opt, @PathParam("req") String req);
+    }
+
+    @Path("/")
+    public interface FakeoInterface {
+        @GET
+        @Path("foo/{req}")
+        String path(@PathParam("req") String req);
 
         @GET
         @Path("foo")
@@ -65,28 +73,23 @@ public interface FakeoInterface {
     }
 
     @Test
-    public void testAbsentPath() throws Exception {
-        proxy.path(Optional.<String>absent(), "str2");
-        RecordedRequest takeRequest = server.takeRequest();
-        // note that this differs from feign's default behavior of leaving the template
-        // in the path. The default would be "/{opt}/foo"
-        // This is because feign expands null values to their parameter name if they
-        // have no Expander and the value is null
-        assertThat(takeRequest.getPath(), is("/null/foo/str2"));
-    }
-
-    @Test
-    public void testEmptyStringPath() throws Exception {
-        proxy.path(Optional.<String>of(""), "str2");
-        RecordedRequest takeRequest = server.takeRequest();
-        assertThat(takeRequest.getPath(), is("//foo/str2"));
+    public void testCannotDecorateInterfaceWithOptionalPathParam() {
+        try {
+            FeignClients.standard().createProxy(Optional.<SSLSocketFactory>absent(),
+                    ImmutableSet.of("https://localhost:" + server.getPort()), CannotDecorateInterface.class);
+            fail();
+        } catch (RuntimeException e) {
+            assertThat(e.getMessage(), is("Cannot use Guava Optionals with PathParams."
+                    + " (Class: com.palantir.remoting.http.GuavaOptionalAwareContractTest$CannotDecorateInterface,"
+                    + " Method: path, Param: arg0)"));
+        }
     }
 
     @Test
-    public void testStringPath() throws Exception {
-        proxy.path(Optional.<String>of("str"), "str2");
+    public void testRegularPathParam() throws Exception {
+        proxy.path("str2");
         RecordedRequest takeRequest = server.takeRequest();
-        assertThat(takeRequest.getPath(), is("/str/foo/str2"));
+        assertThat(takeRequest.getPath(), is("/foo/str2"));
     }
 
     @Test