Correctly handle server content negotiation via Accept header. Fixes m…

…icronaut-projects#2743

core/src/main/java/io/micronaut/core/type/DefaultArgument.java

@@ -35,7 +35,7 @@
 @Internal
 public class DefaultArgument<T> implements Argument<T> {
 
-    static final Set<Class> CONTAINER_TYPES = CollectionUtils.setOf(
+    static final Set<Class<?>> CONTAINER_TYPES = CollectionUtils.setOf(
         List.class,
         Set.class,
         Map.class,

function-web/src/main/java/io/micronaut/function/web/AnnotatedFunctionRouteBuilder.java

@@ -159,7 +159,7 @@ public void process(BeanDefinition<?> beanDefinition, ExecutableMethod<?, ?> met
                                     }
                                 } else {
                                     route.body(method.getArgumentNames()[0])
-                                            .acceptAll();
+                                            .consumesAll();
                                 }
                             }
                         }

http-client-core/src/main/java/io/micronaut/http/client/interceptor/HttpClientIntroductionAdvice.java

@@ -528,7 +528,7 @@ protected void doOnComplete() {
             } else {
                 BlockingHttpClient blockingHttpClient = httpClient.toBlocking();
 
-                if (void.class != javaReturnType) {
+                if (void.class != javaReturnType && httpMethod != HttpMethod.HEAD) {
                     request.accept(acceptTypes);
                 }
 

http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java

@@ -1932,6 +1932,9 @@ public void handlerRemoved(ChannelHandlerContext ctx) {
                     } else {
                         channelPool.release(ch);
                     }
+                } else {
+                    // just close it to prevent any future reads without a handler registered
+                    ctx.close();
                 }
             }
 

http-client/src/test/groovy/io/micronaut/http/client/ClientScopeSpec.groovy

