Parsing ArrayRef[Some::Class] with single attribute and single result fails

showaltb commented 5 years ago

I've found a problem in parsing an S3 ListBucketResult message containing a single <CommonPrefixes> value. Here is an example of a message that doesn't parse correctly:

<ListBucketResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
  <Name>fedora-completed-test</Name>
  <Prefix>41/</Prefix>
  <KeyCount>1</KeyCount>
  <MaxKeys>1000</MaxKeys>
  <Delimiter>/</Delimiter>
  <IsTruncated>false</IsTruncated>
  <CommonPrefixes>
    <Prefix>41/000/</Prefix>
  </CommonPrefixes>
</ListBucketResult>

Note the single result for <CommonPrefixes>.

The value of $unserialized_struct after https://github.com/pplu/aws-sdk-perl/blob/master/lib/Paws/Net/XMLResponse.pm#L301 is:

0  HASH(0x7fee64b9cb80)
   'CommonPrefixes' => HASH(0x7fee64b9f090)
      'Prefix' => '41/000/'
   'Delimiter' => '/'
   'IsTruncated' => 'false'
   'KeyCount' => 1
   'MaxKeys' => 1000
   'Name' => 'fedora-completed-test'
   'Prefix' => '41/'
   'xmlns' => 'http://s3.amazonaws.com/doc/2006-03-01/'

Note that CommonPrefixes is a hash ref. If there are multiple entries in the XML, CommonPrefixes would be an array ref.

The problem occurs in the block of code at https://github.com/pplu/aws-sdk-perl/blob/master/lib/Paws/Net/XMLResponse.pm#L241-L252:

        if ($value_ref eq 'HASH') {
          if (exists $value->{ member }) {
            $value = $value->{ member };
          } elsif (exists $value->{ entry }) {
            $value = $value->{ entry  };
          } elsif (keys %$value == 1) {
            $value = $value->{ (keys %$value)[0] };
          } else {
            #die "Can't detect the item that has the array in the response hash";
          }
          $value_ref = ref($value);
        }

