Improve member target diff checks for maps

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

@@ -25,9 +25,11 @@
 import software.amazon.smithy.diff.Differences;
 import software.amazon.smithy.model.Model;
 import software.amazon.smithy.model.shapes.CollectionShape;
+import software.amazon.smithy.model.shapes.MapShape;
 import software.amazon.smithy.model.shapes.MemberShape;
 import software.amazon.smithy.model.shapes.Shape;
 import software.amazon.smithy.model.shapes.ShapeId;
+import software.amazon.smithy.model.shapes.ShapeType;
 import software.amazon.smithy.model.shapes.SimpleShape;
 import software.amazon.smithy.model.traits.EnumTrait;
 import software.amazon.smithy.model.traits.Trait;
@@ -99,7 +101,7 @@ private static List<String> areShapesCompatible(Shape oldShape, Shape newShape)
                                               oldShape.getType(), newShape.getType()));
         }
 
-        if (!(oldShape instanceof SimpleShape || oldShape instanceof CollectionShape)) {
+        if (!(oldShape instanceof SimpleShape || oldShape instanceof CollectionShape || oldShape instanceof MapShape)) {
             return ListUtils.of(String.format("The name of a %s is significant", oldShape.getType()));
         }
 
@@ -117,25 +119,42 @@ private static List<String> areShapesCompatible(Shape oldShape, Shape newShape)
         }
 
         if (oldShape instanceof CollectionShape) {
-            MemberShape oldMember = ((CollectionShape) oldShape).getMember();
-            MemberShape newMember = ((CollectionShape) newShape).getMember();
-            if (!oldMember.getTarget().equals(newMember.getTarget())) {
-                results.add(String.format("Both the old and new shapes are a %s, but the old shape targeted "
-                                          + "`%s` while the new shape targets `%s`",
-                                          oldShape.getType(),
-                                          oldMember.getTarget(),
-                                          newMember.getTarget()));
-            } else if (!oldMember.getAllTraits().equals(newMember.getAllTraits())) {
-                results.add(String.format("Both the old and new shapes are a %s, but their members have "
-                                          + "differing traits. %s",
-                                          oldShape.getType(),
-                                          createTraitDiffMessage(oldMember, newMember)));
-            }
+            evaluateMember(oldShape.getType(), results,
+                    ((CollectionShape) oldShape).getMember(),
+                    ((CollectionShape) newShape).getMember());
+        } else if (oldShape instanceof MapShape) {
+            MapShape oldMapShape = (MapShape) oldShape;
+            MapShape newMapShape = (MapShape) newShape;
+            // Both the key and value need to be evaluated for maps.
+            evaluateMember(oldShape.getType(), results,
+                    oldMapShape.getKey(),
+                    newMapShape.getKey());
+            evaluateMember(oldShape.getType(), results,
+                    oldMapShape.getValue(),
+                    newMapShape.getValue());
         }
 
         return results;
     }
 
+    private static void evaluateMember(
+            ShapeType oldShapeType,
+            List<String> results,
+            MemberShape oldMember,
+            MemberShape newMember
+    ) {
+        String memberSlug = oldShapeType == ShapeType.MAP ? oldMember.getMemberName() + " " : "";
+        if (!oldMember.getTarget().equals(newMember.getTarget())) {
+            results.add(String.format("Both the old and new shapes are a %s, but the old shape %stargeted "
+                            + "`%s` while the new shape targets `%s`",
+                    oldShapeType, memberSlug, oldMember.getTarget(), newMember.getTarget()));
+        } else if (!oldMember.getAllTraits().equals(newMember.getAllTraits())) {
+            results.add(String.format("Both the old and new shapes are a %s, but their %smembers have "
+                            + "differing traits. %s",
+                    oldShapeType, memberSlug, createTraitDiffMessage(oldMember, newMember)));
+        }
+    }
+
     private static String createSimpleMessage(ChangedShape<MemberShape> change, Shape oldTarget, Shape newTarget) {
         return String.format(
                 "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

@@ -221,6 +221,26 @@ public void detectsAcceptableListMemberChangesInNestedTargets() {
                            + "backward compatible."));
     }
 
+    @Test
+    public void detectsAcceptableMapMemberChangesInNestedTargets() {
+        Model modelA = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-valid-nested2-a.smithy"))
+                .assemble()
+                .unwrap();
+        Model modelB = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-valid-nested2-b.smithy"))
+                .assemble()
+                .unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "ChangedMemberTarget").size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, Severity.WARNING).size(), equalTo(1));
+        assertThat(TestHelper.findEvents(events, "ChangedMemberTarget").get(0).getMessage(),
+                equalTo("The shape targeted by the member `smithy.example#A$member` changed from "
+                        + "`smithy.example#B1` (map) to `smithy.example#B2` (map). This was determined "
+                        + "backward compatible."));
+    }
+
     @Test
     public void detectsInvalidListMemberChangesInNestedTargets() {
         Model modelA = Model.assembler()
@@ -264,4 +284,92 @@ public void detectsInvalidListMemberTargetChange() {
                            + "shapes are a list, but the old shape targeted `smithy.example#MyString` while "
                            + "the new shape targets `smithy.example#MyString2`."));
     }
