Skip to content

Set content preview to null when response fails with exception #6589

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ikhoon merged 1 commit into from
Jan 20, 2026
Merged

Set content preview to null when response fails with exception #6589

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/src/main/java/com/linecorp/armeria/internal/logging/ContentPreviewingUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ protected ContentPreviewerHttpResponse(
this.ctx = ctx;
whenComplete().handle((unused, cause) -> {
if (responseContentPreviewer == null) {
ctx.logBuilder().responseContentPreview(null);
return null;
}
if (!responseContentPreviewer.isDisabled()) {
Expand Down
1 change: 1 addition & 0 deletions core/src/main/java/com/linecorp/armeria/server/logging/ContentPreviewingService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ public HttpResponse serve(ServiceRequestContext ctx, HttpRequest req) throws Exc
final HttpResponse res = unwrap().serve(ctx, req);
return setUpResponseContentPreviewer(contentPreviewerFactory, ctx, res, responsePreviewSanitizer);
} catch (Throwable t) {
ctx.logBuilder().responseContentPreview(null);
return HttpResponse.ofFailure(t);
}
}
Expand Down
199 changes: 136 additions & 63 deletions ...src/test/java/com/linecorp/armeria/server/logging/ContentPreviewingErrorResponseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,18 @@

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

import java.util.stream.Stream;

import org.junit.jupiter.api.extension.ExtensionContext;
import org.junit.jupiter.api.extension.RegisterExtension;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.ArgumentsProvider;
import org.junit.jupiter.params.provider.ArgumentsSource;
import org.junit.jupiter.params.provider.ValueSource;

import com.linecorp.armeria.client.BlockingWebClient;
import com.linecorp.armeria.common.AggregatedHttpResponse;
import com.linecorp.armeria.common.HttpResponse;
import com.linecorp.armeria.common.HttpStatus;
import com.linecorp.armeria.common.RequestContext;
import com.linecorp.armeria.common.logging.RequestLog;
import com.linecorp.armeria.server.HttpStatusException;
import com.linecorp.armeria.server.ServerBuilder;
import com.linecorp.armeria.server.ServiceErrorHandler;
import com.linecorp.armeria.server.annotation.ExceptionHandlerFunction;
import com.linecorp.armeria.server.annotation.Get;
import com.linecorp.armeria.server.annotation.ProducesText;
Expand All @@ -43,48 +38,157 @@
class ContentPreviewingErrorResponseTest {

@RegisterExtension
static final ServerExtension server = new ServerExtension() {
static final ServerExtension serverWithErrorHandler = new ServerExtension() {
@Override
protected void configure(ServerBuilder sb) {
configureServer(sb, true);
}
};

@RegisterExtension
static final ServerExtension serverWithoutServerErrorHandler = new ServerExtension() {
@Override
protected void configure(ServerBuilder sb) {
configureServer(sb, false);
}
};

private static void configureServer(ServerBuilder sb, boolean errorHandler) {
if (errorHandler) {
sb.errorHandler((ctx, cause) -> {
return HttpResponse.of("errorHandler: " + cause.getMessage());
});
}

sb.service("/aborted/http-status-exception", (ctx, req) ->
HttpResponse.ofFailure(HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR)));
sb.service("/aborted/unexpected-exception", (ctx, req) ->
HttpResponse.ofFailure(new IllegalStateException("Oops!")));
sb.service("/aborted/http-status-exception", (ctx, req) ->
HttpResponse.ofFailure(HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR)));
sb.service("/aborted/unexpected-exception", (ctx, req) ->
HttpResponse.ofFailure(new IllegalStateException("Oops!")));

sb.service("/throw/http-status-exception", (ctx, req) -> {
throw HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR);
});
sb.service("/throw/unexpected-exception", (ctx, req) -> {
throw new IllegalStateException("Oops!");
});
sb.service("/throw/http-status-exception", (ctx, req) -> {
throw HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR);
});
sb.service("/throw/unexpected-exception", (ctx, req) -> {
throw new IllegalStateException("Oops!");
});

sb.annotatedService("/annotated", new ContentPreviewingAnnotatedService());
sb.annotatedService("/annotated", new ContentPreviewingAnnotatedService());
if (errorHandler) {
sb.annotatedService("/annotatedExceptionHandler",
new ContentPreviewingAnnotatedService(),
(ExceptionHandlerFunction) (ctx, req, cause) -> {
return HttpResponse.of("exceptionHandler: " + cause.getMessage());
});

sb.decorator(LoggingService.newDecorator());
sb.decorator(ContentPreviewingService.newDecorator(Integer.MAX_VALUE));
}
};

final ServiceErrorHandler serviceErrorHandler = (ctx, cause) -> {
return HttpResponse.of("serviceErrorHandler: " + cause.getMessage());
};
sb.withRoute(bindingBuilder -> {
if (errorHandler) {
bindingBuilder.errorHandler(serviceErrorHandler);
}
bindingBuilder.path("/binding/aborted/http-status-exception")
.build((ctx, req) -> {
return HttpResponse.ofFailure(
HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR));
});
});
sb.withRoute(bindingBuilder -> {
if (errorHandler) {
bindingBuilder.errorHandler(serviceErrorHandler);
}
bindingBuilder.path("/binding/aborted/unexpected-exception")
.build((ctx, req) -> {
return HttpResponse.ofFailure(new IllegalStateException("Oops!"));
});
});
sb.withRoute(bindingBuilder -> {
if (errorHandler) {
bindingBuilder.errorHandler(serviceErrorHandler);
}
bindingBuilder.path("/binding/throw/http-status-exception")
.build((ctx, req) -> {
throw HttpStatusException.of(HttpStatus.INTERNAL_SERVER_ERROR);
});
});
sb.withRoute(bindingBuilder -> {
if (errorHandler) {
bindingBuilder.errorHandler(serviceErrorHandler);
}
bindingBuilder.path("/binding/throw/unexpected-exception")
.build((ctx, req) -> {
throw new IllegalStateException("Oops!");
});
});

