Skip to content

Commit 34d2396

Browse files
kstichkstich
committed
Improve member target diff checks for maps
This commit updates the ChangedMemberTarget diff evaluator to properly check changes to map keys and values the same way it checks changes to list members.
1 parent ce7b765 commit 34d2396

12 files changed

+300
-15
lines changed

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

Lines changed: 34 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,11 @@
2525
import software.amazon.smithy.diff.Differences;
2626
import software.amazon.smithy.model.Model;
2727
import software.amazon.smithy.model.shapes.CollectionShape;
28+
import software.amazon.smithy.model.shapes.MapShape;
2829
import software.amazon.smithy.model.shapes.MemberShape;
2930
import software.amazon.smithy.model.shapes.Shape;
3031
import software.amazon.smithy.model.shapes.ShapeId;
32+
import software.amazon.smithy.model.shapes.ShapeType;
3133
import software.amazon.smithy.model.shapes.SimpleShape;
3234
import software.amazon.smithy.model.traits.EnumTrait;
3335
import software.amazon.smithy.model.traits.Trait;
@@ -99,7 +101,7 @@ private static List<String> areShapesCompatible(Shape oldShape, Shape newShape)
99101
oldShape.getType(), newShape.getType()));
100102
}
101103

102-
if (!(oldShape instanceof SimpleShape || oldShape instanceof CollectionShape)) {
104+
if (!(oldShape instanceof SimpleShape || oldShape instanceof CollectionShape || oldShape instanceof MapShape)) {
103105
return ListUtils.of(String.format("The name of a %s is significant", oldShape.getType()));
104106
}
105107

@@ -117,25 +119,42 @@ private static List<String> areShapesCompatible(Shape oldShape, Shape newShape)
117119
}
118120

119121
if (oldShape instanceof CollectionShape) {
120-
MemberShape oldMember = ((CollectionShape) oldShape).getMember();
121-
MemberShape newMember = ((CollectionShape) newShape).getMember();
122-
if (!oldMember.getTarget().equals(newMember.getTarget())) {
123-
results.add(String.format("Both the old and new shapes are a %s, but the old shape targeted "
124-
+ "`%s` while the new shape targets `%s`",
125-
oldShape.getType(),
126-
oldMember.getTarget(),
127-
newMember.getTarget()));
128-
} else if (!oldMember.getAllTraits().equals(newMember.getAllTraits())) {
129-
results.add(String.format("Both the old and new shapes are a %s, but their members have "
130-
+ "differing traits. %s",
131-
oldShape.getType(),
132-
createTraitDiffMessage(oldMember, newMember)));
133-
}
122+
evaluateMember(oldShape.getType(), results,
123+
((CollectionShape) oldShape).getMember(),
124+
((CollectionShape) newShape).getMember());
125+
} else if (oldShape instanceof MapShape) {
126+
MapShape oldMapShape = (MapShape) oldShape;
127+
MapShape newMapShape = (MapShape) newShape;
128+
// Both the key and value need to be evaluated for maps.
129+
evaluateMember(oldShape.getType(), results,
130+
oldMapShape.getKey(),
131+
newMapShape.getKey());
132+
evaluateMember(oldShape.getType(), results,
133+
oldMapShape.getValue(),
134+
newMapShape.getValue());
134135
}
135136

136137
return results;
137138
}
138139

140+
private static void evaluateMember(
141+
ShapeType oldShapeType,
142+
List<String> results,
143+
MemberShape oldMember,
144+
MemberShape newMember
145+
) {
146+
String memberSlug = oldShapeType == ShapeType.MAP ? oldMember.getMemberName() + " " : "";
147+
if (!oldMember.getTarget().equals(newMember.getTarget())) {
148+
results.add(String.format("Both the old and new shapes are a %s, but the old shape %stargeted "
149+
+ "`%s` while the new shape targets `%s`",
150+
oldShapeType, memberSlug, oldMember.getTarget(), newMember.getTarget()));
151+
} else if (!oldMember.getAllTraits().equals(newMember.getAllTraits())) {
152+
results.add(String.format("Both the old and new shapes are a %s, but their %smembers have "
153+
+ "differing traits. %s",
154+
oldShapeType, memberSlug, createTraitDiffMessage(oldMember, newMember)));
155+
}
156+
}
157+
139158
private static String createSimpleMessage(ChangedShape<MemberShape> change, Shape oldTarget, Shape newTarget) {
140159
return String.format(
141160
"The shape targeted by the member `%s` changed from `%s` (%s) to `%s` (%s). ",

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

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,26 @@ public void detectsAcceptableListMemberChangesInNestedTargets() {
221221
+ "backward compatible."));
222222
}
223223

