Add primaryIdentifier to cfnResource trait

This commit adds the primaryIdentifier field to the cfnResource trait.
This supports service teams that have CFN support that deviates from
their APIs in their primary identifier, normally from a human readable
value to an ARN. This field is deprecated from the start, as it is not
the preferred path for supporting resource identifiers.

The value of the primaryIdentifier field, when set, MUST be the name of
a property defined on the resource shape and that property MUST be
defined as a string or enum shape.

Author: kstich

docs/source-2.0/aws/aws-cloudformation.rst

@@ -55,6 +55,11 @@ supports the following members:
         Members of these structures with the same names MUST resolve to the
         same target. See :ref:`aws-cloudformation-property-deriviation` for
         more information.
+    * - primaryIdentifier
+      - ``string``
+      - **Deprecated** An alternative resource property to use as the primary
+        identifier for the CloudFormation resource. The value MUST be the name
+        of a property on the resource shape that targets a string shape.
 
 The following example defines a simple resource that is also a CloudFormation
 resource:

smithy-aws-cloudformation-traits/src/main/java/software/amazon/smithy/aws/cloudformation/traits/CfnResourceIndex.java

@@ -174,6 +174,14 @@ public CfnResourceIndex(Model model) {
                                 });
                     }
 
+                    // If the primary identifier is rerouted, the derived identifiers are
+                    // additional identifiers for the resource.
+                    if (trait.getPrimaryIdentifier().isPresent()) {
+                        CfnResource tempResource = builder.build();
+                        builder.addAdditionalIdentifier(SetUtils.copyOf(tempResource.getPrimaryIdentifiers()));
+                        builder.primaryIdentifiers(SetUtils.of(trait.getPrimaryIdentifier().get()));
+                    }
+
                     resourceDefinitions.put(resourceId, builder.build());
                 });
     }

smithy-aws-cloudformation-traits/src/main/java/software/amazon/smithy/aws/cloudformation/traits/CfnResourcePropertyValidator.java

@@ -4,6 +4,8 @@
  */
 package software.amazon.smithy.aws.cloudformation.traits;
 
+import static java.lang.String.format;
+
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -12,6 +14,7 @@
 import java.util.TreeSet;
 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.shapes.ResourceShape;
+import software.amazon.smithy.model.shapes.Shape;
 import software.amazon.smithy.model.shapes.ShapeId;
 import software.amazon.smithy.model.validation.AbstractValidator;
 import software.amazon.smithy.model.validation.ValidationEvent;
@@ -27,10 +30,9 @@ public List<ValidationEvent> validate(Model model) {
         List<ValidationEvent> events = new ArrayList<>();
 
         CfnResourceIndex cfnResourceIndex = CfnResourceIndex.of(model);
-        model.shapes(ResourceShape.class)
-                .filter(shape -> shape.hasTrait(CfnResourceTrait.ID))
-                .map(shape -> validateResource(model, cfnResourceIndex, shape))
-                .forEach(events::addAll);
+        for (ResourceShape resource : model.getResourceShapesWithTrait(CfnResourceTrait.class)) {
+            events.addAll(validateResource(model, cfnResourceIndex, resource));
+        }
 
         return events;
     }
@@ -44,17 +46,48 @@ private List<ValidationEvent> validateResource(
         List<ValidationEvent> events = new ArrayList<>();
         String resourceName = trait.getName().orElse(resource.getId().getName());
 
-        cfnResourceIndex.getResource(resource)
-                .map(CfnResource::getProperties)
-                .ifPresent(properties -> {
-                    for (Map.Entry<String, CfnResourceProperty> property : properties.entrySet()) {
-                        validateResourceProperty(model, resource, resourceName, property).ifPresent(events::add);
-                    }
-                });
+        if (trait.getPrimaryIdentifier().isPresent()) {
+            validateResourcePrimaryIdentifier(model, resource, trait.getPrimaryIdentifier().get())
+                    .ifPresent(events::add);
+        }
+
+        Optional<CfnResource> cfnResourceOptional = cfnResourceIndex.getResource(resource);
+        if (cfnResourceOptional.isPresent()) {
+            for (Map.Entry<String, CfnResourceProperty> property : cfnResourceOptional.get()
+                    .getProperties()
+                    .entrySet()) {
+                validateResourceProperty(model, resource, resourceName, property).ifPresent(events::add);
+            }
+        }
 
         return events;
     }
 
+    private Optional<ValidationEvent> validateResourcePrimaryIdentifier(
+            Model model,
+            ResourceShape resource,
+            String primaryIdentifier
+    ) {
+        Map<String, ShapeId> properties = resource.getProperties();
+        if (!properties.containsKey(primaryIdentifier)) {
+            return Optional.of(error(resource,
+                    resource.expectTrait(CfnResourceTrait.class),
+                    format("The alternative resource primary identifier, `%s`, must be a property of the resource.",
+                            primaryIdentifier)));
+        }
+
+        Shape propertyTarget = model.expectShape(properties.get(primaryIdentifier));
+        if (!propertyTarget.isStringShape() && !propertyTarget.isEnumShape()) {
+            return Optional.of(error(resource,
+                    resource.expectTrait(CfnResourceTrait.class),
+                    format("The alternative resource primary identifier, `%s`, targets a `%s` shape, it must target a `string`.",
+                            primaryIdentifier,
+                            propertyTarget.getType())));
+        }
+
+        return Optional.empty();
+    }
+
     private Optional<ValidationEvent> validateResourceProperty(
             Model model,
             ResourceShape resource,
@@ -72,7 +105,7 @@ private Optional<ValidationEvent> validateResourceProperty(
 
         if (propertyTargets.size() > 1) {
             return Optional.of(error(resource,
-                    String.format("The `%s` property of the generated `%s` "
+                    format("The `%s` property of the generated `%s` "
                             + "CloudFormation resource targets multiple shapes: %s. Reusing member names that "
                             + "target different shapes can cause confusion for users of the API. This target "
                             + "discrepancy must either be resolved in the model or one of the members must be "

smithy-aws-cloudformation-traits/src/main/java/software/amazon/smithy/aws/cloudformation/traits/CfnResourceTrait.java

@@ -25,11 +25,13 @@ public final class CfnResourceTrait extends AbstractTrait
 
     private final String name;
     private final List<ShapeId> additionalSchemas;
+    private final String primaryIdentifier;
 
     private CfnResourceTrait(Builder builder) {
         super(ID, builder.getSourceLocation());
         name = builder.name;
         additionalSchemas = ListUtils.copyOf(builder.additionalSchemas);
+        primaryIdentifier = builder.primaryIdentifier;
     }
 
     /**
@@ -50,6 +52,15 @@ public List<ShapeId> getAdditionalSchemas() {
         return additionalSchemas;
     }
 
+    /**
+     * Gets the alternative resource property to use as the primary identifier for the CloudFormation resource.
+     *
+     * @return Returns the optional alternative primary identifier.
+     */
+    public Optional<String> getPrimaryIdentifier() {
+        return Optional.ofNullable(primaryIdentifier);
+    }
+
     public static Builder builder() {
         return new Builder();
     }
@@ -83,6 +94,7 @@ public Trait createTrait(ShapeId target, Node value) {
     public static final class Builder extends AbstractTraitBuilder<CfnResourceTrait, Builder> {
         private String name;
         private final List<ShapeId> additionalSchemas = new ArrayList<>();
+        private String primaryIdentifier;
 
         private Builder() {}
 
@@ -106,5 +118,10 @@ public Builder additionalSchemas(List<ShapeId> additionalSchemas) {
             this.additionalSchemas.addAll(additionalSchemas);
             return this;
         }
+
+        public Builder primaryIdentifier(String primaryIdentifier) {
+            this.primaryIdentifier = primaryIdentifier;
+            return this;
+        }
     }
 }

smithy-aws-cloudformation-traits/src/main/resources/META-INF/smithy/aws.cloudformation.smithy

@@ -6,8 +6,8 @@ namespace aws.cloudformation
 /// additional identifier for the resource.
 @unstable
 @trait(
-    selector: "structure > :test(member > string)",
-    conflicts: [cfnExcludeProperty],
+    selector: "structure > :test(member > string)"
+    conflicts: [cfnExcludeProperty]
     breakingChanges: [{change: "remove"}]
 )
 structure cfnAdditionalIdentifier {}
@@ -16,7 +16,7 @@ structure cfnAdditionalIdentifier {}
 /// to differ from a structure member name used in the model.
 @unstable
 @trait(
-    selector: "structure > member",
+    selector: "structure > member"
     breakingChanges: [{change: "any"}]
 )
 string cfnName
@@ -25,12 +25,12 @@ string cfnName
 /// CloudFormation resource definitions.
 @unstable
 @trait(
-    selector: "structure > member",
+    selector: "structure > member"
     conflicts: [
-        cfnAdditionalIdentifier,
-        cfnMutability,
+        cfnAdditionalIdentifier
+        cfnMutability
         cfnDefaultValue
-    ],
+    ]
     breakingChanges: [{change: "add"}]
 )
 structure cfnExcludeProperty {}
@@ -39,7 +39,7 @@ structure cfnExcludeProperty {}
 /// for the property of the CloudFormation resource.
 @unstable
 @trait(
-    selector: "resource > operation -[input, output]-> structure > member",
+    selector: "resource > operation -[input, output]-> structure > member"
     conflicts: [cfnExcludeProperty]
 )
 structure cfnDefaultValue {}
@@ -48,7 +48,7 @@ structure cfnDefaultValue {}
 /// when part of a CloudFormation resource.
 @unstable
 @trait(
-    selector: "structure > member",
+    selector: "structure > member"
     conflicts: [cfnExcludeProperty]
 )
 enum cfnMutability {
@@ -86,16 +86,22 @@ enum cfnMutability {
 /// Indicates that a Smithy resource is a CloudFormation resource.
 @unstable
 @trait(
-    selector: "resource",
+    selector: "resource"
     breakingChanges: [{change: "presence"}]
 )
 structure cfnResource {
     /// Provides a custom CloudFormation resource name.
-    name: String,
+    name: String
 
     /// A list of additional shape IDs of structures that will have their
     /// properties added to the CloudFormation resource.
-    additionalSchemas: StructureIdList,
+    additionalSchemas: StructureIdList
+
+    /// An alternative resource property to use as the primary identifier
+    /// for the CloudFormation resource. The value MUST be the name of a
+    /// property on the resource shape that targets a string shape.
+    @deprecated(message: "Prefer the resource's identifiers when generating resource schemas.")
+    primaryIdentifier: String
 }
 
 @private

smithy-aws-cloudformation-traits/src/test/java/software/amazon/smithy/aws/cloudformation/traits/CfnResourceIndexTest.java

@@ -28,6 +28,7 @@ public class CfnResourceIndexTest {
     private static final ShapeId FOO = ShapeId.from("smithy.example#FooResource");
     private static final ShapeId BAR = ShapeId.from("smithy.example#BarResource");
     private static final ShapeId BAZ = ShapeId.from("smithy.example#BazResource");
+    private static final ShapeId TAD = ShapeId.from("smithy.example#TadResource");
 
     private static Model model;
     private static CfnResourceIndex cfnResourceIndex;
@@ -118,7 +119,20 @@ public static Collection<ResourceData> data() {
         bazResource.readOnlyProperties = SetUtils.of("bazId", "bazImplicitReadProperty");
         bazResource.writeOnlyProperties = SetUtils.of("bazImplicitWriteProperty");
 
-        return ListUtils.of(fooResource, barResource, bazResource);
+        ResourceData tadResource = new ResourceData();
+        tadResource.resourceId = TAD;
+        tadResource.identifiers = SetUtils.of("tadArn");
+        tadResource.additionalIdentifiers = ListUtils.of(SetUtils.of("tadId"));
+        tadResource.mutabilities = MapUtils.of(
+                "tadId",
+                SetUtils.of(Mutability.READ),
+                "tadArn",
+                SetUtils.of(Mutability.READ));
+        tadResource.createOnlyProperties = SetUtils.of();
+        tadResource.readOnlyProperties = SetUtils.of("tadId", "tadArn");
+        tadResource.writeOnlyProperties = SetUtils.of();
+
+        return ListUtils.of(fooResource, tadResource, bazResource, tadResource);
     }
 
     @ParameterizedTest

smithy-aws-cloudformation-traits/src/test/java/software/amazon/smithy/aws/cloudformation/traits/CfnResourceTraitTest.java

@@ -7,6 +7,7 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.equalTo;
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -36,6 +37,14 @@ public void loadsFromModel() {
         assertThat(barTrait.getName().get(), equalTo("CustomResource"));
         assertFalse(barTrait.getAdditionalSchemas().isEmpty());
         assertThat(barTrait.getAdditionalSchemas(), contains(ShapeId.from("smithy.example#ExtraBarRequest")));
+
+        Shape tadResource = result.expectShape(ShapeId.from("smithy.example#TadResource"));
+        assertTrue(tadResource.hasTrait(CfnResourceTrait.class));
+        CfnResourceTrait tadTrait = tadResource.expectTrait(CfnResourceTrait.class);
+        assertFalse(tadTrait.getName().isPresent());
+        assertTrue(tadTrait.getAdditionalSchemas().isEmpty());
+        assertTrue(tadTrait.getPrimaryIdentifier().isPresent());
+        assertEquals("tadArn", tadTrait.getPrimaryIdentifier().get());
     }
 
     @Test

smithy-aws-cloudformation-traits/src/test/resources/software/amazon/smithy/aws/cloudformation/traits/cfn-resources.smithy

@@ -12,23 +12,57 @@ resource FooResource {
 }
 
 @cfnResource(
-    name: "CustomResource",
+    name: "CustomResource"
     additionalSchemas: [ExtraBarRequest]
 )
 resource BarResource {
     identifiers: {
         barId: BarId
-    },
-    operations: [ExtraBarOperation],
+    }
+    operations: [ExtraBarOperation]
+}
+
+@cfnResource(primaryIdentifier: "tadArn")
+resource TadResource {
+    identifiers: {
+        tadId: String
+    }
+    properties: {
+        tadArn: String
+    }
+    create: CreateTad
+    read: GetTad
+}
+
+operation CreateTad {
+    input := {}
+    output := {
+        @required
+        tadId: String
+        tadArn: String
+    }
+}
+
+@readonly
+operation GetTad {
+    input := {
+        @required
+        tadId: String
+    }
+    output := {
+        @required
+        tadId: String
+        tadArn: String
+    }
 }
 
 operation ExtraBarOperation {
-    input: ExtraBarRequest,
+    input: ExtraBarRequest
 }
 
 structure ExtraBarRequest {
     @required
-    barId: BarId,
+    barId: BarId
 }
 
 string FooId

smithy-aws-cloudformation-traits/src/test/resources/software/amazon/smithy/aws/cloudformation/traits/errorfiles/primary-identifier-swap.errors

@@ -0,0 +1,5 @@
+[ERROR] smithy.example#BarResource: The alternative resource primary identifier, `barArn`, must be a property of the resource. | CfnResourceProperty
+[ERROR] smithy.example#BazResource: The alternative resource primary identifier, `bazArn`, targets a `integer` shape, it must target a `string`. | CfnResourceProperty
+[WARNING] smithy.example#FooResource: This shape applies a trait that is unstable: aws.cloudformation#cfnResource | UnstableTrait.aws.cloudformation#cfnResource
+[WARNING] smithy.example#BarResource: This shape applies a trait that is unstable: aws.cloudformation#cfnResource | UnstableTrait.aws.cloudformation#cfnResource
+[WARNING] smithy.example#BazResource: This shape applies a trait that is unstable: aws.cloudformation#cfnResource | UnstableTrait.aws.cloudformation#cfnResource