sb.decorator(LoggingService.newDecorator());
sb.decorator(ContentPreviewingService.newDecorator(Integer.MAX_VALUE));
}

@ParameterizedTest
@ValueSource(strings = {
"/aborted/http-status-exception",
"/aborted/unexpected-exception",
"/throw/http-status-exception",
"/throw/unexpected-exception",
"/annotated/aborted/http-status-exception",
"/annotated/aborted/unexpected-exception",
"/annotated/throw/http-status-exception",
"/annotated/throw/unexpected-exception",
"/binding/aborted/http-status-exception",
"/binding/aborted/unexpected-exception",
"/binding/throw/http-status-exception",
"/binding/throw/unexpected-exception",
})
void shouldSetNullContentPreviewWhenAnExceptionIsRaisedAndErrorHandlerSet(String path) throws Exception {
serverWithErrorHandler.blockingWebClient().get(path);

final RequestContext ctx = serverWithErrorHandler.requestContextCaptor().poll();
final RequestLog log = ctx.log().whenComplete().join();

assertThat(log.responseContentPreview()).isNull();
}

@ParameterizedTest
@ArgumentsSource(ShouldRecordErrorResponseContentPreviewingArgumentsProvider.class)
void shouldRecordErrorResponseContentPreviewing(String path, String responseContent) throws Exception {
final BlockingWebClient client = server.blockingWebClient();
final AggregatedHttpResponse res = client.get(path);
assertThat(res.contentUtf8()).isEqualTo(responseContent);
@ValueSource(strings = {
"/aborted/http-status-exception",
"/aborted/unexpected-exception",
"/throw/http-status-exception",
"/throw/unexpected-exception",
"/annotated/aborted/http-status-exception",
"/annotated/aborted/unexpected-exception",
"/annotated/throw/http-status-exception",
"/annotated/throw/unexpected-exception",
"/binding/aborted/http-status-exception",
"/binding/aborted/unexpected-exception",
"/binding/throw/http-status-exception",
"/binding/throw/unexpected-exception",
})
void shouldSetNullContentPreviewWhenAnExceptionIsRaisedAndNoErrorHandlerSet(String path) throws Exception {
serverWithoutServerErrorHandler.blockingWebClient().get(path);

final RequestContext ctx = serverWithoutServerErrorHandler.requestContextCaptor().poll();
final RequestLog log = ctx.log().whenComplete().join();

final RequestContext ctx = server.requestContextCaptor().poll();
assertThat(log.responseContentPreview()).isNull();
}

@ParameterizedTest
@ValueSource(strings = {
"/annotatedExceptionHandler/aborted/http-status-exception",
"/annotatedExceptionHandler/aborted/unexpected-exception",
"/annotatedExceptionHandler/throw/http-status-exception",
"/annotatedExceptionHandler/throw/unexpected-exception",
})
void annotatedServiceExceptionHandlerAlwaysRecordContentPreview(String path) throws Exception {
final AggregatedHttpResponse res = serverWithErrorHandler.blockingWebClient().get(path);
final String content = res.contentUtf8();

final RequestContext ctx = serverWithErrorHandler.requestContextCaptor().poll();
final RequestLog log = ctx.log().whenComplete().join();

assertThat(log.responseContentPreview()).isEqualTo(responseContent);
assertThat(log.responseContentPreview()).isEqualTo(content);
}

@ProducesText
Expand All @@ -96,7 +200,7 @@ public HttpResponse abortedHttpStatusException() {

@Get("/aborted/unexpected-exception")
public HttpResponse abortedUnexpectedException() {
return HttpResponse.ofFailure(new IllegalStateException("Oops!"));
return HttpResponse.ofFailure(new IllegalArgumentException("Oops!"));
}

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

@Get("/throw/unexpected-exception")
public HttpResponse throwUnexpectedException() {
throw new IllegalStateException("Oops!");
}
}

private static final class ShouldRecordErrorResponseContentPreviewingArgumentsProvider
implements ArgumentsProvider {
@Override
public Stream<? extends Arguments> provideArguments(ExtensionContext context) {
return Stream.of(Arguments.of("/aborted/http-status-exception",
"errorHandler: 500 Internal Server Error"),
Arguments.of("/aborted/unexpected-exception",
"errorHandler: Oops!"),
Arguments.of("/throw/http-status-exception",
"errorHandler: 500 Internal Server Error"),
Arguments.of("/throw/unexpected-exception",
"errorHandler: Oops!"),
Arguments.of("/annotated/aborted/http-status-exception",
"errorHandler: 500 Internal Server Error"),
Arguments.of("/annotated/aborted/unexpected-exception",
"errorHandler: Oops!"),
Arguments.of("/annotated/throw/http-status-exception",
"errorHandler: 500 Internal Server Error"),
Arguments.of("/annotated/throw/unexpected-exception",
"errorHandler: Oops!"),
Arguments.of("/annotatedExceptionHandler/aborted/http-status-exception",
"exceptionHandler: 500 Internal Server Error"),
Arguments.of("/annotatedExceptionHandler/aborted/unexpected-exception",
"exceptionHandler: Oops!"),
Arguments.of("/annotatedExceptionHandler/throw/http-status-exception",
"exceptionHandler: 500 Internal Server Error"),
Arguments.of("/annotatedExceptionHandler/throw/unexpected-exception",
"exceptionHandler: Oops!"));
throw new IllegalArgumentException("Oops!");
}
}
}
Loading