Repository: smithy-lang/smithy
Pull request #2483 (2024-12-06 04:02:33)

Add service id to the error message to aid debugging
#### Background

When a resource is in the closure of more than one service the `TaggableResource` error won't say for which one it failed, e.g.,

```
[ERROR] example.weather#City: Resource does not have tagging CRUD operations and is not compatible with service-wide tagging operations. | TaggableResource 
```

This change add the service to better understand where the issue is.

```
[ERROR] example.weather#City: Resource does not have tagging CRUD operations and is not compatible with service-wide tagging operations for service `example.weather#Weather`.
```

While there I also made minor changes to improve the code around.

#### Testing

* Added tests for this case

#### Links
* Links to additional context, if necessary
* Issue #, if applicable (see [here](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword) for a list of keywords to use for linking issues)

---
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.



Patch:
diff --git a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/AwsTagIndex.java b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/AwsTagIndex.java
index 0ec02cb01..8ed708a77 100644
--- a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/AwsTagIndex.java
+++ b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/AwsTagIndex.java
@@ -83,7 +83,7 @@ public final class AwsTagIndex implements KnowledgeIndex {
     }
 
     public static AwsTagIndex of(Model model) {
-        return new AwsTagIndex(model);
+        return model.getKnowledge(AwsTagIndex.class, AwsTagIndex::new);
     }
 
     /**
diff --git a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/ServiceTaggingValidator.java b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/ServiceTaggingValidator.java
index 9d08b7373..abcddb549 100644
--- a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/ServiceTaggingValidator.java
+++ b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/ServiceTaggingValidator.java
@@ -19,7 +19,7 @@ import static software.amazon.smithy.aws.traits.tagging.TaggingShapeUtils.LIST_T
 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.ArrayList;
 import java.util.List;
 import java.util.Optional;
 import software.amazon.smithy.model.FromSourceLocation;
@@ -36,7 +36,7 @@ public final class ServiceTaggingValidator extends AbstractValidator {
     @Override
     public List<ValidationEvent> validate(Model model) {
         AwsTagIndex awsTagIndex = AwsTagIndex.of(model);
-        List<ValidationEvent> events = new LinkedList<>();
+        List<ValidationEvent> events = new ArrayList<>();
         for (ServiceShape service : model.getServiceShapesWithTrait(TagEnabledTrait.class)) {
             events.addAll(validateService(service, awsTagIndex));
         }
@@ -44,7 +44,7 @@ public final class ServiceTaggingValidator extends AbstractValidator {
     }
 
     private List<ValidationEvent> validateService(ServiceShape service, AwsTagIndex awsTagIndex) {
-        List<ValidationEvent> events = new LinkedList<>();
+        List<ValidationEvent> events = new ArrayList<>();
         TagEnabledTrait trait = service.expectTrait(TagEnabledTrait.class);
 
         Optional<ShapeId> tagResourceId = awsTagIndex.getTagResourceOperation(service.getId());
diff --git a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/TagEnabledServiceValidator.java b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/TagEnabledServiceValidator.java
index 5984d14e4..e29059ed0 100644
--- a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/TagEnabledServiceValidator.java
+++ b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/TagEnabledServiceValidator.java
@@ -15,7 +15,7 @@
 
 package software.amazon.smithy.aws.traits.tagging;
 
-import java.util.LinkedList;
+import java.util.ArrayList;
 import java.util.List;
 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.knowledge.TopDownIndex;
@@ -30,7 +30,7 @@ import software.amazon.smithy.model.validation.ValidationEvent;
 public final class TagEnabledServiceValidator extends AbstractValidator {
     @Override
     public List<ValidationEvent> validate(Model model) {
-        List<ValidationEvent> events = new LinkedList<>();
+        List<ValidationEvent> events = new ArrayList<>();
         TopDownIndex topDownIndex = TopDownIndex.of(model);
         AwsTagIndex tagIndex = AwsTagIndex.of(model);
         for (ServiceShape service : model.getServiceShapesWithTrait(TagEnabledTrait.class)) {
@@ -44,7 +44,7 @@ public final class TagEnabledServiceValidator extends AbstractValidator {
             AwsTagIndex tagIndex,
             TopDownIndex topDownIndex
     ) {
-        List<ValidationEvent> events = new LinkedList<>();
+        List<ValidationEvent> events = new ArrayList<>();
         TagEnabledTrait trait = service.expectTrait(TagEnabledTrait.class);
 
         int taggableResourceCount = 0;
diff --git a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/TaggableResourceValidator.java b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/TaggableResourceValidator.java
index 2dfa663ad..edbab2e2c 100644
--- a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/TaggableResourceValidator.java
+++ b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/TaggableResourceValidator.java
@@ -16,8 +16,8 @@
 package software.amazon.smithy.aws.traits.tagging;
 
 import java.util.AbstractMap;
+import java.util.ArrayList;
 import java.util.Collection;
-import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -40,7 +40,7 @@ import software.amazon.smithy.model.validation.ValidationEvent;
 public final class TaggableResourceValidator extends AbstractValidator {
     @Override
     public List<ValidationEvent> validate(Model model) {
-        List<ValidationEvent> events = new LinkedList<>();
+        List<ValidationEvent> events = new ArrayList<>();
         TopDownIndex topDownIndex = TopDownIndex.of(model);
         AwsTagIndex tagIndex = AwsTagIndex.of(model);
         for (ServiceShape service : model.getServiceShapes()) {
@@ -72,7 +72,7 @@ public final class TaggableResourceValidator extends AbstractValidator {
             ServiceShape service,
             AwsTagIndex awsTagIndex
     ) {
-        List<ValidationEvent> events = new LinkedList<>();
+        List<ValidationEvent> events = new ArrayList<>();
         // Generate danger if resource has tag property in update API.
         if (awsTagIndex.isResourceTagOnUpdate(resource.getId())) {
             Shape operation = resource.getUpdate().isPresent()
@@ -87,7 +87,6 @@ public final class TaggableResourceValidator extends AbstractValidator {
         //    list or tag keys input or output member
         // 2. Tagging via APIs specified in the @taggable trait which are validated
         //    through the tag property, and must be resource instance operations
-        //Caution: avoid short circuiting behavior.
         boolean isServiceWideTaggable = awsTagIndex.serviceHasTagApis(service.getId());
         boolean isInstanceOpTaggable = isTaggableViaInstanceOperations(model, resource);
 
@@ -96,9 +95,11 @@ public final class TaggableResourceValidator extends AbstractValidator {
                     + " It must use the `aws.api@arn` trait."));
         }
 
-        if (!(isServiceWideTaggable || isInstanceOpTaggable)) {
-            events.add(error(resource, "Resource does not have tagging CRUD operations and is not compatible"
-                    + " with service-wide tagging operations."));
+        if (!isServiceWideTaggable && !isInstanceOpTaggable) {
+            events.add(error(resource, String.format("Resource does not have tagging CRUD operations and is not"
+                                                     + " compatible with service-wide tagging operations"
+                                                     + " for service `%s`.",
+                                                     service.getId())));
         }
 
         return events;
@@ -174,7 +175,7 @@ public final class TaggableResourceValidator extends AbstractValidator {
     }
 
     private Collection<Map.Entry<MemberShape, Shape>> collectMemberTargetShapes(ShapeId ioShapeId, Model model) {
-        Collection<Map.Entry<MemberShape, Shape>> collection = new LinkedList<>();
+        Collection<Map.Entry<MemberShape, Shape>> collection = new ArrayList<>();
         for (MemberShape memberShape : model.expectShape(ioShapeId).members()) {
             collection.add(new AbstractMap.SimpleImmutableEntry<>(
                     memberShape, model.expectShape(memberShape.getTarget())));
diff --git a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/TaggingShapeUtils.java b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/TaggingShapeUtils.java
index 5c2d3857d..4fb4d62a6 100644
--- a/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/TaggingShapeUtils.java
+++ b/smithy-aws-traits/src/main/java/software/amazon/smithy/aws/traits/tagging/TaggingShapeUtils.java
@@ -190,10 +190,10 @@ final class TaggingShapeUtils {
         Model model,
         ResourceShape resource
     ) {
-        PropertyBindingIndex propertyBindingIndex = PropertyBindingIndex.of(model);
-        Optional<String> property = resource.expectTrait(TaggableTrait.class).getProperty();
-        if (property.isPresent()) {
-            if (operationId.isPresent()) {
+        if (operationId.isPresent()) {
+            PropertyBindingIndex propertyBindingIndex = PropertyBindingIndex.of(model);
+            Optional<String> property = resource.expectTrait(TaggableTrait.class).getProperty();
+            if (property.isPresent()) {
                 OperationShape operation = model.expectShape(operationId.get()).asOperationShape().get();
                 Shape inputShape = model.expectShape(operation.getInputShape());
                 return isTagPropertyInShape(property.get(), inputShape, propertyBindingIndex);


Test patch:
diff --git a/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/tagging/invalid-tag-enabled-service-list-broken.errors b/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/tagging/invalid-tag-enabled-service-list-broken.errors
index 61ce2317f..8de5f15ce 100644
--- a/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/tagging/invalid-tag-enabled-service-list-broken.errors
+++ b/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/tagging/invalid-tag-enabled-service-list-broken.errors
@@ -1,3 +1,3 @@
 [DANGER] example.weather#Weather: Shape `example.weather#ListTagsForResource` does not satisfy 'ListTagsForResource' operation requirements. | ServiceTagging
