Repository: smithy-lang/smithy
Pull request #2640 (2025-05-14 19:11:01)

Fix endpointTests missing required param validation
This commit removes extraneous code that cause parameters to be double counted in test cases. This would cacade into a mismatch in counts between test cases and parameters.

This extra counting was introduced in the endpoint ruleset refactoring incorrectly. Existing rulesets continue to work and a new test case shows the correct failure mode.

---
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-rules-engine/src/main/java/software/amazon/smithy/rulesengine/validators/RuleSetParameterValidator.java b/smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/validators/RuleSetParameterValidator.java
index 9592cdcc0..54d445bac 100644
--- a/smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/validators/RuleSetParameterValidator.java
+++ b/smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/validators/RuleSetParameterValidator.java
@@ -33,7 +33,6 @@ import software.amazon.smithy.rulesengine.traits.ClientContextParamsTrait;
 import software.amazon.smithy.rulesengine.traits.ContextParamTrait;
 import software.amazon.smithy.rulesengine.traits.EndpointRuleSetTrait;
 import software.amazon.smithy.rulesengine.traits.EndpointTestCase;
-import software.amazon.smithy.rulesengine.traits.EndpointTestOperationInput;
 import software.amazon.smithy.rulesengine.traits.EndpointTestsTrait;
 import software.amazon.smithy.rulesengine.traits.OperationContextParamsTrait;
 import software.amazon.smithy.rulesengine.traits.StaticContextParamDefinition;
