Skip to content

Commit ddcf972

Browse files
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.
1 parent a859ef2 commit ddcf972

File tree

8 files changed

+134
-7
lines changed

8 files changed

+134
-7
lines changed
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"type": "feature",
3+
"description": "Updated `Walker` to walk relationships in their defined order, rather than the reverse of their defined order.",
4+
"pull_requests": [
5+
"[#2746](https://github.com/smithy-lang/smithy/pull/2746)"
6+
]
7+
}
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
{
2+
"type": "feature",
3+
"description": "Updated `TopDownIndex` to return results in modeled order.",
4+
"pull_requests": [
5+
"[#2746](https://github.com/smithy-lang/smithy/pull/2746)"
6+
]
7+
}

smithy-codegen-core/src/test/java/software/amazon/smithy/codegen/core/directed/CodegenDirectorTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -337,7 +337,7 @@ public void testShapesGenerationWithoutOrder() {
337337
runner.run();
338338

339339
assertThat(testDirected.generatedShapes,
340-
contains(
340+
containsInAnyOrder(
341341
ShapeId.from("smithy.example#FooOperation"),
342342
ShapeId.from("smithy.example#FooOperationOutput"),
343343
ShapeId.from("smithy.example#A"),

smithy-model/src/main/java/software/amazon/smithy/model/knowledge/TopDownIndex.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,9 @@
77
import java.util.Collection;
88
import java.util.Collections;
99
import java.util.HashMap;
10+
import java.util.LinkedHashSet;
1011
import java.util.Map;
1112
import java.util.Set;
12-
import java.util.TreeSet;
1313
import java.util.function.Predicate;
1414
import software.amazon.smithy.model.Model;
1515
import software.amazon.smithy.model.neighbor.NeighborProvider;
@@ -56,8 +56,8 @@ public static TopDownIndex of(Model model) {
5656
}
5757

5858
private void findContained(ShapeId container, Collection<Shape> shapes) {
59-
Set<ResourceShape> containedResources = new TreeSet<>();
60-
Set<OperationShape> containedOperations = new TreeSet<>();
59+
Set<ResourceShape> containedResources = new LinkedHashSet<>();
60+
Set<OperationShape> containedOperations = new LinkedHashSet<>();
6161

6262
for (Shape shape : shapes) {
6363
if (!shape.getId().equals(container)) {

smithy-model/src/main/java/software/amazon/smithy/model/neighbor/Walker.java

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,15 @@ public boolean hasNext() {
154154
while (!stack.isEmpty()) {
155155
// Every relationship is returned, even if the same shape is pointed
156156
// to multiple times from a single shape.
157-
Relationship relationship = stack.pop();
157+
158+
// Use removeLast to retrieve relationships in their defined order rather
159+
// than the reverse of the defined order. Note that this only preserves
160+
// the order of a particular relationship type, not the order of all
161+
// relationship types. So a resources `operation` relationships will be
162+
// resolved in the order of that list, but the resources's `resource`
163+
// relationships will nevertheless always appear first because that is
164+
// simply the order that the NeighborVisitor checks them in.
165+
Relationship relationship = stack.removeLast();
158166

159167
// Only traverse this relationship if the shape it points to hasn't
160168
// already been traversed.

smithy-model/src/test/java/software/amazon/smithy/model/knowledge/TopDownIndexTest.java

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,14 @@
99
import static org.hamcrest.Matchers.containsInAnyOrder;
1010
import static org.hamcrest.Matchers.empty;
1111

12+
import java.util.List;
13+
import java.util.stream.Collectors;
1214
import org.junit.jupiter.api.Test;
1315
import software.amazon.smithy.model.Model;
1416
import software.amazon.smithy.model.shapes.OperationShape;
1517
import software.amazon.smithy.model.shapes.ResourceShape;
1618
import software.amazon.smithy.model.shapes.ServiceShape;
19+
import software.amazon.smithy.model.shapes.Shape;
1720
import software.amazon.smithy.model.shapes.ShapeId;
1821

1922
public class TopDownIndexTest {
@@ -67,4 +70,59 @@ public void findsAllChildren() {
6770

6871
assertThat(childIndex.getContainedResources(ShapeId.from("ns.foo#NotThere")), empty());
6972
}
73+
74+
@Test
75+
public void preservesModeledOrder() {
76+
Model model = Model.assembler()
77+
.addImport(TopDownIndexTest.class.getResource("top-down-order.smithy"))
78+
.assemble()
79+
.unwrap();
80+
81+
TopDownIndex index = TopDownIndex.of(model);
82+
List<ShapeId> serviceOperations = index
83+
.getContainedOperations(ShapeId.from("com.example#Service"))
84+
.stream()
85+
.map(Shape::toShapeId)
86+
.collect(Collectors.toList());
87+
assertThat(serviceOperations,
88+
contains(
89+
ShapeId.from("com.example#OperationB"),
90+
ShapeId.from("com.example#OperationA"),
91+
ShapeId.from("com.example#OperationC"),
92+
ShapeId.from("com.example#OperationD"),
93+
ShapeId.from("com.example#OperationO"),
94+
ShapeId.from("com.example#OperationG")));
95+
96+
List<ShapeId> resourceOperations = index
97+
.getContainedOperations(ShapeId.from("com.example#ResourceA"))
98+
.stream()
99+
.map(Shape::toShapeId)
100+
.collect(Collectors.toList());
101+
assertThat(resourceOperations,
102+
contains(
103+
ShapeId.from("com.example#OperationD"),
104+
ShapeId.from("com.example#OperationO"),
105+
ShapeId.from("com.example#OperationG")));
106+
107+
List<ShapeId> serviceResources = index
108+
.getContainedResources(ShapeId.from("com.example#Service"))
109+
.stream()
110+
.map(Shape::toShapeId)
111+
.collect(Collectors.toList());
112+
assertThat(serviceResources,
113+
contains(
114+
ShapeId.from("com.example#ResourceA"),
115+
ShapeId.from("com.example#ResourceC"),
116+
ShapeId.from("com.example#ResourceB")));
117+
118+
List<ShapeId> resourceResources = index
119+
.getContainedResources(ShapeId.from("com.example#ResourceA"))
120+
.stream()
121+
.map(Shape::toShapeId)
122+
.collect(Collectors.toList());
123+
assertThat(resourceResources,
124+
contains(
125+
ShapeId.from("com.example#ResourceC"),
126+
ShapeId.from("com.example#ResourceB")));
127+
}
70128
}
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
$version: "2.0"
2+
3+
namespace com.example
4+
5+
6+
service Service {
7+
operations: [
8+
OperationB
9+
OperationA
10+
OperationC
11+
]
12+
resources: [
13+
ResourceA
14+
]
15+
}
16+
17+
operation OperationB { }
18+
19+
operation OperationC {}
20+
21+
operation OperationA {}
22+
23+
resource ResourceA {
24+
operations: [
25+
OperationD
26+
OperationO
27+
OperationG
28+
]
29+
resources: [
30+
ResourceC
31+
ResourceB
32+
]
33+
}
34+
35+
resource ResourceB {}
36+
37+
resource ResourceC {}
38+
39+
operation OperationD {}
40+
41+
operation OperationG {}
42+
43+
operation OperationO {}

smithy-rules-engine/src/main/java/software/amazon/smithy/rulesengine/validators/RuleSetParameterValidator.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
package software.amazon.smithy.rulesengine.validators;
66

77
import java.util.ArrayList;
8+
import java.util.Collection;
9+
import java.util.Collections;
810
import java.util.HashMap;
911
import java.util.HashSet;
1012
import java.util.List;
@@ -71,7 +73,9 @@ private void validate(
7173
) {
7274
// Pull all the parameters used in this service related to endpoints, validating that
7375
// they are of matching types across the traits that can define them.
74-
Set<OperationShape> operations = topDownIndex.getContainedOperations(service);
76+
List<OperationShape> operations = new ArrayList<>(topDownIndex.getContainedOperations(service));
77+
Collections.reverse(operations);
78+
7579
Map<String, Parameter> modelParams = validateAndExtractParameters(errors, model, service, operations);
7680
// Make sure parameters align across Params <-> RuleSet transitions.
7781
validateParametersMatching(errors, service, sourceLocation, parameters, modelParams);
@@ -85,7 +89,7 @@ private Map<String, Parameter> validateAndExtractParameters(
8589
List<ValidationEvent> errors,
8690
Model model,
8791
ServiceShape service,
88-
Set<OperationShape> containedOperations
92+
Collection<OperationShape> containedOperations
8993
) {
9094
Map<String, Parameter> endpointParams = new HashMap<>();
9195

0 commit comments

Comments
 (0)