Fix multiple XML collection deserialization bugs (#768)

This commit fixes a couple issues with generating code to deserialize
structure, union, or body members that target collection members.

1. Unflattened shapes that could return multiple entries in their
collection but only return one would result in an error at runtime
trying to iterate over an object like it was an Array.
2. Unflattened shapes that were not set in the provided output would
result in an error at runtime when trying to validate the entry element
exists without validating that its parent element existed.

This update also bumps the version of smithy-aws-traits to latest.

Author: kstich

codegen/smithy-aws-typescript-codegen/build.gradle.kts

@@ -33,7 +33,7 @@ tasks.withType<Test> {
 }
 
 dependencies {
-    api("software.amazon.smithy:smithy-aws-traits:0.9.6")
+    api("software.amazon.smithy:smithy-aws-traits:0.9.7")
     api("software.amazon.smithy:smithy-typescript-codegen:0.1.0")
     testCompile("org.junit.jupiter:junit-jupiter-api:5.4.0")
     testRuntime("org.junit.jupiter:junit-jupiter-engine:5.4.0")

codegen/smithy-aws-typescript-codegen/src/main/java/software/amazon/smithy/aws/typescript/codegen/AwsQuery.java

@@ -23,7 +23,6 @@
 import software.amazon.smithy.model.traits.TimestampFormatTrait.Format;
 import software.amazon.smithy.typescript.codegen.TypeScriptWriter;
 import software.amazon.smithy.typescript.codegen.integration.HttpRpcProtocolGenerator;
-import software.amazon.smithy.typescript.codegen.integration.ProtocolGenerator;
 
 /**
  * Handles generating the aws.query protocol for services. It handles reading and
@@ -33,15 +32,12 @@
  * This builds on the foundations of the {@link HttpRpcProtocolGenerator} to handle
  * standard components of the HTTP requests and responses.
  *
- * A set of service-specific customizations exist for Amazon EC2:
- *
  * @see QueryShapeSerVisitor
  * @see XmlShapeDeserVisitor
  * @see QueryMemberSerVisitor
  * @see XmlMemberDeserVisitor
  * @see AwsProtocolUtils
  * @see <a href="https://awslabs.github.io/smithy/spec/xml.html">Smithy XML traits.</a>
- * @see <a href="https://awslabs.github.io/smithy/spec/aws-core.html#ec2QueryName-trait">Smithy EC2 Query Name trait.</a>
  */
 final class AwsQuery extends HttpRpcProtocolGenerator {
 
@@ -76,14 +72,14 @@ protected void generateDocumentBodyShapeDeserializers(GenerationContext context,
     }
 
     @Override
-    public void generateSharedComponents(ProtocolGenerator.GenerationContext context) {
+    public void generateSharedComponents(GenerationContext context) {
         super.generateSharedComponents(context);
         AwsProtocolUtils.generateXmlParseBody(context);
 
         TypeScriptWriter writer = context.getWriter();
 
         // Generate a function that handles the complex rules around deserializing
-        // an error code from a rest-xml error.
+        // an error code from an xml error body.
         SymbolReference responseType = getApplicationProtocol().getResponseType();
         writer.openBlock("const loadQueryErrorCode = (\n"
                        + "  output: $T,\n"

codegen/smithy-aws-typescript-codegen/src/main/java/software/amazon/smithy/aws/typescript/codegen/AwsRestXml.java

@@ -225,14 +225,19 @@ protected void deserializeOutputDocument(
             List<HttpBinding> documentBindings
     ) {
         SymbolProvider symbolProvider = context.getSymbolProvider();
-        XmlShapeDeserVisitor shapeDeserVisitor = new XmlShapeDeserVisitor(context);
+        XmlShapeDeserVisitor shapeVisitor = new XmlShapeDeserVisitor(context);
 
         for (HttpBinding binding : documentBindings) {
             MemberShape memberShape = binding.getMember();
+            // Grab the target shape so we can use a member deserializer on it.
+            Shape target = context.getModel().expectShape(memberShape.getTarget());
             // The name of the member to get from the output shape.
             String memberName = symbolProvider.toMemberName(memberShape);
 
-            shapeDeserVisitor.deserializeNamedStructureMember(context, memberName, memberShape, () -> "data");
+            shapeVisitor.deserializeNamedMember(context, memberName, memberShape, "data", (dataSource, visitor) -> {
+                TypeScriptWriter writer = context.getWriter();
+                writer.write("contents.$L = $L;", memberName, target.accept(visitor));
+            });
         }
     }
 }

codegen/smithy-aws-typescript-codegen/src/main/java/software/amazon/smithy/aws/typescript/codegen/XmlShapeDeserVisitor.java

@@ -15,8 +15,11 @@
 
 package software.amazon.smithy.aws.typescript.codegen;
 
+import java.util.ArrayList;
+import java.util.List;
 import java.util.Map;
-import java.util.function.Supplier;
+import java.util.function.BiConsumer;
+import java.util.stream.Collectors;
 import software.amazon.smithy.codegen.core.CodegenException;
 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.shapes.CollectionShape;
@@ -99,17 +102,35 @@ protected void deserializeStructure(GenerationContext context, StructureShape sh
             members.forEach((memberName, memberShape) -> writer.write("$L: undefined,", memberName));
         });
 
-        members.forEach((memberName, memberShape) ->
-                deserializeNamedStructureMember(context, memberName, memberShape, () -> "output"));
+        members.forEach((memberName, memberShape) -> {
+            // Grab the target shape so we can use a member deserializer on it.
+            Shape target = context.getModel().expectShape(memberShape.getTarget());
+            deserializeNamedMember(context, memberName, memberShape, "output", (dataSource, visitor) -> {
+                writer.write("contents.$L = $L;", memberName, target.accept(visitor));
+            });
+        });
 
         writer.write("return contents;");
     }
 
-    void deserializeNamedStructureMember(
+    /**
+     * Generates an if statement for deserializing an output member, validating its
+     * presence at the correct location, handling collections and flattening, and
+     * dispatching to the supplied function to generate the body of the if statement.
+     *
+     * @param context The generation context.
+     * @param memberName The name of the member being deserialized.
+     * @param memberShape The shape of the member being deserialized.
+     * @param inputLocation The parent input location of the member being deserialized.
+     * @param statementBodyGenerator A function that generates the proper deserialization
+     *   after member presence is validated.
+     */
+    void deserializeNamedMember(
             GenerationContext context,
             String memberName,
             MemberShape memberShape,
-            Supplier<String> inputLocation
+            String inputLocation,
+            BiConsumer<String, DocumentMemberDeserVisitor> statementBodyGenerator
     ) {
         TypeScriptWriter writer = context.getWriter();
         Model model = context.getModel();
@@ -122,28 +143,41 @@ void deserializeNamedStructureMember(
         Shape target = context.getModel().expectShape(memberShape.getTarget());
         // Handle @xmlFlattened for collections and maps.
         boolean isFlattened = memberShape.hasTrait(XmlFlattenedTrait.class);
+        // Handle targets that return multiple entities per member.
+        boolean deserializationReturnsArray = deserializationReturnsArray(target);
 
         // Build a string based on the traits that represents the location.
         // Note we don't need to handle @xmlAttribute here because the parser flattens
         // attributes in to the main structure.
-        StringBuilder sourceBuilder = new StringBuilder(inputLocation.get())
-                .append("['").append(locationName).append("']");
-
-        // Go in to a specialized element for unflattened aggregates
-        if (deserializationReturnsArray(target) && !isFlattened) {
+        StringBuilder sourceBuilder = new StringBuilder(inputLocation).append("['").append(locationName).append("']");
+        // Track any locations we need to validate on our way to the final element.
+        List<String> locationsToValidate = new ArrayList<>();
+
+        // Go in to a specialized element for unflattened aggregates.
+        if (deserializationReturnsArray && !isFlattened) {
+            // Make sure we validate the wrapping element is set.
+            locationsToValidate.add(sourceBuilder.toString());
+            // Update the target element.
             String targetLocation = getUnnamedAggregateTargetLocation(model, target);
             sourceBuilder.append("['").append(targetLocation).append("']");
         }
 
         // Handle the response property.
         String source = sourceBuilder.toString();
-        writer.openBlock("if ($L !== undefined) {", "}", source, () -> {
-            if (isFlattened) {
+        // Validate the resulting target element is set.
+        locationsToValidate.add(source);
+        // Build a string of the elements to validate before deserializing.
+        String validationStatement = locationsToValidate.stream()
+                .map(location -> location + " !== undefined")
+                .collect(Collectors.joining(" && "));
+        writer.openBlock("if ($L) {", "}", validationStatement, () -> {
+            String dataSource = source;
+            // The XML parser will set one K:V for a member that could return multiple entries but only has one.
+            if (deserializationReturnsArray) {
                 writer.write("const wrappedItem = ($1L instanceof Array) ? $1L : [$1L];", source);
+                dataSource = "wrappedItem";
             }
-            writer.write("contents.$L = $L;", memberName,
-                    // Dispatch to the output value provider for any additional handling.
-                    target.accept(getMemberVisitor(isFlattened ? "wrappedItem" : source)));
+            statementBodyGenerator.accept(dataSource, getMemberVisitor(dataSource));
         });
     }
 
@@ -165,38 +199,16 @@ private String getUnnamedAggregateTargetLocation(Model model, Shape shape) {
     @Override
     protected void deserializeUnion(GenerationContext context, UnionShape shape) {
         TypeScriptWriter writer = context.getWriter();
-        Model model = context.getModel();
 
         // Check for any known union members and return when we find one.
         Map<String, MemberShape> members = shape.getAllMembers();
         members.forEach((memberName, memberShape) -> {
-            // Use the @xmlName trait if present on the member, otherwise use the member name.
-            String locationName = memberShape.getTrait(XmlNameTrait.class)
-                    .map(XmlNameTrait::getValue)
-                    .orElse(memberName);
             // Grab the target shape so we can use a member deserializer on it.
-            Shape target = context.getModel()
-                                   .expectShape(memberShape.getTarget());
-            // Handle @xmlFlattened for collections and maps.
-            boolean isFlattened = memberShape.hasTrait(XmlFlattenedTrait.class);
-
-            // Build a string based on the traits that represents the location.
-            // Note we don't need to handle @xmlAttribute here because the parser flattens
-            // attributes in to the main structure.
-            StringBuilder sourceBuilder = new StringBuilder("output['").append(locationName).append("']");
-
-            // Go in to a specialized element for unflattened aggregates
-            if (deserializationReturnsArray(target) && !isFlattened) {
-                String targetLocation = getUnnamedAggregateTargetLocation(model, target);
-                sourceBuilder.append("['").append(targetLocation).append("']");
-            }
-
-            // Handle the response property.
-            String source = sourceBuilder.toString();
-            writer.openBlock("if ($L !== undefined) {", "}", source, () -> {
+            Shape target = context.getModel().expectShape(memberShape.getTarget());
+            deserializeNamedMember(context, memberName, memberShape, "output", (dataSource, visitor) -> {
                 writer.openBlock("return {", "};", () -> {
                     // Dispatch to the output value provider for any additional handling.
-                    writer.write("$L: $L", memberName, target.accept(getMemberVisitor(source)));
+                    writer.write("$L: $L", memberName, target.accept(visitor));
                 });
             });
         });