Route53::ChangeTagsForResource not generated in a correct fashion

[piratefinn]

Severe XML generation issues were noticed while making tests for creating the XML for a method, noticeably ChangeTagsForResource. It was noticed that in part of RestXmlCaller.pm, if the attribute of a method was an Array of Objects (such as the ArrayRef[Paws::Route53::Tag] of AddTags in ChangeTagsForResource and because it was being passed the object inside an array, it got confused and broke down completely.

In the process of fixing this, two changes were made. firstly, as seen in this commit on a branch currently being worked on, RestXmlCaller was generally made more sane to properly deal with everything, along with avoiding repetition of code. This ironically had a fallout effect of helping improve the XML generation of other methods being tested somewhat.

The second change was one that would require a change in the autolib generation, which is the addition of traits and request_name's to AddKeys and RemoveTagKeys in ChangeTagsForResource as seen here:

has AddTags => (is => 'ro', isa => 'ArrayRef[Paws::Route53::Tag]', traits => ['NameInRequest'], request_name => 'Key');
has RemoveTagKeys => (is => 'ro', isa => 'ArrayRef[Str|Undef]', traits => ['NameInRequest'], request_name => 'Tag');

Because I realise this is an auto-lib change and thus one on a generated file, it comes down to "We need a way to fix this on generation", and/or we improve RestXmlCaller.pm somehow. Hopefully this issue is clear enough.

[pplu]

Hi,

The other serializers look to see if the constraints data type is an object or a "simple" value, and then take action accordingly (without the need for extra signaling from the class. If I've understood correctly, this can be done without modifying class generation.

[shadowcat-mst]

No. That logic is necessary (and present) but not sufficient, here.

Remember that, being XML, it has to generate e.g.

<AddTags>
  <Tag>foo</Tag>
  <Tag>bar</Tag>
</AddTags>

so the name of the inner element has to come from somewhere, and that trait is how it's handled for everything else.

The previous code was flat-out broken and I refactored it to not be, but while I'm pretty good at miracles, "conjure the right XML tag name out of thin air" is sadly a little beyond even my abilities given the names of the inner tags don't seem to have any particular correspondence to the name of the outer tags (i.e. I can't do some "de-pluralise" type trick).

The object/simple value stuff is actually what I fixed first, and then now it was actually trying to serialize the arrayrefs rather than just throwing an exception we discovered the missing information.

So unless I'm completely confused, we do need to modify the class generation and some advice on doing so would be much appreciated.

[pplu]

OK. I understand the problem better (the example of the XML that had to be generated put me gave me the context I needed). I think this is best dealth with adding stuff to the auto-lib classes (to signal what to do correctly) instead of applying some heuristics (pluralizing, depluralizing, etc) which in this case is not even possible 😆

So, tips around changing the builders: for Route53: being a RestXML type service you would add the logic here: templates/restxml/callargs_class.tt. The class that generates everything is Paws/API/Builder/restxml.pm, which basically has all it's logic in Paws/API/Builder.pm.

Instead of generating the whole SDK every time, you can call build-bin/gen-classes.pl --classes botocore/botocore/data/route53/2013-04-01/service-2.json to just generate Route53. Be aware that you may be also changing any RestXML service too (S3 and CloudFront). You can add extra files to the end of gen-classes.pl to generate these too.

If you feel that you need a new role to signal the appropiate behaviour: go ahead: they are in lib/Paws/API.pm

I hope this is the advice you were looking for 🙏

[castaway]

In this particular case we actually want to generate: <AddTags> <Tag> <Key>string</Key> <Value>string</Value> </Tag> </AddTags>

In the service json we have: AddTags isa shape:TagList TagList isa type:list, member is shape:Tag Tag isa type:structure, members: Key, Value

The generation outputs this as: Paws::Route53::ChangeTagsForResource: has AddTags => (is => 'ro', isa => 'ArrayRef[Paws::Route53::Tag]');

Paws::Route53::Tag: has Key => (is => 'ro', isa => 'Str'); has Value => (is => 'ro', isa => 'Str');

So somehow we need to indicate/store what the name of the element inside the list is..

[castaway]

Some other items which are/have lists, do set the request_name this way.. some somehow twice, eg:

package Paws::S3::MetricsAndOperator; use Moose; has Prefix => (is => 'ro', isa => 'Str'); has Tags => (is => 'ro', isa => 'ArrayRef[Paws::S3::Tag]', request_name => 'Tag', request_name => 'Tag', traits => ['NameInRequest','NameInRequest']); 1;

.. This one is a sub shape, not a top-level method, so I'm guessing the generation is slightly different

[castaway]

The shape-handling differs in object.tt (the MetricsAndOperator object class template) and callargs_class.tt (the method class template) - specifically the first contains: [%- IF (member.type == 'list' .., and the second doesn't at all.

Could we extract the shape-handling code into a sub-template and re-use it in both?

[pplu]

As you whish. You guys know more about the details of the problem than me by now 😄

[castaway]

Tests in t/route53/xml_creation.t in tests/stabilisation verify that this has been fixed.

[byterock]

I will add these to my https://github.com/byterock/aws-sdk-perl/tree/s3ObjectTagging branch and see if I can get them to work as well