Check errIn, not err
#754
Check errIn, not err
#754
Conversation
It really looks like `errIn` should be checked, not `err`.
| @@ -437,12 +437,12 @@ func (c Client) exec(verb, url string, headers map[string]string, body io.Reader | |||
| // response contains storage service error object, unmarshal | |||
| if resp.Header.Get("Content-Type") == "application/xml" { | |||
| errIn := serviceErrFromXML(respBody, &storageErr) | |||
| if err != nil { // error unmarshaling the error response | |||
| if errIn != nil { // error unmarshaling the error response | |||
| err = errIn | |||
There was a problem hiding this comment.
Good catch seeing the masked error. However, I'm not sure this fix goes far enough to fix it. As it is in this PR, errIn gets blown away anyway.
@mcardosos, what are your thoughts? Maybe we should be extending the AzureStorageServiceError to include an inner error in the case of marshaling trouble?
There was a problem hiding this comment.
Take a look into the serviceErrFromXML and serviceErrFromJSON funcs. They both include the unmarshal errors in AzureStorageServiceError.Message. I actually think we could get rid of if err != nil. Code would look like this...
if resp.Header.Get("Content-Type") == "application/xml" {
serviceErrFromXML(respBody, &storageErr)
} else {
serviceErrFromJSON(respBody, &storageErr)
}There was a problem hiding this comment.
Talking IRL with @mcardosos, she showed me that we do actually percolate up the un-marshaling error already. I like the blurb that she has above, let's do that :)
There was a problem hiding this comment.
@marstr I noticed afterwards that the error is blown away, but thought that you need to take a look at the whole code block. As I did this at work I didn't want to put too much time into it.
|
👋🏻 out of interest does this fix #748? |
|
@tombuildsstuff These issues are not related :( |
|
@md2perpe ping |
|
@mcardosos pong |
|
It seems like this PR has gone stale. I'm going to close it out. |
It really looks like
errInshould be checked, noterr.