Skip to content

Commit 3e0951b

Browse files
0x1306e6d0x1306e6d
committed
Address comments by @minwoox 
1 parent d417af7 commit 3e0951b

3 files changed

Lines changed: 137 additions & 99 deletions

File tree

‎core/src/main/java/com/linecorp/armeria/internal/logging/ContentPreviewingUtil.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,7 @@ protected ContentPreviewerHttpResponse(
115115
this.ctx = ctx;
116116
whenComplete().handle((unused, cause) -> {
117117
if (responseContentPreviewer == null) {
118+
ctx.logBuilder().responseContentPreview(null);
118119
return null;
119120
}
120121
if (!responseContentPreviewer.isDisabled()) {

‎core/src/main/java/com/linecorp/armeria/server/logging/ContentPreviewingService.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,6 +176,7 @@ public HttpResponse serve(ServiceRequestContext ctx, HttpRequest req) throws Exc
176176
final HttpResponse res = unwrap().serve(ctx, req);
177177
return setUpResponseContentPreviewer(contentPreviewerFactory, ctx, res, responsePreviewSanitizer);
178178
} catch (Throwable t) {
179+
ctx.logBuilder().responseContentPreview(null);
179180
return HttpResponse.ofFailure(t);
180181
}
181182
}

‎core/src/test/java/com/linecorp/armeria/server/logging/ContentPreviewingErrorResponseTest.java‎

Lines changed: 135 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,10 @@
1818

1919
import static org.assertj.core.api.Assertions.assertThat;
2020

21-
import java.util.stream.Stream;
22-
23-
import org.junit.jupiter.api.extension.ExtensionContext;
2421
import org.junit.jupiter.api.extension.RegisterExtension;
2522
import org.junit.jupiter.params.ParameterizedTest;
26-
import org.junit.jupiter.params.provider.Arguments;
27-
import org.junit.jupiter.params.provider.ArgumentsProvider;
28-
import org.junit.jupiter.params.provider.ArgumentsSource;
23+
import org.junit.jupiter.params.provider.ValueSource;
2924

30-
import com.linecorp.armeria.client.BlockingWebClient;
3125
import com.linecorp.armeria.common.AggregatedHttpResponse;
3226
import com.linecorp.armeria.common.HttpResponse;
3327
import com.linecorp.armeria.common.HttpStatus;
@@ -44,76 +38,157 @@
4438
class ContentPreviewingErrorResponseTest {
4539

4640
@RegisterExtension
47-
static final ServerExtension server = new ServerExtension() {
41+
static final ServerExtension serverWithErrorHandler = new ServerExtension() {
4842
@Override
4943
protected void configure(ServerBuilder sb) {
44+
configureServer(sb, true);
45+
}
46+
};
47+
48+
@RegisterExtension
49+
static final ServerExtension serverWithoutServerErrorHandler = new ServerExtension() {
50+
@Override
51+
protected void configure(ServerBuilder sb) {
52+
configureServer(sb, false);
53+
}
54+
};
55+
56+
private static void configureServer(ServerBuilder sb, boolean errorHandler) {
57+
if (errorHandler) {
5058
sb.errorHandler((ctx, cause) -> {
5159
return HttpResponse.of("errorHandler: " + cause.getMessage());
5260
});
61+
}
5362

54-
sb.service("/aborted/http-status-exception", (ctx, req) ->
55-
HttpResponse.ofFailure(HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR)));
56-
sb.service("/aborted/unexpected-exception", (ctx, req) ->
57-
HttpResponse.ofFailure(new IllegalStateException("Oops!")));
63+
sb.service("/aborted/http-status-exception", (ctx, req) ->
64+
HttpResponse.ofFailure(HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR)));
65+
sb.service("/aborted/unexpected-exception", (ctx, req) ->
66+
HttpResponse.ofFailure(new IllegalStateException("Oops!")));
5867

59-
sb.service("/throw/http-status-exception", (ctx, req) -> {
60-
throw HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR);
61-
});
62-
sb.service("/throw/unexpected-exception", (ctx, req) -> {
63-
throw new IllegalStateException("Oops!");
64-
});
68+
sb.service("/throw/http-status-exception", (ctx, req) -> {
69+
throw HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR);
70+
});
71+
sb.service("/throw/unexpected-exception", (ctx, req) -> {
72+
throw new IllegalStateException("Oops!");
73+
});
6574

66-
sb.annotatedService("/annotated", new ContentPreviewingAnnotatedService());
75+
sb.annotatedService("/annotated", new ContentPreviewingAnnotatedService());
76+
if (errorHandler) {
6777
sb.annotatedService("/annotatedExceptionHandler",
6878
new ContentPreviewingAnnotatedService(),
6979
(ExceptionHandlerFunction) (ctx, req, cause) -> {
7080
return HttpResponse.of("exceptionHandler: " + cause.getMessage());
7181
});
72-
73-
final ServiceErrorHandler serviceErrorHandler = (ctx, cause) -> {
74-
return HttpResponse.of("serviceErrorHandler: " + cause.getMessage());
75-
};
76-
sb.route()
77-
.path("/binding/aborted/http-status-exception")
78-
.errorHandler(serviceErrorHandler)
79-
.build((ctx, req) -> {
80-
return HttpResponse.ofFailure(HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR));
81-
});
82-
sb.route()
83-
.path("/binding/aborted/unexpected-exception")
84-
.errorHandler(serviceErrorHandler)
85-
.build((ctx, req) -> {
86-
return HttpResponse.ofFailure(new IllegalStateException("Oops!"));
87-
});
88-
sb.route()
89-
.path("/binding/throw/http-status-exception")
90-
.errorHandler(serviceErrorHandler)
91-
.build((ctx, req) -> {
92-
throw HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR);
93-
});
94-
sb.route()
95-
.path("/binding/throw/unexpected-exception")
96-
.errorHandler(serviceErrorHandler)
97-
.build((ctx, req) -> {
98-
throw new IllegalStateException("Oops!");
99-
});
100-
101-
sb.decorator(LoggingService.newDecorator());
102-
sb.decorator(ContentPreviewingService.newDecorator(Integer.MAX_VALUE));
10382
}
104-
};
83+
84+
final ServiceErrorHandler serviceErrorHandler = (ctx, cause) -> {
85+
return HttpResponse.of("serviceErrorHandler: " + cause.getMessage());
86+
};
87+
sb.withRoute(bindingBuilder -> {
88+
if (errorHandler) {
89+
bindingBuilder.errorHandler(serviceErrorHandler);
90+
}
91+
bindingBuilder.path("/binding/aborted/http-status-exception")
92+
.build((ctx, req) -> {
93+
return HttpResponse.ofFailure(
94+
HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR));
95+
});
96+
});
97+
sb.withRoute(bindingBuilder -> {
98+
if (errorHandler) {
99+
bindingBuilder.errorHandler(serviceErrorHandler);
100+
}
101+
bindingBuilder.path("/binding/aborted/unexpected-exception")
102+
.build((ctx, req) -> {
103+
return HttpResponse.ofFailure(new IllegalStateException("Oops!"));
104+
});
105+
});
106+
sb.withRoute(bindingBuilder -> {
107+
if (errorHandler) {
108+
bindingBuilder.errorHandler(serviceErrorHandler);
109+
}
110+
bindingBuilder.path("/binding/throw/http-status-exception")
111+
.build((ctx, req) -> {
112+
throw HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR);
113+
});
114+
});
115+
sb.withRoute(bindingBuilder -> {
116+
if (errorHandler) {
117+
bindingBuilder.errorHandler(serviceErrorHandler);
118+
}
119+
bindingBuilder.path("/binding/throw/unexpected-exception")
120+
.build((ctx, req) -> {
121+
throw new IllegalStateException("Oops!");
122+
});
123+
});
124+
125+
sb.decorator(LoggingService.newDecorator());
126+
sb.decorator(ContentPreviewingService.newDecorator(Integer.MAX_VALUE));
127+
}
128+
129+
@ParameterizedTest
130+
@ValueSource(strings = {
131+
"/aborted/http-status-exception",
132+
"/aborted/unexpected-exception",
133+
"/throw/http-status-exception",
134+
"/throw/unexpected-exception",
135+
"/annotated/aborted/http-status-exception",
136+
"/annotated/aborted/unexpected-exception",
137+
"/annotated/throw/http-status-exception",
138+
"/annotated/throw/unexpected-exception",
139+
"/binding/aborted/http-status-exception",
140+
"/binding/aborted/unexpected-exception",
141+
"/binding/throw/http-status-exception",
142+
"/binding/throw/unexpected-exception",
143+
})
144+
void shouldSetNullContentPreviewWhenAnExceptionIsRaisedAndErrorHandlerSet(String path) throws Exception {
145+
serverWithErrorHandler.blockingWebClient().get(path);
146+
147+
final RequestContext ctx = serverWithErrorHandler.requestContextCaptor().poll();
148+
final RequestLog log = ctx.log().whenComplete().join();
149+
150+
assertThat(log.responseContentPreview()).isNull();
151+
}
105152

