diff --git a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/RemovedShape.java b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/RemovedShape.java index c0abb8a9d1e..60693ae94b0 100644 --- a/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/RemovedShape.java +++ b/smithy-diff/src/main/java/software/amazon/smithy/diff/evaluators/RemovedShape.java @@ -19,11 +19,13 @@ import java.util.stream.Collectors; import software.amazon.smithy.diff.Differences; import software.amazon.smithy.model.shapes.Shape; +import software.amazon.smithy.model.traits.EnumTrait; import software.amazon.smithy.model.traits.PrivateTrait; import software.amazon.smithy.model.validation.ValidationEvent; /** - * Creates an ERROR event when a non-private shape is removed. + * Creates an ERROR event when a non-private non-scalar shape is removed. + * Creates a NOTE event when a non-private scalar shape is removed. */ public final class RemovedShape extends AbstractDiffEvaluator { @Override @@ -31,7 +33,9 @@ public List evaluate(Differences differences) { return differences.removedShapes() .filter(shape -> !shape.hasTrait(PrivateTrait.class)) .filter(shape -> !isMemberOfRemovedShape(shape, differences)) - .map(shape -> error(shape, String.format("Removed %s `%s`", shape.getType(), shape.getId()))) + .map(shape -> isScalarType(shape) + ? note(shape, String.format("Removed %s `%s`", shape.getType(), shape.getId())) + : error(shape, String.format("Removed %s `%s`", shape.getType(), shape.getId()))) .collect(Collectors.toList()); } @@ -40,4 +44,19 @@ private boolean isMemberOfRemovedShape(Shape shape, Differences differences) { .filter(member -> !differences.getNewModel().getShapeIds().contains(member.getContainer())) .isPresent(); } + + private boolean isScalarType(Shape shape) { + return shape.isIntegerShape() + || shape.isBigDecimalShape() + || shape.isBigIntegerShape() + || shape.isBlobShape() + || shape.isBooleanShape() + || shape.isByteShape() + || shape.isDoubleShape() + || shape.isFloatShape() + || shape.isShortShape() + || shape.isTimestampShape() + || shape.isLongShape() + || (shape.isStringShape() && !shape.hasTrait(EnumTrait.class)); + } } diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/ModelDiffTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/ModelDiffTest.java index cbc22a4169a..307dacbe98e 100644 --- a/smithy-diff/src/test/java/software/amazon/smithy/diff/ModelDiffTest.java +++ b/smithy-diff/src/test/java/software/amazon/smithy/diff/ModelDiffTest.java @@ -11,6 +11,7 @@ import software.amazon.smithy.model.Model; import software.amazon.smithy.model.node.Node; import software.amazon.smithy.model.shapes.StringShape; +import software.amazon.smithy.model.shapes.StructureShape; import software.amazon.smithy.model.traits.SensitiveTrait; import software.amazon.smithy.model.validation.Severity; import software.amazon.smithy.model.validation.ValidatedResult; @@ -77,7 +78,7 @@ public void testsEquality() { @Test public void findsBreakingChanges() { Model oldModel = Model.builder() - .addShape(StringShape.builder().id("smithy.example#Str").build()) + .addShape(StructureShape.builder().id("smithy.example#Str").build()) .build(); Model newModel = Model.builder().build(); ModelDiff.Result result = ModelDiff.builder().oldModel(oldModel).newModel(newModel).compare(); diff --git a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/RemovedShapeTest.java b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/RemovedShapeTest.java index 7cae1ff6349..7de7c260e75 100644 --- a/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/RemovedShapeTest.java +++ b/smithy-diff/src/test/java/software/amazon/smithy/diff/evaluators/RemovedShapeTest.java @@ -23,10 +23,24 @@ import org.junit.jupiter.api.Test; import software.amazon.smithy.diff.ModelDiff; import software.amazon.smithy.model.Model; +import software.amazon.smithy.model.shapes.BigDecimalShape; +import software.amazon.smithy.model.shapes.BigIntegerShape; +import software.amazon.smithy.model.shapes.BlobShape; +import software.amazon.smithy.model.shapes.BooleanShape; +import software.amazon.smithy.model.shapes.ByteShape; +import software.amazon.smithy.model.shapes.DoubleShape; +import software.amazon.smithy.model.shapes.FloatShape; +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.MemberShape; import software.amazon.smithy.model.shapes.Shape; +import software.amazon.smithy.model.shapes.ShortShape; import software.amazon.smithy.model.shapes.StringShape; +import software.amazon.smithy.model.shapes.StructureShape; +import software.amazon.smithy.model.shapes.TimestampShape; +import software.amazon.smithy.model.traits.EnumDefinition; +import software.amazon.smithy.model.traits.EnumTrait; import software.amazon.smithy.model.traits.PrivateTrait; import software.amazon.smithy.model.validation.Severity; import software.amazon.smithy.model.validation.ValidationEvent; @@ -34,8 +48,8 @@ public class RemovedShapeTest { @Test public void detectsShapeRemoval() { - Shape shapeA1 = StringShape.builder().id("foo.baz#Baz").build(); - Shape shapeB1 = StringShape.builder().id("foo.baz#Bam").build(); + Shape shapeA1 = StructureShape.builder().id("foo.baz#Baz").build(); + Shape shapeB1 = StructureShape.builder().id("foo.baz#Bam").build(); Model modelA = Model.assembler().addShapes(shapeA1, shapeB1).assemble().unwrap(); Model modelB = Model.assembler().addShapes(shapeB1).assemble().unwrap(); List events = ModelDiff.compare(modelA, modelB); @@ -45,6 +59,46 @@ public void detectsShapeRemoval() { assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(1)); } + @Test + public void emitsNotesForScalarShapes() { + Shape[] scalarShapes = new Shape[] { + IntegerShape.builder().id("foo.baz#BazOne").build(), + BigDecimalShape.builder().id("foo.baz#BazTwo").build(), + BigIntegerShape.builder().id("foo.baz#BazThree").build(), + BlobShape.builder().id("foo.baz#BazFour").build(), + BooleanShape.builder().id("foo.baz#BazFive").build(), + ByteShape.builder().id("foo.baz#BazSix").build(), + DoubleShape.builder().id("foo.baz#BazSeven").build(), + FloatShape.builder().id("foo.baz#BazEight").build(), + ShortShape.builder().id("foo.baz#BazNine").build(), + TimestampShape.builder().id("foo.baz#BazTen").build(), + LongShape.builder().id("foo.baz#BazEleven").build(), + StringShape.builder().id("foo.baz#BazTwelve").build() + }; + Model modelA = Model.assembler().addShapes(scalarShapes).assemble().unwrap(); + Model modelB = Model.assembler().assemble().unwrap(); + List events = ModelDiff.compare(modelA, modelB); + + assertThat(TestHelper.findEvents(events, "RemovedShape").size(), equalTo(12)); + assertThat("Scalar removals should be NOTE severity", + events.stream().allMatch(event -> Severity.NOTE.equals(event.getSeverity()))); + } + + @Test + public void emitsErrorForEnumString() { + Shape shapeA1 = StringShape.builder() + .id("foo.baz#Baz") + .addTrait(EnumTrait.builder().addEnum(EnumDefinition.builder().value("val").build()).build()) + .build(); + Model modelA = Model.assembler().addShapes(shapeA1).assemble().unwrap(); + Model modelB = Model.assembler().addShapes().assemble().unwrap(); + List events = ModelDiff.compare(modelA, modelB); + + assertThat(TestHelper.findEvents(events, "RemovedShape").size(), equalTo(1)); + assertThat(TestHelper.findEvents(events, shapeA1.getId()).size(), equalTo(1)); + assertThat(TestHelper.findEvents(events, Severity.ERROR).size(), equalTo(1)); + } + @Test public void doesNotEmitForPrivateShapes() { Shape shape = StringShape.builder().id("foo.baz#Baz").addTrait(new PrivateTrait()).build();