Use custom unmarshalError handler for Route53

[abustany]

Certains Route53 calls like ChangeResourceRecordSets will return
specific error types, so we need a custom handler to take those into
account.

Open questions:

• The current awserr.Error type doesn't account for "composite" errors, so for now I just join the messages with \n... Is there a better way to solve that issue?
• Due to the API of private/protocol/query, I had to copy paste the unmarshalling for standard XML errors. One option would be to add a UnmarshalErrorFromData(statusCode int, body []byte) error function there.

[abustany]

(for the record: I based this PR on 51ee380)

[abustany]

Woooops, hadn't run make unit that runs the linter, fixed the struct names, CI should now hopefully pass.

[jasdel]

Hi @abustany thanks for taking the time to create this PR. Adding error handling for these classes of errors will be a good addition to the SDK.

To handle the composite errors I'd suggest creating a error sub type similar to awserr.RequestFailure and s3manager.MultiUploadFailure. For this case I'd add a new error interface type to awserr which still has a code and message, but also contains a list of additional error code and messages. Normal err.Error() would print out the error code and message like normal along with the batched errors. But if a user wants to get details on the specific batched errors they could cast awserr.Error to something like awserr.BatchedErrors.

I'll look through the SDK to see if batched errors can occur with other service API operations, but in the interim I think it makes sense for the base XML protocol parser to handle batched errors if it can be done generically.

[abustany]

ok, thanks a lot for the guidance, I'll cook a patch based on that. Looking at the APIs I know from AWS (and quickly grepping through the JSON API specs), I can't really find any other kind of batched errors (S3 multi-delete for example still returns HTTP 200 even if some deletes fail). There are many AWS APIs I'm not familiar with though, so it's quite likely that I missed some.

[abustany]

I wrote a quick and dirty go script to check errors structures across the various APIs: http://paste.fedoraproject.org/291392/76716614 (you can just go run it in the root of the directory). The output for the current HEAD is at http://paste.fedoraproject.org/291394/14477673 . Unless the script is doing something wrong, it looks like InvalidChangeBatch is the only structure of its kind in the API. I would therefore tend to argue that we should do a route53 specific error, instead of having the code in the restxml parser... If you still prefer to have it in the restxml package, I can of course also go ahead with that.

[jasdel]

Hi @abustany good idea to scan the models. I verified this as well InvalidChangeBatch is the only error type the SDK receives which is a batch of errors. In all other cases these batched error instances are included in the success respon. For future growth I think we still want the awserr.BatchedError interface to be a generic container for batched errors like this. But I think you're correct that using a custom error unmarshaller in Route53 is good idea.

Also it would be great to have some test cases of the custom error unmarshaller.

[abustany]

apologies about the delay on that one, I still intend to fix that issue, but have higher priorities at work at the moment.

[jasdel]

Thanks for the update @abustany no rush, we'll keep this PR open for you.

[abustany]

OK, here is a new version of the branch that adds a standard type to awserr for batched errors, and tests for route53 error unmarshalling. I'm still a bit unhappy about the unmarshalling code since it duplicates some of the code in the restxml package. If you're fine with me adding an additional method there to unmarshal an error from a given []byte payload instead of the body of the HTTP response (which we can't use since we already consumed it in the custom unmarhsaller), we can get rid of the code duplication (ie the standardXMLErrorResponse struct).

[jasdel]

Looks pretty good, and thanks for adding the tests also!

I agree getting rid of the duplication would be helpful. I know this is done for S3 and another service, but not adding additional duplication would be great. I'm curious what using a []byte instead of io.Reader would look like to remove the duplication.

In other places where we've needed to rewind a stream bytes.Reader was used to wrapped in an ioutil.NopCloser. This worked well, but isn't very flexible since the bytes.Reader is lost when its wrapped. I'm thinking that it makes sense to create a new type in the aws package which is composed of a bytes.Reader and also an nop closer. Something like:

type ByteReadCloser struct {
    *bytes.Reader
}
func (b *ByteReadCloser) Close() error { return nil }

This would more cleanly allow us to share the logic used for multi tiered error parsing.

[abustany]

So I updated the branch again, it looks like in that case I can get away by simply "monkey patching" the Body of the http.Request with a bytes.Reader as you suggested. I'm not fully sure what would be the use of the ByteReadCloser type you're suggesting (are there cases where we need to access the bytes.Reader again?), but then I haven't seen much of the rest of the code base :-)

[jasdel]

Thanks for the update @abustany. The benefit of using the ByteReadCloser type is the SDK's unmarshaller could rewind the body when multiple parsing attempts is needed for the body. Specifically this occurs when the request fails, and an error response is returned. Specifically like in this case where the error could be of multiple types.

#331 highlights this issue where multiple error unmarshalling are helpful.

[jasdel]

Replacing with #593 which is synced with the tip of master. Thanks again @abustany for taking the time create this PR and and investigate the issue.