Add decorator support for ServiceErrorHandler #6570
Conversation
0x1306e6d
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
📝 WalkthroughWalkthroughGeneralized error-handling decorators: removed the "Server" prefix from interfaces/classes, added overloads to let decorators wrap both ServerErrorHandler and ServiceErrorHandler, extracted CORS and content-preview logic into helper methods, and applied the new decorator wiring across builders, configs, and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6570 +/- ##
============================================
- Coverage 74.46% 74.38% -0.08%
- Complexity 22234 23714 +1480
============================================
Files 1963 2128 +165
Lines 82437 88614 +6177
Branches 10764 11587 +823
============================================
+ Hits 61385 65914 +4529
- Misses 15918 17165 +1247
- Partials 5134 5535 +401 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
Looks good overall, left a minor question
| static ServerErrorHandler decorate(ServerErrorHandler delegate) { | ||
| ServerErrorHandler decorated = delegate; | ||
| for (DecoratingServerErrorHandlerFunction decoratorFunction : decoratorFunctions) { | ||
| for (DecoratingErrorHandlerFunction decoratorFunction : decoratorFunctions) { |
There was a problem hiding this comment.
Question) Since the ServiceErrorHandler is decorated, is there still a need to decorate the ServerErrorHandler?
e.g. the following diff seems to still pass the test:
user@AL02437565 upstream-armeria % git diff
diff --git a/core/src/main/java/com/linecorp/armeria/server/ErrorHandlerDecorators.java b/core/src/main/java/com/linecorp/armeria/server/ErrorHandlerDecorators.java
index faa2867cf..a614d3af0 100644
--- a/core/src/main/java/com/linecorp/armeria/server/ErrorHandlerDecorators.java
+++ b/core/src/main/java/com/linecorp/armeria/server/ErrorHandlerDecorators.java
@@ -35,11 +35,7 @@ final class ErrorHandlerDecorators {
private ErrorHandlerDecorators() {}
static ServerErrorHandler decorate(ServerErrorHandler delegate) {
- ServerErrorHandler decorated = delegate;
- for (DecoratingErrorHandlerFunction decoratorFunction : decoratorFunctions) {
- decorated = new DecoratingServerErrorHandler(decorated, decoratorFunction);
- }
- return decorated;
+ return delegate;
}
static ServiceErrorHandler decorate(ServiceErrorHandler delegate) {
user@AL02437565 upstream-armeria %
The concern is that implementors of the default ErrorHandlerDecorators#decoratorFunctions need to be aware that each DecoratingErrorHandlerFunction may be applied twice.
There was a problem hiding this comment.
That's a good finding. I think we need to remove the decorating here:
and add it here instead:
There was a problem hiding this comment.
Thanks!
Regardless, I found another edge case where the content preview is not recorded. An HttpResponse can be created directly via renderStatus(...) when it is an AbortedHttpResponse. If a service returns HttpResponse.ofFailure(HttpResponseException(...)) without an error handler, the error response is created using renderStatus(...) directly:
Because onServiceException(...) is not called with the returned response, the request log does not complete. My idea is to apply the same decorating logic to the response returned from the delegate’s renderStatus(...).
I’m unsure whether there is any chance the decorator could be applied twice. For example, via renderStatus(...) -> FallbackService -> ContentPreviewingService or CorsService.
There was a problem hiding this comment.
Good catch, there's definitely an edge case here.
Plus, it looks like we're generating a content preview even when a preceding decorator fails, which we shouldn't be doing.
What do you think about preventing the preview from being generated whenever an exception is raised?
Since we already set the preview to null on an exception in DefaultRequestLog, this seems like the right fix.
armeria/core/src/main/java/com/linecorp/armeria/common/logging/DefaultRequestLog.java
Lines 1471 to 1473 in d068172
I think we could set the preview to null at these two spots to solve it:
There was a problem hiding this comment.
Plus, it looks like we're generating a content preview even when a preceding decorator fails, which we shouldn't be doing.
Indeed, because content preview generation is done after the response is processed by the recovery step and whether the content preview decorator was actually executed is not considered.
What do you think about preventing the preview from being generated whenever an exception is raised?
If so, should we generate the preview for an aborted response (HttpResponse.ofFailure(...))? I’m a bit confused about what ContentPreviewErrorHandler would do if we choose this behavior.
There was a problem hiding this comment.
Glad to hear that. I agree that #6577 is the ultimate goal for addressing all of these issues.
There was a problem hiding this comment.
@0x1306e6d So, could you fix this PR to always complete the request log?
There was a problem hiding this comment.
I don't fully understand what 'always complete the request log' means. Could you please provide more details? If you meant setting the content preview to null for failed responses, I’m unsure how to retrieve the failed response preview until #6577 is available.
There was a problem hiding this comment.
If you meant setting the content preview to null for failed responses,
Yeah, that's what I meant. 😉
I initially thought we should prioritize the completeness of the request log, even if it meant disabling the content preview on failed responses. The idea was to ship this fix before #6577.
However, we already released 1.35.0 and Since #6577 will likely be included in the next release, we can probably wait for that.
So if you don't want to apply the workaround for this PR, I think we can just close this PR and implement #6577.
There was a problem hiding this comment.
I see. I added a commit to ensure the termination of RequestLog, and #6577 will provide the fundamental fix for the content preview.
One question I have is that the RequestLog termination may have different characteristics from the original PR. Also, CorsService requires a ServiceErrorHandler decorator. Shouldn’t we separate these into different PRs for better management?
jrhee17
left a comment
jrhee17
left a comment
There was a problem hiding this comment.
I'm still organizing my thoughts on https://github.com/line/armeria/pull/6570/files#r2655383994.
As this PR is an independent change, let me approve for now
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/test/java/com/linecorp/armeria/server/logging/ContentPreviewingErrorResponseTest.java (1)
203-213: Consider exception type consistency.The annotated service methods now use
IllegalArgumentException(lines 203, 213), while the direct service bindings still useIllegalStateException(lines 66, 72, 103, 121). While this doesn't affect test correctness, using consistent exception types would improve readability and reduce potential confusion.♻️ Optional: Use consistent exception types
If intentional variety is not required for test coverage, consider standardizing to one exception type:
@Get("/aborted/unexpected-exception") public HttpResponse abortedUnexpectedException() { - return HttpResponse.ofFailure(new IllegalArgumentException("Oops!")); + return HttpResponse.ofFailure(new IllegalStateException("Oops!")); } @Get("/throw/unexpected-exception") public HttpResponse throwUnexpectedException() { - throw new IllegalArgumentException("Oops!"); + throw new IllegalStateException("Oops!"); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/java/com/linecorp/armeria/internal/logging/ContentPreviewingUtil.javacore/src/main/java/com/linecorp/armeria/server/logging/ContentPreviewingService.javacore/src/test/java/com/linecorp/armeria/server/logging/ContentPreviewingErrorResponseTest.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/test/java/com/linecorp/armeria/server/logging/ContentPreviewingErrorResponseTest.javacore/src/main/java/com/linecorp/armeria/server/logging/ContentPreviewingService.javacore/src/main/java/com/linecorp/armeria/internal/logging/ContentPreviewingUtil.java
🧬 Code graph analysis (1)
core/src/test/java/com/linecorp/armeria/server/logging/ContentPreviewingErrorResponseTest.java (3)
core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java (1)
ServerBuilder(171-2569)core/src/main/java/com/linecorp/armeria/server/logging/LoggingService.java (1)
LoggingService(39-88)core/src/main/java/com/linecorp/armeria/server/logging/ContentPreviewingService.java (1)
ContentPreviewingService(61-183)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-11
- GitHub Check: build-ubicloud-standard-16-jdk-17-leak
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-25
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-21-snapshot-blockhound
- GitHub Check: build-ubicloud-standard-16-jdk-8
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-17-coverage
- GitHub Check: build-ubicloud-standard-16-jdk-11
- GitHub Check: site
- GitHub Check: lint
- GitHub Check: flaky-tests
- GitHub Check: Kubernetes Chaos test
- GitHub Check: Summary
🔇 Additional comments (5)
core/src/main/java/com/linecorp/armeria/server/logging/ContentPreviewingService.java (1)
174-181: LGTM! Properly finalizes deferred log property on exception.The explicit
responseContentPreview(null)call ensures the deferredRESPONSE_CONTENT_PREVIEWproperty is always completed, even when the service throws an exception. This prevents incomplete logs and aligns with the PR objective to properly handle error responses.core/src/main/java/com/linecorp/armeria/internal/logging/ContentPreviewingUtil.java (1)
116-120: LGTM! Ensures log completion when previewer is uninitialized.Explicitly setting
responseContentPreview(null)when the previewer is never initialized ensures the deferred log property is always finalized. This handles cases where the response fails before headers are sent, maintaining log consistency across all execution paths.core/src/test/java/com/linecorp/armeria/server/logging/ContentPreviewingErrorResponseTest.java (3)
41-61: LGTM! Clean dual-server test setup.The two
ServerExtensioninstances enable comprehensive testing of both scenarios: with and without error handlers. This structure effectively validates that content previews are properly handled in both configurations, which aligns with the PR objective.
84-123: LGTM! Binding routes effectively test ServiceErrorHandler integration.The binding-based routes with
ServiceErrorHandler(lines 84-123) directly validate the core PR objective: ensuring that error handler decorators now apply toServiceErrorHandlerinstances. The comprehensive coverage of error scenarios (aborted/throw × http-status/unexpected) ensures the decorator integration works correctly.
129-192: LGTM! Comprehensive parameterized test coverage.The three test methods effectively validate:
- Content preview is null when error handlers are configured (both server and service level)
- Content preview is null when no error handlers are present
- Annotated service exception handlers properly record content previews
The inclusion of binding routes in test parameters ensures
ServiceErrorHandlerdecorator integration is properly validated, which is the core objective of this PR.
4 participants
Motivation:
Following #6269, error handler decorators like
CorsServerErrorHandlerandContentPreviewServerErrorHandlerwere introduced to support decoratingServerErrorHandler. However, these decorators only worked withServerErrorHandlers and did not supportServiceErrorHandlerconfigured viaServiceBindingBuilder.errorHandler(). If aServiceErrorHandleris configured, theServerErrorHandleracts as a fallback, so the error response is returned directly without passing through decorators:armeria/core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java
Lines 1393 to 1395 in 6777284
Modifications:
DecoratingErrorHandlerFunctionto support bothServerErrorHandlerandServiceErrorHandler.ServiceConfigBuilderto automatically decorateServiceErrorHandlerinstances.Result:
ServiceErrorHandler.