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<>();