fix(codegen): check javascript property validity for property access (#3649)

* fix(codegen): check javascript property validity for property access

* fix(codegen): checkstyle fixes

* fix(codegen): property access code cleanup

Author: kuhe

.gitignore

@@ -47,6 +47,7 @@ dist-*
 
 *.tsbuildinfo
 
+codegen/.attach*
 codegen/.project
 codegen/.classpath
 codegen/.settings/

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

@@ -15,6 +15,8 @@
 
 package software.amazon.smithy.aws.typescript.codegen;
 
+import static software.amazon.smithy.aws.typescript.codegen.propertyaccess.PropertyAccessor.getFrom;
+
 import java.util.List;
 import java.util.Set;
 import software.amazon.smithy.aws.traits.protocols.RestXmlTrait;
@@ -36,6 +38,7 @@
 import software.amazon.smithy.typescript.codegen.integration.HttpBindingProtocolGenerator;
 import software.amazon.smithy.utils.SmithyInternalApi;
 
+
 /**
  * Handles generating the aws.rest-xml protocol for services. It handles reading and
  * writing from document bodies, including generating any functions needed for
@@ -267,10 +270,10 @@ private void serializePayload(
         writer.write("let contents: any;");
 
         // Generate an if statement to set the body node if the member is set.
-        writer.openBlock("if (input.$L !== undefined) {", "}", memberName, () -> {
+        writer.openBlock("if ($L !== undefined) {", "}", getFrom("input", memberName), () -> {
             Shape target = context.getModel().expectShape(member.getTarget());
             writer.write("contents = $L;",
-                    getInputValue(context, Location.PAYLOAD, "input." + memberName, member, target));
+                    getInputValue(context, Location.PAYLOAD, getFrom("input", memberName), member, target));
 
             String targetName = target.getTrait(XmlNameTrait.class)
                             .map(XmlNameTrait::getValue)

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

@@ -15,6 +15,8 @@
 
 package software.amazon.smithy.aws.typescript.codegen;
 
+import static software.amazon.smithy.aws.typescript.codegen.propertyaccess.PropertyAccessor.getFrom;
+
 import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.HashSet;
@@ -176,7 +178,7 @@ private void generateCommandMiddlewareResolver(String configType) {
                     } else {
                         writer.openBlock("$L(", ")", marshallInput, () -> {
                             writer.write("this.input,");
-                            writer.write("this.$L,", COMMAND_INPUT_KEYNODES);
+                            writer.write(getFrom("this", COMMAND_INPUT_KEYNODES) + ",");
                             writer.write("$L,", marshallOptions);
                         });
                     }

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

@@ -15,6 +15,8 @@
 
 package software.amazon.smithy.aws.typescript.codegen;
 
+import static software.amazon.smithy.aws.typescript.codegen.propertyaccess.PropertyAccessor.getFrom;
+
 import java.util.Map;
 import java.util.TreeMap;
 import java.util.function.BiFunction;
@@ -165,17 +167,22 @@ protected void deserializeStructure(GenerationContext context, StructureShape sh
                 String locationName = memberNameStrategy.apply(memberShape, memberName);
                 Shape target = context.getModel().expectShape(memberShape.getTarget());
 
+                String propertyAccess = getFrom("output", locationName);
+
                 if (usesExpect(target)) {
                     // Booleans and numbers will call expectBoolean/expectNumber which will handle
                     // null/undefined properly.
                     writer.write("$L: $L,",
                             memberName,
-                            target.accept(getMemberVisitor(memberShape, "output." + locationName)));
+                            target.accept(getMemberVisitor(memberShape, propertyAccess)));
                 } else {
-                    writer.write("$1L: (output.$2L !== undefined && output.$2L !== null)"
-                                    + " ? $3L: undefined,", memberName, locationName,
-                            // Dispatch to the output value provider for any additional handling.
-                            target.accept(getMemberVisitor(memberShape, "output." + locationName)));
+                    writer.write(
+                        "$1L: ($2L !== undefined && $2L !== null) ? $3L: undefined,",
+                        memberName,
+                        propertyAccess,
+                        // Dispatch to the output value provider for any additional handling.
+                        target.accept(getMemberVisitor(memberShape, propertyAccess))
+                    );
                 }
             });
         });
@@ -202,21 +209,25 @@ protected void deserializeUnion(GenerationContext context, UnionShape shape) {
             Shape target = model.expectShape(memberShape.getTarget());
             String locationName = memberNameStrategy.apply(memberShape, memberName);
 
-            String memberValue = target.accept(getMemberVisitor(memberShape, "output." + locationName));
+            String memberValue = target.accept(getMemberVisitor(memberShape, getFrom("output", locationName)));
             if (usesExpect(target)) {
                 // Booleans and numbers will call expectBoolean/expectNumber which will handle
                 // null/undefined properly.
                 writer.openBlock("if ($L !== undefined) {", "}", memberValue, () -> {
                     writer.write("return { $L: $L as any }", memberName, memberValue);
                 });
             } else {
-                writer.openBlock("if (output.$L !== undefined && output.$L !== null) {", "}", locationName,
-                    locationName, () -> {
-                        writer.openBlock("return {", "};", () -> {
+                writer.openBlock(
+                    "if ($1L !== undefined && $1L !== null) {", "}",
+                    getFrom("output", locationName),
+                    () -> writer.openBlock(
+                        "return {", "};",
+                        () -> {
                             // Dispatch to the output value provider for any additional handling.
                             writer.write("$L: $L", memberName, memberValue);
-                        });
-                    });
+                        }
+                    )
+                );
             }
         });
         // Or write to the unknown member the element in the output.

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

@@ -0,0 +1,52 @@
+/*
+ * Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License").
+ * You may not use this file except in compliance with the License.
+ * A copy of the License is located at
+ *
+ *  http://aws.amazon.com/apache2.0
+ *
+ * or in the "license" file accompanying this file. This file is distributed
+ * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+package software.amazon.smithy.aws.typescript.codegen.propertyaccess;
+
+import java.util.regex.Pattern;
+
+public final class PropertyAccessor {
+    /**
+     * Starts with alpha or underscore, and contains only alphanumeric and underscores.
+     */
+    public static final Pattern VALID_JAVASCRIPT_PROPERTY_NAME = Pattern.compile("^(?![0-9])[a-zA-Z0-9$_]+$");
+
+    private PropertyAccessor() {}
+
+    /**
+     * @param propertyName - property being accessed.
+     * @return brackets wrapping the name if it's not a valid JavaScript property name.
+     */
+    public static String getPropertyAccessor(String propertyName) {
+        if (VALID_JAVASCRIPT_PROPERTY_NAME.matcher(propertyName).matches()) {
+            return "." + propertyName;
+        }
+        if (propertyName.contains("\"")) {
+            // This doesn't handle cases of the special characters being pre-escaped in the propertyName,
+            // but that case does not currently need to be addressed.
+            return "[`" + propertyName + "`]";
+        }
+        return "[\"" + propertyName + "\"]";
+    }
+
+    /**
+     * @param variable - object host.
+     * @param propertyName - property being accessed.
+     * @return e.g. someObject.prop or someObject['property name'] or reluctantly someObject[`bad"property"name`].
+     */
+    public static String getFrom(String variable, String propertyName) {
+        return variable + getPropertyAccessor(propertyName);
+    }
+}

codegen/smithy-aws-typescript-codegen/src/test/java/software/amazon/smithy/aws/typescript/codegen/propertyaccess/PropertyAccessorTest.java

@@ -0,0 +1,24 @@
+package software.amazon.smithy.aws.typescript.codegen.propertyaccess;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+class PropertyAccessorTest {
+    @Test
+    void getFrom() {
+        assertEquals("output.fileSystemId", PropertyAccessor.getFrom("output", "fileSystemId"));
+        assertEquals("output.__fileSystemId", PropertyAccessor.getFrom("output", "__fileSystemId"));
+    }
+
+    @Test
+    void getFromQuoted() {
+        assertEquals("output[\"0fileSystemId\"]", PropertyAccessor.getFrom("output", "0fileSystemId"));
+        assertEquals("output[\"file-system-id\"]", PropertyAccessor.getFrom("output", "file-system-id"));
+    }
+
+    @Test
+    void getFromExtraQuoted() {
+        assertEquals("output[`file\"system\"id`]", PropertyAccessor.getFrom("output", "file\"system\"id"));
+    }
+}