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

Paws::Pinpoint does not appear to work #212

Closed
lukec opened this issue Nov 5, 2017 · 56 comments

Comments

Projects
None yet
4 participants
@lukec
Copy link

commented Nov 5, 2017

Hello,

I've been trying to use the new Pinpoint module to send a text message, but I can't get even basic usage of the module working.

For instance using the aws command line tool, I can run aws pinpoint get-apps and see apps. But Paws::Pinpoint's GetApps() fails using the same credentials.

I tried creating a simple examples/pinpoint.pl but it fails immediately:

#!/usr/bin/env perl

use strict;
use warnings;

use Data::Printer;
use Paws;

my $pin = Paws->service('Pinpoint', region => 'us-east-1', debug => 1);

my $app = $pin->GetApps();
p $app;

I'm new to the Paws code so not sure where to start with looking for the fix.

@pplu

This comment has been minimized.

Copy link
Owner

commented Nov 7, 2017

Hi,

Thanks for the report. I'll have to take a look at whats happening. If you want to help diagnosing: I recommend that you Dump the request object in Paws::Net::Caller, to see if it matches what you would expect being transmitted over the wire

@lukec

This comment has been minimized.

Copy link
Author

commented Nov 7, 2017

Thanks, I'll try that. I saw the debug flag in there but I couldn't figure out how to set it from the example script or what the flag did.

@lukec

This comment has been minimized.

Copy link
Author

commented Nov 7, 2017

I would think a 403 response would be due to credentials, but I am using the same creds (as far as I can tell) for both Paws and the aws CLI. The debug on both shows they are both hitting GET https://pinpoint.us-east-1.amazonaws.com/v1/apps with no body

@lukec

This comment has been minimized.

Copy link
Author

commented Nov 7, 2017

@pplu Perhaps the difference is that in the AWS CLI, the body is `` (empty string), while in Paws, the body is {}

@pplu

This comment has been minimized.

Copy link
Owner

commented Nov 7, 2017

It is possible. Other APIs don't seem to matter about that, but maybe pinpoint is specially picky about it. Try to set the body content to empty string in the caller. If it's that, we'll find a less ad-hoc solution 😄

@lukec

This comment has been minimized.

Copy link
Author

commented Nov 7, 2017

Ok, that got us further.

The error we see now is: Credential should be scoped to correct service: 'mobiletargeting'.

I haven't encountered this so i'll search it up - but open to any ideas.

@lukec

This comment has been minimized.

Copy link
Author

commented Nov 7, 2017

OK, I think I found the crux of the problem.

In the aws CLI tool, the credential is for mobiletargeting. Here's part of what gets signed: 20171107/us-east-1/mobiletargeting/aws4_request

But in Paws, we use: 20171107/us-east-1/pinpoint/aws4_request

Have you seen this before in other modules - where the credentials differs from the service name?

@pplu

This comment has been minimized.

Copy link
Owner

commented Nov 7, 2017

Edit the Pinpoint.pm file. There’s an attribute with value ‘pinpoint’. Change it to mobiletargeting.

That should get you working :)

@lukec

This comment has been minimized.

Copy link
Author

commented Nov 7, 2017

It's not so simple. That will change the service host - but it looks like the service should remain pinpoint.

I see in the golang api client, the use the CredentialScope option to override the service name: https://github.com/aws/aws-sdk-go/blob/262fa7531bccfe21fc41b25bde81dad7e8ea3320/models/endpoints/endpoints.json#L1128

I see in Paws that there is a similar mechanism, but it only looks to be used for overriding the region, not the service name.

OK, so given that, I tried overriding forcing the service name from pinpoint to mobiletargeting, but when I do that I get a signature error. 🤔

@lukec

This comment has been minimized.

Copy link
Author

commented Nov 7, 2017

@lukec

This comment has been minimized.

Copy link
Author

commented Nov 7, 2017

Well I'm kind of stumped. I tried overriding the service name for signing - and it comes out looking correct but the API request is rejected due to a signing problem. I can't figure out how to make it work. Any ideas?

@pplu

This comment has been minimized.

Copy link
Owner

commented Nov 7, 2017

Change the region_rules of the service so it doesn’t use {service} and Instead uses pinpoint. The signature mismatch must be because the body’s content is signed too, so sending an empty body when the signature was for “{}” makes it mismatch.

@lukec

This comment has been minimized.

Copy link
Author

commented Nov 7, 2017

Awesome, progress! Thank you so much @pplu.

OK, you were right - I was setting content to '' after the signature. For a hack I set it to '' in Paws::Net::V4Signature, just before i overrode the service to mobiletargeting. This worked! (sort of).

We did get a success response, but the Paws moose code had trouble with the response:

