Skip to content

Commit

Permalink
Fail when the request is with the incorrect type (#8261)
Browse files Browse the repository at this point in the history
  • Loading branch information
@dstepanov
dstepanov authored Nov 4, 2022
1 parent 4ec18d8 commit 332d794
Show file tree
Hide file tree
Showing 22 changed files with 279 additions and 226 deletions.
1 change: 1 addition & 0 deletions buildSrc/build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ plugins {
}

repositories {
mavenCentral()
gradlePluginPortal()
}

Expand Down
9 changes: 4 additions & 5 deletions http-client/src/main/java/io/micronaut/http/client/netty/MutableHttpRequestWrapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@
import io.micronaut.core.annotation.Internal;
import io.micronaut.core.annotation.NonNull;
import io.micronaut.core.annotation.Nullable;
import io.micronaut.core.convert.ArgumentConversionContext;
import io.micronaut.core.convert.ConversionContext;
import io.micronaut.core.convert.ConversionService;
import io.micronaut.core.type.Argument;
import io.micronaut.http.HttpRequest;
import io.micronaut.http.HttpRequestWrapper;
import io.micronaut.http.MutableHttpHeaders;
Expand Down Expand Up @@ -79,13 +79,12 @@ public <T> Optional<T> getBody(@NonNull Class<T> type) {
}
}

@NonNull
@Override
public <T> Optional<T> getBody(@NonNull Argument<T> type) {
public <T> Optional<T> getBody(ArgumentConversionContext<T> conversionContext) {
if (body == null) {
return getDelegate().getBody(type);
return getDelegate().getBody(conversionContext);
} else {
return conversionService.convert(body, ConversionContext.of(type));
return conversionService.convert(body, conversionContext);
}
}

Expand Down
6 changes: 3 additions & 3 deletions http-client/src/main/java/io/micronaut/http/client/netty/NettyClientHttpRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

import io.micronaut.core.annotation.Internal;
import io.micronaut.core.annotation.NonNull;
import io.micronaut.core.convert.ConversionContext;
import io.micronaut.core.convert.ArgumentConversionContext;
import io.micronaut.core.convert.ConversionService;
import io.micronaut.core.convert.value.MutableConvertibleValues;
import io.micronaut.core.convert.value.MutableConvertibleValuesMap;
Expand Down Expand Up @@ -175,8 +175,8 @@ public <T> Optional<T> getBody(Class<T> type) {
}

@Override
public <T> Optional<T> getBody(Argument<T> type) {
return getBody().flatMap(b -> conversionService.convert(b, ConversionContext.of(type)));
public <T> Optional<T> getBody(ArgumentConversionContext<T> conversionContext) {
return getBody().flatMap(b -> conversionService.convert(b, conversionContext));
}

@Override
Expand Down
38 changes: 22 additions & 16 deletions http-netty/src/main/java/io/micronaut/http/netty/NettyMutableHttpResponse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import io.micronaut.core.annotation.TypeHint;
import io.micronaut.core.async.publisher.Publishers;
import io.micronaut.core.convert.ArgumentConversionContext;
import io.micronaut.core.convert.ConversionContext;
import io.micronaut.core.convert.ConversionService;
import io.micronaut.core.convert.value.MutableConvertibleValues;
import io.micronaut.core.convert.value.MutableConvertibleValuesMap;
Expand Down Expand Up @@ -247,10 +246,9 @@ public <T1> Optional<T1> getBody(Class<T1> type) {
return getBody(Argument.of(type));
}

@SuppressWarnings("unchecked")
@Override
public <T> Optional<T> getBody(Argument<T> type) {
return bodyConvertor.convert(type, body);
public <T> Optional<T> getBody(ArgumentConversionContext<T> conversionContext) {
return bodyConvertor.convert(conversionContext, body);
}

@Override
Expand Down Expand Up @@ -355,14 +353,14 @@ private BodyConvertor newBodyConvertor() {
return new BodyConvertor() {

@Override
public Optional convert(Argument valueType, Object value) {
public Optional convert(ArgumentConversionContext conversionContext, Object value) {
if (value == null) {
return Optional.empty();
}
if (Argument.OBJECT_ARGUMENT.equalsType(valueType)) {
if (Argument.OBJECT_ARGUMENT.equalsType(conversionContext.getArgument())) {
return Optional.of(value);
}
return convertFromNext(conversionService, valueType, value);
return convertFromNext(conversionService, conversionContext, value);
}

};
Expand All @@ -372,31 +370,39 @@ private abstract static class BodyConvertor<T> {

private BodyConvertor<T> nextConvertor;

public abstract Optional<T> convert(Argument<T> valueType, T value);
public abstract Optional<T> convert(ArgumentConversionContext<T> valueType, T value);

protected synchronized Optional<T> convertFromNext(ConversionService conversionService, Argument<T> conversionValueType, T value) {
protected synchronized Optional<T> convertFromNext(ConversionService conversionService, ArgumentConversionContext<T> conversionContext, T value) {
if (nextConvertor == null) {
Optional<T> conversion;
ArgumentConversionContext<T> context = ConversionContext.of(conversionValueType);
if (value instanceof ByteBuffer) {
conversion = conversionService.convert(((ByteBuffer) value).asNativeBuffer(), context);
conversion = conversionService.convert(((ByteBuffer) value).asNativeBuffer(), conversionContext);
} else {
conversion = conversionService.convert(value, context);
conversion = conversionService.convert(value, conversionContext);
}
nextConvertor = new BodyConvertor<T>() {

@Override
public Optional<T> convert(Argument<T> valueType, T value) {
if (conversionValueType.equalsType(valueType)) {
public Optional<T> convert(ArgumentConversionContext<T> currentConversionContext, T value) {
if (currentConversionContext == conversionContext) {
return conversion;
}
return convertFromNext(conversionService, valueType, value);
if (currentConversionContext.getArgument().equalsType(conversionContext.getArgument())) {
conversionContext.getLastError().ifPresent(error -> {
error.getOriginalValue().ifPresentOrElse(
originalValue -> currentConversionContext.reject(originalValue, error.getCause()),
() -> currentConversionContext.reject(error.getCause())
);
});
return conversion;
}
return convertFromNext(conversionService, currentConversionContext, value);
}

};
return conversion;
}
return nextConvertor.convert(conversionValueType, value);
return nextConvertor.convert(conversionContext, value);
}

public void cleanup() {
Expand Down
36 changes: 22 additions & 14 deletions http-server-netty/src/main/java/io/micronaut/http/server/netty/NettyHttpRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import io.micronaut.core.annotation.NonNull;
import io.micronaut.core.annotation.Nullable;
import io.micronaut.core.async.publisher.Publishers;
import io.micronaut.core.convert.ConversionContext;
import io.micronaut.core.convert.ArgumentConversionContext;
import io.micronaut.core.convert.ConversionService;
import io.micronaut.core.convert.value.MutableConvertibleValues;
import io.micronaut.core.convert.value.MutableConvertibleValuesMap;
Expand Down Expand Up @@ -345,16 +345,15 @@ private String getContent(HttpData data) {
return newValue;
}

@SuppressWarnings("unchecked")
@Override
public <T1> Optional<T1> getBody(Class<T1> type) {
return getBody(Argument.of(type));
}

@SuppressWarnings("unchecked")
@Override
public <T1> Optional<T1> getBody(Argument<T1> type) {
return getBody().flatMap(t -> bodyConvertor.convert(type, t));
public <T1> Optional<T1> getBody(ArgumentConversionContext<T1> conversionContext) {
return getBody().flatMap(t -> bodyConvertor.convert(conversionContext, t));
}

/**
Expand Down Expand Up @@ -611,14 +610,14 @@ private BodyConvertor newBodyConvertor() {
return new BodyConvertor() {

@Override
public Optional convert(Argument valueType, Object value) {
public Optional convert(ArgumentConversionContext conversionContext, Object value) {
if (value == null) {
return Optional.empty();
}
if (Argument.OBJECT_ARGUMENT.equalsType(valueType)) {
if (Argument.OBJECT_ARGUMENT.equalsType(conversionContext.getArgument())) {
return Optional.of(value);
}
return convertFromNext(conversionService, valueType, value);
return convertFromNext(conversionService, conversionContext, value);
}

};
Expand Down Expand Up @@ -786,25 +785,34 @@ private abstract static class BodyConvertor<T> {

private BodyConvertor<T> nextConvertor;

public abstract Optional<T> convert(Argument<T> valueType, T value);
public abstract Optional<T> convert(ArgumentConversionContext<T> conversionContext, T value);

protected synchronized Optional<T> convertFromNext(ConversionService conversionService, Argument<T> conversionValueType, T value) {
protected synchronized Optional<T> convertFromNext(ConversionService conversionService, ArgumentConversionContext<T> conversionContext, T value) {
if (nextConvertor == null) {
Optional<T> conversion = conversionService.convert(value, ConversionContext.of(conversionValueType));
Optional<T> conversion = conversionService.convert(value, conversionContext);
nextConvertor = new BodyConvertor<T>() {

@Override
public Optional<T> convert(Argument<T> valueType, T value) {
if (conversionValueType.equalsType(valueType)) {
public Optional<T> convert(ArgumentConversionContext<T> currentConversionContext, T value) {
if (currentConversionContext == conversionContext) {
return conversion;
}
if (currentConversionContext.getArgument().equalsType(conversionContext.getArgument())) {
conversionContext.getLastError().ifPresent(error -> {
error.getOriginalValue().ifPresentOrElse(
originalValue -> currentConversionContext.reject(originalValue, error.getCause()),
() -> currentConversionContext.reject(error.getCause())
);
});
return conversion;
}
return convertFromNext(conversionService, valueType, value);
return convertFromNext(conversionService, currentConversionContext, value);
}

};
return conversion;
}
return nextConvertor.convert(conversionValueType, value);
return nextConvertor.convert(conversionContext, value);
}

public void cleanup() {
Expand Down
43 changes: 33 additions & 10 deletions http-server-netty/src/main/java/io/micronaut/http/server/netty/RoutingInBoundHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
import io.micronaut.http.server.RouteExecutor;
import io.micronaut.http.server.binding.RequestArgumentSatisfier;
import io.micronaut.http.server.exceptions.InternalServerException;
import io.micronaut.http.server.multipart.MultipartBody;
import io.micronaut.http.server.netty.configuration.NettyHttpServerConfiguration;
import io.micronaut.http.server.netty.multipart.NettyCompletedFileUpload;
import io.micronaut.http.server.netty.multipart.NettyPartData;
Expand All @@ -63,6 +64,7 @@
import io.micronaut.http.server.netty.types.files.NettySystemFileCustomizableResponseType;
import io.micronaut.http.server.types.files.FileCustomizableResponseType;
import io.micronaut.runtime.http.codec.TextPlainCodec;
import io.micronaut.web.router.MethodBasedRouteMatch;
import io.micronaut.web.router.RouteInfo;
import io.micronaut.web.router.RouteMatch;
import io.micronaut.web.router.resource.StaticResourceResolver;
Expand Down Expand Up @@ -113,6 +115,7 @@
import java.nio.channels.ClosedChannelException;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -292,16 +295,7 @@ protected void channelRead0(ChannelHandlerContext ctx, io.micronaut.http.HttpReq
}
// try to fulfill the argument requirements of the route
RouteMatch<?> route = requestArgumentSatisfier.fulfillArgumentRequirements(routeMatch, httpRequest, false);

Optional<Argument<?>> bodyArgument = route.getBodyArgument()
.filter(argument -> argument.getAnnotationMetadata().hasAnnotation(Body.class));

// The request body is required, so at this point we must have a StreamedHttpRequest
io.netty.handler.codec.http.HttpRequest nativeRequest1 = nettyHttpRequest.getNativeRequest();
if (!route.isExecutable() &&
HttpMethod.permitsRequestBody(nettyHttpRequest.getMethod()) &&
nativeRequest1 instanceof StreamedHttpRequest &&
(bodyArgument.isEmpty() || !route.isSatisfied(bodyArgument.get().getName()))) {
if (shouldReadBody(nettyHttpRequest, route)) {
return ReactiveExecutionFlow.fromPublisher(
Mono.create(emitter -> httpContentProcessorResolver.resolve(nettyHttpRequest, route)
.subscribe(buildSubscriber(nettyHttpRequest, route, emitter))
Expand Down Expand Up @@ -362,6 +356,35 @@ private void writeResponse(ChannelHandlerContext ctx,
}
}

private boolean shouldReadBody(NettyHttpRequest<?> nettyHttpRequest, RouteMatch<?> routeMatch) {
if (!HttpMethod.permitsRequestBody(nettyHttpRequest.getMethod())) {
return false;
}
io.netty.handler.codec.http.HttpRequest nativeRequest = nettyHttpRequest.getNativeRequest();
if (!(nativeRequest instanceof StreamedHttpRequest)) {
// Illegal state: The request body is required, so at this point we must have a StreamedHttpRequest
return false;
}
if (routeMatch instanceof MethodBasedRouteMatch<?, ?> methodBasedRouteMatch) {
if (Arrays.stream(methodBasedRouteMatch.getArguments()).anyMatch(argument -> MultipartBody.class.equals(argument.getType()))) {
// MultipartBody will subscribe to the request body in MultipartBodyArgumentBinder
return false;
}
if (Arrays.stream(methodBasedRouteMatch.getArguments()).anyMatch(argument -> HttpRequest.class.equals(argument.getType()))) {
// HttpRequest argument in the method
return true;
}
}
Optional<Argument<?>> bodyArgument = routeMatch.getBodyArgument()
.filter(argument -> argument.getAnnotationMetadata().hasAnnotation(Body.class));
if (bodyArgument.isPresent() && !routeMatch.isSatisfied(bodyArgument.get().getName())) {
// Body argument in the method
return true;
}
// Might be some body parts
return !routeMatch.isExecutable();
}

@Override
public FileCustomizableResponseType find(HttpRequest<?> httpRequest) {
Optional<URL> optionalUrl = staticResourceResolver.resolve(httpRequest.getUri().getPath());
Expand Down
12 changes: 7 additions & 5 deletions ...r-netty/src/test/groovy/io/micronaut/http/server/netty/binding/JsonBodyBindingSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,20 +273,22 @@ class JsonBodyBindingSpec extends AbstractMicronautSpec {
)).blockFirst()

then:
HttpClientResponseException ex = thrown()
ex.response.code() == HttpStatus.BAD_REQUEST.code
ex.response.getBody(Map).get()._embedded.errors[0].message.contains("Required argument [HttpRequest request] not specified")
response.code() == HttpStatus.OK.code
response.body() == 'not found'
}

void "test request generic type conversion error"() {
when:
String json = '[1,2,3]'
HttpResponse<String> response = Flux.from(rxClient.exchange(
Flux.from(rxClient.exchange(
HttpRequest.POST('/json/request-generic', json), String
)).blockFirst()

then:
response.body() == "not found"
def e = thrown(HttpClientResponseException)
def response = e.response
response.getStatus() == HttpStatus.BAD_REQUEST
response.body().toString().contains("no int/Int-argument constructor/factory method to deserialize from Number value")
}

@Issue("https://github.com/micronaut-projects/micronaut-core/issues/5088")
Expand Down
4 changes: 3 additions & 1 deletion http-server/src/main/java/io/micronaut/http/server/RouteExecutor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import io.micronaut.core.annotation.Internal;
import io.micronaut.core.annotation.NonNull;
import io.micronaut.core.annotation.Nullable;
import io.micronaut.http.codec.CodecException;
import io.micronaut.http.reactive.execution.ReactiveExecutionFlow;
import io.micronaut.core.async.publisher.Publishers;
import io.micronaut.core.execution.ExecutionFlow;
Expand Down Expand Up @@ -647,8 +648,9 @@ private RouteMatch<?> findErrorRoute(Throwable cause,
if (errorRoute == null) {
// Second try is by status route if the status is known
HttpStatus errorStatus = null;
if (cause instanceof UnsatisfiedRouteException) {
if (cause instanceof UnsatisfiedRouteException || cause instanceof CodecException) {
// when arguments do not match, then there is UnsatisfiedRouteException, we can handle this with a routed bad request
// or when incoming request body is not in the expected format
errorStatus = HttpStatus.BAD_REQUEST;
} else if (cause instanceof HttpStatusException) {
errorStatus = ((HttpStatusException) cause).getStatus();
Expand Down
4 changes: 2 additions & 2 deletions http-server/src/main/java/io/micronaut/http/server/binding/RequestArgumentSatisfier.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public RequestBinderRegistry getBinderRegistry() {
* @return The route
*/
public RouteMatch<?> fulfillArgumentRequirements(RouteMatch<?> route, HttpRequest<?> request, boolean satisfyOptionals) {
Collection<Argument> requiredArguments = route.getRequiredArguments();
Collection<Argument<?>> requiredArguments = route.getRequiredArguments();
Map<String, Object> argumentValues;

if (requiredArguments.isEmpty()) {
Expand All @@ -82,7 +82,7 @@ public RouteMatch<?> fulfillArgumentRequirements(RouteMatch<?> route, HttpReques
} else {
argumentValues = new LinkedHashMap<>(requiredArguments.size());
// Begin try fulfilling the argument requirements
for (Argument argument : requiredArguments) {
for (Argument<?> argument : requiredArguments) {
getValueForArgument(argument, request, satisfyOptionals).ifPresent(value ->
argumentValues.put(argument.getName(), value));
}
Expand Down
Loading

0 comments on commit 332d794

Please sign in to comment.