From 1ece4c14ec67e5f2cfff893af15292c42d719e51 Mon Sep 17 00:00:00 2001 From: Tomas Langer Date: Thu, 7 Oct 2021 19:50:45 +0200 Subject: [PATCH] Correctly handling custom reason phrase of status (#3464) * Correctly handling custom reason phrase of status in webserver and webclient. Signed-off-by: Tomas Langer * Fix to use correct status phrases Signed-off-by: Tomas Langer * Checkstyle fix. Signed-off-by: Tomas Langer --- .../java/io/helidon/common/http/Http.java | 15 ++- .../helidon/webclient/NettyClientHandler.java | 24 +---- .../webserver/cors/AbstractCorsTest.java | 15 ++- .../webserver/cors/TestTwoCorsConfigs.java | 6 +- .../helidon/webserver/BareResponseImpl.java | 10 +- .../helidon/webserver/ReasonPhraseTest.java | 102 ++++++++++++++++++ 6 files changed, 139 insertions(+), 33 deletions(-) create mode 100644 webserver/webserver/src/test/java/io/helidon/webserver/ReasonPhraseTest.java diff --git a/common/http/src/main/java/io/helidon/common/http/Http.java b/common/http/src/main/java/io/helidon/common/http/Http.java index 817736f6270..8c5558e0ed6 100644 --- a/common/http/src/main/java/io/helidon/common/http/Http.java +++ b/common/http/src/main/java/io/helidon/common/http/Http.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2020 Oracle and/or its affiliates. + * Copyright (c) 2018, 2021 Oracle and/or its affiliates. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -56,7 +56,16 @@ private Http() { * Copied from JAX-RS. */ public enum Status implements ResponseStatus { - + /** + * 100 Continue, + * see HTTP/1.1 documentations. + */ + CONTINUE_100(100, "Continue"), + /** + * 101 Switching Protocols, + * see HTTP/1.1 documentations. + */ + SWITCHING_PROTOCOLS_101(101, "Switching Protocols"), /** * 200 OK, see HTTP/1.1 documentation. */ @@ -632,7 +641,7 @@ private boolean reasonPhraseEquals(ResponseStatus other) { @Override public String toString() { return "ResponseStatus{code=" + code() - + ", reason" + reasonPhrase() + + ", reason=" + reasonPhrase() + "}"; } }); diff --git a/webclient/webclient/src/main/java/io/helidon/webclient/NettyClientHandler.java b/webclient/webclient/src/main/java/io/helidon/webclient/NettyClientHandler.java index 5a4471bb0c1..2decdac6eb2 100644 --- a/webclient/webclient/src/main/java/io/helidon/webclient/NettyClientHandler.java +++ b/webclient/webclient/src/main/java/io/helidon/webclient/NettyClientHandler.java @@ -18,7 +18,6 @@ import java.io.IOException; import java.util.ArrayList; import java.util.List; -import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionStage; import java.util.concurrent.atomic.AtomicBoolean; @@ -271,28 +270,7 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) { } private Http.ResponseStatus helidonStatus(HttpResponseStatus nettyStatus) { - final int statusCode = nettyStatus.code(); - - Optional status = Http.Status.find(statusCode); - if (status.isPresent()) { - return status.get(); - } - return new Http.ResponseStatus() { - @Override - public int code() { - return statusCode; - } - - @Override - public Family family() { - return Family.of(statusCode); - } - - @Override - public String reasonPhrase() { - return nettyStatus.reasonPhrase(); - } - }; + return Http.ResponseStatus.create(nettyStatus.code(), nettyStatus.reasonPhrase()); } private static final class HttpResponsePublisher extends BufferedEmittingPublisher { diff --git a/webserver/cors/src/test/java/io/helidon/webserver/cors/AbstractCorsTest.java b/webserver/cors/src/test/java/io/helidon/webserver/cors/AbstractCorsTest.java index 76cef6950ac..a2a36958eb1 100644 --- a/webserver/cors/src/test/java/io/helidon/webserver/cors/AbstractCorsTest.java +++ b/webserver/cors/src/test/java/io/helidon/webserver/cors/AbstractCorsTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2020 Oracle and/or its affiliates. + * Copyright (c) 2020, 2021 Oracle and/or its affiliates. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -25,6 +25,7 @@ import io.helidon.webclient.WebClient; import io.helidon.webclient.WebClientRequestBuilder; import io.helidon.webclient.WebClientResponse; + import org.junit.jupiter.api.Test; import static io.helidon.common.http.Http.Header.ORIGIN; @@ -131,7 +132,9 @@ void test2PreFlightForbiddenOrigin() throws ExecutionException, InterruptedExcep .toCompletableFuture() .get(); - assertThat(res.status(), is(Http.Status.FORBIDDEN_403)); + Http.ResponseStatus status = res.status(); + assertThat(status.code(), is(Http.Status.FORBIDDEN_403.code())); + assertThat(status.reasonPhrase(), is("CORS origin is not in allowed list")); } @Test @@ -173,7 +176,9 @@ void test2PreFlightForbiddenMethod() throws ExecutionException, InterruptedExcep .toCompletableFuture() .get(); - assertThat(res.status(), is(Http.Status.FORBIDDEN_403)); + Http.ResponseStatus status = res.status(); + assertThat(status.code(), is(Http.Status.FORBIDDEN_403.code())); + assertThat(status.reasonPhrase(), is("CORS origin is denied")); } @Test @@ -192,7 +197,9 @@ void test2PreFlightForbiddenHeader() throws ExecutionException, InterruptedExcep .toCompletableFuture() .get(); - assertThat(res.status(), is(Http.Status.FORBIDDEN_403)); + Http.ResponseStatus status = res.status(); + assertThat(status.code(), is(Http.Status.FORBIDDEN_403.code())); + assertThat(status.reasonPhrase(), is("CORS headers not in allowed list")); } @Test diff --git a/webserver/cors/src/test/java/io/helidon/webserver/cors/TestTwoCorsConfigs.java b/webserver/cors/src/test/java/io/helidon/webserver/cors/TestTwoCorsConfigs.java index 9d29b4a37fc..f5c1b9998b9 100644 --- a/webserver/cors/src/test/java/io/helidon/webserver/cors/TestTwoCorsConfigs.java +++ b/webserver/cors/src/test/java/io/helidon/webserver/cors/TestTwoCorsConfigs.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2020 Oracle and/or its affiliates. + * Copyright (c) 2020, 2021 Oracle and/or its affiliates. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -51,7 +51,9 @@ public void startupClient() { void test1PreFlightAllowedOriginOtherGreeting() throws ExecutionException, InterruptedException { WebClientResponse res = runTest1PreFlightAllowedOrigin(); - assertThat(res.status(), is(Http.Status.FORBIDDEN_403)); + Http.ResponseStatus status = res.status(); + assertThat(status.code(), is(Http.Status.FORBIDDEN_403.code())); + assertThat(status.reasonPhrase(), is("CORS origin is denied")); } @AfterAll diff --git a/webserver/webserver/src/main/java/io/helidon/webserver/BareResponseImpl.java b/webserver/webserver/src/main/java/io/helidon/webserver/BareResponseImpl.java index d6ce18f23f5..b57e28ce4f7 100644 --- a/webserver/webserver/src/main/java/io/helidon/webserver/BareResponseImpl.java +++ b/webserver/webserver/src/main/java/io/helidon/webserver/BareResponseImpl.java @@ -169,7 +169,15 @@ public void writeStatusAndHeaders(Http.ResponseStatus status, Map> headerEntry : headers.entrySet()) { response.headers().add(headerEntry.getKey(), headerEntry.getValue()); } diff --git a/webserver/webserver/src/test/java/io/helidon/webserver/ReasonPhraseTest.java b/webserver/webserver/src/test/java/io/helidon/webserver/ReasonPhraseTest.java new file mode 100644 index 00000000000..f30dbcd95ca --- /dev/null +++ b/webserver/webserver/src/test/java/io/helidon/webserver/ReasonPhraseTest.java @@ -0,0 +1,102 @@ +/* + * Copyright (c) 2021 Oracle and/or its affiliates. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.helidon.webserver; + +import java.util.concurrent.TimeUnit; + +import io.helidon.common.LogConfig; +import io.helidon.common.http.Http; +import io.helidon.webclient.WebClient; +import io.helidon.webclient.WebClientResponse; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import static io.helidon.common.http.Http.Status.BAD_REQUEST_400; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; + +class ReasonPhraseTest { + public static final String CUSTOM_ERROR = "Custom error"; + private static final Http.ResponseStatus CUSTOM_BAD_REQUEST = Http.ResponseStatus.create(BAD_REQUEST_400.code(), + CUSTOM_ERROR); + private static WebServer server; + private static WebClient client; + + @BeforeAll + static void createWebServer() { + LogConfig.configureRuntime(); + server = WebServer.builder() + .routing(Routing.builder() + .get("/default", ReasonPhraseTest::defaultCode) + .get("/custom", ReasonPhraseTest::customCode)) + .build() + .start() + .await(10, TimeUnit.SECONDS); + + client = WebClient.builder() + .baseUri("http://localhost:" + server.port()) + .build(); + } + + @AfterAll + static void stopWebServer() { + if (server != null) { + server.shutdown() + .await(10, TimeUnit.SECONDS); + } + } + + @Test + void testDefaultReasonPhrase() { + WebClientResponse response = client.get() + .path("/default") + .request() + .await(10, TimeUnit.SECONDS); + + Http.ResponseStatus status = response.status(); + assertThat(status.code(), is(400)); + assertThat(status.reasonPhrase(), is(BAD_REQUEST_400.reasonPhrase())); + + response.close(); + } + + @Test + void testCustomReasonPhrase() { + WebClientResponse response = client.get() + .path("/custom") + .request() + .await(10, TimeUnit.SECONDS); + + Http.ResponseStatus status = response.status(); + assertThat(status.code(), is(400)); + assertThat(status.reasonPhrase(), is(CUSTOM_ERROR)); + + response.close(); + } + + private static void defaultCode(ServerRequest req, ServerResponse res) { + res.status(BAD_REQUEST_400) + .send(); + } + + private static void customCode(ServerRequest req, ServerResponse res) { + res.status(CUSTOM_BAD_REQUEST) + .send(); + } +}