Attribute (ApplicationsResponse) does not pass the type constraint because: Validation failed for 'Paws::Pinpoint::ApplicationsResponse' with value {"Item":[{"Name":"recollecttest_MobileHub","Id":"150b9f9af7ff45fdb82e581bfb18f0cf"},{"Name":"Halifax","Id":"5192d948f33e470facd11f04abc60702"}]} (not isa Paws::Pinpoint::ApplicationsResponse)```

So to summarize the issues I've seen:

1) Pinpoint must use a separate service name during credentials signing.  We will need to build this new mechanism so that the pinpoint module can declare it uses a different signing name.

2) The `pinpoint.GetApps()` API call must set the body to be an empty string, not `{}`.

3) The Response from `GetApps()` doesn't properly serialize into our Moose objects.

Any thoughts on these?  
@lukec

This comment has been minimized.

Copy link
Author

commented Nov 11, 2017

@pplu hello, can you help direct me on the fixes describe above ^?

I have a small patch for #1, not sure what you think:

diff --git a/auto-lib/Paws/Pinpoint.pm b/auto-lib/Paws/Pinpoint.pm
index 50b93ec7b..d2fb0dbc9 100644
--- a/auto-lib/Paws/Pinpoint.pm
+++ b/auto-lib/Paws/Pinpoint.pm
@@ -1,6 +1,7 @@
 package Paws::Pinpoint;
   use Moose;
   sub service { 'pinpoint' }
+  sub signing_service { 'mobiletargeting' }
   sub version { '2016-12-01' }
   sub flattened_arrays { 0 }
   has max_attempts => (is => 'ro', isa => 'Int', default => 5);
diff --git a/lib/Paws/Net/V4Signature.pm b/lib/Paws/Net/V4Signature.pm
index 9b896243d..b949eab69 100644
--- a/lib/Paws/Net/V4Signature.pm
+++ b/lib/Paws/Net/V4Signature.pm
@@ -30,7 +30,11 @@ package Paws::Net::V4Signature;
       $request->header( 'X-Amz-Security-Token' => $self->session_token );
     }
 
-    my $sig = Net::Amazon::Signature::V4->new( $self->access_key, $self->secret_key, $self->_region_for_signature, $self->service );
+    my $s = $self->service;
+    if ($self->can('signing_service')) {
+        $s = $self->signing_service;
+    }
+    my $sig = Net::Amazon::Signature::V4->new( $self->access_key, $self->secret_key, $self->_region_for_signature, $s);
     $sig->sign( $request );
   }
 1;

For #2 - I'm not sure how the Paws::Pinpoint::GetApps class should specify the body should be empty string. Perhaps using _stream_param? (very unsure).

And then for #3 above, it looks to me like the Paws::Pinpoint::GetAppsResponse or Paws::Pinpoint::ApplicationsResponse is not getting converted from a JSON string into a hash...

Attribute (ApplicationsResponse) does not pass the type constraint because: Validation failed for 'Paws::Pinpoint::ApplicationsResponse' with value {"Item":[]} (not isa Paws::Pinpoint::Appl
icationsResponse) 

Can you advise on how i should approach #2 and #3 above?

@lukec

This comment has been minimized.

Copy link
Author

commented Nov 11, 2017

@pplu If you know of any devs that know this codebase well and have cycles to fix pinpoint, my company would be happy to sponsor the work to flesh this out.

@autarch

This comment has been minimized.

Copy link
Contributor

commented May 23, 2018

It looks the pricing service has a similar problem.

The service sub returns api.pricing but when I look at the aws client it signs it using the name pricing.

@lukec

This comment has been minimized.

Copy link
Author

commented May 23, 2018

Hi Dave! 👋 Nice to bump into you!

@pplu

This comment has been minimized.

Copy link
Owner

commented Jun 13, 2018

JFYI: the signing issue has been solved with PR #243, that will go into release 0.37 of Paws

@byterock

This comment has been minimized.

Copy link

commented Oct 9, 2018

Hate to reopen an old issue but I am not getting pinpoint to work with ASW

I have paws .38 installed But all I aget is the same 403 The request could not be satisfied error on all commands.

I use the same code as above and like before I can run all the commands with 'aws' command line tool, and the same credential and get the same results;

Any more thoughts

@byterock

This comment has been minimized.

Copy link

commented Oct 9, 2018

Oh by the way pricing does work now where is did not with 0.36

@byterock

This comment has been minimized.

Copy link

commented Oct 10, 2018

one more follow on.

I got this to work in python with this header

'Authorization': 'AWS4-HMAC-SHA256 
             Credential=AKIAILZQTWYWQB5XQDVA/20181010/us-east-1/mobiletargeting/aws4_request, 
      SignedHeaders=host;x-amz-date, 
              Signature=30e22c164f1d2e28821a2a91075cac5c61e3e0211e14c4c52da2670d0a9eea6e'

while in perl the above is
'Authorization' => 'AWS4-HMAC-SHA256
Credential=AKIAILZQTWYWQB5XQDVA/20181010/us-east-1/mobiletargeting/aws4_request,
SignedHeaders=date;host;x-amz-content-sha256;x-amz-date,
Signature=980f99f555a1f5b33f3c5390eb1f524278f52a194c0de7f1572e9b92915fa1cd',

So the SignedHeaders are a little diffrent??

@byterock

This comment has been minimized.

Copy link

commented Oct 12, 2018

Ok an update on this

I did get it to resond and like luke it was the content

that was set to
'content' => '{}',

which sould be
'content' => '',

when the content is empty;

Now to get a fix for Paws in there

w{}

@byterock

This comment has been minimized.

Copy link

commented Oct 12, 2018

Ok now working on a patch for this

what I am getting to is this line in Paws::Net::RestJsonRespons->new_from_response

return $class->new(%args);

in this case %args is

{
'ApplicationSettingsResource' => '{"ApplicationId":"69f039f36c8f46ae9dff52a5243ac232",
"QuietTime":{},
"Limits":{},
"CampaignHook":{},
"CloudWatchMetricsEnabled":false}'

}

which points to a string I have converted this to a Hash-ref

{
'ApplicationSettingsResource' => {
'ApplicationId' => 'xxxxxxxxxxxxxxxxxxxxxx',
'CloudWatchMetricsEnabled' => bless( do{(my $o = 0)}, 'JSON::PP::Boolean' ),
'QuietTime' => {},
'CampaignHook' => {},
'Limits' => {}
}
};

which still gives me the 'same' as none of the sub classes are being Coerced. I did try this

ApplicationSettingsResource' => Paws::Pinpoint::ApplicationSettingsResource->new({"ApplicationId"=>"xxxxx",
"QuietTime"=>Paws::Pinpoint::QuietTime->new({}),
"Limits"=>Paws::Pinpoint::CampaignLimits->new({}),
"CampaignHook"=>Paws::Pinpoint::CampaignHook->new({}),
"CloudWatchMetricsEnabled"=>'false'})

which does work but of coures quite useless.

Any hint on how to do the coerce??

@pplu

This comment has been minimized.

Copy link
Owner

commented Oct 15, 2018

@byterock : I'm having a hard time understanding where you're stuck. Maybe you can share your changes up to date so I can better understand where you're going. Also, a good way to get a test case is to create a test case in t/10_responses. It basically consists of a xxxxx.response.test.yml with a description of the service and the method call, whith some additional test of what values to expect from the return (see t/10_responses/sts-get-session-token.response.test.yml for inspiration). Together with the respose.test.yml file, you create a file with the same name, but without the .test.yml part with the response from the service. See the matching t/10_responses/sts-get-session-token.response for reference.

You can execute a single test with something like carton exec perl -I lib/ -I auto-lib/ t/10_responses.t t/10_responses/sts-get-session-token.response

Hope it helps

@byterock

This comment has been minimized.

Copy link

commented Oct 15, 2018

Ok I will write up a test today or tomorrow

The patch I have is a little off It works for some of the pinpoint commands but not others

@byterock

This comment has been minimized.

Copy link

commented Oct 15, 2018

Ok I am trying with this test Still trying to get the suite to work though but something to play with

pinpoint-getapps.response.test.yml

---
call: getapps
service: PINPOINT
tests:
  - expected: []
    op: eq
    path: ApplicationsResponse

and

pinpoint-getapps.response

---
content: '{"ApplicationsResponse":[]}'
headers:
  content-length: 26
  content-type: application/json
  date: 'Thu, 28 Jan 2016 22:13:38 GMT'
  x-amzn-requestid: 61aec99f-c60c-11e5-8ae7-e73fabb85e7a
status: 200

Hope this helps

@byterock

This comment has been minimized.

Copy link

commented Oct 15, 2018

I fixed the above test a little as Pinpoint and GetApps where in the wrong case and ran it against the SDK and got

not ok 1 - Call Pinpoint->GetApps from t/10_responses/pinpoint-getapps.response
#   Failed test 'Call Pinpoint->GetApps from t/10_responses/pinpoint-getapps.response'
#   at t/10_responses.t line 108.
# died: Moose::Exception::ValidationFailedForTypeConstraint (Attribute (ApplicationsResponse) does not pass the type constraint because: Validation failed for 'Paws::Pinpoint::ApplicationsResponse' with value {"ApplicationsResponse":[]} (not isa Paws::Pinpoint::ApplicationsResponse) at /usr/local/lib/perl/5.18.2/Moose/Object.pm line 24
#       Moose::Object::new at lib/Paws/Net/RestJsonResponse.pm line 243
#       Paws::Net::RestJsonResponse::new_from_response at lib/Paws/Net/RestJsonResponse.pm line 211
#       Paws::Net::RestJsonResponse::response_to_object at lib/Paws/Net/RestJsonResponse.pm line 23
#       Paws::Net::RestJsonResponse::process at t/lib/FileCaller.pm line 50
#       FileCaller::do_call at /usr/local/share/perl/5.18.2/Paws/Pinpoint.pm line 150
#       Paws::Pinpoint::GetApps at t/10_responses.t line 107
#       Test::Exception::lives_ok at t/10_responses.t line 108
#       main::test_file at t/10_responses.t line 32
# )
not ok 2 - Can't test method access because something went horribly wrong in the call to GetApps
#   Failed test 'Can't test method access because something went horribly wrong in the call to GetApps'
#   at t/10_responses.t line 111.
1..2

Will see what happens then I apply my patch

@pplu

This comment has been minimized.

Copy link
Owner

commented Oct 16, 2018

Ok I will write up a test today or tomorrow

The patch I have is a little off It works for some of the pinpoint commands but not others

It's OK if the patch doesn't fix everything, it's important to see how you're solving the issues. Another important thing is to have the tests to ensure that it doesn't break once it's fixed (AWS APIs for different services have subtle differences and nuances, and we want to keep track of these fixes through tests).

If you set up a development environment: (https://github.com/pplu/aws-sdk-perl#development-setup),, we can share the files you produce via a branch. You create a fork of Paws in gitlab, git clone, git checkout release/0.39 (the latest code), and git checkout -b fix/issue_212 (to create a new branch). After that, you can git push --set-upstream origin fix/issue_212 to share your work. You can also create a Pull Request to my repo so we can chat about the changes inside the PR.

@pplu

This comment has been minimized.

Copy link
Owner

commented Oct 16, 2018

OK. Now I have a good grasp of the problem, and how to get it fixed (thanks for your test cases!)

I've opened a new branch (fix/issue_212, which we can work on together): #285

To get the problem fixed I've had to edit the pinpoint service-2.json botocore file: pplu/botocore@de6f4ea, creating the appropiate shapes for the things that GetApps returns. Later I found out that the shape I was declaring was already declared beforehand, so that simplified things: pplu/botocore@d600a6c. (Note that if you edit a service-2.json file, you can regenerate the Paws code with: carton exec builder-bin/gen_classes.pl --classes botocore/botocore/data/pinpoint/2016-12-01/service-2.json)

So on that branch, GetApps is now working correctly (passing a modified version of the tests you contributed)

To get the rest of the methods fixed:

  • It looks like all lot of them are declared with a "payload", which seems incorrect (as it tells Paws to try to put the raw content of the response into that attribute). So that would need fixing (grep -L "_stream_param" auto-lib/Paws/Pinpoint/* will give you an idea of who they are)
  • I would also suspect that a lot of them are incorrectly declaring the shape of the returned datastructure. (GetAppsResponse was not correctly defined, but ExportJobsResponse was correctly defined)

Can you help out to correct the service-2.json (and add some tests) for at least the methods you're interesed in?

@byterock

This comment has been minimized.

Copy link

commented Oct 17, 2018

Ok I now have a proper fix for issue

  1. GetApps must have empty string for content not default '{}'

Will need write access to the branch to Push it up to the branch.

In brief what I had to do was create a new class attribute '_empty_content' that I can use in RestJsonCaller.pm to set the content to ""
I changed
/lib/Paws/Net/RestJsonCaller.pm
/templates/restjson/callargs_class.tt
and
/botocore/data/pinpoint/2016-12-01/service-2.json

I can get the test to work but they to not cover the above problem

Going to work on the GetApp next as that had two errors

  1. RestJson
  2. content not encapsulated
  3. missing header 'Content-Type'
  4. missing header 'Content-Length'

cheers

@pplu

This comment has been minimized.

Copy link
Owner

commented Oct 18, 2018

Great to hear you are advancing! 😎

Will need write access to the branch to Push it up to the branch.

Would you mind forking the repo onto your account? once that is done, you can
git remote add byterock git@github.com:byterock/aws-sdk-perl.git

work on a branch, preferrably, and then

git push --set-upstream byterock your_branch

with this, you will have a branch on your forked copy. When you push, you can issue a merge request to the main repo.

@byterock

This comment has been minimized.

Copy link

commented Oct 18, 2018

Ok thanks for that
I got my patch to work.

I think the botocore file is mostly ok as playing with the python and java APIs for GetApps I get this coming back in both cases

{
"ApplicationsResponse": {
"Item": [
{
"Id": "69f039f36c8f46ae9dff52a5243ac232",
"Name": "cgtsms_MobileHub"
},
...}
}
}

though the content in the response the 'ApplicationsResponse' key is not present

'content' => '{"Item":[{"Name":"cgtsms_MobileHub","Id":"69f039f36c8f46ae9dff52a5243ac232"},

So the question is do we code to make it work like python and java or are we in the Perl world going to be disruptive again ;)

I will get the first patch in shortly

@byterock

This comment has been minimized.

Copy link

commented Oct 18, 2018

Ok have commited my chages to my account branch.

I am going to try another tack where I Paws version of 'GetApps' will just give

"Item": [
{
"Id": "69f039f36c8f46ae9dff52a5243ac232",
"Name": "cgtsms_MobileHub"
},
...}
}

by using your shapes suggestion

More a question if we should make it more Java/Python like or stick with what is comming directly from AWS

@byterock

This comment has been minimized.

Copy link

commented Oct 18, 2018

Ok got a patch where the output is for a GetApp is

bless( {
'Name' => 'cgtsms_MobileHub',
'_request_id' => '2d229a31-d310-11e8-8a4b-0161ee024917',
'Id' => '69f039f36c8f46ae9dff52a5243ac232'
}, 'Paws::Pinpoint::GetAppResponse' );

The above only needs changes to the 'Boto' file but there will be a great deal of them.

AND

The issue of sending {} as the content on the request is still present

So still need a change to the Template and boto files to get a flag to fix the {} issue as I discoved that some APIs need {} and die on ""

In the end It all depends on how we want the end product to look like

Patch 1 Object with pointer to object (Python,JAVA style)

'ApplicationSettingsResource' => bless( {
'Limits' => bless( {}, 'Paws::Pinpoint::CampaignLimits' ),
'QuietTime' => bless( {}, 'Paws::Pinpoint::QuietTime' ),
'CampaignHook' => bless( {}, 'Paws::Pinpoint::CampaignHook' ),
'ApplicationId' => '69f039f36c8f46ae9dff52a5243ac232'
}, 'Paws::Pinpoint::ApplicationSettingsResource' )
}, 'Paws::Pinpoint::GetApplicationSettingsResponse' );

Patch 2 return just the object (Perl)

bless( {
'_request_id' => '2cb08d7f-d312-11e8-81e7-f920a5e7df78',
'QuietTime' => bless( {}, 'Paws::Pinpoint::QuietTime' ),
'ApplicationId' => '69f039f36c8f46ae9dff52a5243ac232',
'Limits' => bless( {}, 'Paws::Pinpoint::CampaignLimits' ),
'CampaignHook' => bless( {}, 'Paws::Pinpoint::CampaignHook' )
}, 'Paws::Pinpoint::GetApplicationSettingsResponse' );

From what I have been playing with I think we should go with Patch 2. All of the other calls I have used do this.

If you could answer the above I should be able to get it out quickly

Cheers
John

@byterock

This comment has been minimized.

Copy link

commented Oct 19, 2018

Ok I think I cracked it with the least amout of changes

I am following this pattern

given the orginal json

"GetCampaigns" : {
  "name" : "GetCampaigns",
  "http" : {
    "method" : "GET",
    "requestUri" : "/v1/apps/{application-id}/campaigns",
    "responseCode" : 200,
  },
  "input" : {
    "shape" : "GetCampaignsRequest"
  },
  "output" : {
    "shape" : "GetCampaignsResponse",
    "documentation" : "200 response"
  },
  "errors" : [ {

...

I make this change

"GetCampaigns" : {
  "name" : "GetCampaigns",
  "http" : {
    "method" : "GET",
    "requestUri" : "/v1/apps/{application-id}/campaigns",
    "responseCode" : 200,
    "emptyContent" : 1
  },
  "input" : {
     "shape" :  "GetCampaignsRequest"
  },
  "output" : {
    "shape" : "CampaignsResponse",
    "documentation" : "200 response"
  },
  "errors" : [ {

...

and I get the this result

bless( {
'Item' => [],
'_request_id' => '754f27de-d3a0-11e8-b1b6-332b4440aca4'
}, 'Paws::Pinpoint::CampaignsResponse' );

The only Paws change I will need to do it is that 'emptyContent' in a flag in the JSON and Template to empty out the content in the 'RestJsonCaller.pm'

I think this will be the best solution.

any Thoughts

@byterock

This comment has been minimized.

Copy link

commented Oct 20, 2018

Ok all fixed up
in the end a few changes to RestJsonCaller.pm to account for

  1. Empty content cannot be '{}' on pinpoint
  2. check the '$call->_stream_param' attribute to see it it an object and if it is
  3. encode the object
  4. set the content
  5. set the content type header
  6. set the content lenght header

Also changes to boto to beig to list here

my repos are

https://github.com/byterock/botocore
and
https://github.com/byterock/aws-sdk-perl

@pplu

This comment has been minimized.

Copy link
Owner

commented Oct 23, 2018

Thanks for the hard work @byterock ! 💃 🕺 🎉

@byterock

This comment has been minimized.

Copy link

commented Oct 26, 2018

Ok Been playing with it for a few days this week real world testing the bits and pieces

Still have some more bugs to fix for example
GetEndpoint

will patch as I find and fix

@byterock

This comment has been minimized.

Copy link

commented Jun 10, 2019

Ok Finaly patchs have to added to my branch of

https://github.com/byterock/botocore

I tested with live AWS data and I have the API working fully. I have a good test script but it requires an AWS account and a properly configured AWS IAM profile. Even with that account about 20% of the API has to be tested in seperate files that requre a full working project to be present and aboutr 2 to 3 hours of set up of roles iams and other bits and bods to get them to test, Not really much use adding them here as they will be custome to every AWS user config.

@byterock

This comment has been minimized.

Copy link

commented Jun 21, 2019

Any idea when this will get into 41?

by the way the test suite I mentioned above costs about 3$ to run as one has to pay for all the services that are being tested.

@pplu

This comment has been minimized.

Copy link
Owner

commented Jul 2, 2019

I've just merged in your changes into the pinpoint definitions and regenerated Paws. Since AWS has also worked on those files, can you please take a look at if the merge was correct?

Here you have the PinPoint generated with the merged botocore definitions: https://github.com/pplu/aws-sdk-perl/tree/pinpoint_patch. The merge to the definition is here: pplu/botocore@7787a1f

Once you confirm, we should be ready for shipping 0.41

@byterock

This comment has been minimized.

Copy link

commented Jul 3, 2019

Not sure if this is what you need but I had to make some changes to

lib/Paws/API/Caller.pm and
lib/Paws/Net/RestJsonCaller.pm

and of course some changes to

botocore/data/pinpoint/2016-12-01/service-2.json

Now how am I going to get those changes into you brach I cannot seem to push to it.

Do you want a Diff or the raw files or should I branch/pull

Not 100% sure what to do??

@byterock

This comment has been minimized.

Copy link

commented Jul 3, 2019

I did a second round of testing now only

lib/Paws/API/Caller.pm

needs the '_is_internal_type ' sub added

+  sub _is_internal_type {
+    my ($self, $att_type) = @_;
+    return ($att_type eq 'Str' or $att_type eq 'Str|Undef' or $att_type eq 'Int' or $att_type eq 'Bool' or $att_type eq 'Num');
+  }

and some changes to service-2.json


diff --git a/botocore/data/pinpoint/2016-12-01/service-2.json b/botocore/data/pinpoint/2016-12-01/service-2.json
index 870ff6fd..c796032c 100644
--- a/botocore/data/pinpoint/2016-12-01/service-2.json
+++ b/botocore/data/pinpoint/2016-12-01/service-2.json
@@ -23,7 +23,7 @@
         "shape" : "CreateAppRequest"
       },
       "output" : {
-        "shape" : "CreateAppResponse",
+        "shape" : "ApplicationResponse",
         "documentation" : "<p>The request succeeded and the specified resource was created.</p>"
       },
       "errors" : [ {
@@ -58,7 +58,7 @@
         "shape" : "CreateCampaignRequest"
       },
       "output" : {
-        "shape" : "CreateCampaignResponse",
+        "shape" : "CampaignResponse",
         "documentation" : "<p>The request succeeded and the specified resource was created.</p>"
       },
       "errors" : [ {
@@ -163,7 +163,7 @@
         "shape" : "CreateSegmentRequest"
       },
       "output" : {
-        "shape" : "CreateSegmentResponse",
+        "shape" : "SegmentResponse",
         "documentation" : "<p>The request succeeded and the specified resource was created.</p>"
       },
       "errors" : [ {
@@ -373,7 +373,7 @@
         "shape" : "DeleteAppRequest"
       },
       "output" : {
-        "shape" : "DeleteAppResponse",
+        "shape" : "ApplicationResponse",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -933,7 +933,7 @@
         "shape" : "GetAppRequest"
       },
       "output" : {
-        "shape" : "GetAppResponse",
+        "shape" : "ApplicationResponse",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -968,7 +968,7 @@
         "shape" : "GetApplicationSettingsRequest"
       },
       "output" : {
-        "shape" : "GetApplicationSettingsResponse",
+        "shape" : "ApplicationSettingsResource",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -1108,7 +1108,7 @@
         "shape" : "GetCampaignActivitiesRequest"
       },
       "output" : {
-        "shape" : "GetCampaignActivitiesResponse",
+        "shape" : "ActivitiesResponse",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -1143,7 +1143,7 @@
         "shape" : "GetCampaignVersionRequest"
       },
       "output" : {
-        "shape" : "GetCampaignVersionResponse",
+        "shape" : "CampaignResponse",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -1178,7 +1178,7 @@
         "shape" : "GetCampaignVersionsRequest"
       },
       "output" : {
-        "shape" : "GetCampaignVersionsResponse",
+        "shape" : "CampaignsResponse",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -1248,7 +1248,7 @@
         "shape" : "GetChannelsRequest"
       },
       "output" : {
-        "shape" : "GetChannelsResponse",
+        "shape" : "ChannelsResponse",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -1318,7 +1318,7 @@
         "shape" : "GetEndpointRequest"
       },
       "output" : {
-        "shape" : "GetEndpointResponse",
+        "shape" : "EndpointResponse",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -1895,7 +1895,7 @@
         "shape" : "PhoneNumberValidateRequest"
       },
       "output" : {
-        "shape" : "PhoneNumberValidateResponse",
+        "shape" : "NumberValidateResponse",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -2446,7 +2446,7 @@
         "shape" : "UpdateEndpointRequest"
       },
       "output" : {
-        "shape" : "UpdateEndpointResponse",
+        "shape" : "MessageBody",
         "documentation" : "<p>The request was accepted for processing. Processing may not be complete.</p>"
       },
       "errors" : [ {
@@ -5765,7 +5765,7 @@
       },
       "required" : [ "ApplicationId", "CampaignId" ]
     },
-    "GetCampaignVersionsResponse" : {
+    "CampaignVersionsResponse" : {
       "type" : "structure",
       "members" : {
         "CampaignsResponse" : {

@byterock

This comment has been minimized.

Copy link

commented Jul 3, 2019

opps one more for Boto

@@ -2035,7 +2035,7 @@
"shape" : "SendMessagesRequest"
},
"output" : {

  •    "shape" : "SendMessagesResponse",
    
  •    "shape" : "MessageResponse",
       "documentation" : "<p>The request succeeded.</p>"
     },
     "errors" : [ {
    
@pplu

This comment has been minimized.

Copy link
Owner

commented Jul 8, 2019

I did a second round of testing now only

lib/Paws/API/Caller.pm

needs the '_is_internal_type ' sub added

+  sub _is_internal_type {
+    my ($self, $att_type) = @_;
+    return ($att_type eq 'Str' or $att_type eq 'Str|Undef' or $att_type eq 'Int' or $att_type eq 'Bool' or $att_type eq 'Num');
+  }

Who is calling this? I can't find anyone calling _is_internal_type in branch release/0.41.

I'll apply your botocore changes and regenerate everything in a while.

@byterock

This comment has been minimized.

Copy link

commented Jul 8, 2019

Ah ok that is in 'PAWS.pm'

I might not need that after all.

Will give it another try. Should get back to you within the hour.

@byterock

This comment has been minimized.

Copy link

commented Jul 8, 2019

Ok I see where I made a mistake

I think this is only patch for the core code

/lib/Paws/Net/RestJsonCaller.pm

diff --git a/lib/Paws/Net/RestJsonCaller.pm b/lib/Paws/Net/RestJsonCaller.pm
index b6d39b8f1..1ac2b8898 100644
--- a/lib/Paws/Net/RestJsonCaller.pm
+++ b/lib/Paws/Net/RestJsonCaller.pm
@@ -5,7 +5,7 @@ package Paws::Net::RestJsonCaller;
   use POSIX qw(strftime);
   use URI::Template;
   use JSON::MaybeXS;
-
+  use Scalar::Util;
   use Paws::Net::RestJsonResponse;

   has response_to_object => (
@@ -116,12 +116,26 @@ package Paws::Net::RestJsonCaller;

     if ($call->can('_stream_param')) {
       my $param_name = $call->_stream_param;
-      $request->content($call->$param_name);
-      #$request->headers->header( 'content-length' => $request->content_length );
+      if (Scalar::Util::blessed($call->$param_name)){
+          my $attribute = $call->$param_name;
+          my $content   = encode_json($self->_to_jsoncaller_params($attribute));
+          $request->content($content);
+          $request->headers->header('Content-Type'=>'application/json');
+          $request->headers->header('Content-Length'=>length($content));
+      }
+      else {
+          $request->content($call->$param_name);
+      }
+         #$request->headers->header( 'content-length' => $request->content_length );
       #$request->headers->header( 'content-type'   => $self->content_type );
     } else {
       my $data = $self->_to_jsoncaller_params($call);
-      $request->content(encode_json($data)) if (keys %$data > 0);
+         if (keys(%{$data})){
+          $request->content(encode_json($data));
+      }
+      else {
+          $request->content("");
+      }
     }

The Boto changes stay the same

If you can merge that patch in I can try again with a clean checkout. I have my test suite up and running .

pplu added a commit that referenced this issue Jul 8, 2019

@pplu

This comment has been minimized.

Copy link
Owner

commented Jul 8, 2019

Hi,

Just pushed your changes to the pinpoint_patch branch. This branch has been generated with your botocore changes applied too.

@byterock

This comment has been minimized.

Copy link

commented Jul 8, 2019

Hmm still something not comming back corrctetly. Will have to spend some more cycles tonight and see what is going on.
I might be something in boto I am fairly sure the Paws code is ok

Look for another post later tonight

@byterock

This comment has been minimized.

Copy link

commented Jul 8, 2019

Yep just had a chance to check things again and there are still some boto issues

diff --git a/botocore/data/pinpoint/2016-12-01/service-2.json b/botocore/data/pinpoint/2016-12-01/service-2.json
index 870ff6fd..98676dfd 100644
--- a/botocore/data/pinpoint/2016-12-01/service-2.json
+++ b/botocore/data/pinpoint/2016-12-01/service-2.json
@@ -23,7 +23,7 @@
         "shape" : "CreateAppRequest"
       },
       "output" : {
-        "shape" : "CreateAppResponse",
+        "shape" : "ApplicationResponse",
         "documentation" : "<p>The request succeeded and the specified resource was created.</p>"
       },
       "errors" : [ {
@@ -58,7 +58,7 @@
         "shape" : "CreateCampaignRequest"
       },
       "output" : {
-        "shape" : "CreateCampaignResponse",
+        "shape" : "CampaignResponse",
         "documentation" : "<p>The request succeeded and the specified resource was created.</p>"
       },
       "errors" : [ {
@@ -163,7 +163,7 @@
         "shape" : "CreateSegmentRequest"
       },
       "output" : {
-        "shape" : "CreateSegmentResponse",
+        "shape" : "SegmentResponse",
         "documentation" : "<p>The request succeeded and the specified resource was created.</p>"
       },
       "errors" : [ {
@@ -373,7 +373,7 @@
         "shape" : "DeleteAppRequest"
       },
       "output" : {
-        "shape" : "DeleteAppResponse",
+        "shape" : "ApplicationResponse",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -933,7 +933,7 @@
         "shape" : "GetAppRequest"
       },
       "output" : {
-        "shape" : "GetAppResponse",
+        "shape" : "ApplicationResponse",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -968,7 +968,7 @@
         "shape" : "GetApplicationSettingsRequest"
       },
       "output" : {
-        "shape" : "GetApplicationSettingsResponse",
+        "shape" : "ApplicationSettingsResource",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -1108,7 +1108,7 @@
         "shape" : "GetCampaignActivitiesRequest"
       },
       "output" : {
-        "shape" : "GetCampaignActivitiesResponse",
+        "shape" : "ActivitiesResponse",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -1143,7 +1143,7 @@
         "shape" : "GetCampaignVersionRequest"
       },
       "output" : {
-        "shape" : "GetCampaignVersionResponse",
+        "shape" : "CampaignResponse",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -1178,7 +1178,7 @@
         "shape" : "GetCampaignVersionsRequest"
       },
       "output" : {
-        "shape" : "GetCampaignVersionsResponse",
+        "shape" : "CampaignsResponse",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -1248,7 +1248,7 @@
         "shape" : "GetChannelsRequest"
       },
       "output" : {
-        "shape" : "GetChannelsResponse",
+        "shape" : "ChannelsResponse",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -1318,7 +1318,7 @@
         "shape" : "GetEndpointRequest"
       },
       "output" : {
-        "shape" : "GetEndpointResponse",
+        "shape" : "EndpointResponse",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -1895,7 +1895,7 @@
         "shape" : "PhoneNumberValidateRequest"
       },
       "output" : {
-        "shape" : "PhoneNumberValidateResponse",
+        "shape" : "NumberValidateResponse",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -2035,7 +2035,7 @@
         "shape" : "SendMessagesRequest"
       },
       "output" : {
-        "shape" : "SendMessagesResponse",
+        "shape" : "MessageResponse",
         "documentation" : "<p>The request succeeded.</p>"
       },
       "errors" : [ {
@@ -2446,7 +2446,7 @@
         "shape" : "UpdateEndpointRequest"
       },
       "output" : {
-        "shape" : "UpdateEndpointResponse",
+        "shape" : "MessageBody",
         "documentation" : "<p>The request was accepted for processing. Processing may not be complete.</p>"
       },
       "errors" : [ {
@@ -5765,7 +5765,7 @@
       },
       "required" : [ "ApplicationId", "CampaignId" ]
     },
-    "GetCampaignVersionsResponse" : {
+    "CampaignVersionsResponse" : {
       "type" : "structure",
       "members" : {
         "CampaignsResponse" : {

boto.txt

@pplu

This comment has been minimized.

Copy link
Owner

commented Jul 9, 2019

Please attach the whole pinpoint service-2.json, or point me to it in a repo. I'll just copy that over.

@byterock

This comment has been minimized.

Copy link

commented Jul 9, 2019

ok attached my version of service-2.json

service-2.json.gz

pplu added a commit that referenced this issue Jul 10, 2019

@pplu

This comment has been minimized.

Copy link
Owner

commented Jul 10, 2019

Your changes with the submitted service-2.json are on the pinpoint_patch branch. Hope everything is OK so we can release!

@byterock

This comment has been minimized.

Copy link

commented Jul 10, 2019

Success.

I did clean build l from the 'pinpoint_patch' branch and all my tests pass.

just for kicks here is an edited review of what I did



[PAWS]$  git clone https://github.com/pplu/aws-sdk-perl.git
-perl'...
remote: Enumerating objects: 3397, done.
remote: Counting objects: 100% (3397/3397), done.
remote: Compressing objects: 100% (2063/2063), done.
remote: Total 144080 (delta 2167), reused 1942 (delta 1330), pack-reused 140683
Receiving objects: 100% (144080/144080), 40.72 MiB | 24.18 MiB/s, done.
Resolving deltas: 100% (125603/125603), done.
[PAWS]$ cd aws-sdk-perl
[aws-sdk-perl]$ git checkout pinpoint_patch
Branch pinpoint_patch set up to track remote branch pinpoint_patch from origin.
Switched to a new branch 'pinpoint_patch'
[aws-sdk-perl]$ git pull
Already up-to-date.
[aws-sdk-perl]$ carton install
...
157 distributions installed
Complete! Modules were installed into /home/scolesj/PAWS/aws-sdk-perl/local
[aws-sdk-perl]$ make pull-other-sdks
git submodule init
Submodule 'botocore' (https://github.com/pplu/botocore.git) registered for path 'botocore'
git submodule update
Cloning into '/home/scolesj/PAWS/aws-sdk-perl/botocore'...
Submodule path 'botocore': checked out '7d94ef85b32b9c5ac6b9a064c8079f0663953702'
cd botocore && \
  git checkout develop
Previous HEAD position was 7d94ef85... service-2.json from byterock
Switched to branch 'develop'
Your branch is up-to-date with 'origin/develop'.
cd botocore && \
  if [ -z "`git remote -v | grep ^boto`" ]; then git remote add boto https://github.com/boto/botocore.git; fi

Then I ran my tests making sure I was pointing to the correct libs

./full_pp_test.t
ok 1 - 00_CreateApp worked got Paws::Pinpoint::ApplicationResponse
ok 2 - 01_GetApps worked got Paws::Pinpoint::GetAppsResponse
ok 3 - 02_GetApp worked got Paws::Pinpoint::ApplicationResponse
ok 4 - 03_GetApplicationSettings worked got Paws::Pinpoint::ApplicationSettingsResource
ok 5 - 04_CreateEndpoint worked got Paws::Pinpoint::MessageBody
ok 6 - 05_CreateSegment worked got Paws::Pinpoint::SegmentResponse
ok 7 - 06_CreateCampaign worked got Paws::Pinpoint::CampaignResponse
ok 8 - 07_GetCampaignActivities worked got Paws::Pinpoint::ActivitiesResponse
ok 9 - 08_GetCampaignVersions worked got Paws::Pinpoint::CampaignsResponse
ok 10 - 09_GetCampaignVersion worked got Paws::Pinpoint::CampaignResponse
ok 11 - 10_GetChannels worked got Paws::Pinpoint::ChannelsResponse
ok 12 - 11_GetEndpoint worked got Paws::Pinpoint::EndpointResponse
ok 13 - 98_DeleteApp worked got Paws::Pinpoint::ApplicationResponse
ok 14 - 99_PhoneNumberValidate worked got Paws::Pinpoint::NumberValidateResponse

So I am good to go.

@pplu

This comment has been minimized.

Copy link
Owner

commented Jul 11, 2019

0.41 Just hit the CPAN with a fix for this issue. Thanks for your hard work. I think we should follow up on how to add some test cases (I've opened #336 to do this).

@pplu pplu closed this Jul 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.