+
+    @Test
+    public void detectsInvalidMapKeyChangesInNestedTargets() {
+        Model modelA = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-invalid-nested-mapkey1-a.smithy"))
+                .assemble()
+                .unwrap();
+        Model modelB = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-invalid-nested-mapkey1-b.smithy"))
+                .assemble()
+                .unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "ChangedMemberTarget").size(), equalTo(1));
+        ValidationEvent event = TestHelper.findEvents(events, "ChangedMemberTarget").get(0);
+        assertThat(event.getSeverity(), equalTo(Severity.ERROR));
+        assertThat(event.getMessage(),
+                equalTo("The shape targeted by the member `smithy.example#A$member` changed from "
+                        + "`smithy.example#B1` (map) to `smithy.example#B2` (map). Both the old and new "
+                        + "shapes are a map, but their key members have differing traits. The newly targeted "
+                        + "shape now has the following additional traits: [smithy.api#pattern]."));
+    }
+
+    @Test
+    public void detectsInvalidMapKeyTargetChange() {
+        Model modelA = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-invalid-nested-mapkey2-a.smithy"))
+                .assemble()
+                .unwrap();
+        Model modelB = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-invalid-nested-mapkey2-b.smithy"))
+                .assemble()
+                .unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "ChangedMemberTarget").size(), equalTo(1));
+        ValidationEvent event = TestHelper.findEvents(events, "ChangedMemberTarget").get(0);
+        assertThat(event.getSeverity(), equalTo(Severity.ERROR));
+        assertThat(event.getMessage(),
+                equalTo("The shape targeted by the member `smithy.example#A$member` changed from "
+                        + "`smithy.example#B1` (map) to `smithy.example#B2` (map). Both the old and new "
+                        + "shapes are a map, but the old shape key targeted `smithy.example#MyString` while "
+                        + "the new shape targets `smithy.example#MyString2`."));
+    }
+
+    @Test
+    public void detectsInvalidMapValueChangesInNestedTargets() {
+        Model modelA = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-invalid-nested-mapvalue1-a.smithy"))
+                .assemble()
+                .unwrap();
+        Model modelB = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-invalid-nested-mapvalue1-b.smithy"))
+                .assemble()
+                .unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "ChangedMemberTarget").size(), equalTo(1));
+        ValidationEvent event = TestHelper.findEvents(events, "ChangedMemberTarget").get(0);
+        assertThat(event.getSeverity(), equalTo(Severity.ERROR));
+        assertThat(event.getMessage(),
+                equalTo("The shape targeted by the member `smithy.example#A$member` changed from "
+                        + "`smithy.example#B1` (map) to `smithy.example#B2` (map). Both the old and new "
+                        + "shapes are a map, but their value members have differing traits. The newly targeted "
+                        + "shape now has the following additional traits: [smithy.api#pattern]."));
+    }
+
+    @Test
+    public void detectsInvalidMapValueTargetChange() {
+        Model modelA = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-invalid-nested-mapvalue2-a.smithy"))
+                .assemble()
+                .unwrap();
+        Model modelB = Model.assembler()
+                .addImport(getClass().getResource("changed-member-target-invalid-nested-mapvalue2-b.smithy"))
+                .assemble()
+                .unwrap();
+        List<ValidationEvent> events = ModelDiff.compare(modelA, modelB);
+
+        assertThat(TestHelper.findEvents(events, "ChangedMemberTarget").size(), equalTo(1));
+        ValidationEvent event = TestHelper.findEvents(events, "ChangedMemberTarget").get(0);
+        assertThat(event.getSeverity(), equalTo(Severity.ERROR));
+        assertThat(event.getMessage(),
+                equalTo("The shape targeted by the member `smithy.example#A$member` changed from "
+                        + "`smithy.example#B1` (map) to `smithy.example#B2` (map). Both the old and new "
+                        + "shapes are a map, but the old shape value targeted `smithy.example#MyString` while "
+                        + "the new shape targets `smithy.example#MyString2`."));
+    }
 }

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

@@ -0,0 +1,15 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B1
+}
+
+map B1 {
+    key: MyString
+    value: MyString
+}
+
+string MyString

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

@@ -0,0 +1,17 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B2
+}
+
+map B2 {
+    @pattern("^[a-z]+$")
+    key: MyString
+
+    value: MyString
+}
+
+string MyString

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

@@ -0,0 +1,15 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B1
+}
+
+map B1 {
+    key: MyString
+    value: MyString
+}
+
+string MyString

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

@@ -0,0 +1,17 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B2
+}
+
+map B2 {
+    key: MyString2
+    value: MyString
+}
+
+string MyString
+
+string MyString2

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

@@ -0,0 +1,15 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B1
+}
+
+map B1 {
+    key: MyString
+    value: MyString
+}
+
+string MyString

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

@@ -0,0 +1,17 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B2
+}
+
+map B2 {
+    key: MyString
+
+    @pattern("^[a-z]+$")
+    value: MyString
+}
+
+string MyString

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

@@ -0,0 +1,15 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B1
+}
+
+map B1 {
+    key: MyString
+    value: MyString
+}
+
+string MyString

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

@@ -0,0 +1,17 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B2
+}
+
+map B2 {
+    key: MyString
+    value: MyString2
+}
+
+string MyString
+
+string MyString2

smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-valid-nested2-a.smithy

@@ -0,0 +1,15 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B1
+}
+
+map B1 {
+    key: MyString
+    value: MyString
+}
+
+string MyString

smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-valid-nested2-b.smithy

@@ -0,0 +1,15 @@
+// See ChangedMemberTargetTest
+$version: "2.0"
+
+namespace smithy.example
+
+structure A {
+    member: B2
+}
+
+map B2 {
+    key: MyString
+    value: MyString
+}
+
+string MyString