From eb2c23ee22de375fdf3bf3976ad4da0ef44fd7c0 Mon Sep 17 00:00:00 2001 From: Michael Dowling Date: Fri, 2 Sep 2022 10:41:25 -0700 Subject: [PATCH] Ignore valid set->list type change in diff It isn't an error to migrate a set to a list with the uniqueItems trait. This is a recommended change because sets are deprecated and should not be implemented by any tooling. --- .../diff/evaluators/ChangedShapeType.java | 18 ++++++++++++++++++ .../diff/evaluators/ChangedShapeTypeTest.java | 19 +++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedShapeType.java b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedShapeType.java index 68a1ec1265b..68df5f683c1 100644 --- a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedShapeType.java +++ b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/ChangedShapeType.java @@ -17,7 +17,11 @@ import java.util.List; import java.util.stream.Collectors; +import software.amazon.smithy.diff.ChangedShape; import software.amazon.smithy.diff.Differences; +import software.amazon.smithy.model.shapes.Shape; +import software.amazon.smithy.model.shapes.ShapeType; +import software.amazon.smithy.model.traits.UniqueItemsTrait; import software.amazon.smithy.model.validation.ValidationEvent; /** @@ -28,9 +32,23 @@ public final class ChangedShapeType extends AbstractDiffEvaluator { public List evaluate(Differences differences) { return differences.changedShapes() .filter(diff -> diff.getOldShape().getType() != diff.getNewShape().getType()) + .filter(diff -> !expectedSetToListChange(diff)) .map(diff -> error(diff.getNewShape(), String.format( "Shape `%s` type was changed from `%s` to `%s`.", diff.getShapeId(), diff.getOldShape().getType(), diff.getNewShape().getType()))) .collect(Collectors.toList()); } + + private boolean expectedSetToListChange(ChangedShape diff) { + ShapeType oldType = diff.getOldShape().getType(); + ShapeType newType = diff.getNewShape().getType(); + + // Smithy diff doesn't raise an issue if a set is changed to a list and the list + // has the uniqueItems trait. Set is deprecated and this is a recommended change. + if (oldType == ShapeType.SET && newType == ShapeType.LIST) { + return diff.getNewShape().hasTrait(UniqueItemsTrait.class); + } + + return false; + } } diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedShapeTypeTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedShapeTypeTest.java index 87a742f2b3d..156bdcb1013 100644 --- a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedShapeTypeTest.java +++ b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/ChangedShapeTypeTest.java @@ -16,12 +16,15 @@ package software.amazon.smithy.diff.evaluators; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; import java.util.List; import org.junit.jupiter.api.Test; import software.amazon.smithy.diff.ModelDiff; import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.node.Node; +import software.amazon.smithy.model.shapes.ModelSerializer; import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.StringShape; import software.amazon.smithy.model.shapes.TimestampShape; @@ -40,4 +43,20 @@ public void detectsTypeChanges() { assertThat(TestHelper.findEvents(events, "ChangedShapeType").size(), equalTo(1)); assertThat(TestHelper.findEvents(events, shapeA1.getId()).size(), equalTo(1)); } + + @Test + public void ignoresExpectedSetToListMigration() { + String rawModel = "$version: \"1.0\"\nnamespace smithy.example\nset Foo { member: String }\n"; + Model oldModel = Model.assembler().addUnparsedModel("example.smithy", rawModel) + .assemble().unwrap(); + Node serialized = ModelSerializer.builder().build().serialize(oldModel); + Model newModel = Model.assembler() + .addDocumentNode(serialized) + .assemble() + .unwrap(); + + List events = ModelDiff.compare(oldModel, newModel); + + assertThat(TestHelper.findEvents(events, "ChangedShapeType"), empty()); + } }