Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

If no Accept header is set default to the first 'produces' type #11014

Merged
merged 1 commit into from
Jul 29, 2020
+77 −23
Merged

If no Accept header is set default to the first 'produces' type #11014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion extensions/vertx-web/deployment/src/main/java/io/quarkus/vertx/web/deployment/Methods.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ class Methods {
static final MethodDescriptor OBJECT_CONSTRUCTOR = MethodDescriptor.ofConstructor(Object.class);

static final MethodDescriptor ROUTE_HANDLERS_SET_CONTENT_TYPE = MethodDescriptor
.ofMethod(RouteHandlers.class, "setContentType", void.class, RoutingContext.class);
.ofMethod(RouteHandlers.class, "setContentType", void.class, RoutingContext.class, String.class);

static final MethodDescriptor OPTIONAL_OF_NULLABLE = MethodDescriptor
.ofMethod(Optional.class, "ofNullable", Optional.class, Object.class);
Expand Down
34 changes: 19 additions & 15 deletions ...vertx-web/deployment/src/main/java/io/quarkus/vertx/web/deployment/VertxWebProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,14 +232,6 @@ void addAdditionalRoutes(
for (AnnotationInstance route : businessMethod.getRoutes()) {
String routeString = route.toString(true);
Handler<RoutingContext> routeHandler = routeHandlers.get(routeString);
if (routeHandler == null) {
String handlerClass = generateHandler(new HandlerDescriptor(businessMethod.getMethod()),
businessMethod.getBean(), businessMethod.getMethod(), classOutput, transformedAnnotations,
routeString, reflectiveHierarchy);
reflectiveClasses.produce(new ReflectiveClassBuildItem(false, false, handlerClass));
routeHandler = recorder.createHandler(handlerClass);
routeHandlers.put(routeString, routeHandler);
}

AnnotationValue regexValue = route.value(VALUE_REGEX);
AnnotationValue pathValue = route.value(VALUE_PATH);
Expand All @@ -252,6 +244,7 @@ void addAdditionalRoutes(
String regex = null;
String[] produces = producesValue.asStringArray();
String[] consumes = consumesValue.asStringArray();

HttpMethod[] methods = Arrays.stream(methodsValue.asEnumArray()).map(HttpMethod::valueOf)
.toArray(HttpMethod[]::new);
Integer order = orderValue.asInt();
Expand Down Expand Up @@ -287,6 +280,15 @@ void addAdditionalRoutes(
consumes = baseConsumes;
}

if (routeHandler == null) {
String handlerClass = generateHandler(new HandlerDescriptor(businessMethod.getMethod()),
businessMethod.getBean(), businessMethod.getMethod(), classOutput, transformedAnnotations,
routeString, reflectiveHierarchy, produces.length > 0 ? produces[0] : null);
reflectiveClasses.produce(new ReflectiveClassBuildItem(false, false, handlerClass));
routeHandler = recorder.createHandler(handlerClass);
routeHandlers.put(routeString, routeHandler);
}

RouteMatcher matcher = new RouteMatcher(path, regex, produces, consumes, methods, order);
matchers.put(matcher, businessMethod.getMethod());
Function<Router, io.vertx.ext.web.Route> routeFunction = recorder.createRouteFunction(matcher,
Expand Down Expand Up @@ -316,7 +318,7 @@ void addAdditionalRoutes(
for (AnnotatedRouteFilterBuildItem filterMethod : routeFilterBusinessMethods) {
String handlerClass = generateHandler(new HandlerDescriptor(filterMethod.getMethod()),
filterMethod.getBean(), filterMethod.getMethod(), classOutput, transformedAnnotations,
filterMethod.getRouteFilter().toString(true), reflectiveHierarchy);
filterMethod.getRouteFilter().toString(true), reflectiveHierarchy, null);
reflectiveClasses.produce(new ReflectiveClassBuildItem(false, false, handlerClass));
Handler<RoutingContext> routingHandler = recorder.createHandler(handlerClass);
AnnotationValue priorityValue = filterMethod.getRouteFilter().value();
Expand Down Expand Up @@ -408,7 +410,7 @@ private void validateRouteMethod(BeanInfo bean, MethodInfo method, TransformedAn

private String generateHandler(HandlerDescriptor desc, BeanInfo bean, MethodInfo method, ClassOutput classOutput,
TransformedAnnotationsBuildItem transformedAnnotations, String hashSuffix,
BuildProducer<ReflectiveHierarchyBuildItem> reflectiveHierarchy) {
BuildProducer<ReflectiveHierarchyBuildItem> reflectiveHierarchy, String defaultProduces) {

String baseName;
if (bean.getImplClazz().enclosingClass() != null) {
Expand Down Expand Up @@ -446,7 +448,7 @@ private String generateHandler(HandlerDescriptor desc, BeanInfo bean, MethodInfo

implementConstructor(bean, invokerCreator, beanField, contextField, containerField);
implementInvoke(desc, bean, method, invokerCreator, beanField, contextField, containerField, transformedAnnotations,
reflectiveHierarchy);
reflectiveHierarchy, defaultProduces);

invokerCreator.close();
return generatedName.replace('/', '.');
Expand Down Expand Up @@ -482,7 +484,7 @@ void implementConstructor(BeanInfo bean, ClassCreator invokerCreator, FieldCreat
void implementInvoke(HandlerDescriptor descriptor, BeanInfo bean, MethodInfo method, ClassCreator invokerCreator,
FieldCreator beanField, FieldCreator contextField, FieldCreator containerField,
TransformedAnnotationsBuildItem transformedAnnotations,
BuildProducer<ReflectiveHierarchyBuildItem> reflectiveHierarchy) {
BuildProducer<ReflectiveHierarchyBuildItem> reflectiveHierarchy, String defaultProduces) {
// The descriptor is: void invoke(RoutingContext rc)
MethodCreator invoke = invokerCreator.getMethodCreator("invoke", void.class, RoutingContext.class);
ResultHandle beanHandle = invoke.readInstanceField(beanField.getFieldDescriptor(), invoke.getThis());
Expand Down Expand Up @@ -544,12 +546,14 @@ void implementInvoke(HandlerDescriptor descriptor, BeanInfo bean, MethodInfo met
MethodDescriptor methodDescriptor = MethodDescriptor
.ofMethod(bean.getImplClazz().name().toString(), method.name(), returnType, parameterTypes);

// If no content-type header is set then try to use the most acceptable content type
// the business method can override this manually if required
invoke.invokeStaticMethod(Methods.ROUTE_HANDLERS_SET_CONTENT_TYPE, routingContext,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we should only set the content-type header if the business method did not set it manually, i.e. after it was executed. If we set it before then multiple content-type headers may be set to the response.

defaultProduces == null ? invoke.loadNull() : invoke.load(defaultProduces));

// Invoke the business method handler
ResultHandle res = invoke.invokeVirtualMethod(methodDescriptor, beanInstanceHandle, paramHandles);

// If no content-type header is set then try to use the most acceptable content type
invoke.invokeStaticMethod(Methods.ROUTE_HANDLERS_SET_CONTENT_TYPE, routingContext);

// Get the response: HttpServerResponse response = rc.response()
MethodDescriptor end = Methods.getEndMethodForContentType(descriptor);
if (descriptor.isReturningUni()) {
Expand Down
38 changes: 38 additions & 0 deletions extensions/vertx-web/deployment/src/test/java/io/quarkus/vertx/web/mutiny/SyncRouteTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,26 @@
import static io.restassured.RestAssured.when;
import static org.hamcrest.Matchers.*;

import java.net.URL;

import javax.json.Json;
import javax.json.JsonObject;
import javax.json.stream.JsonParser;

import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.impl.client.CloseableHttpClient;
import org.apache.http.impl.client.HttpClientBuilder;
import org.jboss.shrinkwrap.api.ShrinkWrap;
import org.jboss.shrinkwrap.api.spec.JavaArchive;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.test.QuarkusUnitTest;
import io.quarkus.test.common.http.TestHTTPResource;
import io.quarkus.vertx.web.Route;
import io.quarkus.vertx.web.RoutingExchange;
import io.vertx.core.buffer.Buffer;
import io.vertx.ext.web.RoutingContext;

Expand All @@ -19,6 +32,9 @@ public class SyncRouteTest {
static final QuarkusUnitTest config = new QuarkusUnitTest()
.setArchiveProducer(() -> ShrinkWrap.create(JavaArchive.class).addClasses(SimpleBean.class));

@TestHTTPResource
URL url;

@Test
public void testSynchronousRoute() {
when().get("hello-sync").then().statusCode(200)
Expand Down Expand Up @@ -49,6 +65,21 @@ public void testSynchronousRoute() {
.body(containsString("boom"));
}

//https://github.com/quarkusio/quarkus/issues/10960
@Test
public void testNoAcceptHeaderContentType() throws Exception {
//RESTAssured always sets an Accept header
try (CloseableHttpClient client = HttpClientBuilder.create().build()) {
CloseableHttpResponse res = client.execute(new HttpGet(this.url.toExternalForm() + "/default-content-type"));
JsonParser parser = Json.createParser(res.getEntity().getContent());
parser.next();
JsonObject obj = parser.getObject();
Assertions.assertEquals("neo", obj.getString("name"));
Assertions.assertEquals(12345, obj.getInt("id"));
Assertions.assertEquals("application/json", res.getFirstHeader("content-type").getValue());
}
}

static class SimpleBean {

@Route(path = "hello-sync")
Expand Down Expand Up @@ -87,6 +118,13 @@ Person getPersonUtf8(RoutingContext context) {
return new Person("neo", 12345);
}

@Route(path = "default-content-type", produces = "application/json")
void hi(RoutingExchange routing) {
routing.ok(new io.vertx.core.json.JsonObject()
.put("name", "neo")
.put("id", 12345).encode());
}

}

static class Person {
Expand Down
5 changes: 4 additions & 1 deletion extensions/vertx-web/runtime/src/main/java/io/quarkus/vertx/web/Route.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,14 +110,17 @@
/**
* If set to a positive number, it indicates the place of the route in the chain.
*
* @see io.vertx.ext.web.Route#order()
* @see io.vertx.ext.web.Route#order(int)
*/
int order() default 0;

/**
* Used for content-based routing.
* <p>
* If no {@code Content-Type} header is set then try to use the most acceptable content type.
*
* If the request does not contain an 'Accept' header and no content type is explicitly set in the
* handler then the content type will be set to the first content type in the array.
*
* @see io.vertx.ext.web.Route#produces(String)
* @see RoutingContext#getAcceptableContentType()
Expand Down
21 changes: 15 additions & 6 deletions extensions/vertx-web/runtime/src/main/java/io/quarkus/vertx/web/runtime/RouteHandlers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import io.quarkus.arc.Arc;
import io.quarkus.arc.impl.LazyValue;
import io.quarkus.security.identity.SecurityIdentity;
import io.vertx.core.Handler;
import io.vertx.core.http.HttpServerResponse;
import io.vertx.ext.web.RoutingContext;

Expand All @@ -18,14 +19,22 @@ private RouteHandlers() {
private static final LazyValue<Event<SecurityIdentity>> SECURITY_IDENTITY_EVENT = new LazyValue<>(
RouteHandlers::createEvent);

public static void setContentType(RoutingContext context) {
public static void setContentType(RoutingContext context, String defaultContentType) {
HttpServerResponse response = context.response();
if (response.headers().get(CONTENT_TYPE) == null) {
String acceptableContentType = context.getAcceptableContentType();
if (acceptableContentType != null) {
response.putHeader(CONTENT_TYPE, acceptableContentType);
context.addHeadersEndHandler(new Handler<Void>() {
@Override
public void handle(Void aVoid) {
//use a listener to set the content type if it has not been set
if (response.headers().get(CONTENT_TYPE) == null) {
String acceptableContentType = context.getAcceptableContentType();
if (acceptableContentType != null) {
response.putHeader(CONTENT_TYPE, acceptableContentType);
} else if (defaultContentType != null) {
response.putHeader(CONTENT_TYPE, defaultContentType);
}
}
}
}
});
}

static void fireSecurityIdentity(SecurityIdentity identity) {
Expand Down