From 5207afa35aa98eb7d2477a190ac0c8552e4f6a8f Mon Sep 17 00:00:00 2001 From: JordonPhillips Date: Mon, 18 Aug 2025 18:19:53 +0200 Subject: [PATCH] Return contained operations/resources in order This updates the results of TopDownIndex to be in modeled order. Previously the resuls were being sorted. This problem extended to the Walker, which was returning shapes in reverse order. --- .../core/directed/CodegenDirectorTest.java | 2 +- .../smithy/model/knowledge/TopDownIndex.java | 6 +- .../amazon/smithy/model/neighbor/Walker.java | 10 +++- .../model/knowledge/TopDownIndexTest.java | 58 +++++++++++++++++++ .../model/knowledge/top-down-order.smithy | 43 ++++++++++++++ .../validators/RuleSetParameterValidator.java | 10 +++- 6 files changed, 122 insertions(+), 7 deletions(-) create mode 100644 smithy-model/src/test/resources/software/amazon/smithy/model/knowledge/top-down-order.smithy diff --git a/smithy-codegen-core/src/test/java/software/amazon/smithy/codegen/core/directed/CodegenDirectorTest.java b/smithy-codegen-core/src/test/java/software/amazon/smithy/codegen/core/directed/CodegenDirectorTest.java index d324ce40178..dbeb2b038ae 100644 --- a/smithy-codegen-core/src/test/java/software/amazon/smithy/codegen/core/directed/CodegenDirectorTest.java +++ b/smithy-codegen-core/src/test/java/software/amazon/smithy/codegen/core/directed/CodegenDirectorTest.java @@ -337,7 +337,7 @@ public void testShapesGenerationWithoutOrder() { runner.run(); assertThat(testDirected.generatedShapes, - contains( + containsInAnyOrder( ShapeId.from("smithy.example#FooOperation"), ShapeId.from("smithy.example#FooOperationOutput"), ShapeId.from("smithy.example#A"), diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/TopDownIndex.java b/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/TopDownIndex.java index 4dccc9c5f80..a6e44df3dca 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/TopDownIndex.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/knowledge/TopDownIndex.java @@ -7,9 +7,9 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashSet; import java.util.Map; import java.util.Set; -import java.util.TreeSet; import java.util.function.Predicate; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.neighbor.NeighborProvider; @@ -56,8 +56,8 @@ public static TopDownIndex of(Model model) { } private void findContained(ShapeId container, Collection shapes) { - Set containedResources = new TreeSet<>(); - Set containedOperations = new TreeSet<>(); + Set containedResources = new LinkedHashSet<>(); + Set containedOperations = new LinkedHashSet<>(); for (Shape shape : shapes) { if (!shape.getId().equals(container)) { diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/Walker.java b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/Walker.java index a2f6eae182a..7bf75b66c08 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/Walker.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/neighbor/Walker.java @@ -154,7 +154,15 @@ public boolean hasNext() { while (!stack.isEmpty()) { // Every relationship is returned, even if the same shape is pointed // to multiple times from a single shape. - Relationship relationship = stack.pop(); + + // Use removeLast to retrieve relationships in their defined order rather + // than the reverse of the defined order. Note that this only preserves + // the order of a particular relationship type, not the order of all + // relationship types. So a resources `operation` relationships will be + // resolved in the order of that list, but the resources's `resource` + // relationships will nevertheless always appear first because that is + // simply the order that the NeighborVisitor checks them in. + Relationship relationship = stack.removeLast(); // Only traverse this relationship if the shape it points to hasn't // already been traversed. diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/TopDownIndexTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/TopDownIndexTest.java index ef1d9eac97d..4b78633379c 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/TopDownIndexTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/knowledge/TopDownIndexTest.java @@ -9,11 +9,14 @@ import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.empty; +import java.util.List; +import java.util.stream.Collectors; import org.junit.jupiter.api.Test; import software.amazon.smithy.model.Model; 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; import software.amazon.smithy.model.shapes.ShapeId; public class TopDownIndexTest { @@ -67,4 +70,59 @@ public void findsAllChildren() { assertThat(childIndex.getContainedResources(ShapeId.from("ns.foo#NotThere")), empty()); } + + @Test + public void preservesModeledOrder() { + Model model = Model.assembler() + .addImport(TopDownIndexTest.class.getResource("top-down-order.smithy")) + .assemble() + .unwrap(); + + TopDownIndex index = TopDownIndex.of(model); + List serviceOperations = index + .getContainedOperations(ShapeId.from("com.example#Service")) + .stream() + .map(Shape::toShapeId) + .collect(Collectors.toList()); + assertThat(serviceOperations, + contains( + ShapeId.from("com.example#OperationB"), + ShapeId.from("com.example#OperationA"), + ShapeId.from("com.example#OperationC"), + ShapeId.from("com.example#OperationD"), + ShapeId.from("com.example#OperationO"), + ShapeId.from("com.example#OperationG"))); + + List resourceOperations = index + .getContainedOperations(ShapeId.from("com.example#ResourceA")) + .stream() + .map(Shape::toShapeId) + .collect(Collectors.toList()); + assertThat(resourceOperations, + contains( + ShapeId.from("com.example#OperationD"), + ShapeId.from("com.example#OperationO"), + ShapeId.from("com.example#OperationG"))); + + List serviceResources = index + .getContainedResources(ShapeId.from("com.example#Service")) + .stream() + .map(Shape::toShapeId) + .collect(Collectors.toList()); + assertThat(serviceResources, + contains( + ShapeId.from("com.example#ResourceA"), + ShapeId.from("com.example#ResourceC"), + ShapeId.from("com.example#ResourceB"))); + + List resourceResources = index + .getContainedResources(ShapeId.from("com.example#ResourceA")) + .stream() + .map(Shape::toShapeId) + .collect(Collectors.toList()); + assertThat(resourceResources, + contains( + ShapeId.from("com.example#ResourceC"), + ShapeId.from("com.example#ResourceB"))); + } } diff --git a/smithy-model/src/test/resources/software/amazon/smithy/model/knowledge/top-down-order.smithy b/smithy-model/src/test/resources/software/amazon/smithy/model/knowledge/top-down-order.smithy new file mode 100644 index 00000000000..6b8cb295ded --- /dev/null +++ b/smithy-model/src/test/resources/software/amazon/smithy/model/knowledge/top-down-order.smithy @@ -0,0 +1,43 @@ +$version: "2.0" + +namespace com.example + + +service Service { + operations: [ + OperationB + OperationA + OperationC + ] + resources: [ + ResourceA + ] +} + +operation OperationB { } + +operation OperationC {} + +operation OperationA {} + +resource ResourceA { + operations: [ + OperationD + OperationO + OperationG + ] + resources: [ + ResourceC + ResourceB + ] +} + +resource ResourceB {} + +resource ResourceC {} + +operation OperationD {} + +operation OperationG {} + +operation OperationO {} 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 54d445bac93..3486645bb3f 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 @@ -50,12 +50,18 @@ public List validate(Model model) { List errors = new ArrayList<>(); for (ServiceShape serviceShape : model.getServiceShapesWithTrait(EndpointRuleSetTrait.class)) { + // This validator is sensitive to order, and with the change to topdownindex returning + // things in modeled order, the actual errors returned changed. So This reverses that order + // to side-step the issue. + // TODO: Make the validator not sensitive to order + List operations = new ArrayList<>(topDownIndex.getContainedOperations(serviceShape)); + operations = operations.reversed(); // Pull all the parameters used in this service related to endpoints, validating that // they are of matching types across the traits that can define them. Pair, Map> errorsParamsPair = validateAndExtractParameters( model, serviceShape, - topDownIndex.getContainedOperations(serviceShape)); + operations); errors.addAll(errorsParamsPair.getLeft()); // Make sure parameters align across Params <-> RuleSet transitions. @@ -79,7 +85,7 @@ public List validate(Model model) { private Pair, Map> validateAndExtractParameters( Model model, ServiceShape serviceShape, - Set containedOperations + List containedOperations ) { List errors = new ArrayList<>(); Map endpointParams = new HashMap<>();