Fix several bugs in AwsTagIndex

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

@@ -44,7 +44,6 @@
 import software.amazon.smithy.model.knowledge.TopDownIndex;
 import software.amazon.smithy.model.node.ObjectNode;
 import software.amazon.smithy.model.shapes.MemberShape;
-import software.amazon.smithy.model.shapes.OperationShape;
 import software.amazon.smithy.model.shapes.ResourceShape;
 import software.amazon.smithy.model.shapes.ServiceShape;
 import software.amazon.smithy.model.shapes.Shape;
@@ -425,11 +424,10 @@ private void injectTagsIfNecessary(
                     ShapeId definition = resource.getProperties().get(trait.getProperty().get());
                     builder.addMember(tagPropertyName, definition);
                 } else {
-                    // Taggability must be through service-wide TagResource operation
-                    OperationShape tagResourceOp = model.expectShape(
-                        tagIndex.getTagResourceOperation(resource.getId()).get(), OperationShape.class);
-                    // A valid TagResource operation certainly has a single tags input member
-                    MemberShape member = AwsTagIndex.getTagsMember(model, tagResourceOp).get();
+                    // A valid TagResource operation certainly has a single tags input member.
+                    AwsTagIndex awsTagIndex = AwsTagIndex.of(model);
+                    Optional<ShapeId> tagOperation = tagIndex.getTagResourceOperation(resource.getId());
+                    MemberShape member = awsTagIndex.getTagsMember(tagOperation.get()).get();
                     member = member.toBuilder().id(builder.getId().withMember(tagPropertyName)).build();
                     builder.addMember(member);
                 }

smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/ServiceTaggingValidator.java

@@ -15,11 +15,15 @@
 
 package software.amazon.smithy.aws.traits.tagging;
 
+import static software.amazon.smithy.aws.traits.tagging.TaggingShapeUtils.LIST_TAGS_OPNAME;
+import static software.amazon.smithy.aws.traits.tagging.TaggingShapeUtils.TAG_RESOURCE_OPNAME;
+import static software.amazon.smithy.aws.traits.tagging.TaggingShapeUtils.UNTAG_RESOURCE_OPNAME;
+
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Optional;
+import software.amazon.smithy.model.FromSourceLocation;
 import software.amazon.smithy.model.Model;
-import software.amazon.smithy.model.SourceLocation;
 import software.amazon.smithy.model.shapes.ServiceShape;
 import software.amazon.smithy.model.shapes.ShapeId;
 import software.amazon.smithy.model.validation.AbstractValidator;
