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

S3: REST-XML prototype unmarshaler deligates to Legacy Unmarshaler (GetObject operation) #470

Merged

Conversation

skotambkar
Copy link
Contributor

  • PR serves as a first step towards our new unmarshaler design.
  • unmarshal.go outlines our unmarshaler type and associated unmarshaling methods that currently delegate to legacy unmarshaler of the SDK.

@skotambkar skotambkar self-assigned this Jan 7, 2020
@skotambkar skotambkar added the pr/needs-review This PR needs a review from a Member. label Jan 7, 2020
@skotambkar skotambkar marked this pull request as ready for review January 7, 2020 17:11
@skotambkar skotambkar requested a review from jasdel January 7, 2020 17:11
service/s3/unmarshal.go Outdated Show resolved Hide resolved
service/s3/unmarshal.go Outdated Show resolved Hide resolved
service/s3/unmarshal.go Outdated Show resolved Hide resolved
service/s3/proto_api_op_GetObject_test.go Outdated Show resolved Hide resolved
service/s3/unmarshal.go Outdated Show resolved Hide resolved
@skotambkar skotambkar force-pushed the proto/unmarshalS3 branch 2 times, most recently from 98e6f7a to 68ec8bf Compare January 27, 2020 22:55
Copy link
Contributor

@jasdel jasdel left a comment

Choose a reason for hiding this comment

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

Will want to check the rebasing of this PR, has a few extra changes from the feature branch that probably are not intended. (json and rest encoders)

service/s3/unmarshal.go Outdated Show resolved Hide resolved
service/s3/unmarshal.go Outdated Show resolved Hide resolved
@skotambkar skotambkar force-pushed the proto/unmarshalS3 branch 2 times, most recently from 6c8641e to 590df65 Compare March 2, 2020 16:51
@jasdel jasdel force-pushed the feature/GeneratedMarshaling branch from 875184f to 2550cb3 Compare March 2, 2020 18:24
service/s3/unmarshal.go Outdated Show resolved Hide resolved
service/s3/unmarshal.go Outdated Show resolved Hide resolved
service/s3/unmarshal.go Outdated Show resolved Hide resolved
Comment on lines 74 to 81
func unmarshalErrorPrototype(r *aws.Request, d *xml.Decoder, startToken xml.Token) error {
// protoXMLErrorResponse is error response struct for xml errors.
var respErr = protoXMLErrorResponse{}

// Delegate to reflection based decoding utils
if start, ok := startToken.(xml.StartElement); ok {
err := d.DecodeElement(&respErr, &start)
if err != nil && err != io.EOF {
Copy link
Contributor

Choose a reason for hiding this comment

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

would be great for this to take a StartElement type instead of Token. The caller should be responsible for getting the first StartElement. This is helpful because the first Token might be something like the XML preamble.

service/s3/unmarshal.go Outdated Show resolved Hide resolved
service/s3/unmarshal.go Show resolved Hide resolved
service/s3/unmarshal.go Outdated Show resolved Hide resolved
service/s3/unmarshal.go Outdated Show resolved Hide resolved
@skotambkar skotambkar requested a review from jasdel March 15, 2020 21:35
@skotambkar skotambkar changed the title service: prototype GetObject op to use unmarshaler type, that delegates to legacy unmarshaler S3: REST-XML prototype unmarshaler deligates to Legacy Unmarshaler (GetObject operation) Mar 22, 2020
@skotambkar skotambkar merged commit 89fdd8c into aws:feature/GeneratedMarshaling May 14, 2020
@skotambkar skotambkar deleted the proto/unmarshalS3 branch May 14, 2020 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/needs-review This PR needs a review from a Member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants