Fix misleading error 'empty body' for responses with a body that coul…

…d not be decoded. (micronaut-projects#4677) * Fix misleading error 'empty body' for reponses with a body that could not be decoded. Fixes micronaut-projects#4613 * Look for the bytes in the response to determine if a body was sent. Fix tests * Set the body bytes even if there is no media type to handle the response * Remove redundant variable
glorrian · Dec 9, 2020 · ab5c4a2 · ab5c4a2
1 parent adbdc9a
commit ab5c4a2
Show file tree

Hide file tree

Showing 5 changed files with 74 additions and 20 deletions.
diff --git a/http-client-core/src/main/java/io/micronaut/http/client/BlockingHttpClient.java b/http-client-core/src/main/java/io/micronaut/http/client/BlockingHttpClient.java
@@ -16,13 +16,15 @@
 package io.micronaut.http.client;
 
 import io.micronaut.core.annotation.Blocking;
+import io.micronaut.core.io.buffer.ByteBuffer;
 import io.micronaut.core.type.Argument;
 import io.micronaut.http.HttpRequest;
 import io.micronaut.http.HttpResponse;
 import io.micronaut.http.HttpStatus;
 import io.micronaut.http.client.exceptions.HttpClientResponseException;
 
 import java.io.Closeable;
+import java.util.Optional;
 
 /**
  * A blocking HTTP client interface that features a subset of the operations provided by {@link HttpClient} and
@@ -140,12 +142,18 @@ default <I, O, E> O retrieve(HttpRequest<I> request, Argument<O> bodyType, Argum
         if (HttpStatus.class.isAssignableFrom(bodyType.getType())) {
             return (O) response.getStatus();
         } else {
-            return response
-                    .getBody()
-                    .orElseThrow(() -> new HttpClientResponseException(
-                            "Empty body",
-                            response
-                    ));
+            Optional<O> body = response.getBody();
+            if (!body.isPresent() && response.getBody(Argument.of(byte[].class)).isPresent()) {
+                throw new HttpClientResponseException(
+                        String.format("Failed to decode the body for the given content type [%s]", response.getContentType().orElse(null)),
+                        response
+                );
+            } else {
+                return body.orElseThrow(() -> new HttpClientResponseException(
+                        "Empty body",
+                        response
+                ));
+            }
         }
     }
 

diff --git a/http-client-core/src/main/java/io/micronaut/http/client/HttpClient.java b/http-client-core/src/main/java/io/micronaut/http/client/HttpClient.java
@@ -157,11 +157,17 @@ default <I, O, E> Publisher<O> retrieve(HttpRequest<I> request, Argument<O> body
                 return (O) response.getStatus();
             } else {
                 Optional<O> body = response.getBody();
-                return body
-                        .orElseThrow(() -> new HttpClientResponseException(
-                                "Empty body",
-                                response
-                        ));
+                if (!body.isPresent() && response.getBody(byte[].class).isPresent()) {
+                    throw new HttpClientResponseException(
+                            String.format("Failed to decode the body for the given content type [%s]", response.getContentType().orElse(null)),
+                            response
+                    );
+                } else {
+                    return body.orElseThrow(() -> new HttpClientResponseException(
+                            "Empty body",
+                            response
+                    ));
+                }
             }
         });
     }

diff --git a/http-client/src/main/java/io/micronaut/http/client/netty/FullNettyClientHttpResponse.java b/http-client/src/main/java/io/micronaut/http/client/netty/FullNettyClientHttpResponse.java
@@ -267,18 +267,16 @@ private <T> Optional convertByteBuf(ByteBuf content, Argument<T> type) {
         Optional<MediaType> contentType = getContentType();
         if (contentType.isPresent()) {
             if (mediaTypeCodecRegistry != null) {
+                bodyBytes = ByteBufUtil.getBytes(content);
                 if (CharSequence.class.isAssignableFrom(type.getType())) {
                     Charset charset = contentType.flatMap(MediaType::getCharset).orElse(StandardCharsets.UTF_8);
-                    bodyBytes = ByteBufUtil.getBytes(content);
                     return Optional.of(new String(bodyBytes, charset));
                 } else if (type.getType() == byte[].class) {
-                    bodyBytes = ByteBufUtil.getBytes(content);
                     return Optional.of(bodyBytes);
                 } else {
                     Optional<MediaTypeCodec> foundCodec = mediaTypeCodecRegistry.findCodec(contentType.get());
                     if (foundCodec.isPresent()) {
                         MediaTypeCodec codec = foundCodec.get();
-                        bodyBytes = ByteBufUtil.getBytes(content);
                         return Optional.of(codec.decode(type, bodyBytes));
                     }
                 }

diff --git a/http-client/src/test/groovy/io/micronaut/http/client/HttpGetSpec.groovy b/http-client/src/test/groovy/io/micronaut/http/client/HttpGetSpec.groovy
@@ -588,6 +588,24 @@ class HttpGetSpec extends Specification {
         client.close()
     }
 
+    void "test an invalid content type"() {
+        when:
+        myGetClient.invalidContentType()
+
+        then:
+        def ex = thrown(HttpClientResponseException)
+        ex.message == "Failed to decode the body for the given content type [does/notexist]"
+    }
+
+    void "test an invalid content type reactive response"() {
+        when:
+        myGetClient.invalidContentTypeReactive().blockingGet()
+
+        then:
+        def ex = thrown(HttpClientResponseException)
+        ex.message == "Failed to decode the body for the given content type [does/notexist]"
+    }
+
     @Controller("/get")
     static class GetController {
 
@@ -705,6 +723,11 @@ class HttpGetSpec extends Specification {
         String multipleMappings() {
             return "multiple mappings"
         }
+
+        @Get(value = "/invalidContentType", produces = "does/notexist")
+        String invalidContentType() {
+            return "hello"
+        }
     }
 
 
@@ -874,6 +897,13 @@ class HttpGetSpec extends Specification {
 
         @Get("/multiple/mappings")
         String multipleMappings()
+
+        @Get(value = "/invalidContentType", consumes = "does/notexist")
+        Book invalidContentType()
+
+        @Get(value = "/invalidContentType", consumes = "does/notexist")
+        Single<Book> invalidContentTypeReactive()
+
     }
 
     @Client("http://not.used")

diff --git a/http-client/src/test/groovy/io/micronaut/http/client/HttpHeadSpec.groovy b/http-client/src/test/groovy/io/micronaut/http/client/HttpHeadSpec.groovy
@@ -15,9 +15,7 @@
  */
 package io.micronaut.http.client
 
-import io.micronaut.context.ApplicationContext
 import io.micronaut.core.convert.format.Format
-import io.micronaut.core.io.buffer.ByteBuffer
 import io.micronaut.core.type.Argument
 import io.micronaut.http.HttpRequest
 import io.micronaut.http.HttpResponse
@@ -28,15 +26,13 @@ import io.micronaut.http.annotation.Get
 import io.micronaut.http.annotation.Head
 import io.micronaut.http.annotation.Header
 import io.micronaut.http.annotation.QueryValue
+import io.micronaut.http.annotation.Status
 import io.micronaut.http.client.annotation.Client
 import io.micronaut.http.client.exceptions.HttpClientResponseException
-import io.micronaut.runtime.server.EmbeddedServer
-import io.micronaut.test.annotation.MicronautTest
+import io.micronaut.test.extensions.spock.annotation.MicronautTest
 import io.reactivex.Flowable
 import io.reactivex.Single
 import io.reactivex.functions.Consumer
-import spock.lang.AutoCleanup
-import spock.lang.Shared
 import spock.lang.Specification
 import spock.util.concurrent.PollingConditions
 
@@ -329,6 +325,15 @@ class HttpHeadSpec extends Specification {
         ex.message == "Empty body"
     }
 
+    void "test no body returned"() {
+        when:
+        myGetClient.noContent()
+
+        then:
+        def ex = thrown(HttpClientResponseException)
+        ex.message == "Empty body"
+    }
+
     void "test a request with a custom host header"() {
         when:
         String body = client.toBlocking().retrieve(
@@ -478,6 +483,10 @@ class HttpHeadSpec extends Specification {
         HttpResponse multipleMappings() {
             return HttpResponse.ok().header("X-Test", "multiple mappings")
         }
+
+        @Head("/no-content")
+        @Status(HttpStatus.NO_CONTENT)
+        void noContent() {}
     }
 
     static class Book {
@@ -531,6 +540,9 @@ class HttpHeadSpec extends Specification {
 
         @Head("/multiple/mappings")
         HttpResponse multipleMappings()
+
+        @Head("/no-content")
+        String noContent()
     }
 
     @javax.inject.Singleton