From d721aab740cc851e4ddd12caea14bb459db7a8cf Mon Sep 17 00:00:00 2001
From: Miles Ziemer <mziemer@amazon.com>
Date: Thu, 5 Dec 2024 08:50:42 -0500
Subject: [PATCH] Fix unique smoke test id validation

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.
---
 ...b4361f200aabca46d06a5985bbd4ccc1fc60c.json |  7 ++++
 .../UniqueSmokeTestCaseIdValidator.java       | 28 ++++++--------
 .../traits/errorfiles/duplicate-ids.errors    |  2 +
 .../traits/errorfiles/duplicate-ids.smithy    | 37 ++++++++++++++++++-
 4 files changed, 55 insertions(+), 19 deletions(-)
 create mode 100644 .changes/next-release/bugfix-a4eb4361f200aabca46d06a5985bbd4ccc1fc60c.json

diff --git a/.changes/next-release/bugfix-a4eb4361f200aabca46d06a5985bbd4ccc1fc60c.json b/.changes/next-release/bugfix-a4eb4361f200aabca46d06a5985bbd4ccc1fc60c.json
new file mode 100644
index 00000000000..9a229a99dbe
--- /dev/null
+++ b/.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)"
+  ]
+}
diff --git a/smithy-smoke-test-traits/src/main/java/software/amazon/smithy/smoketests/traits/UniqueSmokeTestCaseIdValidator.java b/smithy-smoke-test-traits/src/main/java/software/amazon/smithy/smoketests/traits/UniqueSmokeTestCaseIdValidator.java
index ed8d16684ad..5040d203bd3 100644
--- a/smithy-smoke-test-traits/src/main/java/software/amazon/smithy/smoketests/traits/UniqueSmokeTestCaseIdValidator.java
+++ b/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
             }
         }
     }
-
-
 }
diff --git a/smithy-smoke-test-traits/src/test/resources/software/amazon/smithy/smoketests/traits/errorfiles/duplicate-ids.errors b/smithy-smoke-test-traits/src/test/resources/software/amazon/smithy/smoketests/traits/errorfiles/duplicate-ids.errors
index a24d3d91d47..8b9fb608e83 100644
--- a/smithy-smoke-test-traits/src/test/resources/software/amazon/smithy/smoketests/traits/errorfiles/duplicate-ids.errors
+++ b/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
diff --git a/smithy-smoke-test-traits/src/test/resources/software/amazon/smithy/smoketests/traits/errorfiles/duplicate-ids.smithy b/smithy-smoke-test-traits/src/test/resources/software/amazon/smithy/smoketests/traits/errorfiles/duplicate-ids.smithy
index 2b316c032f0..15a93c0b338 100644
--- a/smithy-smoke-test-traits/src/test/resources/software/amazon/smithy/smoketests/traits/errorfiles/duplicate-ids.smithy
+++ b/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 := {}
-}
\ No newline at end of file
+}
+
+@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 := {}
+}