Before this block of code runs for CommonPrefixes, $value is the HashRef { Prefix => '41/100' }. Because it has a single key, this case is run:

          } elsif (keys %$value == 1) {
            $value = $value->{ (keys %$value)[0] };

which changes $value to the string 41/100. This then fails to parse in the code immediately following which tries to build the ArrayRef[Paws::S3::CommonPrefix].

I think the problem is that:

The CommonPrefixes element contains only one result, which makes $value_ref eq 'HASH', invoking this block, and
The Paws::S3::CommonPrefix structure only has one attribute (Prefix). This makes keys %$value == 1, which invokes the corruption of $value.

I don't really understand the purpose of this block. I was able to get this particular case to parse by changing the test at line 246 from:

          } elsif (keys %$value == 1) {

to:

          } elsif (keys %$value == 1 && ref($value->{ (keys %$value)[0] })) {

In other words, don't replace $value unless the single hash value is itself a reference. But that's mostly a guess.

Also I see this construct used at other places in the file (e.g. lines 103, 129, and 203). I'm not sure if those have the same issue as well.

I will try to get development set up and see if I can understand what this block is supposed to be doing. If anyone has any guidance or suggestions, I would welcome them.

showaltb commented 5 years ago

@pplu any guidance you can give on this?

pplu commented 5 years ago

Hi!

Thanks for the ping. This got issue got sidetracked :sob:

I've pushed a test case in a branch to reproduce the bug while I try to make sense of everything, and investigate why that code was there in the first place.

Thanks for the detailed description of the bug. It helps a lot.

pplu commented 5 years ago

Note: you can checkout the branch and execute only these tests with:

carton exec perl -I lib/ t/10_responses.t t/10_responses/s3-list-buckets-oneprefix.response t/10_responses/s3-list-buckets-twoprefix.response

pplu commented 5 years ago

Here's what I see for now. This is just a braindump. I'm just commenting in the open so there can be discussion.

From what I see: it seems like the affected responses are for services that:

return XML (JSON services don't do this type of transformation)
there is a return object with only one attribute.

In this case the keys %$value == 1 will evaluate to true, with the keys that were supposedly for the objects constructor.

I want to know how many return objects with just one property there are in the APIs (TODO).

In RestXMLResponse:

For Arrays: the whole keys %$value == 1 condition can be commented and it doesn't break tests
For non-arrays: commenting the keys %$value == 1 condition breaks tests for t/10_responses/route53-listgeolocations.response
for maps, commenting the keys %$value == 1 condition doesn't break tests

In XMLResponse:

For non-arrays: it doesn't break anything
For arrays: Breaks tests for t/10_responses/ec2-cancel-reserved-instances-listing.response, t/10_responses/ec2-create-network-acl.response, t/10_responses/rds-describe-db-engine-versions.response and a bunch more.
for maps, commenting the keys %$value == 1 condition doensn't break tests

The keys %$value == 1 code was introduced in Paws 0.14 (from 2015). Changelog here: https://metacpan.org/changes/distribution/Paws#L672. git diff release-0.13 release-0.14 lib/ t/ reveals that t/10_responses/cloudformation-liststacks.response, t/10_responses/cloudformation-liststacks.response.test.yml, t/10_responses/cloudformation-validatetemplate.response, t/10_responses/cloudformation-validatetemplate.response, t/10_responses/monitoring-listmetrics.response were added.

For the moment this is what I've been able to investigate

castaway commented 4 years ago

Looks like the difference here is that CommonPrefixes contains a shape CommonPrefixList which has (one or more) of the CommonPrefix results. Because CommonPrefixList is flattened:true in the service-1.json, CommonPrefixList doesn't show up in the XML response.

This seems to pass both Route53 and the new fix/issue_315 tests. (Patch against tests/stabilisation) - there's probably a cleaner way to do it..

diff --git a/lib/Paws/Net/Caller.pm b/lib/Paws/Net/Caller.pm
index 442181a..6ea3a6f 100755
--- a/lib/Paws/Net/Caller.pm
+++ b/lib/Paws/Net/Caller.pm
@@ -43,7 +43,7 @@ package Paws::Net::Caller;
     if ($response->status == 599){
       return Paws::Exception->new(message => $response->content, code => 'ConnectionError', request_id => '');
     } else {
-      return $service->response_to_object->process($call_object, $response);
+      return $service->response_to_object->process($call_object, $response, $service->flattened_arrays);
     }
   }
 1;
diff --git a/lib/Paws/Net/ResponseRole.pm b/lib/Paws/Net/ResponseRole.pm
index 3f502c1..b567aeb 100644
--- a/lib/Paws/Net/ResponseRole.pm
+++ b/lib/Paws/Net/ResponseRole.pm
@@ -3,7 +3,7 @@ package Paws::Net::ResponseRole;

   sub response_to_object {
-    my ($self, $call_object, $response) = @_;
+    my ($self, $call_object, $response, $flattened) = @_;
     my ($http_status, $content, $headers) = ($response->status, $response->content, $response->headers);;

     $call_object = $call_object->meta->name;
@@ -56,7 +56,7 @@ package Paws::Net::ResponseRole;
         $unserialized_struct->{lc $key} = $headers->{$key};
       }

-      my $o_result = $self->new_from_result_struct($call_object->_returns, $unserialized_struct);
+      my $o_result = $self->new_from_result_struct($call_object->_returns, $unserialized_struct, , $flattened);
       return $o_result;
     } else {
       return Paws::API::Response->new(
diff --git a/lib/Paws/Net/RestXMLResponse.pm b/lib/Paws/Net/RestXMLResponse.pm
index c0bb1f0..2385b39 100644
--- a/lib/Paws/Net/RestXMLResponse.pm
+++ b/lib/Paws/Net/RestXMLResponse.pm
@@ -36,12 +36,12 @@ package Paws::Net::RestXMLResponse;
   }

   sub process {
-    my ($self, $call_object, $response) = @_;
+    my ($self, $call_object, $response, $flattened) = @_;

     if ( $response->status >= 300 ) {
         return $self->error_to_exception($call_object, $response);
     } else {
-        return $self->response_to_object($call_object, $response);
+        return $self->response_to_object($call_object, $response, $flattened);
     }
   }

@@ -134,9 +134,9 @@ package Paws::Net::RestXMLResponse;
   }

   sub new_from_result_struct {
-    my ($self, $class, $result) = @_;
+    my ($self, $class, $result, $flattened) = @_;
     my %args;
-    
+
     if ($class->does('Paws::API::StrToObjMapParser')) {
       return $self->handle_response_strtoobjmap($class, $result);
     } elsif ($class->does('Paws::API::StrToNativeMapParser')) {
@@ -248,10 +248,11 @@ package Paws::Net::RestXMLResponse;
             $value = $value->{ member };
           } elsif (exists $value->{ entry }) {
             $value = $value->{ entry  };
-          } elsif (keys %$value == 1) {
+          } elsif (keys %$value == 1 && !$flattened) {
             $value = $value->{ (keys %$value)[0] };
           } else {
-            #die "Can't detect the item that has the array in the response hash";
+              #die "Can't detect the item that has the array in the response hash";
+              $value = [ $value ];
           }
           $value_ref = ref($value);
         }

byterock commented 4 years ago

I think I have solved this one already.

Is there a specific test checked in or is it this just this block of XML

`

fedora-completed-test 41/ 1 1000 / false 41/000/

`

castaway commented 4 years ago

If you look back up this thread - there is a test thats in a branch that @pplu made..

It would be handy if you could go and comment on all the issue you've solved (preferably with how+where), so other folks don't go repeating work, please?

byterock commented 4 years ago

I had a little free time and copied over the tests from

https://github.com/pplu/aws-sdk-perl/commit/f58c7030155aa9d64af341352f3f0ce4f8330e23?diff=unified

and in my s3ObjectTagging Branch https://github.com/byterock/aws-sdk-perl/tree/s3ObjectTagging

all of the tests pass

perl t/10_responses.t t/10_responses/s3-list-buckets* Paws::S3 is not stable / supported / entirely developed at /home/scolesj/aws-sdk-perl/auto-lib/Paws/S3.pm line 2. ok 1 - Call S3->ListObjects from t/10_responses/s3-list-buckets-oneprefix.response ok 2 - Got Name eq fedora-completed-test from result ok 3 - Got Prefix eq 41/ from result ok 4 - Got CommonPrefixes.0.Prefix eq 41/000/ from result ok 5 - Call S3->ListBuckets from t/10_responses/s3-list-buckets.response ok 6 - Got Buckets.0.Name eq botostats from result ok 7 - Got Buckets.1.Name eq bucket-1355863083 from result ok 8 - Got Buckets.2.Name eq elasticbeanstalk-us-east-1-419278470775 from result ok 9 - Got Buckets.3.Name eq elasticbeanstalk-us-west-2-419278470775 from result ok 10 - Got Buckets.4.Name eq encryption-1332788550 from result ok 11 - Got Buckets.5.Name eq encryption-1346779637 from result ok 12 - Got Buckets.6.Name eq garnaat-amazon from result ok 13 - Got Buckets.7.Name eq garnaat_test_lifecycle from result ok 14 - Got Buckets.8.Name eq keytest-1355862601 from result ok 15 - Got Buckets.9.Name eq keytest-1355863376 from result ok 16 - Got Buckets.10.Name eq mgtest2cloudformation from result ok 17 - Got Buckets.11.Name eq mgtestcloudformation from result ok 18 - Got Buckets.12.Name eq mitchtestcloudformation from result ok 19 - Got Buckets.13.Name eq multidelete-1355862684 from result ok 20 - Got Buckets.14.Name eq multidelete-1355862730 from result ok 21 - Got Buckets.15.Name eq multidelete-1355863407 from result ok 22 - Got Buckets.16.Name eq multidelete-1355863439 from result ok 23 - Got Buckets.17.Name eq multidelete-1355863446 from result ok 24 - Got Buckets.18.Name eq multidelete-1355863455 from result ok 25 - Got Buckets.19.Name eq multidelete-1355863464 from result ok 26 - Got Buckets.20.Name eq multidelete-1355863470 from result ok 27 - Got Buckets.21.Name eq multipart-1355862845 from result ok 28 - Got Buckets.22.Name eq multipart-1355862901 from result ok 29 - Got Buckets.23.Name eq multipart-1355863550 from result ok 30 - Got Buckets.24.Name eq multipart-1355863593 from result ok 31 - Got Buckets.25.Name eq pyconprod from result ok 32 - Got Buckets.26.Name eq src-bucket-1332098908 from result ok 33 - Got Buckets.27.Name eq src-bucket-1332788526 from result ok 34 - Got Buckets.28.Name eq src-bucket-1332789019 from result ok 35 - Got Buckets.29.Name eq src-bucket-1332868116 from result ok 36 - Got Buckets.30.Name eq src-bucket-1339608971 from result ok 37 - Got Buckets.31.Name eq src-bucket-1357699848 from result ok 38 - Got Buckets.32.Name eq stats.pythonboto.org from result ok 39 - Got Buckets.33.Name eq test-1357854245 from result ok 40 - Got Buckets.34.Name eq test-1357854246 from result ok 41 - Got Buckets.35.Name eq version-1328224364 from result ok 42 - Call S3->ListObjects from t/10_responses/s3-list-buckets-twoprefix.response ok 43 - Got Name eq fedora-completed-test from result ok 44 - Got Prefix eq 41/ from result ok 45 - Got CommonPrefixes.0.Prefix eq 41/000/ from result ok 46 - Got CommonPrefixes.1.Prefix eq 41/001/ from result 1..46`

Will have a peek about to see what else I might of fixed

showaltb commented 4 years ago

Original reporter here. Thanks all for the yeoman work on this. Looking forward to next release and getting rid of my monkey patch :smile:

pplu / aws-sdk-perl

Parsing ArrayRef[Some::Class] with single attribute and single result fails #315