@@ -67,8 +66,7 @@ public final class RuleSetParameterValidator extends AbstractValidator {
 
             // Check that tests declare required parameters, only defined parameters, etc.
             if (serviceShape.hasTrait(EndpointTestsTrait.ID)) {
-                errors.addAll(validateTestsParameters(model,
-                        topDownIndex,
+                errors.addAll(validateTestsParameters(
                         serviceShape,
                         serviceShape.expectTrait(EndpointTestsTrait.class),
                         ruleSet));
@@ -227,19 +225,13 @@ public final class RuleSetParameterValidator extends AbstractValidator {
     }
 
     private List<ValidationEvent> validateTestsParameters(
-            Model model,
-            TopDownIndex topDownIndex,
             ServiceShape serviceShape,
             EndpointTestsTrait trait,
             EndpointRuleSet ruleSet
     ) {
         List<ValidationEvent> errors = new ArrayList<>();
         Set<String> rulesetParamNames = new HashSet<>();
-        Map<String, List<Parameter>> testSuiteParams = extractTestSuiteParameters(model,
-                topDownIndex,
-                serviceShape,
-                ruleSet,
-                trait);
+        Map<String, List<Parameter>> testSuiteParams = extractTestSuiteParameters(trait.getTestCases());
 
         for (Parameter parameter : ruleSet.getParameters()) {
             String name = parameter.getName().toString();
@@ -290,54 +282,12 @@ public final class RuleSetParameterValidator extends AbstractValidator {
         return errors;
     }
 
-    private Map<String, List<Parameter>> extractTestSuiteParameters(
-            Model model,
-            TopDownIndex topDownIndex,
-            ServiceShape serviceShape,
-            EndpointRuleSet ruleSet,
-            EndpointTestsTrait trait
-    ) {
+    private Map<String, List<Parameter>> extractTestSuiteParameters(List<EndpointTestCase> testCases) {
         Map<String, List<Parameter>> params = new HashMap<>();
-        for (EndpointTestCase testCase : trait.getTestCases()) {
+        for (EndpointTestCase testCase : testCases) {
             List<Parameter> testParams = new ArrayList<>();
             for (Map.Entry<String, Node> entry : testCase.getParams().getStringMap().entrySet()) {
-                testParams.add(buildParameter(entry));
-            }
-
-            for (EndpointTestOperationInput input : testCase.getOperationInputs()) {
-                // These are easy, they're defined to match the clientContextParams.
-                for (Map.Entry<String, Node> entry : input.getClientParams().getStringMap().entrySet()) {
-                    testParams.add(buildParameter(entry));
-                }
-
-                // A little indirection here, since only parameters tracks the name of the built-in.
-                for (Map.Entry<String, Node> entry : input.getBuiltInParams().getStringMap().entrySet()) {
-                    for (Parameter parameter : ruleSet.getParameters()) {
-                        if (parameter.isBuiltIn() && parameter.getBuiltIn().get().equals(entry.getKey())) {
-                            testParams.add(buildParameter(entry, parameter.getName().toString()));
-                        }
-                    }
-                }
-
-                // Lots of indirection, the tests define operation inputs based on the name of the input
-                // member, but the map here needs to use the value of the parameter via contextParam.
-                for (OperationShape operationShape : topDownIndex.getContainedOperations(serviceShape)) {
-                    if (operationShape.getId().getName().equals(input.getOperationName())) {
-                        StructureShape inputShape = model.expectShape(
-                                operationShape.getInputShape(),
-                                StructureShape.class);
-                        for (String name : inputShape.getMemberNames()) {
-                            MemberShape memberShape = inputShape.getMember(name).get();
-                            if (input.getOperationParams().containsMember(name)
-                                    && memberShape.hasTrait(ContextParamTrait.ID)) {
-                                String paramName = memberShape.expectTrait(ContextParamTrait.class).getName();
-                                testParams.add(buildParameter(paramName,
-                                        input.getOperationParams().expectMember(name)));
-                            }
-                        }
-                        break;
-                    }
-                }
+                testParams.add(buildParameter(entry.getKey(), entry.getValue()));
             }
 
             for (Parameter parameter : testParams) {
@@ -348,14 +298,6 @@ public final class RuleSetParameterValidator extends AbstractValidator {
         return params;
     }
 
-    private Parameter buildParameter(Map.Entry<String, Node> entry) {
-        return buildParameter(entry.getKey(), entry.getValue());
-    }
-
-    private Parameter buildParameter(Map.Entry<String, Node> entry, String name) {
-        return buildParameter(name, entry.getValue());
-    }
-
     private Parameter buildParameter(String name, Node node) {
         Value value = Value.fromNode(node);
         return Parameter.builder()


Test patch:
diff --git a/smithy-rules-engine/src/test/resources/software/amazon/smithy/rulesengine/language/errorfiles/invalid/invalid-builtins.smithy b/smithy-rules-engine/src/test/resources/software/amazon/smithy/rulesengine/language/errorfiles/invalid/invalid-builtins.smithy
index 90166121a..9a3f63885 100644
--- a/smithy-rules-engine/src/test/resources/software/amazon/smithy/rulesengine/language/errorfiles/invalid/invalid-builtins.smithy
+++ b/smithy-rules-engine/src/test/resources/software/amazon/smithy/rulesengine/language/errorfiles/invalid/invalid-builtins.smithy
@@ -68,6 +68,7 @@ use smithy.rules#endpointTests
         }
     ]
 })
+@suppress(["RuleSetParameter.TestCase.Unused"])
 service ExampleService {
     version: "2022-01-01",
     operations: [GetThing]
diff --git a/smithy-rules-engine/src/test/resources/software/amazon/smithy/rulesengine/language/errorfiles/valid/default-values.smithy b/smithy-rules-engine/src/test/resources/software/amazon/smithy/rulesengine/language/errorfiles/valid/default-values.smithy
index f137a1872..ea185fafd 100644
--- a/smithy-rules-engine/src/test/resources/software/amazon/smithy/rulesengine/language/errorfiles/valid/default-values.smithy
+++ b/smithy-rules-engine/src/test/resources/software/amazon/smithy/rulesengine/language/errorfiles/valid/default-values.smithy
@@ -140,6 +140,7 @@ use smithy.rules#staticContextParams
         }
     ]
 })
+@suppress(["RuleSetParameter.TestCase.Unused"])
 service FizzBuzz {
     version: "2022-01-01",
     operations: [GetThing]
diff --git a/smithy-rules-engine/src/test/resources/software/amazon/smithy/rulesengine/traits/errorfiles/invalid-test-missing-required-built-in.errors b/smithy-rules-engine/src/test/resources/software/amazon/smithy/rulesengine/traits/errorfiles/invalid-test-missing-required-built-in.errors
new file mode 100644
index 000000000..a22630ccc
--- /dev/null
+++ b/smithy-rules-engine/src/test/resources/software/amazon/smithy/rulesengine/traits/errorfiles/invalid-test-missing-required-built-in.errors
@@ -0,0 +1,3 @@
+[WARNING] smithy.example#ExampleService: This shape applies a trait that is unstable: smithy.rules#endpointRuleSet | UnstableTrait
+[WARNING] smithy.example#ExampleService: This shape applies a trait that is unstable: smithy.rules#endpointTests | UnstableTrait
+[ERROR] smithy.example#ExampleService: Required parameter `endpoint` is missing in at least one test case | RuleSetParameter.TestCase.RequiredMissing
diff --git a/smithy-rules-engine/src/test/resources/software/amazon/smithy/rulesengine/traits/errorfiles/invalid-test-missing-required-built-in.smithy b/smithy-rules-engine/src/test/resources/software/amazon/smithy/rulesengine/traits/errorfiles/invalid-test-missing-required-built-in.smithy
new file mode 100644
index 000000000..b741275ee
--- /dev/null
+++ b/smithy-rules-engine/src/test/resources/software/amazon/smithy/rulesengine/traits/errorfiles/invalid-test-missing-required-built-in.smithy
@@ -0,0 +1,59 @@
+$version: "2.0"
+
+namespace smithy.example
+
+use smithy.rules#endpointRuleSet
+use smithy.rules#endpointTests
+
+@endpointRuleSet({
+    version: "1.0"
+    parameters: {
+        endpoint: {
+            type: "string"
+            builtIn: "SDK::Endpoint"
+            documentation: "docs"
+            required: true
+        }
+    }
+    rules: [
+        {
+            documentation: "Passthrough"
+            conditions: []
+            endpoint: {
+                url: "https://example.com"
+            }
+            type: "endpoint"
+        }
+    ]
+})
+@endpointTests({
+    version: "1.0"
+    testCases: [
+        {
+            params: {}
+            operationInputs: [{
+                operationName: "GetThing"
+                builtInParams: {
+                    "SDK::Endpoint": "https://example.com"
+                }
+            }]
+            expect: {
+                endpoint: {
+                    url: "https://example.com"
+                }
+            }
+        }
+    ]
+})
+@suppress(["RuleSetParameter.TestCase.Unused"])
+service ExampleService {
+    version: "2020-07-02"
+    operations: [GetThing]
+}
+
+operation GetThing {
+    input := {
+        fizz: String
+    }
+}
+