fix http.Flusher and io.ReaderFrom implementation
#923
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #923 +/- ##
==========================================
+ Coverage 81.81% 82.57% +0.75%
==========================================
Files 160 160
Lines 9064 8988 -76
==========================================
+ Hits 7416 7422 +6
+ Misses 1399 1317 -82
Partials 249 249
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
http/interceptor.go
Outdated
| } | ||
|
|
||
| func (i *rwInterceptor) Flush() { | ||
| // coraza middleware always needs to buffer the entire request, response cycle |
There was a problem hiding this comment.
I wonder if we should drop a log here same way we do in https://github.com/corazawaf/coraza/pull/923/files#diff-110e39705812e7ff833d85e01bd91376daaf071b1bd9c458fc5ed9a0287a88a6L34
There was a problem hiding this comment.
This comment made me take a closer look at http.Flusher and there is one thing that can and should be in this implementation.
WriteHeader must be called if it wasn't called before with a status code of 200.
Thank you for pointing out that the was room for improvement here.
|
It looks good to me, just fix the coverage. Any thoughts @anuraaga ? |
Co-authored-by: José Carlos Chávez <[email protected]>
…ish-f8e6e94207' of https://github.com/romainmenke/coraza into fix-flush-and-readerfrom-implementation--generous-starfish-f8e6e94207
|
I am done with edits now. Thank you @jcchavezs for the review 🙇 |
|
This should trigger a release I think.
…On Tue, 21 Nov 2023, 06:28 Anuraag Agrawal, ***@***.***> wrote:
Merged #923 <#923> into main.
—
Reply to this email directly, view it on GitHub
<#923 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXOYAW2CFRQ43AOMGJEO6DYFQ3WBAVCNFSM6AAAAAA7TERI2CVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJRGAYTSNJWHA2DMOA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
romainmenke
deleted the
fix-flush-and-readerfrom-implementation--generous-starfish-f8e6e94207
branch
|
Thank you for the reviews everyone 🙇 |
|
@mholt do you think this is something we should port to the caddy connector? |
|
That might be a good idea if it fixes a bug. |
Make sure that you've checked the boxes below before you submit PR:
Before this change
http.Flusherandio.ReaderFromessentially acted like a bypass for the coraza middleware because they directly called the underlying writer.http.Flusheris unimplementable because it doesn't make sense to flush early to the client. It does have one observable effect. A call toFlushbefore a call toWrite(orWriteHeader) also implies a call toWriteHeader.io.ReaderFromcan be implemented by linking it withWritethroughio.Copy