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 more context when failing to unmarshal JSON #833

Merged
merged 1 commit into from
Apr 15, 2019
Merged
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
31 changes: 29 additions & 2 deletions stripe.go
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ func (s *BackendImplementation) Do(req *http.Request, body *bytes.Buffer, v inte
s.LeveledLogger.Debugf("Response: %s\n", string(resBody))

if v != nil {
return json.Unmarshal(resBody, v)
return s.UnmarshalJSONVerbose(res.StatusCode, resBody, v)
}

return nil
Expand All @@ -450,9 +450,10 @@ func (s *BackendImplementation) Do(req *http.Request, body *bytes.Buffer, v inte
// ResponseToError converts a stripe response to an Error.
func (s *BackendImplementation) ResponseToError(res *http.Response, resBody []byte) error {
var raw rawError
if err := json.Unmarshal(resBody, &raw); err != nil {
if err := s.UnmarshalJSONVerbose(res.StatusCode, resBody, &raw); err != nil {
return err
}

// no error in resBody
if raw.E == nil {
err := errors.New(string(resBody))
Expand Down Expand Up @@ -505,6 +506,32 @@ func (s *BackendImplementation) SetNetworkRetriesSleep(sleep bool) {
s.networkRetriesSleep = sleep
}

// UnmarshalJSONVerbose unmarshals JSON, but in case of a failure logs and
// produces a more descriptive error.
func (s *BackendImplementation) UnmarshalJSONVerbose(statusCode int, body []byte, v interface{}) error {
err := json.Unmarshal(body, v)
if err != nil {
// If we got invalid JSON back then something totally unexpected is
// happening (caused by a bug on the server side). Put a sample of the
// response body into the error message so we can get a better feel for
// what the problem was.
bodySample := string(body)
if len(bodySample) > 500 {
bodySample = bodySample[0:500] + " ..."
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we log the whole body? Maybe we log verbosity? asking because when this happens we sometimes ask the user to give us what they received. We had this in a recent issue where the response were truncated incorrectly server-side after a certain size.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it's possible, but I'm basically trying to protect against a totally degenerate scenario where we say dump a 40k HTML page or something by accident.

I think we should pick a number and truncate at it as protection, but I'm okay with one that's on the bigger side though, like 500 or such. What do you think?


// Make sure a multi-line response ends up all on one line
bodySample = strings.Replace(bodySample, "\n", "\\n", -1)

newErr := fmt.Errorf("Couldn't deserialize JSON (response status: %v, body sample: '%s'): %v",
statusCode, bodySample, err)
s.LeveledLogger.Errorf("%s", newErr.Error())
return newErr
}

return nil
}

// Checks if an error is a problem that we should retry on. This includes both
// socket errors that may represent an intermittent problem and some special
// HTTP statuses.
Expand Down
45 changes: 45 additions & 0 deletions stripe_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,51 @@ func TestStripeAccount(t *testing.T) {
assert.Equal(t, TestMerchantID, req.Header.Get("Stripe-Account"))
}

func TestUnmarshalJSONVerbose(t *testing.T) {
type testServerResponse struct {
Message string `json:"message"`
}

backend := stripe.GetBackend(stripe.APIBackend).(*stripe.BackendImplementation)

// Valid JSON
{
type testServerResponse struct {
Message string `json:"message"`
}

var sample testServerResponse
err := backend.UnmarshalJSONVerbose(200, []byte(`{"message":"hello"}`), &sample)
assert.NoError(t, err)
assert.Equal(t, "hello", sample.Message)
}

// Invalid JSON (short)
{
body := `server error`

var sample testServerResponse
err := backend.UnmarshalJSONVerbose(200, []byte(body), &sample)
assert.Regexp(t,
fmt.Sprintf(`^Couldn't deserialize JSON \(response status: 200, body sample: '%s'\): invalid character`, body),
err)
}

// Invalid JSON (long, and therefore truncated)
{
// Assembles a body that's at least as long as our maximum sample.
// body is ~130 characters * 5.
bodyText := `this is a really long body that will be truncated when added to the error message to protect against dumping huge responses in logs `
body := bodyText + bodyText + bodyText + bodyText + bodyText

var sample testServerResponse
err := backend.UnmarshalJSONVerbose(200, []byte(body), &sample)
assert.Regexp(t,
fmt.Sprintf(`^Couldn't deserialize JSON \(response status: 200, body sample: '%s ...'\): invalid character`, body[0:500]),
err)
}
}

func TestUserAgent(t *testing.T) {
c := stripe.GetBackend(stripe.APIBackend).(*stripe.BackendImplementation)

Expand Down