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

don't unmarshall http error response #30

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@tuxcanfly
Copy link
Contributor

tuxcanfly commented Sep 15, 2014

e.g 429 Too many requests is parsed and a cryptic error message is shown:

rpcCommand: error reading json message: error unmarshalling json reply: invalid character 'T' after top-level value

instead, don't parse unless the response code indicates success

@tuxcanfly

This comment has been minimized.

Copy link
Contributor Author

tuxcanfly commented Sep 15, 2014

Note:

  • Removed duplicate "error getting json reply" error prefixes
  • Removed hardcoded branch for "401 Unauthorized"

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:handle-http-errors branch from 88f5ca0 to 5827c38 Sep 16, 2014

@tuxcanfly

This comment has been minimized.

Copy link
Contributor Author

tuxcanfly commented Sep 16, 2014

Build is probably failing due to recent version of golint working on packages instead of files:

golang/lint#63 (comment)

Removing the *.go from the lint script should fix this

fgt golint -min_confidence=0.9

Unfortunately this won't lint test files in the btcjson_test package

@jcvernaleo

This comment has been minimized.

Copy link
Member

jcvernaleo commented Sep 16, 2014

@tuxcanfly I've updated the travis config for btcjson. Please rebase the PR with that and then I'll take a look at it.
Thanks!

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:handle-http-errors branch from 5827c38 to 086c4ac Sep 16, 2014

@tuxcanfly

This comment has been minimized.

Copy link
Contributor Author

tuxcanfly commented Sep 16, 2014

@jcvernaleo yup, rebased

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:handle-http-errors branch from 086c4ac to 316f1a9 Sep 16, 2014

@tuxcanfly

This comment has been minimized.

Copy link
Contributor Author

tuxcanfly commented Sep 16, 2014

the assumption here is that only 200 OK responses are safe for unmarshalling, which IMO is reasonable.

@jcvernaleo

This comment has been minimized.

Copy link
Member

jcvernaleo commented Sep 16, 2014

@tuxcanfly Branch looks good except for the 401 Unauthorized. I think that error should be treated differently. Was there some specific reason why you wanted to remove it?

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:handle-http-errors branch from 316f1a9 to c78ca58 Sep 16, 2014

@tuxcanfly

This comment has been minimized.

Copy link
Contributor Author

tuxcanfly commented Sep 16, 2014

None, I thought it might not be required anymore. Restored.

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:handle-http-errors branch 3 times, most recently from 9c55cc1 to 04c50b1 Sep 16, 2014

@tuxcanfly

This comment has been minimized.

Copy link
Contributor Author

tuxcanfly commented Sep 16, 2014

Trimmed down the changes to avoid updating GetRaw signature. The 401 check wouldn't be hit after all, so removed it.

jsonapi.go Outdated
@@ -712,6 +713,10 @@ func rpcRawCommand(user string, password string, server string,
return result, err
}
result, err = GetRaw(resp.Body)
if resp.StatusCode != http.StatusOK {

This comment has been minimized.

@jrick

jrick Sep 16, 2014

Member

Wouldn't it make more sense to check for a valid response status code before reading the response body?

This comment has been minimized.

@tuxcanfly

tuxcanfly Sep 16, 2014

Author Contributor

We need the body for a descriptive error message.

} else {
err = fmt.Errorf("error unmarshalling json reply: %v", err)
}
err = fmt.Errorf("error unmarshalling json reply: %v", err)

This comment has been minimized.

@jrick

jrick Sep 16, 2014

Member

Perhaps it would be better to just return the error returned by the json package, instead of wrapping it.

This comment has been minimized.

@tuxcanfly

tuxcanfly Sep 16, 2014

Author Contributor

Yeah, it's already being wrapped by "error reading json message" outside in rpcCommand

This comment has been minimized.

@tuxcanfly

tuxcanfly Sep 16, 2014

Author Contributor

Updated.

jsonapi.go Outdated
@@ -712,6 +713,10 @@ func rpcRawCommand(user string, password string, server string,
return result, err
}
result, err = GetRaw(resp.Body)
if resp.StatusCode != http.StatusOK {
err := fmt.Errorf("error getting json reply: %s", result)

This comment has been minimized.

@jrick

jrick Sep 16, 2014

Member

result may or may not be set here, and it's unrelated to the bad status code. I'd do something like this instead:

return nil, fmt.Errorf("bad HTTP status code: %d", resp.StatusCode)

Or, even better:

type BadStatusCode int

func (e BadStatusCode) Error() string {
    status := int(e)
    return fmt.Sprintf("http bad status: %d %s", status, http.StatusText(status))
}

return nil, BadStatusCode(resp.StatusCode)

This comment has been minimized.

@tuxcanfly

tuxcanfly Sep 16, 2014

Author Contributor

Yup, looks better. Pushed.

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:handle-http-errors branch 3 times, most recently from 9e84880 to 5d94f18 Sep 16, 2014

@tuxcanfly tuxcanfly force-pushed the tuxcanfly:handle-http-errors branch from 5d94f18 to 4833f85 Sep 16, 2014

@jrick

This comment has been minimized.

Copy link
Member

jrick commented Sep 16, 2014

ok

@jcvernaleo

This comment has been minimized.

Copy link
Member

jcvernaleo commented Sep 16, 2014

Thanks @tuxcanfly !
I added ok's to the commit message so I need to manually close this PR, but it is in.

@jcvernaleo jcvernaleo closed this Sep 16, 2014

@tuxcanfly tuxcanfly deleted the tuxcanfly:handle-http-errors branch Sep 16, 2014

@tuxcanfly

This comment has been minimized.

Copy link
Contributor Author

tuxcanfly commented Sep 16, 2014

Great, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment