From c6d222c28cfe5b365cc172a9784dca1ef70f29ba Mon Sep 17 00:00:00 2001 From: Kevin Stich Date: Mon, 28 Oct 2024 12:03:22 -0700 Subject: [PATCH] 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. --- .../diff/evaluators/ChangedMemberTarget.java | 49 +++++--- .../evaluators/ChangedMemberTargetTest.java | 108 ++++++++++++++++++ ...ber-target-invalid-nested-mapkey1-a.smithy | 15 +++ ...ber-target-invalid-nested-mapkey1-b.smithy | 17 +++ ...ber-target-invalid-nested-mapkey2-a.smithy | 15 +++ ...ber-target-invalid-nested-mapkey2-b.smithy | 17 +++ ...r-target-invalid-nested-mapvalue1-a.smithy | 15 +++ ...r-target-invalid-nested-mapvalue1-b.smithy | 17 +++ ...r-target-invalid-nested-mapvalue2-a.smithy | 15 +++ ...r-target-invalid-nested-mapvalue2-b.smithy | 17 +++ ...anged-member-target-valid-nested2-a.smithy | 15 +++ ...anged-member-target-valid-nested2-b.smithy | 15 +++ 12 files changed, 300 insertions(+), 15 deletions(-) create mode 100644 smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey1-a.smithy create mode 100644 smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey1-b.smithy create mode 100644 smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey2-a.smithy create mode 100644 smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapkey2-b.smithy create mode 100644 smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue1-a.smithy create mode 100644 smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue1-b.smithy create mode 100644 smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue2-a.smithy create mode 100644 smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-invalid-nested-mapvalue2-b.smithy create mode 100644 smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-valid-nested2-a.smithy create mode 100644 smithy-diff/src/test/resources/software/amazon/smithy/diff/evaluators/changed-member-target-valid-nested2-b.smithy 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 73ce120339b..f53cb1eb1bf 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.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 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 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 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 change, Shape oldTarget, Shape newTarget) { return String.format( "The shape targeted by the member `%s` changed from `%s` (%s) to `%s` (%s). ", 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 a405893f140..c2613028fdd 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 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 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 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 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 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 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 00000000000..66fb1a03a37 --- /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 00000000000..10053781899 --- /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 00000000000..66fb1a03a37 --- /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 00000000000..fee26f65ef6 --- /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 00000000000..66fb1a03a37 --- /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 00000000000..97b5fbbcd1c --- /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 00000000000..66fb1a03a37 --- /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 00000000000..77a42f2aa43 --- /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 00000000000..66fb1a03a37 --- /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 00000000000..0e7f9eb06e2 --- /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