@@ -29,69 +33,58 @@
  * Validates service satisfies AWS tagging requirements.
  */
 public final class ServiceTaggingValidator extends AbstractValidator {
-    private static final String TAG_RESOURCE_OPNAME = "TagResource";
-    private static final String UNTAG_RESOURCE_OPNAME = "UntagResource";
-    private static final String LISTTAGS_OPNAME = "ListTagsForResource";
-
     @Override
     public List<ValidationEvent> validate(Model model) {
         AwsTagIndex awsTagIndex = AwsTagIndex.of(model);
         List<ValidationEvent> events = new LinkedList<>();
         for (ServiceShape service : model.getServiceShapesWithTrait(TagEnabledTrait.class)) {
-            events.addAll(validateService(model, service, awsTagIndex));
+            events.addAll(validateService(service, awsTagIndex));
         }
         return events;
     }
 
-    private List<ValidationEvent> validateService(Model model, ServiceShape service, AwsTagIndex awsTagIndex) {
+    private List<ValidationEvent> validateService(ServiceShape service, AwsTagIndex awsTagIndex) {
         List<ValidationEvent> events = new LinkedList<>();
-        SourceLocation tagEnabledTraitLoc = service.expectTrait(TagEnabledTrait.class).getSourceLocation();
+        TagEnabledTrait trait = service.expectTrait(TagEnabledTrait.class);
 
         Optional<ShapeId> tagResourceId = awsTagIndex.getTagResourceOperation(service.getId());
         if (tagResourceId.isPresent()) {
             if (!awsTagIndex.serviceHasValidTagResourceOperation(service.getId())) {
-                events.add(getMessageUnqualifedOperation(service, tagEnabledTraitLoc,
-                                                tagResourceId.get(), TAG_RESOURCE_OPNAME));
+                events.add(getInvalidOperationEvent(service, trait, tagResourceId.get(), TAG_RESOURCE_OPNAME));
             }
         } else {
-            events.add(getMessageMissingOperation(service, tagEnabledTraitLoc, TAG_RESOURCE_OPNAME));
+            events.add(getMissingOperationEvent(service, trait, TAG_RESOURCE_OPNAME));
         }
 
         Optional<ShapeId> untagResourceId = awsTagIndex.getUntagResourceOperation(service.getId());
         if (untagResourceId.isPresent()) {
             if (!awsTagIndex.serviceHasValidUntagResourceOperation(service.getId())) {
-                events.add(getMessageUnqualifedOperation(service, tagEnabledTraitLoc,
-                                                untagResourceId.get(), UNTAG_RESOURCE_OPNAME));
+                events.add(getInvalidOperationEvent(service, trait, untagResourceId.get(), UNTAG_RESOURCE_OPNAME));
             }
         } else {
-            events.add(getMessageMissingOperation(service, tagEnabledTraitLoc, UNTAG_RESOURCE_OPNAME));
+            events.add(getMissingOperationEvent(service, trait, UNTAG_RESOURCE_OPNAME));
         }
 
-        Optional<ShapeId> listTagResourceId = awsTagIndex.getListTagsForResourceOperation(service.getId());
-        if (listTagResourceId.isPresent()) {
+        Optional<ShapeId> listTagsId = awsTagIndex.getListTagsForResourceOperation(service.getId());
+        if (listTagsId.isPresent()) {
             if (!awsTagIndex.serviceHasValidListTagsForResourceOperation(service.getId())) {
-                events.add(getMessageUnqualifedOperation(service, tagEnabledTraitLoc,
-                                                listTagResourceId.get(), LISTTAGS_OPNAME));
+                events.add(getInvalidOperationEvent(service, trait, listTagsId.get(), LIST_TAGS_OPNAME));
             }
         } else {
-            events.add(getMessageMissingOperation(service, tagEnabledTraitLoc, LISTTAGS_OPNAME));
+            events.add(getMissingOperationEvent(service, trait, LIST_TAGS_OPNAME));
         }
 
         return events;
     }
 
-    private ValidationEvent getMessageMissingOperation(
-        ServiceShape service,
-        SourceLocation location,
-        String opName
-    ) {
+    private ValidationEvent getMissingOperationEvent(ServiceShape service, FromSourceLocation location, String opName) {
         return warning(service, location, "Service marked `aws.api#TagEnabled` is missing an operation named "
-                                            + "'" + opName + ".'");
+                + "'" + opName + ".'");
     }
 
-    private ValidationEvent getMessageUnqualifedOperation(
+    private ValidationEvent getInvalidOperationEvent(
         ServiceShape service,
-        SourceLocation location,
+        FromSourceLocation location,
         ShapeId opId,
         String opName
     ) {

smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/TagEnabledServiceValidator.java

@@ -34,13 +34,12 @@ public List<ValidationEvent> validate(Model model) {
         TopDownIndex topDownIndex = TopDownIndex.of(model);
         AwsTagIndex tagIndex = AwsTagIndex.of(model);
         for (ServiceShape service : model.getServiceShapesWithTrait(TagEnabledTrait.class)) {
-            events.addAll(validateService(model, service, tagIndex, topDownIndex));
+            events.addAll(validateService(service, tagIndex, topDownIndex));
         }
         return events;
     }
 
     private List<ValidationEvent> validateService(
-            Model model,
             ServiceShape service,
             AwsTagIndex tagIndex,
             TopDownIndex topDownIndex

smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/TagResourcePropertyNameValidator.java

@@ -32,9 +32,7 @@ public List<ValidationEvent> validate(Model model) {
 
         for (ResourceShape resource : model.getResourceShapesWithTrait(TaggableTrait.class)) {
             TaggableTrait trait = resource.expectTrait(TaggableTrait.class);
-            if (trait.getProperty()
-                    .filter(property -> !TaggingShapeUtils.isTagDesiredName(property))
-                    .isPresent()) {
+            if (trait.getProperty().isPresent() && !TaggingShapeUtils.isTagDesiredName(trait.getProperty().get())) {
                 events.add(warning(resource, String.format("Suggested tag property name is '%s'.",
                     TaggingShapeUtils.getDesiredTagsPropertyName())));
             }

smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/TagResourcePropertyTypeValidator.java

@@ -36,14 +36,11 @@ public List<ValidationEvent> validate(Model model) {
         for (ResourceShape resource : model.getResourceShapesWithTrait(TaggableTrait.class)) {
             TaggableTrait trait = resource.expectTrait(TaggableTrait.class);
             Map<String, ShapeId> properties = resource.getProperties();
-            if (trait.getProperty().isPresent()) {
-                ShapeId propertyShapeId = properties.get(trait.getProperty().get());
-                if (propertyShapeId != null) {
-                    Shape propertyShape = model.expectShape(propertyShapeId);
-                    if (!TaggingShapeUtils.verifyTagsShape(model, propertyShape)) {
-                        events.add(error(resource, "Tag property must be a list shape targeting a member"
-                                + " containing a pair of strings, or a Map shape targeting a string member."));
-                    }
+            if (trait.getProperty().isPresent() && properties.containsKey(trait.getProperty().get())) {
+                Shape propertyShape = model.expectShape(properties.get(trait.getProperty().get()));
+                if (!TaggingShapeUtils.verifyTagsShape(model, propertyShape)) {
+                    events.add(error(resource, "Tag property must be a list shape targeting a member"
+                            + " containing a pair of strings, or a Map shape targeting a string member."));
                 }
             }
         }

smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/TaggableResourceValidator.java

@@ -24,7 +24,6 @@
 import java.util.function.Predicate;
 import software.amazon.smithy.aws.traits.ArnTrait;
 import software.amazon.smithy.model.Model;
-import software.amazon.smithy.model.knowledge.PropertyBindingIndex;
 import software.amazon.smithy.model.knowledge.TopDownIndex;
 import software.amazon.smithy.model.shapes.MemberShape;
 import software.amazon.smithy.model.shapes.OperationShape;
@@ -44,11 +43,10 @@ public List<ValidationEvent> validate(Model model) {
         List<ValidationEvent> events = new LinkedList<>();
         TopDownIndex topDownIndex = TopDownIndex.of(model);
         AwsTagIndex tagIndex = AwsTagIndex.of(model);
-        PropertyBindingIndex propertyBindingIndex = PropertyBindingIndex.of(model);
         for (ServiceShape service : model.getServiceShapesWithTrait(TagEnabledTrait.class)) {
             for (ResourceShape resource : topDownIndex.getContainedResources(service)) {
                 if (resource.hasTrait(TaggableTrait.class)) {
-                    events.addAll(validateResource(model, resource, service, tagIndex, propertyBindingIndex));
+                    events.addAll(validateResource(model, resource, service, tagIndex));
                 } else if (resource.hasTrait(ArnTrait.class) && tagIndex.serviceHasTagApis(service.getId())) {
                     // If a resource does not have the taggable trait, but has an ARN, and the service has tag
                     // operations, it is most likely a mistake.
@@ -63,14 +61,15 @@ private List<ValidationEvent> validateResource(
             Model model,
             ResourceShape resource,
             ServiceShape service,
-            AwsTagIndex awsTagIndex,
-            PropertyBindingIndex propertyBindingIndex
+            AwsTagIndex awsTagIndex
     ) {
         List<ValidationEvent> events = new LinkedList<>();
         // Generate danger if resource has tag property in update API.
         if (awsTagIndex.isResourceTagOnUpdate(resource.getId())) {
-            Shape updateOperation = model.expectShape(resource.getUpdate().get());
-            events.add(danger(updateOperation, "Update resource lifecycle operation should not support updating tags"
+            Shape operation = resource.getUpdate().isPresent()
+                    ? model.expectShape(resource.getUpdate().get())
+                    : model.expectShape(resource.getPut().get());
+            events.add(danger(operation, "Update and put resource lifecycle operations should not support updating tags"
                     + " because it is a privileged operation that modifies access."));
         }
         // A valid taggable resource must support one of the following:
@@ -81,8 +80,7 @@ private List<ValidationEvent> validateResource(
         //    through the tag property, and must be resource instance operations
         //Caution: avoid short circuiting behavior.
         boolean isServiceWideTaggable = awsTagIndex.serviceHasTagApis(service.getId());
-        boolean isInstanceOpTaggable = isTaggableViaInstanceOperations(events, model, resource, service,
-                propertyBindingIndex);
+        boolean isInstanceOpTaggable = isTaggableViaInstanceOperations(model, resource);
 
         if (isServiceWideTaggable && !isInstanceOpTaggable && !resource.hasTrait(ArnTrait.class)) {
             events.add(error(resource, "Resource is taggable only via service-wide tag operations."
@@ -98,16 +96,10 @@ private List<ValidationEvent> validateResource(
     }
 
     private Optional<OperationShape> resolveTagOperation(ShapeId tagApiId, Model model) {
-        return model.getShape(tagApiId).flatMap(shape -> shape.asOperationShape());
+        return model.getShape(tagApiId).flatMap(Shape::asOperationShape);
     }
 
-    private boolean isTaggableViaInstanceOperations(
-            List<ValidationEvent> events,
-            Model model,
-            ResourceShape resource,
-            ServiceShape service,
-            PropertyBindingIndex propertyBindingIndex
-    ) {
+    private boolean isTaggableViaInstanceOperations(Model model, ResourceShape resource) {
         TaggableTrait taggableTrait = resource.expectTrait(TaggableTrait.class);
         if (taggableTrait.getApiConfig().isPresent()) {
             TaggableApiConfig apiConfig = taggableTrait.getApiConfig().get();
@@ -117,62 +109,46 @@ private boolean isTaggableViaInstanceOperations(
 
             Optional<OperationShape> tagApi = resolveTagOperation(apiConfig.getTagApi(), model);
             if (tagApi.isPresent()) {
-                tagApiVerified = TaggingShapeUtils.isTagPropertyInInput(Optional.of(
-                        tagApi.get().getId()), model, resource, propertyBindingIndex)
-                        && verifyTagApi(tagApi.get(), model, service, resource);
+                tagApiVerified = TaggingShapeUtils.isTagPropertyInInput(
+                        Optional.of(tagApi.get().getId()), model, resource)
+                        && verifyTagApi(tagApi.get(), model);
             }
 
             Optional<OperationShape> untagApi = resolveTagOperation(apiConfig.getUntagApi(), model);
             if (untagApi.isPresent()) {
-                untagApiVerified = verifyUntagApi(untagApi.get(), model, service, resource);
+                untagApiVerified = verifyUntagApi(untagApi.get(), model);
             }
 
-
             Optional<OperationShape> listTagsApi = resolveTagOperation(apiConfig.getListTagsApi(), model);
             if (listTagsApi.isPresent()) {
-                listTagsApiVerified = verifyListTagsApi(listTagsApi.get(), model, service, resource);
+                listTagsApiVerified = verifyListTagsApi(listTagsApi.get(), model);
             }
 
             return tagApiVerified && untagApiVerified && listTagsApiVerified;
         }
         return false;
     }
 
-    private boolean verifyListTagsApi(
-        OperationShape listTagsApi,
-        Model model,
-        ServiceShape service,
-        ResourceShape resource
-    ) {
+    private boolean verifyListTagsApi(OperationShape listTagsApi, Model model) {
         // Verify Tags map or list member but on the output.
         return exactlyOne(collectMemberTargetShapes(listTagsApi.getOutputShape(), model),
                 memberEntry -> TaggingShapeUtils.isTagDesiredName(memberEntry.getKey().getMemberName())
-                && TaggingShapeUtils.verifyTagsShape(model, memberEntry.getValue()));
+                        && TaggingShapeUtils.verifyTagsShape(model, memberEntry.getValue()));
     }
 
-    private boolean verifyUntagApi(
-        OperationShape untagApi,
-        Model model,
-        ServiceShape service,
-        ResourceShape resource
-    ) {
-        // Tag API has a tags property on its input AND has exactly one member targetting a tag shape with an
+    private boolean verifyUntagApi(OperationShape untagApi, Model model) {
+        // Tag API has a tags property on its input AND has exactly one member targeting a tag shape with an
         // appropriate name.
         return exactlyOne(collectMemberTargetShapes(untagApi.getInputShape(), model),
                 memberEntry -> TaggingShapeUtils.isTagKeysDesiredName(memberEntry.getKey().getMemberName())
-                && TaggingShapeUtils.verifyTagKeysShape(model, memberEntry.getValue()));
+                        && TaggingShapeUtils.verifyTagKeysShape(model, memberEntry.getValue()));
     }
 
-    private boolean verifyTagApi(
-        OperationShape tagApi,
-        Model model,
-        ServiceShape service,
-        ResourceShape resource
-    ) {
-        // Tag API has exactly one member targetting a tag list or map shape with an appropriate name.
+    private boolean verifyTagApi(OperationShape tagApi, Model model) {
+        // Tag API has exactly one member targeting a tag list or map shape with an appropriate name.
         return exactlyOne(collectMemberTargetShapes(tagApi.getInputShape(), model),
                 memberEntry -> TaggingShapeUtils.isTagDesiredName(memberEntry.getKey().getMemberName())
-                && TaggingShapeUtils.verifyTagsShape(model, memberEntry.getValue()));
+                        && TaggingShapeUtils.verifyTagsShape(model, memberEntry.getValue()));
     }
 
     private boolean exactlyOne(