@@ -337,7 +337,7 @@ class ClientScopeSpec extends Specification {
     @JacksonFeatures(disabledDeserializationFeatures = DeserializationFeature.ADJUST_DATES_TO_CONTEXT_TIME_ZONE)
     static interface MyServiceJacksonFeatures {
 
-        @Get
+        @Get(consumes = MediaType.TEXT_PLAIN)
         String name()
     }
 

http-client/src/test/groovy/io/micronaut/http/client/HttpGetSpec.groovy

@@ -786,7 +786,7 @@ class HttpGetSpec extends Specification {
     @Client("http://not.used")
     static interface OverrideUrlClient {
 
-        @Get("{+url}/get/simple")
+        @Get(value = "{+url}/get/simple", consumes = MediaType.TEXT_PLAIN)
         String overrideUrl(String url);
 
     }

http-client/src/test/groovy/io/micronaut/http/client/HttpHeadSpec.groovy

@@ -505,7 +505,7 @@ class HttpHeadSpec extends Specification {
         @Head("/dateTimeQuery")
         String formatDateTimeQuery(@QueryValue @Format('yyyy-MM-dd') LocalDate myDate)
 
-        @Head("/no-head")
+        @Head(value = "/no-head")
         String noHead()
 
         @Head("/multiple")

http-client/src/test/groovy/io/micronaut/http/client/HttpPutSpec.groovy

@@ -162,7 +162,7 @@ class HttpPutSpec extends Specification {
         BlockingHttpClient blockingHttpClient = client.toBlocking()
         String result = blockingHttpClient.retrieve(
                 HttpRequest.PUT("/put/optionalJson", [enable: true])
-                        .accept(MediaType.APPLICATION_JSON_TYPE),
+                        .accept(MediaType.TEXT_PLAIN),
 
                 String
         )

http-client/src/test/groovy/io/micronaut/http/client/ReadTimeoutSpec.groovy

@@ -20,6 +20,7 @@ import io.micronaut.http.HttpHeaders
 import io.micronaut.http.HttpRequest
 import io.micronaut.http.HttpResponse
 import io.micronaut.http.MediaType
+import io.micronaut.http.annotation.Consumes
 import io.micronaut.http.annotation.Controller
 import io.micronaut.http.annotation.Get
 import io.micronaut.http.client.annotation.Client
@@ -366,6 +367,7 @@ class ReadTimeoutSpec extends Specification {
     }
 
     @Client("/timeout")
+    @Consumes(MediaType.TEXT_PLAIN)
     static interface TestClient {
         @Get
         String get()

http-client/src/test/groovy/io/micronaut/http/client/aop/ClientFilterSpec.groovy

@@ -117,7 +117,7 @@ class ClientFilterSpec extends Specification{
     @Requires(property = 'spec.name', value = "ClientFilterSpec")
     @Client('/filters')
     static interface MyApi {
-        @Get('/name')
+        @Get(value = '/name', consumes = MediaType.TEXT_PLAIN)
         String name()
     }
 

http-client/src/test/groovy/io/micronaut/http/client/aop/NotFoundSpec.groovy

@@ -17,6 +17,7 @@ package io.micronaut.http.client.aop
 
 import io.micronaut.context.ApplicationContext
 import io.micronaut.http.MediaType
+import io.micronaut.http.annotation.Consumes
 import io.micronaut.http.annotation.Controller
 import io.micronaut.http.annotation.Get
 import io.micronaut.http.client.annotation.Client
@@ -54,6 +55,7 @@ class NotFoundSpec extends Specification {
     @Client('/not-found')
     static interface InventoryClient {
         @Get('/maybe/{isbn}')
+        @Consumes(MediaType.TEXT_PLAIN)
         Maybe<Boolean> maybe(String isbn)
 
         @Get(value = '/flowable/{isbn}', processes = MediaType.TEXT_EVENT_STREAM)

http-client/src/test/groovy/io/micronaut/http/client/convert/DateTimeConversionSpec.groovy

@@ -47,7 +47,7 @@ class DateTimeConversionSpec extends Specification {
     }
 
     static interface TimeApi {
-        @Get(uri = "/offset", produces = MediaType.TEXT_PLAIN)
+        @Get(uri = "/offset", processes = MediaType.TEXT_PLAIN)
         String time(@NotNull @QueryValue OffsetDateTime time)
     }
 

http-client/src/test/resources/logback.xml

@@ -11,7 +11,7 @@
     <root level="info">
         <appender-ref ref="STDOUT" />
     </root>
-    <!--<logger name="io.micronaut.http" level="TRACE"/>-->
+    <logger name="io.micronaut.http" level="TRACE"/>
     <!--<logger name="io.netty.handler.logging" level="TRACE" />-->
     <!--<logger name="io.micronaut.jackson.parser" level="TRACE"/>-->
 </configuration>

http-server-netty/build.gradle

@@ -15,7 +15,10 @@ dependencies {
     testImplementation project(":http-client")
     testImplementation group: 'org.powermock', name: 'powermock-module-junit4', version: '2.0.5'
     testImplementation group: 'org.powermock', name: 'powermock-api-mockito2', version: '2.0.5'
-
+    testImplementation dependencyModuleVersion("micronaut.test", "micronaut-test-spock"), {
+        exclude module:'micronaut-runtime'
+        exclude module:'micronaut-inject'
+    }
     testImplementation dependencyModuleVersion("groovy", "groovy-json")
     testImplementation dependencyModuleVersion("groovy", "groovy-templates")
     testImplementation dependencyVersion("rxjava2")

http-server-netty/src/main/java/io/micronaut/http/server/netty/RoutingInBoundHandler.java

@@ -53,6 +53,7 @@
 import io.micronaut.core.reflect.ClassUtils;
 import io.micronaut.core.type.Argument;
 import io.micronaut.core.type.ReturnType;
+import io.micronaut.core.util.CollectionUtils;
 import io.micronaut.http.HttpHeaders;
 import io.micronaut.http.HttpMethod;
 import io.micronaut.http.HttpRequest;
@@ -494,56 +495,79 @@ protected void channelRead0(ChannelHandlerContext ctx, io.micronaut.http.HttpReq
 
         RouteMatch<?> route;
 
+        final String requestMethodName = request.getMethodName();
         if (routeMatch == null) {
             if (LOG.isDebugEnabled()) {
                 LOG.debug("No matching route found for URI {} and method {}", request.getUri(), httpMethod);
             }
 
-            // if there is no route present try to locate a route that matches a different content type
-            Set<MediaType> existingRouteConsumes = router
-                    .find(httpMethod, requestPath, request)
-                    .map(UriRouteMatch::getRoute)
-                    .flatMap(r -> r.getConsumes().stream())
-                    .collect(Collectors.toSet());
-
-            if (!existingRouteConsumes.isEmpty() && !existingRouteConsumes.contains(MediaType.ALL_TYPE)) {
-                MediaType contentType = request.getContentType().orElse(null);
-                if (contentType != null) {
-                    if (!existingRouteConsumes.contains(contentType)) {
-                        if (LOG.isDebugEnabled()) {
-                            LOG.debug("Content type not allowed for URI {}, method {}, and content type {}", request.getUri(),
-                                    request.getMethodName(), contentType);
-                        }
 
-                        handleStatusError(
-                                ctx,
-                                request,
-                                nettyHttpRequest,
-                                HttpResponse.status(HttpStatus.UNSUPPORTED_MEDIA_TYPE),
-                                "Content Type [" + contentType + "] not allowed. Allowed types: " + existingRouteConsumes);
-                        return;
-                    }
+            // if there is no route present try to locate a route that matches a different HTTP method
+            final List<UriRouteMatch<?, ?>> anyMatchingRoutes = router
+                    .findAny(request.getUri().toString(), request)
+                    .collect(Collectors.toList());
+            MediaType contentType = request.getContentType().orElse(null);
+            final Collection<MediaType> acceptedTypes = request.accept();
+            final boolean hasAcceptHeader = CollectionUtils.isNotEmpty(acceptedTypes);
+
+            Set<MediaType> acceptableContentTypes = contentType != null ? new HashSet<>(5) : null;
+            Set<String> allowedMethods = new HashSet<>(5);
+            Set<MediaType> produceableContentTypes = hasAcceptHeader ? new HashSet<>(5) : null;
+            for (UriRouteMatch<?, ?> anyRoute : anyMatchingRoutes) {
+                final String routeMethod = anyRoute.getRoute().getHttpMethodName();
+                if (!requestMethodName.equals(routeMethod)) {
+                    allowedMethods.add(routeMethod);
+                }
+                if (contentType != null && !anyRoute.doesConsume(contentType)) {
+                    acceptableContentTypes.addAll(anyRoute.getRoute().getConsumes());
+                }
+                if (hasAcceptHeader && !anyRoute.doesProduce(acceptedTypes)) {
+                    produceableContentTypes.addAll(anyRoute.getRoute().getProduces());
                 }
             }
 
-            // if there is no route present try to locate a route that matches a different HTTP method
-            Set<String> existingRouteMethods = router
-                    .findAny(request.getUri().toString(), request)
-                    .map(UriRouteMatch::getRoute)
-                    .map(UriRoute::getHttpMethodName)
-                    .collect(Collectors.toSet());
+            if (CollectionUtils.isNotEmpty(acceptableContentTypes)) {
+                if (LOG.isDebugEnabled()) {
+                    LOG.debug("Content type not allowed for URI {}, method {}, and content type {}", request.getUri(),
+                            requestMethodName, contentType);
+                }
+
+                handleStatusError(
+                        ctx,
+                        request,
+                        nettyHttpRequest,
+                        HttpResponse.status(HttpStatus.UNSUPPORTED_MEDIA_TYPE),
+                        "Content Type [" + contentType + "] not allowed. Allowed types: " + acceptableContentTypes);
+                return;
+            }
+
+            if (CollectionUtils.isNotEmpty(produceableContentTypes)) {
+                if (LOG.isDebugEnabled()) {
+                    LOG.debug("Content type not allowed for URI {}, method {}, and content type {}", request.getUri(),
+                            requestMethodName, contentType);
+                }
+
+                handleStatusError(
+                        ctx,
+                        request,
+                        nettyHttpRequest,
+                        HttpResponse.status(HttpStatus.NOT_ACCEPTABLE),
+                        "Specified Accept Types " + acceptedTypes + " not supported. Supported types: " + produceableContentTypes);
+                return;
+            }
+
+            if (!allowedMethods.isEmpty()) {
 
-            if (!existingRouteMethods.isEmpty()) {
                 if (LOG.isDebugEnabled()) {
-                    LOG.debug("Method not allowed for URI {} and method {}", request.getUri(), request.getMethodName());
+                    LOG.debug("Method not allowed for URI {} and method {}", request.getUri(), requestMethodName);
                 }
 
                 handleStatusError(
                         ctx,
                         request,
                         nettyHttpRequest,
-                        HttpResponse.notAllowedGeneric(existingRouteMethods),
-                        "Method [" + request.getMethodName() + "] not allowed for URI [" + request.getUri() + "]. Allowed methods: " + existingRouteMethods);
+                        HttpResponse.notAllowedGeneric(allowedMethods),
+                        "Method [" + requestMethodName + "] not allowed for URI [" + request.getUri() + "]. Allowed methods: " + allowedMethods);
                 return;
             }
 
@@ -567,9 +591,9 @@ protected void channelRead0(ChannelHandlerContext ctx, io.micronaut.http.HttpReq
 
         if (LOG.isDebugEnabled()) {
             if (route instanceof MethodBasedRouteMatch) {
-                LOG.debug("Matched route {} - {} to controller {}", request.getMethodName(), requestPath, route.getDeclaringType());
+                LOG.debug("Matched route {} - {} to controller {}", requestMethodName, requestPath, route.getDeclaringType());
             } else {
-                LOG.debug("Matched route {} - {}", request.getMethodName(), requestPath);
+                LOG.debug("Matched route {} - {}", requestMethodName, requestPath);
             }
         }
         // all ok proceed to try and execute the route
@@ -597,7 +621,7 @@ private void handleStatusError(
             handleRouteMatch(routeMatch, nettyHttpRequest, ctx);
         } else {
 
-            if (HttpMethod.permitsRequestBody(request.getMethod())) {
+            if (request.getMethod() != HttpMethod.HEAD) {
                 JsonError error = newError(request, message);
                 defaultResponse.body(error);
             }

http-server-netty/src/test/groovy/io/micronaut/http/server/netty/ContentNegotiationSpec.groovy

@@ -0,0 +1,93 @@
+package io.micronaut.http.server.netty
+
+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.MediaType
+import io.micronaut.http.annotation.Controller
+import io.micronaut.http.annotation.Get
+import io.micronaut.http.annotation.Produces
+import io.micronaut.http.client.RxHttpClient
+import io.micronaut.http.client.annotation.Client
+import io.micronaut.http.client.exceptions.HttpClientResponseException
+import io.micronaut.http.hateoas.JsonError
+import io.micronaut.test.annotation.MicronautTest
+import spock.lang.Specification
+import spock.lang.Unroll
+
+import javax.inject.Inject
+import static io.micronaut.http.server.netty.ContentNegotiationSpec.NegotiatingController.*
+
+@MicronautTest
+class ContentNegotiationSpec extends Specification {
+
+    @Inject
+    @Client("/")
+    RxHttpClient client
+
+
+    @Unroll
+    void "test ACCEPT header content negotiation #header"() {
+        expect:
+        client.retrieve(HttpRequest.GET("/negotiate").accept(header as MediaType[]), String)
+                .blockingFirst() == response
+
+        where:
+        header                                                                            | response
+        [MediaType.APPLICATION_GRAPHQL_TYPE]                                              | JSON // should default to the all handler
+        [new MediaType("application/json;q=0.5"), new MediaType("application/xml;q=0.9")] | XML
+        [MediaType.APPLICATION_JSON_TYPE]                                                 | JSON
+        [MediaType.APPLICATION_JSON_TYPE, MediaType.APPLICATION_XML_TYPE]                 | JSON
+        [MediaType.APPLICATION_XML_TYPE, MediaType.APPLICATION_JSON_TYPE]                 | XML
+        [MediaType.APPLICATION_XML_TYPE]                                                  | XML
+        [MediaType.TEXT_PLAIN_TYPE]                                                       | TEXT
+        [MediaType.ALL_TYPE]                                                              | JSON
+
+    }
+
+    void "test send unacceptable type"() {
+        when: "An unacceptable type is sent"
+        client.retrieve(HttpRequest.GET("/negotiate/other")
+                .accept(MediaType.APPLICATION_GRAPHQL), Argument.STRING, JsonError.TYPE)
+                .blockingFirst()
+
+        then: "An exception is thrown that states the acceptable types"
+        def e = thrown(HttpClientResponseException)
+        def response = e.response
+        response.status() == HttpStatus.NOT_ACCEPTABLE
+        response.body().toString().contains("Specified Accept Types [application/graphql] not supported. Supported types: [text/plain]")
+    }
+
+    @Controller("/negotiate")
+    static class NegotiatingController {
+
+        public static final String XML = "<hello>world</hello>"
+        public static final String JSON = '{"hello":"world"}'
+        public static final String TEXT = 'Hello World'
+
+        @Get("/")
+        @Produces(MediaType.APPLICATION_XML)
+        String xml() {
+            return XML
+        }
+
+        @Get("/")
+        @Produces([MediaType.APPLICATION_JSON, MediaType.ALL])
+        String json() {
+            return JSON
+        }
+
+        @Get("/")
+        @Produces(MediaType.TEXT_PLAIN)
+        String text() {
+            return TEXT
+        }
+
+        @Get("/other")
+        @Produces(MediaType.TEXT_PLAIN)
+        String other() {
+            return TEXT
+        }
+    }
+}

http-server-netty/src/test/groovy/io/micronaut/http/server/netty/context/ServerRequestContextSpec.groovy

@@ -19,6 +19,7 @@ import io.micronaut.context.ApplicationContext
 import io.micronaut.http.HttpRequest
 import io.micronaut.http.HttpResponse
 import io.micronaut.http.MediaType
+import io.micronaut.http.annotation.Consumes
 import io.micronaut.http.annotation.Controller
 import io.micronaut.http.annotation.Get
 import io.micronaut.http.annotation.Produces
@@ -72,6 +73,7 @@ class ServerRequestContextSpec extends Specification {
     }
 
     @Client('/test-context')
+    @Consumes(MediaType.TEXT_PLAIN)
     static interface TestClient {
 
         @Get("/method")