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.

Sign up for GitHub

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

fix: request body logging [#84] #94

Merged
merged 2 commits into from
Dec 10, 2019
Merged

fix: request body logging [#84] #94

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
313e579
fix: request body logging [https://github.com/getsentry/sentry-go/iss…
Nov 27, 2019
c24128f
clean up
Dec 4, 2019
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
15 changes: 8 additions & 7 deletions interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package sentry

import (
"bytes"
"context"
"fmt"
"io/ioutil"
Expand Down Expand Up @@ -101,15 +102,15 @@ func (r Request) FromHTTPRequest(request *http.Request) Request {
r.QueryString = request.URL.RawQuery

// Body
if request.GetBody != nil {
if bodyCopy, err := request.GetBody(); err == nil && bodyCopy != nil {
body, err := ioutil.ReadAll(bodyCopy)
if err == nil {
r.Data = string(body)
}
if request.Body != nil {
bodyBytes, err := ioutil.ReadAll(request.Body)
rhcarvalho marked this conversation as resolved.
Show resolved Hide resolved
_ = request.Body.Close()
rhcarvalho marked this conversation as resolved.
Show resolved Hide resolved
if err == nil {
rhcarvalho marked this conversation as resolved.
Show resolved Hide resolved
// We have to restore original state of *request.Body
request.Body = ioutil.NopCloser(bytes.NewBuffer(bodyBytes))
r.Data = string(bodyBytes)
}
}

return r
}

Expand Down
37 changes: 37 additions & 0 deletions interfaces_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package sentry

import (
"bytes"
"io/ioutil"
"net/http"
"testing"
)

const testPayload = `{"test_data": true}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is not used anywhere else and to avoid polluting the global namespace for tests, how about we move this inside of the test method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


func TestRequest_FromHTTPRequest(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's awesome to see tests for this, thank you very much!!! ❤️

We follow the Go MixedCaps naming convention, could you please update the function name to TestRequestFromHTTPRequest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

t.Run("reading_body", func(t *testing.T) {
payload := bytes.NewBufferString(testPayload)
req, err := http.NewRequest("POST", "/test/", payload)
assertEqual(t, err, nil)
assertNotEqual(t, req, nil)
sentryRequest := Request{}
sentryRequest = sentryRequest.FromHTTPRequest(req)
assertEqual(t, sentryRequest.Data, testPayload)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

The body of the first test is exactly the same as the early portion of the second test.

Since the assert* helpers call t.Error, execution continues even after an "assertion error". In that case, I think it is enough to have just the second test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I Agree, second one should cover all we need


t.Run("reading_body_twice", func(t *testing.T) {
payload := bytes.NewBufferString(testPayload)
req, err := http.NewRequest("POST", "/test/", payload)
assertEqual(t, err, nil)
assertNotEqual(t, req, nil)
sentryRequest := Request{}
sentryRequest = sentryRequest.FromHTTPRequest(req)
assertEqual(t, sentryRequest.Data, testPayload)

// Re-reading original *http.Request.Body
reqBody, err := ioutil.ReadAll(req.Body)
assertEqual(t, err, nil)
assertEqual(t, string(reqBody), testPayload)
})
}