diff --git a/smithy-model/src/main/java/software/amazon/smithy/model/transform/ReplaceShapes.java b/smithy-model/src/main/java/software/amazon/smithy/model/transform/ReplaceShapes.java index 2347b3e2229..497da8d57b3 100644 --- a/smithy-model/src/main/java/software/amazon/smithy/model/transform/ReplaceShapes.java +++ b/smithy-model/src/main/java/software/amazon/smithy/model/transform/ReplaceShapes.java @@ -27,6 +27,7 @@ import java.util.Set; import java.util.stream.Collectors; import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.shapes.CollectionShape; import software.amazon.smithy.model.shapes.ListShape; import software.amazon.smithy.model.shapes.MapShape; import software.amazon.smithy.model.shapes.MemberShape; @@ -34,6 +35,7 @@ import software.amazon.smithy.model.shapes.Shape; import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.shapes.ShapeVisitor; +import software.amazon.smithy.model.shapes.SimpleShape; import software.amazon.smithy.model.shapes.StructureShape; import software.amazon.smithy.model.shapes.UnionShape; import software.amazon.smithy.utils.OptionalUtils; @@ -84,7 +86,7 @@ Model transform(ModelTransformer transformer, Model model) { return model; } - assertNoShapeChangedType(model, shouldReplace); + assertShapeTypeChangesSound(model, shouldReplace); Model.Builder builder = createReplacedModelBuilder(model, shouldReplace); // If a member shape changes, then ensure that the containing shape @@ -121,18 +123,37 @@ private List determineShapesToReplace(Model model) { .collect(toList()); } - private void assertNoShapeChangedType(Model model, List shouldReplace) { + private void assertShapeTypeChangesSound(Model model, List shouldReplace) { // Throws if any mappings attempted to change a shape's type. shouldReplace.stream() .flatMap(previous -> Pair.flatMapStream(previous, p -> model.getShape(p.getId()))) .filter(pair -> pair.getLeft().getType() != pair.getRight().getType()) + .filter(pair -> !isReplacementValid(model, pair.getLeft(), pair.getRight())) .forEach(pair -> { - throw new RuntimeException(String.format( + throw new IllegalStateException(String.format( "Cannot change the type of %s from %s to %s", - pair.getLeft().getId(), pair.getLeft().getType(), pair.getRight().getType())); + pair.getLeft().getId(), pair.getRight().getType(), pair.getLeft().getType())); }); } + private boolean isReplacementValid(Model model, Shape left, Shape right) { + if (left instanceof CollectionShape && right instanceof CollectionShape) { + validateListCollectionReplacement(model, (CollectionShape) left, (CollectionShape) right); + return true; + } + return left instanceof SimpleShape && right instanceof SimpleShape; + } + + private void validateListCollectionReplacement(Model model, CollectionShape left, CollectionShape right) { + Shape leftTarget = model.expectShape(left.getMember().getTarget()); + Shape rightTarget = model.expectShape(right.getMember().getTarget()); + if (!isReplacementValid(model, leftTarget, rightTarget)) { + throw new IllegalStateException(String.format( + "Cannot change the type of %s from %s to %s", + left.getMember().getId(), rightTarget.getType(), leftTarget.getType())); + } + } + private Model.Builder createReplacedModelBuilder(Model model, List shouldReplace) { // Add member shapes to the builder. This builder is mutated // by the visitor, which will ensure that newly added members diff --git a/smithy-model/src/test/java/software/amazon/smithy/model/transform/ReplaceShapesTest.java b/smithy-model/src/test/java/software/amazon/smithy/model/transform/ReplaceShapesTest.java index 0e93e46f344..dd3e34a80d1 100644 --- a/smithy-model/src/test/java/software/amazon/smithy/model/transform/ReplaceShapesTest.java +++ b/smithy-model/src/test/java/software/amazon/smithy/model/transform/ReplaceShapesTest.java @@ -16,20 +16,23 @@ package software.amazon.smithy.model.transform; import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.Arrays; import java.util.Collections; import java.util.Optional; import org.hamcrest.Matchers; -import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import software.amazon.smithy.model.Model; import software.amazon.smithy.model.SourceLocation; import software.amazon.smithy.model.shapes.IntegerShape; import software.amazon.smithy.model.shapes.ListShape; +import software.amazon.smithy.model.shapes.LongShape; import software.amazon.smithy.model.shapes.MapShape; import software.amazon.smithy.model.shapes.MemberShape; +import software.amazon.smithy.model.shapes.SetShape; import software.amazon.smithy.model.shapes.ShapeId; import software.amazon.smithy.model.shapes.StringShape; import software.amazon.smithy.model.shapes.StructureShape; @@ -41,14 +44,85 @@ public class ReplaceShapesTest { @Test - public void cannotChangeShapeTypes() { - Assertions.assertThrows(RuntimeException.class, () -> { + public void cannotChangeComplexShapeTypes() { + IllegalStateException ex = assertThrows(IllegalStateException.class, () -> { ShapeId shapeId = ShapeId.from("ns.foo#id1"); StringShape shape = StringShape.builder().id(shapeId).build(); - Model model = Model.builder().addShape(shape).build(); + Model model = Model.builder() + .addShape(shape) + .addShape(LongShape.builder().id("ns.foo#id2").build()) + .build(); ModelTransformer transformer = ModelTransformer.create(); - transformer.mapShapes(model, s -> IntegerShape.builder().id(shapeId).build()); + transformer.mapShapes(model, s -> { + if (s.getId().equals(shapeId)) { + return StructureShape.builder() + .id(shapeId) + .addMember( + MemberShape.builder() + .id(ShapeId.from("ns.foo#id1$member1")) + .target("ns.foo#id2") + .build() + ).build(); + } + return s; + }); }); + assertEquals("Cannot change the type of ns.foo#id1 from string to structure", ex.getMessage()); + } + + @Test + public void canChangeSimpleShapeTypes() { + ShapeId shapeId = ShapeId.from("ns.foo#id1"); + IntegerShape shape = IntegerShape.builder().id(shapeId).build(); + Model model = Model.builder().addShape(shape).build(); + ModelTransformer transformer = ModelTransformer.create(); + transformer.mapShapes(model, s -> LongShape.builder().id(shapeId).build()); + } + + @Test + public void canExchangeSimilarListCollectionTypes() { + ShapeId shapeId = ShapeId.from("ns.foo#id1"); + ListShape shape = ListShape.builder().id(shapeId).member(ShapeId.from("smithy.api#Long")).build(); + Model model = Model.builder() + .addShape(shape) + .addShape(LongShape.builder().id(ShapeId.from("smithy.api#Long")).build()) + .build(); + ModelTransformer transformer = ModelTransformer.create(); + transformer.mapShapes(model, s -> { + if (s.getId().equals(shapeId)) { + return SetShape.builder().id(shapeId).member(ShapeId.from("smithy.api#Long")).build(); + } + return s; + }); + } + + @Test + public void canNotExchangeListCollectionTypesWithNonReplaceableMembers() { + ShapeId shapeId = ShapeId.from("ns.foo#id1"); + ShapeId structId = ShapeId.from("ns.foo#id2"); + ListShape shape = ListShape.builder().id(shapeId).member(ShapeId.from("smithy.api#Long")).build(); + Model model = Model.builder() + .addShape(shape) + .addShape(LongShape.builder().id(ShapeId.from("smithy.api#Long")).build()) + .addShape(StructureShape.builder() + .id(structId) + .addMember( + MemberShape.builder() + .id(structId.withMember("member1")) + .target("smithy.api#Long") + .build() + ).build()) + .build(); + IllegalStateException ex = assertThrows(IllegalStateException.class, () -> { + ModelTransformer transformer = ModelTransformer.create(); + transformer.mapShapes(model, s -> { + if (s.getId().equals(shapeId)) { + return SetShape.builder().id(shapeId).member(structId).build(); + } + return s; + }); + }); + assertEquals("Cannot change the type of ns.foo#id1$member from long to structure", ex.getMessage()); } @Test