Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add StringWriter interface to ResponseWriter #921

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 32 additions & 18 deletions http/interceptor.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,25 @@ func (i *rwInterceptor) Write(b []byte) (int, error) {
return i.w.Write(b)
}

// WriteString buffers the response body until the request body limit is reach or an
// interruption is triggered, this buffer is later used to analyse the body in
// the response processor.
// If the body isn't accessible or the mime type isn't processable, the response
// body is being written to the delegate response writer directly.
func (i *rwInterceptor) WriteString(s string) (n int, err error) {
return i.Write([]byte(s))
}

func (i *rwInterceptor) Header() http.Header {
return i.w.Header()
}

var _ http.ResponseWriter = (*rwInterceptor)(nil)
type responseWriter interface {
http.ResponseWriter
io.StringWriter
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind pointing at an example of another similar code that implements io.StringWriter? This doesn't seem so bad, but I'm wondering when a caller wouldn't just call Write([]byte(s)) themselves without inspecting the interfaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used to a thing but now I only find old references.
Like these :

I suspect that WriteString on ResponseWriter is no longer common and that Write([]bytes(s)) is the idiomatic way.


We still have a bunch of tests to make sure that all middleware we add implements the needed interfaces. But it has been a while (±6 years) since I revisited those tests.

e.g. CloseNotifier is also unimplemented here but that is documented as deprecated in http.

So, yeah, this is ok to close for me :)
It can always be revisited if there is an actual need for this and not because of some legacy requirement.


Thank you for the super speedy review 🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks a lot for the investigation @romainmenke! Yeah let's go ahead and close for now then, if we find a strong use case for it we can reintroduce it.

}

var _ responseWriter = (*rwInterceptor)(nil)

// wrap wraps the interceptor into a response writer that also preserves
// the http interfaces implemented by the original response writer to avoid
Expand Down Expand Up @@ -175,103 +189,103 @@ func wrap(w http.ResponseWriter, r *http.Request, tx types.Transaction) (
switch {
case !isHijacker && !isPusher && !isFlusher && !isReader:
return struct {
http.ResponseWriter
responseWriter
}{i}, responseProcessor
case !isHijacker && !isPusher && !isFlusher && isReader:
return struct {
http.ResponseWriter
responseWriter
io.ReaderFrom
}{i, reader}, responseProcessor
case !isHijacker && !isPusher && isFlusher && !isReader:
return struct {
http.ResponseWriter
responseWriter
http.Flusher
}{i, flusher}, responseProcessor
case !isHijacker && !isPusher && isFlusher && isReader:
return struct {
http.ResponseWriter
responseWriter
http.Flusher
io.ReaderFrom
}{i, flusher, reader}, responseProcessor
case !isHijacker && isPusher && !isFlusher && !isReader:
return struct {
http.ResponseWriter
responseWriter
http.Pusher
}{i, pusher}, responseProcessor
case !isHijacker && isPusher && !isFlusher && isReader:
return struct {
http.ResponseWriter
responseWriter
http.Pusher
io.ReaderFrom
}{i, pusher, reader}, responseProcessor
case !isHijacker && isPusher && isFlusher && !isReader:
return struct {
http.ResponseWriter
responseWriter
http.Pusher
http.Flusher
}{i, pusher, flusher}, responseProcessor
case !isHijacker && isPusher && isFlusher && isReader:
return struct {
http.ResponseWriter
responseWriter
http.Pusher
http.Flusher
io.ReaderFrom
}{i, pusher, flusher, reader}, responseProcessor
case isHijacker && !isPusher && !isFlusher && !isReader:
return struct {
http.ResponseWriter
responseWriter
http.Hijacker
}{i, hijacker}, responseProcessor
case isHijacker && !isPusher && !isFlusher && isReader:
return struct {
http.ResponseWriter
responseWriter
http.Hijacker
io.ReaderFrom
}{i, hijacker, reader}, responseProcessor
case isHijacker && !isPusher && isFlusher && !isReader:
return struct {
http.ResponseWriter
responseWriter
http.Hijacker
http.Flusher
}{i, hijacker, flusher}, responseProcessor
case isHijacker && !isPusher && isFlusher && isReader:
return struct {
http.ResponseWriter
responseWriter
http.Hijacker
http.Flusher
io.ReaderFrom
}{i, hijacker, flusher, reader}, responseProcessor
case isHijacker && isPusher && !isFlusher && !isReader:
return struct {
http.ResponseWriter
responseWriter
http.Hijacker
http.Pusher
}{i, hijacker, pusher}, responseProcessor
case isHijacker && isPusher && !isFlusher && isReader:
return struct {
http.ResponseWriter
responseWriter
http.Hijacker
http.Pusher
io.ReaderFrom
}{i, hijacker, pusher, reader}, responseProcessor
case isHijacker && isPusher && isFlusher && !isReader:
return struct {
http.ResponseWriter
responseWriter
http.Hijacker
http.Pusher
http.Flusher
}{i, hijacker, pusher, flusher}, responseProcessor
case isHijacker && isPusher && isFlusher && isReader:
return struct {
http.ResponseWriter
responseWriter
http.Hijacker
http.Pusher
http.Flusher
io.ReaderFrom
}{i, hijacker, pusher, flusher, reader}, responseProcessor
default:
return struct {
http.ResponseWriter
responseWriter
}{i}, responseProcessor
}
}
30 changes: 30 additions & 0 deletions http/interceptor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
package http

import (
"io"
"net/http"
"net/http/httptest"
"testing"
Expand Down Expand Up @@ -44,3 +45,32 @@ func TestWriteHeader(t *testing.T) {
t.Errorf("unexpected status code, want %d, have %d", want, have)
}
}

func TestWriteString(t *testing.T) {
waf, err := coraza.NewWAF(coraza.NewWAFConfig())
if err != nil {
t.Fatal(err)
}

tx := waf.NewTransaction()
req, _ := http.NewRequest("GET", "", nil)
res := httptest.NewRecorder()
rw, responseProcessor := wrap(res, req, tx)
n, err := rw.(io.StringWriter).WriteString("hello")
if err != nil {
t.Fatal(err)
}

if n != 5 {
t.Errorf("unexpected number of bytes written, want 5, have %d", n)
}

err = responseProcessor(tx, req)
if err != nil {
t.Errorf("unexpected error: %v", err)
}

if want, have := "hello", res.Body.String(); want != have {
t.Errorf("unexpected response body, want %s, have %s", want, have)
}
}