224+
@Test
225+
public void detectsAcceptableMapMemberChangesInNestedTargets() {
226+
Model modelA = Model.assembler()
227+
.addImport(getClass().getResource("changed-member-target-valid-nested2-a.smithy"))
228+
.assemble()
229+
.unwrap();
230+
Model modelB = Model.assembler()
231+
.addImport(getClass().getResource("changed-member-target-valid-nested2-b.smithy"))
232+
.assemble()
233+
.unwrap();
234+
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
235+
236+
assertThat(TestHelper.findEvents(events, "ChangedMemberTarget").size(), equalTo(1));
237+
assertThat(TestHelper.findEvents(events, Severity.WARNING).size(), equalTo(1));
238+
assertThat(TestHelper.findEvents(events, "ChangedMemberTarget").get(0).getMessage(),
239+
equalTo("The shape targeted by the member `smithy.example#A$member` changed from "
240+
+ "`smithy.example#B1` (map) to `smithy.example#B2` (map). This was determined "
241+
+ "backward compatible."));
242+
}
243+
224244
@Test
225245
public void detectsInvalidListMemberChangesInNestedTargets() {
226246
Model modelA = Model.assembler()
@@ -264,4 +284,92 @@ public void detectsInvalidListMemberTargetChange() {
264284
+ "shapes are a list, but the old shape targeted `smithy.example#MyString` while "
265285
+ "the new shape targets `smithy.example#MyString2`."));
266286
}
287+
288+
@Test
289+
public void detectsInvalidMapKeyChangesInNestedTargets() {
290+
Model modelA = Model.assembler()
291+
.addImport(getClass().getResource("changed-member-target-invalid-nested-mapkey1-a.smithy"))
292+
.assemble()
293+
.unwrap();
294+
Model modelB = Model.assembler()
295+
.addImport(getClass().getResource("changed-member-target-invalid-nested-mapkey1-b.smithy"))
296+
.assemble()
297+
.unwrap();
298+
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
299+
300+
assertThat(TestHelper.findEvents(events, "ChangedMemberTarget").size(), equalTo(1));
301+
ValidationEvent event = TestHelper.findEvents(events, "ChangedMemberTarget").get(0);
302+
assertThat(event.getSeverity(), equalTo(Severity.ERROR));
303+
assertThat(event.getMessage(),
304+
equalTo("The shape targeted by the member `smithy.example#A$member` changed from "
305+
+ "`smithy.example#B1` (map) to `smithy.example#B2` (map). Both the old and new "
306+
+ "shapes are a map, but their key members have differing traits. The newly targeted "
307+
+ "shape now has the following additional traits: [smithy.api#pattern]."));
308+
}
309+
310+
@Test
311+
public void detectsInvalidMapKeyTargetChange() {
312+
Model modelA = Model.assembler()
313+
.addImport(getClass().getResource("changed-member-target-invalid-nested-mapkey2-a.smithy"))
314+
.assemble()
315+
.unwrap();
316+
Model modelB = Model.assembler()
317+
.addImport(getClass().getResource("changed-member-target-invalid-nested-mapkey2-b.smithy"))
318+
.assemble()
319+
.unwrap();
320+
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
321+
322+
assertThat(TestHelper.findEvents(events, "ChangedMemberTarget").size(), equalTo(1));
323+
ValidationEvent event = TestHelper.findEvents(events, "ChangedMemberTarget").get(0);
324+
assertThat(event.getSeverity(), equalTo(Severity.ERROR));
325+
assertThat(event.getMessage(),
326+
equalTo("The shape targeted by the member `smithy.example#A$member` changed from "
327+
+ "`smithy.example#B1` (map) to `smithy.example#B2` (map). Both the old and new "
328+
+ "shapes are a map, but the old shape key targeted `smithy.example#MyString` while "
329+
+ "the new shape targets `smithy.example#MyString2`."));
330+
}
331+
332+
@Test
333+
public void detectsInvalidMapValueChangesInNestedTargets() {
334+
Model modelA = Model.assembler()
335+
.addImport(getClass().getResource("changed-member-target-invalid-nested-mapvalue1-a.smithy"))
336+
.assemble()
337+
.unwrap();
338+
Model modelB = Model.assembler()
339+
.addImport(getClass().getResource("changed-member-target-invalid-nested-mapvalue1-b.smithy"))
340+
.assemble()
341+
.unwrap();
342+
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
343+
344+
assertThat(TestHelper.findEvents(events, "ChangedMemberTarget").size(), equalTo(1));
345+
ValidationEvent event = TestHelper.findEvents(events, "ChangedMemberTarget").get(0);
346+
assertThat(event.getSeverity(), equalTo(Severity.ERROR));
347+
assertThat(event.getMessage(),
348+
equalTo("The shape targeted by the member `smithy.example#A$member` changed from "
349+
+ "`smithy.example#B1` (map) to `smithy.example#B2` (map). Both the old and new "
350+
+ "shapes are a map, but their value members have differing traits. The newly targeted "
351+
+ "shape now has the following additional traits: [smithy.api#pattern]."));
352+
}
353+
354+
@Test
355+
public void detectsInvalidMapValueTargetChange() {
356+
Model modelA = Model.assembler()
357+
.addImport(getClass().getResource("changed-member-target-invalid-nested-mapvalue2-a.smithy"))
358+
.assemble()
359+
.unwrap();
360+
Model modelB = Model.assembler()
361+
.addImport(getClass().getResource("changed-member-target-invalid-nested-mapvalue2-b.smithy"))
362+
.assemble()
363+
.unwrap();
364+
List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
365+
366+
assertThat(TestHelper.findEvents(events, "ChangedMemberTarget").size(), equalTo(1));
367+
ValidationEvent event = TestHelper.findEvents(events, "ChangedMemberTarget").get(0);
368+
assertThat(event.getSeverity(), equalTo(Severity.ERROR));
369+
assertThat(event.getMessage(),
370+
equalTo("The shape targeted by the member `smithy.example#A$member` changed from "
371+
+ "`smithy.example#B1` (map) to `smithy.example#B2` (map). Both the old and new "
372+
+ "shapes are a map, but the old shape value targeted `smithy.example#MyString` while "
373+
+ "the new shape targets `smithy.example#MyString2`."));
374+
}
267375
}

