Repository: smithy-lang/smithy
Pull request #2434 (2024-10-28 19:06:07)

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.

-----

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.



Patch:
diff --git a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedMemberTarget.java b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedMemberTarget.java
index 73ce12033..f53cb1eb1 100644
--- a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedMemberTarget.java
+++ b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedMemberTarget.java
@@ -25,9 +25,11 @@ import software.amazon.smithy.diff.ChangedShape;
 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 @@ public final class ChangedMemberTarget extends AbstractDiffEvaluator {
                                               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 @@ public final class ChangedMemberTarget extends AbstractDiffEvaluator {
         }
 
         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). ",


Test patch:
diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedMemberTargetTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedMemberTargetTest.java
index a405893f1..c2613028f 100644
--- a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedMemberTargetTest.java
+++ b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedMemberTargetTest.java
@@ -221,6 +221,26 @@ public class ChangedMemberTargetTest {
                            + "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 class ChangedMemberTargetTest {
                            + "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`."));
+    }
 }
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey1-a.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey1-a.smithy
new file mode 100644
index 000000000..66fb1a03a
--- /dev/null
+++ b/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
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey1-b.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey1-b.smithy
new file mode 100644
index 000000000..100537818
--- /dev/null
+++ b/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
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey2-a.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey2-a.smithy
new file mode 100644
index 000000000..66fb1a03a
--- /dev/null
+++ b/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
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey2-b.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey2-b.smithy
new file mode 100644
index 000000000..fee26f65e
--- /dev/null
+++ b/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
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue1-a.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue1-a.smithy
new file mode 100644
index 000000000..66fb1a03a
--- /dev/null
+++ b/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
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue1-b.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue1-b.smithy
new file mode 100644
index 000000000..97b5fbbcd
--- /dev/null
+++ b/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
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue2-a.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue2-a.smithy
new file mode 100644
index 000000000..66fb1a03a
--- /dev/null
+++ b/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
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue2-b.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue2-b.smithy
new file mode 100644
index 000000000..77a42f2aa
--- /dev/null
+++ b/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
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-valid-nested2-a.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-valid-nested2-a.smithy
new file mode 100644
index 000000000..66fb1a03a
--- /dev/null
+++ b/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
diff --git a/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-valid-nested2-b.smithy b/smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-valid-nested2-b.smithy
new file mode 100644
index 000000000..0e7f9eb06
--- /dev/null
+++ b/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