Merge pull request square#1647 from square/jw/forbid-null-in-maps

Forbid null map values, and null maps.
Eli-Fox · Mar 4, 2016 · 36de10d · 36de10d
2 parents 90ffc80 + b28f905
commit 36de10d
Show file tree

Hide file tree

Showing 2 changed files with 130 additions and 17 deletions.
diff --git a/retrofit/src/main/java/retrofit2/RequestAction.java b/retrofit/src/main/java/retrofit2/RequestAction.java
@@ -133,17 +133,21 @@ static final class QueryMap<T> extends RequestAction<Map<String, T>> {
     }
 
     @Override void perform(RequestBuilder builder, Map<String, T> value) throws IOException {
-      if (value == null) return; // Skip null values.
+      if (value == null) {
+        throw new IllegalArgumentException("Query map was null.");
+      }
 
       for (Map.Entry<String, T> entry : value.entrySet()) {
         String entryKey = entry.getKey();
         if (entryKey == null) {
           throw new IllegalArgumentException("Query map contained null key.");
         }
         T entryValue = entry.getValue();
-        if (entryValue != null) { // Skip null values.
-          builder.addQueryParam(entryKey, valueConverter.convert(entryValue), encoded);
+        if (entryValue == null) {
+          throw new IllegalArgumentException(
+              "Query map contained null value for key '" + entryKey + "'.");
         }
+        builder.addQueryParam(entryKey, valueConverter.convert(entryValue), encoded);
       }
     }
   }
@@ -175,17 +179,21 @@ static final class FieldMap<T> extends RequestAction<Map<String, T>> {
     }
 
     @Override void perform(RequestBuilder builder, Map<String, T> value) throws IOException {
-      if (value == null) return; // Skip null values.
+      if (value == null) {
+        throw new IllegalArgumentException("Field map was null.");
+      }
 
       for (Map.Entry<String, T> entry : value.entrySet()) {
         String entryKey = entry.getKey();
         if (entryKey == null) {
           throw new IllegalArgumentException("Field map contained null key.");
         }
         T entryValue = entry.getValue();
-        if (entryValue != null) { // Skip null values.
-          builder.addFormField(entryKey, valueConverter.convert(entryValue), encoded);
+        if (entryValue == null) {
+          throw new IllegalArgumentException(
+              "Field map contained null value for key '" + entryKey + "'.");
         }
+        builder.addFormField(entryKey, valueConverter.convert(entryValue), encoded);
       }
     }
   }
