Skip to content

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

Merged
ikhoon merged 1 commit intoline:mainfrom
0x1306e6d:complete-request-log
Jan 20, 2026
Merged

Set content preview to null when response fails with exception#6589
ikhoon merged 1 commit intoline:mainfrom
0x1306e6d:complete-request-log

Conversation

@0x1306e6d
Copy link
Copy Markdown
Contributor

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:

// Will auto-fill response content and its preview if response has failed.
deferredFlags = this.deferredFlags & ~(RESPONSE_CONTENT.flag() |
RESPONSE_CONTENT_PREVIEW.flag());

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.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 15, 2026

📝 Walkthrough

Walkthrough

These changes add explicit null logging for response content previews in error scenarios and refactor the error response test suite to use parameterized testing with multiple server configurations for comprehensive behavior verification.

Changes

Cohort / File(s) Summary
Response Content Preview Logging
core/src/main/java/com/linecorp/armeria/internal/logging/ContentPreviewingUtil.java, core/src/main/java/com/linecorp/armeria/server/logging/ContentPreviewingService.java
Added ctx.logBuilder().responseContentPreview(null) calls in two locations: when response previewer is unavailable and in the exception handling path, ensuring null content previews are explicitly logged.
Error Response Test Refactoring
core/src/test/java/com/linecorp/armeria/server/logging/ContentPreviewingErrorResponseTest.java
Restructured test infrastructure to support parameterized testing with two ServerExtension instances, introduced centralized configureServer helper method, migrated from ArgumentsProvider to ValueSource, added conditional error handler wiring, and reworked test assertions to verify behavior across error handler presence/absence scenarios.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 Null previews now logged, oh what a treat,
When errors hop by, responses stay neat,
Tests parameterized, servers now two,
Error handlers dance—with or without their shoe! 🥾

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Set content preview to null when response fails with exception' is clear, concise, and directly describes the main change: setting content preview to null in exception scenarios.
Description check ✅ Passed The description comprehensively explains the motivation (inconsistency with DefaultRequestLog), modifications made to three files, and the expected result of the changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d897515 and 5f16553.

📒 Files selected for processing (3)
  • core/src/main/java/com/linecorp/armeria/internal/logging/ContentPreviewingUtil.java
  • core/src/main/java/com/linecorp/armeria/server/logging/ContentPreviewingService.java
  • core/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 in site/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 @UnstableApi annotation.
  • However, this annotation is NOT required under the following conditions:
    • If the class or method is located in a package containing .internal or .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/main/java/com/linecorp/armeria/internal/logging/ContentPreviewingUtil.java
  • core/src/main/java/com/linecorp/armeria/server/logging/ContentPreviewingService.java
  • core/src/test/java/com/linecorp/armeria/server/logging/ContentPreviewingErrorResponseTest.java
⏰ 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-21-snapshot-blockhound
  • GitHub Check: build-windows-latest-jdk-25
  • GitHub Check: build-ubicloud-standard-16-jdk-11
  • GitHub Check: build-ubicloud-standard-16-jdk-8
  • GitHub Check: build-ubicloud-standard-16-jdk-17-leak
  • GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-17-coverage
  • GitHub Check: build-ubicloud-standard-16-jdk-25
  • GitHub Check: build-macos-latest-jdk-25
  • GitHub Check: Kubernetes Chaos test
  • GitHub Check: lint
  • GitHub Check: flaky-tests
  • GitHub Check: site
  • GitHub Check: Summary
🔇 Additional comments (7)
core/src/main/java/com/linecorp/armeria/server/logging/ContentPreviewingService.java (1)

178-181: LGTM! Correctly sets null preview when exception occurs.

This ensures the deferred RESPONSE_CONTENT_PREVIEW property is fulfilled when the service throws an exception, maintaining consistency with DefaultRequestLog behavior.

core/src/main/java/com/linecorp/armeria/internal/logging/ContentPreviewingUtil.java (1)

116-120: LGTM! Handles the case where response fails before headers are received.

When responseContentPreviewer is null, it means ResponseHeaders were never received (e.g., HttpResponse.ofFailure() was used). Explicitly setting the preview to null ensures the deferred log property is fulfilled.

core/src/test/java/com/linecorp/armeria/server/logging/ContentPreviewingErrorResponseTest.java (5)

40-54: LGTM! Clean test setup with two server configurations.

The dual server extensions effectively test both scenarios (with and without error handler) while sharing configuration logic via configureServer.


56-127: LGTM! Comprehensive server configuration for all failure scenarios.

The configuration covers multiple failure patterns (throw vs. ofFailure) across different route types (direct, annotated, binding) with proper conditional error handler wiring.


129-175: LGTM! Parameterized tests properly verify null preview for both configurations.

The tests comprehensively cover all failure paths and verify that responseContentPreview is null regardless of whether an error handler is configured.


177-192: LGTM! Correctly tests that annotated exception handlers record content preview.

This test validates the expected behavioral difference: annotated service exception handlers produce responses before ContentPreviewingService in the decorator chain, so their responses are previewed normally, unlike server/service-level error handlers.


194-215: LGTM! Annotated service provides complete test coverage for failure scenarios.

The service properly implements all four combinations of failure patterns for annotated service testing.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 15, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.38%. Comparing base (8150425) to head (5f16553).
⚠️ Report is 321 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6589      +/-   ##
============================================
- Coverage     74.46%   74.38%   -0.08%     
- Complexity    22234    23715    +1481     
============================================
  Files          1963     2128     +165     
  Lines         82437    88596    +6159     
  Branches      10764    11585     +821     
============================================
+ Hits          61385    65903    +4518     
- Misses        15918    17159    +1241     
- Partials       5134     5534     +400     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍

Copy link
Copy Markdown
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍👍

@ikhoon ikhoon added the defect label Jan 16, 2026
@ikhoon ikhoon merged commit 7915847 into line:main Jan 20, 2026
14 of 18 checks passed
@ikhoon ikhoon added this to the 1.36.0 milestone Jan 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants