Requirements for stabilisation?

[shadowcat-mst]

So, a number of components in Paws have something like this -

https://metacpan.org/source/JLMARTIN/Paws-0.36/lib/Paws/S3.pm#L2

Is there a list anywhere of what's required to get to the point where that warning can go away? If not, how would we go about getting to a point where there was such a list?

[pplu]

There are three services marked with warnings: S3, Route53 and Glacier. They basically pertain to the RestXML set of services. These services are a bit more complex than the others, each one having their special corner cases, and maybe the code even needs to be specialized for each service to get everything working correctly.

Having a comprehensive set of tests to verify if anything breaks would be a must (note: there are already some tests for stuff that was getting reported) I've revisited open issues that would need looking at before considering each one of these stable.

• S3

  • Handle S3 301 Moved responses #106 (Nice to have)
  • S3 PutObject URI encodes key? #111
  • URI's are being double encoded, first by _call_uri, then again by URI::Template #221
  • S3::ListObjectVersions does not support a prefix #171
  • S3 DeleteObjects Method missing 'Content-MD5' Header #149
  • CreateMultipartUpload generates invalid request #92

• Glacier

• Full Glacier support #30

• Route53

• Paws::Route53::ChangeResourceRecordSets problem... #204

• Paws::Route53::ListResourceRecordSets -- HostedZoneId not being passed through to request #128

[castaway]

Hi @pplu I've visited each one of the items listed above (and in some cases left comments), I also made a plan for a set of tests to use to verify fixes for each one, which we'll be working on implementing next. I see the tests generally use mocking for any checks against data sent to the APIs, we will use more of that.

Have you looked at / considered any end to end testing? I discovered https://github.com/localstack/localstack and wondered if we could use it, in a special subset of tests that only run if localstack is installed (indicated by an environment variable or similar) - any thoughts? Alternately we could run the existing set of tests against it with some tweaks based on env vars, and then maybe against AWS itself too, if the tester wants to.

[pplu]

I also made a plan for a set of tests to use to verify fixes for each one, which we'll be working on implementing next. I see the tests generally use mocking for any checks against data sent to the APIs, we will use more of that.

Great! If you want some help around the test suite or have doubts about how to test a specific case, don't doubt in contacting me.

Have you looked at / considered any end to end testing? I discovered https://github.com/localstack/localstack and wondered if we could use it, in a special subset of tests that only run if localstack is installed (indicated by an environment variable or similar) - any thoughts? Alternately we could run the existing set of tests against it with some tweaks based on env vars, and then maybe against AWS itself too, if the tester wants to.

I'd love end to end testing to happen. Most of the test suite is derived from real calls and real responses, so you could say that the tooling is in place. There is a pluggable caller called Paws::Net::MockCaller that accepts two modes: RECORD, where it actually calls the services, but records the input parameters and the response from the backend service. When in REPLAY mode it only accepts the same calling sequence, with the same parameters for each call, and instead of doing the HTTP call, it reads the recorded HTTP responses (so all the response handling logic is invoked). If you want to develop test suites against localstack I'd be happy to have them.

[castaway]

Having just gotten a bit confused with it - it seems if you "record" with credentials set to Test::CustomCredentials, it doesn't send creds that work, at least not for me, I get "The AWS Access Key Id you provided does not exist in our records.".. it records fine if I comment out that setting.

Is it supposed to work with them set? Do we need the CustomCredentials, since it seems to be the REPLAY/RECORD setting that tells it whether we are calling the live backend or not.

[pplu]

The MockCaller is supposed to be used with valid AWS credentials when in record mode (just using the default credential provider, developing with creds in ~/.aws/credentials or in ENV vars. In Replay mode the credential provider doesn't matter (the TestCredentials are useful in test so that Paws need anything special in the environment)

[pplu]

@castaway @piratefinn @shadowcat-mst : Tell me if you feel some of the branches can be merged for an upcoming release (as I want to merge them to release/0.39 as soon as possible). I'm kind of lost inside all the branches, so I ask you to please mention me what branches you think can be merged when you have them "ready" (I'm not actively trying to know if your branches are ready for merge or not).

[piratefinn]

@pplu After some discussion with @castaway, we decided #265 and #261 are good to pull. I'm closing 266 for now until we have tinkered with other elements more.

[pplu]

Hi,
#261 is passing all tests. I'll merge it to release/0.39
#265 is failing tests https://travis-ci.org/pplu/aws-sdk-perl/builds/404615248 so I won't merge it yet. I understand that #265 is a bunch of tests, with no functionality changes.

[castaway]

Darn, they were supposed to be all TODO unless passing. We will recheck

[castaway]

Hi @pplu , the following PRs have been tested against tests/stabilisation and can be merged:

#278 #276 #275 #261

#265 should be merged first, that one is currently failing due to the botocore update, the fixes are in #278

Jess

[pplu]

Hi!

@castaway I've merged all the branches you mentioned onto the tests/stabilization branch. After regenerating all the service classes, I got a failure in t/s3/xml_creation.t that was basically because of the XML being generated in an undeterministic manner due to get_all_attributes not returning attributes in the same order (https://travis-ci.org/pplu/aws-sdk-perl/builds/410815936 shows the behavior). I modified the caller (0cae73c) but now the route53 tests are broken (https://travis-ci.org/pplu/aws-sdk-perl/builds/410831075).

What way do you recommend fixing the route53 XML tests? They seem to be using XML::Compare, but I don't know if that is helping or not (it doesn't help you find why the test isn't passing).

[pplu]

BTW: 💃 🎉 🎈 @shadowcat-mst @castaway @piratefinn

[castaway]

Hi.. huh, they passed for me last I tried - the use of XML::Compare was an attempt to have it not care which order internal XML elements were in, though I'm not sure if it worked.. I would say for now, if you've managed to have them output in a known order (alphabetical?) then update the tests to match, assuming the XML is generally correct..

[pplu]

I've just pushed a commit to tests/stabilization that fixes the route53 tests, so we're all green 😄

@castaway, @piratefinn Can you please add items to the Changes file for what you consider fixed?

[castaway]

Yes, now done + pushed to our tests/stabilisation branch (after pulling your changes)

[castaway]

hi @pplu, where did we get to on merging tests/stabilisation (#265) ? I've lost track..

[pplu]

I lost track too :(

[castaway]

I will rerun it against current release branch..

[byterock]

I am still playing with th S3 side of things. Working my way though 'BucketAnalyticsConfigurations' PUT/LIST/GET/Delete etc

After I am finished those I will see if I can get the S3 you memtioned above done.

Would love to get rid of that warning ;)

[castaway]

We're working on them ourselves, I was just trying to see where we'd gotten to (still retesting the branch, as rebase left a failing test)

[byterock]

I am about 80% complete on the code side of things on S3 API side of things you can check out my changes on this branch/pull
of paws
https://github.com/byterock/aws-sdk-perl/tree/s3ObjectTagging
and
boto

https://github.com/byterock/botocore/tree/s3-changes

so far I have gotten
Put/Get/List/Delete of
BucketAnalyticsConfiguration
BucketCors
BucketEncryption
BucketInventoryConfiguration

all working 100%, in scripts

no tests yet in the /t yet as most of the problems have been with the 'Request' side of things

Which of the S3s are you working on so we do not bash each other code

[castaway]

I've lost track as we started last year and then trailed off a bit - would suggest you check items against tests/stabilisation (#265) - which also contains fixes as all the bits were merged together.. before starting on them.

Hopefully will have that rebased on 0.42 soon, and we can merge it and go from there

[castaway]

Right, thats #265 rebased and tests all passing.. now to try and figure out which of the above list it covers.

[castaway]

I am about 80% complete on the code side of things on S3 API side of things you can check out my changes on this branch/pull
of paws
https://github.com/byterock/aws-sdk-perl/tree/s3ObjectTagging
and
boto

https://github.com/byterock/botocore/tree/s3-changes

so far I have gotten
Put/Get/List/Delete of
BucketAnalyticsConfiguration
BucketCors
BucketEncryption
BucketInventoryConfiguration

all working 100%, in scripts

no tests yet in the /t yet as most of the problems have been with the 'Request' side of things

Which of the S3s are you working on so we do not bash each other code

Hey soo two things:

1. Are those boto patches adding Content-MD5 etc to the botocore service-1.json etc? Have you seen this issue in which they didnt want such a patch? Bug: DeleteObjects in S3 module boto/botocore#1302

2. It looks like so far we've covered/fixed all the items at the top of this issue apart from S3 DeleteObjects Method missing 'Content-MD5' Header #149 and CreateMultipartUpload generates invalid request #92 (both of which are similar issues to what you seem to be looking at)

[byterock]

Ok I havea few more in the pipe one which is very interting and I thing will get rid of the flatten problems

will checkout out your branch and see how it matches up with my test set

[castaway]

Ok I havea few more in the pipe one which is very interting and I thing will get rid of the flatten problems

will checkout out your branch and see how it matches up with my test set

I have a feeling we may have fixed those in the branch.. will see!

Btw which issue # is the one about /?version (query params with no values) etc not working? I couldnt find it

[byterock]

I was just going systematically though all the 'actions' of S3 and fixing them as I went along. I added them to the bug list as I fixed them.

[castaway]

hmm annoying, Im sure there was one

[byterock]

What woudl be the most up to date branch??

[byterock]

Ok I fighre that out

tests/stabilisation

So far These bugs have not been Fixed on that branch

PutBucketAnalyticsConfiguration
#350

The issue is the boto has
"requestUri":"/{Bucket}?analytics"

and when run though request we get

/dev.cargotel.paws?id=test1333'

should be

/dev.cargotel.paws?analytics=&id=test1333'

PutBucketCors
#351

Flattening issue

<CORSConfiguration> <CORSRule> <AllowedOrigin>http://www.example.com</AllowedOrigin> ...

Paws was giving
<CORSConfiguration> <CORSRules> <CORSRule> <AllowedOrigins> <AllowedOrigin>http://www.example.com</AllowedOrigin> ...

boto change on this one

PutBucketLifecycleConfiguration
#353

PutBucktMetricsConfiguration
#354

boto change
Missing "ContentMD5" as required by AWS

PutBucketPolicy
#355

this isseu here is the API requires JSON not XML

These have been fixed

GetBucketPolicyOutput
#355

[castaway]

bits of s3 use JSON not XML !? impressive.. The /?analytics one is the URI bug I was looking for (I'm sure someone reported an issue in URI.pm for that) ..

Looks like you found a bunch that hadn't been previously reported (we were working on the list at the top of this issue) .. mebbe we should make a new list?

[byterock]

One response is in JSON and I think one put as well.

I have fixed 'PutBucketAcl '
the #356

this gets the
traits => ['NameInRequest','NameInRequest'],
bug (nobody noticed that one. I reuse to fix a flatten/tagging issue and that call is the only one to use XML traits ARG

anway don't have many more to reivew/fix at least for S3.

[castaway]

Hi @pplu, could we get #265 looked at / merged please? Then ideally a review of the changes @byterock has been making would be useful, to know if thats on the right track.

[byterock]

I found a fix for that ' The /?analytics Bug'

you can see i on this chommit
byterock@08d44a3

Nothing special I just treat all query params as URI and change the template that is comming in.

No more changes to boto yeah

[castaway]

I found a fix for that ' The /?analytics Bug'

you can see i on this chommit
byterock@08d44a3

Nothing special I just treat all query params as URI and change the template that is comming in.

No more changes to boto yeah

You're manually building the query string? Surely this would be better fixed in URI itself, rather than adding extra level code to Paws?

[byterock]

Well the code change is simple enough I treat both ParamInURI and ParamInQuery the same except if when it is a ParamInQuery I add the item to the template.

if ($attribute->does('Paws::API::Attribute::Trait::ParamInQuery')) {
my $att_name = $attribute->name;
$uri_template .= $joiner.$attribute->query_name."={".$att_name."}";
$vars->{ $att_name } = $call->$att_name

and then let template sort it out.

At the AWs server end doesn't really care if we call a param a URI or Query param as long as in comes along in the end as all of these work

/some.bucket/?analytics&id=10001
/some.bucket/?analytics=&id=10001
/some.bucket/?id=10001&analytics

as a matter of fact I am now getting into a problem with my new test case where the URI and URL are failing at times as the order of the params keeps changes once it goes into a request object

[castaway]

Hi & happy new year, @pplu , would it be possible to get an update on when you'll be able to look at merging the various PRs? Unfortunately we now have some conflicting ones (SC has fixed some things, and @byterock has fixed some in different ways), plus the Moo conversion will need to be updated after the merge (or done first and everything else updated)

[byterock]

I went through all the bugs on the I could find on S3 and the good old

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

seems to be working for 99% everything.

I am going to look at the tests on this brach and move them into mine to see if any fail and then check my real world tests to see if the newer tests are ok

[byterock]

Ok here is the first one (all tests run against the https://github.com/byterock/aws-sdk-perl/tree/s3ObjectTagging branch)

t/s3/content_headers.t

This test checks for the existance of the 'Content-Length' header

I have done some real world testsing and the 'Contnet-Lenght' header does not need to be included

Below is the request and response from a real test.
bless( { 'method' => 'PUT', 'date' => '20200204T221945Z', 'headers' => bless( { 'content-md5' => 'jeZMJlnIjr5T4V2k0kf3sA==', 'authorization' => 'AWS4-HMAC-SHA256 Credential=AKIAJ25UFIFHZ4WML4BQ/20200204/us-east-1/s3/aws4_request,SignedHeaders=content-md5;content-type;date;host;x-amz-content-sha256;x-amz-date,Signature=c8817743aa6fa579894cbeaa996e190eb2ab8869da4007bf9be22e379fd844db', 'x-amz-content-sha256' => '0ddf26e60424f76a462d28de46e32715db6c6a7efe651eb51e441b9b28f81994', 'content-type' => 'application/xml', 'host' => 's3.amazonaws.com', '::std_case' => { 'x-amz-date' => 'X-Amz-Date', 'x-amz-content-sha256' => 'X-Amz-Content-Sha256' }, 'x-amz-date' => '20200204T221945Z', 'date' => '20200204T221945Z' }, 'HTTP::Headers' ), 'content' => '<AccessControlPolicy xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><AccessControlList><Grant><Grantee xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:type="CanonicalUser"><DisplayName>OwnerDisplayName</DisplayName><ID>61fc60cb3564d2688646074ceaa202e56ee65cf6e56c28b3a034eabd193d54c8</ID></Grantee><Permission>FULL_CONTROL</Permission></Grant></AccessControlList><Owner><DisplayName>Nothing</DisplayName><ID>61fc60cb3564d2688646074ceaa202e56ee65cf6e56c28b3a034eabd193d54c8</ID></Owner></AccessControlPolicy>', 'parameters' => { 'AccessControlPolicy.Grants.Grant.1.Grantee.DisplayName' => 'OwnerDisplayName', 'AccessControlPolicy.Owner.ID' => '61fc60cb3564d2688646074ceaa202e56ee65cf6e56c28b3a034eabd193d54c8', 'AccessControlPolicy.Grants.Grant.1.Permission' => 'FULL_CONTROL', 'AccessControlPolicy.Owner.DisplayName' => 'Nothing', 'AccessControlPolicy.Grants.Grant.1.Grantee.ID' => '61fc60cb3564d2688646074ceaa202e56ee65cf6e56c28b3a034eabd193d54c8', 'AccessControlPolicy.Grants.Grant.1.Grantee.Type' => 'CanonicalUser', 'Bucket' => 'oneoffpaws' }, 'uri' => '/oneoffpaws?acl', 'url' => 'https://s3.amazonaws.com/oneoffpaws?acl' }, 'Paws::Net::S3APIRequest' )

I think we can drop this test. Unless I am missing something?

[byterock]

The following tests all pass;

s3/uri_avoid_chars.t
s3/uri_encoding.t
s3/multipartupload.t

s3/xml_creation.t

is a little flawed
The XML is not correct
AWS requires the ' xmlns="http://s3.amazonaws.com/doc/2006-03-01/"' in the root tag of both calls

as found in boto
"SelectObjectContent":{
...
"input":{
"shape":"SelectObjectContentRequest",
"locationName":"SelectObjectContentRequest",
"xmlNamespace":{"uri":"http://s3.amazonaws.com/doc/2006-03-01/"}
},

as well the XML sorting might be differnt so test can fail randomly

I can correct for the 'xmlNamespace' but not the sort

I only have

t/s3/uri_avoid_chars.t

to look at but I am not sure what that is testing??

[castaway]

Hmm, re the Content-Length headers, this was from #92, and then the AWS docs which say "Length of the message (without the headers) according to RFC 2616. This header is required for PUTs and operations that load XML, such as logging and ACLs." - see: https://docs.aws.amazon.com/AmazonS3/latest/API/RESTCommonRequestHeaders.html

Not really much harm in sending them.. mebbe one day the docs will catch up with reality..

[castaway]

Re xml_creation.t - aha thanks, have you got changes that sort out the xmlNamespace bits? Can we easily extract them? (and matching tests?)

Re: t/s3/uri_avoid_chars.t this is a test to ensure that Paws does not allow users to attempt to create objects using characters which AWS does not allow.

[byterock]

I would just drop the xml_creation.t test as I created a new generic test suite for that

09_requests.t

That should cover all the XML creation and its little odd bits

All the changes are in the

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

branch

As for 't/s3/uri_avoid_chars.t '

I am not sure if this is a correct test for this level of API. Would be useful for a CLI or alike but nowhere in PAWS is there pre-call data sanitization.

Anyway AWS will fail on creation when you have such a bad character. Better error handling might be what we want here.

I think the only part of Rest XML that is not working or tested in 'S3::SelectObjectContent' with that

"event":true stream.

Will see if I can play with that later today

[byterock]

I attmpeted to run teh 09 and 10 test suites on a few other branches today with rater poor resutls (almost all fails)

What branch apart form mine should we test it agianst?

I found this one

https://github.com/shadow-dot-cat/aws-sdk-perl.git

was rather out of date for the Rest XML bits ??

cheers
John

[castaway]

Can we keep this discussion on #372 please?