Skip to content

Commit 1bf0cea

Browse files
mtdowlingmtdowling
committed
Make structure order part of the contract
This change updates Smithy to make the order of structure members part of the contract of structures and unions. We previously sorted these members automatically. However, languages like C, C++, Rust and others require the order of structures to be consistent, and new fields added to the end to maintain ABI compatibility. This change adds that language to the spec, updates the semantic model to use insertion order, updates equality checks to ensure that members have the same order, and adds a diff evaluator to detect a violation.
1 parent 590f98b commit 1bf0cea

File tree

13 files changed

+343
-72
lines changed

13 files changed

+343
-72
lines changed

‎docs/source/1.0/spec/core/shapes.rst‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -809,6 +809,12 @@ Traits can be applied to structure members:
809809
}
810810
}
811811

812+
New members added to existing structures SHOULD be added to the end of the
813+
structure. This ensures that programming languages that require a specific
814+
data structure layout or alignment for code generated from Smithy models are
815+
able to maintain backward compatibility.
816+
817+
812818
.. _union:
813819

814820
Union
@@ -861,6 +867,11 @@ The following example defines a union shape with several members:
861867
}
862868
}
863869

870+
New members added to existing unions SHOULD be added to the end of the
871+
union. This ensures that programming languages that require a specific
872+
data structure layout or alignment for code generated from Smithy models are
873+
able to maintain backward compatibility.
874+
864875

865876
.. _default-values:
866877

‎smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedMemberOrder.java‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
/*
2+
* Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License").
5+
* You may not use this file except in compliance with the License.
6+
* A copy of the License is located at
7+
*
8+
* http://aws.amazon.com/apache2.0
9+
*
10+
* or in the "license" file accompanying this file. This file is distributed
11+
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
12+
* express or implied. See the License for the specific language governing
13+
* permissions and limitations under the License.
14+
*/
15+
16+
package software.amazon.smithy.diff.evaluators;
17+
18+
import java.util.Collection;
19+
import java.util.Iterator;
20+
import java.util.List;
21+
import java.util.stream.Collectors;
22+
import java.util.stream.Stream;
23+
import software.amazon.smithy.diff.ChangedShape;
24+
import software.amazon.smithy.diff.Differences;
25+
import software.amazon.smithy.model.shapes.MemberShape;
26+
import software.amazon.smithy.model.shapes.StructureShape;
27+
import software.amazon.smithy.model.shapes.UnionShape;
28+
import software.amazon.smithy.model.validation.ValidationEvent;
29+
30+
/**
31+
* Creates a DANGER event when a structure or union member is added
32+
* anywhere other than the end of the previous definition or the
33+
* member order is changed.
34+
*/
35+
public final class ChangedMemberOrder extends AbstractDiffEvaluator {
36+
@Override
37+
public List<ValidationEvent> evaluate(Differences differences) {
38+
Stream<ChangedShape<?>> changes = Stream.concat(
39+
differences.changedShapes(StructureShape.class),
40+
differences.changedShapes(UnionShape.class));
41+
42+
return changes
43+
.filter(diff -> isUnordered(diff.getOldShape().members(), diff.getNewShape().members()))
44+
.map(diff -> danger(diff.getNewShape(), String.format(
45+
"%s shape members were reordered. This can cause ABI compatibility issues in languages "
46+
+ "like C, C++, and Rust where the layout and alignment of a data structure matters.",
47+
diff.getOldShape().getType())))
48+
.collect(Collectors.toList());
49+
}
50+
51+
private static boolean isUnordered(Collection<MemberShape> a, Collection<MemberShape> b) {
52+
Iterator<MemberShape> aIter = a.iterator();
53+
Iterator<MemberShape> bIter = b.iterator();
54+
55+
while (aIter.hasNext()) {
56+
// If members were removed, then this check is satisfied (though there are
57+
// other backward incompatible changes that other evaluators will detect).
58+
if (!bIter.hasNext()) {
59+
break;
60+
}
61+
62+
String oldMember = aIter.next().getMemberName();
63+
String newMember = bIter.next().getMemberName();
64+
if (!oldMember.equals(newMember)) {
65+
return true;
66+
}
67+
}
68+
69+
return false;
70+
}
71+
}

‎smithy-diff/src/main/resources/META-INF/services/software.amazon.smithy.diff.DiffEvaluator‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ software.amazon.smithy.diff.evaluators.AddedShape
66
software.amazon.smithy.diff.evaluators.AddedTraitDefinition
77
software.amazon.smithy.diff.evaluators.ChangedEnumTrait
88
software.amazon.smithy.diff.evaluators.ChangedLengthTrait
9+
software.amazon.smithy.diff.evaluators.ChangedMemberOrder
910
software.amazon.smithy.diff.evaluators.ChangedMemberTarget
1011
software.amazon.smithy.diff.evaluators.ChangedMetadata
1112
software.amazon.smithy.diff.evaluators.ChangedOperationInput

‎smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedMemberOrderTest.java‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
/*
2+
* Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License").
5+
* You may not use this file except in compliance with the License.
6+
* A copy of the License is located at
7+
*
8+
* http://aws.amazon.com/apache2.0
9+
*
10+
* or in the "license" file accompanying this file. This file is distributed
11+
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
12+
* express or implied. See the License for the specific language governing
13+
* permissions and limitations under the License.
14+
*/
15+
16+
package software.amazon.smithy.diff.evaluators;
17+
18+
import static org.hamcrest.MatcherAssert.assertThat;
19+
import static org.hamcrest.Matchers.empty;
20+
import static org.hamcrest.Matchers.equalTo;
21+
22+
import java.util.List;
23+
import org.junit.jupiter.api.Test;
24+
import software.amazon.smithy.diff.ModelDiff;
25+
import software.amazon.smithy.model.Model;
26+
import software.amazon.smithy.model.shapes.MemberShape;
27+
import software.amazon.smithy.model.shapes.Shape;
28+
import software.amazon.smithy.model.shapes.StringShape;
29+
import software.amazon.smithy.model.shapes.StructureShape;
30+
import software.amazon.smithy.model.validation.Severity;
31+
import software.amazon.smithy.model.validation.ValidationEvent;
32+
33+
public class ChangedMemberOrderTest {
34+
@Test
35+
public void detectsOrderChanges() {
36+
Shape shape1 = StringShape.builder().id("foo.baz#String").build();
37+
MemberShape member1 = MemberShape.builder().id("foo.baz#Struct$member1").target(shape1).build();
38+
MemberShape member2 = MemberShape.builder().id("foo.baz#Struct$member2").target(shape1).build();
39+
StructureShape oldStruct = StructureShape.builder()
40+
.id("foo.baz#Struct")
41+
.addMember(member1)
42+
.addMember(member2)
43+
.build();
44+
45+
StructureShape newStruct = StructureShape.builder()
46+
.id("foo.baz#Struct")
47+
.addMember(member2)
48+
.addMember(member1)
49+
.build();
50+
51+
Model modelA = Model.assembler().addShapes(shape1, oldStruct).assemble().unwrap();
52+
Model modelB = Model.assembler().addShapes(shape1, newStruct).assemble().unwrap();
53+
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
54+
55+
assertThat(TestHelper.findEvents(events, "ChangedMemberOrder").size(), equalTo(1));
56+
assertThat(TestHelper.findEvents(events, oldStruct.getId()).size(), equalTo(1));
57+
assertThat(TestHelper.findEvents(events, Severity.DANGER).size(), equalTo(1));
58+
}
59+
60+
@Test
61+
public void detectsOrderInsertionChanges() {
62+
Shape shape1 = StringShape.builder().id("foo.baz#String").build();
63+
MemberShape member1 = MemberShape.builder().id("foo.baz#Struct$member1").target(shape1).build();
64+
MemberShape member2 = MemberShape.builder().id("foo.baz#Struct$member2").target(shape1).build();
65+
StructureShape oldStruct = StructureShape.builder()
66+
.id("foo.baz#Struct")
67+
.addMember(member1)
68+
.addMember(member2)
69+
.build();
70+
71+
MemberShape member3 = MemberShape.builder().id("foo.baz#Struct$member3").target(shape1).build();
72+
StructureShape newStruct = StructureShape.builder()
73+
.id("foo.baz#Struct")
74+
.addMember(member1)
75+
.addMember(member3)
76+
.addMember(member2)
77+
.build();
78+
79+
Model modelA = Model.assembler().addShapes(shape1, oldStruct).assemble().unwrap();
80+
Model modelB = Model.assembler().addShapes(shape1, newStruct).assemble().unwrap();
81+
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
82+
83+
assertThat(TestHelper.findEvents(events, "ChangedMemberOrder").size(), equalTo(1));
84+
assertThat(TestHelper.findEvents(events, oldStruct.getId()).size(), equalTo(1));
85+
assertThat(TestHelper.findEvents(events, Severity.DANGER).size(), equalTo(1));
86+
}
87+
88+
@Test
89+
public void detectsCompatibleChanges() {
90+
Shape shape1 = StringShape.builder().id("foo.baz#String").build();
91+
MemberShape member1 = MemberShape.builder().id("foo.baz#Struct$member1").target(shape1).build();
92+
MemberShape member2 = MemberShape.builder().id("foo.baz#Struct$member2").target(shape1).build();
93+
StructureShape oldStruct = StructureShape.builder()
94+
.id("foo.baz#Struct")
95+
.addMember(member1)
96+
.addMember(member2)
97+
.build();
98+
99+
MemberShape member3 = MemberShape.builder().id("foo.baz#Struct$member3").target(shape1).build();
100+
StructureShape newStruct = StructureShape.builder()
101+
.id("foo.baz#Struct")
102+
.addMember(member1)
103+
.addMember(member2)
104+
.addMember(member3)
105+
.build();
106+
107+
Model modelA = Model.assembler().addShapes(shape1, oldStruct).assemble().unwrap();
108+
Model modelB = Model.assembler().addShapes(shape1, newStruct).assemble().unwrap();
109+
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
110+
111+
assertThat(TestHelper.findEvents(events, "ChangedMemberOrder"), empty());
112+
}
113+
114+
// Ignore the fact that a member was removed.
115+
@Test
116+
public void ignoresOtherBreakingChanges() {
117+
Shape shape1 = StringShape.builder().id("foo.baz#String").build();
118+
MemberShape member1 = MemberShape.builder().id("foo.baz#Struct$member1").target(shape1).build();
119+
MemberShape member2 = MemberShape.builder().id("foo.baz#Struct$member2").target(shape1).build();
120+
StructureShape oldStruct = StructureShape.builder()
121+
.id("foo.baz#Struct")
122+
.addMember(member1)
123+
.addMember(member2)
124+
.build();
125+
126+
StructureShape newStruct = StructureShape.builder()
127+
.id("foo.baz#Struct")
128+
.addMember(member1)
129+
.build();
130+
131+
Model modelA = Model.assembler().addShapes(shape1, oldStruct).assemble().unwrap();
132+
Model modelB = Model.assembler().addShapes(shape1, newStruct).assemble().unwrap();
133+
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
134+
135+
assertThat(TestHelper.findEvents(events, "ChangedMemberOrder"), empty());
136+
}
137+
}

