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

Just a quick question on return 'status' for S3 #345

Open
byterock opened this issue Sep 25, 2019 · 5 comments
Open

Just a quick question on return 'status' for S3 #345

byterock opened this issue Sep 25, 2019 · 5 comments

Comments

@byterock
Copy link
Collaborator

byterock commented Sep 25, 2019

Looking up the AWS API for rest Post ResoreObject https://docs.aws.amazon.com/AmazonS3/latest/API/RESTObjectPOSTrestore.html

I noticed that there are two responses possable 200 and 202

If the object copy is not previously restored, then Amazon S3 returns 202 Accepted in the response.

If the object copy is previously restored, Amazon S3 returns 200 OK in the response.

I was compiling a test for the above an notices that 'status' was missing from the 'Paws::S3::RestoreObjectOutput' class. Would it be propper to add it in??

I have been playing with this and the only pratical was to add it in is to treat it the same as '_request_id'

giving that I added this line in the apprprate templates

has _request_id => (is => 'ro', isa => 'Str');
++has _status => (is => 'ro', isa => 'Str');

and then in a few places in the approtate net response calls

$unserialized_struct->{ _request_id } = $request_id;

  • $unserialized_struct->{ _status } = $http_status;

Does hit I think amost every generated class

Anyone else need this or I am just adding something not needed

@pplu
Copy link
Owner

pplu commented Sep 30, 2019

I'd try to avoid leaking HTTP details to the user by default. It's true that for this API call the HTTP Status is relevant to the caller, so exposing it seems legit.

I'd try to limit the scope of exposing the HTTP return status by adding an attribute role like https://github.com/pplu/aws-sdk-perl/blob/master/lib/Paws/API.pm#L1 that signal the response to object routines to copy the HTTP Status over to an attribute.

@byterock
Copy link
Collaborator Author

byterock commented Oct 7, 2019

Ok I added in th status as you suggests.

I found it under botocore as

"location":"statusCode",

It is used in a number of othere classes

I added it in as a new trait as you suggested, not 100% sure on the name though

the tag is

#343

please have a look and see if I am on the right track.

@pplu
Copy link
Owner

pplu commented Oct 9, 2019

Ok I added in th status as you suggests.
I found it under botocore as
"location":"statusCode",
It is used in a number of othere classes

OK. This means that we can make the builders put the trait on the appropiate attributes, which I see you already found 👍

I added it in as a new trait as you suggested, not 100% sure on the name though

I think the name would better be ParamInStatus (just as a reflex of the other traits).

please have a look and see if I am on the right track.

I think you're on the right track 💃

@byterock
Copy link
Collaborator Author

byterock commented Oct 27, 2019

Ok this is all fixed up on this branch

https://github.com/byterock/aws-sdk-perl/tree/s3ObjectTagging

@byterock
Copy link
Collaborator Author

I have taken this out for now. Can we change this from a 'bug' to a 'Feature' request.

byterock referenced this issue in byterock/aws-sdk-perl Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants