Address comments

Author: haydenbaker

smithy-aws-cloudformation/src/main/java/software/amazon/smithy/aws/cloudformation/schema/model/Property.java

@@ -16,7 +16,9 @@
 package software.amazon.smithy.aws.cloudformation.schema.model;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
+import java.util.Optional;
 import software.amazon.smithy.jsonschema.Schema;
 import software.amazon.smithy.model.node.Node;
 import software.amazon.smithy.model.node.ObjectNode;
@@ -32,8 +34,6 @@
  * @see <a href="https://github.com/aws-cloudformation/cloudformation-cli/blob/master/src/rpdk/core/data/schema/provider.definition.schema.v1.jsonL74">Resource Type Properties JSON Schema</a>
  */
 public final class Property implements ToNode, ToSmithyBuilder<Property> {
-    private final boolean insertionOrder;
-    private final List<String> dependencies;
     private final Schema schema;
     // Other reserved property names in definition but not in the validation
     // JSON Schema, so not defined in code:
@@ -49,14 +49,12 @@ private Property(Builder builder) {
             schemaBuilder = builder.schema.toBuilder();
         }
 
-        this.insertionOrder = builder.insertionOrder;
-        if (this.insertionOrder) {
+        if (builder.insertionOrder) {
             schemaBuilder.putExtension("insertionOrder", Node.from(true));
         }
 
-        this.dependencies = builder.dependencies;
-        if (!this.dependencies.isEmpty()) {
-            schemaBuilder.putExtension("dependencies", Node.fromStrings(this.dependencies));
+        if (!builder.dependencies.isEmpty()) {
+            schemaBuilder.putExtension("dependencies", Node.fromStrings(builder.dependencies));
         }
 
         this.schema = schemaBuilder.build();
@@ -69,19 +67,12 @@ public Node toNode() {
 
     @Override
     public Builder toBuilder() {
-        return builder()
-                .insertionOrder(insertionOrder)
-                .dependencies(dependencies)
-                .schema(schema);
+        return builder().schema(schema);
     }
 
     public static Property fromNode(Node node) {
         ObjectNode objectNode = node.expectObjectNode();
         Builder builder = builder();
-
-        objectNode.getBooleanMember("insertionOrder", builder::insertionOrder);
-        objectNode.getArrayMember("dependencies", StringNode::getValue, builder::dependencies);
-
         builder.schema(Schema.fromNode(objectNode));
 
         return builder.build();
@@ -96,11 +87,17 @@ public static Builder builder() {
     }
 
     public boolean isInsertionOrder() {
-        return insertionOrder;
+        Optional<Boolean> insertionOrder = schema.getExtension("insertionOrder")
+                .map(n -> n.toNode().expectBooleanNode().getValue());
+
+        return insertionOrder.orElse(false);
     }
 
     public List<String> getDependencies() {
-        return dependencies;
+        Optional<List<String>> dependencies = schema.getExtension("dependencies")
+                .map(n -> n.toNode().expectArrayNode().getElementsAs(StringNode::getValue));
+
+        return dependencies.orElse(Collections.emptyList());
     }
 
     public Schema getSchema() {

smithy-aws-cloudformation/src/main/java/software/amazon/smithy/aws/cloudformation/schema/model/ResourceSchema.java

@@ -166,6 +166,11 @@ public Builder toBuilder() {
                 .tagging(tagging);
     }
 
+    public static ResourceSchema fromNode(Node node) {
+        NodeMapper mapper = new NodeMapper();
+        return mapper.deserializeInto(node, ResourceSchema.builder()).build();
+    }
+
     public static Builder builder() {
         return new Builder();
     }

smithy-aws-cloudformation/src/test/java/software/amazon/smithy/aws/cloudformation/schema/fromsmithy/ResourceSchemaTest.java

@@ -19,7 +19,6 @@
 import org.junit.jupiter.params.provider.MethodSource;
 import software.amazon.smithy.aws.cloudformation.schema.model.ResourceSchema;
 import software.amazon.smithy.model.node.Node;
-import software.amazon.smithy.model.node.NodeMapper;
 import software.amazon.smithy.utils.IoUtils;
 
 import java.io.IOException;
@@ -35,19 +34,18 @@ public class ResourceSchemaTest {
     @ParameterizedTest
     @MethodSource("resourceSchemaFiles")
     public void validateResourceSchemaFromNodeToNode(String resourceSchemaFile) {
-        NodeMapper mapper = new NodeMapper();
         String json = IoUtils.readUtf8File(resourceSchemaFile);
 
         Node node = Node.parse(json);
-        ResourceSchema schemaFromNode = mapper.deserialize(node, ResourceSchema.class);
+        ResourceSchema schemaFromNode = ResourceSchema.fromNode(node);
         Node nodeFromSchema = schemaFromNode.toNode();
 
-        Node.assertEquals(nodeFromSchema.withDeepSortedKeys(), node.withDeepSortedKeys());
+        Node.assertEquals(nodeFromSchema, node);
     }
 
     public static List<String> resourceSchemaFiles() {
         try {
-            Path definitionPath = Paths.get(ResourceSchemaTest.class.getResource("aws-sagemaker-domain.json").toURI());
+            Path definitionPath = Paths.get(ResourceSchemaTest.class.getResource("aws-sagemaker-domain.cfn.json").toURI());
 
             return Files.walk(Paths.get(definitionPath.getParent().toUri()))
                     .filter(Files::isRegularFile)

smithy-aws-cloudformation/src/test/resources/software/amazon/smithy/aws/cloudformation/schema/fromsmithy/aws-sagemaker-domain.json renamed to smithy-aws-cloudformation/src/test/resources/software/amazon/smithy/aws/cloudformation/schema/fromsmithy/aws-sagemaker-domain.cfn.json

@@ -499,8 +499,8 @@
     "/properties/DomainSettings/RStudioServerProDomainSettings/DefaultResourceSpec",
     "/properties/KmsKeyId",
     "/properties/SubnetIds",
-    "/properties/VpcId",
-    "/properties/Tags"
+    "/properties/Tags",
+    "/properties/VpcId"
   ],
   "writeOnlyProperties": [
     "/properties/Tags"
@@ -510,27 +510,27 @@
   ],
   "readOnlyProperties": [
     "/properties/DomainArn",
-    "/properties/Url",
     "/properties/DomainId",
     "/properties/HomeEfsFileSystemId",
     "/properties/SecurityGroupIdForDomainBoundary",
-    "/properties/SingleSignOnManagedApplicationInstanceId"
+    "/properties/SingleSignOnManagedApplicationInstanceId",
+    "/properties/Url"
   ],
   "handlers": {
     "create": {
       "permissions": [
-        "sagemaker:CreateApp",
-        "sagemaker:CreateDomain",
-        "sagemaker:DescribeDomain",
-        "sagemaker:DescribeImage",
-        "sagemaker:DescribeImageVersion",
+        "efs:CreateFileSystem",
         "iam:CreateServiceLinkedRole",
         "iam:PassRole",
-        "efs:CreateFileSystem",
         "kms:CreateGrant",
         "kms:Decrypt",
         "kms:DescribeKey",
-        "kms:GenerateDataKeyWithoutPlainText"
+        "kms:GenerateDataKeyWithoutPlainText",
+        "sagemaker:CreateApp",
+        "sagemaker:CreateDomain",
+        "sagemaker:DescribeDomain",
+        "sagemaker:DescribeImage",
+        "sagemaker:DescribeImageVersion"
       ]
     },
     "read": {
@@ -540,12 +540,12 @@
     },
     "update": {
       "permissions": [
+        "iam:PassRole",
         "sagemaker:CreateApp",
-        "sagemaker:UpdateDomain",
         "sagemaker:DescribeDomain",
         "sagemaker:DescribeImage",
         "sagemaker:DescribeImageVersion",
-        "iam:PassRole"
+        "sagemaker:UpdateDomain"
       ]
     },
     "delete": {