‎smithy-model/src/main/java/software/amazon/smithy/model/loader/LoaderVisitor.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import java.util.Collection;
2222
import java.util.Collections;
2323
import java.util.HashMap;
24+
import java.util.LinkedHashMap;
2425
import java.util.List;
2526
import java.util.Map;
2627
import java.util.Objects;
@@ -83,10 +84,10 @@ final class LoaderVisitor {
8384
private final List<ForwardReferenceResolver> forwardReferenceResolvers = new ArrayList<>();
8485

8586
/** Shapes that have yet to be built. */
86-
private final Map<ShapeId, AbstractShapeBuilder> pendingShapes = new HashMap<>();
87+
private final Map<ShapeId, AbstractShapeBuilder> pendingShapes = new LinkedHashMap<>();
8788

8889
/** Built shapes to add to the Model. Keys are not allowed to conflict with pendingShapes. */
89-
private final Map<ShapeId, Shape> builtShapes = new HashMap<>();
90+
private final Map<ShapeId, Shape> builtShapes = new LinkedHashMap<>();
9091

9192
/** Built trait definitions. */
9293
private final Map<ShapeId, TraitDefinition> builtTraitDefinitions = new HashMap<>();

‎smithy-model/src/main/java/software/amazon/smithy/model/node/ObjectNode.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919

2020
import java.util.Collection;
2121
import java.util.Collections;
22-
import java.util.HashMap;
2322
import java.util.HashSet;
2423
import java.util.LinkedHashMap;
2524
import java.util.Map;
@@ -221,7 +220,7 @@ public Map<String, Node> getMembersByPrefix(String prefix) {
221220
public Map<String, Node> getStringMap() {
222221
Map<String, Node> map = stringMap;
223222
if (map == null) {
224-
map = new HashMap<>(nodeMap.size());
223+
map = new LinkedHashMap<>(nodeMap.size());
225224
for (Map.Entry<StringNode, Node> entry : nodeMap.entrySet()) {
226225
map.put(entry.getKey().getValue(), entry.getValue());
227226
}

‎smithy-model/src/main/java/software/amazon/smithy/model/shapes/ModelSerializer.java‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -250,8 +250,6 @@ private ObjectNode createStructureAndUnion(Shape shape, Map<String, MemberShape>
250250
ObjectNode result = createTypedNode(shape);
251251
result = result.withMember("members", members.entrySet().stream()
252252
.map(entry -> Pair.of(entry.getKey(), entry.getValue().accept(this)))
253-
// Sort by key to ensure a consistent member order.
254-
.sorted(Comparator.comparing(Pair::getLeft))
255253
.collect(ObjectNode.collectStringKeys(Pair::getLeft, Pair::getRight)));
256254
return withTraits(shape, result);
257255
}

‎smithy-model/src/main/java/software/amazon/smithy/model/shapes/NamedMembersShape.java‎

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,27 +18,30 @@
1818
import java.util.ArrayList;
1919
import java.util.Collection;
2020
import java.util.Collections;
21-
import java.util.HashMap;
21+
import java.util.LinkedHashMap;
2222
import java.util.List;
2323
import java.util.Map;
2424
import java.util.Optional;
25-
import java.util.TreeMap;
2625
import java.util.function.Consumer;
2726

2827
/**
2928
* Abstract classes shared by structure and union shapes.
29+
*
30+
* <p>The order of members in structures and unions are the same as the
31+
* order that they are defined in the model.
3032
*/
3133
abstract class NamedMembersShape extends Shape {
3234

3335
private final Map<String, MemberShape> members;
36+
private volatile List<String> memberNames;
3437

3538
NamedMembersShape(NamedMembersShape.Builder<?, ?> builder) {
3639
super(builder, false);
3740
assert builder.members != null;
3841

3942
// Copy the members to make them immutable and ensure that each
4043
// member has a valid ID that is prefixed with the shape ID.
41-
members = Collections.unmodifiableMap(new TreeMap<>(builder.members));
44+
members = Collections.unmodifiableMap(new LinkedHashMap<>(builder.members));
4245

4346
members.forEach((key, value) -> {
4447
ShapeId expected = getId().withMember(key);
@@ -60,12 +63,19 @@ public Map<String, MemberShape> getAllMembers() {
6063
}
6164

6265
/**
63-
* Returns a list of member names.
66+
* Returns an ordered list of member names based on the order they are
67+
* defined in the model.
6468
*
65-
* @return Returns list of member names.
69+
* @return Returns an immutable list of member names.
6670
*/
6771
public List<String> getMemberNames() {
68-
return new ArrayList<>(members.keySet());
72+
List<String> names = memberNames;
73+
if (names == null) {
74+
names = Collections.unmodifiableList(new ArrayList<>(members.keySet()));
75+
memberNames = names;
76+
}
77+
78+
return names;
6979
}
7080

7181
/**
@@ -85,7 +95,13 @@ public Collection<MemberShape> members() {
8595

8696
@Override
8797
public boolean equals(Object other) {
88-
return super.equals(other) && members.equals(((NamedMembersShape) other).members);
98+
if (!super.equals(other)) {
99+
return false;
100+
}
101+
102+
// Members are ordered, so do a test on the ordering and their values.
103+
NamedMembersShape b = (NamedMembersShape) other;
104+
return getMemberNames().equals(b.getMemberNames()) && members.equals(b.members);
89105
}
90106

91107
/**
@@ -95,7 +111,7 @@ public boolean equals(Object other) {
95111
*/
96112
abstract static class Builder<B extends Builder, S extends NamedMembersShape> extends AbstractShapeBuilder<B, S> {
97113

98-
Map<String, MemberShape> members = new HashMap<>();
114+
Map<String, MemberShape> members = new LinkedHashMap<>();
99115

100116
/**
101117
* Replaces the members of the builder.

‎smithy-model/src/test/java/software/amazon/smithy/model/node/ObjectNodeTest.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import static org.hamcrest.CoreMatchers.instanceOf;
2121
import static org.hamcrest.CoreMatchers.is;
2222
import static org.hamcrest.MatcherAssert.assertThat;
23+
import static org.hamcrest.Matchers.contains;
2324
import static org.hamcrest.Matchers.containsInAnyOrder;
2425
import static org.junit.jupiter.api.Assertions.assertEquals;
2526
import static org.junit.jupiter.api.Assertions.assertFalse;
@@ -98,6 +99,19 @@ public void containsMember() {
9899
assertTrue(node.containsMember("bam"));
99100
}
100101

102+
@Test
103+
public void membersAreOrdered() {
104+
ObjectNode node = ObjectNode.objectNodeBuilder()
105+
.withMember("foo", "bar")
106+
.withMember("baz", true)
107+
.withMember("bam", false)
108+
.build();
109+
110+
assertThat(node.getMembers().values(),
111+
contains(node.expectMember("foo"), node.expectMember("baz"), node.expectBooleanMember("bam")));
112+
assertThat(node.getStringMap().keySet(), contains("foo", "baz", "bam"));
113+
}
114+
101115
@Test
102116
public void getMemberByType() {
103117
ObjectNode node = ObjectNode.objectNodeBuilder()

0 commit comments

Comments
 (0)