@@ -222,7 +230,9 @@ static final class PartMap<T> extends RequestAction<Map<String, T>> {
     }
 
     @Override void perform(RequestBuilder builder, Map<String, T> value) throws IOException {
-      if (value == null) return; // Skip null values.
+      if (value == null) {
+        throw new IllegalArgumentException("Part map was null.");
+      }
 
       for (Map.Entry<String, T> entry : value.entrySet()) {
         String entryKey = entry.getKey();
@@ -231,7 +241,8 @@ static final class PartMap<T> extends RequestAction<Map<String, T>> {
         }
         T entryValue = entry.getValue();
         if (entryValue == null) {
-          continue; // Skip null values.
+          throw new IllegalArgumentException(
+              "Part map contained null value for key '" + entryKey + "'.");
         }
 
         Headers headers = Headers.of(

diff --git a/retrofit/src/test/java/retrofit2/RequestBuilderTest.java b/retrofit/src/test/java/retrofit2/RequestBuilderTest.java
@@ -459,6 +459,22 @@ Call<ResponseBody> method(@QueryMap Foo a) {
     assertThat(request.url().toString()).isEqualTo("http://example.com/?hello=world");
   }
 
+  @Test public void queryMapRejectsNull() {
+    class Example {
+      @GET("/") //
+      Call<ResponseBody> method(@QueryMap Map<String, String> a) {
+        return null;
+      }
+    }
+
+    try {
+      buildRequest(Example.class, new Object[] { null });
+      fail();
+    } catch (IllegalArgumentException e) {
+      assertThat(e).hasMessage("Query map was null.");
+    }
+  }
+
   @Test public void queryMapRejectsNullKeys() {
     class Example {
       @GET("/") //
@@ -479,6 +495,26 @@ Call<ResponseBody> method(@QueryMap Map<String, String> a) {
     }
   }
 
+  @Test public void queryMapRejectsNullValues() {
+    class Example {
+      @GET("/") //
+      Call<ResponseBody> method(@QueryMap Map<String, String> a) {
+        return null;
+      }
+    }
+
+    Map<String, String> queryParams = new LinkedHashMap<>();
+    queryParams.put("ping", "pong");
+    queryParams.put("kit", null);
+
+    try {
+      buildRequest(Example.class, queryParams);
+      fail();
+    } catch (IllegalArgumentException e) {
+      assertThat(e).hasMessage("Query map contained null value for key 'kit'.");
+    }
+  }
+
   @Test public void twoBodies() {
     class Example {
       @PUT("/") //
@@ -942,7 +978,6 @@ Call<ResponseBody> method(@QueryMap Map<String, Object> query) {
 
     Map<String, Object> params = new LinkedHashMap<>();
     params.put("kit", "kat");
-    params.put("foo", null);
     params.put("ping", "pong");
 
     Request request = buildRequest(Example.class, params);
@@ -962,7 +997,6 @@ Call<ResponseBody> method(@QueryMap(encoded = true) Map<String, Object> query) {
 
     Map<String, Object> params = new LinkedHashMap<>();
     params.put("kit", "k%20t");
-    params.put("foo", null);
     params.put("pi%20ng", "p%20g");
 
     Request request = buildRequest(Example.class, params);
@@ -1420,7 +1454,6 @@ Call<ResponseBody> method(@PartMap Map<String, RequestBody> parts) {
 
     Map<String, RequestBody> params = new LinkedHashMap<>();
     params.put("ping", RequestBody.create(null, "pong"));
-    params.put("foo", null); // Should be skipped.
     params.put("kit", RequestBody.create(null, "kat"));
 
     Request request = buildRequest(Example.class, params);
@@ -1442,8 +1475,6 @@ Call<ResponseBody> method(@PartMap Map<String, RequestBody> parts) {
         .contains("Content-Disposition: form-data;")
         .contains("name=\"kit\"")
         .contains("\r\nkat\r\n--");
-
-    assertThat(bodyString).doesNotContain("name=\"foo\"\r\n");
   }
 
   @Test public void multipartPartMapWithEncoding() throws IOException {
@@ -1457,7 +1488,6 @@ Call<ResponseBody> method(@PartMap(encoding = "8-bit") Map<String, RequestBody>
 
     Map<String, RequestBody> params = new LinkedHashMap<>();
     params.put("ping", RequestBody.create(null, "pong"));
-    params.put("foo", null); // Should be skipped.
     params.put("kit", RequestBody.create(null, "kat"));
 
     Request request = buildRequest(Example.class, params);
@@ -1481,8 +1511,23 @@ Call<ResponseBody> method(@PartMap(encoding = "8-bit") Map<String, RequestBody>
         .contains("name=\"kit\"")
         .contains("Content-Transfer-Encoding: 8-bit")
         .contains("\r\nkat\r\n--");
+  }
 
-    assertThat(bodyString).doesNotContain("name=\"foo\"\r\n");
+  @Test public void multipartPartMapRejectsNull() {
+    class Example {
+      @Multipart //
+      @POST("/foo/bar/") //
+      Call<ResponseBody> method(@PartMap Map<String, RequestBody> parts) {
+        return null;
+      }
+    }
+
+    try {
+      buildRequest(Example.class, new Object[] { null });
+      fail();
+    } catch (IllegalArgumentException e) {
+      assertThat(e).hasMessage("Part map was null.");
+    }
   }
 
   @Test public void multipartPartMapRejectsNullKeys() {
@@ -1506,6 +1551,27 @@ Call<ResponseBody> method(@PartMap Map<String, RequestBody> parts) {
     }
   }
 
+  @Test public void multipartPartMapRejectsNullValues() {
+    class Example {
+      @Multipart //
+      @POST("/foo/bar/") //
+      Call<ResponseBody> method(@PartMap Map<String, RequestBody> parts) {
+        return null;
+      }
+    }
+
+    Map<String, RequestBody> params = new LinkedHashMap<>();
+    params.put("ping", RequestBody.create(null, "pong"));
+    params.put("kit", null);
+
+    try {
+      buildRequest(Example.class, params);
+      fail();
+    } catch (IllegalArgumentException e) {
+      assertThat(e).hasMessage("Part map contained null value for key 'kit'.");
+    }
+  }
+
   @Test public void multipartPartMapMustBeMap() {
     class Example {
       @Multipart //
@@ -1694,13 +1760,29 @@ Call<ResponseBody> method(@FieldMap Map<String, Object> fieldMap) {
 
     Map<String, Object> fieldMap = new LinkedHashMap<>();
     fieldMap.put("kit", "kat");
-    fieldMap.put("foo", null);
     fieldMap.put("ping", "pong");
 
     Request request = buildRequest(Example.class, fieldMap);
     assertBody(request.body(), "kit=kat&ping=pong");
   }
 
+  @Test public void fieldMapRejectsNull() {
+    class Example {
+      @FormUrlEncoded //
+      @POST("/") //
+      Call<ResponseBody> method(@FieldMap Map<String, Object> a) {
+        return null;
+      }
+    }
+
+    try {
+      buildRequest(Example.class, new Object[] { null });
+      fail();
+    } catch (IllegalArgumentException e) {
+      assertThat(e).hasMessage("Field map was null.");
+    }
+  }
+
   @Test public void fieldMapRejectsNullKeys() {
     class Example {
       @FormUrlEncoded //
@@ -1712,7 +1794,6 @@ Call<ResponseBody> method(@FieldMap Map<String, Object> a) {
 
     Map<String, Object> fieldMap = new LinkedHashMap<>();
     fieldMap.put("kit", "kat");
-    fieldMap.put("foo", null);
     fieldMap.put(null, "pong");
 
     try {
@@ -1723,6 +1804,27 @@ Call<ResponseBody> method(@FieldMap Map<String, Object> a) {
     }
   }
 
+  @Test public void fieldMapRejectsNullValues() {
+    class Example {
+      @FormUrlEncoded //
+      @POST("/") //
+      Call<ResponseBody> method(@FieldMap Map<String, Object> a) {
+        return null;
+      }
+    }
+
+    Map<String, Object> fieldMap = new LinkedHashMap<>();
+    fieldMap.put("kit", "kat");
+    fieldMap.put("foo", null);
+
+    try {
+      buildRequest(Example.class, fieldMap);
+      fail();
+    } catch (IllegalArgumentException e) {
+      assertThat(e).hasMessage("Field map contained null value for key 'foo'.");
+    }
+  }
+
   @Test public void fieldMapMustBeAMap() {
     class Example {
       @FormUrlEncoded //