106153
@ParameterizedTest
107-
@ArgumentsSource(ShouldRecordErrorResponseContentPreviewingArgumentsProvider.class)
108-
void shouldRecordErrorResponseContentPreviewing(String path, String responseContent) throws Exception {
109-
final BlockingWebClient client = server.blockingWebClient();
110-
final AggregatedHttpResponse res = client.get(path);
111-
assertThat(res.contentUtf8()).isEqualTo(responseContent);
154+
@ValueSource(strings = {
155+
"/aborted/http-status-exception",
156+
"/aborted/unexpected-exception",
157+
"/throw/http-status-exception",
158+
"/throw/unexpected-exception",
159+
"/annotated/aborted/http-status-exception",
160+
"/annotated/aborted/unexpected-exception",
161+
"/annotated/throw/http-status-exception",
162+
"/annotated/throw/unexpected-exception",
163+
"/binding/aborted/http-status-exception",
164+
"/binding/aborted/unexpected-exception",
165+
"/binding/throw/http-status-exception",
166+
"/binding/throw/unexpected-exception",
167+
})
168+
void shouldSetNullContentPreviewWhenAnExceptionIsRaisedAndNoErrorHandlerSet(String path) throws Exception {
169+
serverWithoutServerErrorHandler.blockingWebClient().get(path);
170+
171+
final RequestContext ctx = serverWithoutServerErrorHandler.requestContextCaptor().poll();
172+
final RequestLog log = ctx.log().whenComplete().join();
112173

113-
final RequestContext ctx = server.requestContextCaptor().poll();
174+
assertThat(log.responseContentPreview()).isNull();
175+
}
176+
177+
@ParameterizedTest
178+
@ValueSource(strings = {
179+
"/annotatedExceptionHandler/aborted/http-status-exception",
180+
"/annotatedExceptionHandler/aborted/unexpected-exception",
181+
"/annotatedExceptionHandler/throw/http-status-exception",
182+
"/annotatedExceptionHandler/throw/unexpected-exception",
183+
})
184+
void annotatedServiceExceptionHandlerAlwaysRecordContentPreview(String path) throws Exception {
185+
final AggregatedHttpResponse res = serverWithErrorHandler.blockingWebClient().get(path);
186+
final String content = res.contentUtf8();
187+
188+
final RequestContext ctx = serverWithErrorHandler.requestContextCaptor().poll();
114189
final RequestLog log = ctx.log().whenComplete().join();
115190

116-
assertThat(log.responseContentPreview()).isEqualTo(responseContent);
191+
assertThat(log.responseContentPreview()).isEqualTo(content);
117192
}
118193

119194
@ProducesText
@@ -125,7 +200,7 @@ public HttpResponse abortedHttpStatusException() {
125200

126201
@Get("/aborted/unexpected-exception")
127202
public HttpResponse abortedUnexpectedException() {
128-
return HttpResponse.ofFailure(new IllegalStateException("Oops!"));
203+
return HttpResponse.ofFailure(new IllegalArgumentException("Oops!"));
129204
}
130205

131206
@Get("/throw/http-status-exception")
@@ -135,46 +210,7 @@ public HttpResponse throwHttpStatusException() {
135210

136211
@Get("/throw/unexpected-exception")
137212
public HttpResponse throwUnexpectedException() {
138-
throw new IllegalStateException("Oops!");
139-
}
140-
}
141-
142-
private static final class ShouldRecordErrorResponseContentPreviewingArgumentsProvider
143-
implements ArgumentsProvider {
144-
@Override
145-
public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
146-
return Stream.of(Arguments.of("/aborted/http-status-exception",
147-
"errorHandler: 500 Internal Server Error"),
148-
Arguments.of("/aborted/unexpected-exception",
149-
"errorHandler: Oops!"),
150-
Arguments.of("/throw/http-status-exception",
151-
"errorHandler: 500 Internal Server Error"),
152-
Arguments.of("/throw/unexpected-exception",
153-
"errorHandler: Oops!"),
154-
Arguments.of("/annotated/aborted/http-status-exception",
155-
"errorHandler: 500 Internal Server Error"),
156-
Arguments.of("/annotated/aborted/unexpected-exception",
157-
"errorHandler: Oops!"),
158-
Arguments.of("/annotated/throw/http-status-exception",
159-
"errorHandler: 500 Internal Server Error"),
160-
Arguments.of("/annotated/throw/unexpected-exception",
161-
"errorHandler: Oops!"),
162-
Arguments.of("/annotatedExceptionHandler/aborted/http-status-exception",
163-
"exceptionHandler: 500 Internal Server Error"),
164-
Arguments.of("/annotatedExceptionHandler/aborted/unexpected-exception",
165-
"exceptionHandler: Oops!"),
166-
Arguments.of("/annotatedExceptionHandler/throw/http-status-exception",
167-
"exceptionHandler: 500 Internal Server Error"),
168-
Arguments.of("/annotatedExceptionHandler/throw/unexpected-exception",
169-
"exceptionHandler: Oops!"),
170-
Arguments.of("/binding/aborted/http-status-exception",
171-
"serviceErrorHandler: 500 Internal Server Error"),
172-
Arguments.of("/binding/aborted/unexpected-exception",
173-
"serviceErrorHandler: Oops!"),
174-
Arguments.of("/binding/throw/http-status-exception",
175-
"serviceErrorHandler: 500 Internal Server Error"),
176-
Arguments.of("/binding/throw/unexpected-exception",
177-
"serviceErrorHandler: Oops!"));
213+
throw new IllegalArgumentException("Oops!");
178214
}
179215
}
180216
}

0 commit comments

Comments
 (0)