Enable request buffering when request body tracing#3440
Enable request buffering when request body tracing#3440candrews wants to merge 1 commit intospring-cloud:2.0.xfrom
Conversation
Request body tracing reads the body then it will be read again later for actual handling. Therefore, request buffering is required when request body tracing. Fixes spring-cloud#3418
Codecov Report
@@ Coverage Diff @@
## 2.0.x #3440 +/- ##
============================================
- Coverage 66.65% 66.14% -0.52%
+ Complexity 1484 1482 -2
============================================
Files 183 183
Lines 6922 6924 +2
Branches 845 846 +1
============================================
- Hits 4614 4580 -34
- Misses 1994 2028 +34
- Partials 314 316 +2
Continue to review full report at Codecov.
|
3 similar comments
Codecov Report
@@ Coverage Diff @@
## 2.0.x #3440 +/- ##
============================================
- Coverage 66.65% 66.14% -0.52%
+ Complexity 1484 1482 -2
============================================
Files 183 183
Lines 6922 6924 +2
Branches 845 846 +1
============================================
- Hits 4614 4580 -34
- Misses 1994 2028 +34
- Partials 314 316 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## 2.0.x #3440 +/- ##
============================================
- Coverage 66.65% 66.14% -0.52%
+ Complexity 1484 1482 -2
============================================
Files 183 183
Lines 6922 6924 +2
Branches 845 846 +1
============================================
- Hits 4614 4580 -34
- Misses 1994 2028 +34
- Partials 314 316 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## 2.0.x #3440 +/- ##
============================================
- Coverage 66.65% 66.14% -0.52%
+ Complexity 1484 1482 -2
============================================
Files 183 183
Lines 6922 6924 +2
Branches 845 846 +1
============================================
- Hits 4614 4580 -34
- Misses 1994 2028 +34
- Partials 314 316 +2
Continue to review full report at Codecov.
|
| // If it's going through the dispatcher we need to buffer the body | ||
| ctx.setRequest(new Servlet30RequestWrapper(request)); | ||
| } | ||
| else if (zuulProperties.isTraceRequestBody()) { |
There was a problem hiding this comment.
I'm not sure this is exactly what I described.
turn off trace request body if zuul.use-filter=true.
This enables buffering if isTraceRequestBody() which isn't the same.
There was a problem hiding this comment.
turn off trace request body if zuul.use-filter=true.
What you suggest seems like it'll be really awkward to program and unexpected for users:
if zuul.use-filter=true, ignore zuul.traceRequestBody and treat it as if it were always false. This implementation would confuse users who expect zuul.traceRequestBody to do something, and it's weird for traceRequestBody to be a variable that's sometimes ignored. IMHO, making zuul.traceRequestBody always work as expected is a better solution for maintainability and users alike, which is the approach I took in this PR.
There was a problem hiding this comment.
It also unexpectedly introduces buffering because trace request body is true by default. I won't apply this PR as is.
There was a problem hiding this comment.
Can you provide any guidance on what I can do?
Things I can think of include making some other change that you suggest or rebasing against another branch.
There was a problem hiding this comment.
Add another case here
There was a problem hiding this comment.
Apologies for my density... what case are suggesting be added?
There was a problem hiding this comment.
if zuul.use-filter=true: return false
3 participants
Request body tracing reads the body then it will be read again later for actual handling.
Therefore, request buffering is required when request body tracing.
Fixes #3418