‎smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey1-a.smithy‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// See ChangedMemberTargetTest
2+
$version: "2.0"
3+
4+
namespace smithy.example
5+
6+
structure A {
7+
member: B1
8+
}
9+
10+
map B1 {
11+
key: MyString
12+
value: MyString
13+
}
14+
15+
string MyString

‎smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey1-b.smithy‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
// See ChangedMemberTargetTest
2+
$version: "2.0"
3+
4+
namespace smithy.example
5+
6+
structure A {
7+
member: B2
8+
}
9+
10+
map B2 {
11+
@pattern("^[a-z]+$")
12+
key: MyString
13+
14+
value: MyString
15+
}
16+
17+
string MyString

‎smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey2-a.smithy‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// See ChangedMemberTargetTest
2+
$version: "2.0"
3+
4+
namespace smithy.example
5+
6+
structure A {
7+
member: B1
8+
}
9+
10+
map B1 {
11+
key: MyString
12+
value: MyString
13+
}
14+
15+
string MyString

‎smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey2-b.smithy‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
// See ChangedMemberTargetTest
2+
$version: "2.0"
3+
4+
namespace smithy.example
5+
6+
structure A {
7+
member: B2
8+
}
9+
10+
map B2 {
11+
key: MyString2
12+
value: MyString
13+
}
14+
15+
string MyString
16+
17+
string MyString2

‎smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue1-a.smithy‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// See ChangedMemberTargetTest
2+
$version: "2.0"
3+
4+
namespace smithy.example
5+
6+
structure A {
7+
member: B1
8+
}
9+
10+
map B1 {
11+
key: MyString
12+
value: MyString
13+
}
14+
15+
string MyString

‎smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue1-b.smithy‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
// See ChangedMemberTargetTest
2+
$version: "2.0"
3+
4+
namespace smithy.example
5+
6+
structure A {
7+
member: B2
8+
}
9+
10+
map B2 {
11+
key: MyString
12+
13+
@pattern("^[a-z]+$")
14+
value: MyString
15+
}
16+
17+
string MyString

‎smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue2-a.smithy‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// See ChangedMemberTargetTest
2+
$version: "2.0"
3+
4+
namespace smithy.example
5+
6+
structure A {
7+
member: B1
8+
}
9+
10+
map B1 {
11+
key: MyString
12+
value: MyString
13+
}
14+
15+
string MyString

‎smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue2-b.smithy‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
// See ChangedMemberTargetTest
2+
$version: "2.0"
3+
4+
namespace smithy.example
5+
6+
structure A {
7+
member: B2
8+
}
9+
10+
map B2 {
11+
key: MyString
12+
value: MyString2
13+
}
14+
15+
string MyString
16+
17+
string MyString2

0 commit comments

Comments
 (0)