-[ERROR] example.weather#City: Resource does not have tagging CRUD operations and is not compatible with service-wide tagging operations. | TaggableResource
+[ERROR] example.weather#City: Resource does not have tagging CRUD operations and is not compatible with service-wide tagging operations for service `example.weather#Weather`. | TaggableResource
 [WARNING] example.weather#Weather: Service marked `aws.api#tagEnabled` trait does not have consistent tagging operations implemented: {TagResource, UntagResource, and ListTagsForResource}. | TagEnabledService
diff --git a/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/tagging/invalid-tag-on-create.errors b/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/tagging/invalid-tag-on-create.errors
index c11ce595a..67922eecc 100644
--- a/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/tagging/invalid-tag-on-create.errors
+++ b/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/tagging/invalid-tag-on-create.errors
@@ -2,4 +2,4 @@
 [WARNING] example.weather#Weather: Service marked `aws.api#TagEnabled` is missing an operation named 'TagResource.' | ServiceTagging
 [WARNING] example.weather#Weather: Service marked `aws.api#TagEnabled` is missing an operation named 'UntagResource.' | ServiceTagging
 [WARNING] example.weather#Weather: Service marked `aws.api#tagEnabled` trait does not have consistent tagging operations implemented: {TagResource, UntagResource, and ListTagsForResource}. | TagEnabledService
