From a15ef81641577fd4b9b070ddba440591fcecb438 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=EA=B9=80=EC=84=A0=EC=9A=B0?= Date: Sat, 18 May 2024 11:04:45 +0900 Subject: [PATCH] Apply comments --- .../annotation/DefaultAnnotatedService.java | 30 ++-- .../linecorp/armeria/server/HttpService.java | 2 - .../armeria/server/HttpServiceOptions.java | 4 +- .../server/HttpServiceOptionsBuilder.java | 7 + .../armeria/server/ServiceConfigBuilder.java | 4 - .../server/WrappingTransientHttpService.java | 2 +- .../server/annotation/HttpServiceOption.java | 2 +- .../server/HttpServiceOptionsTest.java | 51 ------- .../armeria/server/RoutingContextTest.java | 2 + .../annotation/HttpServiceOptionTest.java | 143 ++++++++++++++++++ .../graphql/GraphqlWebSocketService.java | 13 ++ 11 files changed, 190 insertions(+), 70 deletions(-) create mode 100644 core/src/test/java/com/linecorp/armeria/server/annotation/HttpServiceOptionTest.java diff --git a/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java b/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java index 3d5363aa221..c741a6ebc52 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java +++ b/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java @@ -60,6 +60,7 @@ import com.linecorp.armeria.internal.server.annotation.AnnotatedValueResolver.ResolverContext; import com.linecorp.armeria.server.HttpService; import com.linecorp.armeria.server.HttpServiceOptions; +import com.linecorp.armeria.server.HttpServiceOptionsBuilder; import com.linecorp.armeria.server.Route; import com.linecorp.armeria.server.RoutingContext; import com.linecorp.armeria.server.ServiceRequestContext; @@ -113,7 +114,6 @@ final class DefaultAnnotatedService implements AnnotatedService { @Nullable private final String name; - @Nullable private final HttpServiceOptions options; DefaultAnnotatedService(Object object, Method method, @@ -182,16 +182,14 @@ final class DefaultAnnotatedService implements AnnotatedService { // following must be called only after method.setAccessible(true) methodHandle = asMethodHandle(method, object); - final HttpServiceOption httpServiceOption = AnnotationUtil.findFirst(method, HttpServiceOption.class); + HttpServiceOption httpServiceOption = AnnotationUtil.findFirst(method, HttpServiceOption.class); + if (httpServiceOption == null) { + httpServiceOption = AnnotationUtil.findFirst(object.getClass(), HttpServiceOption.class); + } if (httpServiceOption != null) { - options = HttpServiceOptions.builder() - .requestTimeoutMillis(httpServiceOption.requestTimeoutMillis()) - .maxRequestLength(httpServiceOption.maxRequestLength()) - .requestAutoAbortDelayMillis( - httpServiceOption.requestAutoAbortDelayMillis()) - .build(); + options = buildHttpServiceOptions(httpServiceOption); } else { - options = null; + options = HttpServiceOptions.of(); } } @@ -238,6 +236,20 @@ private static void warnIfHttpResponseArgumentExists(Type returnType, } } + private HttpServiceOptions buildHttpServiceOptions(HttpServiceOption httpServiceOption) { + final HttpServiceOptionsBuilder builder = HttpServiceOptions.builder(); + if (httpServiceOption.requestTimeoutMillis() >= 0) { + builder.requestTimeoutMillis(httpServiceOption.requestTimeoutMillis()); + } + if (httpServiceOption.maxRequestLength() >= 0) { + builder.maxRequestLength(httpServiceOption.maxRequestLength()); + } + if (httpServiceOption.requestAutoAbortDelayMillis() >= 0) { + builder.requestAutoAbortDelayMillis(httpServiceOption.requestAutoAbortDelayMillis()); + } + return builder.build(); + } + @Override public String name() { return name; diff --git a/core/src/main/java/com/linecorp/armeria/server/HttpService.java b/core/src/main/java/com/linecorp/armeria/server/HttpService.java index db3930bfcc4..71b83067dc6 100644 --- a/core/src/main/java/com/linecorp/armeria/server/HttpService.java +++ b/core/src/main/java/com/linecorp/armeria/server/HttpService.java @@ -25,7 +25,6 @@ import com.linecorp.armeria.common.HttpResponse; import com.linecorp.armeria.common.Request; import com.linecorp.armeria.common.Response; -import com.linecorp.armeria.common.annotation.Nullable; import com.linecorp.armeria.common.annotation.UnstableApi; /** @@ -76,7 +75,6 @@ default ExchangeType exchangeType(RoutingContext routingContext) { /** * Returns the {@link HttpServiceOptions} of this {@link HttpService}. */ - @Nullable @UnstableApi default HttpServiceOptions options() { return HttpServiceOptions.of(); diff --git a/core/src/main/java/com/linecorp/armeria/server/HttpServiceOptions.java b/core/src/main/java/com/linecorp/armeria/server/HttpServiceOptions.java index 2039bfc9831..048c02b37af 100644 --- a/core/src/main/java/com/linecorp/armeria/server/HttpServiceOptions.java +++ b/core/src/main/java/com/linecorp/armeria/server/HttpServiceOptions.java @@ -64,7 +64,7 @@ public long requestTimeoutMillis() { } /** - * Returns the server-side maximum length of a request. + * Returns the server-side maximum length of a request. {@code -1} if not set. */ public long maxRequestLength() { return maxRequestLength; @@ -72,7 +72,7 @@ public long maxRequestLength() { /** * Returns the amount of time to wait before aborting an {@link HttpRequest} when its corresponding - * {@link HttpResponse} is complete. + * {@link HttpResponse} is complete. {@code -1} if not set. */ public long requestAutoAbortDelayMillis() { return requestAutoAbortDelayMillis; diff --git a/core/src/main/java/com/linecorp/armeria/server/HttpServiceOptionsBuilder.java b/core/src/main/java/com/linecorp/armeria/server/HttpServiceOptionsBuilder.java index c05168f2136..fda9fdb8d69 100644 --- a/core/src/main/java/com/linecorp/armeria/server/HttpServiceOptionsBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/server/HttpServiceOptionsBuilder.java @@ -16,6 +16,8 @@ package com.linecorp.armeria.server; +import static com.google.common.base.Preconditions.checkArgument; + import com.linecorp.armeria.common.HttpRequest; import com.linecorp.armeria.common.HttpResponse; @@ -33,6 +35,8 @@ public final class HttpServiceOptionsBuilder { * Returns the server-side timeout of a request in milliseconds. */ public HttpServiceOptionsBuilder requestTimeoutMillis(long requestTimeoutMillis) { + checkArgument(requestTimeoutMillis >= 0, "requestTimeoutMillis: %s (expected: >= 0)", + requestTimeoutMillis); this.requestTimeoutMillis = requestTimeoutMillis; return this; } @@ -41,6 +45,7 @@ public HttpServiceOptionsBuilder requestTimeoutMillis(long requestTimeoutMillis) * Returns the server-side maximum length of a request. */ public HttpServiceOptionsBuilder maxRequestLength(long maxRequestLength) { + checkArgument(maxRequestLength >= 0, "maxRequestLength: %s (expected: >= 0)", maxRequestLength); this.maxRequestLength = maxRequestLength; return this; } @@ -50,6 +55,8 @@ public HttpServiceOptionsBuilder maxRequestLength(long maxRequestLength) { * {@link HttpResponse} is complete. */ public HttpServiceOptionsBuilder requestAutoAbortDelayMillis(long requestAutoAbortDelayMillis) { + checkArgument(requestAutoAbortDelayMillis >= 0, "requestAutoAbortDelayMillis: %s (expected: >= 0)", + requestAutoAbortDelayMillis); this.requestAutoAbortDelayMillis = requestAutoAbortDelayMillis; return this; } diff --git a/core/src/main/java/com/linecorp/armeria/server/ServiceConfigBuilder.java b/core/src/main/java/com/linecorp/armeria/server/ServiceConfigBuilder.java index 8d42385e2e6..9837931561c 100644 --- a/core/src/main/java/com/linecorp/armeria/server/ServiceConfigBuilder.java +++ b/core/src/main/java/com/linecorp/armeria/server/ServiceConfigBuilder.java @@ -90,10 +90,6 @@ final class ServiceConfigBuilder implements ServiceConfigSetters { this.service = requireNonNull(service, "service"); final HttpServiceOptions options = service.options(); - if (options == null) { - return; - } - if (options.requestTimeoutMillis() != -1) { requestTimeoutMillis = options.requestTimeoutMillis(); } diff --git a/core/src/main/java/com/linecorp/armeria/server/WrappingTransientHttpService.java b/core/src/main/java/com/linecorp/armeria/server/WrappingTransientHttpService.java index 77d0327f9ba..9ce420361d2 100644 --- a/core/src/main/java/com/linecorp/armeria/server/WrappingTransientHttpService.java +++ b/core/src/main/java/com/linecorp/armeria/server/WrappingTransientHttpService.java @@ -22,7 +22,7 @@ import com.linecorp.armeria.common.HttpResponse; /** - * Decorates a {@link HttpService} to be treated as {@link TransientService} without inheritance. + * Decorates an {@link HttpService} to be treated as {@link TransientService} without inheritance. */ final class WrappingTransientHttpService extends SimpleDecoratingHttpService implements TransientHttpService { diff --git a/core/src/main/java/com/linecorp/armeria/server/annotation/HttpServiceOption.java b/core/src/main/java/com/linecorp/armeria/server/annotation/HttpServiceOption.java index 75d27100ea0..771ddf27737 100644 --- a/core/src/main/java/com/linecorp/armeria/server/annotation/HttpServiceOption.java +++ b/core/src/main/java/com/linecorp/armeria/server/annotation/HttpServiceOption.java @@ -30,7 +30,7 @@ * An annotation used to configure {@link HttpServiceOptions} of an {@link HttpService}. */ @Retention(RetentionPolicy.RUNTIME) -@Target({ ElementType.METHOD }) +@Target({ ElementType.METHOD, ElementType.TYPE }) public @interface HttpServiceOption { /** diff --git a/core/src/test/java/com/linecorp/armeria/server/HttpServiceOptionsTest.java b/core/src/test/java/com/linecorp/armeria/server/HttpServiceOptionsTest.java index b9c83aeae0c..0c4b6b6b7e8 100644 --- a/core/src/test/java/com/linecorp/armeria/server/HttpServiceOptionsTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/HttpServiceOptionsTest.java @@ -22,8 +22,6 @@ import com.linecorp.armeria.common.HttpRequest; import com.linecorp.armeria.common.HttpResponse; -import com.linecorp.armeria.server.annotation.Get; -import com.linecorp.armeria.server.annotation.HttpServiceOption; /** * The priority of configurations from highest to lowest: @@ -132,53 +130,4 @@ public HttpServiceOptions options() { httpServiceOptions.requestAutoAbortDelayMillis()); } } - - @Test - void httpServiceOptionAnnotationShouldBeAppliedWhenConfigured() { - final class TestAnnotatedService { - @HttpServiceOption( - requestTimeoutMillis = 11111, - maxRequestLength = 1111, - requestAutoAbortDelayMillis = 111 - ) - @Get("/test1") - public HttpResponse test1() { - return HttpResponse.of("OK"); - } - - @Get("/test2") - public HttpResponse test2() { - return HttpResponse.of("OK"); - } - } - - final long DEFAULT_REQUEST_TIMEOUT_MILLIS = 30001; - final long DEFAULT_MAX_REQUEST_LENGTH = 30002; - final long DEFAULT_REQUEST_AUTO_ABORT_DELAY_MILLIS = 30003; - - try (Server server = Server.builder().annotatedService(new TestAnnotatedService()) - .requestTimeoutMillis(DEFAULT_REQUEST_TIMEOUT_MILLIS) - .maxRequestLength(DEFAULT_MAX_REQUEST_LENGTH) - .requestAutoAbortDelayMillis(DEFAULT_REQUEST_AUTO_ABORT_DELAY_MILLIS) - .build()) { - final ServiceConfig sc1 = server.serviceConfigs() - .stream() - .filter(s -> s.route().paths().contains("/test1")) - .findFirst().orElse(null); - - assertThat(sc1).isNotNull(); - assertThat(sc1.requestTimeoutMillis()).isEqualTo(11111); - assertThat(sc1.maxRequestLength()).isEqualTo(1111); - assertThat(sc1.requestAutoAbortDelayMillis()).isEqualTo(111); - - // default values should be applied - final ServiceConfig sc2 = server.serviceConfigs() - .stream() - .filter(s -> s.route().paths().contains("/test2")) - .findFirst().get(); - assertThat(sc2.requestTimeoutMillis()).isEqualTo(DEFAULT_REQUEST_TIMEOUT_MILLIS); - assertThat(sc2.maxRequestLength()).isEqualTo(DEFAULT_MAX_REQUEST_LENGTH); - assertThat(sc2.requestAutoAbortDelayMillis()).isEqualTo(DEFAULT_REQUEST_AUTO_ABORT_DELAY_MILLIS); - } - } } diff --git a/core/src/test/java/com/linecorp/armeria/server/RoutingContextTest.java b/core/src/test/java/com/linecorp/armeria/server/RoutingContextTest.java index a1fe3e7d0d3..9f579d01490 100644 --- a/core/src/test/java/com/linecorp/armeria/server/RoutingContextTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/RoutingContextTest.java @@ -18,6 +18,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import java.util.List; @@ -141,6 +142,7 @@ static RoutingContext create(VirtualHost virtualHost, String path, @Nullable Str static VirtualHost virtualHost() { final HttpService service = mock(HttpService.class); + when(service.options()).thenReturn(HttpServiceOptions.of()); final Server server = Server.builder() .virtualHost("example.com") .serviceUnder("/", service) diff --git a/core/src/test/java/com/linecorp/armeria/server/annotation/HttpServiceOptionTest.java b/core/src/test/java/com/linecorp/armeria/server/annotation/HttpServiceOptionTest.java new file mode 100644 index 00000000000..0cc9fda619e --- /dev/null +++ b/core/src/test/java/com/linecorp/armeria/server/annotation/HttpServiceOptionTest.java @@ -0,0 +1,143 @@ +/* + * Copyright 2024 LINE Corporation + * + * LINE Corporation licenses this file to you 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: + * + * https://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 com.linecorp.armeria.server.annotation; + +import static org.assertj.core.api.AssertionsForClassTypes.assertThat; + +import org.junit.jupiter.api.Test; + +import com.linecorp.armeria.common.HttpResponse; +import com.linecorp.armeria.server.Server; +import com.linecorp.armeria.server.ServiceConfig; + +public class HttpServiceOptionTest { + @Test + void httpServiceOptionAnnotationShouldBeAppliedWhenConfiguredAtMethodLevel() { + final class TestAnnotatedService { + @HttpServiceOption( + requestTimeoutMillis = 11111, + maxRequestLength = 1111 + ) + @Get("/test1") + public HttpResponse test1() { + return HttpResponse.of("OK"); + } + + @Get("/test2") + public HttpResponse test2() { + return HttpResponse.of("OK"); + } + } + + final long DEFAULT_REQUEST_TIMEOUT_MILLIS = 30001; + final long DEFAULT_MAX_REQUEST_LENGTH = 30002; + final long DEFAULT_REQUEST_AUTO_ABORT_DELAY_MILLIS = 30003; + + try (Server server = Server.builder().annotatedService(new TestAnnotatedService()) + .requestTimeoutMillis(DEFAULT_REQUEST_TIMEOUT_MILLIS) + .maxRequestLength(DEFAULT_MAX_REQUEST_LENGTH) + .requestAutoAbortDelayMillis(DEFAULT_REQUEST_AUTO_ABORT_DELAY_MILLIS) + .build()) { + final ServiceConfig sc1 = server.serviceConfigs() + .stream() + .filter(s -> s.route().paths().contains("/test1")) + .findFirst().orElse(null); + + assertThat(sc1).isNotNull(); + assertThat(sc1.requestTimeoutMillis()).isEqualTo(11111); + assertThat(sc1.maxRequestLength()).isEqualTo(1111); + assertThat(sc1.requestAutoAbortDelayMillis()).isEqualTo(DEFAULT_REQUEST_AUTO_ABORT_DELAY_MILLIS); + + // default values should be applied + final ServiceConfig sc2 = server.serviceConfigs() + .stream() + .filter(s -> s.route().paths().contains("/test2")) + .findFirst().get(); + assertThat(sc2.requestTimeoutMillis()).isEqualTo(DEFAULT_REQUEST_TIMEOUT_MILLIS); + assertThat(sc2.maxRequestLength()).isEqualTo(DEFAULT_MAX_REQUEST_LENGTH); + assertThat(sc2.requestAutoAbortDelayMillis()).isEqualTo(DEFAULT_REQUEST_AUTO_ABORT_DELAY_MILLIS); + } + } + + @Test + void httpServiceOptionAnnotationShouldBeAppliedWhenConfiguredAtClassLevel() { + @HttpServiceOption( + requestTimeoutMillis = 11111, + maxRequestLength = 1111 + ) + final class TestAnnotatedService { + @Get("/test") + public HttpResponse test() { + return HttpResponse.of("OK"); + } + } + + final long DEFAULT_REQUEST_TIMEOUT_MILLIS = 30001; + final long DEFAULT_MAX_REQUEST_LENGTH = 30002; + final long DEFAULT_REQUEST_AUTO_ABORT_DELAY_MILLIS = 30003; + + try (Server server = Server.builder().annotatedService(new TestAnnotatedService()) + .requestTimeoutMillis(DEFAULT_REQUEST_TIMEOUT_MILLIS) + .maxRequestLength(DEFAULT_MAX_REQUEST_LENGTH) + .requestAutoAbortDelayMillis(DEFAULT_REQUEST_AUTO_ABORT_DELAY_MILLIS) + .build()) { + final ServiceConfig sc = server.serviceConfigs() + .stream() + .filter(s -> s.route().paths().contains("/test")) + .findFirst().orElse(null); + + assertThat(sc).isNotNull(); + assertThat(sc.requestTimeoutMillis()).isEqualTo(11111); + assertThat(sc.maxRequestLength()).isEqualTo(1111); + assertThat(sc.requestAutoAbortDelayMillis()).isEqualTo(DEFAULT_REQUEST_AUTO_ABORT_DELAY_MILLIS); + } + } + + @Test + void httpServiceOptionAnnotationAtMethodLevelShouldOverrideHttpServiceOptionAtClassLevel() { + @HttpServiceOption( + requestTimeoutMillis = 11111, + maxRequestLength = 1111, + requestAutoAbortDelayMillis = 111 + ) + final class TestAnnotatedService { + @HttpServiceOption( + requestTimeoutMillis = 22222, + maxRequestLength = 2222, + requestAutoAbortDelayMillis = 222 + ) + @Get("/test") + public HttpResponse test() { + return HttpResponse.of("OK"); + } + } + + try (Server server = Server.builder() + .annotatedService(new TestAnnotatedService()) + .build()) { + final ServiceConfig sc = server.serviceConfigs() + .stream() + .filter(s -> s.route().paths().contains("/test")) + .findFirst().orElse(null); + + assertThat(sc).isNotNull(); + assertThat(sc.requestTimeoutMillis()).isEqualTo(22222); + assertThat(sc.maxRequestLength()).isEqualTo(2222); + assertThat(sc.requestAutoAbortDelayMillis()).isEqualTo(222); + } + } +} diff --git a/graphql/src/main/java/com/linecorp/armeria/server/graphql/GraphqlWebSocketService.java b/graphql/src/main/java/com/linecorp/armeria/server/graphql/GraphqlWebSocketService.java index ffe15121764..38180d6ed55 100644 --- a/graphql/src/main/java/com/linecorp/armeria/server/graphql/GraphqlWebSocketService.java +++ b/graphql/src/main/java/com/linecorp/armeria/server/graphql/GraphqlWebSocketService.java @@ -24,6 +24,8 @@ import com.linecorp.armeria.common.annotation.Nullable; import com.linecorp.armeria.common.websocket.WebSocket; import com.linecorp.armeria.common.websocket.WebSocketWriter; +import com.linecorp.armeria.internal.common.websocket.WebSocketUtil; +import com.linecorp.armeria.server.HttpServiceOptions; import com.linecorp.armeria.server.ServiceRequestContext; import com.linecorp.armeria.server.websocket.WebSocketProtocolHandler; import com.linecorp.armeria.server.websocket.WebSocketService; @@ -33,6 +35,12 @@ final class GraphqlWebSocketService implements GraphqlService, WebSocketService, WebSocketServiceHandler { private static final String GRAPHQL_TRANSPORT_WS = "graphql-transport-ws"; + private static final HttpServiceOptions DEFAULT_OPTIONS = HttpServiceOptions + .builder() + .requestTimeoutMillis(WebSocketUtil.DEFAULT_REQUEST_RESPONSE_TIMEOUT_MILLIS) + .maxRequestLength(WebSocketUtil.DEFAULT_MAX_REQUEST_RESPONSE_LENGTH) + .requestAutoAbortDelayMillis(WebSocketUtil.DEFAULT_REQUEST_AUTO_ABORT_DELAY_MILLIS) + .build(); private final WebSocketService delegate; private final Function @@ -74,4 +82,9 @@ public WebSocket handle(ServiceRequestContext ctx, WebSocket in) { in.subscribe(new GraphqlWebSocketSubscriber(protocol, outgoing)); return outgoing; } + + @Override + public HttpServiceOptions options() { + return DEFAULT_OPTIONS; + } }