Skip to content

Commit

Permalink
fix: Broken HTTP status code propagation to the client
Browse files Browse the repository at this point in the history
If the error code of an exception is in the range from 100 to 999, the
ErrorHandler is responsible for setting the HTTP status code. Setting
the status code to -1 signals the ErrorHandler that it must set a
proper status code, based on the passed exception.

This logic was broken, because in Vert.x 4.0 the behavior of failing a
routing context has changed. The new default status code has changed
from -1 to 500.
  • Loading branch information
pk-work committed Feb 28, 2021
1 parent c14c4ec commit 9627ea7
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 39 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@
import static io.vertx.core.http.HttpMethod.PUT;
import static io.vertx.ext.web.impl.Utils.pathOffset;
import static java.lang.Character.isUpperCase;
import static java.net.HttpURLConnection.HTTP_BAD_REQUEST;
import static java.net.HttpURLConnection.HTTP_FORBIDDEN;
import static java.net.HttpURLConnection.HTTP_NOT_FOUND;

import java.net.URLDecoder;
import java.nio.charset.StandardCharsets;
Expand Down Expand Up @@ -117,22 +114,14 @@ decodedQueryPath, multiMapToMap(request.headers()), routingContext.getBody()).ad
case FAILURE_CODE_TIMEOUT:
routingContext.fail(GATEWAY_TIMEOUT.code());
return;
case HTTP_BAD_REQUEST:
routingContext.fail(HTTP_BAD_REQUEST);
return;
case HTTP_FORBIDDEN:
routingContext.fail(HTTP_FORBIDDEN);
return;
case HTTP_NOT_FOUND:
routingContext.fail(HTTP_NOT_FOUND);
return;
default:
/* nothing to do here, fail the routingContext with a 500! */
/* nothing to do here, propagate error to the ErrorHandler */
}
}

// fallback, fail the routing context with a 500
routingContext.fail(cause);
// Propagate error to the ErrorHandler which sets the status code depending on the passed
// exception.
routingContext.fail(-1, cause);
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import static io.neonbee.internal.handler.RawDataEndpointHandler.determineQualifiedName;
import static io.neonbee.internal.verticle.ServerVerticle.DEFAULT_RAW_BASE_PATH;
import static io.neonbee.test.helper.DeploymentHelper.NEONBEE_NAMESPACE;
import static io.netty.handler.codec.http.HttpResponseStatus.INTERNAL_SERVER_ERROR;
import static java.net.HttpURLConnection.HTTP_BAD_REQUEST;
import static java.net.HttpURLConnection.HTTP_FORBIDDEN;
import static java.net.HttpURLConnection.HTTP_NOT_FOUND;
Expand All @@ -12,9 +13,13 @@

import java.util.UUID;
import java.util.concurrent.TimeUnit;
import java.util.stream.Stream;

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import io.neonbee.data.DataAdapter;
import io.neonbee.data.DataContext;
Expand Down Expand Up @@ -67,40 +72,31 @@ public void checkQualifiedName() {
assertThat(determineQualifiedName(mockRoutingContext("nsA/nsB/verticle/path"))).isNull();
}

@Test
static Stream<Arguments> customStatusCodes() {
return Stream.of(Arguments.of(HTTP_BAD_REQUEST), Arguments.of(HTTP_FORBIDDEN), Arguments.of(HTTP_NOT_FOUND),
Arguments.of(INTERNAL_SERVER_ERROR.code()));
}

@ParameterizedTest(name = "{index}: with status code {0}")
@MethodSource("customStatusCodes")
@Timeout(value = 2, timeUnit = TimeUnit.SECONDS)
@DisplayName("RawDataEndpointHandler must forward specific HTTP errors to the client")
void testHTTPExceptions(VertxTestContext testContext) {
@DisplayName("RawDataEndpointHandler must forward custom status codes from DataExceptions to the client")
void testHTTPExceptions(int statusCode, VertxTestContext testContext) {
String verticleName = "TestVerticle" + UUID.randomUUID().toString();
Verticle dummyVerticle = createDummyDataVerticle(NEONBEE_NAMESPACE + '/' + verticleName)
.withDataAdapter(new DataAdapter<String>() {

@Override
public Future<String> retrieveData(DataQuery query, DataContext context) {
switch (query.getUriPath()) {
case "/400":
return Future.failedFuture(new DataException(HTTP_BAD_REQUEST));
case "/403":
return Future.failedFuture(new DataException(HTTP_FORBIDDEN));
case "/404":
return Future.failedFuture(new DataException(HTTP_NOT_FOUND));

default:
return Future.succeededFuture("");
}
return Future.failedFuture(new DataException(statusCode));
}
});

deployVerticle(dummyVerticle).compose(v -> sendRequest(verticleName, "400", "")).compose(resp -> {
testContext.verify(() -> assertThat(resp.statusCode()).isEqualTo(HTTP_BAD_REQUEST));
return sendRequest(verticleName, "403", "");
}).compose(resp -> {
testContext.verify(() -> assertThat(resp.statusCode()).isEqualTo(HTTP_FORBIDDEN));
return sendRequest(verticleName, "404", "");
}).onComplete(testContext.succeeding(resp -> {
testContext.verify(() -> assertThat(resp.statusCode()).isEqualTo(HTTP_NOT_FOUND));
testContext.completeNow();
}));
deployVerticle(dummyVerticle).compose(v -> sendRequest(verticleName, "", ""))
.onComplete(testContext.succeeding(resp -> {
testContext.verify(() -> assertThat(resp.statusCode()).isEqualTo(statusCode));
testContext.completeNow();
}));
}

@Test
Expand Down

0 comments on commit 9627ea7

Please sign in to comment.