Relax validation for responses with empty content status. #5862
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: An exception is currently raised when aggregating a response if the response's status indicates empty content but content is present. However, a third-party legacy server might send a response with content despite an empty content status, which is beyond the client's control. Modification: - Changed the behavior to log a debug message instead of raising an exception when content exists for a status that indicates empty content. Result: - Armeria client now works with legacy servers that send content with an empty content status without raising exceptions.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5862 +/- ##
============================================
+ Coverage 74.57% 74.61% +0.04%
- Complexity 21643 21693 +50
============================================
Files 1893 1896 +3
Lines 80153 80383 +230
Branches 10523 10575 +52
============================================
+ Hits 59772 59980 +208
- Misses 15341 15344 +3
- Partials 5040 5059 +19 ☔ View full report in Codecov by Sentry. |
ikhoon
reviewed
Aug 8, 2024
| if (!tryWrite(headers)) { | ||
| return; | ||
| } | ||
|
|
||
| // Add content if not empty. | ||
| if (!contentAlwaysEmpty && !content.isEmpty()) { | ||
| if (headers.status().isContentAlwaysEmpty()) { |
There was a problem hiding this comment.
Need to release the resource?
Suggested change
| if (headers.status().isContentAlwaysEmpty()) { | |
| if (headers.status().isContentAlwaysEmpty()) { | |
| content.close(); |
There was a problem hiding this comment.
ikhoon
approved these changes
Aug 8, 2024
| if (!tryWrite(headers)) { | ||
| return; | ||
| } | ||
|
|
||
| // Add content if not empty. | ||
| if (!contentAlwaysEmpty && !content.isEmpty()) { | ||
| if (headers.status().isContentAlwaysEmpty()) { |
jrhee17
approved these changes
Aug 8, 2024
| import com.linecorp.armeria.common.util.Exceptions; | ||
|
|
||
| final class HttpResponseUtil { | ||
|
|
||
| static final Logger logger = LoggerFactory.getLogger(HttpResponseUtil.class); |
There was a problem hiding this comment.
optional) HttpResponseUtil seems like an package-private class which makes it susceptible to us refactoring this class without realizing that users are using this logger pattern
There was a problem hiding this comment.
Add a comment for that. 😉
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
An exception is currently raised when aggregating a response if the response's status indicates empty content but content is present. However, a third-party legacy server might send a response with content despite an empty content status, which is beyond the client's control.
Modification:
Result: