Merge pull request #47 from Netflix/skip-query

Skip query template parameters when corresponding java arg is null

CHANGES.md

@@ -1,5 +1,6 @@
 ### Version 4.2/3.3
 * Document and enforce JAX-RS annotation processing from server POV
+* Skip query template parameters when corresponding java arg is null
 
 ### Version 4.1/3.2
 * update to dagger 1.1

feign-core/src/main/java/feign/RequestTemplate.java

@@ -23,6 +23,7 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -98,9 +99,7 @@ public final class RequestTemplate implements Serializable {
     for (Entry<String, ?> entry : unencoded.entrySet()) {
       encoded.put(entry.getKey(), urlEncode(String.valueOf(entry.getValue())));
     }
-    String queryLine = expand(queryLine(), encoded);
-    queries.clear();
-    pullAnyQueriesOutOfUrl(new StringBuilder(queryLine));
+    replaceQueryValues(encoded);
     String resolvedUrl = expand(url.toString(), encoded).replace("%2F", "/");
     url = new StringBuilder(resolvedUrl);
 
@@ -505,6 +504,37 @@ public final class RequestTemplate implements Serializable {
     return request().toString();
   }
 
+  /**
+   * Replaces query values which are templated with corresponding values from the {@code unencoded} map.
+   * Any unresolved queries are removed.
+   */
+  public void replaceQueryValues(Map<String, ?> unencoded) {
+    Iterator<Entry<String, Collection<String>>> iterator = queries.entrySet().iterator();
+    while (iterator.hasNext()) {
+      Entry<String, Collection<String>> entry = iterator.next();
+      if (entry.getValue() == null) {
+        continue;
+      }
+      Collection<String> values = new ArrayList<String>();
+      for (String value : entry.getValue()) {
+        if (value.indexOf('{') == 0 && value.indexOf('}') == value.length() - 1) {
+          Object variableValue = unencoded.get(value.substring(1, value.length() - 1));
+          // only add non-null expressions
+          if (variableValue != null) {
+            values.add(String.valueOf(variableValue));
+          }
+        } else {
+          values.add(value);
+        }
+      }
+      if (values.isEmpty()) {
+        iterator.remove();
+      } else {
+        entry.setValue(values);
+      }
+    }
+  }
+
   public String queryLine() {
     if (queries.isEmpty())
       return "";
@@ -524,6 +554,5 @@ public final class RequestTemplate implements Serializable {
     return queryBuilder.insert(0, '?').toString();
   }
 
-
   private static final long serialVersionUID = 1L;
 }

feign-core/src/test/java/feign/RequestTemplateTest.java

@@ -18,7 +18,6 @@ package feign;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableMap;
-
 import org.testng.annotations.Test;
 
 import static feign.RequestTemplate.expand;
@@ -133,4 +132,35 @@ public class RequestTemplateTest {
         + "\n" //
         + "{\"customer_name\": \"netflix\", \"user_name\": \"denominator\", \"password\": \"password\"}");
   }
+
+  @Test public void skipUnresolvedQueries() throws Exception {
+    RequestTemplate template = new RequestTemplate().method("GET")//
+        .append("/domains/{domainId}/records")//
+        .query("optional", "{optional}")//
+        .query("name", "{nameVariable}");
+
+    template = template.resolve(ImmutableMap.<String, Object>builder()//
+        .put("domainId", 1001)//
+        .put("nameVariable", "denominator.io")//
+        .build()
+    );
+
+    assertEquals(template.toString(), ""//
+        + "GET /domains/1001/records?name=denominator.io HTTP/1.1\n");
+  }
+
+  @Test public void allQueriesUnresolvable() throws Exception {
+    RequestTemplate template = new RequestTemplate().method("GET")//
+        .append("/domains/{domainId}/records")//
+        .query("optional", "{optional}")//
+        .query("optional2", "{optional2}");
+
+    template = template.resolve(ImmutableMap.<String, Object>builder()//
+        .put("domainId", 1001)//
+        .build()
+    );
+
+    assertEquals(template.toString(), ""//
+        + "GET /domains/1001/records HTTP/1.1\n");
+  }
 }

feign-jaxrs/README.md

@@ -30,7 +30,7 @@ Adds the first value as the `Content-Type` header.
 #### `@PathParam`
 Links the value of the corresponding parameter to a template variable declared in the path.
 #### `@QueryParam`
-Links the value of the corresponding parameter to a query parameter.
+Links the value of the corresponding parameter to a query parameter.  When invoked, null will skip the query param.
 #### `@HeaderParam`
 Links the value of the corresponding parameter to a header.
 #### `@FormParam`