Skip to content

Commit 7915847

Browse files
0x1306e6d0x1306e6d
authored
Set content preview to null when response fails with exception (#6589)
Motivation: Following the discussion in #6570 (comment), content preview was being generated even when a preceding decorator failed with an exception. This behavior was inconsistent with `DefaultRequestLog`, which sets the preview to null when an exception is present: https://github.com/line/armeria/blob/d068172432163c42908ad5d509262cc4022afb04/core/src/main/java/com/linecorp/armeria/common/logging/DefaultRequestLog.java#L1471-L1473 The content preview should not be recorded when the response flow is interrupted by an exception, as the decorator did not actually process the response content. Modifications: - Set `responseContentPreview(null)` in `ContentPreviewingUtil` when `responseContentPreviewer` is null. - Set `responseContentPreview(null)` in `ContentPreviewingService` when service execution throws an exception. - Updated tests to verify null content preview for various exception scenarios with and without error handlers. Result: - Response content preview is properly set to null when exceptions are raised. - Consistent behavior between decorator-level exceptions and error handler responses.
1 parent 8024374 commit 7915847

3 files changed

Lines changed: 138 additions & 63 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: 136 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -18,23 +18,18 @@
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;
3428
import com.linecorp.armeria.common.RequestContext;
3529
import com.linecorp.armeria.common.logging.RequestLog;
3630
import com.linecorp.armeria.server.HttpStatusException;
3731
import com.linecorp.armeria.server.ServerBuilder;
32+
import com.linecorp.armeria.server.ServiceErrorHandler;
3833
import com.linecorp.armeria.server.annotation.ExceptionHandlerFunction;
3934
import com.linecorp.armeria.server.annotation.Get;
4035
import com.linecorp.armeria.server.annotation.ProducesText;
@@ -43,48 +38,157 @@
4338
class ContentPreviewingErrorResponseTest {
4439

4540
@RegisterExtension
46-
static final ServerExtension server = new ServerExtension() {
41+
static final ServerExtension serverWithErrorHandler = new ServerExtension() {
4742
@Override
4843
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) {
4958
sb.errorHandler((ctx, cause) -> {
5059
return HttpResponse.of("errorHandler: " + cause.getMessage());
5160
});
61+
}
5262

53-
sb.service("/aborted/http-status-exception", (ctx, req) ->
54-
HttpResponse.ofFailure(HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR)));
55-
sb.service("/aborted/unexpected-exception", (ctx, req) ->
56-
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!")));
5767

58-
sb.service("/throw/http-status-exception", (ctx, req) -> {
59-
throw HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR);
60-
});
61-
sb.service("/throw/unexpected-exception", (ctx, req) -> {
62-
throw new IllegalStateException("Oops!");
63-
});
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+
});
6474

65-
sb.annotatedService("/annotated", new ContentPreviewingAnnotatedService());
75+
sb.annotatedService("/annotated", new ContentPreviewingAnnotatedService());
76+
if (errorHandler) {
6677
sb.annotatedService("/annotatedExceptionHandler",
6778
new ContentPreviewingAnnotatedService(),
6879
(ExceptionHandlerFunction) (ctx, req, cause) -> {
6980
return HttpResponse.of("exceptionHandler: " + cause.getMessage());
7081
});
71-
72-
sb.decorator(LoggingService.newDecorator());
73-
sb.decorator(ContentPreviewingService.newDecorator(Integer.MAX_VALUE));
7482
}
75-
};
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+
}
76152

77153
@ParameterizedTest
78-
@ArgumentsSource(ShouldRecordErrorResponseContentPreviewingArgumentsProvider.class)
79-
void shouldRecordErrorResponseContentPreviewing(String path, String responseContent) throws Exception {
80-
final BlockingWebClient client = server.blockingWebClient();
81-
final AggregatedHttpResponse res = client.get(path);
82-
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();
83173

84-
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();
85189
final RequestLog log = ctx.log().whenComplete().join();
86190

87-
assertThat(log.responseContentPreview()).isEqualTo(responseContent);
191+
assertThat(log.responseContentPreview()).isEqualTo(content);
88192
}
89193

90194
@ProducesText
@@ -96,7 +200,7 @@ public HttpResponse abortedHttpStatusException() {
96200

97201
@Get("/aborted/unexpected-exception")
98202
public HttpResponse abortedUnexpectedException() {
99-
return HttpResponse.ofFailure(new IllegalStateException("Oops!"));
203+
return HttpResponse.ofFailure(new IllegalArgumentException("Oops!"));
100204
}
101205

102206
@Get("/throw/http-status-exception")
@@ -106,38 +210,7 @@ public HttpResponse throwHttpStatusException() {
106210

107211
@Get("/throw/unexpected-exception")
108212
public HttpResponse throwUnexpectedException() {
109-
throw new IllegalStateException("Oops!");
110-
}
111-
}
112-
113-
private static final class ShouldRecordErrorResponseContentPreviewingArgumentsProvider
114-
implements ArgumentsProvider {
115-
@Override
116-
public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
117-
return Stream.of(Arguments.of("/aborted/http-status-exception",
118-
"errorHandler: 500 Internal Server Error"),
119-
Arguments.of("/aborted/unexpected-exception",
120-
"errorHandler: Oops!"),
121-
Arguments.of("/throw/http-status-exception",
122-
"errorHandler: 500 Internal Server Error"),
123-
Arguments.of("/throw/unexpected-exception",
124-
"errorHandler: Oops!"),
125-
Arguments.of("/annotated/aborted/http-status-exception",
126-
"errorHandler: 500 Internal Server Error"),
127-
Arguments.of("/annotated/aborted/unexpected-exception",
128-
"errorHandler: Oops!"),
129-
Arguments.of("/annotated/throw/http-status-exception",
130-
"errorHandler: 500 Internal Server Error"),
131-
Arguments.of("/annotated/throw/unexpected-exception",
132-
"errorHandler: Oops!"),
133-
Arguments.of("/annotatedExceptionHandler/aborted/http-status-exception",
134-
"exceptionHandler: 500 Internal Server Error"),
135-
Arguments.of("/annotatedExceptionHandler/aborted/unexpected-exception",
136-
"exceptionHandler: Oops!"),
137-
Arguments.of("/annotatedExceptionHandler/throw/http-status-exception",
138-
"exceptionHandler: 500 Internal Server Error"),
139-
Arguments.of("/annotatedExceptionHandler/throw/unexpected-exception",
140-
"exceptionHandler: Oops!"));
213+
throw new IllegalArgumentException("Oops!");
141214
}
142215
}
143216
}

0 commit comments

Comments
 (0)