Fix NPE in OptionalDecoder (#788)

The decoded instance (from delegate.decode()) will be null when a
response body is null. This decoded instance (null) will result in a NPE
in the Optional since Optional.of is used. Changing this to
Optional.ofNullable will return an empty Optional when the decoded
instance is null.

core/src/main/java/feign/optionals/OptionalDecoder.java

@@ -39,7 +39,7 @@ public Object decode(Response response, Type type) throws IOException {
       return Optional.empty();
     }
     Type enclosedType = Util.resolveLastTypeParameter(type, Optional.class);
-    return Optional.of(delegate.decode(response, enclosedType));
+    return Optional.ofNullable(delegate.decode(response, enclosedType));
   }
 
   static boolean isOptional(Type type) {

core/src/test/java/feign/optionals/OptionalDecoderTests.java

@@ -27,7 +27,10 @@ public class OptionalDecoderTests {
 
   interface OptionalInterface {
     @RequestLine("GET /")
-    Optional<String> get();
+    Optional<String> getAsOptional();
+
+    @RequestLine("GET /")
+    String get();
   }
 
   @Test
@@ -41,8 +44,8 @@ public void simple404OptionalTest() throws IOException, InterruptedException {
         .decoder(new OptionalDecoder(new Decoder.Default()))
         .target(OptionalInterface.class, server.url("/").toString());
 
-    assertThat(api.get().isPresent()).isFalse();
-    assertThat(api.get().get()).isEqualTo("foo");
+    assertThat(api.getAsOptional().isPresent()).isFalse();
+    assertThat(api.getAsOptional().get()).isEqualTo("foo");
   }
 
   @Test
@@ -54,6 +57,45 @@ public void simple204OptionalTest() throws IOException, InterruptedException {
         .decoder(new OptionalDecoder(new Decoder.Default()))
         .target(OptionalInterface.class, server.url("/").toString());
 
-    assertThat(api.get().isPresent()).isFalse();
+    assertThat(api.getAsOptional().isPresent()).isFalse();
+  }
+
+  @Test
+  public void test200WithOptionalString() throws IOException, InterruptedException {
+    final MockWebServer server = new MockWebServer();
+    server.enqueue(new MockResponse().setResponseCode(200).setBody("foo"));
+
+    final OptionalInterface api = Feign.builder()
+        .decoder(new OptionalDecoder(new Decoder.Default()))
+        .target(OptionalInterface.class, server.url("/").toString());
+
+    Optional<String> response = api.getAsOptional();
+
+    assertThat(response.isPresent()).isTrue();
+    assertThat(response).isEqualTo(Optional.of("foo"));
+  }
+
+  @Test
+  public void test200WhenResponseBodyIsNull() throws IOException, InterruptedException {
+    final MockWebServer server = new MockWebServer();
+    server.enqueue(new MockResponse().setResponseCode(200));
+
+    final OptionalInterface api = Feign.builder()
+        .decoder(new OptionalDecoder(((response, type) -> null)))
+        .target(OptionalInterface.class, server.url("/").toString());
+
+    assertThat(api.getAsOptional().isPresent()).isFalse();
+  }
+
+  @Test
+  public void test200WhenDecodingNoOptional() throws IOException, InterruptedException {
+    final MockWebServer server = new MockWebServer();
+    server.enqueue(new MockResponse().setResponseCode(200).setBody("foo"));
+
+    final OptionalInterface api = Feign.builder()
+        .decoder(new OptionalDecoder(new Decoder.Default()))
+        .target(OptionalInterface.class, server.url("/").toString());
+
+    assertThat(api.get()).isEqualTo("foo");
   }
 }