Fix CFN handler permissions for noReplace resources

smithy-aws-cloudformation/src/main/java/software/amazon/smithy/aws/cloudformation/schema/fromsmithy/mappers/HandlerPermissionMapper.java

@@ -67,10 +67,9 @@ public void before(Context context, ResourceSchema.Builder resourceSchema) {
                 .orElse(SetUtils.of());
         createPermissions.addAll(putPermissions);
         // Put operations without the noReplace trait are used for updates.
-        resource.getPut()
-                .map(model::expectShape)
-                .filter(shape -> !shape.hasTrait(NoReplaceTrait.class))
-                .ifPresent(shape -> updatePermissions.addAll(putPermissions));
+        if (!resource.hasTrait(NoReplaceTrait.class)) {
+            updatePermissions.addAll(putPermissions);
+        }
 
         // Set the create and update handlers, if they have permissions, now that they're complete.
         if (!createPermissions.isEmpty()) {

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

@@ -16,6 +16,7 @@
 package software.amazon.smithy.aws.cloudformation.schema.fromsmithy.mappers;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.util.List;
@@ -47,12 +48,8 @@ public void supportsExternalDocumentationUrls() {
         config.setOrganizationName("Smithy");
         config.setService(ShapeId.from("smithy.example#TestService"));
 
-        List<ResourceSchema> schemas = CfnConverter.create()
-                .config(config)
-                .convert(model);
-
-        assertEquals(1, schemas.size());
-        ResourceSchema schema = schemas.get(0);
+        ResourceSchema schema = getBarSchema(config);
+        assertNotNull(schema);
         assertTrue(schema.getDocumentationUrl().isPresent());
         assertEquals("https://docs.example.com", schema.getDocumentationUrl().get());
         assertTrue(schema.getSourceUrl().isPresent());
@@ -67,15 +64,20 @@ public void supportsCustomExternalDocNames() {
         config.setExternalDocs(ListUtils.of("main"));
         config.setSourceDocs(ListUtils.of("code"));
 
-        List<ResourceSchema> schemas = CfnConverter.create()
-                .config(config)
-                .convert(model);
-
-        assertEquals(1, schemas.size());
-        ResourceSchema schema = schemas.get(0);
+        ResourceSchema schema = getBarSchema(config);
+        assertNotNull(schema);
         assertTrue(schema.getDocumentationUrl().isPresent());
         assertEquals("https://docs2.example.com", schema.getDocumentationUrl().get());
         assertTrue(schema.getSourceUrl().isPresent());
         assertEquals("https://source2.example.com", schema.getSourceUrl().get());
     }
+
+    private ResourceSchema getBarSchema(CfnConfig config) {
+        for (ResourceSchema schema : CfnConverter.create().config(config).convert(model)) {
+            if (schema.getTypeName().endsWith("FooResource")) {
+                return schema;
+            }
+        }
+        return null;
+    }
 }

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

@@ -19,9 +19,11 @@
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.util.Map;
 import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 import software.amazon.smithy.aws.cloudformation.schema.CfnConfig;
 import software.amazon.smithy.aws.cloudformation.schema.fromsmithy.CfnConverter;
@@ -32,24 +34,30 @@
 import software.amazon.smithy.model.shapes.ShapeId;
 
 public final class HandlerPermissionMapperTest {
-    @Test
-    public void addsHandlerPermissionsByDefault() {
-        Model model = Model.assembler()
-                .addImport(HandlerPermissionMapperTest.class.getResource("simple.smithy"))
+
+    private static Model model;
+
+    @BeforeAll
+    public static void loadModel() {
+        model = Model.assembler()
+                .addImport(DocumentationMapperTest.class.getResource("simple.smithy"))
                 .discoverModels()
                 .assemble()
                 .unwrap();
+    }
 
+    private ObjectNode getResourceByName(String resourceName) {
         CfnConfig config = new CfnConfig();
         config.setOrganizationName("Smithy");
         config.setService(ShapeId.from("smithy.example#TestService"));
+        Map<String, ObjectNode> resourceNodes = CfnConverter.create().config(config).convertToNodes(model);
+        return resourceNodes.get(resourceName);
+    }
 
-        ObjectNode resourceNode = CfnConverter.create()
-                .config(config)
-                .convertToNodes(model)
-                .get("Smithy::TestService::FooResource");
-
-        Map<String, Node> handlersDefined = resourceNode.expectObjectMember("handlers").getStringMap();
+    @Test
+    public void addsCRUHandlerPermissionsByDefault() {
+        ObjectNode fooResourceNode = getResourceByName("Smithy::TestService::FooResource");
+        Map<String, Node> handlersDefined = fooResourceNode.expectObjectMember("handlers").getStringMap();
         Assertions.assertEquals(3, handlersDefined.size());
         assertThat(handlersDefined.keySet(), containsInAnyOrder("create", "read", "update"));
 
@@ -63,14 +71,36 @@ public void addsHandlerPermissionsByDefault() {
                            .expectArrayMember("permissions").getElementsAs(StringNode::getValue),
                 contains("testservice:UpdateFooOperation"));
     }
+
     @Test
-    public void canDisableHandlerPermissionsGeneration() {
-        Model model = Model.assembler()
-                .addImport(HandlerPermissionMapperTest.class.getResource("simple.smithy"))
-                .discoverModels()
-                .assemble()
-                .unwrap();
+    public void addsPutHandlerPermissionsByDefault() {
+        ObjectNode barResourceNode = getResourceByName("Smithy::TestService::BarResource");
+        Map<String, Node> handlersDefined = barResourceNode.expectObjectMember("handlers").getStringMap();
+        Assertions.assertEquals(2, handlersDefined.size());
+        assertThat(handlersDefined.keySet(), containsInAnyOrder("create", "update"));
 
+        assertThat(handlersDefined.get("create").expectObjectNode()
+                        .expectArrayMember("permissions").getElementsAs(StringNode::getValue),
+                contains("testservice:CreateBar"));
+        assertThat(handlersDefined.get("update").expectObjectNode()
+                        .expectArrayMember("permissions").getElementsAs(StringNode::getValue),
+                contains("testservice:CreateBar"));
+    }
+
+    @Test
+    public void addsPutWithNoReplaceHandlerPermissionsByDefault() {
+        ObjectNode bazResourceNode = getResourceByName("Smithy::TestService::BazResource");
+        Map<String, Node> handlersDefined = bazResourceNode.expectObjectMember("handlers").getStringMap();
+        Assertions.assertEquals(1, handlersDefined.size());
+        assertThat(handlersDefined.keySet(), contains("create"));
+
+        assertThat(handlersDefined.get("create").expectObjectNode()
+                        .expectArrayMember("permissions").getElementsAs(StringNode::getValue),
+                contains("testservice:CreateBaz"));
+    }
+
+    @Test
+    public void canDisableHandlerPermissionsGeneration() {
         CfnConfig config = new CfnConfig();
         config.setOrganizationName("Smithy");
         config.setService(ShapeId.from("smithy.example#TestService"));

smithy-aws-cloudformation/src/test/resources/software/amazon/smithy/aws/cloudformation/schema/fromsmithy/mappers/simple.smithy

@@ -9,6 +9,8 @@ service TestService {
     version: "2020-07-02",
     resources: [
         FooResource,
+        BarResource
+        BazResource
     ],
 }
 
@@ -115,3 +117,50 @@ structure ComplexProperty {
     property: String,
     another: String,
 }
+
+/// The Bar resource is cooler.
+@externalDocumentation(
+    "Documentation Url": "https://docs.example.com",
+    "Source Url": "https://source.example.com",
+    "Main": "https://docs2.example.com",
+    "Code": "https://source2.example.com",
+)
+@cfnResource
+resource BarResource {
+    identifiers: {
+        barId: String
+    }
+    put: CreateBar
+}
+
+@idempotent
+operation CreateBar {
+    input := for BarResource {
+        @required
+        $barId
+    }
+}
+
+/// The Baz resource is irreplaceable.
+@externalDocumentation(
+    "Documentation Url": "https://docs.example.com",
+    "Source Url": "https://source.example.com",
+    "Main": "https://docs2.example.com",
+    "Code": "https://source2.example.com",
+)
+@cfnResource
+@noReplace
+resource BazResource {
+    identifiers: {
+        bazId: String
+    }
+    put: CreateBaz
+}
+
+@idempotent
+operation CreateBaz {
+    input := for BazResource {
+        @required
+        $bazId
+    }
+}