-[ERROR] example.weather#City: Resource does not have tagging CRUD operations and is not compatible with service-wide tagging operations. | TaggableResource
+[ERROR] example.weather#City: Resource does not have tagging CRUD operations and is not compatible with service-wide tagging operations for service `example.weather#Weather`. | TaggableResource
diff --git a/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/tagging/invalid-tag-service-wide-two-services.errors b/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/tagging/invalid-tag-service-wide-two-services.errors
new file mode 100644
index 000000000..18a45045f
--- /dev/null
+++ b/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/tagging/invalid-tag-service-wide-two-services.errors
@@ -0,0 +1,3 @@
+[WARNING] example.weather#Forecast: Resource is likely missing `aws.api#taggable` trait. | TaggableResource
+[WARNING] example.weather#AnotherService: Service has resources with `aws.api#taggable` applied but does not have the `aws.api#tagEnabled` trait. | TaggableResource
+[ERROR] example.weather#City: Resource does not have tagging CRUD operations and is not compatible with service-wide tagging operations for service `example.weather#AnotherService`. | TaggableResource
diff --git a/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/tagging/invalid-tag-service-wide-two-services.smithy b/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/tagging/invalid-tag-service-wide-two-services.smithy
new file mode 100644
index 000000000..73d034571
--- /dev/null
+++ b/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/tagging/invalid-tag-service-wide-two-services.smithy
@@ -0,0 +1,168 @@
+$version: "2.0"
+
+metadata suppressions = [
+    {
+        id: "UnstableTrait",
+        namespace: "example.weather"
+    }
+]
+
+namespace example.weather
+
+use aws.api#arn
+use aws.api#taggable
+use aws.api#tagEnabled
+
+@tagEnabled
+service Weather {
+    version: "2006-03-01",
+    resources: [City]
+    operations: [GetCurrentTime, TagResource, UntagResource, ListTagsForResource]
+}
+
+@internal
+service AnotherService {
+    version: "2006-03-01"
+    resources: [AnotherResouce]
+}
+
+@arn(template: "resource/{resourceId}/another")
+resource AnotherResouce {
+    identifiers: {
+        cityId: CityId
+    }
+    resources: [
+        City
+    ]
+}
+
+structure Tag {
+    key: String
+    value: String
+}
+
+list TagList {
+    member: Tag
+}
+
+list TagKeys {
+    member: String
+}
+
+operation TagResource {
+    input := {
+        @required
+        arn: String
+        @length(max: 128)
+        tags: TagList
+    }
+    output := { }
+}
+
+operation UntagResource {
+    input := {
+        @required
+        arn: String
+        @required
+        tagKeys: TagKeys
+    }
+    output := { }
+}
+
+operation ListTagsForResource {
+    input := {
+        @required
+        arn: String
+    }
+    output := {
+        @length(max: 128)
+        tags: TagList
+    }
+}
+
+@arn(
+    template: "city/{cityId}/forecast/{forecastId}"
+)
+resource Forecast {
+    identifiers: { 
+        cityId: CityId
+        forecastId: ForecastId
+    }
+}
+
+@taggable(property: "tags")
+@arn(template: "city/{CityId}")
+resource City {
+    identifiers: { cityId: CityId }
+    properties: {
+        name: String
+        coordinates: CityCoordinates
+    }
+    read: GetCity
+    resources: [Forecast]
+}
+
+@pattern("^[A-Za-z0-9 ]+$")
+string ForecastId
+
+@pattern("^[A-Za-z0-9 ]+$")
+string CityId
+
+@readonly
+operation GetCity {
+    input: GetCityInput
+    output: GetCityOutput
+    errors: [NoSuchResource]
+}
+
+@input
+structure GetCityInput {
+    // "cityId" provides the identifier for the resource and
+    // has to be marked as required.
+    @required
+    cityId: CityId
+}
+
+@output
+structure GetCityOutput {
+    // "required" is used on output to indicate if the service
+    // will always provide a value for the member.
+    @required
+    name: String,
+
+    @required
+    coordinates: CityCoordinates
+}
+
+// This structure is nested within GetCityOutput.
+structure CityCoordinates {
+    @required
+    latitude: Float,
+
+    @required
+    longitude: Float,
+}
+
+// "error" is a trait that is used to specialize
+// a structure as an error.
+@error("client")
+structure NoSuchResource {
+    @required
+    resourceType: String
+}
+
+@readonly
+operation GetCurrentTime {
+    input: GetCurrentTimeInput
+    output: GetCurrentTimeOutput
+}
+
+@input
+structure GetCurrentTimeInput {}
+
+@output
+structure GetCurrentTimeOutput {
+    @required
+    time: Timestamp
+}
+
diff --git a/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/tagging/invalid-tag-types.errors b/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/tagging/invalid-tag-types.errors
index 400362b8d..5f8ee4696 100644
--- a/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/tagging/invalid-tag-types.errors
+++ b/smithy-aws-traits/src/test/resources/software/amazon/smithy/aws/traits/errorfiles/tagging/invalid-tag-types.errors
@@ -3,4 +3,4 @@
 [WARNING] example.weather#Weather: Service marked `aws.api#TagEnabled` is missing an operation named 'UntagResource.' | ServiceTagging
 [WARNING] example.weather#Weather: Service marked `aws.api#tagEnabled` trait does not have consistent tagging operations implemented: {TagResource, UntagResource, and ListTagsForResource}. | TagEnabledService
 [ERROR] example.weather#City: Tag property must be a list shape targeting a member containing a pair of strings, or a Map shape targeting a string member. | TagResourcePropertyType
-[ERROR] example.weather#City: Resource does not have tagging CRUD operations and is not compatible with service-wide tagging operations. | TaggableResource
+[ERROR] example.weather#City: Resource does not have tagging CRUD operations and is not compatible with service-wide tagging operations for service `example.weather#Weather`. | TaggableResource