Fix unique smoke test id validation (#2482)

The validator is supposed that all smoke test case ids are unique within
a service's closure, but it was only checking operations directly bound
to the service shape.

.changes/next-release/bugfix-a4eb4361f200aabca46d06a5985bbd4ccc1fc60c.json

@@ -0,0 +1,7 @@
+{
+  "type": "bugfix",
+  "description": "Fixed a bug in smoke test validation where test case ids weren't checked for uniqueness if their operations were bound to a resource shape",
+  "pull_requests": [
+    "[#2482](https://github.com/smithy-lang/smithy/issues/2482)"
+  ]
+}

smithy-smoke-test-traits/src/main/java/software/amazon/smithy/smoketests/traits/UniqueSmokeTestCaseIdValidator.java

@@ -20,18 +20,17 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Collectors;
 import software.amazon.smithy.model.Model;
+import software.amazon.smithy.model.knowledge.TopDownIndex;
 import software.amazon.smithy.model.shapes.OperationShape;
 import software.amazon.smithy.model.shapes.ServiceShape;
 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;
 import software.amazon.smithy.model.validation.ValidationUtils;
-import software.amazon.smithy.utils.ListUtils;
+import software.amazon.smithy.utils.SetUtils;
 
 /**
  * Validates that smoke test cases have unique IDs within a service closure,
@@ -43,30 +42,27 @@ public class UniqueSmokeTestCaseIdValidator extends AbstractValidator {
     public List<ValidationEvent> validate(Model model) {
         List<ValidationEvent> events = new ArrayList<>();
         Set<ServiceShape> serviceShapes = model.getServiceShapes();
-        Set<ShapeId> serviceBoundOperationIds = new HashSet<>();
+        Set<OperationShape> serviceBoundOperations = new HashSet<>();
+        TopDownIndex index = new TopDownIndex(model);
         // Validate test case ids within each service closure
         for (ServiceShape service : serviceShapes) {
-            Set<ShapeId> operationIds = service.getAllOperations();
-            serviceBoundOperationIds.addAll(operationIds);
-            List<Shape> shapes = operationIds.stream()
-                    .map(model::getShape)
-                    .filter(Optional::isPresent)
-                    .map(Optional::get)
-                    .collect(Collectors.toList());
-            addValidationEventsForShapes(shapes, events);
+            Set<OperationShape> boundOperations = index.getContainedOperations(service);
+            addValidationEventsForShapes(boundOperations, events);
+            serviceBoundOperations.addAll(boundOperations);
         }
 
         // Also validate ids are unique within each non-service bound operation
         List<OperationShape> shapes = model.getOperationShapesWithTrait(SmokeTestsTrait.class).stream()
-                .filter(shape -> !serviceBoundOperationIds.contains(shape.getId()))
+                .filter(shape -> !serviceBoundOperations.contains(shape))
                 .collect(Collectors.toList());
+
         for (OperationShape shape : shapes) {
-            addValidationEventsForShapes(ListUtils.of(shape), events);
+            addValidationEventsForShapes(SetUtils.of(shape), events);
         }
         return events;
     }
 
-    private void addValidationEventsForShapes(List<? extends Shape> shapes, List<ValidationEvent> events) {
+    private void addValidationEventsForShapes(Set<OperationShape> shapes, List<ValidationEvent> events) {
         Map<String, List<Shape>> testCaseIdsToOperations = new HashMap<>();
         for (Shape shape : shapes) {
             if (!shape.isOperationShape() || !shape.hasTrait(SmokeTestsTrait.class)) {
@@ -91,6 +87,4 @@ private void addValidationEventsForShapes(List<? extends Shape> shapes, List<Val
             }
         }
     }
-
-
 }

smithy-smoke-test-traits/src/test/resources/software/amazon/smithy/smoketests/traits/errorfiles/duplicate-ids.errors

@@ -1,3 +1,5 @@
 [ERROR] smithy.example#SayHello: Conflicting `smithy.test#smokeTests` test case IDs found for ID `say_hello`: `smithy.example#SayHello`, `smithy.example#SayHello`, `smithy.example#SayHello2` | UniqueSmokeTestCaseId
 [ERROR] smithy.example#SayHello2: Conflicting `smithy.test#smokeTests` test case IDs found for ID `say_hello`: `smithy.example#SayHello`, `smithy.example#SayHello`, `smithy.example#SayHello2` | UniqueSmokeTestCaseId
 [ERROR] smithy.example#SayHello3: Conflicting `smithy.test#smokeTests` test case IDs found for ID `say_hello`: `smithy.example#SayHello3`, `smithy.example#SayHello3` | UniqueSmokeTestCaseId
+[ERROR] smithy.example#SayHello6: Conflicting `smithy.test#smokeTests` test case IDs found for ID `not_say_hello`: `smithy.example#SayHello6`, `smithy.example#SayHello7` | UniqueSmokeTestCaseId
+[ERROR] smithy.example#SayHello7: Conflicting `smithy.test#smokeTests` test case IDs found for ID `not_say_hello`: `smithy.example#SayHello6`, `smithy.example#SayHello7` | UniqueSmokeTestCaseId

smithy-smoke-test-traits/src/test/resources/software/amazon/smithy/smoketests/traits/errorfiles/duplicate-ids.smithy

@@ -94,7 +94,8 @@ operation SayHello4 {
 
 service OtherSayStuff {
     version: "2023-10-11"
-    operations: [SayHello5]
+    operations: [SayHello5, SayHello6]
+    resources: [SayHelloResource]
 }
 
 // Shouldn't conflict between services
@@ -110,4 +111,36 @@ service OtherSayStuff {
 operation SayHello5 {
    input := {}
    output := {}
-}
+}
+
+@smokeTests([
+    {
+        id: "not_say_hello" // Conflicts with resource bound operation
+        params: {}
+        expect: {
+            success: {}
+        }
+    }
+])
+operation SayHello6 {
+    input := {}
+    output := {}
+}
+
+resource SayHelloResource {
+    operations: [SayHello7]
+}
+
+@smokeTests([
+    {
+        id: "not_say_hello"
+        params: {}
+        expect: {
+            success: {}
+        }
+    }
+])
+operation SayHello7 {
+    input := {}
+    output := {}
+}