From b28f9053fd92535f9d41f2010ead31191e79df36 Mon Sep 17 00:00:00 2001 From: Jake Wharton Date: Thu, 3 Mar 2016 22:28:30 -0500 Subject: [PATCH] Forbid null map values, and null maps. --- .../main/java/retrofit2/RequestAction.java | 27 ++-- .../java/retrofit2/RequestBuilderTest.java | 120 ++++++++++++++++-- 2 files changed, 130 insertions(+), 17 deletions(-) diff --git a/retrofit/src/main/java/retrofit2/RequestAction.java b/retrofit/src/main/java/retrofit2/RequestAction.java index 994920b6fd..870587a051 100644 --- a/retrofit/src/main/java/retrofit2/RequestAction.java +++ b/retrofit/src/main/java/retrofit2/RequestAction.java @@ -133,7 +133,9 @@ static final class QueryMap extends RequestAction> { } @Override void perform(RequestBuilder builder, Map value) throws IOException { - if (value == null) return; // Skip null values. + if (value == null) { + throw new IllegalArgumentException("Query map was null."); + } for (Map.Entry entry : value.entrySet()) { String entryKey = entry.getKey(); @@ -141,9 +143,11 @@ static final class QueryMap extends RequestAction> { 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,7 +179,9 @@ static final class FieldMap extends RequestAction> { } @Override void perform(RequestBuilder builder, Map value) throws IOException { - if (value == null) return; // Skip null values. + if (value == null) { + throw new IllegalArgumentException("Field map was null."); + } for (Map.Entry entry : value.entrySet()) { String entryKey = entry.getKey(); @@ -183,9 +189,11 @@ static final class FieldMap extends RequestAction> { 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 extends RequestAction> { } @Override void perform(RequestBuilder builder, Map value) throws IOException { - if (value == null) return; // Skip null values. + if (value == null) { + throw new IllegalArgumentException("Part map was null."); + } for (Map.Entry entry : value.entrySet()) { String entryKey = entry.getKey(); @@ -231,7 +241,8 @@ static final class PartMap extends RequestAction> { } 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 index e46cfee45f..dea6d33b07 100644 --- a/retrofit/src/test/java/retrofit2/RequestBuilderTest.java +++ b/retrofit/src/test/java/retrofit2/RequestBuilderTest.java @@ -459,6 +459,22 @@ Call method(@QueryMap Foo a) { assertThat(request.url().toString()).isEqualTo("http://example.com/?hello=world"); } + @Test public void queryMapRejectsNull() { + class Example { + @GET("/") // + Call method(@QueryMap Map 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 method(@QueryMap Map a) { } } + @Test public void queryMapRejectsNullValues() { + class Example { + @GET("/") // + Call method(@QueryMap Map a) { + return null; + } + } + + Map 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 method(@QueryMap Map query) { Map 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 method(@QueryMap(encoded = true) Map query) { Map 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 method(@PartMap Map parts) { Map 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 method(@PartMap Map 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 method(@PartMap(encoding = "8-bit") Map Map 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 method(@PartMap(encoding = "8-bit") Map .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 method(@PartMap Map 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 method(@PartMap Map parts) { } } + @Test public void multipartPartMapRejectsNullValues() { + class Example { + @Multipart // + @POST("/foo/bar/") // + Call method(@PartMap Map parts) { + return null; + } + } + + Map 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 method(@FieldMap Map fieldMap) { Map 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 method(@FieldMap Map 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 method(@FieldMap Map a) { Map fieldMap = new LinkedHashMap<>(); fieldMap.put("kit", "kat"); - fieldMap.put("foo", null); fieldMap.put(null, "pong"); try { @@ -1723,6 +1804,27 @@ Call method(@FieldMap Map a) { } } + @Test public void fieldMapRejectsNullValues() { + class Example { + @FormUrlEncoded // + @POST("/") // + Call method(@FieldMap Map a) { + return null; + } + } + + Map 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 //